feat(accuracysnes): a third coverage tier for on-cart-impossible assertions - #319
Conversation
…rtions G1.06 and G1.18 were both known-coverable and unrepresentable: the coverage report counted on-cart and scene covers only, so a row whose stimulus comes from outside the cartridge had nowhere to go. The bar is deliberately narrow. Not "a host test also exercises this" -- the cart cannot observe it AT ALL, with the reason stated per row. G1.06 (cartridge /RESET does not reset the PPU) is driven from outside the cartridge: a program cannot pull its own reset and then observe what survived, because observing requires still running. G1.18 (the copier prefix) is about the LOADER, not the machine -- a file's size is decided before a single instruction runs. Reported in its own column and its own total, never folded into the on-cart figure, and the report now orders the three tiers by what the evidence is worth: an on-cart verdict means the same thing on any emulator and on real hardware, a scene needs a host holding the golden, and a host-side cover is THIS PROJECT TESTING ITS OWN CODE -- the one thing AccuracySNES exists to stop being the only evidence. Every entry names the test that carries it, so the claim is checkable rather than asserted, and the tiers are checked in descending order so an assertion with an on-cart test is never reported under a weaker one. G1.06's test is new and inject-verified: adding a `ppu = Ppu::new()` to System::reset -- the tidy-looking change it exists to catch -- fires it. It also pins that the PPU's timeline never moves BACKWARDS across a reset, and the first draft of that assertion was wrong: the timeline legitimately advances, because the vector fetch costs clocks. The test advances the PPU well into a frame first, because resetting from the power-on position would leave it at (0, 0) either way and the assertion would hold whether or not the PPU was reset. Coverage 356 -> 358 of 443 (302 on-cart + 54 scenes + 2 host-side). 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 (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (9)crates/**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
crates/rustysnes-{cpu,ppu,apu,cart,core}/**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.rs📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
crates/rustysnes-core/**/*.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:
🔇 Additional comments (3)
WalkthroughThe change adds host-side AccuracySNES coverage for ChangesAccuracySNES host coverage
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a third, explicitly weaker AccuracySNES coverage tier for dossier assertions that are physically untestable on-cart (stimulus outside the cartridge or loader-only properties), so “known-coverable but unrepresentable” rows can be tracked without inflating the on-cart evidence tier.
Changes:
- Introduces
HOST_COVEREDin the AccuracySNES dossier generator and extends the coverage report/table to include a “Covered (host)” column with strict tier precedence (on-cart → scene → host). - Adds a host-side unit test (
a_soft_reset_leaves_the_ppu_alone) coveringG1.06, and exposesBus::advance_masteraspub(crate)to support that test setup. - Updates project docs and changelog to reflect the new totals and explain the host-side tier’s meaning and constraints.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/roms/AccuracySNES/gen/src/dossier.rs | Adds HOST_COVERED and extends generated coverage reporting to account for host-only assertions with strict tier ordering. |
| crates/rustysnes-core/src/scheduler.rs | Adds a host-side unit test asserting soft reset doesn’t reset PPU state (VRAM survives; timeline doesn’t restart). |
| crates/rustysnes-core/src/bus.rs | Changes advance_master visibility to pub(crate) to allow cross-module (test) callers to advance master clock directly. |
| docs/STATUS.md | Updates documented AccuracySNES coverage counts and description to include the host-side tier. |
| docs/accuracysnes-plan.md | Updates the plan’s coverage summary to include the host-side tier and preserve tier separation rationale. |
| docs/accuracysnes-coverage.md | Regenerated coverage table to include “Covered (host)” and updated totals/text accordingly. |
| CHANGELOG.md | Documents the new coverage tier, its narrow criteria, and the new G1.06 host-side test. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@crates/rustysnes-core/src/scheduler.rs`:
- Around line 627-633: Update the soft-reset assertion in the scheduler test to
capture the PPU’s frame_count(), scanline(), and dot() before and after reset,
then compare the composite frame-aware PPU positions lexicographically or via
the existing position representation. Preserve the requirement that reset may
advance the timeline but must never move it backwards, including across frame
and scanline boundaries.
- Around line 604-606: Update a_soft_reset_leaves_the_ppu_alone to load
synth_rom(0x00) into the System and perform an initial reset before writing
VRAM, ensuring the later reset exercises cartridge /RESET behavior while
preserving the existing VRAM assertions.
In `@tests/roms/AccuracySNES/gen/src/dossier.rs`:
- Around line 500-504: Narrow the reset-test documentation to state only that
the host verifies scanline non-decrease, rather than claiming the full PPU
timeline never moves backward. Update the registry reason in
tests/roms/AccuracySNES/gen/src/dossier.rs lines 500-504 and apply the same
wording to CHANGELOG.md lines 73-76; no test logic change is required.
- Around line 835-856: Update coverage_report in dossier.rs to add a host-side
detail section that iterates HOST_COVERED and lists each entry’s assertion ID,
test name, and reason, including G1.06 and G1.18. Keep the existing host_total
summary and column explanations unchanged.
- Around line 818-819: Update coverage_report’s host-counting logic around
HOST_COVERED so an entry increments by_host only after resolving its referenced
test path to an existing passing host test and confirming exactly one matching
enumerated assertion; do not count entries based solely on the assertion ID, and
reject or report removed, renamed, failing, or ambiguously matched host tests.
🪄 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: 3ce8a966-95fc-4bab-8848-fc41766a8d44
⛔ Files ignored due to path filters (1)
docs/accuracysnes-coverage.mdis excluded by!docs/accuracysnes-coverage.mdand included bydocs/**
📒 Files selected for processing (6)
CHANGELOG.mdcrates/rustysnes-core/src/bus.rscrates/rustysnes-core/src/scheduler.rsdocs/STATUS.mddocs/accuracysnes-plan.mdtests/roms/AccuracySNES/gen/src/dossier.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: test-light
- GitHub Check: accuracysnes
- GitHub Check: lint
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (17)
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.mdcrates/rustysnes-core/src/bus.rscrates/rustysnes-core/src/scheduler.rsdocs/accuracysnes-plan.mdtests/roms/AccuracySNES/gen/src/dossier.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.mdcrates/rustysnes-core/src/bus.rscrates/rustysnes-core/src/scheduler.rsdocs/accuracysnes-plan.mdtests/roms/AccuracySNES/gen/src/dossier.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
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-core/src/bus.rscrates/rustysnes-core/src/scheduler.rs
crates/rustysnes-{cpu,ppu,apu,cart,core}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep core chip implementation changes localized to the owning chip crate and preserve the workspace crate boundaries.
Files:
crates/rustysnes-core/src/bus.rscrates/rustysnes-core/src/scheduler.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-core/src/bus.rscrates/rustysnes-core/src/scheduler.rstests/roms/AccuracySNES/gen/src/dossier.rs
crates/rustysnes-core/**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
The core crate must compile for
thumbv7em-none-eabihfwith--no-default-features; the chip stack must remainno_std.
Files:
crates/rustysnes-core/src/bus.rscrates/rustysnes-core/src/scheduler.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-core/src/bus.rscrates/rustysnes-core/src/scheduler.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-core/src/bus.rscrates/rustysnes-core/src/scheduler.rstests/roms/AccuracySNES/gen/src/dossier.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-core/src/bus.rscrates/rustysnes-core/src/scheduler.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/dossier.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/dossier.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/dossier.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 (3)
📚 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
🪛 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)
🔇 Additional comments (6)
crates/rustysnes-core/src/bus.rs (1)
809-809: LGTM!tests/roms/AccuracySNES/gen/src/dossier.rs (2)
481-499: LGTM!Also applies to: 505-515
797-817: LGTM!Also applies to: 820-826
docs/STATUS.md (1)
26-27: LGTM!Also applies to: 244-244
docs/accuracysnes-plan.md (1)
17-17: LGTM!CHANGELOG.md (1)
56-71: LGTM!Also applies to: 77-78
| fn a_soft_reset_leaves_the_ppu_alone() { | ||
| let mut sys = System::new(0); | ||
| sys.bus.ppu.vram_mut()[0x1234] = 0xBEEF; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Install a cartridge before testing cartridge /RESET.
Line 604 creates System without a cartridge. The later System::reset call exercises the no-cartridge setup, not the cartridge /RESET scenario named by G1.06. A regression that resets PPU state only when Bus::cart is present will pass this test, and the host coverage entry will be false. Load synth_rom(0x00) and perform an initial reset before writing VRAM.
Proposed test setup
let mut sys = System::new(0);
+ sys.bus.cart = Some(Cart::from_rom(&synth_rom(0x00)).expect("synth rom"));
+ sys.reset();
sys.bus.ppu.vram_mut()[0x1234] = 0xBEEF;📝 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.
| fn a_soft_reset_leaves_the_ppu_alone() { | |
| let mut sys = System::new(0); | |
| sys.bus.ppu.vram_mut()[0x1234] = 0xBEEF; | |
| fn a_soft_reset_leaves_the_ppu_alone() { | |
| let mut sys = System::new(0); | |
| sys.bus.cart = Some(Cart::from_rom(&synth_rom(0x00)).expect("synth rom")); | |
| sys.reset(); | |
| sys.bus.ppu.vram_mut()[0x1234] = 0xBEEF; |
🤖 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 `@crates/rustysnes-core/src/scheduler.rs` around lines 604 - 606, Update
a_soft_reset_leaves_the_ppu_alone to load synth_rom(0x00) into the System and
perform an initial reset before writing VRAM, ensuring the later reset exercises
cartridge /RESET behavior while preserving the existing VRAM assertions.
| assert!( | ||
| sys.bus.ppu.scanline() >= line_before, | ||
| "a soft reset moved the PPU's timeline BACKWARDS, from line {line_before} to {}. The \ | ||
| video clock free-runs across a cartridge reset; it may advance (the vector fetch \ | ||
| costs clocks) but it must never restart", | ||
| sys.bus.ppu.scanline() | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Compare a frame-aware PPU position.
Line 628 compares only frame-relative scanline(). If reset occurs on the last scanline and CPU reset work crosses the frame boundary, the PPU can continue correctly while scanline() wraps to zero and the assertion fails. A reset of the dot within the same scanline also passes. Record frame_count(), scanline(), and dot() before and after reset, then compare the composite position.
🤖 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 `@crates/rustysnes-core/src/scheduler.rs` around lines 627 - 633, Update the
soft-reset assertion in the scheduler test to capture the PPU’s frame_count(),
scanline(), and dot() before and after reset, then compare the composite
frame-aware PPU positions lexicographically or via the existing position
representation. Preserve the requirement that reset may advance the timeline but
must never move it backwards, including across frame and scanline boundaries.
| "The reset line is driven from OUTSIDE the cartridge. A program cannot pull its own and \ | ||
| then observe what survived, because observing requires still running -- so no on-cart \ | ||
| test of this assertion can exist, whatever machinery is added. The host can hold state \ | ||
| across `System::reset` and check it, and the test also pins that the PPU's timeline never \ | ||
| moves backwards across one", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The host-coverage text overstates the reset test.
The test checks only scanline non-decrease. It does not prove that the full PPU timeline never moves backward.
tests/roms/AccuracySNES/gen/src/dossier.rs#L500-L504: narrow the registry reason to scanline non-decrease, or strengthen the test to compare a non-wrapping full position.CHANGELOG.md#L73-L76: use the same narrower wording.
As per path instructions, documentation must not claim behavior broader than the corresponding test asserts.
📍 Affects 2 files
tests/roms/AccuracySNES/gen/src/dossier.rs#L500-L504(this comment)CHANGELOG.md#L73-L76
🤖 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/dossier.rs` around lines 500 - 504, Narrow
the reset-test documentation to state only that the host verifies scanline
non-decrease, rather than claiming the full PPU timeline never moves backward.
Update the registry reason in tests/roms/AccuracySNES/gen/src/dossier.rs lines
500-504 and apply the same wording to CHANGELOG.md lines 73-76; no test logic
change is required.
Source: Path instructions
| } else if HOST_COVERED.iter().any(|(a, _, _)| a == id) { | ||
| by_host += 1; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 6 \
'HOST_COVERED|coverage_report|a_soft_reset_leaves_the_ppu_alone|copier_prefix_stripped' \
tests/roms/AccuracySNES/gen/src/dossier.rs \
crates/rustysnes-core \
crates/rustysnes-cart
rg -n -C 6 \
'HOST_COVERED|a_soft_reset_leaves_the_ppu_alone|copier_prefix_stripped' \
. --glob '*.rs' --glob '*.yml' --glob '*.yaml' --glob '*.sh' \
|| trueRepository: doublegate/RustySNES
Length of output: 18924
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- dossier structure and coverage logic ---'
sed -n '470,530p;700,880p' tests/roms/AccuracySNES/gen/src/dossier.rs
printf '%s\n' '--- host test references and execution paths ---'
rg -n -C 5 \
'coverage_report|HOST_COVERED|enumerated|cargo test|cargo nextest|a_soft_reset_leaves_the_ppu_alone|copier_prefix_stripped' \
. --glob '*.rs' --glob '*.yml' --glob '*.yaml' --glob '*.sh' --glob '*.toml'
printf '%s\n' '--- host test bodies ---'
sed -n '604,660p' crates/rustysnes-core/src/scheduler.rs
sed -n '535,555p' crates/rustysnes-cart/src/header.rsRepository: doublegate/RustySNES
Length of output: 150
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
p = Path("tests/roms/AccuracySNES/gen/src/dossier.rs")
s = p.read_text()
m = re.search(r'pub const HOST_COVERED:.*?^];', s, re.S | re.M)
entries = re.findall(r'\(\s*"([^"]+)",\s*"([^"]+)",\s*"', m.group(0)) if m else []
print("HOST_COVERED entries:", len(entries))
for assertion, test, _ in entries:
print(assertion, "=>", test)
# Report whether each assertion ID occurs in the dossier's enumerated assertion
# declarations, and whether each test path occurs anywhere in tracked source.
files = [Path(x) for x in __import__("subprocess").check_output(
["git", "ls-files", "*.rs"], text=True).splitlines()]
all_text = "\n".join(x.read_text(errors="ignore") for x in files)
print("\nEntry checks:")
for assertion, test in entries:
print({
"assertion": assertion,
"dossier_id_occurrences": len(re.findall(rf'"{re.escape(assertion)}"', s)),
"test_path_occurrences": all_text.count(test),
"test_fn_occurrences": len(re.findall(rf'\bfn\s+{re.escape(test.rsplit("::", 1)[-1])}\s*\(', all_text)),
})
# Find all uses of the coverage function and its argument construction.
for i, line in enumerate(s.splitlines(), 1):
if "coverage_report(" in line or "enumerated" in line:
print(f"{i}: {line}")
PYRepository: doublegate/RustySNES
Length of output: 316
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- HOST_COVERED declaration ---'
sed -n '490,520p' tests/roms/AccuracySNES/gen/src/dossier.rs
printf '%s\n' '--- coverage_report implementation ---'
sed -n '738,830p' tests/roms/AccuracySNES/gen/src/dossier.rs
printf '%s\n' '--- report callers ---'
rg -n -C 4 'coverage_report\(' tests/roms/AccuracySNES/gen crates --glob '*.rs'Repository: doublegate/RustySNES
Length of output: 8303
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
dossier = Path("tests/roms/AccuracySNES/gen/src/dossier.rs").read_text()
block = re.search(
r"pub const HOST_COVERED:.*?^];",
dossier,
re.S | re.M,
).group(0)
entries = []
for match in re.finditer(
r'\(\s*"([^"]+)"\s*,\s*"([^"]+)"\s*,\s*"((?:[^"\\]|\\.)*)"\s*\)',
block,
re.S,
):
entries.append(match.groups())
print("HOST_COVERED entries:", len(entries))
for assertion, test, reason in entries:
print(f"{assertion} => {test}")
print(" reason length:", len(reason))
tracked_rs = [
Path(p) for p in __import__("subprocess").check_output(
["git", "ls-files", "crates", "tests/roms/AccuracySNES/gen/src", "*.rs"],
text=True,
).splitlines()
]
texts = {p: p.read_text(errors="ignore") for p in tracked_rs}
for assertion, test, _ in entries:
fn = test.rsplit("::", 1)[-1]
test_hits = [
str(p) for p, text in texts.items()
if re.search(rf"\bfn\s+{re.escape(fn)}\s*\(", text)
]
id_hits = len(re.findall(rf'"{re.escape(assertion)}"', dossier))
exact_path_hits = sum(text.count(test) for text in texts.values())
print({
"assertion": assertion,
"dossier_string_occurrences": id_hits,
"host_test_function_files": test_hits,
"exact_host_path_occurrences": exact_path_hits,
})
PYRepository: doublegate/RustySNES
Length of output: 182
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '490,520p' tests/roms/AccuracySNES/gen/src/dossier.rs
sed -n '738,830p' tests/roms/AccuracySNES/gen/src/dossier.rs
rg -n -C 4 'coverage_report\(' tests/roms/AccuracySNES/gen crates --glob '*.rs'Repository: doublegate/RustySNES
Length of output: 8208
Validate each host entry before counting it.
coverage_report matches only the assertion ID in HOST_COVERED; it does not use the referenced test path or reason. If a host test is removed, renamed, or fails, the report still counts the assertion. Resolve each entry to an existing passing host test and exactly one enumerated assertion, or enforce this contract in a separate CI check.
🤖 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/dossier.rs` around lines 818 - 819, Update
coverage_report’s host-counting logic around HOST_COVERED so an entry increments
by_host only after resolving its referenced test path to an existing passing
host test and confirming exactly one matching enumerated assertion; do not count
entries based solely on the assertion ID, and reject or report removed, renamed,
failing, or ambiguously matched host tests.
| "| `{sub}` | {} | {covered} | {by_scene} | {by_host} | {list} |", | ||
| ids.len() | ||
| ); | ||
| } | ||
| let _ = writeln!( | ||
| s, | ||
| "\n**{covered_total} of {all_total}** enumerated assertion rows covered by an on-cart \ | ||
| test, plus **{scene_total}** covered only by a rendered scene \ | ||
| (`docs/adr/0013`) — **{} of {all_total}** in total.\n", | ||
| covered_total + scene_total | ||
| test, plus **{scene_total}** covered only by a rendered scene (`docs/adr/0013`) and \ | ||
| **{host_total}** covered only by a host-side test — **{} of {all_total}** in total.\n", | ||
| covered_total + scene_total + host_total | ||
| ); | ||
| let _ = writeln!( | ||
| s, | ||
| "The two columns are kept apart on purpose. An on-cart result means the same thing on any \ | ||
| emulator and on real hardware; a rendered scene needs a host holding the golden. Adding \ | ||
| them into one figure would quietly change what the number claims.\n" | ||
| "The three columns are kept apart on purpose, in descending order of what the evidence is \ | ||
| worth. An on-cart result means the same thing on any emulator and on real hardware; a \ | ||
| rendered scene needs a host holding the golden; a **host-side** cover is this project \ | ||
| testing its own code, which is the one thing AccuracySNES exists to stop being the only \ | ||
| evidence. The host tier is admitted only where the cart *physically cannot* observe the \ | ||
| assertion — the stimulus comes from outside the cartridge, or the subject is the loader \ | ||
| rather than the machine — and every entry names the test and the reason \ | ||
| (`dossier.rs::HOST_COVERED`). Adding the columns into one figure would quietly change \ | ||
| what the number claims.\n" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'HOST_COVERED|Covered \(host\)|Assertions covered by a host-side test|G1\.06|G1\.18' \
tests/roms/AccuracySNES/gen/src/dossier.rs \
docs/accuracysnes-coverage.md \
|| trueRepository: doublegate/RustySNES
Length of output: 13031
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- coverage_report structure ---'
sed -n '730,930p' tests/roms/AccuracySNES/gen/src/dossier.rs
printf '%s\n' '--- all report-detail headings and host references ---'
rg -n -C 3 \
'fn coverage_report|## |HOST_COVERED|host-side|scene_assertions|scene' \
tests/roms/AccuracySNES/gen/src/dossier.rs
printf '%s\n' '--- host test definitions ---'
rg -n -C 6 \
'a_soft_reset_leaves_the_ppu_alone|copier_prefix_stripped' \
--glob '*.rs' .Repository: doublegate/RustySNES
Length of output: 23503
Emit host-side evidence in coverage_report. The generated report shows host counts but does not list G1.06, G1.18, their test names, or their reasons. Add a host-side detail section that iterates HOST_COVERED.
🤖 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/dossier.rs` around lines 835 - 856, Update
coverage_report in dossier.rs to add a host-side detail section that iterates
HOST_COVERED and lists each entry’s assertion ID, test name, and reason,
including G1.06 and G1.18. Keep the existing host_total summary and column
explanations unchanged.
…fg(test) Bot review, blocking finding accepted and it was right. Relaxing advance_master to pub(crate) let any non-test code in rustysnes-core step the PPU and SPC clocks directly, around the scheduler -- which is exactly the Central-State Ownership contract the architecture doc states. It is private again, with a #[cfg(test)] pub(crate) forwarder for the one test that needs to put the PPU somewhere distinctive before resetting. Also accepted: the timeline assertion was fragile. `scanline() >= line_before` reads a legitimate end-of-frame WRAP as a pass and, worse, a wrap to 0 as a failure. It is now a two-sided band -- the reset costs a handful of clocks, so the line may advance by at most one -- and the setup asserts it landed mid-frame so a wrap cannot be confused with a restart at all. The 20,000-iteration loop is one call. Declined: a set lookup for HOST_COVERED. Two entries, and the comment already says a set is the answer if it grows. Re-verified: the test still fires when `ppu = Ppu::new()` is injected into System::reset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Blocking finding accepted, and it was right. Relaxing The timeline suggestion was also right, and worse than described. Declined: a set lookup for Re-verified after all three changes: the test still fires when |
Antigravity review (Gemini via Ultra)This PR introduces a third coverage tier ("host-side") to AccuracySNES reporting for assertions physically unobservable by on-cart code (tracking Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
…jections Bot review. The height finding is the same defect I had just fixed for width and missed one line over: `h < SCENE_H + FIRST_ROW` passes a frame that is too TALL. MEASURED: snes9x emits 256x224 normally and 256x448 for interlaced/pseudo-hires frames, and the old test passed those 448 ones straight into the hash, where the loop took the first 224 rows -- half an interlaced picture, silently. Exact now; all 54 scenes still match. Also accepted: a rejected frame now logs its geometry once per distinct size. Leaving the previous hash standing is right for a duped frame (a NULL pointer) but is exactly the wrong kind of quiet for a geometry violation. And SCENE_BUF_LEN references SCENE_W rather than repeating 256. Moved the CHANGELOG entry from ### Added to ### Fixed, where a bug fix belongs. Declined, both with the measurement: - "Unrelated G1.06/G1.18 CHANGELOG entries in this PR": not in the diff. `git diff origin/main..HEAD -- CHANGELOG.md` is +17 lines, all mine; that entry is pre-existing context from #319, already on main. - "`local width` is unused dead code": it is used, at mesen_scenes.lua line 115, as the row stride the hash walks with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…try (#320) * fix(accuracysnes): both scene hosts accepted an out-of-contract geometry The width tests were LOWER BOUNDS -- `w < SCENE_W` in the libretro host, `#buf < ...` in the Mesen2 script -- which catch a frame that is too narrow and miss one that is too WIDE. Too wide is the case that actually happens: a hi-res frame arrives 512 across, both tests pass it, and each then hashes something that is not the contract. The Lua script walks a 512-wide buffer with a stride of 256, hashing a diagonal slice; the C host uses the real pitch and hashes the leftmost 256 columns. A golden blessed from either would be stable, reproducible and wrong. Both are exact now. Widening the scene region past 256x224 is real planned work and has to start from a loud rejection rather than a silent wrong picture, which is what made this worth finding before that work rather than during it. The Mesen2 constant is MEASURED, not derived. The first attempt computed SCENE_W * (SCENE_H + FIRST_ROW) = 59136 -- the size of the region READ rather than of the buffer RETURNED -- and rejected every frame, which is how the real value was found. It is 256 x 239, the whole output frame, and the script now says so with the measurement recorded. All 54 scenes still match on both hosts; three references agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(accuracysnes): the height check was a lower bound too, and log rejections Bot review. The height finding is the same defect I had just fixed for width and missed one line over: `h < SCENE_H + FIRST_ROW` passes a frame that is too TALL. MEASURED: snes9x emits 256x224 normally and 256x448 for interlaced/pseudo-hires frames, and the old test passed those 448 ones straight into the hash, where the loop took the first 224 rows -- half an interlaced picture, silently. Exact now; all 54 scenes still match. Also accepted: a rejected frame now logs its geometry once per distinct size. Leaving the previous hash standing is right for a duped frame (a NULL pointer) but is exactly the wrong kind of quiet for a geometry violation. And SCENE_BUF_LEN references SCENE_W rather than repeating 256. Moved the CHANGELOG entry from ### Added to ### Fixed, where a bug fix belongs. Declined, both with the measurement: - "Unrelated G1.06/G1.18 CHANGELOG entries in this PR": not in the diff. `git diff origin/main..HEAD -- CHANGELOG.md` is +17 lines, all mine; that entry is pre-existing context from #319, already on main. - "`local width` is unused dead code": it is used, at mesen_scenes.lua line 115, as the row stride the hash walks with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
G1.06andG1.18were both known-coverable and unrepresentable: the coverage report counted on-cart and scene covers only, so a row whose stimulus comes from outside the cartridge had nowhere to go.The bar is deliberately narrow
Not "a host test also exercises this" — the cart cannot observe it at all, with the reason stated per row:
G1.06— cartridge/RESETdoes not reset the PPUG1.18— copier prefixReported so a reader can discount it
Its own column, its own total, never folded into the on-cart figure — and the report now orders the three tiers by what the evidence is worth:
Every entry names the test that carries it, so the claim is checkable rather than asserted. The tiers are checked in descending order, so an assertion that has an on-cart test is never reported under a weaker one just because one also exists.
G1.06's test is new, and inject-verifieda_soft_reset_leaves_the_ppu_aloneholds VRAM acrossSystem::reset. Adding appu = Ppu::new()toreset— exactly the tidy-looking change it exists to catch — fires it.Two things the draft got wrong and the measurement fixed:
(0, 0)either way, so that assertion would have held whether or not the PPU was reset. The test now advances the PPU well into a frame first.Verification
cargo fmt --check, workspace clippy at-D warnings,RUSTDOCFLAGS="-D warnings" cargo doc --workspace --exclude rustysnes-android --no-deps(the gate's exact command),cargo test -p rustysnes-core99 passed, AccuracySNES battery 100% on-cart.Coverage 356 → 358 of 443 (302 on-cart + 54 scenes + 2 host-side).
🤖 Generated with Claude Code
Adds host-side AccuracySNES coverage for
G1.06andG1.18. The denominator remains 443; coverage increases to 358: 302 on-cart, 54 scene, and 2 host-side.G1.06covers PPU survival across cartridge/RESET. The regression test verifies VRAM preservation and confirms that the PPU timeline does not move backward. Productionadvance_masterremains private.G1.18covers copier-prefix detection and stripping before program execution. The cartridge cannot observe this loader behavior.The claim is false if either host test fails to establish its dossier assertion, if reset changes VRAM or moves the PPU timeline backward, or if copier-prefix handling is not verified before execution.