fix(probe): carry provenance across the three restores that were clearing it - #405
Merged
Conversation
…ring it
Running the Latency Oracle or the RAM Atlas emptied both the Pixel Provenance
and the Audio Provenance panels. `Nes::restore_inner` clears both stores, which
is right for a genuine timeline change and wrong for a restore that puts back
the state the user is still looking at. v2.3.6 added
`Nes::take_provenance` / `put_provenance` for exactly that exception and wired
it into `RunAhead::finish`, the one call site the bug report had named.
`rustynes-probe` has three more same-timeline restores and none of them used it:
* `Probe::run_uncounted` restores the anchor once per trial, and
`latency::measure_in_place` runs up to 21 trials against the LIVE emulator.
* `latency::measure_in_place` restores the caller's state on the way out, a
restore that sits OUTSIDE every per-trial guard, so fixing the trial path
alone leaves it broken.
* The RAM Atlas panel's `TimelineGuard` restores after each observation.
Both stores are cumulative. "Which instruction last wrote this" can point
thousands of frames back — a palette byte written at level load, a `$4008`
linear-counter reload written once during init — so the records were not
rebuilt by the next frame. They were gone for the session.
Closed by moving the stash into `rustynes_probe::TrialGuard`, renamed from
`CaptureGuard` because it no longer carries only the rewind flag. The two are
the same category of state and belong behind one guard: both live outside the
save state, so neither is carried by a snapshot round trip, and both must
therefore be carried deliberately. The guard is now entered BEFORE the anchor
restore rather than after it, since that restore is one of the things the
stores need protecting from. Restoring in `Drop` keeps the unwind path covered,
and unlike the snapshot restore declined for a `Drop` guard on PR #385, putting
a stash back is infallible, so nothing fallible is being hidden.
`rustynes-probe` gains a `debug-hooks` passthrough feature. Without it the new
code would compile out in precisely the builds that need it: cargo's feature
unification already switches on `rustynes-core/debug-hooks` for a frontend
build, so the clearing is live, while a `cfg` on a feature the crate does not
declare is never true.
Four tests, four independent mutations, one per store and one per site:
removing either put-back, entering the guard after the restore, or unguarding
`measure_in_place` each fails exactly the test that pins it and no other. The
per-site pair is deliberate — a fix covering only `run_uncounted` passes the
trial test and fails the measurement one.
The existing `measure_in_place_restores_the_live_timeline` could not have
caught this. It compares `nes.snapshot()` before and after, and provenance is
deliberately not in the snapshot, so the assertion was true throughout while
the state its own name claims to cover was being destroyed. A test fixture note
for whoever writes the next one: the first `run_frame` after `Nes::from_rom`
returns on an already-pending frame-complete flag without executing an
instruction, so a one-frame fixture leaves the PC at the reset vector and the
attribution table empty.
`docs/pixel-provenance.md` described run-ahead as "the one caller that needs
the exception". The rule stated there is right; the enumeration under it was
not. Both that sentence and the v2.3.6 retraction it already carried are kept
in place rather than rewritten, because the pair is the evidence.
No emulation-core file is touched — the change is confined to `rustynes-probe`
and the frontend panel — so AccuracyCoin 141/141 and nestest 0-diff hold by
construction rather than by assertion.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a provenance-loss defect where probe-driven same-timeline snapshot restores (Latency Oracle and RAM Atlas) were clearing the Pixel Provenance and Audio Provenance stores. It does this by carrying provenance across those restores (similar to the existing run-ahead exception) and by ensuring the probe crate can actually cfg on debug-hooks in the builds where provenance is active.
Changes:
- Move provenance preservation into
rustynes_probe::TrialGuardand use it for probe trials, in-place latency measurement, and RAM Atlas observations. - Add a
rustynes-probedebug-hookspassthrough feature and forward it from the frontend’sdebug-hooksfeature. - Update provenance documentation and the Unreleased changelog to reflect the corrected “same-timeline restore” enumeration.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/pixel-provenance.md | Documents additional same-timeline restore call sites beyond run-ahead and the corrected exception scope. |
| docs/audio-provenance.md | Adds a dedicated section explaining the probe-engine provenance-clearing trap and the fix. |
| crates/rustynes-probe/src/lib.rs | Introduces TrialGuard (renamed from CaptureGuard) to preserve both rewind-capture state and provenance across restores; adds targeted tests. |
| crates/rustynes-probe/src/latency.rs | Wraps measure_in_place in TrialGuard so the final restore also preserves provenance. |
| crates/rustynes-probe/src/atlas.rs | Uses TrialGuard for atlas observations so rewound frames can’t clear/pollute provenance. |
| crates/rustynes-probe/Cargo.toml | Adds debug-hooks feature passthrough to allow intra-crate cfg(feature = \"debug-hooks\"). |
| crates/rustynes-frontend/src/debugger/atlas_panel.rs | Extends the panel’s TimelineGuard to carry provenance across its restore of the live timeline. |
| crates/rustynes-frontend/Cargo.toml | Forwards frontend debug-hooks into rustynes-probe/debug-hooks. |
| CHANGELOG.md | Adds an Unreleased “Fixed” entry describing the provenance-preservation fix and its rationale/tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…issing Both review findings on the PR were correct, and one of them was not a wording problem. `CHANGELOG.md` claimed the fix was "pinned by four tests under four independent mutations — one per store and one per site". There were three tests. The missing one is `measure_in_place` against the PIXEL store, and it is a real gap rather than an off-by-one in prose: the `measure_in_place` mutation fails only the AUDIO test, so a final restore that put back the audio stash and dropped the pixel one would have passed every test on the branch. The claim was written before the fourth test existed, which is the shape of error this project keeps meeting from the other direction — prose asserting a property the tree does not have. Adding the test rather than softening the sentence, because the sentence described the right contract. Verified load-bearing: removing the pixel put-back now fails both pixel tests instead of one. Also corrected, from the same review: the `TrialGuard::provenance` doc said the field is "`None` in a build without the feature". It is not `None`, it is `cfg`d out entirely — a distinction that matters to whoever edits this next, since "present and empty" invites a runtime check where there is no field at all. The rewrite also states the consequence the old wording obscured: without `debug-hooks` there is no per-trial cost of any kind, not merely a cheap one. And the `mem::take` rationale is mirrored onto the frontend's `TimelineGuard`, which uses the identical pattern for the identical reason and had the explanation only at the other site. The CHANGELOG now describes the full 2x2 rather than a count, and records that the fourth cell was missing until review caught it. A matrix is checkable; a number is not. Verified: fmt, `rustynes-probe` clippy and tests in both feature configurations (50 tests with the feature on), the frontend under `debug-hooks`, both wasm32 invocations, and rustdoc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
Response to the Antigravity pass — both items accepted, fixed in
|
Antigravity review (Gemini via Ultra)This PR fixes a defect where same-timeline state restores permanently cleared provenance data by wrapping the latency oracle and RAM atlas operations in guards that extract and preserve the provenance stashes. Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
Owner
Author
Accepted, and it is the exact failure this PR is aboutThe pixel assertions check Disposition: merging this as-is and strengthening the assertions on the stacked branch, not dropping it. The reasoning, so it is a decision rather than a convenience:
What lands there: both pixel tests capture a specific pixel's Noting that this reviewer's run on #404 could not read its patch at all, and both runs here worked, so the telemetry-hook fault appears cleared. |
doublegate
added a commit
that referenced
this pull request
Aug 19, 2026
Delivers the strengthening committed to on #405, where review pointed out an asymmetry that was not defensible in that change: the audio preservation tests compare an exact `RegWrite` — same PC, same cycle — while the pixel ones only asserted `pixel_provenance().is_some()`. That checks the stash came back, not that its contents did, so a regression emptying the store while leaving it armed would have passed the pixel tests and failed the audio ones. Both pixel tests now capture a specific pixel's `PixelProvenance` before the trial and assert it comes back identical, via a shared `armed_with_a_pixel_record` helper that returns the record rather than a flag. The probe pixel is mid-screen (128, 120) on purpose: near an edge, a timing difference could legitimately change what the PPU emitted, which would make the assertion flaky for a reason unrelated to what it tests. Demonstrated rather than argued. Mutating `Ppu::put_provenance` to return an ARMED but EMPTIED frame — precisely the regression the review described — now fails both tests. Under the old assertions it would have passed both. This is the same defect class the branch it sits on is about, one level further in: `measure_in_place_restores_the_live_timeline` asserted snapshot equality while provenance is not in the snapshot, and these asserted armed-ness while the contents are not armed-ness. A weaker assertion does not merely test less; it reports a pass for the failure it was written to catch. Verified: fmt, `rustynes-probe` clippy and tests in both feature configurations (57 off, 70 on), workspace clippy, and rustdoc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
doublegate
added a commit
that referenced
this pull request
Aug 19, 2026
Delivers the strengthening committed to on #405, where review pointed out an asymmetry that was not defensible in that change: the audio preservation tests compare an exact `RegWrite` — same PC, same cycle — while the pixel ones only asserted `pixel_provenance().is_some()`. That checks the stash came back, not that its contents did, so a regression emptying the store while leaving it armed would have passed the pixel tests and failed the audio ones. Both pixel tests now capture a specific pixel's `PixelProvenance` before the trial and assert it comes back identical, via a shared `armed_with_a_pixel_record` helper that returns the record rather than a flag. The probe pixel is mid-screen (128, 120) on purpose: near an edge, a timing difference could legitimately change what the PPU emitted, which would make the assertion flaky for a reason unrelated to what it tests. Demonstrated rather than argued. Mutating `Ppu::put_provenance` to return an ARMED but EMPTIED frame — precisely the regression the review described — now fails both tests. Under the old assertions it would have passed both. This is the same defect class the branch it sits on is about, one level further in: `measure_in_place_restores_the_live_timeline` asserted snapshot equality while provenance is not in the snapshot, and these asserted armed-ness while the contents are not armed-ness. A weaker assertion does not merely test less; it reports a pass for the failure it was written to catch. Verified: fmt, `rustynes-probe` clippy and tests in both feature configurations (57 off, 70 on), workspace clippy, and rustdoc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
doublegate
added a commit
that referenced
this pull request
Aug 19, 2026
* feat(probe): the Divergence Lens — which pixels differ, not just which frame v2.3.8 "Parallax" work item A. `Probe` already answered whether two configurations of the same ROM diverge and at which frame; a trial reduces each frame to one `u64` and `first_divergence` scans the two sample vectors. That reduction is the right shape for detecting a difference and the wrong shape for explaining one — a hash says frame 412 differs and cannot say which pixel, so it has nothing to hand to Pixel Provenance, which is where an answer actually lives. `divergence::localise` closes exactly that gap: detect the frame, re-run both configurations to it, keep the full output instead of its hash, and report the shape of the difference — population count, first pixel in raster order, and the inclusive bounding box. The count and the box are what separate kinds of bug from each other: one pixel is a sprite or a palette entry, 256 in a row is a scanline, tens of thousands is a scroll or a mode change. `is_single_scanline` is offered rather than left to call sites because it is the distinction a caller acts on and the inclusive comparison is easy to get wrong. It localises on the INDEX framebuffer, 256x240 u16s of `(emphasis << 6) | colour` — the PPU's own per-pixel output, before the palette lookup that produces RGBA. Half the bytes and at least as sensitive, because the RGBA buffer is a pure function of this one given the same palette. That proviso holds here because both trials run on the same instance and therefore share whatever palette is loaded; it is written down rather than assumed, because a future two-instance lens would have to revisit it. Three answers, and the third is the point: `Identical`, `Differs`, and `Inconclusive` for an exhausted budget or two trials that cannot be compared. The Latency Oracle's precedent applies directly — `None` and `Some(0)` were never collapsed there, and "I stopped looking" must not arrive wearing the same shape as "they agree". The budget is checked UP FRONT for all four trials, mirroring `atlas::verify_liveness`: spending two on detection and then finding the localisation pair unaffordable would consume the budget that would have answered the question. Detection also disagreeing with the pixel diff returns `Inconclusive` rather than picking a winner. No new engine primitive was needed. A trial restores the anchor on the way IN and not on the way out, so the emulator is left holding the trial's final frame and the Lens can read it off `nes` directly. That assumption is now pinned by `a_trial_leaves_the_emulator_at_its_end_state`, and the fixture is the load- bearing part: the obvious `synth_nrom` renders a blank screen, where "left at the anchor" and "left at the end" are byte-identical, so the test passes under both behaviours and proves nothing. It uses a ROM that drives PPUMASK emphasis from work RAM, and asserts the screen varies at all before asserting anything about the engine. Seven mutations, each caught by the test that pins it, and one of them changed the tests rather than confirming them: replacing the baseline frame with all zeros left the "a perturbation is located" test passing, because "count > 0, on-screen, inside its own bounding box" is true of a comparison against garbage. That is the assertion strength this project keeps getting caught by, so the test was replaced with one that checks the Lens's whole reported record against an independent replay through `Probe`'s own primitives. Both diff branches are covered separately — a Lens that reported a divergence unconditionally would pass the positive test and fail the control. `SCREEN_WIDTH` and `SCREEN_HEIGHT` move to `rustynes_ppu` ungated and `provenance::SCREEN_W` / `SCREEN_H` become aliases of them. They were a second copy of the same two numbers inside a `debug-hooks`-gated module, which made the width unreachable from ungated code and would have invited a third copy here rather than a dependency. `FRAMEBUFFER_LEN` and `FRAMEBUFFER_PIXELS` are now derived from them instead of repeating the literals. That touches `rustynes-ppu`, so the accuracy contract is VERIFIED rather than true by construction: AccuracyCoin 141/141 (100.00%, RAM decoder — the authoritative one; the framebuffer decoder reports 120 and is known-buggy) and `nestest_pc_c000_matches_golden_log` green, alongside fmt, workspace clippy, both `rustynes-probe` feature configurations, both wasm32 invocations, rustdoc, no_std thumbv7em, and the PPU crate's own 95 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(probe): trial-scoped provenance capture, the prerequisite item B needed v2.3.8 "Parallax" work item B, first step. The plan offered two mechanisms for narrowing a divergence below frame granularity and required the cheap one to be proved viable before either was written: read the answer out of the per-cycle records that Audio and Pixel Provenance already keep, rather than bisecting for it over ~17 partial re-runs. Establishing that produced a different answer than either branch expected. The anticipated failure was that two trials' records would not be comparable. The real one is that there are no records to compare, because a trial produces none. `Probe::run_uncounted` restores the anchor through `restore_quiet`, and `Nes::restore_inner` clears both stores; before v2.3.7 a trial's records were therefore destroyed at the start of the next trial, and after it `TrialGuard` holds both stores aside for the trial's duration, leaving the emulator unarmed while it runs. That is deliberate and correct — re-simulated frames never happened on the user's timeline and must not contribute attributions to it. So the mechanism had to be built rather than reused, and v2.3.7's fix is exactly what makes it safe: because the caller's stores are already held aside, a trial can arm FRESH ones with no path by which its re-simulated records could reach the user's. `Probe::run_capturing` arms them after the anchor restore that would otherwise have cleared them, runs the trial, and harvests them before the guard drops and puts the caller's back. The ordering is the whole design; getting it wrong in either direction either loses the capture or leaks it. Capture is per-trial rather than per-`Probe`, and that is a cost decision, not a style one: a per-CPU-cycle mix trace is roughly 29,780 records a frame, so making it a probe-level property would bill the Latency Oracle's 21 trials — around 625k records — for a feature it never reads. `TrialProvenance::Off` is the default and what every existing caller passes. Three mutations, each caught. The one that matters is the leak direction: the caller's store must come back BYTE-IDENTICAL, asserted as the same `RegWrite` from the same PC at the same cycle, not merely armed and not merely non-empty. A store the trial had refilled with its own frames satisfies both weaker checks while reporting instructions that never executed, which is precisely the failure the separation exists to prevent — and precisely the assertion strength this project has been caught by before. The capture test needed its own fixture, and the reason is worth recording: `reg_writing_nrom` writes `$4000` once at reset and then spins, so an anchor taken after that write is followed by frames touching no register at all. A capture test built on it would have asserted "the trial recorded something" against a trial that correctly recorded nothing — an assertion about the fixture wearing the shape of an assertion about the engine. `reg_looping_nrom` writes in a loop instead. Bisection is not dead; it is now the fallback rather than the alternative, and it remains the only option in a build without `debug-hooks`, where capture cannot exist at all. Whether it is needed is still a measurement to make, on the same terms the plan set out. Verified: fmt, workspace clippy, both `rustynes-probe` feature configurations for clippy and tests (56 off, 61 on), and workspace rustdoc. No emulation-core file is touched by this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(probe): the audio Lens — a divergence resolved to the CPU cycle v2.3.8 "Parallax" work item C, unblocked by the trial-scoped capture the previous commit added. `Observable::AudioEnergy` detects that a frame sounds different and can say nothing more: it is a quantised sum of |amplitude| over a whole frame, deliberately coarse because exact float equality across a resampled stream compares noise rather than signal. `divergence::localise_audio` uses the coarse instrument to find the frame and an exact one to find the cycle. It detects under `AudioEnergy`, then re-runs both configurations to the diverging frame CAPTURING their provenance, and compares the two per-CPU-cycle mix traces record by record. The answer is an absolute CPU cycle plus both `MixRecord`s at it — mixed sample, expansion contribution, and all five channels' raw pre-mix outputs. That is finer than the pixel Lens, and it arrives without bisection. Work item B wanted sub-frame resolution and framed it as a choice between ~17 partial re-runs and reading records that already exist; for audio the records genuinely do exist once a trial is asked to keep them, so the cost is two capturing trials rather than seventeen bisecting ones. Splitting detection from localisation across two different instruments has a price, and it is paid explicitly: they can disagree. A frame whose energy differs but whose records do not — or the reverse — returns `Inconclusive` rather than a guess, on the same reasoning the pixel Lens uses. Two measurements disagreeing is a fact about the run, not an answer about the ROM. Two guards on the preconditions for an index-wise comparison are NOT covered by tests, and the comment beside them says so rather than implying otherwise. Neither trace misalignment nor truncation is reachable under the current design — the trace is re-anchored per frame so two trials from one anchor share a `first_cycle`, and `MIX_CAP` is 36,864 against Dendy's worst-case 35,464-cycle frame. That was verified by mutation, not assumed: deleting either guard changes no observable behaviour. They stay because both preconditions are properties of code elsewhere, and a change there should surface as `Inconclusive` rather than as a confidently wrong cycle. The two guards that ARE load-bearing were mutation-checked and each fails exactly its own test. `AudioProvenanceStash` gains read-only `mix_trace` and `register_attribution` accessors. A captured stash is deliberately detached from any emulator, so reading it by putting it back into a scratch `Nes` would undo the separation that makes capture safe in the first place. That touches `rustynes-apu`, so the accuracy contract is VERIFIED rather than assumed: AccuracyCoin 141/141 (100.00%, RAM decoder) and nestest green, plus fmt, workspace clippy, both `rustynes-probe` configurations (56 tests off, 64 on), `rustynes-apu` in both its configurations (151 and 161), both wasm32 invocations, rustdoc, and no_std thumbv7em. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(lens): the Divergence Lens spec, including what it cannot answer Docs-as-spec for the three mechanisms landed so far in v2.3.8 "Parallax": the pixel Lens, trial-scoped provenance capture, and the audio Lens. Added to the MkDocs handbook alongside the other v2.3.x tool pages. Written to record the reasoning that is not recoverable from the code: - Why `Inconclusive` exists and is not a synonym for `Identical`, with the three conditions that produce it — including the one where the coarse detector and the fine localiser disagree, which returns neither answer because two measurements disagreeing is a fact about the run rather than about the ROM. - Why the budget is checked before the first trial rather than discovered halfway, which is the difference between declining and consuming the budget that would have answered the question on a second attempt. - Why localisation reads the INDEX framebuffer, and the one precondition that argument rests on — a shared palette, which holds for two trials on one instance and would not hold for a future two-instance lens. - Why no new engine primitive was needed, and why the test pinning that fact needs a fixture whose screen actually varies: on a blank screen "left at the anchor" and "left at the end" are byte-identical, so the obvious fixture makes the test unfalsifiable. - That a probe trial produces no provenance at all, which is the finding that reshaped work item B, and the strict three-step ordering that makes capture a capture rather than a leak. It also states, in the page rather than only in a commit message, that two guards in the audio path are unreachable under the current design and covered by no test — established by mutation, not assumed. An untested guard described as though it were tested is the exact failure this project keeps meeting: prose asserting an intent is what stopped anyone checking Pixel Provenance against its own code for four releases, and that page already carries two retractions saying so. The "what it deliberately does not do" section is load-bearing rather than decorative. Sub-frame localisation of a PIXEL divergence is still not implemented, cycle bisection remains the fallback and the only option without `debug-hooks`, and whether it is worth building is a measurement still to make. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(frontend): the Divergence Lens panel, with its verdicts under test v2.3.8 "Parallax" work item D. `Tools -> Divergence Lens`, `debug-hooks`-gated and output-only, sitting beside the RAM Atlas because it answers the follow-up question the Atlas raises: the Atlas says a work-RAM address is live, this says what it actually changes — down to a pixel set or a CPU cycle. A work-RAM byte is the perturbation for two reasons. It is the one the Atlas already hands the user, and it is exactly reversible: the trial engine restores the anchor, so nothing about the measurement survives it. Gated on the SAME locked-session predicate as the RAM Atlas, and for identical reasons rather than by analogy. This panel advances the live `Nes` for four trials and pokes work RAM to define the perturbed configuration. Under netplay or a TAS the replayed frames reach peers before the anchor is restored, and under RetroAchievements hardcore the poke is precisely the write that mode exists to forbid. Checking only `nes.is_some()` would bypass a gate that exists for this; that exact mistake was caught in review on PR #392 for the Atlas. Registered with `DebuggerOverlay::clear_rom_bound_analysis` rather than given a fourth bespoke clear. A located pixel set names one game's frame, and the seam where panel state outlives the `Nes` it describes has now caught Pixel Provenance, the Latency Oracle, and the RAM Atlas. The panel is TESTED, which no sibling panel is, and the reason that is possible is a deliberate restructuring rather than more effort. `DebuggerOverlay::new` needs a `wgpu::Device` and a window, so the hook itself cannot be unit-tested — but the plan's actual requirement can be, once the verdict wording is lifted out of the `egui` closure into `video_text` and `audio_text`. The requirement is that "they agree", "I stopped looking", and "here is where" render as three visibly different things, and it is now asserted rather than eyeballed: the identical and inconclusive branches are the pair a careless edit collapses, and an inconclusive verdict must never be worded as an absence of divergence, because that sentence is what the user acts on. Four tests, three mutations, each caught by the test that pins it — including the one that matters most, rewording the inconclusive branch as the identical one, which is the drift the whole `None`-versus-`Some(0)` discipline exists to catch. `clear` is pinned against a panel with every field populated, so a future field that `clear` forgets fails rather than silently outliving its ROM. Verified: fmt, workspace clippy, `debug-hooks` and `full` frontend combos, BOTH wasm32 invocations, rustdoc, and the frontend suite at 516 tests. No emulation- core file is touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(probe): the Divergence Lens left the emulator thirty frames ahead Self-caught while reviewing the new panel against `latency::measure_in_place`'s contract, before either reached a reviewer. `localise` ran its four trials and returned. A trial restores the anchor on the way IN and not on the way out — which is the property the pixel path deliberately relies on to read the diverging frame off `nes` — so the result was correct and the live emulator was left wherever the last trial ended. Through the panel that meant asking "what does this byte change?" silently advanced the user's game by thirty frames, which is a worse bug than any the tool was asked about, and the same one `measure_in_place` carries three paragraphs of comment about avoiding. Both entry points now snapshot on the way in and restore on the way out, through one `in_place` wrapper rather than a restore after each early return. `localise` has six paths out; "every one of them restores" is worth having as a property of the shape instead of a property of inspection. The restore is wrapped in `TrialGuard`, and that half is not incidental. `Nes::restore_inner` clears both provenance stores, and this restore is precisely the same-timeline case that exception exists for — so an unguarded snapshot and restore would put the timeline back and empty the Pixel Provenance and Audio Provenance panels. That is the v2.3.7 defect reintroduced one layer up, by the very commit that fixed it two layers down. Three tests, and the split between them is the point. The timeline assertion compares `nes.snapshot()` across the call and is what caught the original bug. It is also structurally incapable of catching the provenance half, because provenance is deliberately NOT in the snapshot — the same blind spot that let `measure_in_place_restores_the_live_timeline` pass for a release while the state its name claims to cover was being destroyed. So the provenance contract gets its own test and its own mutation, and the audio entry point gets its own timeline test rather than trusting that a shared wrapper covers it, since a fix applied to one call site of a shared path is this project's most-repeated defect. Two mutations, each caught by the test that pins it: deleting the restore fails both timeline tests, and restoring without the guard fails only the provenance one. A third mutation was written, run, and discarded as worthless — moving the guard construction after the body still left the restore wrapped, so it proved nothing, which is the difference between running a mutation and checking one. `docs/divergence-lens.md` records the contract and that it was wrong first. Verified: fmt, workspace clippy, both `rustynes-probe` configurations (57 tests off, 67 on), the frontend under `debug-hooks`, both wasm32 invocations, and rustdoc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(probe): explain a pixel divergence, and close item B without bisection Work item B asked for sub-frame localisation and framed it as a choice: bisect the frame over ~17 partial re-runs, or read per-cycle records that already exist. It required the cheap option to be proved viable before either was written. Proving it produced a third answer. `localise_explained` returns the diverging pixel's `PixelProvenance` from BOTH configurations, so the question it answers is *which causal input differs* — the winning layer, the pattern row, the nametable and attribute addresses, the palette entry, the emphasis mask — rather than *at which cycle the two runs parted*. That is better than bisection rather than cheaper. A cycle index says when; a differing `pattern_addr` says the two runs fetched different tile data, which is a lead someone can follow. Bisection would have spent ~17 extra trials to produce the weaker answer. **So bisection is not implemented, and that is a decision rather than an omission.** It remains the only route without `debug-hooks`, where capture cannot exist, and it is the mechanism to reach for if a case appears whose causal inputs match but whose timing differs. `docs/divergence-lens.md` records why it was not needed first, so the next person does not rediscover the question. `ProvenanceStash` gains a read-only `pixel_frame` accessor, mirroring the audio stash's. A captured stash is deliberately detached from any emulator, so reading it by putting it back into a scratch `Nes` would undo the separation that makes capture safe. Four mutations, and two of them changed the work rather than confirming it. Reading the baseline record from the VARIANT trial failed nothing: asserting "a cause exists, describing the right pixel" is satisfied by a perfectly well-formed cause whose two records are identical. Fixing that required retracting a doc claim first. `differing_fields` was documented as possibly empty at a located pixel, on the reasoning that a colour could differ through emphasis rather than through the causal chain. That is wrong: an index-framebuffer entry is `(emphasis << 6) | colour`, so a differing entry means `color` differs or the emphasis bits do, and those are carried in `color_mask`. There is no third way. The distinction is load-bearing. Under the permissive reading an empty result is a legitimate outcome to render; under the correct one it means the records did not come from the two configurations, which is a defect. With the assertion tightened to match, both trial-swap mutations fail. The explained path is also asserted to AGREE with the plain one on the divergence itself, because two functions that localise the same thing differently is worse than either being wrong alone — and it re-runs the same sequence with capturing trials, which is exactly where a copy drifts. Verified: fmt, workspace clippy, `rustynes-probe` and `rustynes-ppu` clippy with the feature on, probe tests in both configurations (57 off, 70 on), the PPU crate's 95, rustdoc, no_std thumbv7em, both wasm32 invocations, and — since `rustynes-ppu` is touched — AccuracyCoin **141/141 (100.00%, RAM decoder)** and nestest 0-diff, verified rather than assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(frontend): show WHY the located pixel differs, not just which one Completes the user-facing loop the Divergence Lens was built for. The panel already answered "which pixels differ"; it now answers "and here is why" in the same four trials, because `localise_explained` has the two localisation runs capture their own provenance rather than running unarmed. No extra trials, no second button. What it renders is the causal chain the two configurations disagree about — the pattern row, the nametable and palette addresses, the winning layer — with the pattern difference annotated in the terms a reader acts on ("different tile data fetched") rather than as two bare hex addresses. `cause` is a separate field from `video` rather than folded into it, and the reason is the distinction this crate keeps insisting on: `None` here means "no provenance record for that pixel", NOT "no difference". Inside one optional the two would be indistinguishable, which is precisely the collapse the Latency Oracle's `None`-versus-`Some(0)` split exists to prevent. Both cases render, and the missing-record case says so — a silently omitted section reads as an answer. There is a third case that should be unreachable, and it is rendered rather than hidden. An index-framebuffer entry is `(emphasis << 6) | colour`, so a located pixel must differ in `color` or `color_mask`; an empty field list would mean the two records did not come from the two configurations. If that ever appears the panel says so and asks for a report, because a blank section would make a real defect invisible — the exact way Pixel Provenance stayed broken for four releases. The ROM-bound clear test now populates `cause` too, and is mutation-checked against a `clear` that resets every OTHER field: a pixel cause names tile and palette addresses from one game's frame, so leaving it standing across a ROM change is the seam that has already caught four panels. Verified: fmt, workspace clippy, `debug-hooks` and `full` frontend combos, both wasm32 invocations, rustdoc, and the frontend suite at 516. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(plans): add the v2.3.8 "Parallax" plan, and correct its premise v2.3.8's marquee is the Divergence Lens: two `Nes` instances from one anchor under differing configuration, the first divergence localised to a pixel or a sample, and that pixel handed to Pixel Provenance (v2.3.2) and that sample to Audio Provenance (v2.3.7). The plan is written against the substrate as it ACTUALLY is, which differs from the v2.3.7 carry-forward note in one load-bearing way. That note says divergence is "located by the v2.3.6 probe engine's bisection". There is no bisection in `rustynes-probe`, and frame-granular divergence needs none: `Probe::run` materialises one `u64` per frame, so `first_divergence` is a linear scan over an array that already exists. Recorded explicitly, because "we already have bisection" would have made the sub-frame work item look free when it is the one genuinely open design question in the release. The audit also found the substrate is FURTHER along than credited. Two configurations from one anchor already work (`run` + `run_perturbed`), and `run_perturbed`'s doc already argues the attribution licence: `setup` runs after restore and before any frame, so whatever it changes is the only difference. `Probe::agree` already distinguishes "they agree" from "nothing ran". So the real gap is neither detection nor comparison. `Observable` reduces a whole frame to one `u64` — the right shape for detecting divergence and the wrong shape for localising it. The release is "narrow the frame the detector already found, then hand off". Item B is left as an explicit measure-first choice between cycle bisection (~17 partial re-runs) and reading the per-cycle records both provenance features already keep when armed. The cheaper option's failure mode is that the records may not be comparable across instances; establishing that comes before writing either. Per the v2.3.1 precedent — ten candidates measured, all ten rejected — the measurement is the value. The verification bar requires a known-IDENTICAL fixture alongside the known-divergent one, because a Lens that always reports divergence passes a suite that only ever feeds it differing pairs. * docs(plans): settle v2.3.8 item B — a probe trial records no provenance The plan told item B to establish, before writing either mechanism, whether the Divergence Lens could read the divergence out of records that already exist rather than bisecting for it. Establishing it produced a different answer than either branch anticipated. The predicted failure mode was that two trials' records would not be comparable across instances. The real one is that there are no records: a probe trial produces none, under both the old behaviour and the new. `Probe::run_uncounted` restores the anchor through `restore_quiet`, and `Nes::restore_inner` clears both provenance stores. Before the v2.3.7 fix, a trial's records were therefore destroyed at the start of the next trial — along with the caller's, which is the user-visible defect that fix closes. After it, `TrialGuard` holds both stores aside for the trial's whole duration, which leaves the emulator unarmed while a trial runs. That is deliberate and correct: re-simulated frames never happened on the user's timeline and must not contribute attributions to it. So item B neither shrinks to plumbing nor falls back to bisection. Its first step is a new opt-in capability — a trial that arms a fresh store and hands it back — and the v2.3.7 fix is precisely what makes that cheap and safe rather than dangerous, because the caller's store is already held aside, so a captured trial has no path by which to contaminate the user's timeline. Bisection becomes the fallback, and remains the only option in a build without `debug-hooks`, where capture cannot exist at all. Also recorded: the capture must be per-trial rather than per-`Probe`, since a frame is roughly 29,780 CPU cycles and the Latency Oracle's 21 trials would otherwise accumulate around 625k mix records nobody reads. The substrate table gains the two rows this turned up, stated as findings rather than assumptions: the caller's provenance survives a trial as of v2.3.7 and did not before, and a trial's own provenance does not exist. The verification bar gains both `rustynes-probe` feature configurations, because the crate now compiles different code in each and the Lens lives in the gated one, and it gains the assertion-strength requirement for any capture work — the caller's store must come back byte-identical, not merely non-empty, since a store the trial refilled with frames that never happened would satisfy the weaker check. Worth keeping in view: this was caught at the design stage only because the plan required the question to be answered before either mechanism was written. Left to the obvious order, it would have shipped as a Divergence Lens that returned an empty explanation for every input, which is how Pixel Provenance shipped for four releases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(plans): record what v2.3.8 has landed, and that item B is not closed The plan is what people read to find out what is left, so the state belongs in it rather than being inferred from a branch. Items A, C and D have landed along with the spec; item B's prerequisite has landed and its actual question has not been answered. Two findings carried forward rather than left in commit messages: Item A needed no new engine primitive at all. A trial restores the anchor on the way IN and not on the way out, so the diverging frame reads straight off `nes` — the plan's estimate was wrong in the cheap direction, which is worth recording because the same property is what item B's capture path had to work around. Item A also shipped a bug that reviewing it against `latency::measure_in_place` caught before a reviewer did: `localise` ran its trials and returned, leaving the live emulator wherever the last one ended, so through the panel a question advanced the user's game by thirty frames. Both entry points now restore through one wrapper, and that restore is guarded so it does not clear the caller's provenance — the v2.3.7 defect reproduced one layer up by the branch that fixed it two layers down. Item B is explicitly NOT closed. Trial-scoped capture makes the cheap mechanism possible; whether it beats bisection for a PIXEL divergence is still the measurement the plan asked for, and bisection remains the only option in a build without `debug-hooks`, where capture cannot exist. Marking the prerequisite as the item would be the kind of quiet scope shrink this plan exists to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(plans): item B is closed, and this plan's framing of it was wrong The plan offered two mechanisms for sub-frame localisation — bisect the frame, or ride records that already exist — and required the cheap one to be proved viable before either was written. That instruction did its job twice: first by establishing that a probe trial records no provenance at all, and now by producing an answer that is neither option. For audio, riding the records works directly and resolves to a CPU cycle. For pixels, `localise_explained` returns the diverging pixel's `PixelProvenance` from both configurations, so the answer is which causal input differs rather than at which cycle the two runs parted. That is better than bisection rather than cheaper, which is why the plan's framing was wrong rather than merely unresolved: it treated "when" as the question, and the question is "what, and who". Bisection is not implemented, recorded as a decision. It stays the only route without `debug-hooks`, and it is what to reach for if a case appears whose causal inputs match but whose timing differs. Also recorded: a doc claim retracted mid-item. `differing_fields` was documented as possibly empty at a located pixel. It cannot be — an index-framebuffer entry is `(emphasis << 6) | colour`, so a differing entry means `color` or `color_mask` differs. The permissive reading made an empty result something to render; the correct one makes it a defect, and a mutation reading both records from the same trial was caught only once the assertion matched. The plan doc is carried onto the implementation branch rather than merged separately, so the plan and its execution land together and the status table is accurate at the moment anyone reads it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(probe): assert the pixel provenance DATA survives, not just the box Delivers the strengthening committed to on #405, where review pointed out an asymmetry that was not defensible in that change: the audio preservation tests compare an exact `RegWrite` — same PC, same cycle — while the pixel ones only asserted `pixel_provenance().is_some()`. That checks the stash came back, not that its contents did, so a regression emptying the store while leaving it armed would have passed the pixel tests and failed the audio ones. Both pixel tests now capture a specific pixel's `PixelProvenance` before the trial and assert it comes back identical, via a shared `armed_with_a_pixel_record` helper that returns the record rather than a flag. The probe pixel is mid-screen (128, 120) on purpose: near an edge, a timing difference could legitimately change what the PPU emitted, which would make the assertion flaky for a reason unrelated to what it tests. Demonstrated rather than argued. Mutating `Ppu::put_provenance` to return an ARMED but EMPTIED frame — precisely the regression the review described — now fails both tests. Under the old assertions it would have passed both. This is the same defect class the branch it sits on is about, one level further in: `measure_in_place_restores_the_live_timeline` asserted snapshot equality while provenance is not in the snapshot, and these asserted armed-ness while the contents are not armed-ness. A weaker assertion does not merely test less; it reports a pass for the failure it was written to catch. Verified: fmt, `rustynes-probe` clippy and tests in both feature configurations (57 off, 70 on), workspace clippy, and rustdoc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(lens): three review findings, two of them prose contradicting this PR's code All three from the #407 review, all correct, and two of them are exactly the failure this release line exists to find — a claim that was true when written and false by the time it shipped, in the same PR that falsified it. The module docs said "It does not localise **within** a frame". `localise_audio`, added in this PR, localises to an absolute CPU cycle. Rewritten to say what is actually true: audio narrows below the frame because the mix trace already records one entry per cycle; the pixel path does not and does not need to, because `localise_explained` answers which causal input differs rather than at which cycle the two runs parted. `docs/divergence-lens.md` said "the panel surface is tracked separately". This PR adds the panel. Corrected to name it. The third is a real contract gap rather than stale prose. Every entry point runs FOUR trials and passes the same `input` closure to each, and the engine's whole licence to attribute a divergence to `setup` rests on `setup` being the only difference between two trials. A closure carrying state across calls — a counter, an iterator, an RNG — breaks that silently: baseline and variant receive different input sequences and the reported divergence is attributable to either. `FnMut` is what lets a caller hold a buffer, so the type system cannot enforce purity; it is now stated in the module docs and repeated on the entry points. The intra-doc links had to become plain code spans: both referenced items are `debug-hooks`-gated, so a default `cargo doc` cannot resolve them and the `-D warnings` gate fails. Noted at the site so the next reader does not "fix" them back into links — the same trap the workspace already records for feature-only dependencies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
Running the Latency Oracle or the RAM Atlas emptied both the Pixel Provenance and the Audio Provenance panels.
Nes::restore_innerclears both provenance stores. That is right for a genuine timeline change and wrong for a restore that puts back the state the user is still looking at. v2.3.6 addedNes::take_provenance/put_provenancefor exactly that exception, and wired it intoRunAhead::finish— the one call site the bug report named.rustynes-probehas three more same-timeline restores, and none of them used it:Probe::run_uncountedlatency::measure_in_placeatlas_panel::TimelineGuardBoth stores are cumulative. "Which instruction last wrote this" can point thousands of frames back — a palette byte written at level load, a
$4008linear-counter reload written once during init — so the records were not rebuilt by the next frame. They were gone for the session.Why it survived
This is the defect class v2.3.6 was written about, found in three more places, and two things let it through:
docs/pixel-provenance.mdthen described run-ahead as "the one caller that needs the exception" — a correct rule with an incomplete list under it.measure_in_place_restores_the_live_timelinecould not have caught it. It comparesnes.snapshot()before and after, and provenance is deliberately not in the snapshot, so it asserted something strictly weaker than the contract it is named for.The fix
The stash moves into
rustynes_probe::TrialGuard— renamed fromCaptureGuard, because it no longer carries only the rewind flag. Rewind capture and provenance are the same category of state: both live outside the save state, so neither is carried by a snapshot round trip, and both must be carried deliberately. The guard is entered before the anchor restore, since that restore is one of the things they need protecting from.rustynes-probegains adebug-hookspassthrough feature. Without it the guard would compile out in precisely the builds that need it — feature unification already switches onrustynes-core/debug-hooksfor a frontend build, so the clearing is live, while acfgon a feature the crate does not declare is never true.Verification
Four tests under four independent mutations, one per store and one per site — so a fix that put back only one store, or guarded only one of the two probe restores, fails:
measure_in_placeThat last row is the point:
run_uncountedandmeasure_in_placeneed guarding independently.Gates: fmt, workspace clippy,
rustynes-probeclippy in both feature states, four frontend feature combos, both wasm32 invocations, rustdoc, no_std thumbv7em, probe tests in both states, 5 run-ahead provenance tests, 6 atlas tests.No emulation-core file is touched — the change is confined to
rustynes-probeand one frontend panel — so AccuracyCoin 141/141 and nestest 0-diff hold by construction rather than by assertion.