Skip to content

fix(accuracysnes): diagnose the Mesen2 oracle; flag an unverified figure - #287

Merged
doublegate merged 4 commits into
mainfrom
fix/mesen2-oracle-diagnosis
Jul 31, 2026
Merged

fix(accuracysnes): diagnose the Mesen2 oracle; flag an unverified figure#287
doublegate merged 4 commits into
mainfrom
fix/mesen2-oracle-diagnosis

Conversation

@doublegate

@doublegate doublegate commented Jul 31, 2026

Copy link
Copy Markdown
Owner

First v1.29.0 item. Three v1.28.0 rows ended at "no oracle can arbitrate"C7.07's errata,
the scene goldens, and the dot half of C7.05 — so the Mesen2 headless runner was investigated
rather than accepted as "environmental", which is how it had been recorded.

What is now established

  • The hang is genuinely pre-existing. The v1.25.0-era cartridge image, restored from git and
    run unmodified, times out identically. Nothing in this session's rows caused it.

  • It is not the frame budget. The battery needs ~431 frames on RustySNES (MAX_FRAMES 1500),
    snes9x is given 2000, and Mesen2 still times out at 4000.

  • The Lua never observes DONE = 0xA5, and the results magic never reads as ACSN:

    access first magic byte expected
    emu.memType.snesWorkRam, offset $F000 $A1 $41 ('A')
    emu.memType.snesMemory, address $7EF000 $52 $41 ('A')

    So the failure is in the Lua-to-emulator memory bridge, or the battery not executingnot a
    timeout. Exit code 254 has been misreporting the cause the entire time, which is why this looked
    environmental.

  • emu.log does not surface in --testrunner mode. Both readings above had to be smuggled out
    through the process exit code (emu.stop(value)); that is the only diagnostic channel available
    and it is now written down.

Not fixed

I am not claiming a working oracle. docs/accuracysnes-plan.md records the next step — exit with a
byte the runtime writes early and whose value is known, then walk the address until it reads back;
if the bridge is at fault the fix is a one-line memType/offset change, and if the battery genuinely
does not execute that is a much larger Mesen2 configuration question. It also says plainly not to
spend further effort on rows that need an arbiter until this resolves.

One real fix included

mesen_crossval.lua's MAX_FRAMES was 900, against the harness's 1500, mesen_scenes.lua's
4000 and libretro_crossval.c's 2000. The harness comment that names the budgets which must move
together lists the other two and omits this file, which is how it drifted. Aligned to 4000 and
the comment corrected.

It is a latent inconsistency, not the cause — 4000 frames times out exactly as 900 does — and the
commit says so rather than implying a fix.

Docs, one Lua constant, one comment. No generator or ROM change.

🤖 Generated with Claude Code

Summary

The change documents a pre-existing Mesen2 headless-runner hang. The hang triggers with the v1.25.0 cartridge image and persists through 4000 frames. It prevents DONE = 0xA5 and ACSN result markers from being observed. The diagnosis is false if the runner completes, exposes the markers, or the hang depends on the frame budget.

AccuracySNES dossier changes

  • Adds v1.29.0 Mesen2 headless-runner diagnosis assertions.
  • Updates the A5.18 assertion from impossible to borderline and keeps it parked.
  • Records 175-dot fixed instrumentation cost and 166 dots of available measurement budget.
  • Updates frame-budget documentation and aligns mesen_crossval.lua with 4000 frames.
  • The coverage denominator does not move.

No emulator-core observable behavior changes.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2541648e-bc5f-4d12-9246-2de4b6c798f9

📥 Commits

Reviewing files that changed from the base of the PR and between f96f608 and 229fee9.

📒 Files selected for processing (2)
  • crates/rustysnes-test-harness/tests/accuracysnes.rs
  • docs/accuracysnes-plan.md
📜 Recent review details
⏰ Context from checks skipped due to timeout. (4)
  • GitHub Check: test-light
  • GitHub Check: accuracysnes
  • GitHub Check: lint
  • GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (11)
crates/**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Bus owns 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.
Keep unsafe confined to existing allowed areas, namely frontend and FFI code, and document every unsafe block with a // SAFETY: comment.

Files:

  • crates/rustysnes-test-harness/tests/accuracysnes.rs
**/*.rs

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.rs: Use Rust edition 2024 and the toolchain pinned in rust-toolchain.toml (Rust 1.96).
Run cargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy with cargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc because missing_docs is a workspace lint.
Do not run cargo clippy --all-features; scripting and script-wasm are mutually exclusive. Use explicit per-feature jobs instead.

**/*.rs: Do not introduce .unwrap(), .expect(), or panic!() 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 new unsafe { ... } block or unsafe fn must 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 because unsafe_code is a workspace lint.

**/*.rs: Use Rust edition 2024 with the pinned 1.96 toolchain; satisfy workspace pedantic, nursery, missing_docs, and unsafe_code warnings because CI runs with -D warnings. Document every public item.
Keep unsafe code restricted to the frontend and FFI, and include a // SAFETY: justification for each use.
Keep hot paths allocation-free.
Treat rustysnes_core::Bus as 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/accuracysnes.rs
**/*.{rs,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Chip-behavior changes must update both the chip implementation and the corresponding docs/<subsystem>.md documentation.

A chip change must update both the chip implementation and its corresponding docs/<chip>.md documentation in the same change.

Files:

  • crates/rustysnes-test-harness/tests/accuracysnes.rs
  • docs/accuracysnes-plan.md
**/*

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*: Do not commit or vendor the generated snesdev_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 relevant to-dos/ sprint file.

**/*: Preserve the one-directional crate graph: chip crates must not depend on one another; rustysnes-core ties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keep docs/STATUS.md as the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNES v2.0 or engine-lineage anchors as project releases.

Files:

  • crates/rustysnes-test-harness/tests/accuracysnes.rs
  • docs/accuracysnes-plan.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 matching docs/<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
**/*.{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/accuracysnes.rs
crates/**

⚙️ CodeRabbit configuration file

crates/**: Emulator core. Hot paths are allocation-free; unsafe requires a // SAFETY: comment
naming the invariant. Any change to save-stated fields needs a FORMAT_VERSION bump and a
docs/adr/0006 bump-log entry. Behavior changes must update the matching docs/<chip>.md
in the same change.

Files:

  • crates/rustysnes-test-harness/tests/accuracysnes.rs
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
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 generated docs/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.md
🧠 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-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
🔇 Additional comments (3)
docs/accuracysnes-plan.md (2)

458-479: Remove the stale A5.18 arithmetic.

The corrected section records 175 dots of fixed overhead and 166 usable dots, but later text still states approximately 189 dots and 152 usable dots at Lines [513-515]. It also still describes three iterations and a 6-dot signal at Line [520] and Lines [541-544]. Update those references to the four-iteration, 8-dot assessment or remove the obsolete rationale. The document currently presents two incompatible A5.18 designs.

As per path instructions, docs are the spec, not a history log.

Source: Path instructions


481-508: 🎯 Functional Correctness

Keep the documented snes9x limit at 2000 frames. crossval.sh invokes libretro_crossval.c with 2000; the host's 2400 is only the default when no argument is supplied.

			> Likely an incorrect or invalid review comment.
crates/rustysnes-test-harness/tests/accuracysnes.rs (1)

59-62: LGTM!


Walkthrough

The changes correct AccuracySNES timing-budget arithmetic, document instrumentation overhead, revise the A5.18 assessment, diagnose a pre-existing Mesen2 headless oracle hang, and increase mesen_crossval.lua’s MAX_FRAMES from 900 to 4000.

Changes

AccuracySNES timing and runner alignment

Layer / File(s) Summary
Timing-budget assessment
docs/accuracysnes-plan.md
Records the 175-dot fixed instrumentation cost, sets the usable budget to 166 dots, and changes A5.18 to a parked but borderline four-iteration design.
Runner timeout and documentation alignment
scripts/accuracysnes/mesen_crossval.lua, crates/rustysnes-test-harness/tests/accuracysnes.rs, docs/accuracysnes-plan.md, CHANGELOG.md
Raises MAX_FRAMES to 4000, synchronizes the four runner frame-budget references, and documents that the Mesen2 headless oracle hang predates the current changes, exceeds the frame budget, and leaves completion and result markers unread.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 10
✅ Passed checks (10 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title follows Conventional Commits format and clearly describes the main Mesen2 oracle diagnosis.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Changelog Entry ✅ Passed The full main...HEAD PR diff adds a 25-line AccuracySNES entry to CHANGELOG.md, covering the Mesen2 diagnosis and MAX_FRAMES alignment.
Docs-As-Spec ✅ Passed Against origin/main, the sole crates change edits only a Rust documentation comment to list mesen_crossval.lua; MAX_FRAMES and all executable uses remain unchanged.
Accuracysnes Bookkeeping ✅ Passed The full diff from origin/main changes only CHANGELOG.md, harness documentation, the plan, and mesen_crossval.lua; no test or scene under tests/roms/AccuracySNES/gen/src/ changed.
No Panic On Untrusted Input ✅ Passed The PR adds only documentation, a Rust comment, and a Lua frame-limit constant; its diff contains no new .unwrap(), .expect(), or panic!() calls.
Safety Comment On New Unsafe ✅ Passed The PR diff adds no unsafe { ... } blocks or unsafe fn; the only changed Rust content updates frame-budget comments.

Comment @coderabbitai help to get the list of available commands.

@doublegate doublegate changed the title fix(accuracysnes): diagnose the Mesen2 oracle, align a stale frame budget docs(accuracysnes): diagnose the Mesen2 oracle; flag the instrument-overhead attribution Jul 31, 2026
@doublegate

Copy link
Copy Markdown
Owner Author

Added: the instrument-overhead attribution is unverified

Scoping the cheaper measurement instrument (the second v1.29.0 item) turned up a problem with
something I asserted in #283 and repeated in #285, so it is flagged here rather than left standing.

The measurement is real; the attribution is not. A5.19's native arm reads 241 dots at 4 RTIs
and 293 at 8 — 13 dots/RTI marginal, intercept 189. I called that intercept "the instrument".
Reading the code says it should not be:

hv_begin does synchronise to line start, polling hv_read_raw until H < 16 — but it stores
that poll's own reading as the start marker, so the sync sits before the latch, not inside the
delta. What is genuinely inside is hv_begin's tail after the latch plus hv_end's head before
its own — on the order of 30 dots. About 160 are unaccounted for, and nobody has measured the
empty span.

What is and is not in doubt. Every shipped result is a differential where the intercept cancels —
A5.19's 16-dot delta, A5.08's penalties, the sweep, both C7 rows. Those stand.

What rests on the 189 figure is the A5.18 parking decision: "341 − 189 leaves ~152 usable, a
BRK round trip is 39 dots, so only 3 fit". If the true overhead is nearer 30 the budget is ~310
and 8 fit — which would make A5.18 viable exactly as first designed, and its parking wrong.

The next step is one five-line test: measure_begin immediately followed by measure_end,
recorded to a slot. I did not write it here because this PR is docs-only and the result may reopen
A5.18, which deserves its own change.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 `@docs/accuracysnes-plan.md`:
- Around line 571-616: Complete the C7.06 four-injection mapping by documenting
code 3 as the third assert_a8 call in c7_06, which checks phase B’s Range Over
result, alongside codes 1, 2, and 4 in docs/accuracysnes-plan.md lines 571-616.
Mirror the same corrected code-3 description in CHANGELOG.md lines 32-55 within
the “Four injections” sentence.

In `@scripts/accuracysnes/crossval.sh`:
- Around line 159-166: Update the cross-validation logic around
SNES9X_KNOWN_FAILURES so it verifies the actual failing test identities, not
just that the total count equals 14. Preserve the documented known-failure set
and reject runs where an expected failure is missing or an unrelated test
replaces it.

In `@scripts/accuracysnes/mesen_crossval.lua`:
- Around line 24-29: Revert the MAX_FRAMES increase in the Mesen2
cross-validation runner and prevent this step from running unattended in CI
while the documented hang remains unresolved. Update the CI gating or skip logic
associated with mesen_crossval.lua, preserving an explicit way to run it
manually for investigation.
🪄 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: 1aa65e77-6390-4cb6-9745-7908a2023eac

📥 Commits

Reviewing files that changed from the base of the PR and between 1b0020f and 3aae74f.

⛔ Files ignored due to path filters (6)
  • docs/accuracysnes-coverage.md is excluded by !docs/accuracysnes-coverage.md and included by docs/**
  • tests/roms/AccuracySNES/ERROR_CODES.md is excluded by !tests/roms/AccuracySNES/ERROR_CODES.md and included by tests/**
  • tests/roms/AccuracySNES/SOURCE_CATALOG.tsv is excluded by !**/*.tsv, !tests/roms/AccuracySNES/SOURCE_CATALOG.tsv and included by tests/**
  • tests/roms/AccuracySNES/asm/tests_group_a.s is excluded by !tests/roms/AccuracySNES/asm/tests_group_a.s and included by tests/**
  • tests/roms/AccuracySNES/build/accuracysnes-pal.sfc is excluded by !tests/roms/AccuracySNES/build/** and included by tests/**
  • tests/roms/AccuracySNES/build/accuracysnes.sfc is excluded by !tests/roms/AccuracySNES/build/** and included by tests/**
📒 Files selected for processing (7)
  • CHANGELOG.md
  • crates/rustysnes-test-harness/tests/accuracysnes.rs
  • docs/accuracysnes-plan.md
  • scripts/accuracysnes/crossval.sh
  • scripts/accuracysnes/mesen_crossval.lua
  • tests/roms/AccuracySNES/gen/src/dossier.rs
  • tests/roms/AccuracySNES/gen/src/tests/ppu.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
  • GitHub Check: test-light
  • GitHub Check: lint
  • GitHub Check: accuracysnes
  • GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (16)
**/*

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*: Do not commit or vendor the generated snesdev_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 relevant to-dos/ sprint file.

**/*: Preserve the one-directional crate graph: chip crates must not depend on one another; rustysnes-core ties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keep docs/STATUS.md as the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNES v2.0 or engine-lineage anchors as project releases.

Files:

  • scripts/accuracysnes/mesen_crossval.lua
  • scripts/accuracysnes/crossval.sh
  • crates/rustysnes-test-harness/tests/accuracysnes.rs
  • tests/roms/AccuracySNES/gen/src/dossier.rs
  • tests/roms/AccuracySNES/gen/src/tests/ppu.rs
  • CHANGELOG.md
  • docs/accuracysnes-plan.md
scripts/accuracysnes/**

⚙️ CodeRabbit configuration file

scripts/accuracysnes/**: The cross-validation harness: the same AccuracySNES image is run on snes9x (through a
libretro host in C) and on Mesen2 (through its test runner and a Lua script), and their
verdicts are compared with the cart's. Its integrity is the whole argument for the
battery, so flag anything that could make a reference appear to agree — a verdict parsed
loosely, a missing-file path that degrades to success, a scene comparison that skips
rather than fails when the golden is absent. A known reference divergence belongs in
SNES9X_KNOWN_FAILURES with a source citation, never in a widened match.

Files:

  • scripts/accuracysnes/mesen_crossval.lua
  • scripts/accuracysnes/crossval.sh
crates/**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Bus owns 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.
Keep unsafe confined to existing allowed areas, namely frontend and FFI code, and document every unsafe block with a // SAFETY: comment.

Files:

  • crates/rustysnes-test-harness/tests/accuracysnes.rs
**/*.rs

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.rs: Use Rust edition 2024 and the toolchain pinned in rust-toolchain.toml (Rust 1.96).
Run cargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy with cargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc because missing_docs is a workspace lint.
Do not run cargo clippy --all-features; scripting and script-wasm are mutually exclusive. Use explicit per-feature jobs instead.

**/*.rs: Do not introduce .unwrap(), .expect(), or panic!() 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 new unsafe { ... } block or unsafe fn must 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 because unsafe_code is a workspace lint.

**/*.rs: Use Rust edition 2024 with the pinned 1.96 toolchain; satisfy workspace pedantic, nursery, missing_docs, and unsafe_code warnings because CI runs with -D warnings. Document every public item.
Keep unsafe code restricted to the frontend and FFI, and include a // SAFETY: justification for each use.
Keep hot paths allocation-free.
Treat rustysnes_core::Bus as 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/accuracysnes.rs
  • tests/roms/AccuracySNES/gen/src/dossier.rs
  • tests/roms/AccuracySNES/gen/src/tests/ppu.rs
**/*.{rs,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Chip-behavior changes must update both the chip implementation and the corresponding docs/<subsystem>.md documentation.

A chip change must update both the chip implementation and its corresponding docs/<chip>.md documentation in the same change.

Files:

  • crates/rustysnes-test-harness/tests/accuracysnes.rs
  • tests/roms/AccuracySNES/gen/src/dossier.rs
  • tests/roms/AccuracySNES/gen/src/tests/ppu.rs
  • CHANGELOG.md
  • docs/accuracysnes-plan.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 matching docs/<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
**/*.{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/accuracysnes.rs
  • tests/roms/AccuracySNES/gen/src/dossier.rs
  • tests/roms/AccuracySNES/gen/src/tests/ppu.rs
crates/**

⚙️ CodeRabbit configuration file

crates/**: Emulator core. Hot paths are allocation-free; unsafe requires a // SAFETY: comment
naming the invariant. Any change to save-stated fields needs a FORMAT_VERSION bump and a
docs/adr/0006 bump-log entry. Behavior changes must update the matching docs/<chip>.md
in the same change.

Files:

  • crates/rustysnes-test-harness/tests/accuracysnes.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 its dossier.rs::MAP entry, all required regenerated artifacts, and matching count changes in docs/accuracysnes-plan.md. Artifact presence must be judged from the path-filter exclusion list, not from unreadable contents.

Files:

  • tests/roms/AccuracySNES/gen/src/dossier.rs
  • tests/roms/AccuracySNES/gen/src/tests/ppu.rs
tests/roms/AccuracySNES/gen/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Rebuild AccuracySNES after any change to gen/ or asm/; never hand-edit generated asm/tests_group_a.s or asm/scenes.s.

Files:

  • tests/roms/AccuracySNES/gen/src/dossier.rs
  • tests/roms/AccuracySNES/gen/src/tests/ppu.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::MAP must 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/dossier.rs
  • tests/roms/AccuracySNES/gen/src/tests/ppu.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.md when 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
  • docs/accuracysnes-plan.md
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
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 generated docs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.

Files:

  • docs/accuracysnes-plan.md
🧠 Learnings (5)
📚 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/dossier.rs
  • tests/roms/AccuracySNES/gen/src/tests/ppu.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/ppu.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/ppu.rs
📚 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
🔇 Additional comments (5)
tests/roms/AccuracySNES/gen/src/dossier.rs (1)

215-215: LGTM!

tests/roms/AccuracySNES/gen/src/tests/ppu.rs (1)

74-74: LGTM!

Also applies to: 2444-2562, 2564-2607, 2609-2637

docs/accuracysnes-plan.md (1)

458-485: LGTM!

Also applies to: 500-500

crates/rustysnes-test-harness/tests/accuracysnes.rs (1)

59-59: LGTM!

CHANGELOG.md (1)

14-31: LGTM!

Comment thread docs/accuracysnes-plan.md
Comment on lines +159 to +166
# snes9x, +1 test (C7.06 "TimeOver by YLOC+1", fails phase A with code 1): the same scanline-granular
# sprite model as C7.05, seen from the other side. The row requires Time Over to be clear while the
# sprites' OWN line is being evaluated (V = 100) and set once the line they paint on has begun
# (V = 101 = YLOC + 1); snes9x already reads it set on V = 100, i.e. it flags the tile-budget
# overflow a line early because it evaluates and paints in the same pass. The 20-sprite/8x8 control
# passes there, so this is the position being wrong, not the budget. Documented by nocash fullsnes
# and the SNESdev Wiki (34-tile budget, raised by the fetch phase). Region-independent.
SNES9X_KNOWN_FAILURES=14

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Inspect how SNES9X_KNOWN_FAILURES is used in crossval.sh.
fd 'crossval.sh' scripts/accuracysnes --exec grep -n -C 5 'SNES9X_KNOWN_FAILURES' {}

Repository: doublegate/RustySNES

Length of output: 2140


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- crossval.sh relevant control flow ---'
sed -n '145,235p' scripts/accuracysnes/crossval.sh
printf '%s\n' '--- libretro_crossval.c structure and exit paths ---'
ast-grep outline scripts/accuracysnes/libretro_crossval.c
rg -n -C 6 'return|exit|fail|FAIL|argc|argv' scripts/accuracysnes/libretro_crossval.c

Repository: doublegate/RustySNES

Length of output: 17831


Compare failing test identities, not only the failure count. A 15th failure returns exit code 15 and fails the check, but a run that replaces one known failure with an unrelated failure still returns 14 and passes.

🤖 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 `@scripts/accuracysnes/crossval.sh` around lines 159 - 166, Update the
cross-validation logic around SNES9X_KNOWN_FAILURES so it verifies the actual
failing test identities, not just that the total count equals 14. Preserve the
documented known-failure set and reject runs where an expected failure is
missing or an unrelated test replaces it.

Source: Path instructions

Comment on lines +24 to +29
-- Bounds the same run as the in-repo harness's MAX_FRAMES (1500), mesen_scenes.lua's (4000) and
-- libretro_crossval.c's max_frames (2000). This one was left at 900 when the others grew -- the
-- harness comment naming the budgets that must move together does not list this file, which is how
-- it was missed. It is NOT why this runner times out (4000 fails identically); see
-- docs/accuracysnes-plan.md on the Mesen2 oracle.
local MAX_FRAMES = 4000

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🔵 Trivial

Raising MAX_FRAMES to 4000 quadruples the wait time for a run already known to hang for the full duration.

docs/accuracysnes-plan.md documents that Mesen2 still times out at 4000 frames. Until the underlying hang is fixed, every invocation of this runner now waits over 4x longer than before (900 → 4000 frames) before failing, with no additional diagnostic value from the extra frames.

Consider gating or skipping this Mesen2 cross-validation step in CI until the root cause is resolved, rather than only raising the budget it hangs at. If it already runs unattended in CI, this is worth confirming so the wall-clock cost is intentional.

🤖 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 `@scripts/accuracysnes/mesen_crossval.lua` around lines 24 - 29, Revert the
MAX_FRAMES increase in the Mesen2 cross-validation runner and prevent this step
from running unattended in CI while the documented hang remains unresolved.
Update the CI gating or skip logic associated with mesen_crossval.lua,
preserving an explicit way to run it manually for investigation.

Source: Path instructions

doublegate and others added 2 commits July 31, 2026 17:22
…dget

Three v1.28.0 items ended at "no oracle can arbitrate", so the headless
runner was investigated rather than accepted as environmental.

Established:
- The hang is genuinely pre-existing. The v1.25.0-era cartridge image,
  restored from git and run unmodified, times out identically.
- It is NOT the frame budget. The battery needs ~431 frames on RustySNES
  and snes9x is given 2000; Mesen2 still times out at 4000.
- The Lua never observes DONE = 0xA5, and the results magic never reads
  as ACSN -- under snesWorkRam with a $F000 offset it reads $A1, under
  snesMemory with the full $7EF000 address it reads $52; neither is the
  expected $41. So the failure is in the Lua-to-emulator memory bridge,
  or the battery not executing, and not a timeout. Exit code 254 has
  been misreporting the cause all along.
- emu.log does not surface in --testrunner mode, so diagnosis has to be
  smuggled out through the process exit code.

Not fixed. The plan records the next step -- exit with a byte the runtime
writes early and whose value is known, then walk the address until it
reads back -- and says not to spend further effort on rows needing an
arbiter until this resolves.

Separately real: mesen_crossval.lua's MAX_FRAMES was 900 against the
harness's 1500, mesen_scenes.lua's 4000 and libretro_crossval.c's 2000.
The harness comment naming the budgets that must move together omits this
file, which is how it was missed. Aligned, and the comment corrected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The intercept is real -- A5.19's native arm reads 241 dots at 4 RTIs and
293 at 8, so 13 dots/RTI marginal and an intercept of 189. What is NOT
established is that those dots are the instrument.

hv_begin does sync to line start, but it stores THAT poll's own reading
as the start marker, so the sync sits before the latch rather than inside
the delta. What is genuinely inside is hv_begin's tail plus hv_end's head
-- on the order of 30 dots. So ~160 are unaccounted for and nobody has
measured the empty span.

Every shipped result is a differential where the intercept cancels, so
none is in doubt. What rests on the 189 figure is the A5.18 parking
decision: 341 - 189 leaves ~152 usable and only 3 round trips fit. If the
true overhead is nearer 30 the budget is ~310 and 8 fit, which would make
A5.18 viable as first designed and its parking wrong.

Next step is one five-line test: measure_begin immediately followed by
measure_end, recorded to a slot.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@doublegate
doublegate force-pushed the fix/mesen2-oracle-diagnosis branch from fd92a0f to f04f6bf Compare July 31, 2026 21:22
@doublegate doublegate changed the title docs(accuracysnes): diagnose the Mesen2 oracle; flag the instrument-overhead attribution fix(accuracysnes): diagnose the Mesen2 oracle; flag an unverified figure Jul 31, 2026
@doublegate

Copy link
Copy Markdown
Owner Author

Review adjudication

No blocking issues. One applied, one declined with the history, one moot after a rebase.

Applied: the title

Shortened to 72 characters (it was 73) and reworded to name the second commit, which flags the
~189-dot instrument overhead as an unverified attribution.

The other half of that finding — "omits mentioning the addition of test C7.06" — was accurate
when written and is now moot: this branch was cut from the C7.06 work, which has since merged as
#286, and the branch has been rebased onto main so it now carries only the two Mesen2 commits.
The title was describing the diff correctly; the diff was the thing that was wrong.

Declined: "all three budgets must move together"

The harness comment says to check the other budgets in the same change, not to equalize them —
and they have never been equal:

runner budget purpose
in-repo harness 1500 CI-time ceiling
libretro_crossval.c 2000 snes9x host
mesen_scenes.lua 4000 already
mesen_crossval.lua 900 → 4000 the one that had drifted

mesen_crossval.lua was the outlier at 900 and now matches its sibling Lua at 4000. Raising the
harness's 1500 would only lengthen CI on a battery that finishes in ~431 frames, for no diagnostic
gain. The comment was also why this drifted — it named the other two files and omitted this one —
which is fixed in the same commit.

Worth restating from the PR body, since the finding reads as though the budget were the fix: it is
not
. Mesen2 times out at 4000 exactly as it did at 900. The change is a latent-inconsistency fix,
and the commit message says so.

Moot: the duplicate .a8

Not in this PR's diff any more — that hunk belonged to C7.06, now merged. It was declined on #286
with the reasoning: tests_group_a.s is generated output, CI's "assert the committed artifacts are
byte-identical" step re-runs the generator and diffs it, and that step passed.

…stored

The empty span (measure_begin immediately followed by measure_end) is 175
dots, with 8 NOPs at exactly 28 dots marginal. So the instrument's
marginal accuracy is exact, only its fixed cost is large, and A5.19's
189-dot intercept IS the instrument -- the v1.28.0 assessment was right
the first time.

The preceding commit called that figure into doubt on the strength of
reading hv_begin/hv_end and estimating '~30 dots should be inside the
delta'. That estimate was bad arithmetic: the pair runs ~70-90 CPU cycles
between the latches and a cycle is 6-8 master clocks, 1.5-2 dots each.
Counting instructions instead of cycles produced the discrepancy.

Corrected budget: 341 - 175 = 166 dots, against the 152 the A5.18 note
assumed. That moves a BRK round trip from 3 iterations to 4, giving 8
dots of signal against TOL 2 -- better than the 6 that parked it, still a
difference-of-differences and still short of A5.19's 16. A5.18 stays
parked on corrected arithmetic; the verdict is borderline, not
impossible.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@doublegate

Copy link
Copy Markdown
Owner Author

Ran the empty-span test — my flag was wrong

The measurement I asked for in the previous commit:

empty span = 175 dots
8 NOPs     = 203 dots  (marginal = 28 dots)

The instrument's fixed cost is 175 dots. So A5.19's 189-dot intercept is the instrument
(175, plus ~14 for the two no-op clc/xce pairs inside its span), and the v1.28.0 assessment
was right the first time. The flag added in the previous commit is withdrawn, and the section now
records the measured figure instead.

The control matters too: 8 NOPs cost exactly 28 dots (8 x 14 clocks / 4). The instrument's
marginal accuracy is exact — only its fixed cost is large, which is precisely the shape every
differential test relies on.

Where the doubt came from. I read hv_begin/hv_end and estimated "only ~30 dots should be
inside the delta". That was bad arithmetic, not a bad reading: the pair runs ~70-90 CPU cycles
between the two latches (sta f:, php/plp, pha/pla, two jsr/rts pairs, and
hv_read_raw's eleven instructions), and a cycle is 6-8 master clocks — 1.5-2 dots each. I
counted instructions as though they were dots. 70-90 cycles x ~2 dots lands squarely on 175.

Consequence for A5.18. The corrected budget is 341 - 175 = 166 dots, against the 152 its
parking note assumed. A BRK round trip is 39 dots, so 4 iterations fit rather than 3 — 8 dots
of signal against TOL 2, better than the 6 that parked it, but still a difference-of-differences
accumulating quantisation from four measurements and still well short of A5.19's 16. It stays
parked, now on corrected arithmetic rather than a disputed number; the honest verdict is
borderline, not impossible.

The probe itself is not in this PR — it was temporary, and the tree is back to reproducing the
committed ROM byte-for-byte.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
scripts/accuracysnes/mesen_crossval.lua (1)

29-29: 🧹 Nitpick | 🔵 Trivial

Gate the known Mesen2 hang before increasing unattended wait time.

MAX_FRAMES = 4000 increases the maximum wait from 900 to 4000 frames while the plan documents that the runner still hangs at 4000. If CI invokes this script, each failure now waits for the longer limit before returning exit code 254. Confirm the invocation path and gate this diagnostic runner in unattended jobs while preserving manual execution.

Verification command
#!/bin/bash
set -euo pipefail
rg -n -C 4 'mesen_crossval\.lua|MAX_FRAMES|testrunner' .
🤖 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 `@scripts/accuracysnes/mesen_crossval.lua` at line 29, Inspect the invocation
paths for scripts/accuracysnes/mesen_crossval.lua and prevent unattended or CI
jobs from running this diagnostic runner, while retaining its existing manual
execution path. Update the relevant runner configuration or guard around the
invocation rather than increasing MAX_FRAMES, and preserve the existing exit
behavior for explicitly requested manual runs.
🤖 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 `@docs/accuracysnes-plan.md`:
- Around line 474-479: Update the remaining A5.18 references to consistently use
the corrected 166-dot budget: replace the obsolete 189-dot overhead, 152 usable
dots, three iterations, and six-dot signal in the surrounding assessment, and
revise or remove the outdated rationale near the final A5.18 conclusion. Ensure
docs/accuracysnes-plan.md presents only the corrected four-iteration, eight-dot
assessment.
- Line 481: Remove the duplicated Mesen2 heading text in the section heading,
leaving a single correctly formatted “The Mesen2 oracle — diagnosed, not yet
fixed (`v1.29.0`)” title.

In `@scripts/accuracysnes/mesen_crossval.lua`:
- Around line 24-28: Update the descriptions in
scripts/accuracysnes/mesen_crossval.lua (lines 24-28), docs/accuracysnes-plan.md
(lines 487-494), and CHANGELOG.md (lines 34-37) to distinguish the explicit
2000-frame invocation budget passed by scripts/accuracysnes/crossval.sh from
libretro_crossval.c’s 2400-frame default when no argument is provided; preserve
the existing budget relationships and do not describe 2000 as the default.

---

Duplicate comments:
In `@scripts/accuracysnes/mesen_crossval.lua`:
- Line 29: Inspect the invocation paths for
scripts/accuracysnes/mesen_crossval.lua and prevent unattended or CI jobs from
running this diagnostic runner, while retaining its existing manual execution
path. Update the relevant runner configuration or guard around the invocation
rather than increasing MAX_FRAMES, and preserve the existing exit behavior for
explicitly requested manual runs.
🪄 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: 05e4ae0a-3431-46c4-a7cb-2c9422e9cc66

📥 Commits

Reviewing files that changed from the base of the PR and between 3aae74f and f96f608.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • crates/rustysnes-test-harness/tests/accuracysnes.rs
  • docs/accuracysnes-plan.md
  • scripts/accuracysnes/mesen_crossval.lua
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
  • GitHub Check: lint
  • GitHub Check: accuracysnes
  • GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (13)
**/*

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*: Do not commit or vendor the generated snesdev_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 relevant to-dos/ sprint file.

**/*: Preserve the one-directional crate graph: chip crates must not depend on one another; rustysnes-core ties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keep docs/STATUS.md as the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNES v2.0 or engine-lineage anchors as project releases.

Files:

  • scripts/accuracysnes/mesen_crossval.lua
  • crates/rustysnes-test-harness/tests/accuracysnes.rs
  • docs/accuracysnes-plan.md
  • CHANGELOG.md
scripts/accuracysnes/**

⚙️ CodeRabbit configuration file

scripts/accuracysnes/**: The cross-validation harness: the same AccuracySNES image is run on snes9x (through a
libretro host in C) and on Mesen2 (through its test runner and a Lua script), and their
verdicts are compared with the cart's. Its integrity is the whole argument for the
battery, so flag anything that could make a reference appear to agree — a verdict parsed
loosely, a missing-file path that degrades to success, a scene comparison that skips
rather than fails when the golden is absent. A known reference divergence belongs in
SNES9X_KNOWN_FAILURES with a source citation, never in a widened match.

Files:

  • scripts/accuracysnes/mesen_crossval.lua
crates/**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Bus owns 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.
Keep unsafe confined to existing allowed areas, namely frontend and FFI code, and document every unsafe block with a // SAFETY: comment.

Files:

  • crates/rustysnes-test-harness/tests/accuracysnes.rs
**/*.rs

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.rs: Use Rust edition 2024 and the toolchain pinned in rust-toolchain.toml (Rust 1.96).
Run cargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy with cargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc because missing_docs is a workspace lint.
Do not run cargo clippy --all-features; scripting and script-wasm are mutually exclusive. Use explicit per-feature jobs instead.

**/*.rs: Do not introduce .unwrap(), .expect(), or panic!() 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 new unsafe { ... } block or unsafe fn must 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 because unsafe_code is a workspace lint.

**/*.rs: Use Rust edition 2024 with the pinned 1.96 toolchain; satisfy workspace pedantic, nursery, missing_docs, and unsafe_code warnings because CI runs with -D warnings. Document every public item.
Keep unsafe code restricted to the frontend and FFI, and include a // SAFETY: justification for each use.
Keep hot paths allocation-free.
Treat rustysnes_core::Bus as 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/accuracysnes.rs
**/*.{rs,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Chip-behavior changes must update both the chip implementation and the corresponding docs/<subsystem>.md documentation.

A chip change must update both the chip implementation and its corresponding docs/<chip>.md documentation in the same change.

Files:

  • crates/rustysnes-test-harness/tests/accuracysnes.rs
  • docs/accuracysnes-plan.md
  • CHANGELOG.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 matching docs/<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
**/*.{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/accuracysnes.rs
crates/**

⚙️ CodeRabbit configuration file

crates/**: Emulator core. Hot paths are allocation-free; unsafe requires a // SAFETY: comment
naming the invariant. Any change to save-stated fields needs a FORMAT_VERSION bump and a
docs/adr/0006 bump-log entry. Behavior changes must update the matching docs/<chip>.md
in the same change.

Files:

  • crates/rustysnes-test-harness/tests/accuracysnes.rs
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
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 generated docs/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.md
  • CHANGELOG.md
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.md when 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 (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-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
🔇 Additional comments (4)
docs/accuracysnes-plan.md (2)

458-466: LGTM!


594-594: LGTM!

crates/rustysnes-test-harness/tests/accuracysnes.rs (1)

59-59: LGTM!

CHANGELOG.md (1)

14-32: LGTM!

Comment thread docs/accuracysnes-plan.md
Comment on lines +474 to +479
**The corrected span budget is `341 - 175 = 166` dots**, slightly more generous than the 152 the
`A5.18` note assumed. That moves a `BRK` round trip (39 dots) from 3 iterations to **4**, giving 8
dots of signal against `TOL` 2 — better than the 6 that parked it, still a difference-of-differences
accumulating quantisation from four measurements, and still well short of `A5.19`'s 16. `A5.18`
stays parked, on corrected arithmetic rather than a wrong figure; the honest verdict is *borderline*,
not *impossible*.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Update the remaining A5.18 arithmetic.

This section changes the budget to 166 dots and supports four iterations. The same plan still states:

  • Line 513-515: approximately 189 dots of overhead and 152 usable dots.
  • Line 520: only three iterations and six dots of signal.
  • Line 541-544: the same obsolete rationale.

Replace those values with the corrected assessment, or remove the obsolete explanation. Otherwise, the plan gives two incompatible A5.18 designs.

As per path instructions, docs are the spec, not a history log.

🤖 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-plan.md` around lines 474 - 479, Update the remaining A5.18
references to consistently use the corrected 166-dot budget: replace the
obsolete 189-dot overhead, 152 usable dots, three iterations, and six-dot signal
in the surrounding assessment, and revise or remove the outdated rationale near
the final A5.18 conclusion. Ensure docs/accuracysnes-plan.md presents only the
corrected four-iteration, eight-dot assessment.

Source: Path instructions

Comment thread docs/accuracysnes-plan.md Outdated
Comment on lines +24 to +28
-- Bounds the same run as the in-repo harness's MAX_FRAMES (1500), mesen_scenes.lua's (4000) and
-- libretro_crossval.c's max_frames (2000). This one was left at 900 when the others grew -- the
-- harness comment naming the budgets that must move together does not list this file, which is how
-- it was missed. It is NOT why this runner times out (4000 fails identically); see
-- docs/accuracysnes-plan.md on the Mesen2 oracle.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -n -C 5 'libretro_crossval\.c|libretro_crossval|max_frames|MAX_FRAMES|mesen_crossval\.lua' .

Repository: doublegate/RustySNES

Length of output: 50378


🏁 Script executed:

#!/bin/bash
set -euo pipefail
printf '%s\n' '--- mesen_crossval.lua ---'
sed -n '20,32p' scripts/accuracysnes/mesen_crossval.lua
printf '%s\n' '--- accuracysnes-plan.md ---'
sed -n '487,495p' docs/accuracysnes-plan.md
printf '%s\n' '--- CHANGELOG.md ---'
sed -n '32,39p' CHANGELOG.md
printf '%s\n' '--- crossval.sh ---'
sed -n '168,176p' scripts/accuracysnes/crossval.sh
printf '%s\n' '--- libretro_crossval.c ---'
sed -n '217,241p' scripts/accuracysnes/libretro_crossval.c

Repository: doublegate/RustySNES

Length of output: 3446


Describe 2000 as the explicit invocation budget.

scripts/accuracysnes/crossval.sh passes 2000; libretro_crossval.c defaults to 2400 when no argument is provided. Update the mesen_crossval.lua and CHANGELOG.md descriptions to distinguish these values.

📍 Affects 3 files
  • scripts/accuracysnes/mesen_crossval.lua#L24-L28 (this comment)
  • docs/accuracysnes-plan.md#L487-L494
  • CHANGELOG.md#L34-L37
🤖 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 `@scripts/accuracysnes/mesen_crossval.lua` around lines 24 - 28, Update the
descriptions in scripts/accuracysnes/mesen_crossval.lua (lines 24-28),
docs/accuracysnes-plan.md (lines 487-494), and CHANGELOG.md (lines 34-37) to
distinguish the explicit 2000-frame invocation budget passed by
scripts/accuracysnes/crossval.sh from libretro_crossval.c’s 2400-frame default
when no argument is provided; preserve the existing budget relationships and do
not describe 2000 as the default.

The heading dedup is a real defect in the preceding commit -- the edit
concatenated the section header onto itself. Raised in review.

Also rephrased the frame-budget comment as the review suggested, and
corrected 'all three' to 'all four': there are four budgets bounding the
same run, and the comment previously named three because it omitted
mesen_crossval.lua -- which is how that one drifted to 900 in the first
place.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@doublegate

Copy link
Copy Markdown
Owner Author

Second adjudication

The suggestion was a real defect of mine. The blocking finding is not reproduced.

Applied: the duplicated heading

Real, and my fault — the edit that replaced the instrument section concatenated the following
section's header onto itself, leaving ### The Mesen2 oracle …### The Mesen2 oracle … on one line.
Deduplicated. Good catch; a grep -c would have shown it and I did not run one.

Applied: the frame-budget rephrasing

Taken, and one thing corrected beyond it: the comment said "all three bound the same run" when
there are four — the harness, mesen_scenes.lua, mesen_crossval.lua and
libretro_crossval.c. It named three because it omitted mesen_crossval.lua, which is exactly how
that budget drifted to 900 and stayed there. Now four, named explicitly.

Not reproduced: "the CHANGELOG notes a C7.06 behaviour change without the code"

This PR's CHANGELOG diff does not add any C7.06 text:

$ git diff origin/main...HEAD -- CHANGELOG.md | grep '^+' | grep -c 'C7.06'
0

The C7.06 entry is already on main — it landed with #286, which carried the generator change, the
regenerated ROM, docs/accuracysnes-plan.md and the coverage table together. This branch was cut
from that work and has since been rebased onto main, so C7.06 appears in the file but not in
the diff.

Worth adding, since the finding invokes the docs-as-spec rule: C7.06 is a cart test, not a chip
behaviour change. It asserts existing PPU behaviour and changed no emulator code, so there is no
docs/<chip>.md to update — the spec surface for a cart row is docs/accuracysnes-plan.md and the
regenerated coverage table, both of which #286 updated in the same change.

@github-actions

Copy link
Copy Markdown

Antigravity review (Gemini via Ultra)

This PR updates documentation and diagnostic notes regarding the Mesen2 test oracle and bumps MAX_FRAMES in mesen_crossval.lua to match mesen_scenes.lua.

Blocking issues

None found.

Suggestions

  • CHANGELOG.md:14: Diagnostic research and test harness timeout adjustments are recorded under ### Added. These are internal research/test updates rather than new features. Move them under ### Changed or reduce their scope in the changelog.
  • scripts/accuracysnes/mesen_crossval.lua:29 & crates/rustysnes-test-harness/tests/accuracysnes.rs:63: The doc comment claims all four frame limit constants bound the same execution and must move together, yet they remain explicitly mismatched across the harness files (1500 in accuracysnes.rs, 4000 in the Lua scripts, 2000 in libretro_crossval.c). Unify the default values across all test drivers or document why each environment requires a distinct threshold.

Nitpicks

  • CHANGELOG.md:14-37: The changelog entry includes detailed diagnostic narratives and timing math derivations. Keep changelog items high-level and leave full investigation logs in docs/accuracysnes-plan.md.

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

@doublegate
doublegate merged commit ee34709 into main Jul 31, 2026
16 checks passed
@doublegate
doublegate deleted the fix/mesen2-oracle-diagnosis branch July 31, 2026 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant