ci(test): gate the goldens on PRs; keep run-ahead opt-in on measurement - #277
Conversation
Third and final PR of the `v1.26.0` hardening rung. ## The golden vectors ran in no pull-request job Only `lint`, `test-light`, and `accuracysnes` run on a PR. `test-light` never passes `--features test-roms`, and the `accuracysnes` job scoped itself to `--test accuracysnes`. So the 53 rendered-scene goldens and every framebuffer golden were reached only by `full-test`, which is `mode == 'full'` -- `main`, tags, the weekly cron. A PR could shift PPU behaviour, move a golden, and hear nothing until merge-to-main. That is the same structure behind this project's own coprocessor-golden staleness (goldens left stale by post-bless PPU accuracy fixes): a golden vector nothing executes only accumulates drift. Here it was not that nothing ran them ever, but that nothing ran them while it was still cheap to fix. The job now also runs the scene, undisbeliever, rainwarrior, coprocessor, and save-state suites. Cost is bounded and known: ~5 minutes for the three with a committed corpus (scenes ~84s, undisbeliever ~158s, rainwarrior ~54s). The `*_oncart` suites need gitignored commercial dumps and firmware, so they self-skip for free; they are listed anyway so the intent is explicit rather than inferred from an omission, and so they start gating the moment a runner ever has that corpus. Found while looking for RustySNES's analogue of the sibling's `expansion_level_tripwire`. That mechanism does NOT port, and saying so matters more than inventing a stand-in: the only `commercial-roms`-gated suite here (`commercial_screenshots`) is a screenshot generator that asserts nothing, so it holds no golden that can go stale, and PPU accuracy is not parameterised by a small constant set the way expansion-audio levels are. Looking for it surfaced a larger real gap. ## Run-ahead stays opt-in -- measured, not assumed `docs/frontend.md` recorded the per-frame save-state allocation as THE blocker on run-ahead going default-on. `v1.25.0` removed that allocation, so the question was re-measured rather than assumed resolved: save_state ~119 us load_state ~285 us save/load round trip ~0.40 ms one emulated frame 6.39 ms (headless_frame_steady_state) NTSC frame budget 16.64 ms The round trip is 2.4% of the budget. It was never the dominant cost, and removing the allocation -- a real improvement -- did not move the decision. What run-ahead costs is the extra frame of emulation, inherent to the technique: `frames = 1` needs 79% of the budget, leaving ~3.5 ms for present/UI/audio on a fast development machine; `frames = 2` needs 118% and cannot hold 60 fps at all. So it stays opt-in. The plan for this rung said "default-on only if the measurement supports it, and report the number either way" -- it does not, and this is the number. One thing did change as a result. `RunAheadConfig`'s `Default` is now hand-written so `throttle_ms` defaults to 14 ms (ARMED) rather than the derived `0.0` (DISABLED). The throttle is what its own doc calls the thing that keeps run-ahead from turning a latency win into stutter, and the derived default left it off for precisely the user who had just enabled run-ahead from Settings and had no `throttle_ms` line in their config. 14 ms sits below the 16.64 ms NTSC deadline with headroom and is conservative against PAL's 20 ms. An existing config that spells the field out keeps its value -- `#[serde(default)]` fills only absent fields, pinned by a negative-control test alongside the positive one. ## Lockstep Log row for RustyNES `v2.2.1`-`v2.2.4`, and the two 2026-07-12 flags CORRECTED: the GIF/WAV capture subsystem and CRT shader-preset depth were both closed by `v1.25.0` (`av_record.rs`; the multi-pass stack + richer CRT + NTSC pass + `.slangp` import with the naga bridge), yet the roadmap still asked for a maintainer go/no-go on them two weeks later. Also carries the verification comment for #276's declined blocking finding (that `git/matching-refs` 404s on an absent tag -- it returns HTTP 200 with an empty array; `git/ref/tags` is the one that 404s, which is why it was rejected). Documentation of that adjudication, not a fix. Verified: `cargo fmt --check`, frontend clippy `-D warnings`, the new config tests, and the golden suites run under the exact multi-`--test` invocation the CI step uses. 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 (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
🧰 Additional context used📓 Path-based instructions (8)crates/**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.rs📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,md}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
crates/rustysnes-*/**/*📄 CodeRabbit inference engine (Custom checks)
Files:
crates/rustysnes-frontend/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,toml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
crates/**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (3)
WalkthroughThe PR expands AccuracySNES CI coverage, documents release tag lookup behavior, changes the default run-ahead throttle to 14 ms while keeping run-ahead disabled, adds validation tests and timing documentation, and updates project status records. ChangesCI and release automation
Run-ahead configuration
Project status records
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
…budget Two review suggestions applied, both verified rather than taken on description -- and the first one corrected my own wrong assumption. `(x - 0.0).abs() < f32::EPSILON` -> `x == 0.0`. I expected this to trip `clippy::float_cmp` (the workspace runs `pedantic` at `-D warnings`) and was going to decline it on that basis; testing showed clippy exempts a constant comparison, so the suggestion is right and the epsilon form was just noise. The FORM matters though, and the comment now says so: the apparently-more-idiomatic `assert_eq!(x, 0.0)` DOES fire `float_cmp` and fails the gate, so `assert!(x == 0.0)` is the spelling that works. The magic `1000.0 / 60.0988` now derives from `Region::Ntsc.frame_rate()` so the two cannot drift apart, compared in `f64` (what `frame_rate()` returns) to avoid a lossy `as f32` the pedantic cast lints would object to. Declined: the PR title is 71 characters, not the 73 the review reported, so it is already inside the 72-character limit. Verified: `cargo fmt --check`, frontend clippy `-D warnings`, and `config::tests` 13/13. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review adjudicationNo blocking issues. Three findings — two applied, one declined with a measurement. One of the applied ones corrected an assumption of mine. Applied:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rustysnes-frontend/src/config.rs`:
- Around line 922-967: Update the tests
run_ahead_is_off_by_default_but_its_throttle_is_armed and
an_explicit_throttle_of_zero_survives_the_new_default to assert the exact 14.0
ms throttle_ms value for RunAheadConfig::default() and for deserialization when
throttle_ms is omitted, while retaining the explicit 0.0 opt-out assertion and
existing frames checks.
In `@to-dos/LOCKSTEP-CHECKLIST.md`:
- Line 56: Correct the historical CI hardening counts in the checklist entry:
state that all 17 checkouts lacked persist-credentials: false before v1.26.0 and
that all 32 action references used mutable tags. Leave the release-auto.yml:96
finding and the v1.26.0 disposition unchanged.
🪄 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: bfc3b254-ba7b-4f80-9765-e1774907fbb2
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/release-auto.ymlCHANGELOG.mdcrates/rustysnes-frontend/src/config.rsdocs/frontend.mdto-dos/LOCKSTEP-CHECKLIST.mdto-dos/ROADMAP.md
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test-light
- GitHub Check: lint
- GitHub Check: accuracysnes
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{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:
to-dos/ROADMAP.mddocs/frontend.mdto-dos/LOCKSTEP-CHECKLIST.mdcrates/rustysnes-frontend/src/config.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:
to-dos/ROADMAP.mddocs/frontend.mdto-dos/LOCKSTEP-CHECKLIST.mdcrates/rustysnes-frontend/src/config.rsCHANGELOG.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:
to-dos/ROADMAP.mddocs/frontend.mdto-dos/LOCKSTEP-CHECKLIST.mdCHANGELOG.md
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: Flag third-party actions pinned to a branch rather than a tag or commit SHA, jobs without
a least-privilegepermissions:block, and secrets that could reach the log. The
ci-successjob is the required check and must depend on every gate it claims to
aggregate — a new job that is not in itsneeds:list is a gate nothing enforces.
Files:
.github/workflows/release-auto.yml.github/workflows/ci.yml
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/frontend.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/frontend.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-frontend/src/config.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-frontend/src/config.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-frontend/src/config.rs
crates/rustysnes-frontend/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Frontend code is one of the permitted locations for
unsafe; any unsafe use still requires a// SAFETY:comment.
Files:
crates/rustysnes-frontend/src/config.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-frontend/src/config.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-frontend/src/config.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
🪛 LanguageTool
to-dos/LOCKSTEP-CHECKLIST.md
[style] ~56-~56: This sentence may be long and difficult for your reader to follow. Consider inserting a period and starting a new sentence here.
Context: ...DIATELY** in v1.26.0 — all three items, with one divergence found by auditing per-site rather than copyin...
(WITH_THE_SENTENCE)
[grammar] ~56-~56: Ensure spelling is correct
Context: ...compile gate that does not exist there, where RustySNES's has both a per-PR compile gate and a sc...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (8)
.github/workflows/release-auto.yml (1)
126-138: LGTM!to-dos/ROADMAP.md (1)
387-397: LGTM!crates/rustysnes-frontend/src/config.rs (2)
583-602: LGTM!
613-637: LGTM!CHANGELOG.md (2)
12-32: LGTM!
33-59: LGTM!docs/frontend.md (1)
587-625: LGTM!.github/workflows/ci.yml (1)
341-371: LGTM!
| | 2026-07-11 | `v2.1.5` (released) / `main` (v2.1.6 "Expansion Audio" in progress) | Initial baseline for the RustyNES-parity roadmap — see the full gap analysis in the roadmap plan. | Roadmap `v1.5.0`–`v1.19.0` scoped against this baseline. | First entry; establishes the checklist itself. | | ||
| | 2026-07-12 | `v2.1.10` / `main` | 5 RustyNES releases since baseline (`v2.1.6`–`v2.1.10`). New: a GIF/WAV screen+audio capture subsystem (`v2.1.9`); 3 marquee libretro-slang CRT presets (CRT-Royale, guest-advanced, Sony Megatron, `v2.1.9`) vs. RustySNES's own single scanline+aperture-mask `Crt` filter; a SIMD software blitter + wasm-size/startup pass (`v2.1.8`); browser RetroAchievements + Vs. DualSystem libretro support (`v2.1.10`). Raw NTSC composite-signal decode (`v2.1.9`) confirmed genuinely NES-specific (8:1 dot-clock subsampling, no SNES analog) — validates `v1.12.0`'s own out-of-scope call. `v2.1.6`/`v2.1.7` (expansion-audio oracle, 2A03/PPU die-revision modeling) are NES-chip-specific, not applicable. | Capture subsystem: **large catch-up**, no RustySNES rung exists — flagged in `to-dos/ROADMAP.md`'s "Milestones beyond the phases" for a maintainer go/no-go, not silently scoped in. CRT-preset depth: **medium catch-up**, noted as a future-rung candidate, not urgent (RustySNES's existing `Crt` filter is functional). SIMD blitter/wasm pass: **small catch-up**, could fold into `v1.19.0 "Afterburner"` alongside PGO/BOLT when that rung is scoped. Browser RA + Vs. DualSystem: correctly out of scope (RustySNES's RA is native-only; no Vs. System equivalent on SNES). Raw-composite: reconfirmed already-covered/correctly-deferred. | First re-run since baseline — overdue (8 RustySNES releases, `v1.5.0`–`v1.12.0`, had landed with no lockstep check in between); run at the start of scoping `v1.13.0`. | | ||
| | 2026-07-15 | `v2.2.0 "Capstone"` / `main` | RustyNES cut `v2.2.0` since the last check (`v2.1.10`), plus an `[Unreleased]` Dependabot-consolidation PR. New in `v2.2.0`: netplay lobby/matchmaking (browse-and-join room directory + server-side quick-match), delayed-stream spectators, a graded hysteresis-backed desync verdict, peer-liveness RTT timeouts (all `rustynes-netplay` signaling depth); fuzz-target expansion from 3 → 8 cargo-fuzz targets (PPU/APU reg I/O, netplay message parsing, save-state, movie); a read-only ROM Info debugger panel (CRC32/SHA-256/header decode); 4 new MkDocs handbook pages; FDS medium-model completion (CRC-16/gap/continuous head-seek) + Famicom mic + Zapper 3×3 aperture hardening (peripherals); a fuzz-found movie-deserializer OOM-DoS hardening (untrusted `frame_count`/zero-`bytes_per_frame`). Checked RustySNES's own state directly (not assumed): confirmed no `fuzz/` directory exists at all; confirmed `rustysnes-netplay` has no `SignalMessage`/`ListRooms`/`QuickMatch`/`delay_frames`/`DesyncStatus`/`PeerLink` equivalents; confirmed `rustysnes-frontend/src/debugger/` has no ROM-Info-panel equivalent; confirmed `rustysnes-core::movie`'s deserializer is ALREADY hardened against the identical untrusted-`frame_count`/zero-length-frame OOM class (a `deserialize_rejects_a_forged_huge_frame_count_without_oom` regression test already exists — independently discovered, not copied); confirmed RustySNES's `SuperScopeState` uses a purely geometric (coordinate-clamp) offscreen check, architecturally different from a photodiode-luma-sampling model, so the Zapper aperture technique doesn't map onto it. FDS/Famicom-mic reconfirmed NES/Famicom-specific, not applicable. | **No fuzzing infrastructure at all**: **large catch-up** — genuinely new quality-infra category (not incremental growth of anything RustySNES has), and this project's own `docs/testing-strategy.md` already names fuzzability as an intended Layer 1 property never actually built out; flagged in `to-dos/ROADMAP.md`'s "Milestones beyond the phases" for a maintainer go/no-go. **Netplay lobby/matchmaking + spectator/desync/liveness depth**: **large catch-up** — multiple sub-features, would displace no currently-open rung (the ladder is closed) but is genuinely multiple PRs' worth; the `v1.5.0`-era "netplay already at parity" call is now stale against this deepened baseline; also flagged in `ROADMAP.md`. **ROM Info debugger panel**: **small catch-up**, self-contained, fits cleanly into the existing `debugger/` module architecture — candidate for the next time a small patch/quality release is scoped. **MkDocs handbook pages**: already covered in spirit (RustySNES already maintains its own MkDocs site, updated per-release; no distinct action). **Movie-deserializer OOM-DoS hardening**: already covered — RustySNES independently already has equivalent protection. **Zapper aperture hardening**: not directly applicable — architecture mismatch (RustySNES's Super Scope model doesn't sample framebuffer luma at all). **FDS/Famicom mic**: correctly out of scope (no SNES equivalent hardware). **Dependency-consolidation PR**: routine Dependabot hygiene, not a roadmap-scope item. | First re-run since the roadmap ladder closed at `v1.19.0` — confirms the checklist itself stays a live, standing practice even with no open rung to fold findings into; both large-catch-up items intentionally left unscoped pending a maintainer decision rather than opening a new rung unilaterally. | | ||
| | 2026-07-31 | `v2.2.4 "Cartridge"` / `main` | RustyNES cut four releases since the last check (`v2.2.0`): `v2.2.1`, `v2.2.2 "Conduit"`, `v2.2.3 "Datum"`, `v2.2.4 "Cartridge"`. **CI supply-chain hardening** (`v2.2.2`): `persist-credentials: false` on all 19 checkouts, a fail-closed release-tag existence check replacing a three-way-collapsing `git ls-remote`, and a pinned toolchain action. Checked RustySNES's own state directly rather than assuming parity: 15 of its 17 checkouts were unhardened, `release-auto.yml:96` carried the identical `ls-remote` bug, and 34 action references were on mutable tags. **Libretro buildbot recipe** (`.gitlab-ci.yml`, `v2.2.2`) putting the core on RetroArch's in-app downloader; RustySNES has `rustysnes-libretro` but no buildbot recipe. **PPU fast-dot-path promoted to default + PGO** (`v2.2.3`) — NES-specific hot path, and RustySNES already shipped its own PGO/BOLT pipeline in `v1.19.0`. **`expansion_level_tripwire`** (`v2.2.3`) — a CI-run unit test pinning the audio level constants that seven `commercial-roms`-gated golden rows depend on, after those rows sat stale for 28 days. **Mapper residuals + 5B audio calibration** (`v2.2.3`) and **`.info` metadata + FDS disk control** (`v2.2.4`): NES/FDS-specific. **Antigravity reviewer template sync** (`v2.2.4`): RustySNES already did this in `v1.25.0` (#270/#273). | **CI hardening**: **small catch-up, ADOPTED IMMEDIATELY** in `v1.26.0` — all three items, with one divergence found by auditing per-site rather than copying the conclusion: `release-auto.yml`'s `prepare` job **genuinely needs** the checkout credential (it `git push`es the annotated tag), where the sibling's audit found no job did. A blanket sweep would have broken every future release. **Golden-staleness tripwire**: the *mechanism* does not port — RustySNES's only `commercial-roms`-gated suite (`commercial_screenshots`) is a screenshot generator that asserts nothing, so it holds no golden that can go stale, and PPU accuracy is not parameterised by a small constant set the way expansion-audio levels are. But checking for the analogue surfaced a **real and worse** structural gap, closed in `v1.26.0`: the 53 rendered-scene goldens and every framebuffer golden ran in **no pull-request job at all** (only `full-test`, which is main/tags/schedule), which is how this project's own coprocessor-golden staleness happened. **Libretro buildbot**: **medium catch-up**, no rung — candidate for a future distribution cut, flagged in `ROADMAP.md` rather than folded in. **Fast dot path / PGO / mapper / FDS / `.info`**: not applicable or already covered. | First re-run since the fuzzing item from the 2026-07-15 row was closed. Note the sibling is now *behind* on one axis: its `fuzz/` is wired into no workflow and its README claims a compile gate that does not exist there, where RustySNES's has both a per-PR compile gate and a scheduled campaign. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
workflow_dir=".github/workflows"
echo "Checkout and credential settings:"
rg -n -C 4 'actions/checkout@|persist-credentials:' "$workflow_dir"
echo "Release tag lookup:"
rg -n -C 8 'git ls-remote|refs/tags|release tag|tag exists' \
"$workflow_dir/release-auto.yml"
echo "All action references:"
rg -n '^[[:space:]]*uses:[[:space:]]*[^#[:space:]]+@' "$workflow_dir"Repository: doublegate/RustySNES
Length of output: 21200
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Current workflow counts:"
python3 - <<'PY'
from pathlib import Path
import re
checkout_total = 0
checkout_hardened = 0
action_total = 0
mutable = []
for path in sorted(Path(".github/workflows").glob("*.yml")):
lines = path.read_text().splitlines()
for n, line in enumerate(lines, 1):
m = re.search(r"uses:\s*([^#\s]+)@([^\s#]+)", line)
if not m:
continue
action_total += 1
owner_repo, ref = m.groups()
if not re.fullmatch(r"[0-9a-fA-F]{40}", ref):
mutable.append((path, n, owner_repo, ref))
if owner_repo == "actions/checkout":
checkout_total += 1
window = "\n".join(lines[n:min(n + 8, len(lines))])
if re.search(r"persist-credentials:\s*false", window):
checkout_hardened += 1
print(f"checkout_total={checkout_total}")
print(f"checkout_persist_credentials_false={checkout_hardened}")
print(f"action_total={action_total}")
print(f"mutable_action_refs={len(mutable)}")
for item in mutable:
print("%s:%d %s@%s" % item)
PY
echo "Relevant history:"
git log --all --oneline --decorate -n 20 -- to-dos/LOCKSTEP-CHECKLIST.md .github/workflowsRepository: doublegate/RustySNES
Length of output: 1984
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import re
import subprocess
commits = {
"before_81baef5": "81baef5^",
"after_81baef5": "81baef5",
}
def files_at(commit):
out = subprocess.check_output(
["git", "ls-tree", "-r", "--name-only", commit, ".github/workflows"],
text=True,
)
return [line for line in out.splitlines() if line.endswith((".yml", ".yaml"))]
def content_at(commit, path):
return subprocess.check_output(["git", "show", f"{commit}:{path}"], text=True)
for label, commit in commits.items():
checkout_total = checkout_hardened = action_total = 0
mutable = []
ls_remote = []
for path in files_at(commit):
lines = content_at(commit, path).splitlines()
for n, line in enumerate(lines, 1):
m = re.search(r"uses:\s*([^#\s]+)@([^\s#]+)", line)
if not m:
continue
action_total += 1
owner_repo, ref = m.groups()
if not re.fullmatch(r"[0-9a-fA-F]{40}", ref):
mutable.append((path, n, owner_repo, ref))
if owner_repo == "actions/checkout":
checkout_total += 1
window = "\n".join(lines[n:min(n + 8, len(lines))])
if re.search(r"persist-credentials:\s*false", window):
checkout_hardened += 1
for n, line in enumerate(lines, 1):
if "git ls-remote" in line:
ls_remote.append((path, n, line.strip()))
print(f"{label}:")
print(f" checkout_total={checkout_total}")
print(f" checkout_persist_credentials_false={checkout_hardened}")
print(f" checkout_unhardened={checkout_total - checkout_hardened}")
print(f" action_total={action_total}")
print(f" mutable_action_refs={len(mutable)}")
for item in mutable:
print(" ", item)
print(f" ls_remote={ls_remote}")
print("Hardening commit summary:")
print(subprocess.check_output(
["git", "show", "--stat", "--oneline", "--no-renames", "81baef5"],
text=True,
))
PYRepository: doublegate/RustySNES
Length of output: 3881
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import re
import subprocess
commits = {
"before_81baef5": "81baef5^",
"after_81baef5": "81baef5",
}
def files_at(commit):
out = subprocess.check_output(
["git", "ls-tree", "-r", "--name-only", commit, ".github/workflows"],
text=True,
)
return [p for p in out.splitlines() if p.endswith((".yml", ".yaml"))]
def text_at(commit, path):
return subprocess.check_output(["git", "show", f"{commit}:{path}"], text=True)
for label, commit in commits.items():
checkouts = hardened = actions = 0
mutable = []
ls_remote = []
for path in files_at(commit):
lines = text_at(commit, path).splitlines()
for n, line in enumerate(lines, 1):
match = re.search(r"uses:\s*([^#\s]+)@([^\s#]+)", line)
if not match:
continue
actions += 1
owner_repo, ref = match.groups()
if not re.fullmatch(r"[0-9a-fA-F]{40}", ref):
mutable.append((path, n, owner_repo, ref))
if owner_repo == "actions/checkout":
checkouts += 1
block = "\n".join(lines[n:min(n + 8, len(lines))])
if re.search(r"persist-credentials:\s*false", block):
hardened += 1
for n, line in enumerate(lines, 1):
if "git ls-remote" in line:
ls_remote.append((path, n, line.strip()))
print(label)
print(f" checkouts={checkouts}")
print(f" hardened={hardened}")
print(f" unhardened={checkouts - hardened}")
print(f" actions={actions}")
print(f" mutable={len(mutable)}")
print(f" ls_remote={ls_remote}")
for item in mutable:
print(f" mutable_ref={item}")
print("Hardening commit:")
print(subprocess.check_output(
["git", "show", "--stat", "--oneline", "--no-renames", "81baef5"],
text=True,
))
PYRepository: doublegate/RustySNES
Length of output: 4077
Correct the historical audit counts. Before v1.26.0, all 17 checkouts lacked persist-credentials: false, and all 32 action references used mutable tags. The release-auto.yml:96 bug and the v1.26.0 disposition are correct.
🧰 Tools
🪛 LanguageTool
[style] ~56-~56: This sentence may be long and difficult for your reader to follow. Consider inserting a period and starting a new sentence here.
Context: ...DIATELY** in v1.26.0 — all three items, with one divergence found by auditing per-site rather than copyin...
(WITH_THE_SENTENCE)
[grammar] ~56-~56: Ensure spelling is correct
Context: ...compile gate that does not exist there, where RustySNES's has both a per-PR compile gate and a sc...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_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 `@to-dos/LOCKSTEP-CHECKLIST.md` at line 56, Correct the historical CI hardening
counts in the checklist entry: state that all 17 checkouts lacked
persist-credentials: false before v1.26.0 and that all 32 action references used
mutable tags. Leave the release-auto.yml:96 finding and the v1.26.0 disposition
unchanged.
Source: Path instructions
…nine Review round two. Two applied, one declined. **`--no-fail-fast` on the golden-vector step.** Real, and verified by injection rather than taken on description: cargo's default stops after the first failing test BINARY, across binaries and not merely within one. Injecting a failure into `save_state_backward_compat` and running it alongside `save_state_determinism` showed the second suite never ran at all. With ten suites in this step that means one broken golden hides the status of the other nine — an author fixes one, pushes, and only then discovers the next, serialising a debug loop over a ~6-minute job. Fixed with `--no-fail-fast` rather than the suggested split into ten separate steps: one flag against ten cargo startups and ten log sections, and re-verified that the step still FAILS the job while reporting every suite. **`DEFAULT_RUN_AHEAD_THROTTLE_MS`.** The `14.0` is now a named public constant so the value, the doc comment justifying it, and the test that bounds it cannot drift apart; the test asserts against the constant instead of restating the literal. **Declined: "the CHANGELOG entry is too verbose, keep it focused on user-facing behaviour."** This project's CHANGELOG is not a summary — per `docs/adr/0007` the annotated tag body IS the CHANGELOG section verbatim, and `release-auto.yml` reuses it as the GitHub release notes with no separate hand-authored text. Entries are therefore written at release-note quality by design, and the measurements here are the whole justification for a decision (run-ahead staying opt-in) that would otherwise read as an unexplained non-change. Trimming to "user-facing behaviour" would leave the release notes asserting a conclusion with its evidence deleted. Verified: `cargo fmt --check`, frontend clippy `-D warnings`, `cargo doc` with warnings-as-errors, `config::tests` 13/13, ci.yml parses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review adjudication — round twoThree findings. Two applied, one declined. Applied: one failing suite hid the other nineReal, and worse in practice than described — verified by injection rather than taken on description. Cargo's default stops after the first failing test binary, and it stops across binaries, not merely within one: With ten suites in that step, one broken golden hides the status of the other nine: an author fixes one, pushes, waits ~6 minutes, and only then discovers the next. Fixed with The injected test was reverted from a snapshot; Applied: name the
|
… asserted Review round three. One applied, two declined with measurements — and the PAL suggestion is worth recording because I applied it first and it was wrong. **Applied: the absolute timings are host-specific.** The CHANGELOG now says so, and says why they are there anyway: the ratios are what decide run-ahead's default, and a reader can only check a ratio by re-running `save_state_cost` + `headless_frame` and comparing. The percentages are the portable part; the microseconds are the working. **Declined: add a matching `Region::Pal` assertion.** I applied this, then checked the arithmetic before pushing: NTSC's frame budget is 16.64 ms and PAL's is 20.00 ms, so PAL is the LOOSER bound by 3.36 ms and `throttle_ms < NTSC` strictly implies `throttle_ms < PAL`. The added assertion could never fail while the existing one passed — a vacuous assertion sitting beside a real one and reading like extra coverage. That is the exact shape `CLAUDE.md` names under "a guard must not subsume the assertion it protects", and the project already has four recorded instances of the vacuous-test trap. Reverted, with the arithmetic left in the test so the next reader does not re-derive it. (My first pass also wrote a comment claiming NTSC was the bound that would "silently carry a PAL-violating value through", which is backwards — a PAL-violating value exceeds the NTSC bound too and is caught. Fixed.) **Declined: merge the two harness `cargo test` steps.** Measured: a second cargo invocation against an already-built target dir costs 0.58 s — 0.16% of the ~6-minute step. Against that, merging would apply the battery step's `-- --nocapture` to all ten golden suites (flooding the log) and collapse two separately-attributed CI steps into one, so a failure no longer says at a glance whether the battery or the goldens broke. Verified: `cargo fmt --check`, frontend clippy `-D warnings`, `config::tests` 13/13. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review adjudication — round threeThree findings. One applied, two declined with measurements. The PAL one is worth reading — I applied it first, then found it was wrong. Declined: add a matching
|
| region | frame rate | budget |
|---|---|---|
| NTSC | 60.0988 Hz | 16.64 ms |
| PAL | 50.00698 Hz | 20.00 ms |
PAL is the looser bound, by 3.36 ms. So throttle_ms < NTSC strictly implies throttle_ms < PAL — the added assertion can never fail while the existing one passes. It would be a vacuous assertion sitting next to a real one and reading like extra coverage.
That's the exact shape CLAUDE.md names under "a guard must not subsume the assertion it protects", and this project already has four recorded instances of the vacuous-test trap. Reverted, with the arithmetic left in the test so the next reader doesn't re-derive it.
Worth admitting the near-miss: my first pass also wrote a comment claiming NTSC was the bound that would "silently carry a PAL-violating value through" — which is backwards. A PAL-violating value (>20 ms) exceeds the NTSC bound too and is already caught. Fixed.
The underlying observation was still useful: the constant's doc justifies itself against both regions, and now the test explains why only one of them is checkable.
Declined: merge the two harness cargo test steps
Measured — a second cargo invocation against an already-built target dir:
second invocation (no-op freshness check): 0.58 s
0.16% of the ~6-minute step. Against that saving, merging would:
- apply the battery step's
-- --nocaptureto all ten golden suites, flooding the log with output the battery specifically wants and the goldens don't; - collapse two separately-attributed CI steps into one, so a failure no longer says at a glance whether the battery or the goldens broke.
Applied: the timings are host-specific
Fair. The CHANGELOG now says so — and says why they're recorded anyway: the ratios are what decide run-ahead's default, and a reader can only check a ratio by re-running the same two benches (save_state_cost, headless_frame) and comparing. The percentages are the portable part; the microseconds are the working.
Verification
cargo fmt --all --check, frontend clippy -D warnings, config::tests 13/13.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 39-42: Update the benchmark explanation near the “Absolute
timings” text to remove the claim that percentages are portable. State that the
measurements support the decision only for the benchmark machine, and that both
save_state_cost and headless_frame must be rerun when evaluating another target.
🪄 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: 2b4808b4-2fc1-4d9b-8b0e-27aefbe02ae9
📒 Files selected for processing (3)
.github/workflows/ci.ymlCHANGELOG.mdcrates/rustysnes-frontend/src/config.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: accuracysnes
- GitHub Check: test-light
- GitHub Check: lint
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (11)
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: Flag third-party actions pinned to a branch rather than a tag or commit SHA, jobs without
a least-privilegepermissions:block, and secrets that could reach the log. The
ci-successjob is the required check and must depend on every gate it claims to
aggregate — a new job that is not in itsneeds:list is a gate nothing enforces.
Files:
.github/workflows/ci.yml
**/*.{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:
CHANGELOG.mdcrates/rustysnes-frontend/src/config.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
**/*
📄 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:
CHANGELOG.mdcrates/rustysnes-frontend/src/config.rs
**/*.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:
CHANGELOG.md
crates/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Busowns mutable machine state, and the CPU borrows&mut Bus.
Preserve determinism: seed, ROM, and input must produce bit-identical output.
Treat test ROMs as the behavioral specification; when documentation disagrees with passing ROM behavior, update the documentation.
Keepunsafeconfined to existing allowed areas, namely frontend and FFI code, and document everyunsafeblock with a// SAFETY:comment.
Files:
crates/rustysnes-frontend/src/config.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-frontend/src/config.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-frontend/src/config.rs
crates/rustysnes-frontend/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Frontend code is one of the permitted locations for
unsafe; any unsafe use still requires a// SAFETY:comment.
Files:
crates/rustysnes-frontend/src/config.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-frontend/src/config.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-frontend/src/config.rs
🔇 Additional comments (7)
.github/workflows/ci.yml (2)
359-366: LGTM!
367-379: 🔒 Security & PrivacyNo permissions change is required. The workflow-level
permissions: contents: readapplies toaccuracysnes, and the job does not override it.> Likely an incorrect or invalid review comment.CHANGELOG.md (2)
12-32: LGTM!
58-64: LGTM!crates/rustysnes-frontend/src/config.rs (3)
928-982: Pin the documented14.0ms value in both default assertions.Line [937] only checks that
RunAheadConfig::default()matches its own constant. Line [980] only checks that an omitted value is positive. A future positive default can therefore change the documented behavior without failing the tests. Assert the exact14.0value for both paths, using an exact comparison that remains Clippy-compliant.Proposed test adjustment
- assert!(cfg.throttle_ms == DEFAULT_RUN_AHEAD_THROTTLE_MS); + assert_eq!(cfg.throttle_ms.to_bits(), 14.0_f32.to_bits()); ... - assert!(cfg.run_ahead.throttle_ms > 0.0); + assert_eq!(cfg.run_ahead.throttle_ms.to_bits(), 14.0_f32.to_bits());
617-622: LGTM!
637-639: 🩺 Stability & AvailabilityNo change is required. The run-ahead branch requires
config.run_ahead.frames > 0, so the defaultthrottle_msdoes not affect pacing when run-ahead is disabled.> Likely an incorrect or invalid review comment.
| Absolute timings below are from one development machine and will differ on other hardware — they | ||
| are recorded because the **ratios** are what decide this, and a reader can only check a ratio by | ||
| re-running the same two benches (`save_state_cost`, `headless_frame`) and comparing. The | ||
| percentages are the portable part; the microseconds are the working: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not describe host-dependent percentages as portable.
2.4%, 79%, and 118% are calculated from host-specific benchmark times. A faster or slower host changes those percentages. State that the measurements support the decision for the benchmark machine, and require rerunning the benches for another target.
As per path instructions, Markdown prose must match the behavior and evidence it describes.
Suggested wording
- The percentages are the portable part; the microseconds are the working:
+ These percentages describe the benchmark machine; rerun the same benches on target hardware before changing the default:📝 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.
| Absolute timings below are from one development machine and will differ on other hardware — they | |
| are recorded because the **ratios** are what decide this, and a reader can only check a ratio by | |
| re-running the same two benches (`save_state_cost`, `headless_frame`) and comparing. The | |
| percentages are the portable part; the microseconds are the working: | |
| Absolute timings below are from one development machine and will differ on other hardware — they | |
| are recorded because the **ratios** are what decide this, and a reader can only check a ratio by | |
| re-running the same two benches (`save_state_cost`, `headless_frame`) and comparing. These | |
| percentages describe the benchmark machine; rerun the same benches on target hardware before changing the default: |
🤖 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` around lines 39 - 42, Update the benchmark explanation near the
“Absolute timings” text to remove the claim that percentages are portable. State
that the measurements support the decision only for the benchmark machine, and
that both save_state_cost and headless_frame must be rerun when evaluating
another target.
Source: Path instructions
Review round four, nitpick applied: the `assert!(x == y)` at the default check lacked the note its sibling assertion carries about why `assert_eq!` fails clippy's `float_cmp` under `-D warnings`. Two identical-looking lines where only one explains itself is how the next person "simplifies" the unexplained one and breaks the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review adjudication — round fourTwo findings. One applied, one partly applied. Applied: float-comparison comment parityRight, and the reason it matters is specific: two identical-looking Partly applied: commit scopeThe observation is fair — Not split into separate commits, for two reasons:
Where the guidance does bite is if these were going in as separate commits on Verification
|
Antigravity review (Gemini via Ultra)This PR adds golden vector and save-state test targets to the pull request CI workflow, sets Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
Review adjudication — round fiveThree findings, all declined with measurements. Two of them re-raise points already answered; one contradicts this reviewer's own round-two advice. Declined: consolidate the battery step into the golden stepRe-raised from round three, where it was declined at 0.58 s measured overhead (0.16% of a ~6-minute step) against losing per-step failure attribution and flooding ten suites with the battery's The new angle — that the two steps have "different The battery step runs exactly one target, so the flag would be a no-op there. There is no reporting inconsistency to reconcile. (Injected tests reverted; harness diff clean.) Declined: use
|
Third and final PR of the
v1.26.0hardening rung. Based onmain(both prior PRs merged).Two of this rung's three planned items turned out differently than planned once measured. Both are reported as found rather than forced to match the plan.
1. The golden vectors ran in no pull-request job
Only
lint,test-light, andaccuracysnesrun on a PR.test-lightnever passes--features test-roms, and theaccuracysnesjob scoped itself to--test accuracysnes. So the 53 rendered-scene goldens and every framebuffer golden were reached only byfull-test, which ismode == 'full'—main, tags, the weekly cron.A PR could shift PPU behaviour, move a golden, and hear nothing about it until merge-to-main. That's the same structure behind this project's own coprocessor-golden staleness (goldens left stale by post-bless PPU accuracy fixes): a golden vector nothing executes only accumulates drift. Here it wasn't that nothing ran them ever — it's that nothing ran them while it was still cheap to fix.
The
accuracysnesjob now also runs the scene, undisbeliever, rainwarrior, coprocessor, and save-state suites. Cost is bounded and measured: ~5 min for the three with a committed corpus (scenes ~84s, undisbeliever ~158s, rainwarrior ~54s). The*_oncartsuites need gitignored dumps and self-skip for free — listed anyway so the intent is explicit rather than inferred from an omission, and so they start gating the moment a runner ever has that corpus.On the planned "tripwire": it was scoped as a port of the sibling's
expansion_level_tripwire. That mechanism does not port, and saying so is worth more than inventing a stand-in — the onlycommercial-roms-gated suite here (commercial_screenshots) is a screenshot generator that asserts nothing, so it holds no golden that can go stale, and PPU accuracy isn't parameterised by a small constant set the way expansion-audio levels are. Looking for the analogue is what surfaced the larger real gap above.2. Run-ahead stays opt-in — measured, not assumed
docs/frontend.mdrecorded the per-frame save-state allocation as the blocker on run-ahead going default-on.v1.25.0removed that allocation, so the question was re-measured rather than assumed resolved:save_stateload_stateheadless_frame_steady_state)The round trip is 2.4% of the budget. It was never the dominant cost, and removing the allocation — a real improvement — did not move the decision. What run-ahead costs is the extra frame of emulation, inherent to the technique:
frames = 1→ 13.18 ms, 79% of budget, ~3.5 ms left for present/UI/audio on a fast dev machineframes = 2→ 19.57 ms, 118% — cannot hold 60 fps at allThe plan for this rung said "default-on only if the measurement supports it; report the number either way." It doesn't, and this is the number.
One thing did change as a result.
RunAheadConfig'sDefaultis now hand-written sothrottle_msdefaults to 14 ms (armed) rather than the derived0.0(disabled). The throttle is what its own doc calls the thing keeping run-ahead from turning a latency win into stutter — and the derived default left it off for precisely the user who'd just enabled run-ahead from Settings and had nothrottle_msline in their config. An existing config that spells the field out keeps its value (#[serde(default)]fills only absent fields), pinned by a negative-control test alongside the positive one.3. Lockstep
Log row for RustyNES
v2.2.1–v2.2.4, and the two 2026-07-12 flags corrected: the GIF/WAV capture subsystem and CRT shader-preset depth were both closed byv1.25.0(av_record.rs; the multi-pass stack + richer CRT + NTSC pass +.slangpimport with the naga bridge), yet the roadmap still asked for a maintainer go/no-go on them two weeks later.Also carries the verification comment for #276's declined blocking finding — documentation of that adjudication, not a fix.
Verification
cargo fmt --all --check, frontend clippy-D warnings— green.config::tests), including the negative control.--testinvocation the new CI step uses.🤖 Generated with Claude Code
Summary
RunAheadConfig::default()to usethrottle_ms: 14.0withframes: 0. Explicitthrottle_msvalues remain unchanged.RunAheadConfig::default(): the throttle is armed at 14 ms, but run-ahead remains disabled until frames are configured.#276and documents GitHub API behavior used by release automation.