From da7de72024e6eb81298d14047dccc0574cf57077 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 01:53:36 -0400 Subject: [PATCH 1/6] test(harness): pin the release anchors mechanically, and sweep the deferred backlog Two pieces of owed work from the v2.3.9 cut, both of the same kind: a fact this project states in many places, kept in step by nothing but attention. RELEASE-ANCHOR AUDIT `crates/rustynes-test-harness/tests/release_anchor_audit.rs` pins 15 anchors across 10 documents against `[workspace.package] version` -- the README badge and Current Release section, `docs/STATUS.md`, both `AGENTS.md` anchors plus its "never claim a later version" guard, `VERSION-PLAN.md` (header AND the `(current)` row of its release table), `to-dos/ROADMAP.md`, `SUPPORT.md`, `SECURITY.md`, the root `ROADMAP.md`, `OVERVIEW.md`, and `ARCHITECTURE.md`. It is modelled on `libretro_info_audit.rs` deliberately and for the same reason. That audit exists because the libretro `.info` `display_version` drifted from the workspace and told every RetroArch user the wrong LICENCE for eleven days; nothing in the repo compared the two, so nothing could catch it. The prose surfaces had exactly that shape of hole, and at v2.3.9 they were holding SIX different values, the oldest four releases stale. The manifest is the single source of truth; every other statement of the fact is now COMPARED against it rather than maintained beside it. Three assertions beyond the version itself, each for a failure already observed: * The CHANGELOG must carry a section for the workspace version, with a parseable `- - ""` tail. `release-auto.yml` reads that exact line TWICE -- once for the release-body fallback when no `.github/release-notes/vX.Y.Z.md` override exists, and once to derive the release title's codename -- and a malformed header broke the v2.1.7 auto-release outright. Failing in CI beats failing at publish time. * Any anchor that quotes a codename must quote the CHANGELOG's. A correct version beside the PREVIOUS release's codename is the more confusing error: the number looks right, so the sentence around it is trusted. * `VERSION-PLAN.md`'s table must mark exactly one row `(current)`. Its drift is different in kind from a header's -- at v2.3.9 the header said v2.3.6 while the table stopped at v2.3.5 and still marked THAT row current, so the marker was three releases behind the header that was itself three releases behind the tree. The load-bearing property is that it FAILS CLOSED. `versions_at` panics when a marker matches nothing, with a message asking to be taught the new wording. An audit that finds zero anchors and reports success is indistinguishable from one that found them all correct, and that indistinguishability IS the defect class v2.3.9 was about. Proven by mutation rather than asserted, per this project's standing rule that a passing test proves nothing until it has been shown to fail. Five independent mutations, each caught by the test it should be caught by and only that one: version drift (README badge v2.3.9 -> v2.3.7) ....... FAILED, correct codename drift (SUPPORT.md "Crucible" -> "Overtone") FAILED, correct anchor reworded away (STATUS.md marker changed) ..... FAILED, correct (closed) `(current)` marker removed from VERSION-PLAN ........ FAILED, correct CHANGELOG section renamed 2.3.9 -> 2.3.99 ........... FAILED, correct Each mutation restored the file byte-for-byte and `touch`ed it afterwards -- a `shutil.move`-style restore preserves the pre-mutation mtime, which leaves cargo reusing a binary built from mutated source. That failed loudly once before; a stale binary that PASSES is silent. The two small manifest-parsing helpers are duplicated from the libretro audit rather than shared. Cargo compiles each `tests/.rs` as its own crate, and the alternative -- `tests/common/mod.rs` -- pulls in a 46 KB framebuffer/ROM harness this audit has no use for. What is duplicated is a PARSER; the fact still has exactly one home. What it deliberately does not check: whether the prose around the version is TRUE. No test can tell whether a codename fits or whether a scheduler description matches the scheduler. It removes the mechanical claims from the human review surface so the ones needing judgement are what remains. DEFERRED BACKLOG SWEEP (v2.3.5 -> v2.3.9) `to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md` was last reconciled entry-by-entry at v2.3.4. The previous commit in this line DATED it rather than re-dating it, and left the sweep as real work; this is that work. Eleven entries struck, each carrying its evidence inline -- a file that exists, a workflow line number, a test that says so -- rather than a bare tick, so a closure can be disagreed with. Most of what the sweep found was stale by far more than the five releases it was scoped to. The WHOLE of section 6a -- the four items A1-A4 that define the timebase rewrite -- shipped in v2.0.0 "Timebase" on 2026-07-03, six weeks and roughly twenty releases before this sweep, and section 6's preamble still described AccuracyCoin as "100% / 139/139" when it has been an exact 141/141 since v2.0.3. A backlog that lists the project's designated MAJOR release as pending is not merely untidy; it is actively misleading about what the emulator IS. Three entries were closed by something other than what they proposed, and are struck with that distinction stated rather than quietly ticked: * `cargo-hack` feature clippy -- the tool was never adopted; the combos are ENUMERATED in `ci.yml` instead (eight invocations, including the three wasm32 ones). The coverage gap is closed and a real residual remains: a NEW feature is uncovered until someone adds a line. Recorded rather than glossed. * `merge_group` + PR-Ubuntu-only matrix -- the matrix half shipped (`ci.yml:246-252` picks a two-OS set for a regular PR and the full four-OS set otherwise, with `release/*` head branches deliberately taking FULL, which is why a release PR still runs macOS and Windows). Narrowed to the merge queue alone, which genuinely does not exist in any workflow. * R3 (`apu_reset/len_ctrs_enabled`) -- closed in v2.0.0 beta.3, but as a HARNESS ARTIFACT, not an emulation residual, so A4's cycle-accurate reset is not what fixed it. `tests/apu_reset.rs:107` says so verbatim. A residual that dissolves under a corrected measurement is a different outcome from one a refactor closed, and conflating them inflates what the refactor is credited with. Also struck with evidence: BasicBot (`basic_bot_panel.rs`, v1.8.9-beta.4), multi-monitor detachable tool windows (`detached.rs`, v2.3.0 -- whose own preamble records that v2.2.9's affordance merely EMBEDDED the panel), the free arm64 CI leg (`ubuntu-24.04-arm` in both matrix arms), `dorny/paths-filter` per-job skips (closed by v2.3.9: the `CI success` aggregator the entry names as its prerequisite is at `ci.yml:712`, and the second filter step gates `test-roms` at review time), and the two AccuracyCoin PPU tests "ALE + Read" / "Hybrid Addresses" (closed in v2.0.3 by the 2-cycle-ALE promotion -- the first of the two routes that entry itself named). Section 7's mapper entries below the v2.3.4 line are EXPLICITLY NOT swept, and the header says so. They need a ROM corpus to adjudicate, and asserting them from the source alone would be precisely the over-claim this catalogue exists to avoid. GATES cargo fmt --all --check ......................... clean (exit 0) cargo clippy --workspace --all-targets -D warn .. clean (exit 0) release_anchor_audit ............................ 4 passed libretro_info_audit ............................. 3 passed (unaffected) pre-commit markdownlint on changed files ........ passed No emulation source changes, so the AccuracyCoin 141/141 and nestest 0-diff results verified for v2.3.9 stand. Exit codes were checked directly rather than through a pipe -- `cmd | head && echo clean` prints "clean" regardless, which is the same shape of false pass this whole change is about. --- CHANGELOG.md | 65 +++ .../tests/release_anchor_audit.rs | 439 ++++++++++++++++++ to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md | 149 ++++-- 3 files changed, 621 insertions(+), 32 deletions(-) create mode 100644 crates/rustynes-test-harness/tests/release_anchor_audit.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index f4edcec5..68f11259 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,71 @@ cycle-accurate core later replaced. ## [Unreleased] +### Added + +- **A standing release-anchor audit — the drift v2.3.9 corrected by hand cannot + recur silently.** `crates/rustynes-test-harness/tests/release_anchor_audit.rs` + pins **15 anchors across 10 documents** against `[workspace.package] version`: + the README badge and Current Release section, `docs/STATUS.md`, both `AGENTS.md` + anchors plus its "never claim a later version" guard, `VERSION-PLAN.md` (header + *and* the `(current)` row of its release table), `to-dos/ROADMAP.md`, + `SUPPORT.md`, `SECURITY.md`, the root `ROADMAP.md`, `OVERVIEW.md` and + `ARCHITECTURE.md`. + + Modelled deliberately on `libretro_info_audit.rs`, which exists because the + libretro `.info` `display_version` drifted from the workspace and advertised the + wrong licence for eleven days. Same failure, same shape of fix: the manifest is + the single source of truth and every other statement of the fact is *compared* + against it rather than maintained beside it. At v2.3.9 those anchors held **six + different values**, the oldest four releases stale. + + Three assertions beyond the version itself. The CHANGELOG must carry a section + for the workspace version with a parseable `- - ""` tail, + because `release-auto.yml` reads that exact line twice — for the body fallback + and for the release title — and it has broken a release before. Any anchor that + quotes a codename must quote the CHANGELOG's, since a right version beside the + previous release's codename is the more confusing error: the number looks + correct, so the sentence around it gets trusted. And `VERSION-PLAN.md`'s table + must mark exactly one row `(current)` — at v2.3.9 it marked v2.3.5, three + releases behind its own header. + + **It fails closed.** A marker that matches nothing is a failure, never a pass; + an audit that finds zero anchors and reports success is indistinguishable from + one that found them all correct, which is the defect class v2.3.9 was about. + Proven by mutation rather than asserted: five independent mutations — a drifted + badge version, a stale codename, an anchor reworded out of existence, a moved + `(current)` marker, and a renamed CHANGELOG section — each fail the test they + should and only that test. + +### Changed + +- **`to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md` swept entry by entry**, against + `main` @ `fdfb2c04`. Eleven entries struck, each carrying its evidence inline — + a file that exists, a workflow line number, a test that says so — rather than a + bare tick, so a closure can be disagreed with. + + Most of what the sweep found was stale by far more than the five releases it was + scoped to. **The whole of §6a — the four items (A1-A4) defining the timebase + rewrite — shipped in v2.0.0 "Timebase" on 2026-07-03**, six weeks and roughly + twenty releases earlier, and §6's preamble still described AccuracyCoin as + "100% / 139/139" when it has been an exact **141/141** since v2.0.3. A backlog + listing the project's designated MAJOR release as pending is not untidy; it is + misleading about what the emulator is. + + Three entries were closed by something **other than what they proposed**, and + say so rather than being quietly ticked: the feature-combo clippy gap is closed + by *enumerating* the combos in CI (eight invocations, including the wasm32 ones) + rather than by adopting `cargo-hack`, which leaves a real residual — a new + feature is uncovered until someone adds a line; `merge_group` stays open but its + companion clause shipped, so the entry is **narrowed** to the merge queue alone; + and R3 turned out to be a **harness artifact** rather than an emulation + residual, so A4 is not what fixed it, and conflating the two would inflate what + the refactor is credited with. + + §7's mapper entries below the v2.3.4 line are **explicitly not swept** — they + need a ROM corpus to adjudicate, and asserting them from source alone would be + the over-claim this catalogue exists to avoid. + ## [2.3.9] - 2026-08-20 - "Crucible" (what the gates actually cover) A crucible is where something is tested to destruction rather than inspected, and diff --git a/crates/rustynes-test-harness/tests/release_anchor_audit.rs b/crates/rustynes-test-harness/tests/release_anchor_audit.rs new file mode 100644 index 00000000..20541e06 --- /dev/null +++ b/crates/rustynes-test-harness/tests/release_anchor_audit.rs @@ -0,0 +1,439 @@ +//! Standing audit — every document that names the current release must name the +//! release this tree actually is. +//! +//! **Why this exists.** On 2026-08-20, cutting v2.3.9, the current-release +//! version was found written down in **eight** documents with nothing asserting +//! they agree. They had drifted to **six different values**: +//! +//! | document | said | actual | +//! |---|---|---| +//! | `README.md` badge + Current Release | v2.3.7 | v2.3.9 | +//! | `docs/STATUS.md` | v2.3.7 | | +//! | `AGENTS.md` (both anchors) | v2.3.7 | | +//! | `VERSION-PLAN.md` | v2.3.6 (table stopped at v2.3.5, still `(current)`) | | +//! | `to-dos/ROADMAP.md` | v2.3.3 **and** v2.2.5, in two places | | +//! | `SUPPORT.md` | v2.3.0 | | +//! | `ROADMAP.md` (root) | v2.0.4 | | +//! +//! Drift is the default outcome when one fact lives in eight places and a human +//! is the only thing keeping them in step. Two of those documents were +//! additionally wrong about more than the number — `SECURITY.md` was still +//! offering support for `1.0.x`, and the root `ARCHITECTURE.md` was presenting +//! the dot-lockstep scheduler retired in v2.0.0 as the current design — but only +//! the version half is mechanically checkable, and that is what this file checks. +//! +//! **The precedent is deliberate.** `libretro_info_audit.rs` exists because the +//! libretro `.info` `display_version` drifted from the workspace and shipped a +//! wrong licence to users for eleven days. This is the same audit applied to the +//! prose surfaces, for the same reason, in the same shape: the workspace manifest +//! is the single source of truth, and every other statement of the fact is +//! compared against it rather than maintained beside it. +//! +//! **It fails closed.** A marker that matches nothing is a FAILURE, never a pass. +//! That is the load-bearing property: an audit that quietly finds zero anchors +//! and reports success is indistinguishable from one that found them all correct, +//! which is precisely the class of defect the v2.3.9 release was about. If an +//! anchor is reworded or moved, this test fails and asks to be taught the new +//! wording — it does not shrug. +//! +//! **What it deliberately does not check.** Whether the prose around the version +//! is *true* — that the codename fits, that the summary describes the release, or +//! that a scheduler description matches the scheduler. No test can do that. This +//! narrows the human review surface to the claims that need judgement by taking +//! the mechanical ones away from it. +//! +//! The small manifest-parsing helpers are duplicated from +//! `libretro_info_audit.rs` rather than shared. Cargo compiles each +//! `tests/.rs` as its own crate, and the alternative — `tests/common/mod.rs` +//! — pulls in the 46 KB framebuffer/ROM harness this audit has no use for. The +//! duplicated thing is a *parser*; the fact itself still has exactly one home, in +//! `[workspace.package]`. + +use std::path::{Path, PathBuf}; + +/// Workspace root, derived from this crate's manifest dir rather than the CWD +/// (which differs between `cargo test` and a direct binary invocation). +fn workspace_root() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .ancestors() + .nth(2) + .expect("crates// is two levels below the workspace root") + .to_path_buf() +} + +fn read(rel: &str) -> String { + let p = workspace_root().join(rel); + std::fs::read_to_string(&p).unwrap_or_else(|e| panic!("read {}: {e}", p.display())) +} + +/// Pull a `key = "value"` out of the workspace `[workspace.package]` table. +/// +/// Table-scoped rather than a first-match scan, for the reason recorded in +/// `libretro_info_audit.rs`: the manifest carries `version` keys in more than one +/// place, and the first one is not necessarily the one that governs the crates. +/// The CI toolchain resolver was bitten by exactly this shape when it matched the +/// first `channel` key anywhere in `rust-toolchain.toml`. +fn workspace_package_field(key: &str) -> String { + let manifest = read("Cargo.toml"); + let mut in_table = false; + for line in manifest.lines() { + let t = line.trim(); + if t.starts_with('[') { + in_table = t == "[workspace.package]"; + continue; + } + if !in_table { + continue; + } + if let Some((k, v)) = t.split_once('=') + && k.trim() == key + { + return v.trim().trim_matches('"').to_owned(); + } + } + panic!("`[workspace.package]` has no `{key}` key"); +} + +/// One place a document states the current release version. +/// +/// `marker` is the literal text immediately preceding the version, so the audit +/// pins the *claim* rather than merely the presence of a version string +/// somewhere in the file. Every one of these documents also carries a historical +/// trail naming older releases; a looser match would either fire on those or, +/// worse, be satisfied by them. +struct Anchor { + path: &'static str, + /// What the anchor is, phrased for the failure message. + what: &'static str, + marker: &'static str, +} + +/// Every document that states which release this tree is. +/// +/// Adding a new such statement without adding it here is the failure mode this +/// audit exists to prevent, so the list is the deliverable, not the plumbing. +const ANCHORS: &[Anchor] = &[ + Anchor { + path: "README.md", + what: "the version badge", + marker: "badge/version-v", + }, + Anchor { + path: "README.md", + what: "the Current Release section", + marker: "RustyNES's current release is **v", + }, + Anchor { + path: "docs/STATUS.md", + what: "the status-matrix header (the single source of truth for current state)", + marker: "> **Current release: v", + }, + Anchor { + path: "AGENTS.md", + what: "the \"What this is\" current-release block", + marker: "**Current release: v", + }, + Anchor { + path: "AGENTS.md", + what: "the operating-notes current-release bullet", + marker: "The current release is **v", + }, + Anchor { + path: "AGENTS.md", + what: "the \"never claim a later version\" guard", + marker: "**Never claim any version *later* than v", + }, + Anchor { + path: "VERSION-PLAN.md", + what: "the plan header", + marker: "**Current release: v", + }, + Anchor { + path: "to-dos/ROADMAP.md", + what: "the Status section", + marker: "- **Current release:** **RustyNES v", + }, + Anchor { + path: "SUPPORT.md", + what: "the \"can I use RustyNES now\" answer", + marker: "the current release is **v", + }, + Anchor { + path: "SECURITY.md", + what: "the supported-versions preamble", + marker: "The current release is **v", + }, + Anchor { + path: "ROADMAP.md", + what: "the project-status line", + marker: "**Project Status:** v", + }, + Anchor { + path: "ROADMAP.md", + what: "the current-release paragraph", + marker: "The current release is **v", + }, + Anchor { + path: "OVERVIEW.md", + what: "the document's Applies-to header", + marker: "**Applies to:** RustyNES v", + }, + Anchor { + path: "OVERVIEW.md", + what: "the current-release statements", + marker: "The current release is **v", + }, + Anchor { + path: "ARCHITECTURE.md", + what: "the document's Applies-to header", + marker: "**Applies to:** RustyNES v", + }, +]; + +/// Read a `MAJOR.MINOR.PATCH` starting at `s[0]`, stopping at the first +/// character that cannot be part of one. +/// +/// Returns `None` rather than a partial parse, so a marker followed by something +/// that is not a version fails the audit instead of silently comparing garbage. +fn parse_version_prefix(s: &str) -> Option { + let end = s + .find(|c: char| !c.is_ascii_digit() && c != '.') + .unwrap_or(s.len()); + let v = &s[..end]; + let parts: Vec<&str> = v.split('.').collect(); + (parts.len() == 3 + && parts + .iter() + .all(|p| !p.is_empty() && p.chars().all(|c| c.is_ascii_digit()))) + .then(|| v.to_owned()) +} + +/// Every version stated at `marker` in `text`, with the byte offset of each. +/// +/// Panics when the marker is absent. That is the fail-closed contract: a +/// reworded anchor must break this test loudly, because the alternative is an +/// audit that reports success while checking nothing. +fn versions_at(text: &str, anchor: &Anchor) -> Vec<(usize, String)> { + let mut out = Vec::new(); + let mut from = 0usize; + while let Some(i) = text[from..].find(anchor.marker) { + let at = from + i + anchor.marker.len(); + let v = parse_version_prefix(&text[at..]).unwrap_or_else(|| { + panic!( + "{}: found `{}` ({}) but what follows is not a MAJOR.MINOR.PATCH version: {:?}", + anchor.path, + anchor.marker, + anchor.what, + &text[at..(at + 24).min(text.len())] + ) + }); + out.push((at, v)); + from = at; + } + assert!( + !out.is_empty(), + "{}: no occurrence of `{}` ({}).\n\ + \n\ + The anchor was reworded, moved, or deleted. This audit fails closed on \ + purpose -- an anchor it cannot find is one it is not checking, and a \ + silent zero-anchor pass is the exact defect this file was written \ + after. Update the marker in ANCHORS to the new wording (or drop the \ + entry if the statement is genuinely gone).", + anchor.path, + anchor.marker, + anchor.what + ); + out +} + +/// The `## [X.Y.Z] - - "" ()` header for a version. +/// +/// `release-auto.yml` parses this same line twice — once for the release-body +/// fallback when no `.github/release-notes/vX.Y.Z.md` override exists, and once +/// to derive the release title's codename. Pinning it here means a malformed +/// header fails in CI rather than at publish time, which is when it failed for +/// v2.1.7. +fn changelog_header(version: &str) -> String { + let changelog = read("CHANGELOG.md"); + let needle = format!("## [{version}]"); + changelog + .lines() + .find(|l| l.starts_with(&needle)) + .unwrap_or_else(|| { + panic!( + "CHANGELOG.md has no `{needle}` section.\n\ + \n\ + The workspace is at {version}, so either the release entry was \ + not written or the version was bumped early. `release-auto.yml` \ + treats \"version has no matching tag\" as ready-to-release and \ + will fail closed for want of notes -- see the comment above \ + `[workspace.package] version`." + ) + }) + .to_owned() +} + +/// The quoted codename out of a CHANGELOG header line. +fn codename_of(header: &str) -> String { + let open = header + .find('"') + .unwrap_or_else(|| panic!("CHANGELOG header has no quoted codename: {header:?}")); + let rest = &header[open + 1..]; + let close = rest + .find('"') + .unwrap_or_else(|| panic!("CHANGELOG header has an unterminated codename: {header:?}")); + rest[..close].to_owned() +} + +/// Every anchor must state the version this tree actually is. +/// +/// This is the assertion whose absence let eight documents reach six different +/// answers. +#[test] +fn every_release_anchor_names_the_workspace_version() { + let version = workspace_package_field("version"); + let mut wrong: Vec = Vec::new(); + + for anchor in ANCHORS { + let text = read(anchor.path); + for (_, found) in versions_at(&text, anchor) { + if found != version { + wrong.push(format!( + " {} -- {} says v{found}, workspace is {version}", + anchor.path, anchor.what + )); + } + } + } + + assert!( + wrong.is_empty(), + "{} release anchor(s) disagree with `[workspace.package] version` = {version}:\n{}\n\n\ + Every one of these is a statement to a reader about which release they \ + are looking at. Update them in the same change as the version bump; the \ + release cut is the moment they are all correct at once.", + wrong.len(), + wrong.join("\n") + ); +} + +/// The CHANGELOG must carry a section for the version the workspace claims. +#[test] +fn the_changelog_has_a_section_for_the_workspace_version() { + let version = workspace_package_field("version"); + let header = changelog_header(&version); + + // Format: `## [X.Y.Z] - YYYY-MM-DD - "Codename" (theme)`. release-auto.yml + // strips the version and date prefix to derive the title, so a header + // missing either degrades the published title to a bare `RustyNES vX.Y.Z`. + let after_version = header + .split_once(']') + .expect("CHANGELOG header has no closing `]`") + .1; + assert!( + after_version.contains(" - "), + "CHANGELOG header for {version} is missing the ` - - ` tail, \ + which `release-auto.yml` parses to build the release title:\n {header}" + ); + assert!( + !codename_of(&header).is_empty(), + "CHANGELOG header for {version} has an empty codename:\n {header}" + ); +} + +/// Anchors that quote a codename must quote the CHANGELOG's codename. +/// +/// A correct version beside the previous release's codename is still a wrong +/// claim, and it is the more confusing kind: the number looks right, so a reader +/// trusts the sentence around it. +#[test] +fn every_anchor_that_quotes_a_codename_quotes_the_changelog_codename() { + let version = workspace_package_field("version"); + let expected = codename_of(&changelog_header(&version)); + let mut wrong: Vec = Vec::new(); + let mut checked = 0usize; + + for anchor in ANCHORS { + let text = read(anchor.path); + for (at, found) in versions_at(&text, anchor) { + if found != version { + continue; // reported by the version test; do not double-report + } + // A codename, when present, follows as ` "Name"`. + let tail = &text[at + found.len()..]; + let Some(rest) = tail.strip_prefix(" \"") else { + continue; // this anchor states a version only -- legitimate + }; + let Some(close) = rest.find('"') else { + continue; + }; + checked += 1; + let name = &rest[..close]; + if name != expected { + wrong.push(format!( + " {} -- {} says \"{name}\", CHANGELOG says \"{expected}\"", + anchor.path, anchor.what + )); + } + } + } + + assert!( + checked > 0, + "no anchor was found quoting a codename. Either every anchor was \ + reworded to drop the codename, or the ` \"Name\"` shape this test looks \ + for changed. Fail-closed: teach the test the new shape rather than \ + leaving it asserting nothing." + ); + assert!( + wrong.is_empty(), + "{} anchor(s) name the right version with the wrong codename:\n{}", + wrong.len(), + wrong.join("\n") + ); +} + +/// `VERSION-PLAN.md`'s release table must mark the current release `(current)`, +/// and mark only it. +/// +/// The table is the one anchor that carries a per-release row, so the drift it +/// suffers is different in kind: at v2.3.9 the header said v2.3.6 while the table +/// stopped at v2.3.5 and still marked that row `(current)` — four releases +/// unlisted, and the marker three releases stale. +#[test] +fn the_version_plan_table_marks_exactly_the_current_release() { + let version = workspace_package_field("version"); + let plan = read("VERSION-PLAN.md"); + + let marked: Vec<&str> = plan + .lines() + .filter(|l| l.contains("(current)") && l.trim_start().starts_with("| **v")) + .collect(); + + assert_eq!( + marked.len(), + 1, + "VERSION-PLAN.md's release table has {} rows marked `(current)`, expected \ + exactly 1:\n{}", + marked.len(), + marked + .iter() + .map(|l| format!(" {}", &l[..l.len().min(90)])) + .collect::>() + .join("\n") + ); + + let row = marked[0]; + let at = row.find("| **v").expect("checked by the filter above") + "| **v".len(); + let found = parse_version_prefix(&row[at..]).unwrap_or_else(|| { + panic!("VERSION-PLAN.md `(current)` row does not start with a version: {row:?}") + }); + + assert_eq!( + found, version, + "VERSION-PLAN.md marks v{found} as `(current)` but the workspace is at \ + {version}. Add the missing row(s) and move the marker; a release table \ + that stops short is how this one ended up three releases behind its own \ + header." + ); +} diff --git a/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md b/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md index 1a4fd0b6..fc9ba30b 100644 --- a/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md +++ b/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md @@ -9,14 +9,31 @@ > train) against the **current code on `main`**. Items already shipped since they > were written are excluded; everything below was verified still-outstanding. > -> **Audit currency (2026-08-20).** The most recent entry-level reconciliation in -> this file is **v2.3.4** (2026-08-15). The five releases since — **v2.3.5 -> "Manifest"**, **v2.3.6 "Sounding"**, **v2.3.7 "Overtone"**, **v2.3.8 -> "Parallax"** and **v2.3.9 "Crucible"** — have **not** been swept through it, so -> an item below may have been closed without its entry being struck. This note -> exists rather than a silent re-date because an unaudited catalogue that *looks* -> audited is worse than one that admits its cut-off: check `CHANGELOG.md` for -> anything dated after 2026-08-15 before treating an entry here as open. +> **Swept 2026-08-20, against `main` @ `fdfb2c04` (v2.3.9 "Crucible").** The +> v2.3.5 → v2.3.9 window was reconciled entry by entry. **Eleven** entries were +> struck; each carries the evidence inline — a file that exists, a workflow line +> number, or a test that says so — rather than a bare `[x]`, so the closure can be +> disagreed with. +> +> **Most of what this sweep found was stale by far more than five releases.** The +> whole of §6a — the four items (A1–A4) that define the timebase rewrite — shipped +> in **v2.0.0 "Timebase" on 2026-07-03**, six weeks and roughly twenty releases +> before this sweep, and §6's own preamble still described AccuracyCoin as +> "100% / 139/139" when it has been an exact **141/141** since v2.0.3. A backlog +> that lists the project's designated MAJOR release as pending is not merely +> untidy; it is actively misleading about what the emulator is. +> +> **Three entries were closed by something other than what they proposed**, and +> are struck with that distinction stated rather than quietly ticked: the +> feature-combo clippy gap is closed by *enumerating* the combos in CI rather than +> by adopting `cargo-hack`; `merge_group` is still open but its companion clause +> (a light PR matrix) shipped and the entry is narrowed; and R3 turned out to be a +> **harness artifact** rather than an emulation residual, which is a different +> kind of "fixed". +> +> **Not swept: §7 (mapper / coverage gaps) below the entries already dated +> v2.3.4.** Those need a ROM corpus to adjudicate, and asserting them from the +> source alone would be the exact over-claim this catalogue exists to avoid. > > **Reconciled against `main` @ `de682d8` on 2026-06-19** (the v1.7.0 "Forge" > baseline; the entries below remain the authoritative deferred backlog). @@ -48,7 +65,7 @@ > i18n H5 + **web/wasm H6** (§8a/b/c), the `full` alias (§11f), Kid Icarus FDS > (§12b). > - **Open + CI-doable → the v1.8.9 "Backlog" beta train** (additive/off-by-default, -> core byte-identical, AccuracyCoin 139/139 held): the §1 creator tooling (Virtual +> core byte-identical, AccuracyCoin held — 139/139 then, an exact 141/141 since v2.0.3): the §1 creator tooling (Virtual > Pad, Input Macros, BasicBot, multi-viewport, A/V codec depth, FDS Firmware > Manager, Multi-Disk Bundler, Batch Runner, `.fm2` rerecord-count export), §2 > `userdata` SQLite, §4d RA-HUD visual finish, §5b HD-pack spatial conditions, §7 @@ -119,12 +136,21 @@ each but that are **not present in `crates/` on `main`** (verified absent: no the TAStudio piano-roll pattern-paint. Source: [v1.7.0](plans/v1.7.0-forge-plan.md) H9 (pairs with the v1.6.0 piano-roll). Target: **v1.7.x (beta.5)**. Files: `crates/rustynes-frontend/src/tastudio*` + input layer. -- `[ ]` **BasicBot** — a save-state-anchored brute-force search tool (pairs with +- `[x]` **BasicBot** — *(SHIPPED, earlier than this sweep's window: v1.8.9-beta.4, + commit `91ef3b70` "feat: BasicBot control panel (Tools -> BasicBot)". Evidence: + `crates/rustynes-frontend/src/debugger/basic_bot_panel.rs` exists, and + `docs/user-guide/analysis-tools.md` documents it as present in the Tools → + Analysis submenu.)* a save-state-anchored brute-force search tool (pairs with the B1 `tastudio.*` API + Lua `frameadvance`). Source: [v1.7.0](plans/v1.7.0-forge-plan.md) H9. Target: **v1.7.x (beta.5)**. Files: frontend + `crates/rustynes-script`. -- `[ ]` **Multi-monitor / detachable tool windows** — egui multi-viewport so the - debugger/TAStudio panels can pop out into OS windows. Source: +- `[x]` **Multi-monitor / detachable tool windows** — *(SHIPPED v2.3.0 "Datum II", + commit `be4fbef0`. Evidence: `crates/rustynes-frontend/src/detached.rs`, whose + own preamble records that v2.2.9's `show_viewport_immediate` affordance merely + **embedded** the panel and that v2.3.0 replaced it with the real + `set_embed_viewports(false)` multi-viewport path — so the Windows-10 + trapped-window report is genuinely closed, not the stopgap.)* egui multi-viewport + so the debugger/TAStudio panels can pop out into OS windows. Source: [v1.7.0](plans/v1.7.0-forge-plan.md) H9. Target: **v1.7.x (beta.5)**. Files: `crates/rustynes-frontend/src/debugger/`. - `[ ]` **A/V dump-options depth (codec / sync)** — extend the v1.6.0 `av_record` @@ -264,10 +290,21 @@ the v1.7.0 **H1/H2** workstream + a maintainer-manual deploy/verify. ## 6. Accuracy → v2.0.0 "Timebase" (the master-clock rewrite, ADR 0002) +> **Swept 2026-08-20 — §6a is DONE.** All four items (A1-A4) shipped in +> **v2.0.0 "Timebase"** on 2026-07-03; they are struck below with their evidence. +> What remains open in this section is §6b's residuals R1, R2, R4 and R5 plus the +> sub-cycle items in §6c. **Two figures in the paragraph below were wrong and are +> corrected here rather than left as historical texture:** AccuracyCoin is an +> exact **141/141 (100.00%)** on the RAM decoder — not 139/139 — and has been +> since **v2.0.3**, and the section's framing of the timebase rewrite as a future +> release is six weeks and ~20 releases out of date. + All remaining hard-tier accuracy residuals share **one root cause** and converge on the v2.0.0 one-clock + every-cycle-bus-access refactor. They are **outside the -AccuracyCoin oracle** (zero production-ROM impact; AccuracyCoin is 100% / 139/139 -on the shipping default core). The maintainer's standing decision through v1.7.0 +AccuracyCoin oracle** (zero production-ROM impact; AccuracyCoin is an exact +**141/141** on the shipping default core, up from 139/139 when this paragraph was +written — the denominator grew in the v2.0.1 re-sync and v2.0.3 closed the two new +tests). The maintainer's standing decision through v1.7.0 is "keep deferring" point-fixes (ADR 0002 stop-condition; 15+ documented rollbacks); v2.0.0 is the one release licensed to break save-state/determinism and take this on. See [v2.0.0 plan](plans/v2.0.0-master-clock-plan.md) and @@ -275,21 +312,31 @@ take this on. See [v2.0.0 plan](plans/v2.0.0-master-clock-plan.md) and ### 6a. The timebase rewrite itself -- `[ ]` **One monotonic master clock (A1)** — collapse the five-counter substrate +- `[x]` **One monotonic master clock (A1)** — *(SHIPPED v2.0.0 "Timebase", + 2026-07-03, ADR 0029. `LockstepBus::cycle` is now the ONE canonical per-cycle + counter; `Cpu::cycles` and `Apu::cpu_cycle` are **assigned** from it rather than + independently incremented.)* collapse the five-counter substrate (`Cpu::master_clock`, `Cpu::cycles`, `LockstepBus::cycle`/`ppu_clock`, `Apu::cpu_cycle` + `apu_phase`/`put_cycle` parity + DMC byte-timer) to a single `master_clock: u64` with everything else derived by fixed arithmetic. Target: **v2.0.0**. Files: `crates/rustynes-cpu/src/cpu.rs`, `crates/rustynes-core/src/bus.rs`, `crates/rustynes-apu/src/apu.rs`. -- `[ ]` **Every cycle is a bus access (A2)** — replace the `dispatch()`-length + +- `[x]` **Every cycle is a bus access (A2)** — *(SHIPPED v2.0.0 "Timebase", + ADR 0029. Every instruction cycle is a real bus access — no busless filler — and + DMA is the unified per-cycle-interleaved engine rather than a separate stepping + mode. This was the "make-or-break stop-or-go gate" below; it went.)* replace the `dispatch()`-length + `idle_tick` burn-loop and the `dma-cycle-budget` hack with a per-cycle read/write/dummy-read model (interleaved DMA). The make-or-break beta.2 stop-or-go gate. Target: **v2.0.0**. Files: `crates/rustynes-cpu/src/cpu.rs`, `crates/rustynes-core/src/bus.rs`. -- `[ ]` **Reload arm invisible to its own cycle (A3)** — `pending_dmc_dma_next` - latch promoted at the next boundary. Target: **v2.0.0**. Files: +- `[x]` **Reload arm invisible to its own cycle (A3)** — *(SHIPPED v2.0.0. + Evidence: `pending_dmc_dma_next` exists at `crates/rustynes-apu/src/apu.rs:117` + and is carried in the APU snapshot, so it survives save-state round-trip.)* + `pending_dmc_dma_next` latch promoted at the next boundary. Target: **v2.0.0**. Files: `crates/rustynes-apu/src/apu.rs`. -- `[ ]` **Cycle-accurate reset (A4)** — replace the function-call `Nes::reset()` +- `[x]` **Cycle-accurate reset (A4)** — *(SHIPPED v2.0.0. The warm reset is a + clocked sequence including the `$4017` re-write — see `docs/cpu-6502.md` + + `docs/apu-2a03.md`.)* replace the function-call `Nes::reset()` with a real reset sequence (reset-vector-delay cycles + frame-counter re-arm). Target: **v2.0.0**. Files: `crates/rustynes-core/src/nes.rs`. @@ -304,8 +351,14 @@ take this on. See [v2.0.0 plan](plans/v2.0.0-master-clock-plan.md) and - `[ ]` **R2 — `mmc3_test_2/4` #2 reload-to-0 cadence + MMC6 variant** — same M2 sub-cycle axis as R1. Site: `tests/m004_mmc3.rs:187,207`. Target: **v2.0.0 (escape-hatch-able)**. -- `[ ]` **R3 — `apu_reset/len_ctrs_enabled` (FAIL #3)** — needs A4's - cycle-accurate reset. Site: `tests/apu_reset.rs:113`. Target: **v2.0.0**. +- `[x]` **R3 — `apu_reset/len_ctrs_enabled` (FAIL #3)** — *(CLOSED v2.0.0 beta.3, + and **not** in the way this entry predicted. `crates/rustynes-test-harness/tests/apu_reset.rs:107` + records it verbatim: the FAIL #3 was a **harness artifact**, not an emulation + residual, so A4 was not what fixed it. Worth keeping visible — a residual that + dissolves under a corrected measurement is a different outcome from one the + refactor closed, and conflating the two inflates what the refactor is credited + with.)* needs A4's cycle-accurate reset. Site: `tests/apu_reset.rs:113`. + Target: **v2.0.0**. - `[ ]` **R4 — `apu_reset/4017_written` (FAIL #3)** — same cycle-accurate-reset axis. Site: `tests/apu_reset.rs:138`. Target: **v2.0.0**. - `[ ]` **R5 — DMC reload-DMA span `Y=3` vs hardware `Y=4`** — five-counter parity @@ -686,21 +739,46 @@ account, or a hosted deploy (all also listed under their theme above). ## 11. CI / tooling follow-ups (proposed, not yet implemented) -- `[ ]` **`cargo-hack` mutually-exclusive feature clippy in CI** — the +- `[x]` **`cargo-hack` mutually-exclusive feature clippy in CI** — *(CLOSED by a + DIFFERENT mechanism than proposed, so read the distinction rather than the tick. + `cargo-hack` was never adopted; instead the combos are **enumerated explicitly** + in `.github/workflows/ci.yml` — `scripting` (line 286), `scripting,hd-pack` + (288), `retroachievements` (290), `full` (297), `rustynes-mappers + --no-default-features` (311), and three wasm32 combos including + `browser-cheevos` (483, 485, 491). The coverage gap the entry describes is + closed; the powerset tool is not in use, and enumeration means a NEW feature is + not covered until someone adds a line. That residual risk is the reason this is + struck with an explanation instead of silently.)* the `scripting` / `scripting,hd-pack` / `retroachievements` (and the new - `script-ipc` / `browser-cheevos`) clippy combos run **only locally / in the + `script-ipc` / `browser-cheevos`) clippy combos ran **only locally / in the pre-commit hook**; promoting a feature-powerset clippy into the CI lint job closes a real coverage gap (`--fix` can strip cfg-gated code another feature needs). Source: the CI-optimization memory note (PR #120 proposals). Target: **TBD**. Files: `.github/workflows/`. -- `[ ]` **Free arm64 CI leg** — `ubuntu-24.04-arm` is free on public repos and runs - in parallel. Source: CI-optimization note. Target: **TBD**. -- `[ ]` **`dorny/paths-filter` per-job skips** — needs a `ci-success` aggregator - job. A doc-only paths-filter gate already landed (#124); broader per-job skips - remain proposed. Source: CI-optimization note. Target: **TBD**. -- `[ ]` **`merge_group` + PR-Ubuntu-only matrix** — the highest runner-minute - saver but higher risk; maintainer decision pending. Source: CI-optimization note. - Target: **TBD (maintainer decision)**. +- `[x]` **Free arm64 CI leg** — *(SHIPPED, commit `b39889cc`. Evidence: + `ubuntu-24.04-arm` is in BOTH matrix arms of `.github/workflows/ci.yml` + (lines 249 and 252), so it runs on the light PR matrix as well as the full one; + `test (ubuntu-24.04-arm)` was observed green on PR #426.)* `ubuntu-24.04-arm` is + free on public repos and runs in parallel. Source: CI-optimization note. +- `[x]` **`dorny/paths-filter` per-job skips** — *(CLOSED v2.3.9 "Crucible". The + `CI success` aggregator this entry names as the prerequisite exists at + `.github/workflows/ci.yml:712`, `always()` over all nine job groups; the `lint` + job and its siblings gate on `needs.changes.outputs.code`; and v2.3.9 added a + SECOND filter step so `test-roms` is path-gated at review time. The reason it + needed two steps is worth carrying: `predicate-quantifier` is **step-level**, + `code` needs `every` for its `!` exclusions to work at all, and `accuracy` is a + list of ALTERNATIVES that becomes unsatisfiable under `every` — a one-step fix + would have silently disabled the accuracy battery while repairing the other + gate.)* needs a `ci-success` aggregator job. A doc-only paths-filter gate landed + in #124; the broader per-job skips followed. +- `[ ]` **`merge_group`** — *(NARROWED 2026-08-20: the companion clause, a light + PR matrix, has **shipped**. `.github/workflows/ci.yml:246-252` selects + `["ubuntu-latest", "ubuntu-24.04-arm"]` for a regular PR and the full four-OS set + otherwise — with `release/*` head branches deliberately taking the FULL matrix, + which is why a release PR still runs macOS and Windows. Only the merge queue + itself remains, and no `merge_group` trigger exists in any workflow.)* the + highest runner-minute saver but higher risk; maintainer decision pending. + Source: CI-optimization note. Target: **TBD (maintainer decision)**. - `[ ]` **`cargo-nextest` adoption** — ~1.3–1.5× test speedup but needs a separate `cargo test --doc` step and no retries. Source: CI-optimization note. Target: **TBD**. @@ -726,7 +804,14 @@ account, or a hosted deploy (all also listed under their theme above). Files: `tests/roms/AccuracyCoin/`, `tests/roms/accuracycoin/`, `crates/rustynes-test-harness/src/accuracy_coin_catalog.rs`. -- `[ ]` **Pass the two new AccuracyCoin PPU tests — `"ALE + Read"` + `"Hybrid Addresses"`** — +- `[x]` **Pass the two new AccuracyCoin PPU tests — `"ALE + Read"` + `"Hybrid Addresses"`** — + *(CLOSED v2.0.3, by the first of the two routes this entry names. The 2-cycle-ALE + fetch model was promoted from the experimental `mc-ppu-2cycle-ale` flag to the + unconditional, only PPU fetch path (ADR 0030), both experimental flags were + retired, and the shipped default has scored an exact **141/141 (100.00%)** on the + RAM decoder ever since — verified again on this tree by + `accuracycoin_pass_rate_meets_floor`. The `139/141` and "100% / 139/139" figures + elsewhere in this file predate that and are wrong.)* the accuracy gap opened by the v2.0.1 re-sync above. Both hinge on the PPU **octal latch** (the 74LS373 that multiplexes PA0-7 with the CHR data bus via the ALE signal; `AccuracyCoin.asm:2541-2614` is the authoritative cycle-level spec, nesdev From b69b50529a54e5e0a9f53de118ab84661e1ab3b5 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 02:15:23 -0400 Subject: [PATCH 2/6] fix(harness): close two fail-open holes in the anchor audit, and a false citation in the sweep Four review findings from #427, all valid. Two are defects in the audit's own stated property, and one is a factual error I introduced while writing about exactly that class of error. THE AUDIT ACCEPTED A HEADER release-auto.yml CANNOT PARSE Copilot and CodeRabbit converged on this independently. The header check asserted only that the tail contained " - " and a non-empty quoted string, so ## [2.3.9] - "Crucible" passed -- and would still have degraded the published title, because the workflow's s/^## \[[^]]*\][[:space:]]*-[[:space:]]*[0-9-]+[[:space:]]*-[[:space:]]*// strips `- -` and finds nothing to strip. The audit exists to hold the release-automation contract and was holding a weaker one. Each component is now checked separately: the ` - ` prefix, an ISO YYYY-MM-DD date (four/two/two digits, not merely `[0-9-]+`-ish), a non-empty quoted codename, and a parenthesised theme. Four mutations pin it -- date removed, date malformed as `20260820`, theme dropped, codename emptied -- and all four are caught. AN UNTERMINATED CODENAME FAILED OPEN CodeRabbit found the worse one. In the codename check, `tail.strip_prefix(" \"")` succeeding and `rest.find('"')` returning `None` means an anchor that OPENS a codename and never closes it -- malformed. The code `continue`d, treating it as "version-only", and because another well-formed anchor kept `checked > 0` the whole audit still passed. That is a fail-OPEN inside the test whose entire selling point, stated in its own module docs, is failing closed. It now panics, and the distinction is written at the site: reaching the `continue` above is legitimate (an anchor that states a version and no codename), reaching this point is not. I MIS-CITED THE EVIDENCE FOR A CLOSED BACKLOG ENTRY CodeRabbit checked the multi-viewport closure against the source and found the attribution wrong. I wrote that v2.3.0 closed it via `set_embed_viewports(false)`. That string occurs in `detached.rs` exactly once, in a comment explaining why egui's native multi-viewport path was REJECTED -- an immediate viewport is rendered by a re-entrant callback that needs `&ActiveEventLoop`, valid only during event dispatch. The actual implementation is a bespoke `DetachedManager` creating real OS windows, each with its own `egui_winit::State` and `egui_wgpu::Renderer` over a `winit` window. So I read an API name in the comment that rules it out and cited it as the mechanism that shipped. In a sweep whose stated purpose is that every closure carries checkable evidence, that is the failure being swept for, committed while performing the sweep. The entry now describes what the code does, and carries the correction rather than being quietly rewritten -- the wrong citation is the useful part of the record. THE PREAMBLE CONTRADICTED THE FILE Copilot: the preamble says shipped items are "excluded" and everything below "was verified still-outstanding", which stopped being true the moment this sweep started retaining `[x]` entries with their evidence. Amended in place, with the two reasons that drove the change: a struck entry with its evidence is a closure a reader can DISAGREE with, where a deleted one is unreviewable; and the same item was twice re-proposed after being silently dropped. `[ ]` open, `[x]` closed and cited, absent means never catalogued. A NOTE ON THE PROCESS The first attempt at the header fix did not land. The edit script bundled it with the codename fix, the codename anchor did not match (rustfmt had reflowed the region), and the script exited before writing -- so NEITHER was applied while the suite stayed green and reported 4 passed. The mutation pass is what caught it: three header mutations came back "not caught", which is only explicable if the code under test was not there. A green suite after an edit is not evidence the edit happened. GATES cargo fmt --all --check ......................... clean cargo clippy -p rustynes-test-harness -D warn ... clean release_anchor_audit ............................ 4 passed markdownlint on the changed doc ................. passed Mutations, all confirmed to have actually RUN the named test rather than matching zero and exiting 0: header: date removed ........... CAUGHT header: malformed date ......... CAUGHT header: theme dropped .......... CAUGHT header: codename emptied ....... CAUGHT anchor: unterminated codename .. CAUGHT --- .../tests/release_anchor_audit.rs | 78 +++++++++++++++++-- to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md | 29 ++++++- 2 files changed, 95 insertions(+), 12 deletions(-) diff --git a/crates/rustynes-test-harness/tests/release_anchor_audit.rs b/crates/rustynes-test-harness/tests/release_anchor_audit.rs index 20541e06..fec999a5 100644 --- a/crates/rustynes-test-harness/tests/release_anchor_audit.rs +++ b/crates/rustynes-test-harness/tests/release_anchor_audit.rs @@ -323,22 +323,73 @@ fn the_changelog_has_a_section_for_the_workspace_version() { let version = workspace_package_field("version"); let header = changelog_header(&version); - // Format: `## [X.Y.Z] - YYYY-MM-DD - "Codename" (theme)`. release-auto.yml - // strips the version and date prefix to derive the title, so a header - // missing either degrades the published title to a bare `RustyNES vX.Y.Z`. + // The header must match what `release-auto.yml` actually parses, component by + // component: + // + // ## [X.Y.Z] - YYYY-MM-DD - "Codename" (theme) + // + // The first version asserted only that the tail contained " - " and a + // non-empty quoted string. Review on #427 caught that as too loose, and it + // was: `## [2.3.9] - "Crucible"` satisfies both and would still have degraded + // the published title, because the workflow's sed strips `- -` and + // finds nothing to strip. An audit that passes the malformed input it exists + // to reject is worse than no audit, so each component is checked separately. let after_version = header .split_once(']') .expect("CHANGELOG header has no closing `]`") .1; + + // ` - YYYY-MM-DD - ` -- the shape the workflow's + // `s/^## \[[^]]*\][[:space:]]*-[[:space:]]*[0-9-]+[[:space:]]*-[[:space:]]*//` + // removes. A date it cannot match leaves the whole prefix in the title. + let tail = after_version.strip_prefix(" - ").unwrap_or_else(|| { + panic!( + "CHANGELOG header for {version} does not continue ` - - ...`, \ + which `release-auto.yml` strips to build the release title:\n {header}" + ) + }); + let (date, rest) = tail.split_once(" - ").unwrap_or_else(|| { + panic!( + "CHANGELOG header for {version} has no ` - ` after the date, so the \ + title would keep the date prefix:\n {header}" + ) + }); + let date_parts: Vec<&str> = date.split('-').collect(); assert!( - after_version.contains(" - "), - "CHANGELOG header for {version} is missing the ` - - ` tail, \ - which `release-auto.yml` parses to build the release title:\n {header}" + date_parts.len() == 3 + && date_parts[0].len() == 4 + && date_parts[1].len() == 2 + && date_parts[2].len() == 2 + && date_parts + .iter() + .all(|p| p.chars().all(|c| c.is_ascii_digit())), + "CHANGELOG header for {version} has `{date}` where an ISO YYYY-MM-DD date \ + belongs. `release-auto.yml` matches `[0-9-]+`, so a malformed date is \ + either left in the title or swallows part of the codename:\n {header}" ); + + // `"Codename" (theme)` -- what becomes the title's suffix. + let codename = codename_of(&header); assert!( - !codename_of(&header).is_empty(), + !codename.is_empty(), "CHANGELOG header for {version} has an empty codename:\n {header}" ); + assert!( + rest.starts_with('"'), + "CHANGELOG header for {version} does not begin its theme with a quoted \ + codename:\n {header}" + ); + let after_codename = rest[1..] + .split_once('"') + .expect("codename_of already proved the closing quote exists") + .1; + let theme = after_codename.trim(); + assert!( + theme.starts_with('(') && theme.ends_with(')') && theme.len() > 2, + "CHANGELOG header for {version} has no parenthesised theme after the \ + codename. Every release in this file carries one, and it is half of what \ + the published title says:\n {header}" + ); } /// Anchors that quote a codename must quote the CHANGELOG's codename. @@ -364,8 +415,19 @@ fn every_anchor_that_quotes_a_codename_quotes_the_changelog_codename() { let Some(rest) = tail.strip_prefix(" \"") else { continue; // this anchor states a version only -- legitimate }; + // An OPENING quote with no closing one is malformed, not + // "version-only". The first version `continue`d here, so a broken + // release claim slipped through as long as some OTHER anchor kept + // `checked > 0` — a fail-OPEN inside the test whose stated property is + // failing closed. Review on #427 caught it. Reaching the `continue` + // above is legitimate; reaching this point is not. let Some(close) = rest.find('"') else { - continue; + panic!( + "{}: {} opens a codename after v{found} and never closes it: {:?}", + anchor.path, + anchor.what, + &rest[..rest.len().min(40)] + ) }; checked += 1; let name = &rest[..close]; diff --git a/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md b/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md index fc9ba30b..f68f2bc7 100644 --- a/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md +++ b/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md @@ -9,6 +9,16 @@ > train) against the **current code on `main`**. Items already shipped since they > were written are excluded; everything below was verified still-outstanding. > +> **Amended 2026-08-20 (review on #427).** That sentence describes the file as it +> was FIRST built and no longer describes the file. Resolved items are now +> **retained** and marked `[x]` with the evidence that closed them — a release, a +> commit, a file, a test — rather than being deleted. Two reasons, both learned +> here: a struck entry with its evidence is a closure a reader can *disagree +> with*, where a deleted one is unreviewable; and the same item was twice +> re-proposed after being silently dropped. So: `[ ]` is open, `[x]` is closed +> **and cited**, and absence means it was never catalogued rather than that it was +> finished. +> > **Swept 2026-08-20, against `main` @ `fdfb2c04` (v2.3.9 "Crucible").** The > v2.3.5 → v2.3.9 window was reconciled entry by entry. **Eleven** entries were > struck; each carries the evidence inline — a file that exists, a workflow line @@ -146,10 +156,21 @@ each but that are **not present in `crates/` on `main`** (verified absent: no frontend + `crates/rustynes-script`. - `[x]` **Multi-monitor / detachable tool windows** — *(SHIPPED v2.3.0 "Datum II", commit `be4fbef0`. Evidence: `crates/rustynes-frontend/src/detached.rs`, whose - own preamble records that v2.2.9's `show_viewport_immediate` affordance merely - **embedded** the panel and that v2.3.0 replaced it with the real - `set_embed_viewports(false)` multi-viewport path — so the Windows-10 - trapped-window report is genuinely closed, not the stopgap.)* egui multi-viewport + `DetachedManager` creates **real OS windows**, each with its own + `egui_winit::State` and `egui_wgpu::Renderer` over a `winit` window — so the + Windows-10 trapped-window report is genuinely closed, not the v2.2.9 stopgap + where `show_viewport_immediate` merely **embedded** the panel in the single main + viewport.* + * + **Correction, review on #427:** this entry first cited + `set_embed_viewports(false)` as the mechanism. That is wrong — the string + appears in `detached.rs` exactly once, in a comment explaining why egui's + native multi-viewport path was **rejected** (an immediate viewport is rendered + by a re-entrant callback needing `&ActiveEventLoop`, valid only during event + dispatch). Citing an API the module explicitly does not call, having read the + name in the comment that rules it out, is the same failure this sweep exists to + correct — committed while performing it. The closure itself stands; only the + attribution was wrong.)* egui multi-viewport so the debugger/TAStudio panels can pop out into OS windows. Source: [v1.7.0](plans/v1.7.0-forge-plan.md) H9. Target: **v1.7.x (beta.5)**. Files: `crates/rustynes-frontend/src/debugger/`. From b2240320a5d05c87449c0e053792d685673fc239 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 02:17:34 -0400 Subject: [PATCH 3/6] docs(backlog): stop the retained prose contradicting its own closure Sixth review finding on #427 (CodeRabbit), and a real one distinct from the preamble fix: the `[x]` annotations were PREPENDED to each closed entry while the original body still described the same work as pending -- "run only locally", "remain proposed", "was reverted in full". A reader reaching the second half of an entry found it contradicting the first. Three entries corrected: * cargo-hack feature clippy -- the tail said promoting a powerset clippy into CI "closes a real coverage gap" and "Target: TBD". It reads as the gap being closed by enumeration now, with the `Target` line dropped because there is nothing left to target. * free arm64 CI leg -- the tail argued for the leg in the present tense. Past tense: that WAS why it was proposed. * the two AccuracyCoin PPU tests -- the entry retains a long narrative of the v2.0.1 attempt that was reverted in full, which is genuinely confusing beside a closure. Rather than delete it, it now opens with a note saying what it is: the record of a REJECTED approach, kept because the 2-cycle-ALE refactor it names at its end as one of two possible routes is exactly what v2.0.3 built. The stale 139/141 figures inside it are flagged as the state during that attempt. Deleting the narratives would have been quicker and would have lost why each closure took the shape it did. A rejected approach is evidence too. --- to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md b/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md index f68f2bc7..e8c7cfd6 100644 --- a/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md +++ b/to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md @@ -772,15 +772,16 @@ account, or a hosted deploy (all also listed under their theme above). struck with an explanation instead of silently.)* the `scripting` / `scripting,hd-pack` / `retroachievements` (and the new `script-ipc` / `browser-cheevos`) clippy combos ran **only locally / in the - pre-commit hook**; promoting a feature-powerset clippy into the CI lint job - closes a real coverage gap (`--fix` can strip cfg-gated code another feature - needs). Source: the CI-optimization memory note (PR #120 proposals). Target: - **TBD**. Files: `.github/workflows/`. + pre-commit hook**, and the gap that mattered (`--fix` can strip cfg-gated code + another feature needs) is closed — by enumeration in the CI lint job rather + than by a feature powerset. Source: the CI-optimization memory note (PR #120 + proposals). Files: `.github/workflows/ci.yml`. - `[x]` **Free arm64 CI leg** — *(SHIPPED, commit `b39889cc`. Evidence: `ubuntu-24.04-arm` is in BOTH matrix arms of `.github/workflows/ci.yml` (lines 249 and 252), so it runs on the light PR matrix as well as the full one; - `test (ubuntu-24.04-arm)` was observed green on PR #426.)* `ubuntu-24.04-arm` is - free on public repos and runs in parallel. Source: CI-optimization note. + `test (ubuntu-24.04-arm)` was observed green on PR #426.)* `ubuntu-24.04-arm` + was free on public repos and ran in parallel, which is why it was proposed. + Source: CI-optimization note. - `[x]` **`dorny/paths-filter` per-job skips** — *(CLOSED v2.3.9 "Crucible". The `CI success` aggregator this entry names as the prerequisite exists at `.github/workflows/ci.yml:712`, `always()` over all nine job groups; the `lint` @@ -848,6 +849,12 @@ account, or a hosted deploy (all also listed under their theme above). `_ppuBusAddress` (+ 3-dot-delayed `v`, bus not re-synced during render); **ares** models only the `$2007` side (`io.busAddress`); **higan** blocks `$2007` during render. **ATTEMPTED in v2.0.1 and deferred — see [ADR 0030](../docs/adr/0030-accuracycoin-ale-read-hybrid-addresses-octal-latch.md).** + *(Everything from here to the end of this entry describes that FIRST, + reverted attempt. It is kept because it is the reason the SECOND attempt took + the shape it did: the 2-cycle-ALE refactor named at its end as one of two + possible routes is exactly what v2.0.3 then built and shipped. Read it as the + record of a rejected approach, not as open work -- the item is closed above, + and the `139/141` figures below are the state during that attempt.)* The bounded attempt added the `octal_latch` + one-shot hybrid-fetch hook and confirmed the tracking is inert (still 139/141), but RustyNES's single-step fetch model does not hold the per-cycle multiplexed-bus low byte the tests probe, so the corruption lands on From efe0596bf510ec30986e269025c3f5a92535aa21 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 02:27:28 -0400 Subject: [PATCH 4/6] fix(harness): harden the anchor audit's parsing against three fail-open cases Antigravity's second pass on #427. Two findings raised as blocking; one is real, one is a false positive raised for the fourth time in this project's history. Two suggestions and both nitpicks are adopted. REAL: A PRE-RELEASE WORKSPACE VERSION WOULD HAVE FAILED EVERY CORRECT ANCHOR `parse_version_prefix` stops at the first non-digit-non-dot, so with a workspace version of `2.4.0-rc.1` the anchors -- correctly reading `v2.4.0-rc.1` -- parsed back as `2.4.0` and every one of them was reported as disagreeing. A false failure that would have blocked a release cut for a reason unrelated to the anchors. The naive fix, parsing the suffix too, is wrong here and the reason is worth recording: the README badge is `badge/version-v2.3.9-blue.svg`, where the hyphen is a URL delimiter and not a pre-release marker. Teaching the parser about suffixes turns the badge into version `2.3.9-blue.svg`. So the comparison is made on the MAJOR.MINOR.PATCH core of both sides instead, via a new `version_core`, and a test pins that the badge still parses as `2.3.9`. The cost is stated rather than hidden: an anchor reading `v2.4.0` while the tree is at `2.4.0-rc.1` now passes. That is the right call -- whether prose names a pre-release suffix is a maintainer style choice, while naming the wrong release LINE is the error this audit exists to catch. The scenario turned out to be UNREACHABLE TODAY, for a reason neither the review nor I predicted. Reproducing it by setting the workspace version to `2.3.9-rc.1` never reaches the audit, because cargo rejects the manifest first: error: failed to select a version for the requirement `rustynes-apu = "^2.0.0"` candidate versions found which didn't match: 2.3.9-rc.1 A caret requirement does not match a pre-release, so every intra-workspace dependency would have to be rewritten before this workspace could carry one. The guard stays anyway, and `version_core` is tested DIRECTLY rather than through a manifest mutation that cannot run -- the day someone does that work, this audit should not be what blocks them. FALSE POSITIVE: let-chains The review calls `if let Some((k, v)) = t.split_once('=') && k.trim() == key` unstable syntax needing nightly. It is stable in edition 2024, which this workspace uses on a pinned stable 1.96.0; the IDENTICAL construct has been on `main` in `libretro_info_audit.rs` since v2.3.5 (that is where this parser was copied from); and CI compiled this exact file on the previous run -- `fmt + clippy + rustdoc` and `test (ubuntu-latest)` both green. No change made. This is the fourth time the claim has been raised on this repository. ADOPTED: THREE MORE FAIL-OPEN CASES * Table header with an inline comment. `t == "[workspace.package]"` silently skipped the table when the header carried a trailing comment, and the failure mode is fail-OPEN in an unhelpful way: `in_table` stays false, and the panic at the bottom blames a missing key rather than the header. Now `starts_with`. * Inline comment on the value. `version = "2.3.9" # pinned` yielded `2.3.9" # pinned`. The comment is stripped before unquoting. * Whitespace before a codename. `strip_prefix(" \"")` matched exactly one space, so a document writing two silently bypassed the codename check while another anchor kept `checked > 0` -- the same fail-open shape as the unterminated quote fixed in the previous commit. Now `trim_start_matches(' ')` then a bare quote, and the two-space case is mutation-tested. Also adopted: `versions_at` resumes the scan past the parsed version rather than at its start. Safe either way given the marker lengths, but resuming after what was consumed is what the loop means. VERIFIED two spaces + wrong codename ............ CAUGHT (was bypassed) inline comment on table header ......... still passes (was fail-open) pre-release core comparison ............ 5 direct assertions, incl. the badge release_anchor_audit ................... 5 passed clippy -D warnings / rustdoc -D warnings clean --- .../tests/release_anchor_audit.rs | 85 +++++++++++++++++-- 1 file changed, 77 insertions(+), 8 deletions(-) diff --git a/crates/rustynes-test-harness/tests/release_anchor_audit.rs b/crates/rustynes-test-harness/tests/release_anchor_audit.rs index fec999a5..c0c9c7bd 100644 --- a/crates/rustynes-test-harness/tests/release_anchor_audit.rs +++ b/crates/rustynes-test-harness/tests/release_anchor_audit.rs @@ -79,7 +79,11 @@ fn workspace_package_field(key: &str) -> String { for line in manifest.lines() { let t = line.trim(); if t.starts_with('[') { - in_table = t == "[workspace.package]"; + // `starts_with` rather than `==`: a table header may carry a trailing + // inline comment, and an exact match would silently skip the table -- + // which fails OPEN, since `in_table` would stay false and the panic at + // the bottom would blame a missing key. (Review on #427.) + in_table = t.starts_with("[workspace.package]"); continue; } if !in_table { @@ -88,12 +92,69 @@ fn workspace_package_field(key: &str) -> String { if let Some((k, v)) = t.split_once('=') && k.trim() == key { + // Strip a trailing inline comment before unquoting, so + // `version = "2.3.9" # pinned` yields `2.3.9` rather than + // `2.3.9" # pinned`. (Review on #427.) + let v = v.split('#').next().unwrap_or(v); return v.trim().trim_matches('"').to_owned(); } } panic!("`[workspace.package]` has no `{key}` key"); } +/// The `MAJOR.MINOR.PATCH` core of a version, dropping any `SemVer` pre-release or +/// build suffix. +/// +/// The audit compares release *lines*, not exact `SemVer` strings, and that is a +/// decision rather than a shortcut. Review on #427 found the reason: with a +/// pre-release workspace version such as `2.4.0-rc.1`, comparing the raw strings +/// fails every anchor even when all of them are correct, because `v2.4.0-rc.1` in +/// prose parses back as `2.4.0`. Worse, the README badge is +/// `badge/version-v2.3.9-blue.svg`, where the hyphen is a URL delimiter and not a +/// pre-release marker at all — so "just parse the suffix too" turns the badge into +/// version `2.3.9-blue.svg`. +/// +/// Comparing the numeric core sidesteps both. The cost is that an anchor reading +/// `v2.4.0` while the tree is at `2.4.0-rc.1` passes, which is the right call: +/// whether prose names a pre-release suffix is a maintainer style choice, while +/// naming the wrong release line is the error this audit exists to catch. +fn version_core(v: &str) -> &str { + v.split(['-', '+']).next().unwrap_or(v) +} + +/// `version_core` is tested directly, because the situation it exists for cannot +/// be reached through `Cargo.toml`. +/// +/// Setting `[workspace.package] version = "2.3.9-rc.1"` to reproduce the reported +/// failure does not reach this audit at all — **cargo** rejects it first: +/// +/// ```text +/// error: failed to select a version for the requirement `rustynes-apu = "^2.0.0"` +/// candidate versions found which didn't match: 2.3.9-rc.1 +/// ``` +/// +/// A caret requirement does not match a pre-release, so every intra-workspace +/// dependency would have to be rewritten before the workspace could carry a +/// pre-release version. That makes the scenario unreachable *today* rather than +/// impossible — the guard stays, because the day someone does that work this +/// audit should not be the thing that then blocks the release for a reason +/// unrelated to the anchors. +#[test] +fn version_core_drops_a_prerelease_or_build_suffix() { + assert_eq!(version_core("2.3.9"), "2.3.9"); + assert_eq!(version_core("2.4.0-rc.1"), "2.4.0"); + assert_eq!(version_core("2.4.0-beta.5"), "2.4.0"); + assert_eq!(version_core("2.4.0+build.7"), "2.4.0"); + // The README badge is `badge/version-v2.3.9-blue.svg`, where the hyphen is a + // URL delimiter. `parse_version_prefix` already stops at it, so the badge + // never reaches here carrying a suffix -- pinned so a future "parse the + // suffix too" change cannot silently turn the badge into `2.3.9-blue.svg`. + assert_eq!( + parse_version_prefix("2.3.9-blue.svg").as_deref(), + Some("2.3.9") + ); +} + /// One place a document states the current release version. /// /// `marker` is the literal text immediately preceding the version, so the audit @@ -228,7 +289,10 @@ fn versions_at(text: &str, anchor: &Anchor) -> Vec<(usize, String)> { ) }); out.push((at, v)); - from = at; + // Resume past the parsed version rather than at its start. `at` alone is + // safe today because every marker is longer than zero, but resuming after + // what was just consumed is what the loop means. (Review on #427.) + from = at + out.last().expect("just pushed").1.len(); } assert!( !out.is_empty(), @@ -292,14 +356,15 @@ fn codename_of(header: &str) -> String { #[test] fn every_release_anchor_names_the_workspace_version() { let version = workspace_package_field("version"); + let expected = version_core(&version); let mut wrong: Vec = Vec::new(); for anchor in ANCHORS { let text = read(anchor.path); for (_, found) in versions_at(&text, anchor) { - if found != version { + if found != expected { wrong.push(format!( - " {} -- {} says v{found}, workspace is {version}", + " {} -- {} says v{found}, workspace is {expected}", anchor.path, anchor.what )); } @@ -407,12 +472,16 @@ fn every_anchor_that_quotes_a_codename_quotes_the_changelog_codename() { for anchor in ANCHORS { let text = read(anchor.path); for (at, found) in versions_at(&text, anchor) { - if found != version { + if found != version_core(&version) { continue; // reported by the version test; do not double-report } - // A codename, when present, follows as ` "Name"`. - let tail = &text[at + found.len()..]; - let Some(rest) = tail.strip_prefix(" \"") else { + // A codename, when present, follows as ` "Name"` -- but the space + // count is a prose detail, not a contract. Matching exactly one space + // let a document write two and silently bypass the check while some + // OTHER anchor kept `checked > 0`, which is the same fail-open shape + // as the unterminated quote below. (Review on #427.) + let tail = text[at + found.len()..].trim_start_matches(' '); + let Some(rest) = tail.strip_prefix('"') else { continue; // this anchor states a version only -- legitimate }; // An OPENING quote with no closing one is malformed, not From 1577689c6e7db3bb61aff414e5c06e2f00e18c3e Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 03:09:07 -0400 Subject: [PATCH 5/6] fix(harness): a panic that could destroy its own diagnostic, and a claim I nearly acted on unchecked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Antigravity's third pass on #427. Three blocking findings: ONE is real and serious, one is a false positive I verified before acting on, and one is the let-chains claim for the fifth time. REAL: THE PANIC PATH COULD PANIC `&text[at..(at + 24).min(text.len())]` slices a `str` at an arbitrary BYTE offset to build an excerpt for a failure message. These documents are full of em-dashes and arrows, so the offset can land inside a multi-byte character -- and then the audit panics WHILE FORMATTING THE DIAGNOSTIC, replacing the message that explains the real failure with a byte-index error about the reporting code. A diagnostic that can crash the diagnosis is worse than no diagnostic: the failure it was written to explain becomes strictly harder to understand than if the excerpt had been omitted. Three sites, all in message paths, all now `chars().take(n)`. Reproduced before and after. With a marker followed by a run of em-dashes, the audit now reports: SUPPORT.md: found `the current release is **v` ... but what follows is not a MAJOR.MINOR.PATCH version: "———————————" which is the message that was designed, rather than a char-boundary panic. FALSE POSITIVE, CHECKED RATHER THAN ACCEPTED: SUB-TABLE MATCHING The review states `starts_with("[workspace.package]")` also matches `[workspace.package.metadata]`, so the parser would read `version` from the wrong section, and supplied a fix. It is a plausible-sounding claim about a real class of bug, and I had already written the fix and a commit message explaining the regression I had supposedly introduced -- before testing it. It does not hold. The literal ends with `]`; the sub-table has `.` at that position: "[workspace.package.metadata]".starts_with("[workspace.package]") == false '[workspace.package.' vs '[workspace.package]' Confirmed end-to-end as well: injecting such a sub-table above the real one and running the audit under BOTH parser forms reads `2.3.9` either way. The exact-comparison form is kept, because it is correct without requiring a reader to notice that a closing bracket is doing the work -- but the comment now says what is true instead of crediting a fix for a bug that was never there, and a new test pins the property so the reasoning cannot be lost. `sub_table_headers_do_not_match_the_workspace_package_table` asserts the `starts_with` fact directly alongside the four shapes the parser must handle. Recording this because the near-miss is the point. This PR is an audit built to stop unverified claims reaching `main`, and I came within one command of committing an unverified claim about it, in the confident register of a bug fix. The reviewer's other two findings were correct, which is exactly what makes the third one easy to wave through. FALSE POSITIVE, FIFTH OCCURRENCE: let-chains Stable in edition 2024; identical construct on `main` in `libretro_info_audit.rs`; CI has compiled this file green on three runs of this PR. No change. release_anchor_audit ..... 6 passed clippy -D warnings ....... clean --- .../tests/release_anchor_audit.rs | 59 ++++++++++++++++--- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/crates/rustynes-test-harness/tests/release_anchor_audit.rs b/crates/rustynes-test-harness/tests/release_anchor_audit.rs index c0c9c7bd..241d4f10 100644 --- a/crates/rustynes-test-harness/tests/release_anchor_audit.rs +++ b/crates/rustynes-test-harness/tests/release_anchor_audit.rs @@ -79,11 +79,28 @@ fn workspace_package_field(key: &str) -> String { for line in manifest.lines() { let t = line.trim(); if t.starts_with('[') { - // `starts_with` rather than `==`: a table header may carry a trailing - // inline comment, and an exact match would silently skip the table -- - // which fails OPEN, since `in_table` would stay false and the panic at - // the bottom would blame a missing key. (Review on #427.) - in_table = t.starts_with("[workspace.package]"); + // Strip an inline comment, THEN compare exactly. + // + // An exact `==` against the RAW line silently skips a header carrying + // a trailing comment, and does so confusingly: `in_table` stays false + // and the panic at the bottom blames a missing key rather than the + // header. That is the hole this closes. (Review on #427.) + // + // Review also raised a second concern against the `starts_with` form + // that preceded this -- that it would match SUB-TABLES like + // `[workspace.package.metadata]` and read `version` from the wrong + // section. **That one does not hold, and it was checked rather than + // assumed:** the literal ends with `]`, and the sub-table has `.` + // at that position, so `"[workspace.package.metadata]" + // .starts_with("[workspace.package]")` is `false`. Injecting such a + // sub-table above the real one and running the audit confirms it -- + // both forms read `2.3.9`. + // + // The exact form is kept anyway, because it is correct without + // requiring the reader to notice that the closing bracket is doing + // the work. `sub_table_headers_do_not_match_the_workspace_package_table` + // pins the property so the reasoning cannot be lost. + in_table = t.split('#').next().unwrap_or(t).trim() == "[workspace.package]"; continue; } if !in_table { @@ -139,6 +156,24 @@ fn version_core(v: &str) -> &str { /// impossible — the guard stays, because the day someone does that work this /// audit should not be the thing that then blocks the release for a reason /// unrelated to the anchors. +/// A TOML sub-table header must not be mistaken for the table itself. +/// +/// Review on #427 raised this as a blocking defect against the `starts_with` +/// form. It is not one — the literal ends with `]` and a sub-table has `.` there +/// — but the property is worth pinning rather than left to a reader spotting a +/// closing bracket, and the parser now states it structurally. +#[test] +fn sub_table_headers_do_not_match_the_workspace_package_table() { + // The claim that was checked rather than accepted. + assert!(!"[workspace.package.metadata]".starts_with("[workspace.package]")); + // What the parser actually does, in both shapes it must handle. + let exact = |t: &str| t.split('#').next().unwrap_or(t).trim() == "[workspace.package]"; + assert!(exact("[workspace.package]")); + assert!(exact("[workspace.package] # pinned")); + assert!(!exact("[workspace.package.metadata]")); + assert!(!exact("[workspace.dependencies]")); +} + #[test] fn version_core_drops_a_prerelease_or_build_suffix() { assert_eq!(version_core("2.3.9"), "2.3.9"); @@ -274,6 +309,14 @@ fn parse_version_prefix(s: &str) -> Option { /// Panics when the marker is absent. That is the fail-closed contract: a /// reworded anchor must break this test loudly, because the alternative is an /// audit that reports success while checking nothing. +/// +/// The excerpts in the panic messages are built with `chars().take(n)`, never a +/// byte slice. These documents are full of em-dashes and arrows, so +/// `&text[at..at + 24]` can land inside a multi-byte character and panic **while +/// formatting the diagnostic** -- destroying the message that explains the real +/// failure and replacing it with a byte-index error about the reporting code. +/// A diagnostic that can crash the diagnosis is worse than no diagnostic. +/// (Review on #427.) fn versions_at(text: &str, anchor: &Anchor) -> Vec<(usize, String)> { let mut out = Vec::new(); let mut from = 0usize; @@ -285,7 +328,7 @@ fn versions_at(text: &str, anchor: &Anchor) -> Vec<(usize, String)> { anchor.path, anchor.marker, anchor.what, - &text[at..(at + 24).min(text.len())] + text[at..].chars().take(24).collect::() ) }); out.push((at, v)); @@ -495,7 +538,7 @@ fn every_anchor_that_quotes_a_codename_quotes_the_changelog_codename() { "{}: {} opens a codename after v{found} and never closes it: {:?}", anchor.path, anchor.what, - &rest[..rest.len().min(40)] + rest.chars().take(40).collect::() ) }; checked += 1; @@ -549,7 +592,7 @@ fn the_version_plan_table_marks_exactly_the_current_release() { marked.len(), marked .iter() - .map(|l| format!(" {}", &l[..l.len().min(90)])) + .map(|l| format!(" {}", l.chars().take(90).collect::())) .collect::>() .join("\n") ); From 39041f5ba0fca03911358bd125a02b1c7fd15431 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 08:32:42 -0400 Subject: [PATCH 6/6] fix(harness): close two latent fail-opens, and a test that was checking a copy of itself Antigravity's fourth pass on #427. Two adopted, two suggestions adopted, one false positive for the sixth time. The interesting part is a defect in my own test that only the mutation pass could see. TWO LATENT FAIL-OPENS, BOTH THE SAME SHAPE Neither is live; both would wait for a reformat and then silently stop checking. * Markdown emphasis between version and codename. `**v2.3.9** "Crucible"` leaves `tail` starting at `**`, `strip_prefix('"')` fails, and the anchor takes the legitimate "no codename here" path -- silently skipped, while another anchor keeps `checked > 0`. Checked against all ten documents rather than assumed: no anchor is written that way today. Now strips `*` as well as spaces, and the version-only shapes still skip correctly (`**v2.3.9** (2026-08-20)` strips to `(2026-08-20`, not a quote). * A version at the end of a sentence. `parse_version_prefix` consumed contiguous dots, so `v2.3.9.` yielded four parts, returned `None`, and the caller turned that into a PANIC. Fail-closed is right for a missing marker; a full stop is not a missing marker. Trailing dots are trimmed, and `2.3.9.4` is still correctly rejected. A TEST THAT REIMPLEMENTED WHAT IT CHECKED The first version of the emphasis test declared a local fn strip(t: &str) -> &str { t.trim_start_matches([' ', '*']) } and asserted against THAT. It passed, and the mutation removing the production stripping came back NOT CAUGHT -- because the test was never looking at the production code. A test that reimplements its subject is testing itself, and it will agree with itself forever. Fixed by extracting `skip_to_codename` and having both the audit and the test call it. The mutation is now caught. This is the third time in this release that "extract it so a test can reach it" has been the fix -- the injectable predicate in `atomic_write`, `TimelineWatch` in the debugger, and now this -- and in every case the code READ as testable beforehand. SUGGESTIONS ADOPTED AS SCOPE NOTES `v.split('#')` would truncate a TOML value legitimately containing `#`. Correct for every key this private helper is asked for (`version`, `license`); the note now says so, and says that a future caller wanting a `#`-bearing value needs a real parser rather than a patch. The reason a `toml` dependency is not pulled in for a documentation audit is that this test target would be the only thing needing it. FALSE POSITIVE, SIXTH OCCURRENCE: let-chains Stable in edition 2024. CI has now compiled this file green on four runs of this PR, at four different SHAs. No change. release_anchor_audit ..... 8 passed clippy -D warnings ....... clean mutations ................ emphasis stripping CAUGHT, trailing-dot trim CAUGHT --- .../tests/release_anchor_audit.rs | 86 +++++++++++++++++-- 1 file changed, 79 insertions(+), 7 deletions(-) diff --git a/crates/rustynes-test-harness/tests/release_anchor_audit.rs b/crates/rustynes-test-harness/tests/release_anchor_audit.rs index 241d4f10..dfe18d14 100644 --- a/crates/rustynes-test-harness/tests/release_anchor_audit.rs +++ b/crates/rustynes-test-harness/tests/release_anchor_audit.rs @@ -112,6 +112,14 @@ fn workspace_package_field(key: &str) -> String { // Strip a trailing inline comment before unquoting, so // `version = "2.3.9" # pinned` yields `2.3.9` rather than // `2.3.9" # pinned`. (Review on #427.) + // + // Naive for a general TOML value -- it would truncate a string that + // legitimately contains `#`. Correct for every key this helper is + // asked for (`version`, `license`), and the function is private to + // this audit. A future caller wanting a `#`-bearing value needs a real + // parser, not a patch here; the reason a `toml` dependency is not + // pulled in for a documentation audit is that it would be the only + // thing this test target needs it for. let v = v.split('#').next().unwrap_or(v); return v.trim().trim_matches('"').to_owned(); } @@ -162,6 +170,37 @@ fn version_core(v: &str) -> &str { /// form. It is not one — the literal ends with `]` and a sub-table has `.` there /// — but the property is worth pinning rather than left to a reader spotting a /// closing bracket, and the parser now states it structurally. +/// Markdown emphasis between the version and its codename must not skip the +/// codename check, and a version-only anchor must still be skipped. +/// +/// Latent rather than live: no anchor is written `**v2.3.9** "Crucible"` today. +/// Pinned so a reformat cannot quietly reopen the hole. (Review on #427.) +#[test] +fn emphasis_between_version_and_codename_does_not_skip_the_check() { + // Would have been skipped before: emphasis, then the codename. + assert!(skip_to_codename("** \"Crucible\"").starts_with('"')); + assert!(skip_to_codename(" \"Crucible\"").starts_with('"')); + // The shapes that legitimately have no codename must STILL be skipped. + assert!(!skip_to_codename("** (2026-08-20)").starts_with('"')); + assert!(!skip_to_codename(" (the scheduling model is v2.0.0)").starts_with('"')); + assert!(!skip_to_codename("-blue.svg").starts_with('"')); +} + +/// A version at the end of a sentence must parse, not panic. +/// +/// `parse_version_prefix` consumed contiguous dots, so `v2.3.9.` yielded four +/// parts and returned `None` — which the caller turns into a panic. Fail-closed +/// is right for a missing marker; a full stop is not a missing marker. +#[test] +fn trailing_prose_punctuation_does_not_break_version_parsing() { + assert_eq!(parse_version_prefix("2.3.9.").as_deref(), Some("2.3.9")); + assert_eq!(parse_version_prefix("2.3.9...").as_deref(), Some("2.3.9")); + assert_eq!(parse_version_prefix("2.3.9").as_deref(), Some("2.3.9")); + // Still rejects things that are genuinely not a version. + assert_eq!(parse_version_prefix("2.3").as_deref(), None); + assert_eq!(parse_version_prefix("2.3.9.4").as_deref(), None); +} + #[test] fn sub_table_headers_do_not_match_the_workspace_package_table() { // The claim that was checked rather than accepted. @@ -286,6 +325,21 @@ const ANCHORS: &[Anchor] = &[ }, ]; +/// Skip whatever sits between a version and a codename that follows it. +/// +/// A named function rather than an inline `trim_start_matches`, so the test can +/// exercise THIS code instead of a copy of it. The first attempt asserted the +/// property against a local closure duplicating the same call — and a mutation +/// removing the production stripping went uncaught, because the test was never +/// looking at it. A test that reimplements what it checks is testing itself. +/// +/// Strips spaces and markdown emphasis. Both were fail-opens found by review on +/// #427: a second space, or `**v2.3.9** "Crucible"`, sent the codename check down +/// its legitimate "no codename here" path and silently skipped that anchor. +fn skip_to_codename(after_version: &str) -> &str { + after_version.trim_start_matches([' ', '*']) +} + /// Read a `MAJOR.MINOR.PATCH` starting at `s[0]`, stopping at the first /// character that cannot be part of one. /// @@ -295,7 +349,11 @@ fn parse_version_prefix(s: &str) -> Option { let end = s .find(|c: char| !c.is_ascii_digit() && c != '.') .unwrap_or(s.len()); - let v = &s[..end]; + // Trailing dots are prose punctuation, not version components. A sentence + // ending "...is at v2.3.9." would otherwise yield four parts, return `None`, + // and PANIC -- a false failure caused by a full stop. Fail-closed is right for + // a missing marker; it is not right for a period. (Review on #427.) + let v = s[..end].trim_end_matches('.'); let parts: Vec<&str> = v.split('.').collect(); (parts.len() == 3 && parts @@ -518,12 +576,26 @@ fn every_anchor_that_quotes_a_codename_quotes_the_changelog_codename() { if found != version_core(&version) { continue; // reported by the version test; do not double-report } - // A codename, when present, follows as ` "Name"` -- but the space - // count is a prose detail, not a contract. Matching exactly one space - // let a document write two and silently bypass the check while some - // OTHER anchor kept `checked > 0`, which is the same fail-open shape - // as the unterminated quote below. (Review on #427.) - let tail = text[at + found.len()..].trim_start_matches(' '); + // A codename, when present, follows as ` "Name"` -- but the exact + // punctuation between the version and the quote is a prose detail, + // not a contract. Two separate fail-opens were found here by review + // on #427, both the same shape: something the check did not expect + // sent it down `continue`, silently skipping that anchor's codename + // while another anchor kept `checked > 0`. + // + // * two spaces instead of one; + // * markdown emphasis, e.g. `**v2.3.9** "Crucible"`. + // + // NEITHER is live today -- every anchor currently reads + // `v2.3.9 "Crucible"` with a single space, and the emphasis case was + // checked against all ten documents rather than assumed. Both are + // latent, and a fail-open that waits for a reformat is exactly the + // kind this audit exists to remove. + // + // Stripping `*` as well as spaces keeps the version-only anchors + // working: `**v2.3.9** (2026-08-20)` strips to `(2026-08-20`, which + // is not a quote, so it still takes the legitimate `continue`. + let tail = skip_to_codename(&text[at + found.len()..]); let Some(rest) = tail.strip_prefix('"') else { continue; // this anchor states a version only -- legitimate };