feat(frontend): export a classified address from the RAM Atlas into RAM Watch - #413
Conversation
…AM Watch v2.3.9 item C. The RAM Atlas classifies all 2 KiB of work RAM and can verify one address by perturbing it, but until now a verified address could not leave the panel. The atlas is cleared at every ROM transition by design -- it describes one observation window on one cartridge -- so "the address I just proved matters" had to be transcribed by hand into the tool that keeps it. RAM Watch is that tool, so it is the destination rather than a second list. `MemoryComparePanelState::seed_watch` appends on another panel's behalf and returns which of two things happened. Two outcomes rather than a bool: a second click on an address already present did not grow the list, so reporting "added" would be a false report of work done, and reporting a failure would be false in the other direction. That is the same refusal to collapse distinct answers that keeps `Untested` and `Inert` apart in the classifier itself. The seeded entry is byte-scoped and never inherits the panel's own size selector. The atlas's evidence -- change count, direction, range, distinct values -- is per byte, so seeding a u16 would attach a byte's verdict to a second address nothing was ever observed about. Widening it afterwards is a decision the user has made rather than one made for them. It also arrives unfrozen, since a frozen entry writes to the game and an export must not. The exported label carries the verdict AND its lens. Liveness is relative to the observable -- a byte is Live through work RAM and may be Inert through the screen -- and the watch list is precisely where an unqualified "LIVE" would outlive the panel that qualified it. `Untested` is spelled out rather than left blank, because once the atlas is gone an entry with no verdict and an entry that was never tested look identical, and an untested export cites no lens at all: nothing was observed through one, so naming one would dress a hypothesis as a measurement. The button is offered for every address, including `Inert` and `Untested`. Restricting it to verified-live addresses would be the paternalistic reading of the panel's honesty rule; the rule is that a claim carries its evidence, not that unverified addresses are unusable. It is also not gated on the netplay / TAS / hardcore predicate that gates Observe and Verify, because it writes to another panel's list and never advances the emulator. The dispatch lives in the overlay, which is the only place holding both panels' state, and it opens the destination window as part of the export. Memory Compare is drawn earlier in the same pass, so the seeded row appears on the next frame -- which is why the outcome is also reported on the atlas's own status line, where the user is already looking. An export that succeeds silently and one that silently does nothing are otherwise indistinguishable, which is the defect shape this release keeps finding. Six tests, five of them mutation-checked: removing the duplicate guard, inheriting the size selector, dropping the lens from a Live verdict, labelling an untested address as Live, and cloning the request instead of taking it each fail exactly their own test.
|
Warning Review limit reached
Next review available in: 59 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 adds an explicit export/hand-off path from the RAM Atlas panel to the existing RAM Watch list (in the Memory Compare panel), so a user can keep a vetted (or intentionally unvetted) address without manual transcription—even though the Atlas is correctly cleared on ROM transitions.
Changes:
- Added an Atlas→RAM Watch “Send to RAM Watch” UI action that queues an export request from the Atlas panel.
- Dispatched the queued export in
DebuggerOverlay(the only layer that can access both panels’ state), seeding RAM Watch and opening the destination panel. - Introduced a
SeedOutcomereturn type +seed_watchAPI and added focused unit tests around dedupe behavior, byte-scoping, and request draining/clearing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/rustynes-frontend/src/debugger/mod.rs | Dispatches the Atlas export request into Memory Compare’s RAM Watch, opens the destination panel, and reports outcome back to the Atlas status line. |
| crates/rustynes-frontend/src/debugger/memory_compare_panel.rs | Adds SeedOutcome + seed_watch API for cross-panel seeding, with tests ensuring dedupe and byte-scoped/unfrozen exports. |
| crates/rustynes-frontend/src/debugger/atlas_panel.rs | Adds export button + queued request plumbing, label construction that preserves verdict/lens honesty, and tests for label semantics and request draining/clearing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Docs-as-spec: the panel gained a behaviour and its spec did not describe it, which is the drift this project's contribution gate exists to stop. Documents the four properties of the export as the honesty rule applied to four different edges -- the label carries its lens, every address is exportable, the entry is byte-scoped and unfrozen, and a duplicate is reported as a duplicate -- each cross-referenced to the section of this document that establishes the rule it applies. Also narrows two "deliberately not implemented" entries rather than deleting them. The remaining export paths are the Cheat panel, Lua and RetroAchievements authoring, and the note now records the one way a cheat export differs: a cheat is a WRITE, so it needs the locked-session predicate the watch export correctly does without. Per-game persistence stays deferred, with the reason stated instead of implied. The obvious form is unsafe -- a restored verdict without its evidence is a claim that cannot be checked, and this panel's whole argument is that its output can be. A restored verdict WITH its evidence is still a statement about the game state the observation ran in, which the next session does not share. The export is the durable path in the meantime, and RAM Watch already has .wch save and load.
|
Added the spec update this PR should have carried from the start ( It also narrows two "deliberately not implemented" entries rather than deleting them, and records one thing worth having written down before the next export lands: a cheat export is a write, so it needs the locked-session predicate (netplay / TAS / hardcore) that the watch export correctly does without. Per-game persistence stays deferred, now with the reason rather than the label: a restored verdict without its evidence is a claim that cannot be checked — which is this panel's whole argument in reverse — and a restored verdict with its evidence is still a statement about the game state the observation ran in, which the next session does not share. The export plus RAM Watch's existing |
Antigravity review (Gemini via Ultra)Adds a button to the RAM Atlas panel that exports a classified address and its verification status to the Memory Compare RAM Watch list. Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
|
@coderabbitai review |
|
|
Antigravity review addressed — no blocking issues; all three points declined, with reasons. Suggestion — shorten the status to "added to RAM Watch". The tooltip and the status line are read at different moments. The tooltip is gone by the time the status appears, and the status is the only thing on screen naming where the entry went — which matters here more than usual, because Memory Compare is drawn earlier in the overlay's pass, so the seeded row itself does not appear until the next frame. Naming the destination panel is the difference between "it worked, look here" and "it worked, somewhere". Nitpick — the intermediate Nitpick — a For the record
|
|
Tip For best results, initiate chat on the files or code changes.
The status text must name RAM Watch because the seeded entry is not visible until the next overlay frame.
The The record about the rate-limited manual review is clear. No CodeRabbit review findings were produced for this pull request. ✏️ Learnings added
🧠 Learnings usedYou are interacting with an AI system. |
v2.3.9 item C, the RAM Atlas half: a classified address can now leave the panel.
Why an export path at all
The atlas is cleared at every ROM transition, by design — it describes one observation window on one cartridge, and 2,048 labels standing across a ROM change would look like a map of a game they were never derived from. That correctness property has a cost: the address a user just verified by perturbing it disappears with everything else, and had to be transcribed by hand into the tool that keeps addresses.
RAM Watch is that tool, so it is the destination rather than a second list.
The three decisions worth arguing about
1. Two outcomes, not a bool. A second click on an address already watched did not grow the list. Reporting "added" is a false report of work done; reporting a failure is false in the other direction.
SeedOutcomekeeps them apart — the same refusal to collapse distinct answers that keepsUntestedandInertapart in the classifier.2. The label carries the lens, or it over-claims. Liveness is relative to the observable: a byte is
Livethrough work RAM and may beInertthrough the screen. The watch list is precisely where an unqualified "LIVE" would outlive the panel that qualified it.Untestedis spelled out rather than left blank — once the atlas is gone, an entry with no verdict and an entry that was never tested look identical — and an untested export names no lens at all, because nothing was observed through one.3. Every address is exportable, including
InertandUntested. Restricting the button to verified-live addresses would be the paternalistic reading of the panel's honesty rule. The rule is that a claim carries its evidence, not that unverified addresses are unusable: anUntestedsparse byte is a good thing to watch while forming a hypothesis, andInertis documented here as not meaning unused.Two smaller ones
u16would attach a byte's verdict to a second address nothing was observed about.Where the dispatch lives, and why the status line exists
In the overlay, the only place holding both panels' state. It opens the destination window as part of the export — but Memory Compare is drawn earlier in the same pass, so the seeded row itself appears on the next frame. Without a line on the atlas's own status area, the user's only evidence would be a list they may not be looking at, and an export that silently succeeds would be indistinguishable from one that silently did nothing.
The button is also not gated on the netplay / TAS / hardcore predicate that gates Observe and Verify: it writes to another panel's list and never advances the emulator.
Verification
Six tests. Five mutations, each failing exactly its own test:
seeding_the_same_address_twice_adds_one_entrywatch_sizeinstead ofSize::U8a_seeded_entry_is_one_byte_and_unfrozenLiveverdictan_exported_verdict_carries_its_lensUntestedaddress asLIVEan_unverified_export_says_soa_watch_request_is_drained_by_the_first_takerThe sixth (
clearing_discards_a_pending_export) rides the existing*self = Self::default(), so it has no independent mutation — stated rather than counted as coverage it does not have.Gates: fmt, workspace clippy,
debug-hooks/full, both wasm32 invocations, rustdoc, frontend suite at 528.Frontend-only; no emulation-core file and no accuracy path is touched.