feat(harness): make the accuracy battery real (T-71-001/T-HARNESS-03) - #165
Conversation
The v0.8.0 cut criterion is "the battery hits its target pass rate", but the battery was `default_battery_stub()` — an empty probe set that `docs/STATUS.md` reported as "0% (battery stubbed)". `default_battery()` now scores every committed Angrylion RDP conformance vector: 35 probes named `rdp-conformance/<vector>`, each replayed through RustyN64's own RDP and compared byte-for-byte against the oracle's golden framebuffer. Every expected value is externally defined — never RustyN64's own output, which is the honesty constraint the plan sets for this battery. STATUS now reports a real 100% (35/35). An empty battery no longer scores 1.0. `AccuracyReport::ratio` returned "1.0 for an empty battery — vacuously green"; a battery that ran nothing has proven nothing, so it now returns 0.0. This is the vacuous-pass hazard the project already has a lesson about, sitting in the scorer itself. Added `AccuracyReport::failures()` so a regression names the offending vectors. New `conformance` module: the `.rvec` parse/replay moved out of the test file into `src/` so the conformance test and the battery share ONE implementation and ONE vector table (`RDP_VECTORS`). Previously a vector could be exercised by the test while being invisible to the battery; now `vector_bytes(name)` panics on an unknown name, so a typo fails loudly instead of silently skipping. Gates run locally: fmt, clippy -D warnings, cargo test --workspace, rustdoc, markdownlint, no_std cross-build (all green). 36/36 conformance tests and the 4 battery tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR introduces a shared ChangesRDP accuracy and conformance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AccuracyScorer
participant RDP_VECTORS
participant Conformance
participant RustyN64
participant OracleGolden
AccuracyScorer->>RDP_VECTORS: Iterate committed vectors
AccuracyScorer->>Conformance: Parse and replay vector
Conformance->>RustyN64: Load preload and command stream
RustyN64-->>Conformance: Render framebuffer
OracleGolden-->>Conformance: Provide golden framebuffer
Conformance-->>AccuracyScorer: Return match or first mismatch
Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@crates/rustyn64-test-harness/src/conformance.rs`:
- Around line 164-168: Replace the fixed n_words * 2 + 16 loop in the
conformance harness with polling for the architectural RDP completion state
before scoring output. Retain a safety bound only if it is empirically measured,
document its provenance in docs/accuracy-ledger.md, and add a regression vector
requiring more ticks than the former quota to verify incomplete output cannot be
scored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 15a8c799-6730-4a96-bd04-6671645fdda1
📒 Files selected for processing (6)
CHANGELOG.mdcrates/rustyn64-test-harness/src/accuracy.rscrates/rustyn64-test-harness/src/conformance.rscrates/rustyn64-test-harness/src/lib.rscrates/rustyn64-test-harness/tests/rdp_conformance.rsdocs/STATUS.md
Making `.rvec` parse/replay public put three previously-unreachable paths in reach of a directly-constructed `Vector` (all its fields are public), where before only `parse()`-validated vectors could get in: - `(width * height * bpp) as usize` multiplied in u32 BEFORE the cast, so an oversized geometry wrapped to a bogus framebuffer length instead of failing. Widen each operand first. - the RDRAM bounds assertion used raw `+`, which can overflow `usize` on a 32-bit host for an address near u32::MAX. Use `checked_add`. - `first_mismatch` iterated over `got.len()` while indexing `golden_fb`, so a length mismatch would panic out of bounds rather than being reported. Bound the scan by the shorter slice, which also makes the length-mismatch return properly reachable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Adjudication of the Antigravity review (accuracy battery, #165): Blocking — Suggestion — Suggestion — Nitpick — the fallback Good catch on all three — they were latent the moment this became a public API rather than test-file-internal. |
…d budget The replay drained the FIFO for `n_words * 2 + 16` ticks — an undocumented fitted constant with no proof the list had finished. A vector needing more ticks would have been scored from a partly rendered frame, silently invalidating an oracle result. That is the same class of defect as a vacuous pass, and it now sits under the accuracy battery. Drain instead until the FIFO is OBSERVABLY empty (`DPC_CURRENT >= DPC_END` — the very condition `Rdp::tick` no-ops on). The remaining cap is a liveness backstop only, and it ASSERTS rather than falling through, so a command that stalls the FIFO fails loudly instead of quietly producing a wrong comparison. All 36 conformance vectors and the battery still pass, which now also demonstrates that each list genuinely drains rather than being assumed to. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)Replaces the stubbed accuracy battery with a 35-probe suite backed by Angrylion RDP conformance vectors, changes empty battery scoring from 1.0 to 0.0, and moves Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
1 participant
Motivation
The v0.8.0 cut criterion is "the battery hits its target pass rate, and every
residual has a ledger entry". The second half is largely satisfied; the first half
could not be — the battery was
default_battery_stub(), an empty probe set, anddocs/STATUS.mdreported it as "0% (battery stubbed)". This makes it real.Changes
default_battery_stub→default_battery. Scores every committed AngrylionRDP conformance vector: 35 probes (
rdp-conformance/<vector>), each replayedthrough RustyN64's own RDP and compared byte-for-byte against the oracle's golden
framebuffer. Every expected value is externally defined — never our own output,
which is the honesty constraint the plan sets for this battery. STATUS now reports
a real 100% (35/35).
AccuracyReport::ratiodocumenteditself as returning "1.0 for an empty battery — vacuously green". A battery that
ran nothing has proven nothing, so it returns
0.0now. This is the exactvacuous-pass hazard the project keeps a lesson about, sitting in the scorer.
Added
failures()so a regression names the offending vectors.conformancemodule. The.rvecparse/replay moved from the test fileinto
src/, so the conformance test and the battery share one implementation andone vector table. Previously a vector could be exercised by the test while being
invisible to the battery;
vector_bytes(name)now panics on an unknown name, so atypo fails loudly instead of silently skipping a vector.
Guards against the battery going quietly hollow
default_battery_scores_every_committed_vector— one probe per table entry, andasserts a floor on the count.
every_committed_vector_parses/vector_names_are_unique— a shrunken orduplicated table fails.
empty_battery_scores_zero_not_one— pins the non-vacuous ratio.default_battery_matches_the_oracle— the 100% is asserted, and a regression inany RDP path fails here with the vector named.
Gates run locally
cargo fmt --all --check,cargo clippy --workspace --all-targets -D warnings,cargo test --workspace,RUSTDOCFLAGS=-D warnings cargo doc,pre-commit run markdownlint, and theno_stdcross-build — all green. 36/36 conformance testsplus the 4 battery tests pass.
🤖 Generated with Claude Code