ci(v2.3.9): open "Crucible" — what the gates actually cover - #408
Merged
Conversation
…emise checked A crucible is where something is tested to destruction rather than inspected, and that is what this release is about: the gates, what they cover, and what they only appear to cover. Not a feature release. The v2.3.x line has added five tools in four releases, and the recurring finding across all of them has not been that the emulation was wrong — it is that a check reported a pass it had not earned. **Item A5, `test-roms` at review time, is designed rather than sketched**, and the design is checked against the real case rather than argued from principle. The gap stated precisely: `setup` computes one `full` flag, and `test-roms` runs only when it is true — push to `main`, the merge queue, dispatch, the weekly cron, and `release/*` PRs. A regular feature PR never runs the accuracy battery, so an accuracy regression cannot be caught on the PR that causes it. The proposal is to scope by path rather than by event: a visual vector can only move if something that produces one changed, and a frontend-only PR — most of this line — cannot move `scanline_frame_180` whatever it does. A second `paths-filter` output covering the chip crates, core, gamedb, the harness and `tests/`, with `test-roms` running when it OR the existing full flag is true. Verified against the worked example rather than recalled: #396 touched `crates/rustynes-ppu/src/ppu.rs` and #403 touched `crates/rustynes-test-harness/tests/snapshots/`, so both would have run the battery on the PR. `rustynes-gamedb` is included for a non-obvious reason — it rewrites the iNES header on load, so it changes what the emulator is before a cycle runs, which is how the v2.3.4 Sachen defect reached users. **The plan's own open question is settled in the same commit**, because leaving it open is how a design gets written against an assumption. It asked whether a merge queue already runs the battery before merge, which would have made A5 much less valuable. It does not: no `merge_group` event appears in the run history, and `main`'s active ruleset requires exactly one status context, `CI success`. That check sharpens the problem rather than softening it. `CI success` is the single gate on merging, and on a regular feature PR it is satisfied **without the accuracy battery having run** — a skipped job does not fail the aggregate. The required check reports a pass for a property it did not test, which is the same shape as every other finding in this line. Item B generalises the v2.3.6-v2.3.8 frontend defects into the class they now demonstrably are — four instances, two questions to sweep every v2.3.x feature against — and notes that the "did it put back what it borrowed" question has a mechanical answer available: enumerate what is not serialized, because that is exactly the set a snapshot round trip cannot restore. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…k it `test-roms` was FULL-run only — push to `main`, the merge queue, dispatch, the weekly cron, and `release/*` PRs. A regular feature PR never ran the accuracy battery, so an accuracy regression could not be caught on the PR that caused it: it landed, turned `main` red, and needed a second PR to fix. What sharpens this from an inconvenience into a hole: `main`'s active ruleset requires exactly ONE status context, `CI success`, and that job fails on a need whose result is `failure` or `cancelled` — **not** `skipped`. So a skipped `test-roms` satisfied the only gate on merging, and the required check reported a pass for a property it had never tested. Scoped by path rather than by event, because a visual or accuracy vector can only move if something that PRODUCES one changed. A frontend-only PR cannot move `scanline_frame_180` whatever it does. A second `paths-filter` output covers the chip crates, core, gamedb, the test harness and `tests/`; `test-roms` now runs when that fires OR on a full run as before. The cost model the full-run flag exists for is preserved rather than traded away, and that is measured rather than assumed: over the last 40 merged PRs, 11 touch these paths and 29 do not, so roughly 72% of PRs still skip the battery entirely. Had the split come out near half, this would have needed restating rather than adopting. Verified against the worked example rather than recalled. #396 touched `crates/rustynes-ppu/src/ppu.rs` and #403 touched `crates/rustynes-test-harness/tests/snapshots/`, so both would have run the battery on the PR where the regression was introduced and on the one that fixed it. `rustynes-gamedb` is included for a reason that is not obvious from its name: the per-game database rewrites the iNES header on load, so it changes what the emulator IS before a single cycle runs — which is how the v2.3.4 Sachen defect reached users. `rustynes-probe` is deliberately EXCLUDED: it is an output-only analysis engine with no path to affecting emulation, so including it would cost 29 minutes for nothing. Two demonstrations, because a gate that cannot fail is not a gate. The negative one rides on this PR: it touches only `.github/` and `to-dos/`, so `test-roms` must be SKIPPED here — if it runs, the filter is too broad. The positive one is the next PR, v2.3.8's branch, which touches `crates/rustynes-ppu` and will be the first to exercise the gate for real. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e finding Item B asked whether any state that lives outside the save state is missing a guard, and proposed enumerating what is not serialized as the mechanical handle. That enumeration already exists. `snapshot_schema_audit.rs` lists every deliberately-excluded field with a written reason and fails the suite when a new one appears without one, so there is no second list to build — the reuse-before- writing rule applies and this is what it points at. Cross-referencing its 26 output-only fields against "must a guard carry this across a same-timeline restore?" splits them cleanly once the `see <sibling>` indirections are resolved. Everything is rebuilt every frame or every fetch except two: `write_attrib` and `audio_prov`'s `reg_attrib`, both cumulative in the sense that matters — "which instruction last wrote this" can be answerable from thousands of frames ago. Both are already carried, by `ProvenanceStash` and `AudioProvenanceStash`. So the sweep finds NO missing guard, and that negative result is the useful output: the v2.3.7 fix covered the whole set rather than merely the members that had been reported. The actual v2.3.9 finding is the limitation of the method that produced it. The classification required reading prose. `attrib_cycle` says only "see `attrib_pc`"; the ten `hd_*` fields say "hd-pack fetch telemetry" without saying whether that is per-frame; and a keyword pass over the reasons mis-sorted 17 of 26 on the first attempt — my own first attempt, which is why the number is quoted rather than estimated. So the enumeration exists and the durability question it needs to answer is not machine-checkable. Nothing stops the next output-only field from arriving with a reason that is true but silent about whether it survives a restore, and the guard that should carry it from never being written. Recorded as a sized proposal rather than a commitment: give each field a machine-readable durability tag and assert every cumulative one is named by a guard, which converts a prose cross-reference done once into a gate. It needs a way to enumerate guards, so it is not free, and pretending otherwise in a plan is how an estimate becomes a promise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Carried forward as "internal lag plus the frontend pipeline cost `perf.rs` already tracks". That is very nearly right, which is what makes it worth investigating before writing rather than after. The panel today reports `frames * frame_ms` labelled "about N ms of the game's own delay" — correctly scoped, and derived from the console's own frame duration rather than a hardcoded 16.639, which is what keeps PAL and Dendy honest. Three things the one-line note omits: WHICH FIGURE. There are two and they are not interchangeable. `RenderPerf` deliberately splits `wait` — the blocking present — out of `total`, because under Fifo a present that blocks until vblank is correct behaviour rather than a stall. So "internal lag + work + lock" answers how much could in principle be removed, while "internal lag + total" answers how long until the pixel is actually on screen. Publishing one under the other's name is the exact class of error this release exists to find, so the design is to report both, labelled — the same refusal to collapse distinct answers that keeps `None` and `Some(0)` apart in this very panel. RUN-AHEAD. The Oracle measures the game's internal lag, a property of the ROM and independent of the frontend. Run-ahead then removes up to `depth` frames of it. An end-to-end figure that ignores the configured depth overstates what the user experiences, and overstates it worst precisely when they have taken the panel's own advice. THE PERCENTILE ARITHMETIC, which is sound here for a reason worth writing down. `perf.rs` already records that `work p95 = total p95 - wait p95` is invalid, and that the first attempt at it published a table whose `work p95` sat below its `work p50` — percentiles cannot decrease, which is how the error announced itself. Adding is a different operation: internal lag is a CONSTANT, and adding a constant to a distribution shifts every percentile by exactly that constant, so `lag_ms + work_p95` is a real p95 of the total. The valid and invalid cases look alike at a glance, so the distinction is recorded at the design rather than left for whoever writes the arithmetic. No code yet, deliberately. A latency number whose definition is ambiguous is worse than no latency number, and which of the two figures leads is a maintainer call rather than an implementation detail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…es today The cross-compile gate provisions glibc headers for bindgen with a bare `apt-get update && apt-get install`. Both are network fetches with no timeout of their own, so a stalled mirror hangs the step until the JOB timeout fires — 25 minutes for `libretro-cross` — and the run is then reported as cancelled, which reads as infrastructure noise rather than as the specific thing that happened. Not hypothetical, and not rare. During the v2.3.7 cut this hung FOUR times across two PRs: twice in `rust-setup`, once in the armhf provision, once in the aarch64 provision. Every one was in a setup or provisioning step; not one was in a compile or test step. Each cost 25-45 minutes and a manual re-run, and the last of them blocked the release PR itself. The per-job `timeout-minutes` from #400 did exactly what it was for — it bounded the damage. What a job-level timeout cannot do is NOTICE: a stalled fetch inside a 25-minute budget is indistinguishable from a job that is merely slow. Two bounds, doing different jobs. `timeout` per command, so a stalled fetch fails in minutes rather than consuming the whole budget — the job timeout is a backstop against a hang, this is the thing that notices one. And three attempts with linear backoff, because the observed failure is transient: a re-run has cleared it every time. Every attempt emits a `::warning::`, including attempts that eventually succeed. A run that needed three and a run that needed one are identical in the job's conclusion, and that difference is the early warning that the mirrors are degrading — the same reasoning that makes `Inconclusive` a distinct answer everywhere else in this line. Verified by stubbing `sudo`, `timeout` and `sleep` rather than by reading it: all-fail gives three warnings and exit 1; fail-fail-succeed gives two warnings, names the attempt it succeeded on, and exits 0. My first stub was mis-designed — it assumed two `sudo` calls per failing attempt, when a failed `update` short-circuits the `&&` and makes one — which is worth recording because the wrong test looked like a script bug for a moment. Deliberately not a general-purpose apt wrapper: one package, taken from a workflow `env:` and never from event data, which is the injection vector the Actions security guidance names, with a hard failure when unset. The sysroot export moves to its own step so the retry script owns exactly the fetch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eview doubted `check-yaml` proves a file is valid YAML. It says nothing about whether a key is legal where it appears, and seven workflow files were edited during the v2.3.7 cut with no schema check at all. The gap surfaced as a review disagreement on #406 that GitHub's own documentation could not settle: is `timeout-minutes` valid on a job calling a reusable workflow? Neither the workflow-syntax page nor the reuse-workflows page says. `actionlint` encodes the job schema and answered in one command — it is not, and the seven allowed keys are `name`, `uses`, `with`, `secrets`, `needs`, `if`, `permissions`. So adding one is a syntax error rather than an ignored key, which means the existing comment understated the consequence rather than overstating it. Both that comment and the CHANGELOG entry now carry the evidence instead of asserting the conclusion. Adopted as a pre-commit hook, pinned to the installed version like every other hook in that file, for the reason `markdownlint` already documents there: an unpinned linter that gains a rule turns a green tree red on someone else's machine. Two suppressions were needed and both are against intent rather than against correctness. `pgo.yml` carries two deliberately-disabled `if: false` steps, kept rather than deleted so the shape survives for whoever re-enables them behind a harness that benches the right binary. Suppressed in `.github/actionlint.yaml`, scoped to that one file and that one message — and the scoping is VERIFIED, not assumed: adding an `if: false` to `security.yml` still fails the lint. The cost is written into the config rather than buried, because it is real — the rule is now silent for all of `pgo.yml`, actionlint has no line-scoped ignore, and the alternatives are worse (deleting the steps loses the documented shape, rewriting the condition hides intent from the reader to satisfy a linter). The more interesting finding is that adding the config ACTIVATED a dormant check. The self-hosted `agy` runner label was reported as unknown only after `actionlint.yaml` existed — before that, actionlint had nowhere to expect a custom-label declaration and said nothing. A linter config is not inert: the first run after introducing one is where the real inventory appears, not the run before. The label is now declared, which is where a custom runner label should have been documented regardless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… a fix
The sweep's second question ("does it put back what it borrowed?") came back
clean. The first — "can the panel observe what it claims to?" — did not, and what
it found is a candidate rather than a confirmed defect. Recording the difference
is the point.
The lock discipline is sound where it matters most: `produce_one_frame` runs
under one `emu.lock()` held across the whole frame, so a UI read can never catch
a half-filled provenance store. Whatever a panel sees is some complete frame.
WHICH complete frame is the open question. In the `needs_nes` render arm — the
branch taken precisely when a debugger or tool panel is open — there are TWO lock
acquisitions per redraw, not one. The first copies the framebuffer, index
framebuffer, phase and HD tiles into staging, and is what the user ends up
seeing. The second, sixty lines later, is the egui pass where `run_shell_ui`
hands `&mut Nes` to the panels, and is where Pixel Provenance and Audio
Provenance read.
The guard is dropped between them, deliberately, so the composite work does not
hold the emulator. Which means the emulation thread may take the lock in that gap
and produce another frame — and if it does, the screen shows frame N while the
panel describes frame N+1. That would be a confidently wrong answer in the one
panel whose entire purpose is explaining the pixel you are looking at, and it is
the same shape as the v2.3.6 defect differing only in degree: empty then,
off-by-one now.
It is NOT demonstrated. It comes from reading the lock structure, and this line
has already retracted one conclusion drawn from reading rather than measuring, so
it is written down as a hypothesis with the experiment that settles it: record
the frame counter at both acquisitions under `debug-hooks` and count redraws
where they differ. Non-zero confirms; zero over a long capture with a panel open
bounds it.
Deliberately not fixed here, for a reason that is specific rather than cautious.
The obvious fix — merge the two acquisitions — puts the composite work back under
the emulator lock, which is exactly the regression v2.3.0 fixed. The likelier
shape is snapshotting provenance at the first acquisition alongside the
framebuffer. Choosing between them before knowing whether the race actually fires,
or how often, is how the composite work ended up under the lock in the first
place.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ists Correcting my own design from earlier in this line, found by trying to write the code rather than by re-reading the plan. That design argued the percentile arithmetic was sound. `perf.rs` records that `work p95 = total p95 - wait p95` is invalid — differencing percentiles is not the percentile of the difference, and the first attempt at it published a table whose `work p95` sat BELOW its `work p50`. Adding, I argued, is different: internal lag is a constant, and adding a constant shifts every percentile by exactly that constant, so `lag_ms + work_p95` is a real p95. That is true. It is also not enough, and the gap is that NEITHER figure the design specifies is `lag + one series`. Avoidable latency needs `lag + render_work + render_lock`; wall-clock delay needs those plus `render_wait`. `PerfView` exposes all three as separate percentile series, so both figures require summing two or three p95s — which is the identical error `perf.rs` already retracted a table for, in the addition direction rather than the subtraction one. The constant-shift argument rescues exactly one series, and both figures need more than one. So the note this item was carried forward on — "the frontend pipeline cost `perf.rs` already tracks" — is wrong about "already". What is missing is a single per-redraw series: end-to-end pipeline latency recorded as ONE sample per redraw and percentiled as one distribution. That is an addition to `RenderPerf`, not a panel change, and it is the real first step rather than the last one. Recorded with a cheaper interim that does not lie: report `lag_ms + render_work_p95` alone, labelled as game delay plus render work, and state plainly that the vblank wait and lock contention are excluded. It answers less than the carried-forward note promised and it answers it correctly, which is the trade this line keeps choosing. Worth naming the pattern, because it is now three for three in this release: the sweep's second question came back clean, the first came back a hypothesis rather than a defect, and this item came back not-yet-buildable. None of those is the outcome the carried-forward notes anticipated, and each was cheap to establish and expensive to have assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Item C's honest subset. The panel reported "about N ms of the game's own delay" and stopped there; it now also reports game delay PLUS the renderer's own work, which is the part of the pipeline that can be added without lying about it. Three decisions, each visible in the code rather than only in the plan. WHAT IS EXCLUDED, AND WHY IT HAS TO BE. `render_wait` (the blocking present) and `render_lock` (mutex contention) are their own percentile series, and adding two p95s is not the p95 of the sum. That is the error `RenderPerf::work` already exists to prevent — it is kept as a real per-redraw series precisely because `total p95 - wait p95` once produced a published table whose `work p95` sat below its `work p50`. Summing has the same defect as differencing; only the direction changes. What makes THIS sum legitimate is that the lag term is a constant, not a distribution: adding a constant shifts every percentile by exactly that constant. That argument rescues exactly one series, so the label says "game delay + render work" and the panel states plainly what it leaves out. A true wall-clock figure needs a single per-redraw end-to-end series — an addition to `RenderPerf`, not arithmetic over what exists. RUN-AHEAD IS SUBTRACTED. The Oracle measures the game's internal lag, a property of the ROM; run-ahead removes up to `depth` frames of it. A figure ignoring the configured depth overstates what the user experiences, and overstates it worst exactly when they have taken this panel's own advice. Saturating rather than wrapping, so a depth above the measured lag leaves zero. IT DECLINES RATHER THAN GUESSING. Below sixty render samples there is no figure, only a statement of how many samples exist and how many are needed. A percentile over a handful of redraws is noise wearing a number's clothing, and printing it would be this panel doing the exact thing it was built to refuse — the same `None`-versus-`Some(0)` discipline that governs its main result. The arithmetic is lifted out of the `egui` closure into `end_to_end_figure` so it is testable rather than eyeballed, the same restructuring the Divergence Lens needed for its verdicts. Four tests, three mutations, each failing exactly the tests that pin it: dropping the run-ahead subtraction, removing the sample-count refusal, and dropping render work from the sum. `PerfPanelState::render_work` is the accessor, and it offers ONLY that series on purpose — exposing `render_wait` or `render_lock` beside it would invite exactly the sum this whole design refuses. 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>
…se finding `.github/actionlint.yaml` already existed on `main`, added in v2.2.3. It declared the self-hosted `agy` label with a fuller rationale than what replaced it — including why a typo'd label is worse than a failure (the job queues forever), how to run the tool over the whole tree, and why that workflow cannot use a hosted runner. I overwrote it. A `Write` to a path that was never read first, which is the standing read-before-write rule, and the file being unfamiliar to me is not the same thing as the file being absent. The consequence was not only a lost comment. The A5c commit recorded, as one of three findings, that "adding the config activated a dormant check" — the `agy` label reported unknown only after `actionlint.yaml` existed. That is false. Removing the declaration is what made actionlint report the label; the "dormant check" was self-inflicted breakage reported as a discovery. Left standing it would have taught the next reader something untrue about how linter configs behave. Restored to `main`'s content with the `paths:` suppression APPENDED. The diff against `main` is now purely additive, verified rather than asserted. The plan carries the retraction alongside the original claim rather than quietly deleting it, which is this project's convention for exactly this — the evidence that the prose was wrong is worth more than a tidy page. The real lesson is narrower and more useful than the false one: a config file for a tool the repo already uses is precisely the kind of file that looks new because YOU have not seen it. Adopting actionlint as a pre-commit HOOK implied nothing about whether its configuration already existed, and I did not check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Things a future session would otherwise re-derive, or get wrong the same way. A SKIPPED JOB SATISFIES `CI success`, which is `main`'s only required context. That job fails on `failure` or `cancelled` and `skipped` is in neither list, so a gate that did not run reports a pass for a property it never tested. `test-roms` was full-run only, which is why an accuracy regression could not be caught on the PR that caused it. There is no merge queue either — verified, not assumed — so nothing caught it later. When judging whether a gate covers something, check which contexts are required, what the aggregate treats as failure, and whether the job actually ran; a green required check answers none of those alone. EVERY SAME-TIMELINE RESTORE must hand-carry state living outside the save state. v2.3.6 wired the provenance stash into the one call site a report named; three more existed, so the Latency Oracle and RAM Atlas silently emptied both provenance panels. `snapshot_schema_audit.rs` enumerates the excluded set and is where to look — with the caveat that its reasons are free text and a keyword pass over them mis-sorted 17 of 26, so the durability question is not machine- checkable. Assert such state returns byte-identical, never merely "armed". `timeout-minutes` IS NOT VALID on a `uses:` job, and actionlint is how to settle that class of question when GitHub's docs do not. Recorded with the exact diagnostic, because a review asserted the opposite and the docs are silent. Also recorded: `.github/actionlint.yaml` has existed since v2.2.3 — extend it rather than writing over it, which is how a false finding reached a commit body. BOUND EVERY JOB AND EVERY FETCH INSIDE ONE. #400 bounded `ci.yml` alone; six more workflows were unbounded, found when one hung two hours and blocked the v2.3.7 release. A job timeout bounds damage but cannot notice — a stalled fetch inside a 25-minute budget looks like a slow job and reports as cancelled. SUMMING PERCENTILES is as invalid as differencing them. The subtraction case is already in `docs/performance.md`; the addition case broke a v2.3.9 design, and the one legitimate exception (adding a constant) is why exactly one series is exposed to the Latency Oracle. `grep -i` ON A SHORT TOKEN matches more than intended: `-i "RAM"` matches "f-ram-ebuffer", so reading the AccuracyCoin result that way returns the known-buggy framebuffer decoder's 120 instead of the authoritative 141. Two more false negatives the same session came from patterns that could not match at all. A pattern that cannot match looks exactly like content that is not there. Memory files updated alongside: two new entries, two existing ones extended rather than duplicated (read-before-write gains the `Write`-clobbers-silently failure mode; the percentiles entry gains the summing corollary). 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 |
Antigravity review (Gemini via Ultra)Adds precision and bounds to CI by scoping the Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Pull request overview
This PR opens the v2.3.9 “Crucible” line by tightening the project’s “what did we actually test?” guarantees: it makes the heavyweight accuracy battery run on PRs that touch accuracy-relevant paths, hardens CI provisioning against apt hangs, and adds workflow-schema linting to pre-commit. It also exposes the render-work perf series so the Latency Oracle can incorporate the pipeline’s own (percentile) cost without duplicating perf plumbing.
Changes:
- Condition
test-romsexecution on anaccuracypaths-filter (CPU/PPU/APU/mappers/core/gamedb/harness/tests) so PRs that can move vectors run the accuracy battery even when not a full CI run. - Add
.github/scripts/apt-install-retry.sh(bounded + retried apt provisioning) and use it inlibretro-crossfor bindgen sysroot provisioning. - Pin
actionlintas a pre-commit hook and extend.github/actionlint.yamlwith a narrowly-scoped suppression; exposePerfPanelState::render_workand plumb it into the Latency Oracle.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rustynes-frontend/src/debugger/perf_panel.rs | Exposes the render-work perf series via a small accessor for reuse by other panels. |
| crates/rustynes-frontend/src/debugger/mod.rs | Feeds the perf render-work snapshot into the Latency Oracle UI plumbing. |
| CHANGELOG.md | Documents the actionlint-based resolution of the reusable-workflow timeout-minutes schema question. |
| AGENTS.md | Adds standing operational/CI/testing rules learned from recent regressions and reviews. |
| .pre-commit-config.yaml | Adds a pinned actionlint hook so workflow-schema issues are caught locally. |
| .github/workflows/release-auto.yml | Expands the in-file rationale around reusable-workflow schema constraints (actionlint-verified). |
| .github/workflows/ci.yml | Adds accuracy paths-filter output and uses it to trigger test-roms; switches bindgen apt provisioning to the retry script. |
| .github/scripts/apt-install-retry.sh | New bounded+retried apt helper for CI reliability and clearer failure signaling. |
| .github/actionlint.yaml | Appends a targeted ignore for intentional constant if: false usage in pgo.yml. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Owner
Author
Two accepted, one declined on measurement — thanks, the first two were real1. 2. Elevation outermost, 3. Tested against
If the keys were regexes, So on 1.7.12 the suggested Worth flagging in case it is version-dependent: if actionlint gains regex semantics for these keys in a later release, the pin ( |
doublegate
added a commit
that referenced
this pull request
Aug 19, 2026
Review on #408 found two problems with `.github/scripts/apt-install-retry.sh`, and both would have made it worse than nothing on the exact path it exists to protect. ELEVATION MUST BE OUTERMOST, WITH `timeout` INSIDE IT. The original had `timeout` on the outside, which sends the signal to the elevation helper rather than to `apt-get`. The helper may not forward it, leaving `apt-get` orphaned while still holding the dpkg lock — so every subsequent retry fails on the lock rather than on the original problem. A retry loop that guarantees its own retries fail is worse than no retry loop. `DEBIAN_FRONTEND=noninteractive`, for the same class of reason. A package that prompts for configuration blocks on stdin that will never arrive in CI, burning the whole timeout budget waiting for a human who is not there — the exact failure this script bounds, arriving through a door it had left open. Passed explicitly because the environment is scrubbed on elevation. Re-verified with the same stubs: fail-fail-succeed still names the attempt it succeeded on and exits 0. shellcheck clean. Both are recorded in the plan beside the item rather than only here, because the lesson generalises past this script: wrapping a command for reliability puts the wrapper in the signal path, and the wrapper's own failure modes then belong to the thing it was protecting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Correction: the ordering fix did not land in this PR. My reply above said the two What happened, since the mechanism is worth recording: the compound shell command that was supposed to Both findings were correct and both fixes are real; they ride in #409 instead, unchanged. Nothing is lost — but this PR merged without them, and the record here said otherwise. Apologies for the noise; flagging it rather than letting the earlier comment stand. |
doublegate
added a commit
that referenced
this pull request
Aug 19, 2026
… missed (#409) * feat(perf): measure the two-acquisition race instead of reasoning about it v2.3.9 item B recorded a hypothesis: the `needs_nes` render arm — taken exactly when a debugger or tool panel is open — acquires the emulator lock TWICE per redraw. Once to copy the framebuffer the user will see, and again sixty lines later for `run_shell_ui`, where panels read `&mut Nes`. The guard is dropped between them so the composite work does not hold the emulator, which means the emulation thread can take the lock in that gap. If it does, the screen shows frame N while a panel describes N+1 — a confidently wrong answer in Pixel Provenance, whose whole purpose is explaining the pixel you are looking at. That was written down as a hypothesis rather than a defect, with the experiment attached, because this line has already retracted one conclusion drawn from reading rather than measuring. This is the experiment. `Nes::cycle()` is read at both acquisitions and the readings compared. The choice of quantity matters: it is cumulative and monotonic, and `produce_one_frame` holds the lock across a WHOLE frame, so any difference at all means at least one complete frame landed in the gap — there is no partial-frame reading to misinterpret. No frame counter exists on `Nes`, and adding one would have been a second source of truth for something the cycle counter already answers. Both counters are kept, not just the hits. "The race did not fire" and "nothing was observed" both read as zero hits, and only the denominator separates them — the same distinction this release keeps insisting on, applied to its own instrument. The denominator counts redraws where the race COULD have fired: both readings present, meaning a ROM is loaded and the arm ran twice. Counting ROM-less redraws would dilute the rate toward zero and manufacture the reassuring answer. Surfaced in the Performance panel as a rate with the counts beside it, and the hover text says what a zero does and does not mean: it BOUNDS the effect over that capture, it is not proof the race cannot happen. A measurement that reads as a verdict is how the next person stops looking. `debug-hooks`-gated throughout, so the shipped default carries neither the two reads nor the counters. Two tests pin the counter's semantics — that an unobserved redraw is not a clean one, and that hits and observations move independently. What this does NOT do is fix anything. The rate has to be observed on a real session with a panel open before a fix is chosen, because the obvious fix — merging the two acquisitions — puts the composite work back under the emulator lock, which is exactly the regression v2.3.0 fixed. Verified: fmt, workspace clippy with the feature OFF, `debug-hooks` and `full` frontend combos, both wasm32 invocations, rustdoc, and the frontend suite at 522. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ci): two ordering defects in the apt retry, both caught in review Review on #408 found two problems with `.github/scripts/apt-install-retry.sh`, and both would have made it worse than nothing on the exact path it exists to protect. ELEVATION MUST BE OUTERMOST, WITH `timeout` INSIDE IT. The original had `timeout` on the outside, which sends the signal to the elevation helper rather than to `apt-get`. The helper may not forward it, leaving `apt-get` orphaned while still holding the dpkg lock — so every subsequent retry fails on the lock rather than on the original problem. A retry loop that guarantees its own retries fail is worse than no retry loop. `DEBIAN_FRONTEND=noninteractive`, for the same class of reason. A package that prompts for configuration blocks on stdin that will never arrive in CI, burning the whole timeout budget waiting for a human who is not there — the exact failure this script bounds, arriving through a door it had left open. Passed explicitly because the environment is scrubbed on elevation. Re-verified with the same stubs: fail-fail-succeed still names the attempt it succeeded on and exits 0. shellcheck clean. Both are recorded in the plan beside the item rather than only here, because the lesson generalises past this script: wrapping a command for reliability puts the wrapper in the signal path, and the wrapper's own failure modes then belong to the thing it was protecting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(perf): reset the lock-gap counters on clear, plus three review corrections Four findings from the #409 review, one of them a real defect in the instrument this PR exists to add. `RenderPerf::clear()` did not reset the new `lock_gap_*` counters. It is documented as a regime-change reset, so a ROM change or pacing-regime change would have cleared every sample ring and left the numerator and denominator of the skew rate standing — mixing two populations into one percentage and presenting it as a single measurement. That is exactly the defect the `wait` series had before it, and the comment explaining that fix sits four lines above the place the new counters were missing from. Adding to `stats()` and forgetting `clear()` is evidently the shape of this mistake; the test now pins it and the mutation fails without the reset. The `cycle_at_fb` comment said the reading is taken "at the moment the framebuffer is copied". It is taken on ACQUIRING the lock, a few lines earlier. Equivalent — the emulator cannot advance while the guard is held, so every reading inside that scope names the same frame — but "close enough to be misleading" is how prose stops being checked, and a reviewer asking means the next reader would have. The comment now says where it is read and why that is the same thing. `DEBIAN_FRONTEND` is now set with `env` rather than as a bare assignment to the elevation helper. Both work on a standard runner, but the bare form additionally requires SETENV in sudoers, and on a stricter host it fails by refusing to run at all — breaking the wrapper rather than degrading it. Raised independently by both reviewers, which is usually a sign the point is real. And the counters use plain `+= 1`: one increment per redraw cannot overflow a `u64` in any run that terminates, so `saturating_add` implied a bound worth reasoning about where there is none. Verified: shellcheck, the stub replay (fail-fail-succeed still names its attempt and exits 0), fmt, workspace clippy, frontend under `debug-hooks` at 523 tests. 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.
Opens v2.3.9 "Crucible" — testing, correctness, and what the gates actually cover.
A crucible is where something is tested to destruction rather than inspected. This is not a feature release: the v2.3.x line added five tools in four releases, and the recurring finding across all of them was never that the emulation was wrong — it was that a check reported a pass it had not earned.
The finding that frames the release
main's ruleset requires exactly one status context,CI success. That job fails onfailureorcancelled— andskippedis in neither list.test-roms(the accuracy battery) was FULL-run only, so on a regular feature PR it was skipped. The only gate on merging was therefore satisfied without the accuracy suites having run at all. #396 (a PPU fix that legitimately movedvisual_regression__scanline_frame_180) and #403 (the vector update) are the worked example: both PRs were correct, the process was not. And there is no merge queue — verified, nomerge_groupevent appears in the run history — so nothing caught it later either.What lands
test-romsruns on PRs that touch paths able to move a vector (chip crates, core, gamedb, harness,tests/).github/scripts/apt-install-retry.sh— per-commandtimeout+ 3 attemptsactionlintas a pinned pre-commit hookto-dos/plans/v2.3.9-crucible-plan.md, six standing rules inAGENTS.mdA5's cost is measured, not assumed: over the last 40 merged PRs, 11 touch those paths and 29 do not — so ~72% still skip the 29-minute battery and the saving the full-run flag exists for is preserved.
rustynes-gamedbis included for a non-obvious reason: it rewrites the iNES header on load, so it changes what the emulator is before a cycle runs.A5b exists because apt provisioning hung four times across two PRs in one day, always in a setup step and never in a compile or test step, once blocking the v2.3.7 release PR for two hours. A job timeout bounds the damage but cannot notice — a stalled fetch inside a 25-minute budget is indistinguishable from a slow job. Verified by stubbing
sudo/timeout/sleep: all-fail gives three warnings and exit 1; fail-fail-succeed names the attempt it succeeded on.Three results that were not what the carried-forward notes expected
snapshot_schema_audit.rsalready enumerates every non-serialized field; cross-referencing its 26 output-only entries, onlywrite_attribandreg_attribare cumulative, and both are carried. The method's limitation is the finding: the reasons are free text, and a keyword pass mis-sorted 17 of 26.needs_nesrender arm takes two lock acquisitions per redraw (framebuffer copy, then the egui pass where panels read), so the screen may show frameNwhile the panel describesN+1. Not demonstrated — the experiment is written down instead, because the obvious fix puts composite work back under the emulator lock, which is exactly the regression v2.3.0 fixed.lag + render_work.p95, valid only because the lag term is a constant, labelled as such, declining below 60 samples, with run-ahead subtracted.A correction carried in the branch
.github/actionlint.yamlalready existed since v2.2.3. I overwrote it withWriteon a path I had never read, and then recorded the resulting breakage as a finding ("adding the config activated a dormant check") — which was false: removing theagydeclaration is what made the label unknown. Restored tomain's content with the suppression appended; the diff is purely additive, checked withgit diff origin/main. The plan carries the retraction beside the original claim.Verification
fmt,
actionlintclean tree-wide, workspace clippy, frontend underdebug-hooks(520 tests), both wasm32 invocations, rustdoc, no_std thumbv7em.No emulation-core file is touched, so AccuracyCoin and nestest are unaffected by construction — and this PR touches no accuracy path, so
test-romsshould be skipped here. That is A5's negative demonstration: if it runs, the filter is too broad.