test(accuracysnes): A5.19 — RTI is 7 cycles native, 6 emulation

[doublegate]

First row of the v1.28.0 AccuracySNES rung. A5 on-cart coverage 12 → 13 of 15; the ROM
rebuilds byte-identically from accuracysnes-gen.

The assertion

RTI costs 7 cycles native, 6 emulation. The extra native cycle is the PBR pull — an 8-clock
WRAM read, not a 6-clock internal cycle — so eight iterations differ by 16 dots.

The shape

The RTIs are chained through one rti instruction. Every return frame is built before
measure_begin, all returning to the @spin label that is the rti itself, except the
first-pushed one — pulled last — which returns to @done. The measured span therefore contains
nothing but the RTIs: no loop counter, no branch, nothing to dilute the signal.

Three traps, and how each was caught

1. The 341-dot line wrap — the one A5.08 already documents, hit again. Sixteen RTIs is a
~200-dot span; it wrapped and reported native 42 against emulation 76. The native reading
smaller than the emulation one is the arithmetic opposite of the assertion, which is what made it
obviously junk rather than a plausible near-miss.

Diagnosed by measuring at two repeat counts and reading the slope: 4 → 8 RTIs moved native by
exactly 52 dots = 4 × 13, the predicted per-RTI cost. That proved the per-iteration model right and
only the offset junk.

2. rep cannot clear m while E = 1. measure_result emits 16-bit arithmetic; run it in
emulation mode and it silently executes 8-bit, sta writes one byte, and the reading sticks at a
constant regardless of repeat count. A number that does not move when the work moves is the
signature. measure_end is width-neutral (php/plp), so the fix is to latch first and widen
after.

3. The instrument's own overhead does not cancel across a mode boundary — the one that forced
the design. hv_begin/hv_end are subroutines that run at whatever register width the caller is
in, so the native and emulation spans carried different fixed overheads: native 279 against
emulation 332, emulation larger when the assertion says it must be smaller. Every other A5
differential compares two spans in the same mode, which is why this had never surfaced.

The xce pairs now sit inside the span, so both arms run the instrument natively and contain
identical non-RTI instructions, differing only in the mode the RTIs execute in. The native arm
therefore carries a deliberately no-op clc/xce at each end, and says so.

Verification

check	result
both spans, same overhead	native 293, emulation 277 — 189 each
difference, 8 RTIs	16 dots (expected 16, TOL 2)
difference, 4 RTIs	8 dots — linear in the repeat count, so it is a per-iteration cost and not a fixed artifact
injection at the named site	native RTI advancing S without paying the bus cycle collapses the difference to exactly 0 and fails the row
snes9x	divergence count unchanged at 12 — it passes the row too
Mesen2	not consulted: the headless runner times out in this environment, which predates this work
battery	100% on-cart, cargo run -p accuracysnes-gen reproduces the committed ROM byte-for-byte

cargo fmt --check, cargo clippy --workspace --all-targets -D warnings (0), full battery green.

Documented in docs/accuracysnes-plan.md §A5.19 so the next cross-mode row does not re-derive
trap 3, plus the CHANGELOG entry. The coverage table regenerates with the ROM.

🤖 Generated with Claude Code

Adds AccuracySNES dossier assertion A5.19 for RTI timing.

The test claims that RTI takes 7 cycles in native mode and 6 cycles in emulation mode because native mode performs an extra PBR pull. Eight RTI executions must produce a 16-dot difference, and removing the native bus cycle through injection must reduce the difference to zero.

The claim is false if the controlled native/emulation measurements do not differ by one cycle per RTI, or if injection does not remove the difference.

A5 coverage increases from 12/15 to 13/15. The ROM remains byte-identical, and the full test battery passes.

[coderabbitai]

Walkthrough

Adds AccuracySNES test A5.19 for the native-versus-emulation RTI timing difference. The test measures eight chained executions, asserts a 16-dot difference, registers the test, maps its dossier entry, and documents validation results.

Changes

AccuracySNES A5.19

Layer / File(s)	Summary
RTI timing measurement and assertion tests/roms/AccuracySNES/gen/src/tests/cpu.rs	Adds separate internal-cycle and stack-access timing constants. Measures eight chained native four-byte RTI frames against eight emulation three-byte frames and asserts the expected 16-dot difference.
Test registration and result tracking tests/roms/AccuracySNES/gen/src/tests/cpu.rs, tests/roms/AccuracySNES/gen/src/dossier.rs, docs/accuracysnes-plan.md, CHANGELOG.md	Registers A5.19 in Group A, maps it to dossier assertion A5.19, and documents the measurement design, instrumentation fixes, and validation results.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• doublegate/RustySNES#197: Uses the same AccuracySNES Group A test suite and dossier tracking, but covers different CPU behavior.
• doublegate/RustySNES#198: Uses the same CPU timing suite and test menu, but covers JSR (a,X) stack behavior instead of RTI.

🚥 Pre-merge checks | ✅ 8 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title uses a valid Conventional Commits prefix and accurately describes the RTI timing test, but its subject is not in the imperative mood.	Rewrite the subject in the imperative mood, for example: "test(accuracysnes): add A5.19 RTI cycle timing test".
Accuracysnes Bookkeeping	⚠️ Warning	Adding A5.19 changes generated coverage from 291/344 to 292/345, but docs/accuracysnes-plan.md still records 291 on-cart and 344 total.	Update the hand-maintained coverage row in docs/accuracysnes-plan.md to 292 on-cart and 345 total.

✅ Passed checks (8 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 PR diff from base main to HEAD adds a 23-line AccuracySNES A5.19 entry to CHANGELOG.md under Unreleased.
Docs-As-Spec	✅ Passed	The PR diff against origin/main contains no changes under crates/. It adds an AccuracySNES test, generated ROMs, and matching AccuracySNES documentation instead.
No Panic On Untrusted Input	✅ Passed	The patch adds no .unwrap(), .expect(), or panic!() calls. A5.19 uses only local literals, constants, and DSL calls; no untrusted input reaches a panic path.
Safety Comment On New Unsafe	✅ Passed	The PR adds no unsafe {} blocks or unsafe fn declarations; added-line search found no unsafe or SAFETY, and Rust unsafe counts remain 83 before and after.

---

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

[github-actions]

Antigravity review (Gemini via Ultra)

Adds the A5.19 accuracy test ROM case to measure and verify the timing differential of RTI execution between 65816 native mode (7 cycles) and emulation mode (6 cycles).

Blocking issues

None found.

Suggestions

• CHANGELOG.md:14: The PR title and changelog entry use an em-dash (—) in the subject line. Standardize on ASCII hyphens (-) to strictly match Conventional Commit format conventions across automated tooling.
• tests/roms/AccuracySNES/gen/src/tests/cpu.rs:2598-2603: Explicitly document in the code comment for DOTS_PER_8_STACK_ACCESSES why WRAM stack accesses take 8 master clocks (2 dots) vs 6 master clocks (1.5 dots) for internal cycles to clarify the 16 dot calculation for future maintainers.

Nitpicks

• tests/roms/AccuracySNES/asm/tests_group_a.s:4169: The generated assembly contains redundant comment annotations ; -> native (m/x stay 1: still 8-bit) following each xce invocation inside the test procedure.

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

[coderabbitai]

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 `@docs/accuracysnes-plan.md`:
- Around line 458-496: Synchronize the AccuracySNES coverage metadata by marking
A5.19 complete in docs/STATUS.md and to-dos/ROADMAP.md, updating the totals to
292 on-cart and 345 total assertions, and changing A5 coverage to 13 of 15.
Remove A5.19 from the blocked A5 list while preserving the other entries.

🪄 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: e5e89dcd-80e3-4c2b-a40b-06b0ab599d35

📥 Commits

Reviewing files that changed from the base of the PR and between 5c88457 and eb86bdb.

⛔ 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 (4)

• CHANGELOG.md
• docs/accuracysnes-plan.md
• tests/roms/AccuracySNES/gen/src/dossier.rs
• tests/roms/AccuracySNES/gen/src/tests/cpu.rs

📜 Review details

⏰ Context from checks skipped due to timeout. (5)

• GitHub Check: lint
• GitHub Check: accuracysnes
• GitHub Check: test-light
• GitHub Check: review
• GitHub Check: build demo + docs

🧰 Additional context used

📓 Path-based instructions (11)

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>.md documentation.

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

Files:

• docs/accuracysnes-plan.md
• tests/roms/AccuracySNES/gen/src/dossier.rs
• CHANGELOG.md
• tests/roms/AccuracySNES/gen/src/tests/cpu.rs

**/*

📄 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:

• docs/accuracysnes-plan.md
• tests/roms/AccuracySNES/gen/src/dossier.rs
• CHANGELOG.md
• tests/roms/AccuracySNES/gen/src/tests/cpu.rs

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

**/*.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:

• tests/roms/AccuracySNES/gen/src/dossier.rs
• tests/roms/AccuracySNES/gen/src/tests/cpu.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/cpu.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/dossier.rs
• tests/roms/AccuracySNES/gen/src/tests/cpu.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/cpu.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/cpu.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

🧠 Learnings (6)

📚 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/dossier.rs
• tests/roms/AccuracySNES/gen/src/tests/cpu.rs

📚 Learning: 2026-07-21T05:32:09.180Z

Learnt from: doublegate
Repo: doublegate/RustySNES PR: 197
File: tests/roms/AccuracySNES/gen/src/tests/cpu.rs:0-0
Timestamp: 2026-07-21T05:32:09.180Z
Learning: For SNES LoROM CPU accuracy tests covering JMP (a,X) and JSR (a,X) pointer-bank wrapping, do not place the tested effective pointer addresses below $8000 in LoROM banks $00-$3F or $80-$BF. In RustySNES, those banks’ $0000-$1FFF map to the same 8 KiB WRAM (so different bank forms alias identically), while $2000-$7FFF is mirrored I/O/open-bus; this can make pointer-bank assertions vacuous. Instead, use ROM-resident pointers at linker-controlled, bank-distinct addresses and include a paired control/target case that distinguishes the carried-bank behavior (e.g., for the triggering form ($FFFE,X) with X=$1002, avoid a situation that produces wrapped $00:1000 aliasing with an incorrectly carried $01:1000).

Applied to files:

• tests/roms/AccuracySNES/gen/src/tests/cpu.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.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.rs

🔇 Additional comments (6)

tests/roms/AccuracySNES/gen/src/tests/cpu.rs (4)

98-98: LGTM!

---

2589-2610: LGTM!

---

2612-2649: Doc comment matches the code.

The doc comment claims trace correctly to the implementation: the four-vs-three-byte frame difference, the pushed-first/pulled-last exit frame, the switch-before-push ordering for SH, and the eight-not-sixteen line-wrap rationale all match what the code at Line 2650 onward does. The "~104-dot native span against ~88 emulation" figures reconcile exactly with docs/accuracysnes-plan.md's "293/277 with 189 overhead" figures once the shared instrumentation overhead is subtracted (293-189=104, 277-189=88).

---

2658-2679: 🎯 Functional Correctness

Keep the hardcoded PBR = $00. a5_19 is Group A, and the emitter places Group A in the TESTS segment. lorom.cfg maps TESTS to bank $00, so all four return targets execute in bank $00; no mirroring assumption is required.

			> Likely an incorrect or invalid review comment.

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

86-86: LGTM!

CHANGELOG.md (1)

14-36: LGTM!

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check whether STATUS.md and the to-dos sprint files reference A5.19.
set -euo pipefail

rg -n 'A5\.19' docs/STATUS.md to-dos/*.md 2>/dev/null || echo "No A5.19 reference found in STATUS.md or to-dos/"

Repository: doublegate/RustySNES

Length of output: 207

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- docs/STATUS.md ---'
rg -n -C 3 'A5|accuracysnes|coverage|12|13|15' docs/STATUS.md || true

printf '%s\n' '--- sprint files ---'
rg -n -i -C 2 'A5|accuracysnes|accuracy|19' to-dos --glob '*.md' || true

printf '%s\n' '--- plan and generated coverage references ---'
rg -n -C 2 'A5\.19|12 *of *15|13 *of *15|A5' docs/accuracysnes-plan.md docs/accuracysnes-coverage.md 2>/dev/null || true

Repository: doublegate/RustySNES

Length of output: 50377

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- A5 entries in generated coverage ---'
rg -n -C 2 'A5|Group A|group A|12|13|15' docs/accuracysnes-coverage.md | head -200 || true

printf '%s\n' '--- A5.19 and nearby plan entries ---'
rg -n -C 8 'A5\.1[6-9]|A5\.20|A5\.19' docs/accuracysnes-plan.md || true

printf '%s\n' '--- all ticket-like A5.19 references in tracked files ---'
git grep -n -E 'A5[.]19|A5[ _-]?19' -- ':!snesdev_wiki' || true

printf '%s\n' '--- candidate sprint files ---'
git ls-files 'to-dos/**' | rg -i 'accuracy|phase-6|sprint|roadmap' | head -200

Repository: doublegate/RustySNES

Length of output: 19236

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- phase-6 sprint status ---'
rg -n -C 4 'A5|T-04|T-06|AccuracySNES|coverage|344|345|292|291' \
  to-dos/phase-6-accuracy-to-100/sprint-1-triage.md \
  to-dos/phase-6-accuracy-to-100/overview.md \
  to-dos/ROADMAP.md | head -240 || true

printf '%s\n' '--- exact dashboard totals ---'
sed -n '227,240p' docs/STATUS.md
sed -n '9,18p' docs/accuracysnes-coverage.md
sed -n '432,452p' to-dos/ROADMAP.md

Repository: doublegate/RustySNES

Length of output: 23320

---

Synchronize AccuracySNES coverage metadata.

docs/STATUS.md and to-dos/ROADMAP.md report 291 on-cart and 344 total assertions. The generated coverage reports 292 on-cart and 345 total, with A5 at 13 of 15. Record A5.19 as complete and remove it from the blocked A5 list.

🤖 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 458 - 496, Synchronize the
AccuracySNES coverage metadata by marking A5.19 complete in docs/STATUS.md and
to-dos/ROADMAP.md, updating the totals to 292 on-cart and 345 total assertions,
and changing A5 coverage to 13 of 15. Remove A5.19 from the blocked A5 list
while preserving the other entries.

Source: Coding guidelines

[doublegate]

Review adjudication

No blocking issues. Three suggestions, all declined — two because the thing asked for is already
there, one against the repo's established convention.

Em-dash in the subject line. Declined: this is the repo's convention, not a slip. 21 of the last
200 commit subjects use one, and CHANGELOG.md has 1,089. Conventional Commits constrains the
type(scope): prefix, not the punctuation of the description, and the release tooling
(release-auto.yml, ADR 0007) parses the CHANGELOG section rather than the subject. Switching this
one commit to ASCII would make it the odd one out.

Document why a WRAM stack access is 8 clocks against 6 for an internal cycle. Already documented,
at the exact lines cited:

/// Dots elapsed per 8 extra **stack accesses**: 8 x 8 master clocks / 4 clocks-per-dot.
///
/// The stack lives in WRAM, so a push or pull is an 8-clock access rather than a 6-clock internal
/// cycle. `A5.19`'s extra native cycle is exactly one such pull, so the delta it expects is this
/// and not `DOTS_PER_8_INTERNAL`.
const DOTS_PER_8_STACK_ACCESSES: u16 = 16;

Both the arithmetic (8 x 8 / 4) and the reason (WRAM vs internal) are stated, and the constant is
contrasted with the one it would be wrong to reach for.

Repeated ; -> native (m/x stay 1: still 8-bit) comments in the generated assembly. Declined:
asm/tests_group_a.s is generated output — the file header says so and it must never be hand-edited
— and the comment comes from the Asm::enter_native helper, which emits it at every call site by
design. The repetition is a faithful record that the helper ran three times in this proc, which is
precisely the fact a reader of this test needs: two of those xce pairs are deliberate no-ops that
exist only so the native span matches the emulation one instruction for instruction. Suppressing the
comment on repeat calls would hide the thing worth seeing.