feat(accuracysnes): E9.09 echo wraps into page zero; fix the ares gate - #312
Conversation
…es gate The echo write pointer is a 16-bit address with nothing clamping it at $FFFF, so a buffer placed too high carries on into page zero. It is an errata because the failure is remote from its cause: the sound driver corrupts variables it never addressed. The arrangement is the design, not the instrument. ESA = $FF with a 2 KiB buffer wraps after 256 bytes and then writes 1,792 bytes of low RAM -- the directory, the stack, and the running program -- so the test would destroy itself, by an amount a timing window decides. ESA = $F9 with the same length ends the buffer exactly at $FFFF, so the wrapped part is page zero and precisely page zero, and nothing else is in reach however long it runs. The echo offset free-runs whether or not FLG bit 5 has writes enabled, so the starting phase is unknowable; the row waits 768 samples against a 512-sample buffer, which covers every entry for every phase. With EFB = 0 and a constant sample every entry holds the identical four bytes, so the assertion is that the wrapped entry EQUALS the unwrapped one rather than merely that something changed. The program repaints page zero before releasing the APU. Injecting (page << 8) | (offset & $FF) at echo22 fires code 2, the code that names a pointer masked inside the ESA page; the guard passes. Coverage 351 -> 352 of 443 (298 on-cart + 54 scenes), battery 339 tests at 100% on-cart, three references agree. Adding it also made ares' F1.10 verdict flip run to run on one image (eight of eight failing before, then two of five passing). F1.10 samples $4212 at the vblank edge, so anything ahead of Group F moves the phase that decides it -- the same trap that flipped Mesen2's verdict on the same row when E3.06 was rewritten. ARES_KNOWN_FAILURES drops 4 -> 3 and crossval.sh excludes the row by ID resolved through SOURCE_CATALOG.tsv at run time, because the catalogue index is exactly what moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (8)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used📓 Path-based instructions (16)crates/**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.rs📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,md}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
crates/rustysnes-*/**/*📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.{rs,toml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
crates/**⚙️ CodeRabbit configuration file
Files:
docs/**/*.md📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
docs/**/*📄 CodeRabbit inference engine (docs/testing-strategy.md)
Files:
docs/**⚙️ CodeRabbit configuration file
Files:
**/*.md⚙️ CodeRabbit configuration file
Files:
tests/roms/AccuracySNES/gen/src/**/*📄 CodeRabbit inference engine (Custom checks)
Files:
tests/roms/AccuracySNES/gen/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/roms/AccuracySNES/gen/src/**⚙️ CodeRabbit configuration file
Files:
scripts/accuracysnes/**⚙️ CodeRabbit configuration file
Files:
CHANGELOG.md📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
🧠 Learnings (5)📚 Learning: 2026-07-21T02:10:49.581ZApplied to files:
📚 Learning: 2026-07-21T01:34:22.909ZApplied to files:
📚 Learning: 2026-07-21T05:22:58.848ZApplied to files:
📚 Learning: 2026-07-21T06:21:34.629ZApplied to files:
📚 Learning: 2026-07-22T06:12:00.703ZApplied to files:
🪛 LanguageTooldocs/STATUS.md[style] ~244-~244: Try using a descriptive adverb here. (ON_PURPOSE_DELIBERATELY) scripts/accuracysnes/ares_host/README.md[style] ~111-~111: Consider an alternative for the overused word “exactly”. (EXACTLY_PRECISELY) 🔇 Additional comments (13)
WalkthroughThe PR adds the AccuracySNES E9.09 echo-buffer wrapping test and updates its coverage records. It also excludes nondeterministic ares result F1.10 by test ID, with runtime catalogue lookup and updated documentation. ChangesAccuracySNES validation updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant e909_test
participant echo_buffer
participant page_zero
e909_test->>page_zero: Paint sentinel bytes
e909_test->>echo_buffer: Configure and run echo cycle
echo_buffer->>page_zero: Write after 16-bit pointer wrap
e909_test->>page_zero: Compare wrapped and in-buffer bytes
e909_test->>page_zero: Restore RAM
Possibly related PRs
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new AccuracySNES APU/DSP assertion (E9.09) that verifies the S-DSP echo write pointer is 16-bit and wraps past $FFFF into page zero, and hardens the ares cross-validation gate by excluding the now-demonstrated-to-be-phase-fragile F1.10 row by ID (resolved via SOURCE_CATALOG.tsv) rather than by catalogue index.
Changes:
- Add
E9.09test implementation to the AccuracySNES generator sources and wire it into the dossier mapping and measurement reporting. - Regenerate AccuracySNES committed artifacts (assembly, catalog, error-code docs, coverage counts) to include the new row and updated totals.
- Stabilize ares gate in
crossval.shby excluding the unstable row by name/ID at runtime and adjustingARES_KNOWN_FAILURES.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/roms/AccuracySNES/SOURCE_CATALOG.tsv | Regenerated catalog entry list to include E9.09 and shifted indices/addresses accordingly. |
| tests/roms/AccuracySNES/gen/src/tests/apu.rs | Adds the E9.09 generator-side test logic and helpers, plus measurement-slot records. |
| tests/roms/AccuracySNES/gen/src/dossier.rs | Maps dossier assertion E9.09 to the new cart row. |
| tests/roms/AccuracySNES/ERROR_CODES.md | Documents new E9.09 failure codes (generated output). |
| tests/roms/AccuracySNES/asm/tests_group_a.s | Regenerated assembled test bodies/metadata to include test_e9_09 and updated program indices. |
| scripts/accuracysnes/crossval.sh | Excludes F1.10 from ares gate by resolving its catalog index from SOURCE_CATALOG.tsv at runtime; updates expected known-failure count. |
| scripts/accuracysnes/ares_host/README.md | Documents the rationale for excluding F1.10 from the ares gate due to non-reproducibility. |
| docs/STATUS.md | Updates project status metrics to reflect 352/443 covered assertions and 339-test battery. |
| docs/accuracysnes-plan.md | Updates plan/metrics and notes around the new assertion and the ares gating change. |
| docs/accuracysnes-coverage.md | Regenerated coverage totals and uncovered list to account for E9.09 now being covered. |
| crates/rustysnes-test-harness/tests/accuracysnes.rs | Extends Group E batch measurement slot reporting to include the new E9.09 slots. |
| CHANGELOG.md | Documents the new E9.09 addition and the ares gate stabilization for F1.10. |
| /// Whole buffer cycles to wait, over the one the wrap needs. 512 samples each. | ||
| const CYCLES_WAITED: u8 = 16; |
Bot review, three of four accepted. CYCLES_WAITED was a wrong name attached to a right number: 16 is delay BLOCKS, not buffer cycles, and 16 * 48 = 768 samples against a 512-sample buffer. The constant now says so, which is the fact that makes the coverage phase-free. The ares exclusion compares $2 + 0 against skip + 0. Both sides are already strnum in practice, so this changes nothing measurable (3 across three runs before and after) -- it removes the need to know that. The ROM is byte-identical: checksum stays $9AB2. Declined: deriving SOURCE_CATALOG.tsv's path from BASH_SOURCE. The script cd's to the repo root at line 36 and every path in it is root-relative by design, ROM= included; the catalogue is the same shape as everything around it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
doublegate
changed the title
|
Bot review adjudicated — three accepted, one declined. Accepted.
The ROM is byte-identical across the rename: checksum stays Declined: deriving |
Antigravity review (Gemini via Ultra)This PR adds the Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
E9.09— the echo write pointer wraps at the 16-bit boundaryESAnames a page,EDLa length, and the address is computed in sixteen bits with nothing clamping it at$FFFF. A driver that setsESAtoo high does not get a short buffer or a dropped write — it gets the DSP writing four bytes per sample over the bottom of its own RAM. It is an errata because the failure is remote from its cause: the sound driver corrupts variables it never addressed.The arrangement is the design, not the instrument
The obvious setup,
ESA = $FFwith a 2 KiB buffer, wraps after 256 bytes and then writes 1,792 bytes of low RAM — the sample directory, the stack, and the running program at$0200. The test destroys itself, and how much it destroys depends on how long it ran, which is a timing window the row has no reason to need.ESA = $F9with the same length ends the buffer exactly at$FFFF:$F900$FFFC— the last entry below the boundary$0000— the wrap$00FC— the last entry, then the offset resetsSo the wrapped part is page zero and precisely page zero. Nothing else is in reach however long it runs.
No phase to get wrong
The echo offset free-runs whether or not
FLGbit 5 has writes enabled — the flag gates the store, not the pointer — so the starting phase is unknowable. The row waits 768 samples against a 512-sample buffer, so every entry is written for every phase.With
EFB = 0and a constant sample, every entry holds the identical four bytes, which is what makes the row a comparison: the wrapped entry must equal the unwrapped one at$F901, not merely differ from the paint. Page zero is painted$A5first so "never written" is distinguishable from "written as zero", and the program repaints it with zeroes before releasing the APU.Inject-verified at the site it names
(page << 8) | (offset & $FF)atdsp.rs'secho22→ code 2, the code that names a pointer masked inside the ESA page. The guard (code 1, "echo wrote the buffer at all") passes.dsp.rsrestored byte-clean from a pre-edit snapshot afterwards.Fixed:
F1.10's ares verdict is not reproducibleAdding
E9.09— an APU row with nothing to do with controller ports — made ares' verdict onF1.10flip run to run:04on eight runs of eight before, then01on two of five and04on the other three, same binary and same image. The row samples$4212at the vblank edge, so anything added ahead of Group F moves the phase that decides it.This is the second time on this exact row — Mesen2's
F1.10verdict flipped the other way whenE3.06was rewritten. Two of three references now demonstrate the row's cross-host verdict carries no information.A count that includes it is a gate that flakes on unrelated work.
ARES_KNOWN_FAILURESdrops 4 → 3, andcrossval.shresolves the excluded row throughSOURCE_CATALOG.tsvby ID at run time — the catalogue index moves whenever a test is added ahead of it, which is exactly what exposed the problem. Reproducible across runs again (5/5). RustySNES passes the row because of a deliberate fix and that stands.Verification
Cross-validation:
snes9x: OK (14 known),Mesen2: OK (1 known),ares: OK (3 known), 54/54 scenes on both scene hosts,3 reference(s) agree with the cart.Coverage 351 → 352 of 443 (298 on-cart + 54 scenes); battery 339 tests, 100% on-cart.
🤖 Generated with Claude Code
Summary
Adds AccuracySNES assertion
E9.09. It claims that the DSP echo write pointer wraps at the 16-bit boundary into page zero whenESA = $F9and the echo buffer is 2 KiB.The claim is false if the pointer masks, clamps, or otherwise avoids page-zero writes. The test compares wrapped and unwrapped buffer entries across independent phases.
Updates the Ares cross-validation gate to exclude unstable assertion
F1.10by runtime ID lookup. The known-failure count changes from 4 to 3.AccuracySNES coverage increases from 351 to 352 of 443 assertions. The coverage denominator remains 443.