chore(release): cut v2.3.7 "Overtone" - #406
Merged
Merged
Conversation
Workspace version 2.3.6 -> 2.3.7, the CHANGELOG `[Unreleased]` section promoted to `[2.3.7] - 2026-08-19 - "Overtone"`, a maintainer-authored release-notes override at `.github/release-notes/v2.3.7.md`, and the README badge, README "Current Release" blurb, `docs/STATUS.md` header, and `AGENTS.md` release anchors brought forward. An overtone is the structure inside a sound that a single pitch reading throws away, which is the gap Audio Provenance closes: the Audio Scope already plotted the waveform and the Audio Mixer already set the gains, and nothing linked a sample back to the instruction that caused it. The release's subject is not the feature but the trap it inherited, and the release notes lead with that rather than burying it. Pixel Provenance shipped non-functional for four releases; Audio Provenance rides the identical rollback, so the carry landed in the same change as the feature. The enumeration behind that carry then turned out to be incomplete in three more places, which meant running the Latency Oracle or the RAM Atlas silently emptied both panels. `crates/rustynes-libretro/rustynes_libretro.info` is bumped in the same commit rather than as a follow-up, because `libretro_info_audit.rs` pins `display_version` against `[workspace.package]` and fails the suite the moment the two disagree. That gate is deliberate: it is what makes the upstream sync a copy rather than a re-derivation. Per the maintainer decision recorded in `docs/libretro/UPSTREAM_SYNC.md`, a version bump alone is NOT an upstream-sync trigger — the next sync is v2.4.0 — so the upstream `dist/info` copy continues to read v2.3.5 deliberately. A licence change would override that; a version does not. `cargo check --workspace` clean at the new version and the libretro audit green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…king this cut PR #400 gave every job in `ci.yml` an explicit `timeout-minutes`, because without one a job inherits GitHub's six-hour default and a hang blocks a release while reporting nothing at all. It covered `ci.yml` and nothing else, and both the CHANGELOG headline and the release notes then said "every job is now bounded", which was not true. The gap was found the same way the original defect was: by being blocked. During this release's cut, `Clippy Security Lints` hung for over two hours in a setup step — on a job whose observed runtime is two to three minutes — holding the probe-fix PR and, behind it, the release. `security.yml` had no timeout on any of its three jobs, and a sweep of `.github/workflows/` found five more unbounded workflows: `android.yml`, `ios.yml`, `web.yml`, `antigravity-review.yml`, and `release-auto.yml`, the release workflow itself. Budgets are generous multiples of measured runtime rather than guesses — audit and deny complete in under a minute, the security clippy in 2-3, the Android jobs in 10-17, iOS in 11-19, the Pages build in 6-10 — because the purpose is to bound a hang, not to police normal variance on a cold cache. The sweep across every workflow file now comes back empty. Two details that are easy to get wrong and are recorded at the sites: `release-auto.yml`'s `build` job cannot carry a timeout at all — `timeout-minutes` is not valid on a job that uses `uses:` — so only `prepare` is bounded here and the rest of that budget lives on the jobs inside `release.yml`, which already had them. Adding one to `build` would have been a silent no-op at best. `antigravity-review.yml` is bounded harder than the hosted jobs rather than softer. It runs on the maintainer's own self-hosted hardware, so a hung run holds a real machine rather than a disposable VM, which inverts the usual argument for a generous ceiling. The CHANGELOG entry and the release notes are corrected rather than quietly widened: both now say what PR #400 actually covered, that the claim was too strong, and how the remainder was found. A release note asserting a property the tree does not have is the failure this project keeps meeting, and it would have been especially poor in the entry about unbounded jobs. 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)This trivial PR cuts the v2.3.7 "Overtone" release by bumping versions across the workspace, updating documentation and release narratives, and adding explicit Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Pull request overview
Cuts the v2.3.7 “Overtone” release by bumping workspace versioning/anchors, adding the maintainer release-notes override, and closing the remaining gap from PR #400 by adding explicit GitHub Actions job timeouts to the remaining unbounded workflows.
Changes:
- Version bump 2.3.6 → 2.3.7 across workspace metadata (Cargo.toml/Cargo.lock), libretro
display_version, and README/docs release anchors. - Adds maintainer-authored release notes at
.github/release-notes/v2.3.7.mdand moves the release narrative intoCHANGELOG.md. - Adds
timeout-minutesto previously unbounded workflows (security.yml,web.yml,android.yml,ios.yml,antigravity-review.yml,release-auto.yml) to prevent hangs from blocking releases.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates the release badge and “Current Release” section to v2.3.7 “Overtone”. |
| docs/STATUS.md | Advances the status header to v2.3.7 and updates the release summary block. |
| crates/rustynes-libretro/rustynes_libretro.info | Bumps libretro display_version to match the workspace version. |
| CHANGELOG.md | Promotes the v2.3.7 entry out of Unreleased with the release narrative and highlights. |
| Cargo.toml | Bumps [workspace.package] version to 2.3.7. |
| Cargo.lock | Propagates the workspace version bump across crate entries. |
| .github/workflows/web.yml | Adds job timeouts for Pages build/deploy. |
| .github/workflows/security.yml | Adds job timeouts and documents the motivation/incident. |
| .github/workflows/release-auto.yml | Adds a timeout to the prepare job (reusable-workflow job remains delegated). |
| .github/workflows/ios.yml | Adds a timeout to the TestFlight job. |
| .github/workflows/antigravity-review.yml | Adds a timeout for the self-hosted Antigravity review job. |
| .github/workflows/android.yml | Adds timeouts for Android cross-build and Gradle packaging jobs. |
| .github/release-notes/v2.3.7.md | Adds the maintainer override release-notes body for the automated release publisher. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Owner
Author
Declined — the limitation still applies, and I checked rather than assertedThe claim is that
So it is still unsupported, and the comment understates the consequence rather than overstating it: I wrote "a silent no-op at best", and it is in fact a hard syntax error. I will tighten that wording on the v2.3.9 branch rather than restart a green release PR for a comment — same disposition as the assertion-strengthening on #405, and recorded here so it is tracked rather than dropped. The substantive point behind the comment stands either way: the Two things this turned up that are worth more than the finding
Running it across the repo returns two findings, both false positives against intent: the |
doublegate
added a commit
that referenced
this pull request
Aug 19, 2026
…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>
doublegate
added a commit
that referenced
this pull request
Aug 19, 2026
…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>
doublegate
added a commit
that referenced
this pull request
Aug 19, 2026
* docs(plans): open v2.3.9 "Crucible", with item A5 designed and its premise 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> * ci(v2.3.9 A5): run the accuracy battery on PRs that can actually break 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> * docs(plans): v2.3.9 item B — the sweep is clean, and the method is the 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> * docs(plans): v2.3.9 item C — the end-to-end figure hides three decisions 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> * ci(v2.3.9 A5b): bound the apt provisioning steps, which hung four times 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> * ci(v2.3.9 A5c): lint workflows against the schema, and back a claim review 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> * docs(plans): v2.3.9 item B question 1 — a candidate, deliberately not 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> * docs(plans): v2.3.9 item C is not implementable from the data that exists 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> * feat(frontend): the Latency Oracle's interim end-to-end figure 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> * fix(ci): restore the actionlint config I overwrote, and retract a false 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> * docs(agents): record six standing rules learned across v2.3.7-v2.3.9 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> --------- 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.
Cuts v2.3.7 "Overtone" — the audio-provenance release.
An overtone is the structure inside a sound that a single pitch reading throws away, which is the gap this closes: the Audio Scope already plotted the waveform and the Audio Mixer already set the gains, and nothing linked a sample back to the instruction that caused it.
What this PR contains
2.3.6→2.3.7[Unreleased]→[2.3.7] - 2026-08-19 - "Overtone".github/release-notes/v2.3.7.md(maintainer override, ~1,200 words)docs/STATUS.mdheader, bothAGENTS.mdrelease anchorsrustynes_libretro.infodisplay_version→v2.3.7timeout-minuteson the six workflows PR #400 missedThe libretro bump is in the same commit deliberately
libretro_info_audit.rspinsdisplay_versionagainst[workspace.package]and fails the suite the moment the two disagree. That gate is what makes the upstream sync a copy rather than a re-derivation.Per the decision recorded in
docs/libretro/UPSTREAM_SYNC.md, a version bump alone is not an upstream-sync trigger — the next sync is v2.4.0 — so the upstreamdist/infocopy continues to readv2.3.5deliberately, not by oversight. A licence change would override that; a version does not.The CI timeout sweep, and a claim I had to correct
PR #400 bounded every job in
ci.ymlafter a hung job silently skipped the v2.3.6 release. Both the CHANGELOG headline and my first draft of these release notes then said "every job is now bounded". That was not true, and the gap was found the way the original defect was — by blocking this release.Clippy Security Lintshung for over two hours on #405, in a setup step, on a job whose observed runtime is two to three minutes.security.ymlhad notimeout-minuteson any of its three jobs, and a sweep found five more unbounded workflows:android.yml,ios.yml,web.yml,antigravity-review.yml, andrelease-auto.yml— the release workflow itself.All six are bounded now, with budgets set from measured runtimes rather than guesses. The sweep across
.github/workflows/comes back empty.Two details recorded at the sites because they are easy to get wrong:
release-auto.yml'sbuildjob cannot carry a timeout at all —timeout-minutesis not valid on a job that usesuses:. Adding one would have been a silent no-op; its budget lives on the jobs insiderelease.yml, which already had them.antigravity-review.ymlis bounded harder than the hosted jobs, not softer, because it runs on the maintainer's own hardware, where a hung run holds a real machine rather than a disposable VM.The CHANGELOG entry and the release notes are corrected rather than quietly widened: both now say what #400 actually covered, that the claim was too strong, and how the remainder was found. A release note asserting a property the tree does not have would have been a poor thing to ship in the entry about unbounded jobs.
Accuracy
rustynes-apuandrustynes-coreboth change in this release, so the contract is verified, not asserted:nestest_pc_c000_matches_golden_loggreenAfter merge
release-auto.ymltags and publishes off final-greenmain, using this branch's.github/release-notes/v2.3.7.mdas the body. Do not tag by hand — a manual tag races the automation.