test(accuracy): sync AccuracyCoin to upstream ce61bb5 (holds 141/141) - #339
Merged
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (5)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe agy review script now fails closed on serialization errors and protects newly posted comments from cleanup. The AccuracyCoin main ROM and four sub-test ROM binaries were regenerated with updated test code and embedded data. Changesagy review workflow
AccuracyCoin ROM artifacts
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant agy-review.sh
participant flock
participant agy
participant GitHub API
agy-review.sh->>flock: acquire required process lock
flock-->>agy-review.sh: return lock status
agy-review.sh->>agy: run serialized review
agy-review.sh->>GitHub API: post comment after OAuth URL check
agy-review.sh->>GitHub API: delete older matching bot comments
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 `@scripts/agy-review.sh`:
- Around line 548-551: Validate new_comment_id after resolving the latest GitHub
Actions review comment and before issuing the cleanup gh api request. Require a
non-empty positive numeric ID; when validation fails, log the resolution failure
and skip deletion entirely, ensuring the subsequent filter cannot delete the
newly posted comment.
🪄 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: 00b82f6e-0c8e-4835-af34-5478c687d7fb
📒 Files selected for processing (6)
scripts/agy-review.shtests/roms/AccuracyCoin/sub-tests/dmc-bus-conflicts.nestests/roms/AccuracyCoin/sub-tests/iflag-latency.nestests/roms/AccuracyCoin/sub-tests/internal-data-bus.nestests/roms/AccuracyCoin/sub-tests/ppu-misc-2004-stress.nestests/roms/accuracycoin/AccuracyCoin.nes
Comment on lines
+548
to
+551
| new_comment_id="$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ | ||
| --jq '[.[] | select(.user.type == "Bot" and .user.login == "github-actions[bot]") | select(.body | contains("'"${MARKER}"'"))] | last | .id' 2>/dev/null)" | ||
| gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ | ||
| --jq ".[] | select(.user.type == \"Bot\" and .user.login == \"github-actions[bot]\") | select(.body | contains(\"${MARKER}\")) | .id" 2>/dev/null \ | ||
| --jq ".[] | select(.user.type == \"Bot\" and .user.login == \"github-actions[bot]\") | select(.body | contains(\"${MARKER}\")) | select(.id != ${new_comment_id:-0}) | .id" 2>/dev/null \ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate new_comment_id before cleanup.
If the first list request returns no matching comment, new_comment_id can be empty or null. The ${new_comment_id:-0} fallback then excludes no real comment. If the second request sees the newly posted comment, cleanup can delete it.
Require a positive numeric ID before deletion. If ID resolution fails, log the failure and skip cleanup so both the fresh and prior reviews remain available.
🤖 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/agy-review.sh` around lines 548 - 551, Validate new_comment_id after
resolving the latest GitHub Actions review comment and before issuing the
cleanup gh api request. Require a non-empty positive numeric ID; when validation
fails, log the resolution failure and skip deletion entirely, ensuring the
subsequent filter cannot delete the newly posted comment.
There was a problem hiding this comment.
Pull request overview
This PR updates the Antigravity (agy) PR-review helper script, tightening its concurrency guard (fail-closed locking) and reordering comment posting/de-duplication to reduce the risk of losing the last successful bot review.
Changes:
- Make
flock-based serialization fail-closed (missingflockor lock timeout now aborts instead of running unserialized). - Add an unconditional OAuth URL leak guard before any comment deletion/posting actions.
- Post the new PR comment first, then delete prior bot review comments (excluding the newly posted one).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+548
to
+551
| new_comment_id="$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ | ||
| --jq '[.[] | select(.user.type == "Bot" and .user.login == "github-actions[bot]") | select(.body | contains("'"${MARKER}"'"))] | last | .id' 2>/dev/null)" | ||
| gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ | ||
| --jq ".[] | select(.user.type == \"Bot\" and .user.login == \"github-actions[bot]\") | select(.body | contains(\"${MARKER}\")) | .id" 2>/dev/null \ | ||
| --jq ".[] | select(.user.type == \"Bot\" and .user.login == \"github-actions[bot]\") | select(.body | contains(\"${MARKER}\")) | select(.id != ${new_comment_id:-0}) | .id" 2>/dev/null \ |
Comment on lines
+414
to
+418
| # instead of failing. FAIL CLOSED: if flock is missing, or the lock can't be | ||
| # taken/times out, exit rather than let two agy processes race each other -- | ||
| # a fail-open here made the exact collision this lock exists to prevent still | ||
| # reachable (one run can burn the whole ${AGY_RETRIES}x${AGY_LOCK_WAIT}s wait). | ||
| command -v flock >/dev/null 2>&1 || { |
Re-sync the AccuracyCoin accuracy oracle (100thCoin/AccuracyCoin, MIT) to
the newest upstream, `ce61bb5` "Simplified Internal Data Bus error 3"
(2026-08-03), advancing the vendored suite 8 commits from `71f57fb`
(2026-06-26), and re-validate RustyNES against it. Four of the eight
upstream commits change test *behaviour* -- "Simplified Internal Data Bus
error 3", "Fixed a bug in Interrupt Flag Latency", "Updated DMC DMA Bus
Conflicts", and "Fixed an edge case in $2004 Stress Test"; the remaining
four are README typo/formatting fixes. The compiled ROM recompiled
(NROM, 32 KiB PRG + 8 KiB CHR, same 40976 B, new content hash).
Primary oracle ROM
------------------
`tests/roms/accuracycoin/AccuracyCoin.nes` is replaced with the new build
(sha256 verified identical to the freshly fast-forwarded reference clone).
The screen-grid + RAM-direct result decoders in
`crates/rustynes-test-harness/src/accuracy_coin*.rs` read the ROM's own
self-reported per-test results at runtime -- there is no memory-mapped
status protocol and no hard-coded expected-pass list -- so the new logic
is exercised without touching the harness.
Catalog is deliberately NOT regenerated
----------------------------------------
`tests/roms/AccuracyCoin/SOURCE_CATALOG.tsv` (the 146-entry
`suite<TAB>name<TAB>result_addr` source of truth the RAM-direct decoder
embeds via `include_str!`) is unchanged, because the ROM's test *structure*
is byte-identical across the update: the 189 `Suite_*` / `table "name",...`
declaration lines diff to empty between `71f57fb` and `ce61bb5`. Every test
name, suite membership, and `result_addr` is therefore stable (146 rows =
141 assigned tests + 5 "Power On State" print-only tests the upstream
display routine excludes from the result table). Only test logic changed,
not the catalog.
Regenerated single-test sub-test ROMs (4 of 26)
-----------------------------------------------
The custom sub-test ROMs are whole-battery builds with a boot wrapper that
jumps straight to one target test (`scripts/accuracycoin-build/
build_sub_test_rom.py`, which injects `LDY #suite / STY / JSR SetUpSuite /
JSR LoadSuiteMenuNoRendering / LDX #test / STX` into
`AutomaticallyRunEveryTestInROM`). Because a sub-test ROM embeds the entire
recompiled ROM, the four whose *target* test's logic changed are rebuilt
from `ce61bb5`:
- internal-data-bus.nes suite=19 (CPU Behavior 2) test=4
- iflag-latency.nes suite=11 (CPU Interrupts) test=0
- dmc-bus-conflicts.nes suite=12 (DMA Tests) test=6
- ppu-misc-2004-stress.nes suite=18 (PPU Misc.) test=6
The `(suite, test)` indices were recovered directly from each pre-existing
ROM's injected wrapper (byte pattern `A0 ss 84 .. 20 .. .. 20 .. .. A2 tt
86 ..`) rather than re-derived, guaranteeing each rebuild lands the exact
same test as before; the indices were then re-verified against the rebuilt
ROMs. These four feed only the read-only trace/debug binaries
(`trace_dmc_dma`, `trace_apu_reg_activation`, `trace_controller_strobing`,
`validate_sub_test_rom`) -- no `#[test]` or `insta` snapshot references
them, so there is nothing to re-bless. The other 22 sub-tests' target
tests are unchanged upstream and are intentionally left as-is.
Re-validation (unchanged: 141/141, 100.00%)
-------------------------------------------
`cargo test -p rustynes-test-harness --features test-roms --test
accuracycoin --test accuracycoin_runahead` passes (exit 0):
- RAM-direct decoder (authoritative): total=146, pass=130,
pass_with_code=11, fail=0, skipped=0, not_run=5 (the excluded Power On
State tests) => 141/141 assigned tests, 100.00%. Every one of the 20
suites reports zero failures.
- `accuracycoin_is_unaffected_by_run_ahead` holds -- the pass count is
bit-stable across the snapshot/restore run-ahead path.
The four upstream behaviour fixes regressed nothing; RustyNES still passes
all 141 assigned tests against the newest oracle.
The emulator core is byte-identical -- only the external oracle advanced --
so no CHANGELOG behaviour entry is warranted. The reference clone
`ref-proj/AccuracyCoin` (gitignored) was fast-forwarded locally and is not
part of this commit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doublegate
force-pushed
the
test/accuracycoin-sync-ce61bb5
branch
from
August 3, 2026 20:13
a09232f to
cd0c4dc
Compare
Antigravity review (Gemini via Ultra)This PR updates five binary Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Re-syncs the AccuracyCoin accuracy oracle (100thCoin/AccuracyCoin, MIT) to the newest upstream
ce61bb5"Simplified Internal Data Bus error 3" (2026-08-03) — 8 commits ahead of the vendored71f57fb(2026-06-26) — and re-validates RustyNES against it.Four upstream commits change test behaviour (Internal Data Bus, Interrupt Flag Latency, DMC DMA Bus Conflicts,
$2004Stress Test); the rest are README typos. The compiled ROM recompiled (NROM, 32 KiB PRG + 8 KiB CHR, new content hash).What changed
tests/roms/accuracycoin/AccuracyCoin.nes→ new build (sha256 == upstream). The harness reads the ROM's own self-reported results at runtime (no memory-mapped protocol, no hard-coded expected list), so the new logic is exercised with no harness change.SOURCE_CATALOG.tsvintentionally NOT regenerated — the ROM's test structure is byte-identical (the 189Suite_*/table "name",…declaration lines diff to empty), so all 146 catalog entries (141 assigned + 5 excluded Power-On-State) and everyresult_addrare stable. Only test logic changed.internal-data-bus,iflag-latency,dmc-bus-conflicts,ppu-misc-2004-stress) — the ones whose target test's logic changed. Rebuilt fromce61bb5viascripts/accuracycoin-build/build_sub_test_rom.py; the(suite,test)indices were recovered from each old ROM's injected boot-wrapper (guaranteeing the same test lands) and re-verified in the rebuilds. These feed only the read-only trace/debug bins — no#[test]orinstasnapshot references them, so nothing to re-bless. The other 22 sub-tests' targets are unchanged and left as-is.Validation
cargo test -p rustynes-test-harness --features test-roms --test accuracycoin --test accuracycoin_runahead→ pass (exit 0):accuracycoin_is_unaffected_by_run_aheadholds (bit-stable through run-ahead).The four upstream behaviour fixes regressed nothing — RustyNES still passes all 141 assigned tests against the newest oracle. Emulator core is byte-identical (only the external oracle advanced), so no CHANGELOG entry.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests