chore(release): cut v2.6.4 "Rubric" — the last four AccuracyCoin disagreements close, and the gate that certified them covered 88 of 146 by doublegate · Pull Request #474 · doublegate/RustyNES

doublegate · 2026-08-26T08:58:18Z

What this is

The v2.6.4 release cut. The last four AccuracyCoin disagreements close — and then
the gate that certified them is measured to cover 88 of 146 entries.

A rubric is the authoritative statement of the rules, written by whoever set the
test. That is literally where all three fixes came from: AccuracyCoin is
MIT-licensed and its assembly source is one curl away. This version's plan
says the source "is not vendored", which is true of this repository and had been
read as unavailable. They are not the same statement.

The three entries, and where their rules live

entry	rule	in a nesdev page?
`Open Bus`	a `$4015` read does not drive the data bus; its D5 is open bus	no
`Interrupt flag latency`	branches poll before cycles 2 and 4, never before 3	no
`NMI Overlap BRK`	an interrupt sequence does not poll; the hijack window was one cycle narrow	one half yes

Every one is category 1 — the implementation followed documentation that is true
and insufficient.

Two wrong readings, both kept with their corrections

A Fail(N) names test N, one-based. TEST_Fail reports (ErrorCode << 2) | 2
and the runner sets ErrorCode to 1 before dispatching. Decoded as an index it
is off by one, and that made a change from test 7 to test 5 read as partial
progress when it was a regression that also broke a standing gate. v2.6.3's
reading that six entries "sharing one failure code" implied one shared cause is
retracted with it.

A fix that closes one gate and opens another is a scope measurement. The
first poll fix also moved the generic poll; it closed the entry and regressed
apupulse026 and blargg08. All three compare against the same oracle, so one
model satisfies all three. Narrowed to the branch exception, nothing regresses.

The coverage finding — what the release is actually about

With the four closed, the vector reported identical entry for entry across all
146 entries — with 58 of those NotRun on both sides. The comparator was
correct; the window was short. It reaches the CPU catalog and stops partway
through CPU Interrupts, asking nothing about the APU, PPU, sprite-evaluation or
PPU-misc suites.

accuracycoin_status now prints coverage on every comparison and refuses
when any entry is unrun on both sides. The predicate and the refusal are
extracted from main and tested directly — a check that exists only inside
main is a check no test can reach, which is how this property came to be
missing.

Widening the window to 4500 frames paid on its first run: it aborted at cycle
20,636,325, 2.8M past where every previous run had stopped, on a ppu_sel
that followed the halted CPU on a cycle the DMA owned.

Rung 5 does NOT close

Measured on both sides over the same window:

run length	oracle	DUT
17.9M	88	88
60.0M	120	5
134M	146	5

A hang in the suites that had never executed, not a restart. Reported as
unavailable rather than as "141 of 146 differ", which is one defect and not 141.

Verification

gate	result
sibling `./tb/regress.sh`	72 passed, 0 failed
RTL mutations	7 of 8 CAUGHT; the eighth INERT by byte-comparison
`accuracycoin_status` mutations	3 of 3 CAUGHT
AccuracyCoin (RAM decoder)	`pass rate = 100.00% over 141 assigned tests`
nestest	0-diff
`release_anchor_audit` / `libretro_info_audit`	11 / 3 passed
fmt · clippy `-D warnings` · rustdoc · `no_std` · markdownlint	clean

No rustynes-{cpu,ppu,apu,mappers,core} changes, so the two accuracy numbers
hold by construction — and were run anyway.

What a reviewer should look at hardest

The ppu_sel change (rtl/nes_top.sv, sibling) — it makes the decode
follow the bus owner. The write half of that exception was already there; ask
whether the read half has a case I have not considered, particularly an OAM
DMA reading from $20xx.
The coverage refusal's exit code — it turns a previously-passing
comparison into a failure for anyone using a narrow window deliberately. That
is intended, but it is a behaviour change in a tool other gates call.
The inert mutation (br_poll_* capture) — classified by byte-comparison
and documented at the site. If the reasoning about the stimulus that would
separate it is wrong, the note is wrong.

Companion sibling work: RustyNES_MiSTer branch feat/v2.6.4-rung5-close (4834145), which carries the RTL — OAM DMA, the SH group, the interrupt-timing fixes and the ppu_sel decode. That branch is not merged either; it pairs with the release rather than preceding it.

Opens v2.6.4 on the oracle side. The work itself is in the sibling; this is the plan document and one standing rule. ## The plan `to-dos/plans/v2.6.4-rung5-close-plan.md`. Rung 5's acceptance is a status vector that agrees ENTRY FOR ENTRY, and the gate is stated as a measurement before the work rather than after it: accuracycoin_status <ref>.ram.bin <dut>.ram.bin -> 0 of 146 differ with the standing rule for the AccuracyCoin tail applied: **a floor, not a target.** If the nine do not all close, the version ships the number it reached and names every remaining entry. It does not widen the comparison, exclude an entry, or restate the count to look better. The document records the refuted hypotheses alongside the confirmed one, because a dead hypothesis is a result: phantom PRG-RAM at $6000-$7FFF (refuted by a byte-identical re-run) and the SH implementation itself (refuted by 29,774 exact cycles with six of six mutations caught). ## The firewall rule `AGENTS.md` gains: **the provenance firewall is per-REGION, not per-repo.** "RustyNES's code is ours, so it is readable" is true of the repository and NOT uniformly true of every block in it. Eleven files carry a `// Provenance:` header disclosing that a region is derived from a GPL reference emulator, and those regions are black boxes for HDL purposes even though the file is ours -- writing SystemVerilog from them launders the original expression into the DUT through us, which is what ADR 0037 exists to prevent. Found on v2.6.4's first task: its headline job is the five SH-group stores in the DUT, and `crates/rustynes-cpu/src/cpu.rs` line 3 discloses exactly that group as derived from Mesen2's `SyaSxaAxa`. The single most relevant block of oracle source for this version is one that must not be read to write the RTL, and nothing in the tooling said so. The rule carries the maintainer-directed escalation ladder -- vendored docs, then the open Internet, then black-box output comparison, then the derived source as a last resort with the derivation declared in the SIBLING too. In practice rungs 1-3 were sufficient and the escalation went unspent, which is worth recording: the pull toward reading a working implementation is strongest exactly when the two disagree, and that is the moment the rule matters. ## Note on this branch Cut from `origin/main` at the v2.6.3 merge (700d7c5), per the standing rule to branch from `origin/main` and never from an in-flight or just-merged branch head.

…hat is misnamed The full AccuracyCoin battery is 17,868,316 CPU cycles and needs a START press at frame 300, so observing ONE entry costs a full run and yields a byte that says only pass or fail. That is the right acceptance surface for rung 5 and the wrong iteration loop, and v2.6.4 spent a working session paying for it. `scripts/accuracycoin-build/build_sub_test_rom.py` already solves this: it patches the upstream source to boot straight into one entry and assembles through wine plus the upstream `nesasm.exe`. Twenty-six such ROMs were vendored in Session 23 and nothing had been built since. Two more land here: * `sub-tests/open-bus.nes` (--suite 0 --test 7) verdict at $0408 * `sub-tests/nmi-overlap-brk.nes` (--suite 11 --test 1) verdict at $0462 Both reach their verdict from a cold boot with no input, in 4,467,08x cycles against the battery's 17.9M, and both PASS on the oracle -- so a DUT divergence against them is unambiguous. `sub-tests/iflag-latency.nes`, already vendored, reaches its verdict in 893,422 cycles, a factor of twenty. MEASURED WHILE DOING IT: `sub-tests/cpu-open-bus.nes` DOES NOT RUN `Open Bus`. Its verdict lands at $0407, which the catalog assigns to `Dummy write cycles`, and $0408 is never written -- it is off by one row of `Suite_CPUBehavior`. A valid stimulus under the wrong name, and nothing had caught it because a fixture's name is not evidence about its content; anyone reading $0408 out of its dump would have read a byte the ROM never wrote. It is kept as-is in case a gate already references it, and the README now says so plainly. The README also carries the build recipe (the four files to fetch, the suite and test indices, the wine invocation) so the next one does not have to be rediscovered, and the warning that result addresses are not always the catalog's.

…y part of the catalog `accuracycoin_status` reported total=146 pass=80 pass_with_code=8 fail=0 skipped=0 not_run=58 unknown=0 status vectors are IDENTICAL entry for entry across all 146 entries. and that sentence is true. **Fifty-eight of the 146 entries were NotRun on BOTH sides.** Two consoles agreeing about a test neither executed is not evidence about that test. The comparator was working as designed: its vacuity guard refuses an ALL-NotRun vector, which this is not. What was short was the STIMULUS WINDOW. Broken down by suite, the 600-frame golden reaches the CPU catalog and stops partway through `CPU Interrupts`: reached CPU Behavior, addressing-mode wraparound, all seven unofficial opcode groups, Unofficial Immediates, Power On State, and 2 of 3 CPU Interrupts 88 NOT reached APU Registers and DMA (10), APU Tests (9), PPU Behavior (7), PPU VBlank Timing (7), Sprite Evaluation (9), PPU Misc. (10), CPU Behavior 2 (5), and `NMI Overlap IRQ` 58 So the run that certified three closed CPU entries asked the co-simulation DUT no question at all about its PPU or its APU -- the two chips rungs 3 and 4 exist for. MEASURED, NOT ESTIMATED: 4500 frames executes all 146 (134,012,761 CPU cycles), where the oracle's own vector reads `pass=130 pass_with_code=16 fail=0 not_run=0`. WHAT LANDS HERE. The property is enforced rather than documented: * `both_not_run` counts entries NotRun on BOTH sides. A ONE-sided NotRun is a real disagreement -- it is the case the acceptance wording was written for -- and must not be absorbed into this count. * `coverage_gate` prints a coverage line on EVERY two-file comparison, so a reader who sees "identical" can tell how much of the catalog that covers, and REFUSES with a non-zero exit when the count is non-zero. * Both are extracted from `main` and tested directly. A check that exists only inside `main` is a check no test can reach, which is how this property came to be missing in the first place. Four tests, three mutations, all three CAUGHT (`both_not_run` using OR; `coverage_gate` never refusing; one dead entry tolerated). The mutation run needed its classifier fixed first: `cargo test` prints `error: test failed` when a TEST fails, so a classifier keyed on `^error` reported BUILD-FAILED for three mutations that were all catches -- discarding evidence rather than manufacturing it, but wrong either way. `Skipped` on both sides is deliberately NOT missing coverage: the ROM reached the entry and declined it, which is the same distinction the vacuity guard draws. Docs: STATUS.md, README, docs/mister.md, the plan and the mister task board all carry the coverage number beside the agreement number. v2.6.3's reading that six entries "sharing one failure code" implied one shared cause is retracted in the same pass -- `TEST_Fail` reports `(ErrorCode << 2) | 2` and the runner sets `ErrorCode` to 1 before every routine, so a code indexes within one routine and two entries sharing it share nothing. No chip-crate change, so AccuracyCoin 141/141 (RAM decoder) and nestest 0-diff hold by construction.

…greements close, and then the gate that certified them is measured to cover 88 of 146 entries A rubric is the authoritative statement of the rules, written by whoever set the test. That is literally where all three of this release's fixes came from. THE THREE ENTRIES. v2.6.3 left nine differing; the SH group closed five, and this release closes the last three. Every one is category 1 -- the implementation followed documentation that is true and insufficient. AccuracyCoin is MIT-licensed and its assembly source is one `curl` away. The v2.6.4 plan's own note that it "is not vendored" is true of this repository and had been read as "unavailable"; they are not the same statement. The source carries a prose explanation of every assertion, written by the author who chose the stimulus, and it settled all three in minutes: * `Open Bus` -- a read of $4015 does not drive the data bus, and its D5 is open bus. The nesdev pages document the $4016/$4017 case and say nothing about $4015 being exempt; the ROM states the mechanism outright, and its stimulus is the exact instruction the trace divergence had been localised to independently. * `Interrupt flag latency` -- branches poll before cycles 2 and 4 and NEVER before 3. Not in the wiki at all. * `NMI Overlap BRK` -- an interrupt sequence does not poll (stated plainly in the wiki and simply missed here), and the BRK hijack window was one CPU cycle too narrow at its late edge. TWO WRONG READINGS, BOTH KEPT WITH THEIR CORRECTIONS. A `Fail(N)` names test N of that routine, ONE-BASED: `TEST_Fail` reports `(ErrorCode << 2) | 2` and the runner sets `ErrorCode` to 1 before dispatching. Decoded as a zero-based index it is off by one, and that error made a change from test 7 to test 5 read as partial progress when it was a REGRESSION that also broke a standing gate. v2.6.3's reading that six entries "sharing one failure code" implied one shared cause is retracted with it: the code indexes within one routine, so two entries sharing it share nothing. A fix that closes one gate and opens another is a scope measurement, not a trade-off. The first poll fix also moved the GENERIC poll to the second-to-last cycle; it closed the entry and regressed `apupulse026` and `blargg08`. All three compare against the same oracle, so one model satisfies all three. Narrowed to the branch exception alone, nothing regresses. PER-ENTRY STIMULUS. `build_sub_test_rom.py` builds a ROM that boots straight into one catalog entry; twenty-six were vendored in Session 23 and nothing had been built since. Two more land here, and three are standing VERDICT gates (69 -> 72 green, 0 failed) -- verdict rather than bus by measurement, since their per-cycle surfaces are dominated by the open PPU I/O-latch item at 2,331,867 of 4,467,082 cycles on one of them. Also measured: `sub-tests/cpu-open-bus.nes` does not run `Open Bus` -- its verdict lands at $0407, which the catalog assigns to `Dummy write cycles`. THEN THE COVERAGE FINDING, WHICH IS WHAT THE RELEASE IS ACTUALLY ABOUT. With the four closed the vector reported "identical entry for entry across all 146 entries" -- with 58 of those entries `NotRun` on BOTH sides. The comparator was correct; the stimulus window was short. By suite it reaches the CPU catalog and stops partway through `CPU Interrupts`, asking nothing about the APU, PPU, sprite-evaluation or PPU-misc suites -- the chips rungs 3 and 4 exist for. Measured: 4500 frames executes all 146 (134,012,761 cycles). `accuracycoin_status` now enforces it: a coverage line on every comparison and a non-zero exit when any entry is unrun on both sides, with the predicate and the refusal extracted from `main` and tested directly. A check that exists only inside `main` is a check no test can reach, which is how this property came to be missing. 3 of 3 mutations caught. WIDENING IT PAID ON THE FIRST RUN. The wider window aborted at cycle 20,636,325, 2.8 M cycles past where every previous run had stopped: a halted CPU mid-LDA $2007 held `ppu_sel` high through a DMC steal, so on the get cycle the mux preferred the PPU and handed the sample fetch the read buffer instead of the cartridge byte. The comment above the line stated the intent it violated. RUNG 5 DOES NOT CLOSE. Measured on both sides over the same window, the oracle climbs 88 -> 95 -> 117 -> 120 -> 146 while the DUT goes 88 -> 5 -> 5 -> 5, flat for 74 M cycles. That is a hang in the suites that had never executed, not a restart. Reported as unavailable rather than as "141 of 146 differ", which is one defect and not 141. VERIFICATION sibling ./tb/regress.sh 72 passed, 0 failed RTL mutations 7 of 8 CAUGHT; the eighth INERT by byte-compare accuracycoin_status mutations 3 of 3 CAUGHT AccuracyCoin (RAM decoder) pass rate = 100.00% over 141 assigned tests nestest 0-diff release_anchor_audit 11 passed libretro_info_audit 3 passed fmt / clippy -D warnings / rustdoc / no_std / markdownlint clean No `rustynes-{cpu,ppu,apu,mappers,core}` changes, so the two accuracy numbers hold by construction -- and were run anyway.

doublegate · 2026-08-26T08:58:26Z

@coderabbitai review

coderabbitai · 2026-08-26T08:58:29Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1083a928-3e14-40df-8f14-0e288a59c58f

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

Create stacked PR
Commit on current branch

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/v2.6.4-rung5-close

Warning

Your free Security trial is over. An organization admin can activate billing to continue.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai · 2026-08-26T08:58:43Z

⚠️ Action not completed

Head commit changed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…s not hang in a test The release notes and CHANGELOG published an hour ago recorded the DUT as hung in the PPU suites, on the strength of catalog order: `PPU Behavior` follows `Power On State`, and those suites had never executed. Reasonable, and wrong. Probing the PC puts the DUT in a three-cycle self-loop at $80DF, which the ROM spells `INC $EC` / `JMP $80DF` -- the MENU IDLE LOOP, where AccuracyCoin spins while its NMI handler does the work. Only the five results the power-on path writes are present, so the console re-ran power-on and returned to the menu: a RESET. It then idles because START is pressed once, at frames 300-306. Corrected in place rather than deleted: the wrong reading was published, and the thing that separated it from the right one was one probe rather than more thought.

doublegate · 2026-08-26T09:00:26Z

Correction to the rung-5 carry, pushed as bfe633ca.

The release notes and CHANGELOG originally recorded the DUT as hung in the PPU suites, reasoning from catalog order: PPU Behavior follows Power On State, and those suites had never executed. Reasonable, and wrong.

Probing the PC puts the DUT in a three-cycle self-loop at $80DF:

NMIP cyc=59999996 sl=193 dot=300 ppu_nmi_n=1 sync=1 pc=80DF
NMIP cyc=59999997 sl=193 dot=303 ppu_nmi_n=1 sync=0 pc=80E0
NMIP cyc=59999998 sl=193 dot=306 ppu_nmi_n=1 sync=0 pc=80E1
NMIP cyc=59999999 sl=193 dot=309 ppu_nmi_n=1 sync=1 pc=80DF

The ROM at $80DF is E6 EC 4C DF 80 — INC $EC / JMP $80DF, AccuracyCoin's menu idle loop, where the menu spins while its NMI handler does the work.

So the DUT is not stuck inside a test. It is back at the menu with only the five results the power-on path writes, which means the console re-ran power-on: a reset, not a hang. It then idles indefinitely because START is pressed once, at frames 300-306.

That is a different defect and a different search for v2.6.5 — bracket where the result count collapses between 17.9M and 60M, rather than looking for what blocks a PPU test. Corrected in place rather than deleted, because the wrong reading was published and what separated it from the right one was one probe rather than more thought.

github-actions · 2026-08-26T09:01:43Z

Antigravity review (Gemini via Ultra)

This PR introduces a coverage check in accuracycoin_status to refuse comparisons where tests are NotRun on both the oracle and DUT, fixing earlier logic that reported partial runs as complete matches.

Blocking issues

Silent failure path: both_not_run and executed_on_both use .zip(b), which silently truncates the iteration if a and b have unequal lengths. While there is an assert_eq! in coverage_line, both_not_run is called in coverage_gate before this assertion. If upstream length guarantees ever fail, both_not_run will silently return a truncated, incorrect count. Length validation must happen inside or prior to these helper functions to prevent silent logic errors.

Suggestions

Redundant computation: both_not_run(a, b) is called in coverage_gate and then immediately recomputed inside coverage_line(a, b). Pass the computed dead variable as an argument to coverage_line to avoid iterating over the slices twice. (Cite: crates/rustynes-test-harness/src/bin/accuracycoin_status.rs inside coverage_gate and coverage_line).
Error handling: Replace the assert_eq! length check in coverage_line with a Result<String, _> return type to strictly align with the style guide's preference for typed results over panics, even for internal invariants.

Nitpicks

In vacuous, replacing matches!(s, TestStatus::NotRun) with *s == TestStatus::NotRun works but is unnecessary churn.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-26 10:16 UTC

Antigravity review (Gemini via Ultra)

Error: timeout waiting for response

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-26 09:37 UTC

Antigravity review (Gemini via Ultra)

This PR cuts the v2.6.4 release, documenting recent accuracy fixes, adding new sub-test ROMs, and strengthening the accuracycoin_status gate to refuse comparisons where tests were not run on both sides.

Blocking issues

None found.

Suggestions

In crates/rustynes-test-harness/src/bin/accuracycoin_status.rs at coverage_line, the formula a.len() - both - dead assumes a and b are exactly the same length. Since both and dead are computed via zip (which silently truncates to the shortest slice), a length mismatch would incorrectly misclassify the remainder of a as "on one side only". Consider adding a quick assert_eq!(a.len(), b.len()); guard to make this assumption explicit.

Nitpicks

In both_not_run and executed_on_both, you could use matches!(**x, TestStatus::NotRun) instead of **x == TestStatus::NotRun to match the style established in vacuous.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-26 09:32 UTC

Antigravity review (Gemini via Ultra)

This PR resolves four AccuracyCoin test disagreements by correcting emulator rules based on the test ROM's source, and updates the accuracycoin_status gate to enforce full catalog execution.

Blocking issues

None found.

Suggestions

crates/rustynes-test-harness/src/bin/accuracycoin_status.rs:783: In coverage_line, a.len() - both - dead assumes a and b have identical lengths. Since zip silently truncates to the shorter slice, a mismatched length could cause a misleading "one side only" count. Consider asserting a.len() == b.len().

Nitpicks

crates/rustynes-test-harness/src/bin/accuracycoin_status.rs:851: for e in dut.iter_mut().take(5) { *e = TestStatus::Pass; } could be simplified to dut[..5].fill(TestStatus::Pass).

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-26 09:28 UTC

Antigravity review (Gemini via Ultra)

This PR cuts the v2.6.4 "Rubric" release, updating documentation to reflect the latest accuracy measurements and fixing the test harness to correctly report when catalog entries fail to execute on either side.

Blocking issues

Contradictory release claims: The PR title and .github/release-notes/v2.6.4.md claim "the last four AccuracyCoin disagreements close", but the body of the release notes says it "closes the last three". Further contradicting both, docs/mister.md states it "closed two of the last three... NMI Overlap BRK is carried to v2.6.5". The actual state of the fixes is incoherent across the documents.
Broken build script (correctness): In tests/roms/AccuracyCoin/README.md, the instructions run cd /tmp/accoin-src and then immediately invoke python3 scripts/accuracycoin-build/build_sub_test_rom.py. This fails with a "No such file or directory" error because the working directory has changed to /tmp, breaking the relative paths to the python script and the --out destination.

Suggestions

AGENTS.md (line 199): The recommended command grep -rn "^// Provenance:" crates/*/src/*.rs misuses -r. Providing a file glob (*.rs) prevents grep from recursing into subdirectories. Use grep -rn --include="*.rs" "^// Provenance:" crates/*/src/ to properly search recursively.

Nitpicks

crates/rustynes-test-harness/src/bin/accuracycoin_status.rs: Consider adding assert_eq!(a.len(), b.len()) in both_not_run and executed_on_both. While a and b currently have the same length, zip() silently truncates mismatched lengths, which could mask logic errors in the future.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-26 09:11 UTC

Antigravity review (Gemini via Ultra)

This PR finalizes the v2.6.4 release by updating project documentation, adjusting release notes, and introducing a coverage gate to the test harness to prevent passing on partial test runs.

Blocking issues

Correctness (Logging logic): In crates/rustynes-test-harness/src/bin/accuracycoin_status.rs, the calculation a.len() - dead is reported in the console output as the number of "entries executed on both sides". Because dead only tallies entries that are NotRun on both sides, any entry that is NotRun on only one side is mathematically included in a.len() - dead and falsely reported to the user as having executed on both.

Suggestions

In crates/rustynes-test-harness/src/bin/accuracycoin_status.rs (line 864), update the console output text to read "executed on at least one side" to accurately reflect the calculation, or alter the math to measure the strict intersection of tests executed by both sides.
Consider completely removing the incorrectly named cpu-open-bus.nes file (mentioned in tests/roms/AccuracyCoin/README.md), rather than leaving it in place, to avoid future confusion or accidental use.

Nitpicks

The docstrings for both_not_run (line 831) and coverage_gate (line 850) in crates/rustynes-test-harness/src/bin/accuracycoin_status.rs have significant overlap in their explanations and could be condensed.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-26 09:08 UTC

Antigravity review (Gemini via Ultra)

Cuts the v2.6.4 "Rubric" release, updating documentation and adding a strict coverage gate to the test harness to refuse comparisons if any test is NotRun on both sides.

Blocking issues

Correctness: In crates/rustynes-test-harness/src/bin/accuracycoin_status.rs (around line 102), coverage_gate calculates a.len() - dead and reports it as the number of entries executed on both sides. Since dead only counts entries where both sides are NotRun, subtracting it from the total yields the number of entries executed by at least one side. This will falsely report unilateral execution as full mutual coverage if one emulator hangs.

Suggestions

crates/rustynes-test-harness/src/bin/accuracycoin_status.rs (around line 81): Assert a.len() == b.len() in both_not_run. The zip iterator silently truncates to the shorter length, which could mask bugs if the input slice lengths ever differ.
crates/rustynes-test-harness/src/bin/accuracycoin_status.rs (around line 102): Fix the reporting calculation by explicitly counting mutual execution: a.iter().zip(b).filter(|(x, y)| !matches!(x, TestStatus::NotRun) && !matches!(y, TestStatus::NotRun)).count().
The release notes mention that a "wrong comment is kept beside the fix" for the BRK hijack window. If that comment is in this repository, correct or remove it; retaining known-false comments actively misleads future maintainers.

Nitpicks

None.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-26 09:05 UTC

Antigravity review (Gemini via Ultra)

This PR bumps the project version to v2.6.4, updates documentation for the latest AccuracyCoin fixes, and makes a trivial code change adding a coverage gate to accuracycoin_status to prevent accepting partial test runs.

Blocking issues

None found.

Suggestions

crates/rustynes-test-harness/src/bin/accuracycoin_status.rs:835: The coverage_gate output states coverage: {} of {} entries executed on both sides. However, it calculates the count as a.len() - dead. Since dead only counts entries NotRun on both sides, a.len() - dead actually counts entries executed on at least one side. Consider either updating the message to "executed on at least one side" or computing the actual intersection (e.g., a.iter().zip(b).filter(|(x, y)| *x != TestStatus::NotRun && *y != TestStatus::NotRun).count()).

Nitpicks

crates/rustynes-test-harness/src/bin/accuracycoin_status.rs:818: matches!(x, TestStatus::NotRun) can be simplified to *x == TestStatus::NotRun since TestStatus derives PartialEq.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

… state Five rules into AGENTS.md, each earned in this cycle: * A test ROM's own source is the SPECIFICATION, and it may be one `curl` away. AccuracyCoin is MIT-licensed; its comments named three mechanisms in minutes that a full session of tracing had not. A test ROM is stimulus, not a reference implementation, so no firewall question arises. Decode its failure codes from the macro -- `Fail(N)` is one-based, and reading it as an index made a regression read as progress. * A pass count is a claim about what RAN. 146 of 146 'identical' with 58 entries NotRun on both sides. Break results down by group and look for a group at zero; write acceptance a vacuous result cannot satisfy; and treat coverage work as load-bearing, since widening the window found a real defect on its first run. * Comparing at a fixed cycle count assumes both sides reach the same PLACE. Measure the reference over the same axis, do not conclude from two points, and when the question is 'where did it stop', probe the program counter -- catalog order is a suspect, not evidence. * `cargo test` prints `error:` on a TEST FAILURE, so a classifier keyed on `^error` turns catches into BUILD-FAILED. Run the baseline through the same classifier. * An abort path does not flush the trace; use an env-gated per-cycle probe. CLAUDE.local.md moves to the v2.6.4 position and leads with the one thing to pick up first: the DUT RESETS between 17.9 M and 60 M cycles and returns to AccuracyCoin's menu idle loop at $80DF. It is not hung in a PPU test.

…LEAST ONE side, not both Found in review of the change that introduced it, on a release whose subject is a count that described a set it did not measure. `coverage: {} of {} entries executed on both sides` printed `a.len() - dead`, where `dead` counts entries NotRun on BOTH sides. That subtraction is the number executed on at least ONE side. Against the actual run it is not a subtle difference: the reference had zero NotRun, so `dead` was 0 and the line claimed all 146 entries ran on both sides while the DUT had executed five. `executed_on_both` counts what the sentence claims -- neither side NotRun -- and the line now also breaks out how many ran on neither and on exactly one side, which is the shape that would have made the original wrong number obvious. THE FIRST FIX WAS NOT ENOUGH AND THE MUTATION SAID SO. With the arithmetic corrected, reverting the LINE to the subtraction came back NOT CAUGHT: the tests asserted on `both_not_run` and `executed_on_both`, and nothing reached the message. The defect was in the sentence. `coverage_line` is therefore built rather than printed, and a test reads it -- after which the same mutation is CAUGHT. Also applies the review's nitpick: `TestStatus` derives `PartialEq`, so the predicates compare directly instead of via `matches!`. Two tests added (12 total). Mutations: the sentence reverting -> CAUGHT; `executed_on_both` using OR -> CAUGHT.

doublegate · 2026-08-26T09:05:44Z

Thanks — the coverage_gate suggestion is correct and is fixed in 7e0cdf0f, along with the matches! nitpick.

The suggestion was right, and against the real run it is not subtle

a.len() - dead counts entries executed on at least one side, exactly as stated. On the run this release actually took, the reference had zero NotRun, so dead was 0 and the line claimed all 146 entries ran on both sides while the DUT had executed five. On a release whose subject is a count that described a set it did not measure, that is the same defect one level up.

executed_on_both now counts what the sentence claims — neither side NotRun — and the line breaks out how many ran on neither and on exactly one side, which is the shape that would have made the original number obviously wrong:

coverage: 5 of 146 entries executed on both sides (0 on neither, 141 on one side only)

The first fix was not enough, and a mutation said so

With the arithmetic corrected I mutated the line back to the subtraction. NOT CAUGHT — my tests asserted on both_not_run and executed_on_both, and nothing reached the message. The defect you found was in the sentence, so the sentence had to become something a test can read: coverage_line is built rather than printed, and after that the same mutation is CAUGHT.

Two tests added (12 total). Mutations: sentence reverted → CAUGHT; executed_on_both using OR → CAUGHT.

On the nitpick

Applied — TestStatus derives PartialEq, so both predicates now compare directly.

doublegate · 2026-08-26T09:12:26Z

@coderabbitai review

coderabbitai · 2026-08-26T09:12:32Z

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…l gate and I had not run it CI failed `release_notes_are_not_hard_wrapped` on BOTH test legs. The notes file was hand-wrapped at ~76 columns; GitHub renders release bodies with GFM hard line breaks, so every one of those newlines becomes a <br> and the notes would have displayed as a ragged column broken mid-sentence. Fixed with the tool the audit's own failure message names, `scripts/release-automation/reflow.py`. TWO THINGS WORTH RECORDING. The failure arrived first on `test (ubuntu-24.04-arm)`, a leg this project's notes record as having a history of apt provisioning stalls -- so it wore the costume of a known flake. It is not one, and the `ubuntu-latest` leg failing on the identical assertion settles it: this is a property of a file, not of a runner. A documented flake history makes a real failure easier to dismiss, not harder to spot. The ordering miss is the real one. Phase 3's gates ran BEFORE the release ceremony created `.github/release-notes/v2.6.4.md`, the new CHANGELOG section, the bumped anchors and the VERSION-PLAN row. A local suite cannot audit files that do not yet exist, so "all gates green" was true of a tree without the release's own artifacts in it. The gate order wants a second pass: build -> gates -> docs -> ceremony -> gates again. Re-run here, workspace clean.

…DMA, not four and no DMA A scope error in my own release notes, caught by their internal inconsistency: the headline said "the last four disagreements close" while the body said "the SH group closed five, and this release closes the last three". Five plus three is eight, and v2.6.3 left nine. Checking the branch rather than my memory of it settles it. The sibling carries `d0fa185` ("all five SH entries close") and `9701445` ("OAM DMA -- the 513 cycles the console was never spending"), so BOTH are v2.6.4. The release closes all nine, and its largest single deliverable -- OAM DMA -- was missing from the notes entirely, because I described the third of the release I had worked on. Now recorded: $4014 was a register the console decoded and then did nothing with. OAM DMA lands as a real bus master from `nesdev_wiki/DMA.xhtml` -- halt on a read cycle, optional alignment, 256 read/write pairs -- with the documented DMC-get precedence costing OAM its alignment as well as its slot, and a DMA write now driving the open-bus latch. Its halt and alignment were fitted to the oracle first and corrected from the wiki, which is in the ledger rather than squashed. The SH group closes in two steps, the second named by the residual of the first. Those took the vector 9 -> 3; the three in the notes are the tail. Also corrects the VERSION-PLAN row, which still carried the retracted "hang" reading of the rung-5 carry. It is a RESET -- the DUT sits in AccuracyCoin's menu idle loop at $80DF. The anchor lead is corrected in all eight places it appears across seven documents. Audits: release_anchor_audit 11 passed, release_notes_render_audit 2 passed, markdownlint clean.

…ered The verification table's "RTL mutations: 7 of 8 CAUGHT" was my own session's run against the interrupt-timing work, presented as the release's figure. Same shape as the scope error one commit earlier: a number from the part of the release I worked on, standing for the whole. The SH group carried its own run and `280e065` records it -- PROGRAM46 cleared the implementation at 29,774 of 29,774 cycles exact with SIX OF SIX mutations caught, covering the value rule, the page-cross address mangle and register selection across all five opcodes. Both rows are now labelled by subject. Not aggregated into one number, deliberately: they are different gates against different stimulus, and a summed "13 of 14" would imply a single sweep that was never run.

doublegate · 2026-08-26T09:32:25Z

Two scope corrections to the release notes, pushed as 4b2d0aea and cc470b8b. Both were mine, and both were caught by the notes contradicting themselves rather than by a reviewer.

The notes described one third of the release

The headline said "the last four disagreements close" while the body said "the SH group closed five, and this release closes the last three". Five plus three is eight, and v2.6.3 left nine.

git log --oneline origin/main..HEAD --reverse in both repos settles it: the branch also carries d0fa185 ("all five SH entries close") and 9701445 (OAM DMA — "the 513 cycles the console was never spending"). So v2.6.4 closes all nine, and its largest single deliverable was missing from the notes entirely — because I wrote them from the session I remembered rather than from the branch.

$4014 was a register the console decoded and then did nothing with. OAM DMA now lands as a real bus master from nesdev_wiki/DMA.xhtml — halt on a read cycle, optional alignment, 256 read/write pairs — with the documented DMC-get precedence costing OAM its alignment as well as its slot, and a DMA write driving the open-bus latch. Its halt and alignment were fitted to the oracle first and corrected from the wiki, which is in the ledger rather than squashed.

The mutation figure had the same shape

RTL mutations: 7 of 8 CAUGHT was one session's run against the interrupt-timing work, presented as the release's number. The SH group carried its own: 280e065 records PROGRAM46 clearing the implementation at 29,774 of 29,774 cycles exact with 6 of 6 mutations caught, covering the value rule, the page-cross address mangle and register selection across all five opcodes.

Both rows are now labelled by subject, and deliberately not summed — a "13 of 14" would imply one sweep that was never run and would lose the only useful property of the numbers, which is which gate found what.

The VERSION-PLAN row also still carried the retracted hang reading of the rung-5 carry; it says reset now.

Audits after both: release_anchor_audit 11 passed, release_notes_render_audit 2 passed, markdownlint clean.

…th vectors Raised in review, and the claim is conditionally true: `coverage_line` subtracts two `zip`-derived counts from `a.len()`, and `zip` truncates silently to the shorter side -- so unequal lengths would report the tail of `a` as "on one side only". That would be a THIRD wrong count in a tool whose subject is counts that describe the wrong set. The condition is unreachable today, and saying so is part of the answer: both vectors come from `decode_results`, which returns one entry per catalog entry or refuses the dump, and the file already tests exactly that (`decoded_vectors_are_always_catalog_length`, `a_short_dump_is_refused`). Added anyway, because the coupling is invisible at the point that depends on it, and anything that later feeds `coverage_line` from another source gets a loud failure instead of a plausible number. Demonstrated to fire by a `should_panic` test -- the only thing that can reach it, since `main` cannot -- and removing the guard is a CAUGHT mutation. The round's nitpick asked for `matches!` where the PREVIOUS round asked for `==`, so rather than flip-flop the file is unified on `==`: `vacuous` now matches the two predicates instead of the other way round. HOW THIS WAS NEARLY MISSED, which is the part worth recording. The reviewer edits ONE comment per PR and folds earlier rounds into a collapsed archive. Its visible head now reads only "Error: timeout waiting for response" -- the round against the final head timed out -- so the PR showed a green build, zero unresolved threads, and a comment that appeared to contain nothing. The finding was inside the <details> archive, and the only tell was `updated_at` (09:37) differing from `created_at` (09:01).

doublegate added 4 commits August 26, 2026 00:15

Copilot AI lite review requested due to automatic review settings August 26, 2026 08:58

Copilot started reviewing on behalf of doublegate August 26, 2026 08:58 View session

Copilot AI reviewed Aug 26, 2026

doublegate added 2 commits August 26, 2026 05:02

doublegate added 3 commits August 26, 2026 05:24

doublegate merged commit 2a64d09 into main Aug 26, 2026
29 checks passed

doublegate deleted the feat/v2.6.4-rung5-close branch August 26, 2026 11:18

Uh oh!

Conversation

doublegate commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this is

The three entries, and where their rules live

Two wrong readings, both kept with their corrections

The coverage finding — what the release is actually about

Rung 5 does NOT close

Verification

What a reviewer should look at hardest

Uh oh!

doublegate commented Aug 26, 2026

Uh oh!

coderabbitai Bot commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

doublegate commented Aug 26, 2026

Uh oh!

github-actions Bot commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Antigravity review (Gemini via Ultra)

Blocking issues

Suggestions

Nitpicks

Antigravity review (Gemini via Ultra)

Antigravity review (Gemini via Ultra)

Blocking issues

Suggestions

Nitpicks

Antigravity review (Gemini via Ultra)

Blocking issues

Suggestions

Nitpicks

Antigravity review (Gemini via Ultra)

Blocking issues

Suggestions

Nitpicks

Antigravity review (Gemini via Ultra)

Blocking issues

Suggestions

Nitpicks

Antigravity review (Gemini via Ultra)

Blocking issues

Suggestions

Nitpicks

Antigravity review (Gemini via Ultra)

Blocking issues

Suggestions

Nitpicks

Uh oh!

doublegate commented Aug 26, 2026

The suggestion was right, and against the real run it is not subtle

The first fix was not enough, and a mutation said so

On the nitpick

Uh oh!

doublegate commented Aug 26, 2026

Uh oh!

coderabbitai Bot commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doublegate commented Aug 26, 2026

The notes described one third of the release

The mutation figure had the same shape

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

doublegate commented Aug 26, 2026 •

edited

Loading

coderabbitai Bot commented Aug 26, 2026 •

edited

Loading

coderabbitai Bot commented Aug 26, 2026 •

edited

Loading

github-actions Bot commented Aug 26, 2026 •

edited

Loading

coderabbitai Bot commented Aug 26, 2026 •

edited

Loading