feat(accuracysnes): add A6.15 — all 256 opcodes defined, only STP hangs - #331
Conversation
…angs Coverage 360 -> 361 of 443. The row executes each of the 241 straight-line 65C816 opcodes in a WRAM sandbox and counts three outcomes against the length Table 5-4 of the WDC W65C816S datasheet documents: returned where it should, returned late, or did not return. The expectation comes from the datasheet via a new cpu_opcodes.rs, built from the matrix's own columnar rules with a gate that fails the build if they do not tile it -- which caught XBA ($EB) missing on the first run. The terminator cannot be a return. TXS and TCS move the stack pointer, and with x=1 a TXS puts it in page zero, so the return address is no longer where an RTS would pop it from. Control comes back through a JMP, and because a JMP is three bytes the exit addresses are chosen so its OWN operand bytes are harmless one-byte instructions: $AAAA (TAX) clean, $B8B8 (CLV) overshoot. An opcode that consumes one byte too many therefore executes a register transfer and walks into a NOP fill rather than executing half an address. ($7E:EAEA was the first choice and is wrong -- it is inside the space runtime.inc reserves for widening the measurement channel.) The watchdog takes TWO strikes. NMI fires once per vblank and across 241 sandbox runs will eventually land inside a healthy one, so one hit cannot mean stuck. Four opcodes are dangerous even when correct and the preamble handles each rather than excluding it: MVN/MVP move A+1 bytes so A=0; XCE flips to emulation mode only if C is set so CLC first makes it a no-op; TXS/TCS are why the exits restore SP from WRAM; SED and PLP are why they re-establish m, x, d and c. Verified by injection twice. WDM made three bytes -> exactly one LATE, first-bad $42. TRB dp made to jam -> exactly one NO-RETURN, first-bad $14, watchdog rescuing the battery. Picking that second injection is not free: SED and CLC both hang the cart BEFORE A6.15 runs, because the runtime and earlier Group A rows execute them. Also fixes the bug that made this look like a reference defect. The NMI handler runs with the sandbox's DBR ($7E), so `lda $4210` read a WRAM byte and RDNMI was never acknowledged. Three hosts never landed an NMI where it showed; ares did, and the row reported PLA ($68) overshooting on ares alone. It was ours, not ares'. Long addressing fixes it, and the handler now preserves A as well -- RTI restores P and PC but not the accumulator. Battery 311/311. Three references agree; ARES_KNOWN_FAILURES stays 3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughAdded a complete 65C816 opcode metadata table and the AccuracySNES ChangesA6.15 opcode validation
Reference-rendering discrepancy documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant A615 as A6.15 test
participant Table as Opcode table
participant Sandbox as WRAM sandbox
participant Watchdog as NMI watchdog
participant Report as Result handlers
A615->>Table: Read opcode and documented length
A615->>Sandbox: Build safe opcode sandbox
A615->>Sandbox: Execute opcode
Sandbox->>Report: Return at clean or overshoot terminator
Watchdog->>Report: Redirect after second NMI hit
Report->>A615: Record outcome and continue sweep
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends the AccuracySNES cartridge’s CPU coverage by adding a new scored row (A6.15) that executes each straight-line 65C816 opcode in a WRAM sandbox to validate that all opcodes are defined and that only STP is a true “hang” instruction. It introduces a generator-side, datasheet-derived opcode matrix (cpu_opcodes.rs) to keep the row’s expectations independent of rustysnes-cpu, and wires the new row through the generator outputs, harness reporting, and docs/coverage accounting.
Changes:
- Add
A6.15test generation (cpu_defined.rs) and integrate it into the generated test list / dossier mapping. - Add generator-side 65C816 opcode table (
cpu_opcodes.rs) derived from WDC datasheet rules, with invariants enforced by unit tests. - Update generated cartridge artifacts + harness/docs/changelog to include the new row and updated assertion coverage totals.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/roms/AccuracySNES/gen/src/tests/mod.rs | Registers the new cpu_defined module and includes A6.15 in the generated test list. |
| tests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs | Implements the A6.15 WRAM sandbox sweep + watchdog + measurement recording. |
| tests/roms/AccuracySNES/gen/src/main.rs | Adds cpu_opcodes module to the generator crate. |
| tests/roms/AccuracySNES/gen/src/dossier.rs | Adds dossier mapping for A6.15. |
| tests/roms/AccuracySNES/gen/src/cpu_opcodes.rs | New datasheet-based opcode matrix (mode/length/flow), used as the expectation source for A6.15. |
| tests/roms/AccuracySNES/asm/tests_group_a.s | Generated assembly output updated to include the new test and its tables/stubs. |
| tests/roms/AccuracySNES/SOURCE_CATALOG.tsv | Catalog entry added for A6.15 and offsets shifted accordingly (generated output). |
| tests/roms/AccuracySNES/ERROR_CODES.md | Adds the new row’s failure-code documentation (generated output). |
| crates/rustysnes-test-harness/tests/accuracysnes.rs | Adds slot labels for A6.15 measurement outputs (283–286). |
| docs/accuracysnes-plan.md | Updates reported assertion coverage totals (360 → 361). |
| docs/accuracysnes-coverage.md | Updates on-cart covered assertion count (303 → 304) and total coverage (360 → 361). |
| CHANGELOG.md | Documents the new row and the watchdog fix; needs a small consistency tweak for the prior E2.10 coverage number. |
| Mode::Implied => {} | ||
| // A direct-page offset into the seeded window page. `$10`/`$12` hold the indirect pointers. | ||
| Mode::Direct | Mode::StackRelative => out[1] = 0x10, | ||
| // An immediate value of zero, and — for a branch — a zero DISPLACEMENT, which is what makes |
| With the fix the injections are exact where they had been approximate: the jam injection reported | ||
| two stuck opcodes and 63 clean ones before, and reports one and 240 now. | ||
|
|
||
| - **`E2.10` — the full 256-opcode SPC700 cycle sweep, and the coverage number moves to 360 of 443.** |
| /// Nine opcodes are four bytes: the eight ALU long forms' two columns less the ones that are | ||
| /// jumps, plus `JSL` and `JML`. Counting them is a cheap check that the long columns tiled. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@tests/roms/AccuracySNES/gen/src/cpu_opcodes.rs`:
- Around line 24-27: Update the module documentation near the 8-bit-accumulator
explanation to identify tests/cpu_defined.rs, specifically its run_sandbox
function, as the sandbox that establishes sep #$30; remove the incorrect
sweep.rs reference while preserving the surrounding historical context.
- Around line 463-473: Update the rustdoc for
the_long_modes_are_where_they_should_be to state that eighteen opcodes are four
bytes, matching the existing inline comment and assert_eq! expectation; leave
the test logic unchanged.
In `@tests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs`:
- Around line 640-647: Update the A6.15 row title and the module heading to
state that the sweep executes 241 opcodes, not all 256. Explicitly identify the
15 Flow::Leaves opcodes as out of scope because their zero-length tables cause
them to be skipped, including STP and the eight jumps/calls, three returns, BRK,
COP, and WAI.
- Around line 393-396: Add a unit test in the existing tests module that emits
an exit stub through emit_exit_stubs into a scratch Asm, counts its emitted
.byte operands, and asserts the count equals STUB_LEN. In the same test, assert
the byte at JML_TARGET_AT - 1 is $5C, preserving the JML opcode alignment and
validating both constants against the generated stub.
- Around line 583-591: Update the FIRST_BAD sentinel flow: initialize FIRST_BAD
to $00 instead of $FF, adjust note_bad so it treats zero as “none” while storing
the first failing opcode as opcode + 1, and relabel slot 286 to document the
encoded value and zero sentinel. Preserve the existing first-failure behavior
and verdict checks.
- Around line 627-631: In the generated sequence around the `OVER` and `STUCK`
additions, narrow the accumulator to 8-bit mode before `adc f:${var::STUCK}` so
the instruction reads only the `STUCK` byte. Preserve the existing low-byte
masking and ensure the addition still produces the intended `OVER + STUCK`
result without depending on `FIRST_BAD` or an afterward-discarded high byte.
- Around line 195-196: Update the operand generation around Mode::Direct and
Mode::StackRelative so long-indirect cases use the operand that reads the
contiguous pointer at D+$10–D+$12, with the bank byte immediately following the
16-bit address. Adjust run_sandbox seeding so those three bytes form the window
address in bank $7E, and add an assertion validating this invariant. Handle
StackRelative separately with an S-relative operand and correct the misleading
comment.
- Around line 343-348: Update the stub-generation loop to iterate over names
only, since target is discarded there and patched later in the separate
target-patching logic. Also revise the rustdoc describing the stub sequence to
include the emitted REP #$30 between CLC and restoring the saved state, without
removing instructions or changing STUB_LEN.
🪄 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: e9b6ee9b-c44d-4457-aac6-03c03fb4c086
⛔ Files ignored due to path filters (6)
docs/accuracysnes-coverage.mdis excluded by!docs/accuracysnes-coverage.mdand included bydocs/**tests/roms/AccuracySNES/ERROR_CODES.mdis excluded by!tests/roms/AccuracySNES/ERROR_CODES.mdand included bytests/**tests/roms/AccuracySNES/SOURCE_CATALOG.tsvis excluded by!**/*.tsv,!tests/roms/AccuracySNES/SOURCE_CATALOG.tsvand included bytests/**tests/roms/AccuracySNES/asm/tests_group_a.sis excluded by!tests/roms/AccuracySNES/asm/tests_group_a.sand included bytests/**tests/roms/AccuracySNES/build/accuracysnes-pal.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**tests/roms/AccuracySNES/build/accuracysnes.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**
📒 Files selected for processing (8)
CHANGELOG.mdcrates/rustysnes-test-harness/tests/accuracysnes.rsdocs/accuracysnes-plan.mdtests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rstests/roms/AccuracySNES/gen/src/tests/mod.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: lint
- GitHub Check: accuracysnes
- GitHub Check: test-light
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: build demo + docs
- GitHub Check: review
🧰 Additional context used
📓 Path-based instructions (15)
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-plan.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-plan.mdtests/roms/AccuracySNES/gen/src/tests/mod.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/main.rscrates/rustysnes-test-harness/tests/accuracysnes.rsCHANGELOG.mdtests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.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-plan.mdtests/roms/AccuracySNES/gen/src/tests/mod.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/main.rscrates/rustysnes-test-harness/tests/accuracysnes.rsCHANGELOG.mdtests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.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-plan.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-plan.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-plan.mdCHANGELOG.md
**/*.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:
tests/roms/AccuracySNES/gen/src/tests/mod.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/main.rscrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
tests/roms/AccuracySNES/gen/src/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, when a test or scene is added or removed under
tests/roms/AccuracySNES/gen/src/, verify itsdossier.rs::MAPentry, all required regenerated artifacts, and matching count changes indocs/accuracysnes-plan.md. Artifact presence must be judged from the path-filter exclusion list, not from unreadable contents.
Files:
tests/roms/AccuracySNES/gen/src/tests/mod.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.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:
tests/roms/AccuracySNES/gen/src/tests/mod.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/main.rscrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
tests/roms/AccuracySNES/gen/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Rebuild AccuracySNES after any change to
gen/orasm/; never hand-edit generatedasm/tests_group_a.sorasm/scenes.s.
Files:
tests/roms/AccuracySNES/gen/src/tests/mod.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
tests/roms/AccuracySNES/gen/src/**
⚙️ CodeRabbit configuration file
tests/roms/AccuracySNES/gen/src/**: This generates a hardware-accuracy test cartridge. Judge each test by whether it can
distinguish the behavior it names from the alternatives, not by whether it passes.Flag, specifically:
- Vacuity. An assertion whose expected value is also what a broken or absent
implementation produces (zero, "unchanged", "not $FF") needs a paired control assertion
that would fail on that implementation. Say which alternative goes uncaught.- Overstated doc comments. The prose above a test is a claim about what it validates.
If it names behaviors the emitted program does not exercise, or asserts a rationale that
is not true of the code, that is a defect even though the test passes.- Shared state. OAM, CGRAM, VRAM and the S-DSP registers are not reset between tests. A
test that does not establish its own starting conditions may be measuring the previous
one; look for an earlier test that leaves the relevant state dirty.- Timing-marginal reads. Reading a register a few cycles after disturbing it, or
asserting on a value that is still moving, produces a verdict that flips when unrelated
code shifts. Prefer a settle, a disarm, or a provably stationary value.- Scanline geometry. Line 0 is a blanking line; the V counter's low byte aliases on a
312-line PAL frame; the visible height is 224 or 239 depending on overscan. Constants
derived from any of these deserve a second look.- Duplicate coverage.
dossier.rs::MAPmust not claim an assertion another test already
implements. There is a build gate for this, but flag it in review too.
Files:
tests/roms/AccuracySNES/gen/src/tests/mod.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
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-test-harness/tests/accuracysnes.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-test-harness/tests/accuracysnes.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-test-harness/tests/accuracysnes.rs
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.For the full pull request diff against its base branch, modify
CHANGELOG.mdwhen user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.
Files:
CHANGELOG.md
🧠 Learnings (5)
📚 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-plan.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-plan.md
📚 Learning: 2026-07-21T02:10:49.581Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 190
File: tests/roms/AccuracySNES/gen/src/tests/ppu.rs:0-0
Timestamp: 2026-07-21T02:10:49.581Z
Learning: For any AccuracySNES tests that perform a runtime measurement investigation using writes to $2137 and $4201, do not reuse measurement/slot indices that may already be owned by another test. Before using a slot, verify it is unused (e.g., via an on-cart probe/readback that confirms the slot contains no prior test result). Then independently record $213F immediately before and immediately after each $2137/$4201 operation, so the test can attribute changes to its own operation and avoid cross-test interference.
Applied to files:
tests/roms/AccuracySNES/gen/src/tests/mod.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
📚 Learning: 2026-07-21T06:21:34.629Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 198
File: docs/accuracysnes-plan.md:0-0
Timestamp: 2026-07-21T06:21:34.629Z
Learning: When reviewing slot allocation for SNES/ROM measurement channels in generator-driven Rust code, account for slots assigned via generation-time computed writers (e.g., slots derived from formulas like `slot_base = 8 + index * 2`) rather than only literal `record(...)` calls. Trace the computed writer’s full emitted slot range and verify there are no collisions with other opcodes/channels that may use different slot ranges after earlier conflict resolution.
Applied to files:
tests/roms/AccuracySNES/gen/src/tests/mod.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
📚 Learning: 2026-07-22T06:12:00.703Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 201
File: tests/roms/AccuracySNES/gen/src/tests/apu.rs:1508-1538
Timestamp: 2026-07-22T06:12:00.703Z
Learning: When working on APU/voice timing in these AccuracySNES test sources, treat NTSC vs PAL differences as a first-class constraint. In particular, do not change `Voice::settle` (or any settling/code-size/timing logic it affects) unless you cross-validate against both regions (NTSC and PAL), because timing/polling phase shifts can cause regressions that may appear as PAL-only test failures. For ROM-specific expectations like `E7.13`, keep the intentionally chosen ENVX range (e.g., `0x68..=0x7C`) unless you re-validate that the absorbed post-KON timing variation still matches across both regions.
Applied to files:
tests/roms/AccuracySNES/gen/src/tests/mod.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
🔇 Additional comments (14)
tests/roms/AccuracySNES/gen/src/dossier.rs (1)
105-105: LGTM!crates/rustysnes-test-harness/tests/accuracysnes.rs (1)
602-602: LGTM!Also applies to: 632-638
docs/accuracysnes-plan.md (1)
17-17: LGTM!CHANGELOG.md (1)
14-40: LGTM!Also applies to: 42-60
tests/roms/AccuracySNES/gen/src/cpu_opcodes.rs (1)
29-98: LGTM!Also applies to: 100-296, 298-330, 332-388, 390-461
tests/roms/AccuracySNES/gen/src/main.rs (1)
14-14: LGTM!tests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs (7)
226-296: LGTM!
299-330: LGTM!
402-467: LGTM!
481-516: LGTM!Also applies to: 522-581
76-145: LGTM!Also applies to: 151-182, 650-749
356-361: 🩺 Stability & AvailabilityKeep the
$00bank byte.a6_15and its targets are linked inTESTSwithin bank$00for all AccuracySNES configurations. The proposed patch is unnecessary.> Likely an incorrect or invalid review comment.
599-611: 🗄️ Data Integrity & IntegrationRemove this slot-collision finding. Slots 283–286 are uniquely owned by the A6.15 test, mapped in the harness, and covered by
dossier::check_slots; no other writer reaches this range.> Likely an incorrect or invalid review comment.tests/roms/AccuracySNES/gen/src/tests/mod.rs (1)
15-15: LGTM!Also applies to: 28-28
| // A direct-page offset into the seeded window page. `$10`/`$12` hold the indirect pointers. | ||
| Mode::Direct | Mode::StackRelative => out[1] = 0x10, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The long-indirect opcodes read a pointer whose bank byte is $00, and the seeded 24-bit pointer at D+$12 is never used.
Mode::Direct covers [dp] ($07 column) and [dp],Y ($17 column) as well as the plain and 16-bit-indirect forms, and this arm gives all of them operand $10. run_sandbox at lines 474-479 seeds $0210/$0211 with $5000, $0212/$0213 with $5000, and $0214 with $7E.
A long-indirect opcode with operand $10 therefore fetches three bytes from $0210, $0211, $0212, which are $00, $50, $00. The effective address is $00:5000, not $7E:5000. The 24-bit pointer the seeding builds at $0212-$0214 is unreachable, because no emitted operand byte is ever $12. This affects the sixteen [dp] and [dp],Y opcodes across the eight ALU bases, including the two stores.
The comment on line 195 and the comment at lines 471-472 both assert that an indirect access cannot leave the window. That rationale does not hold for those sixteen opcodes. no_operand_reaches_the_sandbox_or_the_runtime_variables does not catch it either: it only checks b[1] >= 0x10.
Make one pointer serve both widths by placing the bank byte directly after the 16-bit pointer.
Separately, Mode::StackRelative is folded into this arm and given the same $10. Stack-relative operands are offsets from S, not from D, so the comment's description of them as "a direct-page offset into the seeded window page" is wrong.
Proposed fix to the pointer seeding
- a.c("Seed the operand window: the indirect pointers at D+$10 (16-bit) and D+$12 (24-bit) both");
- a.c("point back into the window, so an indirect load cannot reach MMIO or the sandbox.");
+ a.c("Seed the operand window. One pointer at D+$10 serves both widths: the two address bytes");
+ a.c("are read by (dp) and (dp),Y, and [dp] and [dp],Y read the bank byte that follows them.");
a.l("rep #$30");
a.l(&format!("lda #${WINDOW:04X}"));
a.l(&format!("sta f:$7E{:04X}", SANDBOX_DP + 0x10));
- a.l(&format!("sta f:$7E{:04X}", SANDBOX_DP + 0x12));
a.l("sep #$20");
a.l("lda #$7E");
- a.l(&format!("sta f:$7E{:04X}", SANDBOX_DP + 0x14));
+ a.l(&format!("sta f:$7E{:04X}", SANDBOX_DP + 0x12));Add a gate that asserts the seeded bytes at D+$10, D+$11 and D+$12 form the window address in bank $7E, so a future change to the seeding cannot silently reintroduce the $00 bank.
🤖 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 `@tests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs` around lines 195 - 196,
Update the operand generation around Mode::Direct and Mode::StackRelative so
long-indirect cases use the operand that reads the contiguous pointer at
D+$10–D+$12, with the bank byte immediately following the 16-bit address. Adjust
run_sandbox seeding so those three bytes form the window address in bank $7E,
and add an assertion validating this invariant. Handle StackRelative separately
with an S-relative operand and correct the misleading comment.
Source: Path instructions
| /// Bytes in one exit stub. Pinned as a constant because the copy loop counts them, and asserted | ||
| /// against the emitted table by a unit test — a miscount would copy a truncated stub into WRAM and | ||
| /// the first opcode to reach it would run off the end. | ||
| const STUB_LEN: u16 = 19; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
STUB_LEN's doc claims a unit test that does not exist.
Lines 393-395 state that STUB_LEN is "asserted against the emitted table by a unit test", and name the consequence of a miscount: the copy loop at lines 365-374 would write a truncated stub into WRAM.
The tests module at lines 650-749 contains five tests. None counts the bytes emit_exit_stubs emits. STUB_LEN = 19 is correct today — 2 + 1 + 1 + 2 + 4 + 1 + 3 + 1 + 4 — but nothing holds it correct, and JML_TARGET_AT = 16 is unguarded in the same way.
Add the gate the doc already promises. Emit the stub into a scratch Asm, count the .byte operands, and compare against STUB_LEN. Assert that the byte at offset JML_TARGET_AT - 1 is $5C, which pins the patch offset to the JML opcode.
🤖 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 `@tests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs` around lines 393 - 396,
Add a unit test in the existing tests module that emits an exit stub through
emit_exit_stubs into a scratch Asm, counts its emitted .byte operands, and
asserts the count equals STUB_LEN. In the same test, assert the byte at
JML_TARGET_AT - 1 is $5C, preserving the JML opcode alignment and validating
both constants against the generated stub.
Source: Path instructions
| a.finish( | ||
| "A6.15", | ||
| 'A', | ||
| "all 256 opcodes defined", | ||
| Provenance::Documented("WDC W65C816S datasheet, Table 5-4 opcode matrix"), | ||
| Kind::Scored, | ||
| None, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The row title claims 256 opcodes; the sweep exercises 241.
finish names the row "all 256 opcodes defined", and the module title at line 1 states "every 65C816 opcode is defined, and only STP hangs".
The sweep executes 241 opcodes. The fifteen Flow::Leaves opcodes get a table length of zero and are skipped at line 270. STP is one of them, so the emitted program never runs it and asserts nothing about whether it hangs. The eight jumps and calls, the three returns, BRK, COP and WAI are likewise never executed, so the row makes no claim about their definedness either.
Restate the title and the module heading to match the 241 opcodes the program executes, and say which fifteen are out of scope and why.
🤖 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 `@tests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs` around lines 640 - 647,
Update the A6.15 row title and the module heading to state that the sweep
executes 241 opcodes, not all 256. Explicitly identify the 15 Flow::Leaves
opcodes as out of scope because their zero-length tables cause them to be
skipped, including STP and the eight jumps/calls, three returns, BRK, COP, and
WAI.
Source: Path instructions
Nine findings, all real. Two were functional and both are operand-safety violations the row's own docs claimed did not exist. - STACK-RELATIVE operands reached MMIO. The cart's stack sits at $1FFF, so `ORA $10,S` addressed $00:200F and a PLA read $00:2000 -- both outside WRAM, in the unmapped/MMIO region. The preamble now moves SP into page 1 for the run; the exits already restored the cart's own from SAVED_SP. - The LONG indirects read a pointer with a bank byte of $00. `[dp]` and `[dp],Y` take all three bytes at the direct-page pointer, and the first draft seeded two separate pointers, giving the long one bank $00 -- so it reached $00:5000, unmapped, rather than $7E:5000. There is now ONE pointer at D+$10, seeded 24-bit; the 16-bit indirects read its first two bytes and take the bank from DBR, which the preamble sets to $7E. - $FF was the "no failure" sentinel AND a real opcode: $FF is SBC long,X, which the row executes, so a genuine failure there would have been reported as `none`. ($DB is STP, not $FF -- I had that wrong in the first version of the test that was supposed to prove the sentinel safe, and the test caught me.) The sentinel is now $00 = BRK, which is in the not-executed set and therefore can never be a real answer. A unit test pins both halves of that. - STUB_LEN's doc claimed a unit test that did not exist. It exists now, and it counts the bytes the table actually emits. - cpu_opcodes.rs named sweep.rs as the sandbox that establishes sep #$30; the consumer of that table is tests/cpu_defined.rs. - The four-byte-opcode test's doc said nine where the assertion says eighteen. - emit_exit_stubs bound `target` and discarded it, and its doc omitted the REP #$30 the stub emits. - The CHANGELOG's E2.10 entry restated a coverage total that A6.15's entry then superseded in the same cumulative section. Re-verified after the SP and pointer changes, which move the image: battery 311/311, three references agree, 55 scenes match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs`:
- Around line 347-349: Correct the instruction name in the stub documentation
comment by replacing TXS with TCS, matching the emitted instruction and the
accumulator-based stack restoration behavior.
🪄 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: f09eb62c-ba42-4f21-a937-3dbdb904f8cc
⛔ Files ignored due to path filters (4)
tests/roms/AccuracySNES/ERROR_CODES.mdis excluded by!tests/roms/AccuracySNES/ERROR_CODES.mdand included bytests/**tests/roms/AccuracySNES/asm/tests_group_a.sis excluded by!tests/roms/AccuracySNES/asm/tests_group_a.sand included bytests/**tests/roms/AccuracySNES/build/accuracysnes-pal.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**tests/roms/AccuracySNES/build/accuracysnes.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**
📒 Files selected for processing (5)
CHANGELOG.mdcrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/dsl.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: lint
- GitHub Check: accuracysnes
- GitHub Check: test-light
- GitHub Check: build demo + docs
- GitHub Check: review
🧰 Additional context used
📓 Path-based instructions (12)
**/*.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:
tests/roms/AccuracySNES/gen/src/dsl.rscrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
**/*.{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:
tests/roms/AccuracySNES/gen/src/dsl.rscrates/rustysnes-test-harness/tests/accuracysnes.rsCHANGELOG.mdtests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.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:
tests/roms/AccuracySNES/gen/src/dsl.rscrates/rustysnes-test-harness/tests/accuracysnes.rsCHANGELOG.mdtests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
tests/roms/AccuracySNES/gen/src/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, when a test or scene is added or removed under
tests/roms/AccuracySNES/gen/src/, verify itsdossier.rs::MAPentry, all required regenerated artifacts, and matching count changes indocs/accuracysnes-plan.md. Artifact presence must be judged from the path-filter exclusion list, not from unreadable contents.
Files:
tests/roms/AccuracySNES/gen/src/dsl.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.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:
tests/roms/AccuracySNES/gen/src/dsl.rscrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
tests/roms/AccuracySNES/gen/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Rebuild AccuracySNES after any change to
gen/orasm/; never hand-edit generatedasm/tests_group_a.sorasm/scenes.s.
Files:
tests/roms/AccuracySNES/gen/src/dsl.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
tests/roms/AccuracySNES/gen/src/**
⚙️ CodeRabbit configuration file
tests/roms/AccuracySNES/gen/src/**: This generates a hardware-accuracy test cartridge. Judge each test by whether it can
distinguish the behavior it names from the alternatives, not by whether it passes.Flag, specifically:
- Vacuity. An assertion whose expected value is also what a broken or absent
implementation produces (zero, "unchanged", "not $FF") needs a paired control assertion
that would fail on that implementation. Say which alternative goes uncaught.- Overstated doc comments. The prose above a test is a claim about what it validates.
If it names behaviors the emitted program does not exercise, or asserts a rationale that
is not true of the code, that is a defect even though the test passes.- Shared state. OAM, CGRAM, VRAM and the S-DSP registers are not reset between tests. A
test that does not establish its own starting conditions may be measuring the previous
one; look for an earlier test that leaves the relevant state dirty.- Timing-marginal reads. Reading a register a few cycles after disturbing it, or
asserting on a value that is still moving, produces a verdict that flips when unrelated
code shifts. Prefer a settle, a disarm, or a provably stationary value.- Scanline geometry. Line 0 is a blanking line; the V counter's low byte aliases on a
312-line PAL frame; the visible height is 224 or 239 depending on overscan. Constants
derived from any of these deserve a second look.- Duplicate coverage.
dossier.rs::MAPmust not claim an assertion another test already
implements. There is a build gate for this, but flag it in review too.
Files:
tests/roms/AccuracySNES/gen/src/dsl.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
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-test-harness/tests/accuracysnes.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-test-harness/tests/accuracysnes.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-test-harness/tests/accuracysnes.rs
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.For the full pull request diff against its base branch, modify
CHANGELOG.mdwhen user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.
Files:
CHANGELOG.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:
CHANGELOG.md
🧠 Learnings (3)
📚 Learning: 2026-07-21T02:10:49.581Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 190
File: tests/roms/AccuracySNES/gen/src/tests/ppu.rs:0-0
Timestamp: 2026-07-21T02:10:49.581Z
Learning: For any AccuracySNES tests that perform a runtime measurement investigation using writes to $2137 and $4201, do not reuse measurement/slot indices that may already be owned by another test. Before using a slot, verify it is unused (e.g., via an on-cart probe/readback that confirms the slot contains no prior test result). Then independently record $213F immediately before and immediately after each $2137/$4201 operation, so the test can attribute changes to its own operation and avoid cross-test interference.
Applied to files:
tests/roms/AccuracySNES/gen/src/dsl.rstests/roms/AccuracySNES/gen/src/cpu_opcodes.rstests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
📚 Learning: 2026-07-21T06:21:34.629Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 198
File: docs/accuracysnes-plan.md:0-0
Timestamp: 2026-07-21T06:21:34.629Z
Learning: When reviewing slot allocation for SNES/ROM measurement channels in generator-driven Rust code, account for slots assigned via generation-time computed writers (e.g., slots derived from formulas like `slot_base = 8 + index * 2`) rather than only literal `record(...)` calls. Trace the computed writer’s full emitted slot range and verify there are no collisions with other opcodes/channels that may use different slot ranges after earlier conflict resolution.
Applied to files:
tests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
📚 Learning: 2026-07-22T06:12:00.703Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 201
File: tests/roms/AccuracySNES/gen/src/tests/apu.rs:1508-1538
Timestamp: 2026-07-22T06:12:00.703Z
Learning: When working on APU/voice timing in these AccuracySNES test sources, treat NTSC vs PAL differences as a first-class constraint. In particular, do not change `Voice::settle` (or any settling/code-size/timing logic it affects) unless you cross-validate against both regions (NTSC and PAL), because timing/polling phase shifts can cause regressions that may appear as PAL-only test failures. For ROM-specific expectations like `E7.13`, keep the intentionally chosen ENVX range (e.g., `0x68..=0x7C`) unless you re-validate that the absorbed post-KON timing variation still matches across both regions.
Applied to files:
tests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs
🔇 Additional comments (6)
crates/rustysnes-test-harness/tests/accuracysnes.rs (1)
602-602: LGTM!Also applies to: 632-638
CHANGELOG.md (1)
14-40: LGTM!Also applies to: 42-60, 61-61
tests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs (2)
709-727: PinJML_TARGET_ATagainst the emitted stub.This test verifies
STUB_LEN, but it does not verify that byte offsetJML_TARGET_AT - 1still contains$5C. A future edit can moveJMLwhile preserving the 19-byte length. The target patch would then overwrite unrelated stub bytes at runtime. Assert theJMLopcode position for both emitted stubs.
97-100: LGTM!Also applies to: 249-262, 351-394, 400-402, 478-530, 602-606, 628-631, 673-677, 729-746
tests/roms/AccuracySNES/gen/src/cpu_opcodes.rs (1)
25-27: LGTM!Also applies to: 463-465
tests/roms/AccuracySNES/gen/src/dsl.rs (1)
196-205: LGTM!
| /// `TXS`: `SEP #$30`, `CLD`, `CLC`, `REP #$30`, put the saved stack pointer back, put the direct | ||
| /// page back, then `JML` into the driver. `DBR` is left alone — the stub runs in bank `$7E` so `PHK`/`PLB` | ||
| /// there would set the wrong one; the driver does it on arrival instead. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the instruction name in the stub documentation.
Line 347 names TXS, but line 364 emits TCS ($1B). TXS would restore the stack pointer from X. This stub restores it from A. Replace TXS with TCS.
Proposed fix
-/// `TXS`: `SEP #$30`, `CLD`, `CLC`, `REP #$30`, put the saved stack pointer back, put the direct
+/// `TCS`: `SEP #$30`, `CLD`, `CLC`, `REP #$30`, put the saved stack pointer back, put the direct📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// `TXS`: `SEP #$30`, `CLD`, `CLC`, `REP #$30`, put the saved stack pointer back, put the direct | |
| /// page back, then `JML` into the driver. `DBR` is left alone — the stub runs in bank `$7E` so `PHK`/`PLB` | |
| /// there would set the wrong one; the driver does it on arrival instead. | |
| /// `TCS`: `SEP #$30`, `CLD`, `CLC`, `REP #$30`, put the saved stack pointer back, put the direct | |
| /// page back, then `JML` into the driver. `DBR` is left alone — the stub runs in bank `$7E` so `PHK`/`PLB` | |
| /// there would set the wrong one; the driver does it on arrival instead. |
🤖 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 `@tests/roms/AccuracySNES/gen/src/tests/cpu_defined.rs` around lines 347 - 349,
Correct the instruction name in the stub documentation comment by replacing TXS
with TCS, matching the emitted instruction and the accumulator-based stack
restoration behavior.
… not our gap It was recorded as "the last per-dot framebuffer gap" -- a Phase 4d item, to be closed by tracking the CGRAM redirect target in the fetch stage per layer, then re-blessing. Reading the third reference overturns that. The recorded account of MesenCE was incomplete. It was described as updating InternalCgramAddress only inside GetRgbColor, "so a transparent pixel leaves the previous opaque column's value standing". It does not: SnesPpu::RenderBgColor() runs AFTER every layer render in a span and sets InternalCgramAddress = 0 for each backdrop column it fills. What survives a span is therefore decided by pass ordering across the whole span -- an artefact of a span-based renderer, not a per-dot physical process. ares settles it. PPU::DAC::paletteColor sets latch.cgramAddress = palette UNCONDITIONALLY, called during per-dot priority resolution in the DAC, including paletteColor(0) for the transparent case (dac.cpp:71). That is RustySNES's model exactly: one assignment per dot, from the composited pixel, backdrop giving zero. So RustySNES and ares implement the same per-dot model and MesenCE differs for architectural reasons; the 7fc6 in the golden encodes MesenCE's renderer rather than hardware. The engine is NOT changed to match a pass-ordering artefact, and the "when 4d lands, remove the entry and re-bless" instruction is withdrawn. Two corroborations. C3.12 -- the scored cart row that asserts the redirect target directly -- passes on BOTH Mesen2 and RustySNES, so the two agree on the assertion and differ only on this homebrew framebuffer hash. And C3.12's own provenance is a per-dot statement, "a CGRAM access during active display uses the colour the PPU IS DRAWING", which the per-dot model implements directly and a span-ordering model only approximates. Same shape as the Mode-5 first-pixel claim, with the lesson from that retraction applied: the third reference was read BEFORE publishing, not after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)Adds AccuracySNES test Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
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-test-harness/tests/undisbeliever_golden.rs`:
- Around line 41-50: Align the PPU documentation with the final
per-dot/span-renderer model rather than stating that transparent pixels retain
the previous opaque address. Update the explanation in
crates/rustysnes-test-harness/tests/undisbeliever_golden.rs lines 41-50 and the
corresponding release-note explanation in CHANGELOG.md lines 162-170; both sites
must describe the same backdrop-column zeroing and pass-ordering behavior.
- Around line 32-35: Update the `PERDOT_KNOWN_GAPS` entry and the related branch
diagnostics around `inidisp_forgot_to_force_blank` to use the
reference-disagreement classification instead of “known per-dot gap (Phase 4d
pending)” or “per-dot gap hash changed.” Keep the intentional mismatch handling
intact while removing all obsolete Phase 4d/pending-gap wording from CI output.
🪄 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: 751025c7-d3fa-4e9c-8edf-d1496cc724fe
📒 Files selected for processing (2)
CHANGELOG.mdcrates/rustysnes-test-harness/tests/undisbeliever_golden.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: accuracysnes
- GitHub Check: test-light
- GitHub Check: lint
- GitHub Check: review
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (9)
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-test-harness/tests/undisbeliever_golden.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-test-harness/tests/undisbeliever_golden.rs
**/*.{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:
crates/rustysnes-test-harness/tests/undisbeliever_golden.rsCHANGELOG.md
**/*
📄 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:
crates/rustysnes-test-harness/tests/undisbeliever_golden.rsCHANGELOG.md
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-test-harness/tests/undisbeliever_golden.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-test-harness/tests/undisbeliever_golden.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-test-harness/tests/undisbeliever_golden.rs
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.For the full pull request diff against its base branch, modify
CHANGELOG.mdwhen user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.
Files:
CHANGELOG.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:
CHANGELOG.md
🔇 Additional comments (1)
CHANGELOG.md (1)
14-40: LGTM!Also applies to: 44-61
| /// # This is a reference disagreement, not a RustySNES gap | ||
| /// | ||
| /// It was recorded as "a Phase 4d (PPU access-during-render) gap — when 4d lands, remove the entry | ||
| /// and re-bless". Reading the third reference (2026-08-02) overturned that: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the obsolete “Phase 4d pending” classification.
The new text classifies inidisp_forgot_to_force_blank as a reference disagreement, but PERDOT_KNOWN_GAPS and the existing branch at Lines 147-159 still report known per-dot gap (Phase 4d pending) and per-dot gap hash changed. Rename the classification and diagnostics so CI output does not identify an intentional reference mismatch as a pending RustySNES defect.
🤖 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-test-harness/tests/undisbeliever_golden.rs` around lines 32
- 35, Update the `PERDOT_KNOWN_GAPS` entry and the related branch diagnostics
around `inidisp_forgot_to_force_blank` to use the reference-disagreement
classification instead of “known per-dot gap (Phase 4d pending)” or “per-dot gap
hash changed.” Keep the intentional mismatch handling intact while removing all
obsolete Phase 4d/pending-gap wording from CI output.
| /// - **MesenCE** writes it per *layer fetch* and then runs `RenderBgColor()` — which zeroes backdrop | ||
| /// columns — **after** every layer render in a span. What survives a span is therefore decided by | ||
| /// pass ordering across the whole span, which is an artefact of a span-based renderer rather than | ||
| /// a per-dot physical process. | ||
| /// | ||
| /// So the `7fc6` encodes MesenCE's renderer architecture. Two corroborations: `C3.12` — the scored | ||
| /// AccuracySNES row that asserts the redirect target directly — **passes on both Mesen2 and | ||
| /// RustySNES**, so they agree on the assertion and differ only on this homebrew hash; and `C3.12`'s | ||
| /// provenance is a per-dot statement ("uses the colour the PPU *is drawing*"), which the per-dot | ||
| /// model implements directly. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the conflicting PPU specification.
The changed documentation states that MesenCE zeroes backdrop columns through span pass ordering. docs/ppu.md still states that transparent pixels retain the previous opaque address. Keep the specification consistent before merge.
crates/rustysnes-test-harness/tests/undisbeliever_golden.rs#L41-L50: align the harness explanation with the final PPU model.CHANGELOG.md#L162-L170: align the release-note explanation with the same PPU model.
📍 Affects 2 files
crates/rustysnes-test-harness/tests/undisbeliever_golden.rs#L41-L50(this comment)CHANGELOG.md#L162-L170
🤖 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-test-harness/tests/undisbeliever_golden.rs` around lines 41
- 50, Align the PPU documentation with the final per-dot/span-renderer model
rather than stating that transparent pixels retain the previous opaque address.
Update the explanation in
crates/rustysnes-test-harness/tests/undisbeliever_golden.rs lines 41-50 and the
corresponding release-note explanation in CHANGELOG.md lines 162-170; both sites
must describe the same backdrop-column zeroing and pass-ordering behavior.
Source: Path instructions
Coverage 360 → 361 of 443.
What the row claims
The 65C816 has no undefined opcodes. Every one of the 256 bytes is a documented instruction with a documented length, and exactly one —
STP— stops the processor untilRESET. The cart executes each of the 241 straight-line opcodes in a WRAM sandbox and counts three outcomes against the length Table 5-4 of the WDC W65C816S datasheet gives it: returned where it should, returned late, or did not return.cpu_opcodes.rsis built from the matrix's own columnar rules with a gate that fails the build if they leave a slot empty or fill one twice — which caughtXBA($EB) missing on the first run.The terminator cannot be a return
TXSandTCSmove the stack pointer — withx = 1aTXSputs it in page zero — so the return address is no longer where anRTSwould pop it from. Control comes back through aJMP, and because aJMPis three bytes, the exit addresses are chosen so its own operand bytes are harmless one-byte instructions:An opcode consuming one byte too many therefore executes a register transfer and walks into the fill, instead of executing half an address. (
$7E:EAEAwas the first choice and is wrong — it sits inside the spaceruntime.increserves for widening the measurement channel to 1792 slots.)The watchdog takes two strikes
NMI fires once per vblank, and across 241 sandbox runs it will eventually land inside a healthy one — so one hit cannot mean "stuck". The handler counts hits per opcode and abandons only on the second.
Dangerous-when-correct opcodes are handled, not excluded
MVN/MVPA + 1bytes —$FFFFwould move 65536A = 0in the preambleXCECis setCLCfirst makes it a no-opTXS/TCSSPSED,PLPd/anything setm,x,d,cVerified by injection, twice
WDMmade a three-byte instruction$42TRB dpmade to jam (PC rewound)$14, watchdog rescues the batteryPicking that second injection is not free:
SEDandCLCboth hang the cart beforeA6.15runs, because the runtime and earlier Group A rows execute them.TRBis executed nowhere else on the cart.The bug that made this look like a reference defect
The NMI handler runs with whatever data bank the sandbox left —
$7E— solda $4210read a WRAM byte and the NMI was never acknowledged. Three hosts happened never to land an NMI where that showed. ares did, and the row reportedPLA($68) overshooting on ares alone, stably, with all 240 other opcodes agreeing.That reads exactly like a reference bug, and it was ours. The chain that settled it:
INXfill showed both ares and RustySNES resuming at the correct offset — soPLA's length was never in question.$68passed on ares — so the failure needed the full sweep's elapsed time.lda f:$004210is DBR-independent and fixes it. The handler now also preservesA:RTIrestoresPandPCbut not the accumulator, and an interrupt that silently rewritesAis not transparent to what it interrupted.With the fix the injections are exact where they had been approximate — the jam injection reported two stuck opcodes and 63 clean ones before, and reports one and 240 now.
Verification
cargo run -p accuracysnes-gen— 346 tests; bank$006,249 bytes freecargo fmt --all --check,cargo clippy --workspace --exclude rustysnes-android --all-targets -- -D warnings,RUSTDOCFLAGS="-D warnings" cargo doc— cleancargo test -p rustysnes-test-harness --features test-roms— 56 passed, battery 311/311crossval.sh— 3 references agree, 55 scenes match / 0 unblessed / 0 mismatched,ARES_KNOWN_FAILURESunchanged at 3Two harness bugs the injections found, both now gated: the driver's scratch first sat at
$7E:0170, which abus.rsrow also uses (WRAM scratch has no allocator, unlike the measurement channel); and moving it into$7E:02xxwould have put it inside the sandbox's own direct page, where the first direct-page operand would have overwritten the driver's state with its own test data.🤖 Generated with Claude Code
Adds AccuracySNES assertion
A6.15for 65C816 opcode-definedness. It executes 241 straight-line opcodes in a WRAM sandbox and checks the documented instruction length. Coverage increases from 360 to 361 of 443; on-cart coverage increases from 303 to 304.The claim is false if any executable opcode returns at the wrong length, returns late, or does not return. The test detects these cases with JMP terminators and a two-strike NMI watchdog.
The watchdog now acknowledges NMI through DBR-independent
$4210access and preserves the accumulator. The test isolates hazardous instructions, includingMVN,MVP,XCE,TXS,TCS,SED, andPLP.Injected defects produce the expected failures: a three-byte
WDMreports a late return, and a jammedTRBreports a non-returning opcode. No emulator-core behavior changes.