chore: salvage /tmp doc tools + reflow the v2.3.1 release notes - #349
Merged
Conversation
…reboot
An NVIDIA driver/kernel update needed a reboot, which wipes /tmp. The
`tmp-salvage` dry run offered 305 candidates totalling ~600 MB; three files were
taken and everything else was deliberately left to be wiped.
The three exist NOWHERE ELSE on disk (checked across all of ~/Code) and were
written during the v2.2.5-v2.3.0 documentation work:
reflow.py stdlib-only. Unwraps hard-wrapped markdown into single
full-width lines while preserving blank lines, ATX
headings, horizontal rules, fenced code blocks, tables,
blockquotes and raw HTML -- the constructs that break
when naively joined. This is the tool that fixed the
GitHub release-notes formatting: notes published from
v2.2.5 onward had been hard-wrapped at ~80 columns,
which GitHub renders as artificially narrow text
instead of using the full width available.
assemble.py bs4. Rendered HTML fragment -> standalone document.
guardrails_assemble.py bs4. Same, for the provenance-guardrails doc: injects a
title block and reddens a curated set of hard takeaways.
WHAT WAS LEFT BEHIND, and why -- recorded in docs/SALVAGE_MANIFEST.md so the
curation is auditable rather than implicit:
* perf.data (586 MB), probe.perf, fp.data, di.data, bolt.log -- perf recordings
and CI logs, regenerable with one command;
* ~100 .md/.json PR-review files -- already posted to PR #348;
* fisk-*.png / v2p-*.png (8 MB) -- screenshots that may be commercial-ROM
framebuffers, not swept into the tree unexamined;
* /tmp/accuracycoin-build/ -- third-party MIT source plus a Windows nesasm.exe.
Vendoring third-party source is a deliberate provenance decision under the
project's firewall rules, not a salvage side effect;
* agy-review.sh + selftest -- duplicates of files already in scripts/ and
Local_Only-Projects/antigravity-pr-review/;
* p3.sh/p4.sh/probe*.sh/mi.rs -- extraction artifacts (workflow YAML pulled out
for shellcheck) and a temp probe already reverted on purpose.
Everything built during the session was already committed (frame_probe.rs,
ab_check.sh, frame_breakdown.sh, the perf_log_check.py p99 gate), so the
scratchpad held no unique source beyond these three.
Verification: SHA-256 checked identical to the /tmp originals after copying; all
three parse; reflow.py is ruff-clean and was round-trip tested against a case
covering paragraphs, list continuations, fenced code, tables and headings.
The two bs4 scripts are preserved VERBATIM and are not ruff-clean (SIM115,
UP031). Deliberate: bs4 is not installed here and no sample fragment survives, so
a lint rewrite could not be executed to prove it still behaved. Rewriting code
that cannot be run is a worse trade than a style nit; the README says so and says
to clean them up when someone next has real input to test against.
README-doc-tools.md is added alongside them, because three unexplained scripts in
release-automation/ six months from now are only marginally better than losing
them.
The v2.3.1 release notes were published hard-wrapped at ~80 columns, which
GitHub renders as artificially narrow text instead of using the full width
available. Every other recent release uses the wide format; measured line-length
profiles of the published bodies:
v2.2.4 median=86 p90=329 max=563 wide
v2.2.5 median=165 p90=383 max=1006 wide
v2.2.9 median=491 p90=820 max=1025 wide
v2.3.0 median=245 p90=639 max=725 wide
v2.3.1 median=75 p90=81 max=136 HARD-WRAPPED <-- the regression
v2.3.1 was the only one out of style: the earlier pass that fixed v2.2.5 onward
held, and this was a fresh regression introduced when the notes were authored.
Reflowed with scripts/release-automation/reflow.py -- the tool written for
exactly this and salvaged from /tmp one commit earlier, which makes forgetting to
run it a particularly avoidable miss. Result: median 75 -> 93, p90 81 -> 240,
max 136 -> 463, in line with the rest.
Content is unchanged, which was verified rather than assumed: word count
identical at 719 before and after, 13 table rows and 6 headings preserved on both
sides. reflow.py only joins wrapped continuation lines within a paragraph or list
item and explicitly preserves blank lines, ATX headings, horizontal rules, fenced
code blocks, tables, blockquotes and raw HTML.
The published release body was updated to match via `gh release edit
--notes-file`, so the source file and GitHub agree; all three platform assets
remain attached.
|
Warning Review limit reached
Next review available in: 39 minutes 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?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdded three release-automation scripts for Markdown reflow and HTML assembly. Documented their recovery and usage. Reformatted the v2.3.1 release notes without changing their content. ChangesRelease automation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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
Adds salvaged documentation/release-note helper scripts to the repository and reformats the v2.3.1 GitHub release notes to remove hard-wrapping so they render at full width.
Changes:
- Add
reflow.pyto unwrap hard-wrapped Markdown while preserving common structural Markdown constructs. - Add two BeautifulSoup-based HTML fragment “assembler” scripts plus a short README describing intended usage.
- Reflow
.github/release-notes/v2.3.1.mdand record the/tmpsalvage decision indocs/SALVAGE_MANIFEST.md.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/release-automation/reflow.py | New stdlib-only Markdown reflow tool used to unwrap hard-wrapped release notes. |
| scripts/release-automation/assemble.py | New BeautifulSoup-based HTML fragment → document assembler (one-off tooling). |
| scripts/release-automation/guardrails_assemble.py | New variant of the assembler for the guardrails document with custom title block and highlighting. |
| scripts/release-automation/README-doc-tools.md | Documents the purpose and usage of the salvaged doc/release-note helpers. |
| docs/SALVAGE_MANIFEST.md | Records what was (and was not) salvaged from /tmp and why. |
| .github/release-notes/v2.3.1.md | Reflows the v2.3.1 release notes to remove hard wrapping while keeping content intact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/SALVAGE_MANIFEST.md`:
- Around line 131-132: Clarify the SHA-256 verification statement in the
recovery evidence near the referenced release-automation documentation: state
whether hashes were captured before subsequent edits to reflow.py or other
files, and record the per-file hashes if they constitute release evidence.
Ensure the wording does not imply that the current files remain byte-identical
unless that is verified, while preserving provenance for recovered and
externally sourced artifacts.
In `@scripts/release-automation/assemble.py`:
- Around line 6-7: Validate that exactly two command-line arguments are provided
before assigning paths or reading files in assemble.py at lines 6-7 and
guardrails_assemble.py at lines 8-9; otherwise exit with a clear usage message.
Apply the same argument-count validation and message consistently in both
scripts.
- Around line 27-33: Validate the required table and NOTE paragraph structure
before slicing in the surrounding fragment assembly logic: handle missing
matches with a clear rejection error, and reject any fragment where the NOTE
index is not after the table index. Preserve the existing before, table, and
middle extraction only after confirming both elements exist and table_i <
note_i.
In `@scripts/release-automation/README-doc-tools.md`:
- Around line 33-41: Update the documentation for assemble.py and
guardrails_assemble.py to state that the rendered HTML input must contain a
top-level table and a top-level paragraph whose text starts with “NOTE”. Keep
the existing usage example, and either document that missing elements cause
failure or add explicit validation with an actionable error in assemble.py.
In `@scripts/release-automation/reflow.py`:
- Around line 46-52: Update flush() to join buffered lines without stripping
trailing spaces, preserving Markdown hard-break markers and each line’s
whitespace boundaries. Revise the blank-line handling around the logic at lines
92-98 so authored runs of blank lines are retained rather than collapsed, and
add round-trip coverage for hard breaks and multiple blank lines.
- Around line 56-78: Update the fence handling in the reflow loop to parse
container prefixes first, record the opening fence character and length, and
only close a code block with a matching delimiter of sufficient length. Ensure
fenced blocks inside blockquotes are recognized correctly, while non-fence code
lines remain unchanged; replace the current in_code toggle logic around flush()
and out.append().
🪄 Autofix
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: 72a90697-26bf-471f-a909-a9c06126a940
📒 Files selected for processing (6)
.github/release-notes/v2.3.1.mddocs/SALVAGE_MANIFEST.mdscripts/release-automation/README-doc-tools.mdscripts/release-automation/assemble.pyscripts/release-automation/guardrails_assemble.pyscripts/release-automation/reflow.py
…tests
Six of the nine bot findings were valid and are fixed; three are declined
with reasons below. One of my own claims in the salvage commit turned out
to be factually wrong, which changed a decline into a fix.
## reflow.py: two real bugs, both verified before and after
* **Hard breaks were destroyed.** `flush()` `strip()`ed every buffered
piece, so `"first␣␣\nsecond"` -- two trailing spaces, a markdown hard
break -- came out as `"first second"`. Confirmed by running it. A
hard-break line now terminates its logical line and keeps the marker.
While fixing it: the continuation after a hard break inside a list
item now keeps the ITEM's indent rather than the buffer's first
indent, or the break would end the list item in markdown. That one
was caught by the new test, not by reading.
* **Any fence closed any block.** `startswith(("```", "~~~"))` toggled
state on sight, so ``` closed a ````-opened block and a tilde fence
closed a backtick one. Now the opening character and run length are
recorded and only a matching, at-least-as-long fence closes.
* **Blank-line runs were collapsed**, contradicting the module's own
"preserves blank lines" contract. Markdown renders one blank like
three, so the collapse bought nothing and cost fidelity. Removed.
`test_reflow.py` -- 16 stdlib-only cases -- pins all of it, plus the
preservation guarantees the docstring claims, so doc and code cannot
drift apart again.
## The bs4 pair: input validation + the claim I got wrong
The salvage README said `bs4` "is not installed here", and used that as
half the reason for leaving these scripts untouched. **It is installed**
(4.15.0). Since the stated reason was partly false, the finding it was
used to decline gets fixed rather than restated:
* Both scripts now check `argv` and exit with a usage message instead
of an `IndexError`.
* `assemble.py` validates its input contract -- a top-level `<table>`
followed by a top-level `<p>` starting with `NOTE` -- instead of
letting `next()` raise `StopIteration`, which reads as an interpreter
bug rather than "wrong input". A NOTE preceding the table is also
rejected; previously it emitted the NOTE twice.
* `test_assemble.py` covers each failure and the success path.
The transform bodies are otherwise unmodified: validation is additive and
cannot change the success path, whereas rewriting the transform could --
and **no original rendered fragment survived the salvage**, so there is
still nothing to diff real output against. That half of the original
reasoning stands, and the README now says only that.
## Documentation corrections
* `README-doc-tools.md` documents `assemble.py`'s required input
structure (it had described both tools as taking any fragment), and
corrects the `bs4` claim rather than leaving it standing.
* `docs/SALVAGE_MANIFEST.md` said the files were "SHA-256 verified
identical to source after copy" while the README said `reflow.py` was
fixed after recovery -- a contradiction in what is meant to be
recovery evidence. It now scopes the hashes to the moment of copy and
states plainly that the tree files are no longer byte-identical.
## Declined
* **`str.removeprefix` needs Python 3.9+.** Verified non-issue: CI runs
3.12, this host runs 3.14, and 3.9 shipped in 2020. Changing it would
add code to serve no reachable configuration.
* The two `ruff` nits (`SIM115`, `UP031`) in the bs4 pair stay. They are
in the untested transform bodies, and the README says when to clean
them up: the first time someone runs one for real.
Verified: both test suites pass; markdownlint clean on both documents.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`wide` 1.6.0 was yanked from crates.io, and `cargo deny check` fails on a yanked dependency. This is **not** caused by this PR: the identical commit `9bb6adbe` passed the security workflow on 2026-08-06 and failed it on 2026-08-10 with nothing in the repo having changed between the two runs. The gate is red on `main` right now and blocks every open PR, so it is fixed here rather than left for whichever branch merges first to trip over. Lockfile only -- `crates/rustynes-frontend/Cargo.toml` asks for `"1.5"` and 1.6.1 satisfies it unchanged. `wide` backs the v2.1.8 SIMD blitter (which measured slower than scalar and is not the shipped default, so the blast radius is a code path nothing takes by default); its four unit tests pass, and `cargo deny check` now reports `advisories ok, bans ok, licenses ok, sources ok`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This PR salvages three documentation automation scripts from Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
doublegate
added a commit
that referenced
this pull request
Aug 12, 2026
…estation (#356) * feat(ppu): per-byte write attribution for CIRAM, OAM, and palette RAM v2.3.3 "Lucid" phase 1. Record, for every byte of the PPU's own memories, the program counter and CPU cycle of the instruction that last wrote it. This is the edge the forthcoming pixel-provenance panel walks to get from a pixel on screen back to the code that produced it. ## The gap this closes RustyNES already shipped every ingredient of pixel provenance except one, and the shape of the gap determined the design: * the Trace Logger (`Nes::trace`) has the PC, the registers, and the cycle -- but no link to any effect; * the Event Viewer (`LockstepBus::events`) has the `$2000-$3FFF` CPU write with its PPU scanline/dot -- but neither the PC nor the *resolved* destination; * the memory access counter (`debugger/access_counter.rs`) has per-address read/write counts and a last-access cycle stamp -- but, again, no PC; * `HdTileSource` has rich per-pixel tile context -- but no write history. Nothing recorded the edge from a byte in PPU memory back to the instruction that stored it. That edge is what this commit adds. ## Why the recording is split across the bus/PPU boundary Neither side knows enough alone. The bus knows the program counter; the PPU never sees it. The PPU knows the effective destination; the bus never sees it, because a `STA $2007` lands in a nametable or in palette RAM depending on the PPU's internal `v` register, itself set by two earlier `$2006` writes -- and an OAM byte arrives either through `$2004` or as one of 256 bytes of a DMA burst. So `Nes::run_frame` pushes the executing instruction's `(pc, cycle)` down to the PPU once per instruction, and the store sites -- `Ppu::write_vram`, `Ppu::write_palette`, `Ppu::oam_dma_write`, and the `$2004` write path -- stamp it. The push lives in the block that already performs the breakpoint check and the trace push, so no new `CpuBus` hook was required and `rustynes-cpu` is untouched. `Nes::step_instruction` mirrors it so single-stepping through a `$2007` store attributes correctly rather than inheriting whatever `run_frame` last latched. ## OAM DMA is attributed to its trigger, not to its victim `STA $4014` does not perform the transfer. It arms `dma_pending`, and the 513 or 514 DMA cycles are then stolen from the instructions that follow. Using the live attribution context would therefore name whichever instruction was being halted -- an answer that is true about the timing and wrong about the cause. The bus calls `Ppu::latch_dma_attrib_context()` in the `$4014` write arm, freezing the triggering instruction, and `oam_dma_write` records against that pair. All 256 bytes name the one store that caused them. This was not theoretical: the pinning test initially failed reporting `$C005`, the `JMP` that followed the trigger, which is what surfaced the distinction. ## What is deliberately not attributed * CHR writes (`$0000-$1FFF`) -- mapper-owned, and possibly ROM, CHR-RAM, or a banked board window, so a byte offset is not a stable identity across a bank switch the way a CIRAM offset is. Pattern-table provenance will be reported as the mapper bank, which the mapper already knows, rather than faked with an unstable offset. * Mapper-supplied nametable memory (MMC5 ExRAM, 4-screen boards), which `PpuBus::write_nametable` absorbs before it reaches CIRAM. * The blocked `$2004`-during-rendering write: the hardware quirk discards the value and only bumps `OAMADDR`, so recording it would attribute a byte to an instruction that demonstrably did not write it. * Sprite-evaluation OAM corruption, which is not a CPU write and has no PC to report. ## Lifetime, cost, and the determinism contract The store is cleared on power-cycle and on BOTH save-state restore paths. A restored state's bytes were not written by any instruction this session executed, so the PCs recorded against those offsets describe a timeline that no longer exists; reporting them would be a confidently wrong answer rather than an absent one. Under run-ahead the per-frame `restore_quiet` therefore leaves exactly the visible frame's writes -- the timeline the user is actually looking at. Storage is 2048 CIRAM + 256 OAM + 32 palette slots of 16 bytes each (~37 KiB), allocated lazily: `Ppu::write_attribution()` is `None` until armed, so an unarmed `debug-hooks` build pays one `Option` discriminant test per PPU-memory write and no heap at all. Every accessor masks its index, so a provenance query cannot panic on an out-of-range offset -- a debugging convenience must never be able to take down the emulator it is inspecting. Nothing in the render, timing, or audio path reads any of it. The module is `debug-hooks`-gated, so the default build is unchanged, and the armed and unarmed builds are bit-identical in framebuffer, audio, and cycle counts. ## Verification * `write_attribution_names_the_instruction_that_wrote_a_nametable_byte` -- a hand-assembled NROM whose `STA $2007` sits at a known address; the assertion is pinned to that address, not to whatever the implementation happens to record. * `oam_dma_attributes_all_256_bytes_to_the_triggering_store` -- all 256 bytes name the `STA $4014`. * `write_attribution_is_invalidated_by_restore` -- records drop across a save-state round-trip while the store stays armed. * `provenance::tests` -- cycle 0 is a real record and not a sentinel, indices wrap rather than panic, last write wins, `clear` forgets all. * The standing snapshot-schema audit caught all five new fields on the first run, exactly as designed; each is now listed with the reason it is excluded rather than serialized. * `cargo test --workspace --features test-roms` green: AccuracyCoin exactly 141/141 both directly and through run-ahead, nestest 0-diff, `visual_regression`, `pal_apu_tests`. `cargo fmt --all --check`, workspace clippy and all four frontend feature combos at `-D warnings`, `RUSTDOCFLAGS="-D warnings" cargo doc`, and the `thumbv7em-none-eabihf` `no_std` cross-compile all clean. Both ROM-driven tests loop their write sequence and run several frames, because the PPU ignores `$2000/$2001/$2005/$2006` writes for ~29,658 CPU cycles after reset, and because the first `run_frame` after power-on returns on the already-latched frame-complete flag having executed almost nothing. Both facts were found by the tests failing first. Spec: `docs/pixel-provenance.md`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(ppu): per-pixel causal record for pixel provenance v2.3.3 "Lucid" phase 2. Every emitted pixel now records the layer that won the priority decision, the exact palette address behind its color, and the nametable / attribute / pattern addresses of the tile ACTUALLY ON SCREEN. Composed with phase 1's write attribution, this closes the chain: pixel -> the bytes that produced it -> the instruction that wrote them. ## Why not the `hd-pack` record The plan was to widen `HdTileSource`'s feature gate. That turned out to be the wrong shape, and the reason is worth keeping: `HdTileSource` carries Mesen-format HD-pack *keys* -- `palette_colors` packed for tile identity, an absolute CHR-ROM tile index, the four covering sprites -- not addresses. Widening its gate would have dragged eight `hd-pack` fetch-telemetry fields into every `debug-hooks` build whether or not the panel was open, to deliver a record that answers a different question. So this adds a separate, lazily-allocated `PixelProvenanceFrame` in the same shape as phase 1's attribution store. `hd-pack` is untouched, which makes it byte-identical by construction rather than by careful review. ## `v` cannot answer "which tile is this pixel from" By the time a tile's pixels reach the screen, `v` has advanced two tiles past it. An address derived from `v` at emit time is wrong for every pixel, and wrong in a way that looks entirely plausible -- which is the worst failure mode a provenance tool can have. So a `ProvBgAddrs { nt, at, pattern }` triple rides the same `latch` -> `next` -> `cur` cascade that carries the pattern bytes through the BG shift registers, promoted by the same reloads. Carried as one struct so a promotion is a single copy and the three addresses cannot drift apart. Two findings came out of building it, both surfaced by the test failing rather than by reading the code: * **A tile is defined when its PATTERN is fetched, not when its nametable byte is read.** The PPU performs two dummy nametable fetches at dots 337-340, after the pre-render line's last real tile is fetched but before the visible line's first reload consumes it. Writing the NT address straight into the latch let those dummies overwrite the pending tile, so pixels x=8..15 reported the tile belonging to x=16..23. The addresses are now held pending and committed in `fetch_bg_lo`, which the dummy fetches never reach. * **The attribute address must be carried, not derived.** An MMC5 vertical split supplies its own attribute address that the standard `$23C0 | (v & 0x0C00) | ...` arithmetic cannot reproduce. The `prefetch_shift_bg_regs` promotion was A/B'd rather than assumed: for the visible region it is redundant, because every displayed tile passes through a reload that overwrites `cur` from `next`, and the test passes identically without it. It is kept for the rendering-disable edge, where pixels can be emitted from a frozen shifter before any further reload. An earlier draft of that comment asserted it caused an off-by-one; the measurement disproved that, and the comment now says what is true. ## The one change on the shipped path `emit_pixel`'s priority chain now yields the palette ADDRESS, with a single `read_palette` after it, instead of reading inline in each arm. This is what makes the exact `$3Fxx` available to the record -- including the `$3F10` sprite family pre-mirroring and the rendering-disabled backdrop-override address, neither of which can be re-derived from the priority result without getting the corners wrong. `read_palette` is a pure read whose greyscale mask is untouched by the sprite-0-hit insert, so hoisting it out of the branches is behaviour-preserving. ## Cost The per-pixel guard is a plain `bool` (`Ppu::prov_armed`) rather than `Option::is_some()`: `emit_pixel` runs 61,440 times a frame and is one of the two hottest functions in the emulator, so the unarmed cost is one predicted branch on an already-hot cache line instead of a discriminant behind a pointer chase. Same shape as the bus's existing `event_logging` and `access_logging` flags. Armed, a frame costs `PixelProvenanceFrame::HEAP_BYTES`. ## What is deliberately not recorded * **The primary OAM sprite number.** Sprite evaluation copies bytes from primary to secondary OAM without retaining the source index, so it does not exist at emit time. The record reports the secondary-OAM slot; the panel matches its Y/tile/attribute against OAM rather than being handed an index the PPU never kept. * **Sprites that lost the priority decision.** Phase 2 records the winner; folding in the covering-sprite list is a phase 3 concern. ## Verification * `pixel_provenance_names_the_displayed_tile_not_the_fetch_pointer` -- a hand-assembled CHR-RAM ROM that fills a nametable and a palette, then asserts the record for a background pixel names the tile ON SCREEN and that the cascade advances exactly one tile per 8-pixel group across the scanline. * `provenance_and_attribution_compose_into_a_causal_chain` -- the end-to-end claim: pixel -> palette entry -> the PC that wrote it. * The snapshot-schema audit caught all eight new fields, again on the first run; each now carries the reason it is excluded rather than serialized, including the honest note that a mid-scanline restore can leave one tile group reporting pre-restore addresses. * `cargo test --workspace --features test-roms` green with zero failures: AccuracyCoin exactly 141/141 both directly and through run-ahead, nestest 0-diff, `visual_regression`. `cargo fmt --all --check`, workspace clippy and all four `rustynes-ppu` feature-gate combinations at `-D warnings`, `RUSTDOCFLAGS="-D warnings" cargo doc`, and the `thumbv7em-none-eabihf` `no_std` cross-compile clean. A third trap the tests found, recorded in the spec so it is not rediscovered: `$2006 = $20, $00` sets `v = $2000`, and bits 12-14 of a VRAM address ARE the fine-Y field -- so that write leaves fine-Y at 2, not 0. The "wrong" pattern address that flagged it was correct. Spec: `docs/pixel-provenance.md`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(frontend): Pixel Provenance inspector panel v2.3.3 "Lucid" phase 3. Tools -> Pixel Provenance: pin a screen pixel and read its whole causal chain. This is the surface that makes phases 1 and 2 usable -- the records existed, but nothing showed them. ## What it reports * **Emitted** -- scanline, dot, screen X, the winning layer, the palette colour swatch, and the grayscale / emphasis bits in effect. * **Palette** -- the address PRE-mirroring (so `$3F10` shows as `$3F10`, the address the program actually used, not the `$3F00` it reads), the entry index, and the instruction that last wrote it. * **Background tile** -- nametable and attribute addresses with their resolved CIRAM offsets and the instructions that wrote those bytes, the palette group, pattern bits, fine scroll, and pattern address. Shown for sprite pixels too, since the background is what the sprite won priority OVER. * **Sprite** -- slot, priority, pattern bits and address, the sprite-0 flag, and the instruction that wrote the slot's OAM bytes. Arming lives in the panel: two checkboxes, both default off, both determinism-neutral. That is the panel's only side effect on the emulator; the report itself is read-only. ## Reuse, and one thing deliberately not reused The coordinate picker follows `hd_pixel_panel.rs`; PC -> source-line resolution reuses `source_map.rs` (a bonus row, present only when a `.dbg` is loaded); the read-only-over-`&Nes` wiring -- `show_*` flag, `*_ui` state, `ToolPanel` variant, `any_nes_tool_open`, the `detachable_window` registration -- follows `rom_info_panel.rs`, so the panel inherits v2.3.0's real multi-viewport pop-out for free. `event_panel.rs`'s scanline x dot timeline was NOT folded in, despite being on the plan's reuse list. The record already carries its own scanline and dot, and a second timeline widget plotting one point would be decoration rather than information. ## Nametable address -> CIRAM offset `Nes::ciram_offset_for_nametable_addr` turns a PPU-space address into the offset `WriteAttribution::ciram` is keyed on. It shares a new `resolve_nt_addr` helper with `PpuBusAdapter::nametable_address` -- ONE definition -- so a board with a per-game mirroring override cannot have the fetch path and the panel disagree about where a byte lives. On boards with mapper-supplied nametable memory (MMC5 ExRAM, 4-screen) some writes never reach internal CIRAM. The function still returns the standard-mirroring offset, because the only way to know whether the mapper absorbed a particular write is to PERFORM one: `write_nametable` takes `&mut self` and has side effects, and a read-only query has no business inventing one. A missing attribution on such a board means "the mapper owns this byte", which the panel states rather than leaving the row blank -- an absent attribution is information, and silently omitting it would read as "nothing wrote this". ## The wiring trap this hit The panel is deliberately NOT gated on the frontend's `debug-hooks` feature. That feature is an ALIAS: `rustynes-frontend` always pulls `rustynes-core` with `debug-hooks` enabled, so the core API is always present, but the alias itself is off by default and exists only so `cargo clippy -p rustynes-frontend --features debug-hooks` resolves. Gating on it -- which the first draft did, in nine places -- would have shipped the panel permanently unreachable in every real build. It surfaced by accident: the menu entry referenced an icon glyph that does not exist (`MAGNIFYING_GLASS` rather than `MAGNIFYING_GLASS_PLUS`) and `cargo check` passed anyway, because the whole block was being `cfg`'d out. A compile error that does not fire is a stronger signal than one that does. ## Verification * `cargo test --workspace --features test-roms` green with zero failures: AccuracyCoin exactly 141/141 both directly and through run-ahead, nestest 0-diff, `visual_regression`. * `cargo fmt --all --check`; clippy at `-D warnings` for the workspace and for every frontend feature combo -- `scripting`, `scripting,hd-pack`, `retroachievements`, `debug-hooks`, `full` -- plus BOTH wasm targets (default and `wasm-canvas`). * `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps` and the `thumbv7em-none-eabihf` `no_std` cross-compile clean. Frontend-only apart from the read-only core accessor, so the deterministic core is untouched. Spec: `docs/pixel-provenance.md`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(movie): deterministic replay attestation + `rustynes verify` v2.3.3 "Lucid" phase 4. A `.rnm` movie can now carry a rolling hash of its run, and anyone with the ROM can independently re-derive it: $ rustynes verify run.rnm --rom game.nes VERIFIED: 150 frames reproduced exactly (hash 23248f1a4b49f4e6). This is the determinism contract turned into evidence. The core already re-derives every pixel from the same ROM plus the same inputs; recording what that produced makes the run checkable by a third party. ## No format-version bump was needed The plan called for a "v3 tail". It turned out `.rnm` already had the precedent: `rerecord_count` is read with `r.u32().unwrap_or(0)`, so a reader that stops earlier ignores it. The attestation is appended the same way behind an `ATTESTATION_MAGIC` marker, so `MOVIE_FORMAT_VERSION` stays at 2, every existing movie round-trips byte-for-byte unchanged, and a pre-v2.3.3 reader parses an attested movie as a plain one. `attested_movie_stays_readable_as_a_plain_movie` pins that by truncating the tail and reparsing. ## The mistake that shaped what is hashed The first implementation hashed the framebuffer alone. The end-to-end tamper check then flipped a button bit in a movie recorded against a test ROM -- and `rustynes verify` confirmed the tampered movie as GENUINE. It was right to. That ROM never reads $4016, so the video output really was identical. But the claim was wrong: output alone does not pin the input stream, and a ROM that ignores input (a test ROM, an attract-mode demo, a cutscene) makes an output-only hash worthless as evidence precisely where evidence matters. Each frame now folds in the input applied AND the framebuffer it produced, so the attestation states the honest claim: *these inputs, applied to this ROM, produced this output.* `flipped_input_fails_even_when_the_rom_ignores_input` keeps it that way. This was found by running the thing, not by reading it. ## Hashing the snapshot instead -- considered and rejected A core-snapshot hash would detect strictly more divergence. It was rejected because the snapshot schema is versioned and bumps between releases (`PPU_SNAPSHOT_VERSION` is at 8), so every bump would silently invalidate every previously-recorded attestation. A 256x240 RGBA framebuffer is stable for as long as the NES is the NES, and an attestation is only worth recording if it can still be checked years later. Audio is NOT covered -- samples are drained by the host as they are produced, so the core cannot see a whole run's audio without frontend cooperation. Documented as a gap rather than implied as coverage. ## Diagnosis, not just a verdict A checkpoint every `ATTESTATION_CHECKPOINT_INTERVAL` (64) frames costs 8 bytes each (~4.5 KiB for ten minutes) and lets a mismatch report the 64-frame window the divergence began in. Verified end to end: a bit flipped at frame 100 reported "first divergence in frames 64..=127". Exit codes are deliberately distinct -- 0 verified, 1 mismatch or error, 3 not attested. A movie that makes no claim has not failed, and collapsing the two would leave a script unable to tell them apart. ## Recording, and the run-ahead hazard The frontend arms attestation automatically when recording starts, EXCEPT while run-ahead is enabled, where it says so and records a plain movie. Run-ahead presents the frame N ahead of the persistent timeline; a verification replay has no run-ahead and re-derives persistent frames, so attesting the presented image would record a hash nobody could reproduce. `emu.rs` folds frames in only on the non-run-ahead path. If run-ahead is toggled on mid-recording, the attestation's frame count falls short of the input stream's -- which `Movie::deserialize` detects and drops the tail for. The failure mode is "no attestation", never "a wrong one", and that guard is tested directly. Two other paths produce no attestation on purpose: a history-viewer export (the rewind ring stores state, not the per-frame video an attestation hashes) and a TAStudio export (an edited input stream has no single continuous run whose output could honestly be described). ## Verification * Six unit tests: independent-replay match, round-trip + absence when not recorded, old-reader compatibility, forged-hash rejection, frame-count-mismatch rejection on load, and the input-tamper case above. * End-to-end through the real CLI against a committed test ROM: a genuine run verifies (exit 0) and a one-bit input edit is caught with its checkpoint window (exit 1). * `cargo test --workspace --features test-roms` green with zero failures: AccuracyCoin exactly 141/141 direct and through run-ahead, nestest 0-diff. * `cargo fmt --all --check`; clippy at `-D warnings` for the workspace, every frontend feature combo, and both wasm targets; `RUSTDOCFLAGS="-D warnings" cargo doc`; the `thumbv7em-none-eabihf` `no_std` cross-compile (the attestation is `core` + `alloc` only). Spec: `docs/pixel-provenance.md`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(deps): bump gradle/actions, taiki-e/install-action, and the clap group Consolidates the three Dependabot PRs that can land, in the pattern used for #305 and #316: apply them on the release branch, verify once, close the individual PRs against this commit. * `gradle/actions/setup-gradle` v6.2.0 -> v6.3.0 (#350), one site in `android.yml`. The `cache-encryption-key` / closed-source caching note above the pin is unaffected. * `taiki-e/install-action` v2.85.5 -> v2.85.10 (#351), both sites in `security.yml`. * clap 4.6.4 -> 4.6.6, clap_builder 4.6.2 -> 4.6.6, clap_complete 4.6.8 -> 4.6.9 (#352) -- `Cargo.lock` only, exactly the three crates Dependabot's `production-dependencies` group named. Reproduced with `cargo update -p clap -p clap_complete` rather than by taking the branch, so the lockfile stays a product of our own resolve. The clap bump matters slightly more than usual this release: v2.3.3 adds the `rustynes verify` subcommand, so the CLI surface is larger than it was when these versions were last exercised. Its 14 CLI tests pass. ## The other three (#353 / #354 / #355) are NOT here egui 0.35 -> 0.36 is blocked upstream, not by our code: `egui-winit` 0.36.1 fails to compile for `wasm32-unknown-unknown` because `egui` 0.36.1 declares `DroppedFile::bytes` only under `cfg(not(target_arch = "wasm32"))` while `egui-winit` implements it unconditionally. RustyNES ships a wasm demo, so that is a hard blocker, and both wasm clippy gates catch it. The full migration -- which also requires wgpu 29 -> 30, since `egui-wgpu` 0.36 needs it and a mixed graph fails to unify device/queue types -- is complete and native-green on the branch `chore/egui-0.36-wgpu-30-blocked`, with its five API deltas and their verification gaps documented in that commit. Rebase it when upstream publishes the fix. Verified: workspace clippy and both wasm clippy gates clean at `-D warnings`; `cargo fmt --all --check` clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * release: v2.3.2 "Lucid" — pixel provenance + replay attestation Cuts the release: workspace version 2.3.1 -> 2.3.2, CHANGELOG section opened, `docs/STATUS.md` and the AGENTS.md current-release block brought current, and `.github/release-notes/v2.3.2.md` written for `release-auto.yml` to publish. ## Numbering Lucid takes 2.3.2, not the 2.3.3 the plan assigned it. "Grain" (the frontend/coupling work) is still on `feat/v2.3.2-grain-frontend` awaiting a maintainer p99 capture, so cutting Lucid at 2.3.3 would tag it before 2.3.2 existed and leave the release list reading out of order. Grain moves to 2.3.3 when its capture lands. Maintainer's call, taken before the version literal was written, because a published tag is immutable. ## Release notes format Written in the project's WIDE format (p50 127 / p90 315 columns), not hard-wrapped — matching v2.2.4 (p90 329) rather than v2.3.1, which shipped at p90 81 and rendered artificially narrow on GitHub. Note that `scripts/release-automation/reflow.py`, the tool for this, is still on the unmerged PR #349; these notes were authored wide directly. ## The AGENTS.md block was two releases stale It still described v2.3.0 as current — v2.3.1 never updated it. Now leads with v2.3.2, then v2.3.1, then the existing v2.3.0 chain. Verified: `cargo test --workspace --features test-roms` green with zero failures (AccuracyCoin exactly 141/141 direct and through run-ahead, nestest 0-diff); markdownlint clean on every touched document. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address the PR #356 review — three real bugs, two overclaims, a bad version sweep 21 bot findings. Most were valid; three were bugs in code I wrote and two were claims that overstated what the feature does. Details below, grouped by what they actually were rather than by thread. ## Bugs * **The panel indexed primary OAM with a secondary-OAM slot.** `WriteAttribution::oam` is keyed on primary OAM byte addresses; `sprite_slot` is the per-scanline secondary slot. Sprite evaluation copies only in-range sprites, so slot N is not primary sprite N whenever an earlier sprite was skipped -- the row confidently named the instruction that wrote a DIFFERENT sprite. Worse, the panel documented that the primary index does not exist at emit time one paragraph below the code that used the slot as if it did. The row is removed until the index is actually carried; the caveat now says why there is no row. * **A short checkpoint list manufactured a mismatch.** `att.checkpoints.get(idx)` returns `None` when the recorded list is shorter than the replay's checkpoint count, and `None != Some(&hash)` is true -- so `verify` reported divergence even when every hash the movie did record, including the final one, matched. Absence now means "nothing recorded here", not disagreement; the final hash remains the gate and the checkpoints only localize. * **A rewind mid-recording silently invalidated the attestation.** A successful `rewind_step_back` restores an earlier state while the input log keeps its full prefix, so the frame counts stay self-consistent and `Movie::verify` reported `Mismatch` on an honest recording. The frontend now drops the attestation on a successful rewind (`MovieUi::invalidate_attestation`) -- "not attested" is the truthful outcome. ## Overclaims * **"prove it is genuine and unmodified" was wrong.** The digest is 64-bit FNV-1a: not collision resistant, and its round function is invertible. It is tamper-EVIDENT -- it catches accidental divergence and casual edits -- but a motivated forger can edit the movie and recompute it. Every surface now says "reproduces the recorded run". Establishing authorship needs a signature over the record with a key the verifier trusts; that is a different feature. * **The verifier assumes a default core profile.** `run_verify` builds a plain `Nes` from ROM bytes, so a recording made with Four Score, a PPU die-revision or power-on model, a game-database override, or a soft-patched ROM cannot reproduce -- and would report a mismatch that is the profile's fault, not the movie's. The format carries no profile field, so the CLI now states the assumption up front instead of mis-blaming the movie. Recording-side eligibility is follow-up. ## A justification I had to withdraw The snapshot-schema audit excluded `prov_frame` on the grounds that it is "overwritten by the next `run_frame` exactly like the framebuffer it shadows, so there is nothing for a restore to carry". That analogy is false: the framebuffer IS serialized and returns consistent with the restored state, while `prov_frame` is not -- so a restore landing mid-frame left pre-restore tile and palette addresses for every pixel above the current scanline, unmarked. Exactly the "confidently wrong answer" the attribution clear exists to prevent. Pixel provenance is now cleared on power-cycle and both restore paths, like attribution, and the audit entry records the corrected reasoning rather than the old one. ## The version sweep I botched When the release was renumbered 2.3.3 -> 2.3.2, I updated `Cargo.toml`, the CHANGELOG, STATUS and AGENTS -- and left **65** `v2.3.3` strings in source comments and public rustdoc, including `Nes::set_write_attribution` and `Nes::set_pixel_provenance`, which ship to consumers. All swept. `docs/performance.md` keeps its one reference: it points at Grain's frontend work, which genuinely IS v2.3.3 now. `AGENTS.md` and `README.md` also still announced v2.3.0 as current -- v2.3.1 never updated them. Both now lead with v2.3.2. ## Tests for the untested path No test drove `first_bad_checkpoint` to `Some(_)`, which is where the false-mismatch bug lived. Added three: a corrupted middle checkpoint is localized to its window while the final hash still agrees; a short checkpoint list still verifies (this one fails without the fix above); and disabling attestation mid-recording yields an unattested movie with the recording intact. ## Also `prov_spr_addr` initializes to `PATTERN_ADDR_NONE` rather than `0` (0 is a valid CHR address; unreachable today, but the adjacent `hd_spr_addr` uses its own sentinel for the same reason). The pre-v2.0.0-timebase warning no longer bakes 14 alignment spaces into the middle of the sentence -- `rustfmt` had collapsed a `\`-continued literal. The OAM DMA cycle count reads 513/514 consistently. `wide` 1.6.0 -> 1.6.1, the yanked-crate fix that unblocks `cargo deny` (pre-existing on `main`). Declined: the CHANGELOG's narrative of the failed first hash design moved to the release notes, keeping the CHANGELOG on shipped behavior. Verified: `cargo test --workspace --features test-roms` zero failures, AccuracyCoin exactly 141/141 direct and through run-ahead, nestest 0-diff; fmt; clippy at `-D warnings` for the workspace, all four frontend feature combos, all four `rustynes-ppu` gate combos, and both wasm targets; `RUSTDOCFLAGS="-D warnings" cargo doc`; `no_std` cross-compile; `cargo deny check` all-ok; markdownlint on every touched document. 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.
Two small chores, no code changes.
1. Salvage three doc tools from
/tmpbefore a rebootAn NVIDIA driver/kernel update needed a reboot, which wipes
/tmp. Thetmp-salvagedry run offered 305 candidates / ~600 MB; three files were taken. All three exist nowhere else on disk (checked across~/Code).reflow.pyassemble.pybs4guardrails_assemble.pybs4Left behind deliberately, recorded in
docs/SALVAGE_MANIFEST.mdso the curation is auditable: a 586 MBperf.dataand other perf recordings (regenerable), ~100 PR-review files already posted to #348, screenshots that may be commercial-ROM framebuffers, and/tmp/accuracycoin-build/— third-party MIT source plus a Windowsnesasm.exe, where vendoring is a deliberate provenance decision rather than a salvage side effect.The two
bs4scripts are preserved verbatim and are not ruff-clean (SIM115,UP031). Deliberate:bs4isn't installed and no sample fragment survives, so a lint rewrite couldn't be executed to prove it still behaved.reflow.py— testable, being stdlib-only — was fixed and round-trip verified.2. Reflow the v2.3.1 release notes
They were published hard-wrapped at ~80 columns, which GitHub renders as artificially narrow. Measured across published bodies:
v2.3.1 was the only one out of style — a fresh regression, not a return of the earlier problem. Fixed with
reflow.py(salvaged one commit earlier, which makes forgetting to run it a particularly avoidable miss): median 75 → 93, p90 81 → 240, max 136 → 463.Content verified unchanged rather than assumed: word count identical at 719, 13 table rows and 6 headings preserved. The published release body was updated to match via
gh release edit, and all three platform assets remain attached.Verification
pre-commitclean (markdownlint, EOF, whitespace, line endings);reflow.pyruff-clean and round-trip tested against paragraphs, list continuations, fenced code, tables and headings; SHA-256 verified identical to the/tmporiginals after copying. No Rust touched — AccuracyCoin and nestest unaffected by construction.Summary by CodeRabbit
Documentation
New Tools