feat(accuracysnes): bless C5.15 — exclude the one undefined hi-res pixel - #324
Conversation
The retraction established that the first hi-res pixel of a line is a genuine reference disagreement (RustySNES + ares emit black, snes9x + Mesen2 the backdrop) on a value ares' own source flags as "not confirmed on hardware". Hashing it would have made EVERY hi-res scene permanently unblessable under ADR 0013 rule 4 over one pixel. `HiResEven` now excludes column 0, so its sample is 255x224 rather than 256x224 -- a deliberate part of the contract, stated in the golden file's header, in the variant's doc, and at both host call sites rather than left to look like an off-by-one. With that column out, all three hosts agree bit-for-bit: snes9x 0xf7ed8ab9ecd95d85 Mesen2 0xf7ed8ab9ecd95d85 RustySNES 0xf7ed8ab9ecd95d85 Rule 4 is satisfied, so the golden is blessed from a render the references agree on. 55 scenes match on both hosts, zero unblessed. Coverage 358 -> 359 of 443 (302 on-cart + 55 scenes + 2 host-side). C5 drops to two uncovered rows, C5.06/C5.07, still blocked by the separate mainscreen-column disagreement. 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 selected for processing (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (8)**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
scripts/accuracysnes/**⚙️ CodeRabbit configuration file
Files:
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:
crates/rustysnes-*/**/*📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.{rs,toml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
crates/**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (4)
WalkthroughChangesAccuracySNES hi-res extraction
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AccuracySNES
participant RustySNES
participant libretro
participant Mesen2
AccuracySNES->>RustySNES: extract HiResEven scene
AccuracySNES->>libretro: extract HiResEven scene
AccuracySNES->>Mesen2: extract HiResEven scene
RustySNES-->>AccuracySNES: return 255-column hash
libretro-->>AccuracySNES: return 255-column hash
Mesen2-->>AccuracySNES: return 255-column hash
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Updates AccuracySNES rendered-scene hashing for hires-even to exclude column 0 (the first hi-res pixel of each scanline), allowing Mode 5 hi-res scene C5.15 to be blessed despite a documented 2-vs-2 reference disagreement on that undefined pixel.
Changes:
- Adjust
hires-evenextraction in the Rust harness and both reference-host implementations (libretro/snes9x C host + Mesen2 Lua host) to start sampling atx=1(i.e., actual column 2 when stepping by 2), making the hashed sample 255×224. - Bless
c5-mode5-hires-16px-tilesby adding its hash totests/golden/accuracysnes-scenes.tsvand updating the golden header contract text. - Update project docs and release notes to reflect 359 / 443 coverage with 55 blessed scenes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/golden/accuracysnes-scenes.tsv | Documents the updated hires-even hashing contract and adds the blessed hash for c5-mode5-hires-16px-tiles. |
| scripts/accuracysnes/mesen_scenes.lua | Starts hires-even hashing at column index 1 to exclude the undefined first hi-res pixel. |
| scripts/accuracysnes/libretro_crossval.c | Starts hires-even hashing at column index 1 to exclude the undefined first hi-res pixel. |
| docs/STATUS.md | Updates the reported AccuracySNES coverage totals to 359 / 443 and 55 scenes. |
| docs/accuracysnes-plan.md | Updates plan totals and notes the new hires-even-extracted scene covering C5.15. |
| docs/accuracysnes-coverage.md | Regenerated coverage totals reflecting C5.15 now covered by a rendered scene. |
| crates/rustysnes-test-harness/tests/accuracysnes_scenes.rs | Implements the new HiResEven first-column exclusion and documents it in-code. |
| CHANGELOG.md | Adds an Unreleased entry describing the C5.15 blessing and the 358 → 359 coverage change. |
| for (unsigned y = 0; y < SCENE_H; y++) { | ||
| const uint8_t *row = (const uint8_t *)d + (size_t)(y + FIRST_ROW) * ystep * p; | ||
| for (unsigned x = 0; x < SCENE_W; x++) { | ||
| for (unsigned x = first_col; x < SCENE_W; x++) { | ||
| unsigned r, g, b; |
| /// 512-wide frame: the even columns — the subscreen half — **starting at column 2**. | ||
| /// | ||
| /// Column 0 is deliberately excluded. It is the first hi-res pixel of the line, and the | ||
| /// references genuinely disagree about it: RustySNES and ares emit black, snes9x and Mesen2 the | ||
| /// backdrop, and ares' own source says *"exact value initializations are not confirmed on |
| # one undefined pixel. Excluding it, all three hosts agree bit-for-bit. Which rule a scene uses is | ||
| # declared in build/scenes.tsv's fourth column (docs/adr/0013, 2026-08-02 supplement). Fixed and |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 23: Update the fenced hash block in CHANGELOG.md by adding the text
language identifier to its opening fence, resolving the markdownlint MD040
violation while leaving the block contents unchanged.
In `@scripts/accuracysnes/libretro_crossval.c`:
- Line 232: Update the scene extraction loop around the x iteration to pack
hires-even pixels contiguously when first_col is 1: write each extracted pixel
at an output index starting from zero rather than its source-column index.
Adjust the related memcpy and fwrite sizes to use the extracted width (255
columns) for hires-even while preserving the existing 256-column behavior for
other modes.
🪄 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: 8ddd2e9f-4856-451f-832f-bc6ade0f4e9c
⛔ Files ignored due to path filters (2)
docs/accuracysnes-coverage.mdis excluded by!docs/accuracysnes-coverage.mdand included bydocs/**tests/golden/accuracysnes-scenes.tsvis excluded by!**/*.tsv,!tests/golden/**and included bytests/**
📒 Files selected for processing (6)
CHANGELOG.mdcrates/rustysnes-test-harness/tests/accuracysnes_scenes.rsdocs/STATUS.mddocs/accuracysnes-plan.mdscripts/accuracysnes/libretro_crossval.cscripts/accuracysnes/mesen_scenes.lua
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: test-light
- GitHub Check: lint
- GitHub Check: accuracysnes
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: review
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (13)
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/STATUS.mddocs/accuracysnes-plan.md
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Chip-behavior changes must update both the chip implementation and the corresponding
docs/<subsystem>.mddocumentation.A chip change must update both the chip implementation and its corresponding
docs/<chip>.mddocumentation in the same change.
Files:
docs/STATUS.mddocs/accuracysnes-plan.mdcrates/rustysnes-test-harness/tests/accuracysnes_scenes.rsCHANGELOG.md
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
docs/STATUS.mddocs/accuracysnes-plan.mdscripts/accuracysnes/mesen_scenes.luascripts/accuracysnes/libretro_crossval.ccrates/rustysnes-test-harness/tests/accuracysnes_scenes.rsCHANGELOG.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/STATUS.mddocs/accuracysnes-plan.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generateddocs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.
Files:
docs/STATUS.mddocs/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/STATUS.mddocs/accuracysnes-plan.mdCHANGELOG.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_FAILURESwith a source citation, never in a widened match.
Files:
scripts/accuracysnes/mesen_scenes.luascripts/accuracysnes/libretro_crossval.c
crates/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Busowns mutable machine state, and the CPU borrows&mut Bus.
Preserve determinism: seed, ROM, and input must produce bit-identical output.
Treat test ROMs as the behavioral specification; when documentation disagrees with passing ROM behavior, update the documentation.
Keepunsafeconfined to existing allowed areas, namely frontend and FFI code, and document everyunsafeblock with a// SAFETY:comment.
Files:
crates/rustysnes-test-harness/tests/accuracysnes_scenes.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use Rust edition 2024 and the toolchain pinned inrust-toolchain.toml(Rust 1.96).
Runcargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy withcargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc becausemissing_docsis a workspace lint.
Do not runcargo clippy --all-features;scriptingandscript-wasmare mutually exclusive. Use explicit per-feature jobs instead.
**/*.rs: Do not introduce.unwrap(),.expect(), orpanic!()on untrusted external input—such as ROM/save-state bytes, netplay messages, Lua or scripting input, or user-supplied paths—outside#[cfg(test)]code. Use typed errors at those boundaries; locally constructed values or values immediately protected by a checked invariant are allowed.
Every newunsafe { ... }block orunsafe fnmust have an adjacent// SAFETY:comment naming the relied-on invariant and its guarantor. Unsafe code outside the frontend and FFI shims should additionally be questioned becauseunsafe_codeis a workspace lint.
**/*.rs: Use Rust edition 2024 with the pinned 1.96 toolchain; satisfy workspacepedantic,nursery,missing_docs, andunsafe_codewarnings because CI runs with-D warnings. Document every public item.
Keepunsafecode restricted to the frontend and FFI, and include a// SAFETY:justification for each use.
Keep hot paths allocation-free.
Treatrustysnes_core::Busas the owner of mutable emulator state; the CPU borrows&mut Bus.
Use the master clock at 21477270 Hz as the timing master; advance the scheduler in lockstep and run other chips on their divisors.
Maintain determinism: seed, ROM, and input must produce bit-identical audio/video; frontend rate control must not alter emulation results.
When implementing hardware behavior, pin and run the failing test ROM first; treat test ROMs as the specification.
Files:
crates/rustysnes-test-harness/tests/accuracysnes_scenes.rs
crates/rustysnes-*/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, any observable behavior change under
crates/rustysnes-<chip>/must be accompanied by an edit to the matchingdocs/<chip>.md; a crate change passes without documentation only when it does not alter observable behavior, with the non-behavioral change stated explicitly.
Files:
crates/rustysnes-test-harness/tests/accuracysnes_scenes.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_scenes.rs
crates/**
⚙️ CodeRabbit configuration file
crates/**: Emulator core. Hot paths are allocation-free;unsaferequires a// SAFETY:comment
naming the invariant. Any change to save-stated fields needs aFORMAT_VERSIONbump and a
docs/adr/0006bump-log entry. Behavior changes must update the matchingdocs/<chip>.md
in the same change.
Files:
crates/rustysnes-test-harness/tests/accuracysnes_scenes.rs
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.For the full pull request diff against its base branch, modify
CHANGELOG.mdwhen user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.
Files:
CHANGELOG.md
🧠 Learnings (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
🪛 LanguageTool
docs/STATUS.md
[style] ~244-~244: Try using a descriptive adverb here.
Context: ...he assertion), kept as separate columns on purpose (docs/accuracysnes-coverage.md, regen...
(ON_PURPOSE_DELIBERATELY)
🪛 markdownlint-cli2 (0.23.1)
CHANGELOG.md
[warning] 23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
CHANGELOG.md (1)
14-22: LGTM!Also applies to: 24-31
docs/STATUS.md (1)
26-27: LGTM!Also applies to: 244-244
docs/accuracysnes-plan.md (1)
17-18: 🗄️ Data Integrity & IntegrationRetain the rendered-scene coverage statement. The generated report records 55 scene-only assertion rows, and all 55 declared scenes are blessed.
> Likely an incorrect or invalid review comment.crates/rustysnes-test-harness/tests/accuracysnes_scenes.rs (1)
74-82: LGTM!Also applies to: 192-198, 210-211
scripts/accuracysnes/libretro_crossval.c (1)
189-194: LGTM!scripts/accuracysnes/mesen_scenes.lua (1)
145-150: LGTM!Also applies to: 162-162
| part of the contract, recorded in the golden file's own header and in the variant's doc. With that | ||
| one column out, all three hosts agree bit-for-bit: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced hash block.
markdownlint reports MD040 for Line 23. Add text to the opening fence.
Proposed fix
- ```
+ ```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@CHANGELOG.md` at line 23, Update the fenced hash block in CHANGELOG.md by
adding the text language identifier to its opening fence, resolving the
markdownlint MD040 violation while leaving the block contents unchanged.
Sources: Path instructions, Linters/SAST tools
Bot review, both suggestions accepted; no blocking issues. All three hosts now loop over the SAMPLE index and derive the source column from it, rather than looping over source columns. That makes the C, Lua and Rust implementations line up statement for statement, which matters more here than it looks: cross-host drift inside this loop is the one thing a golden cannot detect -- every host would produce a stable, reproducible hash of its own slightly different region. Also: the Rust `px` capacity is `sample_w * SCENE_H` rather than `SCENE_W * SCENE_H`, so the allocation states the sample size instead of over-reserving by a row's worth under HiResEven. VERIFIED AS A PURE REFACTOR, which is the only thing that matters for a change that touches the hashing loop: scene19 still hashes 0xf7ed8ab9ecd95d85 on snes9x and Mesen2, and 55 scenes match on both hosts with zero unblessed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both accepted; no blocking issues. The indexing one is worth more than a readability nit. All three hosts now loop over the sample index and derive the source column, rather than looping over source columns — so the C, Lua and Rust implementations line up statement for statement. Cross-host drift inside this loop is the one thing a golden cannot detect: each host would produce a stable, reproducible hash of its own slightly different region, and the gate would report a mismatch with no indication which host was wrong. Making the three loops structurally identical is the cheapest defence against that. The Verified as a pure refactor, which is the only thing that matters when the change touches the hashing loop: |
Antigravity review (Gemini via Ultra)This PR modifies the AccuracySNES scene hashing harness across Rust, C, and Lua to exclude column 0 under Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
The retraction in #323 established that the first hi-res pixel of a line is a genuine reference disagreement — RustySNES and ares emit black, snes9x and Mesen2 the backdrop — on a value ares' own source flags as "exact value initializations are not confirmed on hardware".
Left as it was, that one pixel would have made every hi-res scene permanently unblessable under ADR 0013 rule 4. That is a bad trade for an undefined value.
The fix
HiResEvennow excludes column 0, so its sample is 255×224 rather than 256×224. That is a deliberate part of the contract, not an off-by-one, and it is stated in three places so it reads as intentional wherever someone lands: the golden file's header, theExtractvariant's doc, and both host call sites.With that column out, all three hosts agree bit-for-bit:
Rule 4 is satisfied — the golden is blessed from a render the references agree on, which is the whole point of the rule.
Result
Coverage 358 → 359 of 443 (302 on-cart + 55 scenes + 2 host-side).
C5drops to two uncovered rows —C5.06/C5.07, still blocked by the separate mainscreen-column disagreement, which is a different and much larger divergence (33–35% pairwise) than this one-pixel case.cargo fmt --check, workspace clippy at-D warnings, and the gate's exactcargo doccommand all clean. No existing golden moved.🤖 Generated with Claude Code
Summary
HiResEvennow excludes undefined column 0 from hi-res scene samples. The sample size changes from 256×224 to 255×224.This change claims that
C5.15is valid when columns 1–255 are hashed. The claim is false if column 0 is defined or required for scene validation, or if snes9x, Mesen2, and RustySNES do not produce hash0xf7ed8ab9ecd95d85.AccuracySNES assertion
C5.15is added to coverage. Coverage increases from 358 to 359 of 443 assertions. The denominator remains 443.C5.06andC5.07remain uncovered because of a separate main-screen column disagreement.