test(accuracysnes): C7.06 — Time Over reads set by V = YLOC + 1 - #286
Conversation
|
Warning Review limit reached
Next review available in: 30 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 ignored due to path filters (4)
📒 Files selected for processing (3)
WalkthroughAdded AccuracySNES C7.06 coverage for PPU Time Over timing. The test compares 16x16 and 8x8 sprite layouts at the line boundary, registers the assertion, and documents the expected snes9x divergence. ChangesAccuracySNES C7.06
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
C7 on-cart coverage 7 -> 8 of 16. 8x8 sprites can NEVER reach Time Over. The budget is 34 tiles per line, but range evaluation stops at the 33rd in-range sprite, so 8x8 caps at 32 -- under the limit, permanently. C7.05's forty 8x8 sprites set Range Over and leave Time Over clear, and the eval-line probe reports the same for the same reason. Twenty 16x16 sprites give 40 tiles while staying under the 32-sprite range limit. That separation is itself an assertion: Time Over must set while Range Over stays clear, so a core raising the two together fails. The bracket runs across the line boundary rather than within a line, because the asserted position is fixed and there is no index to sweep: clear on the eval line V = 100, set on V = 101. Phase B samples on line 101 deliberately -- RustySNES raises the flag at HBLANK_START_DOT of the eval line, EARLIER than the assertion requires, and pinning that exact dot would fail a core raising it at (101, 0) exactly, which the assertion permits. A low-tile control (the same 20 sprites at 8x8, 20 tiles) must read clear at phase B's sampling point; without it "set" would only have meant "sprites are present". Four injections, each failing its own code: flagging on any sprite fails code 4 (the control), never setting fails code 2, setting at dot 0 fails code 1. snes9x fails it and is recorded as an expected divergence (SNES9X_KNOWN_FAILURES 13 -> 14): it already reads Time Over set on V = 100, flagging the overflow a line early because it evaluates and paints in one pass. The control passes there, so it is the position that is wrong, not the budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c034f1d to
d52de7c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 14: Update the AccuracySNES C7.06 changelog entry to remove the
unsupported “H = 0” claim, while retaining the measured V = OBJ.YLOC + 1
behavior and the IRQ service-point context. Do not present (101, 0) as the
asserted timing position.
In `@docs/accuracysnes-plan.md`:
- Around line 559-560: Complete the C7.06 injection matrix by updating
docs/accuracysnes-plan.md lines 559-560 to state that raising Range Over with
Time Over fails code 3, and add the same code-3 injection to CHANGELOG.md lines
29-32.
In `@scripts/accuracysnes/crossval.sh`:
- Around line 159-166: Update the C7.06 known-failure comment adjacent to
SNES9X_KNOWN_FAILURES to include exact URLs and section or page references for
nocash fullsnes and the SNESdev Wiki covering the 34-tile budget and fetch-phase
timing. Preserve the existing failure description and ensure the citations
clearly support the documented timing divergence.
🪄 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: 01c099c3-1669-434d-94fc-ab77de915128
⛔ Files ignored due to path filters (6)
docs/accuracysnes-coverage.mdis excluded by!docs/accuracysnes-coverage.mdand included bydocs/**tests/roms/AccuracySNES/ERROR_CODES.mdis excluded by!tests/roms/AccuracySNES/ERROR_CODES.mdand included bytests/**tests/roms/AccuracySNES/SOURCE_CATALOG.tsvis excluded by!**/*.tsv,!tests/roms/AccuracySNES/SOURCE_CATALOG.tsvand included bytests/**tests/roms/AccuracySNES/asm/tests_group_a.sis excluded by!tests/roms/AccuracySNES/asm/tests_group_a.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/**
📒 Files selected for processing (5)
CHANGELOG.mddocs/accuracysnes-plan.mdscripts/accuracysnes/crossval.shtests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test-light
- GitHub Check: accuracysnes
- GitHub Check: lint
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (13)
**/*
📄 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:
scripts/accuracysnes/crossval.shtests/roms/AccuracySNES/gen/src/dossier.rsdocs/accuracysnes-plan.mdCHANGELOG.mdtests/roms/AccuracySNES/gen/src/tests/ppu.rs
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/crossval.sh
**/*.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:
tests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
**/*.{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:
tests/roms/AccuracySNES/gen/src/dossier.rsdocs/accuracysnes-plan.mdCHANGELOG.mdtests/roms/AccuracySNES/gen/src/tests/ppu.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.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Additive features must be default-off so shipped/native,no_std, and wasm builds remain byte-identical.
Never use or configure--all-features; validate opt-in feature combinations individually as required by the project recipe.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/ppu.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.rstests/roms/AccuracySNES/gen/src/tests/ppu.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.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
docs/**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Before changing a subsystem, consult
docs/architecture.md,docs/STATUS.md,CONTRIBUTING.md, the relevant subsystem documentation, and applicable ADRs.New subsystems must add documentation under
docs/.
Files:
docs/accuracysnes-plan.md
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/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/accuracysnes-plan.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
docs/accuracysnes-plan.mdCHANGELOG.md
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 (5)
📚 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.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
📚 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-21T06:21:34.629Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 198
File: docs/accuracysnes-plan.md:0-0
Timestamp: 2026-07-21T06:21:34.629Z
Learning: When reviewing slot allocation for SNES/ROM measurement channels in generator-driven Rust code, account for slots assigned via generation-time computed writers (e.g., slots derived from formulas like `slot_base = 8 + index * 2`) rather than only literal `record(...)` calls. Trace the computed writer’s full emitted slot range and verify there are no collisions with other opcodes/channels that may use different slot ranges after earlier conflict resolution.
Applied to files:
tests/roms/AccuracySNES/gen/src/tests/ppu.rs
📚 Learning: 2026-07-22T06:12:00.703Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 201
File: tests/roms/AccuracySNES/gen/src/tests/apu.rs:1508-1538
Timestamp: 2026-07-22T06:12:00.703Z
Learning: When working on APU/voice timing in these AccuracySNES test sources, treat NTSC vs PAL differences as a first-class constraint. In particular, do not change `Voice::settle` (or any settling/code-size/timing logic it affects) unless you cross-validate against both regions (NTSC and PAL), because timing/polling phase shifts can cause regressions that may appear as PAL-only test failures. For ROM-specific expectations like `E7.13`, keep the intentionally chosen ENVX range (e.g., `0x68..=0x7C`) unless you re-validate that the absorbed post-KON timing variation still matches across both regions.
Applied to files:
tests/roms/AccuracySNES/gen/src/tests/ppu.rs
🔇 Additional comments (4)
docs/accuracysnes-plan.md (1)
472-472: LGTM!tests/roms/AccuracySNES/gen/src/tests/ppu.rs (2)
74-74: LGTM!Also applies to: 2564-2607, 2609-2638
2444-2562: 🎯 Functional CorrectnessNo change required.
HBLANK_START_DOTis 274, after the ~93-dot sample point, so Phase A remains clear as documented.> Likely an incorrect or invalid review comment.tests/roms/AccuracySNES/gen/src/dossier.rs (1)
215-215: LGTM!
|
|
||
| ### Added | ||
|
|
||
| - **AccuracySNES: `C7.06` — Time Over reads set by `V = OBJ.YLOC + 1, H = 0`.** `C7` on-cart |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the unsupported H = 0 precision.
The C7.06 test samples $213E at an IRQ service point on V = 101. It does not assert that Time Over becomes set at H = 0. The plan explicitly treats (101, 0) as one permitted implementation, not as the measured position.
Proposed wording
-- **AccuracySNES: `C7.06` — Time Over reads set by `V = OBJ.YLOC + 1, H = 0`.**
+- **AccuracySNES: `C7.06` — Time Over reads set by `V = OBJ.YLOC + 1`.**📝 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.
| - **AccuracySNES: `C7.06` — Time Over reads set by `V = OBJ.YLOC + 1, H = 0`.** `C7` on-cart | |
| - **AccuracySNES: `C7.06` — Time Over reads set by `V = OBJ.YLOC + 1`.** |
🤖 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 14, Update the AccuracySNES C7.06 changelog entry to
remove the unsupported “H = 0” claim, while retaining the measured V = OBJ.YLOC
+ 1 behavior and the IRQ service-point context. Do not present (101, 0) as the
asserted timing position.
Source: Path instructions
| line passes both A and B. Four injections, each failing its own code — flag on any sprite fails | ||
| **code 4** (the control), never setting fails **code 2**, and setting at dot 0 fails **code 1**. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the C7.06 injection matrix complete.
Both documents claim four injections but omit the Range Over independence failure.
docs/accuracysnes-plan.md#L559-L560: state that raising Range Over with Time Over fails code 3.CHANGELOG.md#L29-L32: add the same code-3 injection to the release note.
📍 Affects 2 files
docs/accuracysnes-plan.md#L559-L560(this comment)CHANGELOG.md#L29-L32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/accuracysnes-plan.md` around lines 559 - 560, Complete the C7.06
injection matrix by updating docs/accuracysnes-plan.md lines 559-560 to state
that raising Range Over with Time Over fails code 3, and add the same code-3
injection to CHANGELOG.md lines 29-32.
Source: Path instructions
| # snes9x, +1 test (C7.06 "TimeOver by YLOC+1", fails phase A with code 1): the same scanline-granular | ||
| # sprite model as C7.05, seen from the other side. The row requires Time Over to be clear while the | ||
| # sprites' OWN line is being evaluated (V = 100) and set once the line they paint on has begun | ||
| # (V = 101 = YLOC + 1); snes9x already reads it set on V = 100, i.e. it flags the tile-budget | ||
| # overflow a line early because it evaluates and paints in the same pass. The 20-sprite/8x8 control | ||
| # passes there, so this is the position being wrong, not the budget. Documented by nocash fullsnes | ||
| # and the SNESdev Wiki (34-tile budget, raised by the fetch phase). Region-independent. | ||
| SNES9X_KNOWN_FAILURES=14 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a traceable source citation.
The known failure is recorded correctly, but the comment only names “fullsnes” and “the SNESdev Wiki.” Add the exact URL and section or page for the 34-tile budget and fetch-phase timing. This lets reviewers distinguish a documented reference divergence from an accidental expected failure.
🤖 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/crossval.sh` around lines 159 - 166, Update the C7.06
known-failure comment adjacent to SNES9X_KNOWN_FAILURES to include exact URLs
and section or page references for nocash fullsnes and the SNESdev Wiki covering
the 34-tile budget and fetch-phase timing. Preserve the existing failure
description and ensure the citations clearly support the documented timing
divergence.
Source: Path instructions
…7/C7.12 C7.12 is a scene needing known-field publication (v1.29.0, shared with C9.03/C9.06). C7.07 is reachable as a test but would fail: RustySNES skips a fully-offscreen sprite before either counter, so no false Time Over can arise, and the row needs a PPU behaviour change first. That change should not be made on the available evidence -- a single errata line, with snes9x the only cross-check, and snes9x is already scanline-granular here and fails both C7.05 and C7.06 for that reason. The $F8/$F9 retraction is what this caution exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Raised in review. With the poison $FF still in the slot, $FF satisfies or violates the flag tests by accident: phase A reads bit 7 set and blames 'Time Over already set', phase B passes bit 7 and then blames 'Range Over set alongside'. Both name a cause that did not happen, and ERROR_CODES.md is meant to be the complete account of failure bytes. Each phase now asserts the sampled byte is not the poison first, with its own code. Verified by suppressing the arm ($4200 = $00): the row now fails code 1, 'the H/V IRQ never fired', instead of a flag assertion. Codes shifted, so the injection matrix was re-run and the docs corrected: flag-on-any-sprite now fails code 7, never-setting code 5, set-at-dot-0 code 2; 1/3/4 are the sentinels. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review adjudicationNo blocking issues. The second suggestion was right and is applied; the first and the nitpicks are Applied: the poison value produced misattributed failure codesReal, and it matters more here than the "suggestion" label implies. With Each phase now asserts the sampled byte is not the poison first, with its own code. Verified by That shifted the other codes, so the injection matrix was re-run rather than assumed — the docs and
Codes 1, 3 and 4 are the three per-phase sentinels. Declined: "the checked-in assembly does not match the generator"Not reproduced. The finding compares a line in Declined: the two nitpicks
|
Antigravity review (Gemini via Ultra)This PR adds the Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
1 participant
Stacked on #285 (C7.05).
C7on-cart coverage 7 → 8 of 16.8x8 sprites can never reach Time Over
The budget is 34 tiles per line, but range evaluation stops at the 33rd in-range sprite — so 8x8
sprites cap out at 32 tiles, under the limit, permanently. #285's forty 8x8 sprites set Range Over
and leave Time Over clear, and
scripts/probes/eval-line-213ereports the same for the same reason.Anything reaching for Time Over with small sprites is measuring nothing.
Twenty 16x16 sprites give 40 tiles while staying under the 32-sprite range limit. That separation
is itself an assertion: Time Over must set while Range Over stays clear, so a core that raises
the two together, or raises whichever it computes first, fails.
The bracket runs across the line boundary
Unlike #285 there is no index to sweep — the asserted position is fixed. So:
V = 100(the eval line)V = 101(YLOC + 1)Phase B samples on line 101 rather than late on line 100 on purpose. RustySNES raises the flag at
HBLANK_START_DOTof the eval line, which is earlier than the dossier requires; asserting thatexact dot would fail a core that raises it at
(101, 0)exactly, which the assertion permits.Sampling on 101 accepts both and still rejects anything later — the test asserts what the row says,
not what this emulator happens to do.
The control, and the injection matrix
Phase A alone cannot tell "not yet" from "never"; phase B alone cannot tell "correct" from "always
set". A third run keeps the same 20 sprites at 8x8 (20 tiles, under budget) and requires Time
Over clear at phase B's sampling point.
Every assertion is load-bearing; none is reachable by another's failure.
Cross-validation
snes9x fails this row, recorded as an expected divergence (
SNES9X_KNOWN_FAILURES13 → 14): italready reads Time Over set on
V = 100, flagging the overflow a line early because it evaluates andpaints in one pass — the same scanline-granular model that fails #285, seen from the other side. The
20-sprite/8x8 control passes on snes9x, so it is the position that is wrong there, not the budget.
Mesen2 could not arbitrate; its headless runner times out in this environment, which predates this
work.
cargo run -p accuracysnes-genreproduces the committed ROM; battery 100% on-cart at 335 tests; fmtand
clippy --workspace -D warningsclean.🤖 Generated with Claude Code
Adds AccuracySNES dossier assertion C7.06 for sprite Time Over timing at
V = YLOC + 1.The test claims that 20 16×16 sprites set Time Over at
V = 101, while$213Eremains clear atV = 100and Range Over remains clear. A 20-sprite 8×8 control must not set Time Over. The claim is false if hardware sets the flag early, fails to set it on the following line, sets Range Over, or flags the 8×8 control.C7 coverage increases from 7/16 to 8/16. The change records snes9x as an expected failure because it flags Time Over early. It adds injection cases for early, missing, and incorrect behavior.