feat(accuracysnes): per-scene capture extraction, and the Mode-5 divergence it found - #322
Conversation
…rgence it found Hi-res scenes were parked behind "widen the capture region". Measuring showed that framing was wrong: the hosts do not agree on the SHAPE of a hi-res frame -- snes9x emits 512x224, Mesen2 512x478 because it line-doubles -- so what is needed is not a bigger hash but a rule for reducing whatever a host emits to the canonical 256x224 sample. `Scene` gains an `extract` field (Direct | HiResEven), emitted as build/scenes.tsv's fourth column so the rule travels with the scene rather than being compiled into any host. All three hosts honour it, and a host meeting a rule it does not implement REJECTS the scene rather than falling back to Direct -- falling back would silently hash the left half of a hi-res picture, which is the failure the exact-geometry checks were just added to stop. The declaration is also not INFERRED from the geometry, deliberately: inferring would let a scene that is supposed to be hi-res, but which a broken core rendered 256 wide, be hashed as Direct and match a Direct golden. The declared rule is what turns that into a failure. The first hi-res scene (c5-mode5-hires-16px-tiles, C5.15) diverged on its first run: snes9x 512x224 0xbcb8d1c2bec08325 Mesen2 512x478 0xbcb8d1c2bec08325 RustySNES 512 wide 0xd8dad9b9cb91e325 The two references agree BIT-FOR-BIT despite different geometries and entirely different extraction paths, which is this project's signature for a real defect rather than a broken test. Consistent with STATUS.md already recording hi-res as "real-title validation still open"; this is the first automated evidence of what that gap is. The scene is left UNBLESSED deliberately. ADR 0013 rule 4 would permit blessing at the reference value, but that turns the scene gate red on a live finding -- an unblessed scene does not fail the gate, so the finding is preserved without taking the tree red. Blessing follows the fix. 54 blessed scenes still match on both hosts; battery unchanged at 100% on-cart; three references agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughChangesThe AccuracySNES scene manifest now includes per-scene framebuffer extraction metadata. RustySNES, libretro, and Mesen2 apply AccuracySNES extraction
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SceneManifest
participant CaptureHost
participant Framebuffer
SceneManifest->>CaptureHost: provide scene extraction rule
CaptureHost->>Framebuffer: validate mode-specific geometry
Framebuffer-->>CaptureHost: provide captured pixels
CaptureHost->>CaptureHost: sample and hash selected pixels
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (6 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends the AccuracySNES rendered-scene oracle contract by adding a per-scene extraction rule (direct vs hires-even) to the scene manifest so each host (Rust harness, libretro C, Mesen2 Lua) can deterministically reduce its emitted framebuffer into the canonical 256×224 sample without hard-coding host-specific behavior.
Changes:
- Add
Extract(Direct|HiResEven) to scene definitions and emit it as the 4th column inbuild/scenes.tsv. - Teach all three hosts (Rust test harness, libretro crossval host, Mesen2 Lua script) to parse and apply extraction rules (and reject unknown values).
- Add the first hi-res Mode 5 scene (
C5.15) and document the immediate cross-host divergence it exposed.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/roms/AccuracySNES/gen/src/scenes.rs | Adds Extract, threads it into Scene, adds a Mode 5 hi-res scene, and emits extraction tags into the manifest. |
| tests/roms/AccuracySNES/build/scenes.tsv | Updates the generated manifest to include the new extract column and the new scene entry. |
| tests/roms/AccuracySNES/asm/scenes.s | Adds the Mode 5 scene implementation and updates the scene count/table. |
| scripts/accuracysnes/mesen_scenes.lua | Loads per-scene extraction tags and applies them during hashing for hi-res scenes. |
| scripts/accuracysnes/libretro_crossval.c | Loads per-scene extraction tags and applies them during hashing for hi-res scenes. |
| docs/adr/0013-accuracysnes-framebuffer-oracle.md | Documents the extraction rule design and records the newly observed Mode 5 divergence. |
| docs/accuracysnes-coverage.md | Adds the new scene (C5.15) to the coverage list. |
| crates/rustysnes-test-harness/tests/accuracysnes_scenes.rs | Parses extraction tags from the manifest and applies them to hashing in the in-repo harness. |
| CHANGELOG.md | Records the new extraction contract and the Mode 5 divergence finding. |
| * THIS cart's numbering. A missing file leaves everything `direct`, which is what every scene was | ||
| * before hi-res; an UNKNOWN rule is fatal, because falling back to `direct` would silently hash the | ||
| * left half of a hi-res picture and produce a golden that looks fine. */ | ||
| static void load_extractions(const char *rom_path, unsigned *out, unsigned n) { | ||
| for (unsigned i = 0; i < n; i++) { | ||
| out[i] = EXTRACT_DIRECT; | ||
| } | ||
| char path[1024]; | ||
| const char *slash = strrchr(rom_path, '/'); | ||
| size_t dir = slash ? (size_t)(slash - rom_path) + 1 : 0; | ||
| if (dir + strlen("scenes.tsv") + 1 > sizeof path) { | ||
| return; | ||
| } | ||
| memcpy(path, rom_path, dir); | ||
| strcpy(path + dir, "scenes.tsv"); | ||
| FILE *f = fopen(path, "r"); | ||
| if (!f) { | ||
| fprintf(stderr, "accuracysnes: no %s; every scene treated as `direct`\n", path); | ||
| return; | ||
| } |
| if not f then | ||
| return | ||
| end |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/accuracysnes/libretro_crossval.c (1)
181-200: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject 256x448 frames for
EXTRACT_DIRECT.If a direct scene emits a 256x448 frame, Line 183 sets
ystepto two and Line 200 accepts the frame. The host then hashes alternate rows instead of rejecting geometry that does not match the direct 256x224 contract.Restrict doubled-height sampling to
EXTRACT_HIRES_EVEN.Proposed fix
- const unsigned ystep = (d && h >= (SCENE_H + FIRST_ROW) * 2u) ? 2u : 1u; + const unsigned ystep = + (want_extract == EXTRACT_HIRES_EVEN && h >= (SCENE_H + FIRST_ROW) * 2u) ? 2u : 1u;🤖 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/libretro_crossval.c` around lines 181 - 200, Update the ystep calculation near the extraction geometry checks so doubled-height sampling is enabled only when want_extract equals EXTRACT_HIRES_EVEN; keep EXTRACT_DIRECT at a single-row step, causing 256x448 direct frames to fail the exact-height validation and be rejected before hashing.
🤖 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 `@scripts/accuracysnes/libretro_crossval.c`:
- Around line 130-159: Make both manifest loaders fail rather than defaulting to
direct extraction: in scripts/accuracysnes/libretro_crossval.c, update
load_extractions to exit failure when scenes.tsv is unavailable, when a
non-comment row fails the four-field sscanf extraction declaration, or when its
scene index is invalid; in scripts/accuracysnes/mesen_scenes.lua, apply the same
failure behavior for an unavailable manifest or any non-comment row lacking a
valid four-field extraction declaration.
In `@tests/roms/AccuracySNES/gen/src/scenes.rs`:
- Around line 597-621: Add the missing C5.15 coverage mapping in dossier.rs::MAP
and create its corresponding golden entry using the existing C5.15 scene
identifier and expected output conventions. Regenerate
docs/accuracysnes-coverage.md and docs/accuracysnes-plan.md so C5.15 is covered
and the blessed-scene total is updated from 54 to 55.
---
Outside diff comments:
In `@scripts/accuracysnes/libretro_crossval.c`:
- Around line 181-200: Update the ystep calculation near the extraction geometry
checks so doubled-height sampling is enabled only when want_extract equals
EXTRACT_HIRES_EVEN; keep EXTRACT_DIRECT at a single-row step, causing 256x448
direct frames to fail the exact-height validation and be rejected before
hashing.
🪄 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: f4568a26-0854-4f39-addb-c29e15eb5bc7
⛔ Files ignored due to path filters (5)
docs/accuracysnes-coverage.mdis excluded by!docs/accuracysnes-coverage.mdand included bydocs/**tests/roms/AccuracySNES/asm/scenes.sis excluded by!tests/roms/AccuracySNES/asm/scenes.sand included bytests/**tests/roms/AccuracySNES/build/accuracysnes-pal.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**tests/roms/AccuracySNES/build/accuracysnes.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**tests/roms/AccuracySNES/build/scenes.tsvis excluded by!**/*.tsv,!tests/roms/AccuracySNES/build/**and included bytests/**
📒 Files selected for processing (6)
CHANGELOG.mdcrates/rustysnes-test-harness/tests/accuracysnes_scenes.rsdocs/adr/0013-accuracysnes-framebuffer-oracle.mdscripts/accuracysnes/libretro_crossval.cscripts/accuracysnes/mesen_scenes.luatests/roms/AccuracySNES/gen/src/scenes.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: test-light
- GitHub Check: accuracysnes
- GitHub Check: lint
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: review
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (18)
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/adr/0013-accuracysnes-framebuffer-oracle.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/adr/0013-accuracysnes-framebuffer-oracle.mdCHANGELOG.mdcrates/rustysnes-test-harness/tests/accuracysnes_scenes.rstests/roms/AccuracySNES/gen/src/scenes.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.mdscripts/accuracysnes/mesen_scenes.luaCHANGELOG.mdcrates/rustysnes-test-harness/tests/accuracysnes_scenes.rsscripts/accuracysnes/libretro_crossval.ctests/roms/AccuracySNES/gen/src/scenes.rs
docs/adr/**/*
📄 CodeRabbit inference engine (docs/adr/0004-determinism-contract.md)
docs/adr/**/*: The emulator must produce bit-identical framebuffer and audio for the same seed, ROM, and input sequence.
Power-on CPU, PPU, and SMP phase alignment must come from a seeded PRNG, and reset must preserve that alignment.
Track the SPC700 asynchronous domain using the integer relative-time accumulator defined by ADR 0001; do not use floating-point timing for this purpose.
The core must not use system time, thread scheduling, or OS RNG as sources of nondeterminism.
Use a fixed nominal 1.024 MHz SPC domain in the deterministic path; resonator drift must be excluded. Any hardware-accurate drift toggle belongs in the frontend, is off by default, and must remain outside the bit-identical path.
RTC chips such as S-RTC and SPC7110's RTC-4513 must use fixed or seeded time and must never read the host wall clock.
Rate control and run-ahead must be implemented in the frontend, never in core synthesis; netplay rollback must be frontend-orchestrated against the deterministic core.
Core code must not introduce hidden nondeterminism, including host time, thread scheduling, OS RNG, or HashMap iteration order.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.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/adr/0013-accuracysnes-framebuffer-oracle.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/adr/0013-accuracysnes-framebuffer-oracle.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/adr/0013-accuracysnes-framebuffer-oracle.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
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
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.rstests/roms/AccuracySNES/gen/src/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.rstests/roms/AccuracySNES/gen/src/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
tests/roms/AccuracySNES/gen/src/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, when a test or scene is added or removed under
tests/roms/AccuracySNES/gen/src/, verify itsdossier.rs::MAPentry, all required regenerated artifacts, and matching count changes indocs/accuracysnes-plan.md. Artifact presence must be judged from the path-filter exclusion list, not from unreadable contents.
Files:
tests/roms/AccuracySNES/gen/src/scenes.rs
tests/roms/AccuracySNES/gen/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Rebuild AccuracySNES after any change to
gen/orasm/; never hand-edit generatedasm/tests_group_a.sorasm/scenes.s.
Files:
tests/roms/AccuracySNES/gen/src/scenes.rs
tests/roms/AccuracySNES/gen/src/**
⚙️ CodeRabbit configuration file
tests/roms/AccuracySNES/gen/src/**: This generates a hardware-accuracy test cartridge. Judge each test by whether it can
distinguish the behavior it names from the alternatives, not by whether it passes.Flag, specifically:
- Vacuity. An assertion whose expected value is also what a broken or absent
implementation produces (zero, "unchanged", "not $FF") needs a paired control assertion
that would fail on that implementation. Say which alternative goes uncaught.- Overstated doc comments. The prose above a test is a claim about what it validates.
If it names behaviors the emitted program does not exercise, or asserts a rationale that
is not true of the code, that is a defect even though the test passes.- Shared state. OAM, CGRAM, VRAM and the S-DSP registers are not reset between tests. A
test that does not establish its own starting conditions may be measuring the previous
one; look for an earlier test that leaves the relevant state dirty.- Timing-marginal reads. Reading a register a few cycles after disturbing it, or
asserting on a value that is still moving, produces a verdict that flips when unrelated
code shifts. Prefer a settle, a disarm, or a provably stationary value.- Scanline geometry. Line 0 is a blanking line; the V counter's low byte aliases on a
312-line PAL frame; the visible height is 224 or 239 depending on overscan. Constants
derived from any of these deserve a second look.- Duplicate coverage.
dossier.rs::MAPmust not claim an assertion another test already
implements. There is a build gate for this, but flag it in review too.
Files:
tests/roms/AccuracySNES/gen/src/scenes.rs
tests/roms/AccuracySNES/gen/src/scenes.rs
⚙️ CodeRabbit configuration file
tests/roms/AccuracySNES/gen/src/scenes.rs: Rendered scenes are hashed framebuffers. A scene proves nothing unless the picture it draws
differs from every other scene's — a stable hash that three emulators agree on is not
evidence, because a scene that renders the wrong thing is equally stable and equally agreed.
Flag any scene whose setup may not be able to express the difference it claims: a periodic
canvas that hides a scroll, a square sprite where a tall one was meant, a register the mode
ignores.
Files:
tests/roms/AccuracySNES/gen/src/scenes.rs
🧠 Learnings (1)
📚 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/scenes.rs
🪛 ast-grep (0.45.0)
scripts/accuracysnes/libretro_crossval.c
[error] 140-140: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: strcpy(path + dir, "scenes.tsv")
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-c)
🪛 OpenGrep (1.26.0)
scripts/accuracysnes/libretro_crossval.c
[WARNING] 141-141: strcpy() has no bounds checking and can lead to buffer overflow. Use strncpy() or strlcpy() with an explicit size instead.
(coderabbit.buffer-overflow.c-strcpy)
🔇 Additional comments (6)
CHANGELOG.md (1)
73-101: LGTM!docs/adr/0013-accuracysnes-framebuffer-oracle.md (1)
181-204: LGTM!tests/roms/AccuracySNES/gen/src/scenes.rs (1)
34-67: LGTM!Also applies to: 81-82, 109-595, 655-2043, 2531-2540
crates/rustysnes-test-harness/tests/accuracysnes_scenes.rs (1)
65-105: LGTM!Also applies to: 154-192, 233-256
scripts/accuracysnes/libretro_crossval.c (1)
467-498: LGTM!scripts/accuracysnes/mesen_scenes.lua (1)
134-165: LGTM!Also applies to: 229-235
| Scene { | ||
| id: "c5-mode5-hires-16px-tiles", | ||
| dossier: "C5.15", | ||
| what: "Mode 5, whose tiles are SIXTEEN pixels wide rather than eight — the canvas font \ | ||
| rendered at double width across a 512-pixel picture. The first scene to declare a \ | ||
| non-`Direct` extraction: the hosts do not agree on the SHAPE of a hi-res frame \ | ||
| (snes9x emits 512x224, Mesen2 512x478 because it line-doubles), so the sample is \ | ||
| the EVEN columns — the subscreen half, the one all three references agree on — and \ | ||
| the even rows where the height doubled too. See the 2026-08-02 supplement in \ | ||
| `docs/adr/0013`.", | ||
| setup: &[ | ||
| "sep #$20", | ||
| "lda #$05", | ||
| "sta $2105 ; BGMODE 5 — 512-wide hi-res, 16-px-wide tiles", | ||
| "stz $210B", | ||
| "lda #(MAP_BASE >> 8)", | ||
| "sta $2107", | ||
| "jsr scene_low_tiles ; the canvas map indexes past the font", | ||
| "sep #$20", | ||
| "lda #$01", | ||
| "sta $212C ; BG1 only", | ||
| "lda #$0F", | ||
| "sta $2100", | ||
| ], | ||
| extract: Extract::HiResEven, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'C5\.15|c5-mode5-hires-16px-tiles' \
tests/roms/AccuracySNES/gen/src/scenes.rs \
tests/roms/AccuracySNES/gen/src/dossier.rs \
docs/accuracysnes-plan.md
rg -n -C 4 'path-filter|exclude|scenes\.tsv|tests_group_a\.s|scenes\.s' \
tests/roms/AccuracySNES
fd -t f . tests/roms/AccuracySNES/build tests/roms/AccuracySNES/gen | sortRepository: doublegate/RustySNES
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- C5.15 references ---'
rg -n -C 5 'C5\.(1[2345])|c5-mode5-hires-16px-tiles|scenes\.tsv|accuracysnes-scenes\.tsv' \
tests/roms/AccuracySNES/gen/src/dossier.rs \
tests/roms/AccuracySNES/gen/src/scenes.rs \
docs/accuracysnes-plan.md \
tests/roms/AccuracySNES/SOURCE_CATALOG.tsv \
tests/roms/AccuracySNES/build/scenes.tsv \
golden/accuracysnes-scenes.tsv || true
printf '%s\n' '--- relevant AccuracySNES files ---'
git ls-files tests/roms/AccuracySNES | rg '(^|/)(build|asm|gen|SOURCE_CATALOG|ERROR_CODES)|accuracysnes-plan' | sort
printf '%s\n' '--- repository state ---'
git status --short
git diff --statRepository: doublegate/RustySNES
Length of output: 14768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dossier MAP and coverage checks ---'
rg -n -C 8 'static MAP|const MAP|MAP:|C5\.(0[1-9]|1[0-9])|scene_assertions|scene_coverage|blessed' \
tests/roms/AccuracySNES/gen/src/dossier.rs
printf '%s\n' '--- generated scene and artifact counts ---'
python3 - <<'PY'
from pathlib import Path
for name in [
"tests/roms/AccuracySNES/build/scenes.tsv",
"tests/roms/AccuracySNES/asm/scenes.s",
"tests/roms/AccuracySNES/build/accuracysnes.sfc",
"tests/roms/AccuracySNES/build/accuracysnes-pal.sfc",
"tests/roms/AccuracySNES/build/accuracysnes-hirom.sfc",
"tests/roms/AccuracySNES/build/accuracysnes-exhirom.sfc",
]:
p = Path(name)
print(f"{name}: {'present' if p.exists() else 'missing'}", end="")
if p.exists():
print(f", bytes={p.stat().st_size}")
else:
print()
p = Path("tests/roms/AccuracySNES/build/scenes.tsv")
if p.exists():
rows = [x for x in p.read_text().splitlines() if x.strip() and not x.lstrip().startswith("#")]
print(f"scenes.tsv data rows: {len(rows)}")
print("C5.15 rows:", [x for x in rows if "C5.15" in x])
p = Path("tests/roms/AccuracySNES/asm/scenes.s")
if p.exists():
text = p.read_text()
print("asm/scenes.s C5.15 occurrences:", text.count("C5.15"))
print("asm/scenes.s scene-id occurrences:", text.count("c5-mode5-hires-16px-tiles"))
PY
printf '%s\n' '--- plan count statements near scene status ---'
sed -n '2400,2440p' docs/accuracysnes-plan.mdRepository: doublegate/RustySNES
Length of output: 7871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- scene source, manifest, and golden membership ---'
python3 - <<'PY'
from pathlib import Path
import re
scenes = Path("tests/roms/AccuracySNES/gen/src/scenes.rs").read_text()
source_ids = re.findall(r'id:\s*"([^"]+)"', scenes)
manifest = Path("tests/roms/AccuracySNES/build/scenes.tsv").read_text().splitlines()
manifest_rows = [line.split("\t") for line in manifest if line.strip() and not line.lstrip().startswith("#")]
manifest_ids = [row[1] for row in manifest_rows]
golden_path = Path("tests/golden/accuracysnes-scenes.tsv")
print("source scene count:", len(source_ids))
print("manifest scene count:", len(manifest_ids))
print("source/manifest IDs equal:", source_ids == manifest_ids)
print("C5.15 source IDs:", [x for x in source_ids if x == "c5-mode5-hires-16px-tiles"])
print("C5.15 manifest rows:", [row for row in manifest_rows if "C5.15" in row])
if golden_path.exists():
golden_rows = [
line.split("\t") for line in golden_path.read_text().splitlines()
if line.strip() and not line.lstrip().startswith("#")
]
golden_ids = [row[0] for row in golden_rows]
print("golden scene count:", len(golden_ids))
print("C5.15 golden rows:", [row for row in golden_rows if row and row[0] == "c5-mode5-hires-16px-tiles"])
else:
print("golden file: missing")
PY
printf '%s\n' '--- exact MAP and plan references ---'
rg -n 'C5\.15|c5-mode5-hires-16px-tiles|54 scenes|55 scenes|scenes blessed' \
tests/roms/AccuracySNES/gen/src/dossier.rs \
docs/accuracysnes-plan.md \
docs/accuracysnes-coverage.md \
tests/golden/accuracysnes-scenes.tsv || true
printf '%s\n' '--- path-filter exclusion lists ---'
rg -n -i -C 3 'path.?filter|exclude|generated artifacts|generated files|scenes\.tsv|accuracysnes\.sfc' \
.github tests/roms/AccuracySNES README.md docs 2>/dev/null | head -200 || trueRepository: doublegate/RustySNES
Length of output: 22192
Complete the C5.15 coverage records.
C5.15 is absent from dossier.rs::MAP. The generated artifacts contain the scene, but no golden entry exists, so docs/accuracysnes-coverage.md lists it as uncovered and docs/accuracysnes-plan.md still reports 54 blessed scenes. Add the MAP and golden entries, then regenerate the coverage and plan counts to 55.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/roms/AccuracySNES/gen/src/scenes.rs` around lines 597 - 621, Add the
missing C5.15 coverage mapping in dossier.rs::MAP and create its corresponding
golden entry using the existing C5.15 scene identifier and expected output
conventions. Regenerate docs/accuracysnes-coverage.md and
docs/accuracysnes-plan.md so C5.15 is covered and the blessed-scene total is
updated from 54 to 55.
Sources: Coding guidelines, Path instructions
… reset Bot review. The blocking finding is real and is the silent-failure shape this host's geometry checks exist to remove: the WARN condition used `<` where the DROP condition used `!=`, so a frame taller than the contract was dropped without a word. They are now character-for-character identical, with a comment saying that if they ever diverge again the quiet one wins and the loud one is decoration. Also accepted: the Rust harness reads scenes.tsv rather than `unwrap_or_default()` -- a read error on a file that EXISTS would slip past `manifest()`'s emptiness assert and make every scene `Direct`, which for a hi-res scene means hashing the left half of the picture. And both manifest parsers strip a trailing CR, since a CRLF checkout would make every tag compare unequal and take the fatal branch. DECLINED, with the measurement: "reset want_extract to Direct when id == 0". It looks like an obvious tidy-up and it silently breaks the hi-res capture. `run_scenes` publishes the scene ID only on frames whose field parity matches, so id == 0 occurs INSIDE a hold, not merely between holds; resetting there disarms the rule for the very frame that gets hashed. Applied literally, snes9x stopped capturing the hi-res scene altogether -- 54 match, 0 unblessed, the scene simply gone. The arming stays sticky and the comment records why, so the next reader does not re-suggest it. The cost is at most one spurious out-of-contract line at a hi-res-to-direct transition. Also declined: collapsing the 54 explicit `extract: Extract::Direct` lines. `SCENES` is a const static, so `..Default::default()` is not available in that position; explicit is honest and greppable. Re-verified: 54 blessed match on both hosts, hi-res unblessed on both, three references agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Blocking finding accepted — it is the silent-failure shape this host's geometry checks exist to remove. The warn condition used Also accepted: the Rust harness now reads Declined, with a measurement: "reset It looks like an obvious tidy-up and it silently breaks the hi-res capture. Applied literally: versus sticky: The arming stays sticky and the code now records why, so the next reader does not re-suggest it. The cost is at most one spurious out-of-contract line at a hi-res-to-direct transition — a far better trade than a missing scene. Declined: collapsing the 54 explicit |
Antigravity review (Gemini via Ultra)This PR introduces per-scene capture extraction rules ( Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
…ES bug (#323) PR #322 published "the two references agree bit-for-bit and RustySNES differs, which is this project's signature for a real defect". That was wrong, and wrong for exactly the reason this project's own notes warn about: I had TWO references, concluded "RustySNES alone", and had not read the third. Diffing the pixels rather than the hashes: the divergence is ONE COLUMN -- column 0 of the extracted sample, the first pixel of the 512-wide picture -- on all 224 rows and nothing else. 224 differing pixels of 57,344. RustySNES 0x0000, snes9x 0x0421 (the backdrop). ares' sfc/ppu/dac.cpp settles it AGAINST the original conclusion. scanline() seeds `math.above.colorEnable = false` under the comment "the first hires pixel of each scanline is transparent // note: exact value initializations are not confirmed on hardware", and below() returns (n15)0 -- black -- in that state. RustySNES seeds `above_enable: false` and does the same. The split is RustySNES + ares against snes9x + Mesen2, on a value ares itself flags as unverified. Consequences, recorded so the scene is not mistaken for a pending fix: there is no defect here; c5-mode5-hires-16px-tiles is NOT blessable under ADR 0013 rule 4 and stays permanently unblessed as a variant set; C5.15 does not become coverage this way, though a hi-res scene avoiding column 0 would make the rest of Mode 5 blessable. The extraction infrastructure is vindicated regardless -- two hosts with different geometries produced identical hashes for the other 57,120 pixels. The durable note: two references agreeing is not "the references agree". This project counts ares and bsnes as ONE reference precisely because lineage matters, and here the lineage that disagreed was the one not consulted. Read the third source BEFORE publishing a defect claim. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Implements the extraction schema specified in #321, and it found a real defect on its first run.
The rule travels with the scene
Scenegains anextractfield (Direct|HiResEven), emitted asbuild/scenes.tsv's fourth column so the rule travels with the scene rather than being compiled into any host. All three hosts — Rust harness, libretro C, Mesen2 Lua — read it.Two design points that are load-bearing rather than decorative:
Direct. Falling back would silently hash the left half of a hi-res picture — exactly the failure fix(accuracysnes): both scene hosts accepted an out-of-contract geometry #320's exact-geometry checks were added to stop.Directand match aDirectgolden. The declaration is what turns that into a failure.C5.15diverged immediately512x2240xbcb8d1c2bec08325512x4780xbcb8d1c2bec083250xd8dad9b9cb91e325The two references agree bit-for-bit despite emitting different geometries and running entirely different extraction paths — snes9x needs even columns only, Mesen2 needs even columns and even rows. That convergence is strong evidence the extraction is correct and the divergence is real, and it is this project's own signature for a genuine defect: three hosts failing identically usually means a broken test; one host failing alone means a bug in that host.
Consistent with
docs/STATUS.mdalready recording hi-res (Modes 5/6) as "real-title validation still open". This is the first automated evidence of what that gap actually is.The scene is left unblessed, deliberately
ADR 0013 rule 4 would permit blessing at the reference value, since the references agree. But that turns the scene gate red on a live finding, and taking the tree red is the maintainer's call. An unblessed scene does not fail the gate, so the finding is preserved without breaking CI. Blessing follows the fix;
C5.15becomes real coverage then.Verification
No existing golden moved.
cargo fmt --check, workspace clippy at-D warnings, andRUSTDOCFLAGS="-D warnings" cargo doc --workspace --exclude rustysnes-android --no-deps(the gate's exact command) all clean.🤖 Generated with Claude Code
Adds per-scene framebuffer extraction rules through
Scene::extract. The AccuracySNES dossier now asserts thatDirectandHiResEvenextraction is declared inscenes.tsv, supported by all hosts, and rejected when unsupported. The dossier also records theC5.15defect: snes9x and Mesen2 produce the same extracted hash, but RustySNES produces a different hash from the hi-res Mode 5 input.C5.15remains unblessed. The coverage denominator does not move.The claim is false if any host infers extraction from frame geometry, accepts an unsupported rule, or produces a different result for a blessed scene. Current verification reports 54 matching scenes, one unblessed scene, and no mismatches.