Skip to content

feat(accuracysnes): ares as a third reference — and retract the A2.10 finding - #309

Merged
doublegate merged 1 commit into
mainfrom
feat/ares-third-reference
Aug 1, 2026
Merged

feat(accuracysnes): ares as a third reference — and retract the A2.10 finding#309
doublegate merged 1 commit into
mainfrom
feat/ares-third-reference

Conversation

@doublegate

@doublegate doublegate commented Aug 1, 2026

Copy link
Copy Markdown
Owner

The retraction is the important half

The plan recorded A2.10 — Mesen2 is the outlier, 2-vs-1 as the battery'''s only unexplained reference disagreement, and PR #304 said ares settled it 3-vs-1. Measured today:

A2.10 (catalogue index 11)
RustySNES pass
Mesen2 pass — failing set is F1.03 + F1.10, nothing else, identical on six runs
ares pass
snes9x FAIL code 1 — documented, the first entry in SNES9X_KNOWN_FAILURES

So the outlier is snes9x, the failure is already explained, and there was never a disagreement on this row. The old text also asserted "A2.10 is not among snes9x'''s 14 known divergences" — it is, at crossval.sh:80.

The original reading took a catalogue index off one host'''s output and attributed it to another, and nothing checked it because no per-row measurement existed. That is the second published claim in one day undone by keying on catalogue indices instead of row names. An index is only meaningful next to the host that produced it and the catalogue that was current, and neither travels with the number.

The ares host was built to settle a row that did not need settling. It still earned itself — it found a real bug in ares''' SMP (#308).

ares wired in as a third reference

snes9x: OK (14 known divergence(s))
Mesen2: OK (2 known divergence(s))
ares:   OK (5 known divergence(s))
cross-validation: 3 reference(s) agree with the cart

ARES_KNOWN_FAILURES=5 carries per-row rationale like the other two constants: C7.05/C7.10/F1.10 are rows snes9x already fails (ares corroborating it rather than standing alone), and E3.06/E8.02 cite the ares $F1 timer-2 reset bug.

Opt-in and skips cleanly. The host is a C++ link against ares''' static libraries and takes minutes, so the block looks for a pre-built binary at $ARES_HOST and prints skip ares: build it with '\''bash scripts/accuracysnes/ares_host/build.sh'\'' when absent. Verified in both states.

One bug caught on the way

Counting failing rows with ("0x" $3) %% 2 == 0 reported 337 failures. POSIX awk does not parse "0x01", so the expression is 0 %% 2 for every byte and every non-skipped row counted. A gate reporting catastrophe out of a parsing bug is a specific kind of dangerous, so the fix matches on the last hex digit and says why in a comment.

Verification

  • bash -n clean; crossval run with and without $ARES_HOST.
  • Mesen2 failing set: six runs, identical (F1.03, F1.10).
  • snes9x failing set: 14 rows, mapped through SOURCE_CATALOG.tsv, A2.10 among them.

🤖 Generated with Claude Code

Adds optional ares cross-validation to AccuracySNES and corrects the A2.10 dossier assertion.

The claim is that RustySNES, Mesen2, and ares pass A2.10, while snes9x fails it as a documented divergence. The claim is false if any of those pass/fail results or the catalogue-index attribution is incorrect. The assertion must also be reconsidered if ares reports one of its five documented per-row exceptions.

The change records ares-specific known failures and skips ares when $ARES_HOST is unavailable. It does not change the AccuracySNES test-row coverage denominator. It changes the available reference-emulator count when ares runs.

The change fixes POSIX awk failure counting for hexadecimal byte strings. The defect triggered when awk interpreted hexadecimal text as numeric input and reported 337 failures instead of matching the final hexadecimal digit.

No emulator-core observable behavior changes.

… finding

RETRACTION, and it is the important half. The plan recorded "A2.10 -- Mesen2
is the outlier, 2-vs-1" as the battery's only unexplained reference
disagreement, and PR #304 said ares settled it 3-vs-1. Measured:

  RustySNES  pass
  Mesen2     PASS   (its failing set is F1.03 and F1.10, nothing else,
                     identical on six runs)
  ares       pass
  snes9x     FAIL code 1 -- and documented, the FIRST entry in
                     SNES9X_KNOWN_FAILURES

So the outlier is snes9x, the failure is already explained, and there was
never a disagreement on this row. The old text also asserted "A2.10 is not
among snes9x's 14 known divergences"; it is, at crossval.sh:80. The original
reading took a catalogue index off one host's output and attributed it to
another, and nothing checked it because no per-row measurement existed.

That is the second published claim in one day undone by keying on catalogue
indices instead of row names. An index is only meaningful next to the host
that produced it and the catalogue that was current, and neither travels with
the number.

The ares host was therefore built to settle a row that did not need settling.
It still earned itself: it found a real bug in ares' SMP.

ares is now wired in as a third reference. cross-validation reports
"3 reference(s) agree with the cart". ARES_KNOWN_FAILURES=5 carries per-row
rationale like the other two constants -- C7.05/C7.10/F1.10 are rows snes9x
already fails, E3.06/E8.02 cite the ares $F1 timer-2 reset bug. The block is
opt-in and skips cleanly when the binary is absent, verified in both states.

One bug caught on the way: counting failures with ("0x" $3) % 2 == 0 reported
337 failures, because POSIX awk does not parse "0x01" and the expression is
0 % 2 for every byte. A gate reporting catastrophe out of a parsing bug is a
specific kind of dangerous; the fix matches the last hex digit and says why.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 1, 2026 15:04
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The change adds optional ares cross-validation with known-failure counting and unavailable-host handling. It also corrects the A2.10 documentation and records the separate ares timer-2 bug.

Changes

ares cross-validation

Layer / File(s) Summary
Optional ares execution and failure validation
scripts/accuracysnes/crossval.sh, CHANGELOG.md
crossval.sh configures and runs an executable ares host, excludes skipped rows, counts failures with POSIX awk, validates the configured count of five known failures, and reports unavailable hosts.
A2.10 result corrections
docs/accuracysnes-plan.md, scripts/accuracysnes/ares_host/README.md, CHANGELOG.md
The documentation records that RustySNES, Mesen2, and ares pass A2.10, while snes9x fails it. The separate ares timer-2 bug is retained.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant crossval.sh
  participant ares_host
  participant awk
  crossval.sh->>ares_host: invoke executable host
  ares_host-->>crossval.sh: return status output
  crossval.sh->>awk: count non-skipped even status bytes
  awk-->>crossval.sh: return failure count
  crossval.sh->>crossval.sh: validate count and update run status
Loading

Possibly related PRs

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 10
✅ Passed checks (10 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses a valid Conventional Commits format, describes both main changes, uses an imperative phrase, and has no trailing period.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Changelog Entry ✅ Passed The full PR diff against origin/main includes a CHANGELOG.md Unreleased entry documenting the new optional ares cross-validation behavior and output.
Docs-As-Spec ✅ Passed The base diff changes only CHANGELOG/docs and crossval.sh. It contains no changes under crates/rustysnes-/, so the docs-as-spec condition is not triggered.
Accuracysnes Bookkeeping ✅ Passed The PR changes only CHANGELOG.md, docs, and scripts; no path under tests/roms/AccuracySNES/gen/src/ changed, so the bookkeeping checks do not apply.
No Panic On Untrusted Input ✅ Passed The PR adds only Markdown and shell changes; the added lines contain no .unwrap(), .expect(), or panic!(), and no new Rust input boundary.
Safety Comment On New Unsafe ✅ Passed The patch changes only Markdown and shell files; the added lines contain no unsafe { ... } blocks or unsafe fn declarations, so no SAFETY comment is required.

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

Antigravity review (Gemini via Ultra)

This PR integrates ares as a third reference emulator into crossval.sh with five documented known failures and retracts the previously published finding attributing test A2.10 divergence to Mesen2.

Blocking issues

None found.

Suggestions

  • scripts/accuracysnes/crossval.sh:313: Redirecting stderr with 2>/dev/null during $ARES_HOST execution suppresses diagnostic messages. If the host binary fails to load shared libraries or crashes, awk receives empty input, outputs 0, and reports ares: 0 failing test(s), expected 5 instead of flagging host execution failure. Consider removing 2>/dev/null and validating that tests actually ran before matching output counts.
  • scripts/accuracysnes/crossval.sh:315: The pattern matching $3 != "ff" && $3 != "00" is case-sensitive and assumes non-prefixed hex bytes. If $ARES_HOST outputs uppercase hex or 0x-prefixed strings (e.g., 0x00 for un-run tests), "0x00" will bypass $3 != "00" and match /[02468aceACE]$/, miscounting un-run tests as failures. Use tolower($3) or strip optional 0x prefixes before checking parity.

Nitpicks

  • PR title feat(accuracysnes): ares as a third reference — and retract the A2.10 finding is 75 characters (exceeding the 72-character limit in the style guide) and contains non-ASCII punctuation ().
  • The commit combines two distinct logical changes: adding the ares cross-validation host and revising documentation for the A2.10 finding retraction.

Automated first-pass review by agy on a self-hosted runner -- not a human review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the AccuracySNES cross-validation workflow to include ares as an optional third reference emulator, and corrects prior documentation/notes by retracting the earlier A2.10 “Mesen2 outlier” claim after re-measurement.

Changes:

  • Add an opt-in ares headless host check to crossval.sh, with ARES_KNOWN_FAILURES=5 and a safer awk-based failure counter.
  • Update docs and changelog to reflect the A2.10 retraction and the rationale for ares’ known divergences.
  • Revise the ares host README to clarify that its original motivating “A2.10 settlement” premise was incorrect, while preserving the value of the ares integration.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
scripts/accuracysnes/crossval.sh Adds an opt-in ares reference run and documents ares’ known divergences and counting logic.
scripts/accuracysnes/ares_host/README.md Updates the motivation narrative and documents what the ares host established/found.
docs/accuracysnes-plan.md Retracts the previous A2.10 arbitration writeup and replaces it with measured, attributed results.
CHANGELOG.md Records the ares third-reference integration and the A2.10 retraction in Unreleased notes.

Comment thread docs/accuracysnes-plan.md

So the outlier is **snes9x**, not Mesen2; the failure is expected and already explained; and there is
no unexplained disagreement on this row at all. The old text additionally asserted "`A2.10` is not
among snes9x's 14 known divergences" — it is, at `crossval.sh:80`.
Comment on lines +222 to +227
# --- ares' own known divergences -----------------------------------------------------------------
#
# ares is the THIRD reference, added 2026-08-01 (`scripts/accuracysnes/ares_host/`). It matters
# because this project's provenance rule counts ares and bsnes as ONE reference — so any row where
# RustySNES and snes9x face Mesen2 is only 2-vs-1 if ares is not already on RustySNES's side, and
# before this there was no way to check.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/accuracysnes/crossval.sh`:
- Around line 254-255: Replace the loose ARES_KNOWN_FAILURES count check in the
ares validation block with structured validation of captured host output:
require a successful host exit status, a complete ACCURACYSNES result block with
valid magic and done markers, and a declared count of five matching the exact
documented (row, status) pairs. Store those expected pairs as data and reject
missing, extra, reordered, or unexpected-index failures, as well as truncated or
malformed output.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d6afc961-70d7-488c-88b1-af174c78f5c8

📥 Commits

Reviewing files that changed from the base of the PR and between 121f666 and 6561a0c.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • docs/accuracysnes-plan.md
  • scripts/accuracysnes/ares_host/README.md
  • scripts/accuracysnes/crossval.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
  • GitHub Check: test-light
  • GitHub Check: accuracysnes
  • GitHub Check: lint
  • GitHub Check: copilot-pull-request-reviewer
  • GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{rs,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Chip-behavior changes must update both the chip implementation and the corresponding docs/<subsystem>.md documentation.

A chip change must update both the chip implementation and its corresponding docs/<chip>.md documentation in the same change.

Files:

  • scripts/accuracysnes/ares_host/README.md
  • docs/accuracysnes-plan.md
  • CHANGELOG.md
**/*

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*: Do not commit or vendor the generated snesdev_wiki/ mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits: <type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevant to-dos/ sprint file.

**/*: Preserve the one-directional crate graph: chip crates must not depend on one another; rustysnes-core ties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keep docs/STATUS.md as the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNES v2.0 or engine-lineage anchors as project releases.

Files:

  • scripts/accuracysnes/ares_host/README.md
  • scripts/accuracysnes/crossval.sh
  • docs/accuracysnes-plan.md
  • CHANGELOG.md
scripts/accuracysnes/**

⚙️ CodeRabbit configuration file

scripts/accuracysnes/**: The cross-validation harness: the same AccuracySNES image is run on snes9x (through a
libretro host in C) and on Mesen2 (through its test runner and a Lua script), and their
verdicts are compared with the cart's. Its integrity is the whole argument for the
battery, so flag anything that could make a reference appear to agree — a verdict parsed
loosely, a missing-file path that degrades to success, a scene comparison that skips
rather than fails when the golden is absent. A known reference divergence belongs in
SNES9X_KNOWN_FAILURES with a source citation, never in a widened match.

Files:

  • scripts/accuracysnes/ares_host/README.md
  • scripts/accuracysnes/crossval.sh
**/*.md

⚙️ CodeRabbit configuration file

**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).

Files:

  • scripts/accuracysnes/ares_host/README.md
  • docs/accuracysnes-plan.md
  • CHANGELOG.md
docs/**/*.md

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Before changing a subsystem, consult docs/architecture.md, docs/STATUS.md, CONTRIBUTING.md, the relevant subsystem documentation, and applicable ADRs.

New subsystems must add documentation under docs/.

Files:

  • docs/accuracysnes-plan.md
docs/**/*

📄 CodeRabbit inference engine (docs/testing-strategy.md)

Chip crates should exceed 90% unit-test coverage, and each chip should be fuzzable in isolation.

Files:

  • docs/accuracysnes-plan.md
docs/**

⚙️ CodeRabbit configuration file

docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generated docs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.

Files:

  • docs/accuracysnes-plan.md
CHANGELOG.md

📄 CodeRabbit inference engine (CONTRIBUTING.md)

User-visible changes must be recorded under the [Unreleased] section.

For the full pull request diff against its base branch, modify CHANGELOG.md when user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.

Files:

  • CHANGELOG.md
🧠 Learnings (2)
📚 Learning: 2026-07-21T01:34:22.909Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 189
File: docs/accuracysnes-plan.md:0-0
Timestamp: 2026-07-21T01:34:22.909Z
Learning: When reviewing the AccuracySNES documentation in docs/accuracysnes-*.md (notably docs/accuracysnes-plan.md vs the generated docs/accuracysnes-coverage.md), treat the reported metrics as intentionally non-equivalent: the battery test count and dossier assertion coverage are not interchangeable. Do not infer one count/coverage from the other during review (e.g., one test may contain multiple assertions, and multiple tests may contribute to a single assertion/row such as E6.02).

Applied to files:

  • docs/accuracysnes-plan.md
📚 Learning: 2026-07-21T05:22:58.848Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 197
File: docs/accuracysnes-plan.md:598-600
Timestamp: 2026-07-21T05:22:58.848Z
Learning: In the AccuracySNES documentation under `docs/`, when an assertion exists in the research dossier but cannot be measured/verified by the current cartridge timing test, distinguish the dossier assertion from test measurability: keep the original hardware assertion (and any contribution to the coverage denominator) intact, withdraw/stop using the specific test coverage only if the sources cannot decompose the required CPU-cycle timing into bus vs internal components, and mark the row as not measurable using the `[NOT CART-MEASURABLE ...]` annotation with links to the corresponding plan section (e.g., `docs/accuracysnes-plan.md` §A5.20) and the related roadmap/ticket (e.g., `to-dos/ROADMAP.md` ticket `T-06-A`). Ensure the documentation/coverage reporting treats the row as uncovered rather than removing or redefining the assertion.

Applied to files:

  • docs/accuracysnes-plan.md
🔇 Additional comments (3)
CHANGELOG.md (1)

14-29: LGTM!

Also applies to: 82-90

docs/accuracysnes-plan.md (1)

842-875: LGTM!

scripts/accuracysnes/ares_host/README.md (1)

28-34: LGTM!

Comment on lines +254 to +255
ARES_KNOWN_FAILURES=5

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Validate the complete ares result and exact failure rows.

The block accepts ares when n == ARES_KNOWN_FAILURES, but n comes from a loose suffix match. Five lines such as status 0 bogus2, five failures at unexpected indices, or truncated output with five matching status lines produce n=5 and print ares: OK. The block also ignores the result markers, magic, done, declared count, indexed row set, and the host exit status. If a failure moves from a documented row such as A2.10 while the total remains five, the gate still passes.

Capture the host output. Check the host exit status and the complete ACCURACYSNES result block. Compare the declared status count and the exact (row, status) set for the five documented ares divergences. Store those expectations as data, not only comments.

As per path instructions, the cross-validation harness must not accept a loosely parsed verdict that can make a reference appear to agree.

Also applies to: 314-322

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/accuracysnes/crossval.sh` around lines 254 - 255, Replace the loose
ARES_KNOWN_FAILURES count check in the ares validation block with structured
validation of captured host output: require a successful host exit status, a
complete ACCURACYSNES result block with valid magic and done markers, and a
declared count of five matching the exact documented (row, status) pairs. Store
those expected pairs as data and reject missing, extra, reordered, or
unexpected-index failures, as well as truncated or malformed output.

Source: Path instructions

@doublegate
doublegate merged commit d75a8ed into main Aug 1, 2026
17 checks passed
@doublegate
doublegate deleted the feat/ares-third-reference branch August 1, 2026 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants