fix(engine): correct the answer-beat frame mechanism and harden its row (follow-up to #7005)

[lgray]

🤖 AI text below 🤖

Summary

Carries the review-response work for #7005 that missed its merge. #7005 was squash-merged at 90c525eb7 from head a6d1a0e62 while three follow-on commits were being reviewed; a push of those commits raced the merge and GitHub never adopted them, so they stranded on the fork branch. This PR is exactly that content, rebased onto current main.

It corrects false mechanism claims that main now carries, replaces orphaned commit-SHA citations, closes the CodeRabbit Major on the answer-beat row, and restores a guard the row's mint detector had dropped. Comments and one test row only — zero production behaviour change. The entire non-comment crates/engine/src/ delta is a single string literal (a line-anchored census pin).

Files changed

• crates/engine/src/game/engine.rs — the sampler comment re-cited to the correct basis; a stale ring-scan comment whose coordinates had rotted onto unrelated code, now naming symbols; census-pin literal
• crates/engine/src/analysis/resource.rs — a scope note on ring_delta_signature stating which fields it actually reads
• crates/engine/tests/integration/loop_shortcut.rs — answer-beat row: assertions widened over every frame a beat mints, mint detector guard restored, reach-guard asserted

Track

Developer

LLM

Model: claude-opus-5
Tier: Frontier
Thinking: high

Implementation method (required)

Method: /engine-implementer

CR references

None added, moved, or removed — git diff over the range matches no CR \d line in either direction. The CR numbers the touched hunks rely on were re-verified against the rules text regardless: CR 732.2a, CR 704.5a, CR 119.3, CR 110.1, CR 104.4b, CR 603.5, CR 122.1, CR 606.3, CR 608.2, CR 703.1/.2/.3, CR 500.8 — all resolve and describe the annotated code.

What main currently carries, and why this is worth a second PR

1. A false mechanism, in three in-tree sites. fix(engine): commit every axis a bounded loop-shortcut offer publishes #7005 claims a ring heterogeneous in waiting_for/priority_player breaks ring_delta_signature's turn-position conjunct. Measured, that function's body reads neither field — it compares ResourceVector::snapshot(&f.normalized) and window_scope_from_cover_frames(..).phase_invariant, and phase_invariant compares only turn_number, phase, extra_phases.is_empty(). The real sensitivity is loop_states_equal_modulo_resources applied to a ring frame's normalized snapshot, which chains to impl PartialEq for GameState. One of the three sites is an assertion message, so on failure it would print a false reason.

2. Citations of a commit that does not exist in this history. Four in-tree references named a pre-rebase SHA that no longer resolves from main; two sat in a permanent production comment. Under squash-merge no branch SHA survives, so they were pre-rotted. Replaced with the PR number.

3. An unclosed CodeRabbit Major. The answer-beat row derived site attribution from a ring-length delta, so a beat recording at both sampler sites would leave the row inspecting a settle frame while both arms passed. Fixed structurally — the arms now run over every frame the beat minted, which makes attribution irrelevant rather than more carefully asserted.

4. A dropped guard. The mint detector omitted the is_some() check its production counterpart has, so a ring clear would reach an unchecked usize subtraction and panic with an overflow message instead of the row's own explanation.

Verification

• Required checks ran clean, or the exact CI-owned alternative is stated below.
• Gate A output below is for the current committed head.
• Final review-impl below is clean for the current committed head.
• Both anchors cite existing analogous code at the same seam.

Measured at b90042d66e5badf3fa9837c3bd6c04488f984ad6, base a998c722413c8ec1364fd19fb8502d7f964fd0af, clean tree:

• cargo fmt --all — clean
• cargo clippy --workspace --all-targets -- -D warnings — EXIT=0
• cargo test -p phase-engine --lib — 18522 passed; 0 failed; 6 ignored
• cargo test -p phase-engine --test integration — 4558 passed; 0 failed; 2 ignored

Row-count movement fully attributed. The +2 integration rows are this branch's two new regression rows — git diff over the range adds 2 and removes 0 #[test]/#[tokio::test]. The last rebase moved the base across three upstream commits (a client change and two chores) which add and remove zero engine tests in both directions, so the count is unchanged by the rebase itself. Zero unexplained movement.

Rebase is content-preserving, verified: git range-diff pairs every commit with =, and the rebase correctly dropped #7005's squash and #7020 as already upstream.

Rebase is content-preserving, verified three ways: git range-diff pairs all three commits with =; all three touched files are byte-identical by sha256 across the rebase; and the only paths differing between the pre- and post-rebase tips belong to #7020, not to this branch.

Discrimination of the answer-beat row. Clobbering priority_player after the sync fails one arm (PlayerId(3) vs PlayerId(0)); clobbering the window fails the other (GameOver vs Priority{PlayerId(0)}), with the first arm passing before it, so the arms are separately live. Clobbering priority_player at the settle sampler fails at a non-forced beat, and the same mutant with the assertion narrowed back to forced-only passes — a two-sided control showing the widened scope is real added coverage rather than a restatement. Five inspected frames against two before the widening.

Gate A

Gate A PASS head=b90042d66e5badf3fa9837c3bd6c04488f984ad6 base=a998c722413c8ec1364fd19fb8502d7f964fd0af

The gate's scope is crates/engine/src/parser, and this PR changes 0 files there. The PASS is real but uninformative for this diff — it passes because no parser code is touched, not because parser code was checked.

Anchored on

• crates/engine/src/game/engine.rs:6050 — the settle-beat record_loop_detect_sample() call in pass_priority_once_with_pipeline, the pre-existing authority for recording a loop-detect frame. The widened assertions rest on both samplers sharing its Priority{player == active_player} conjunct and its record-after-sync_waiting_for ordering.
• crates/engine/src/game/effects/mod.rs:5844 — optional_effect_is_infeasible, the pre-existing single authority for "this optional effect cannot be taken", the seam upfront_optional_gate extends rather than parallels.

Final review-impl

Final review-impl PASS head=b90042d66e5badf3fa9837c3bd6c04488f984ad6

Stated precisely, because the honest answer is not one line. Three independent passes ran on the first three commits: a review of the first (BLOCK, on the false mechanism above), a re-review of the remedy, and a verification pass returning NON-BLOCKING with three LOW diagnostic findings. Rebases since have been measured content-preserving, so those passes bind to this head for that content.

The fourth commit — the fix for this PR's review — has not had a separate independent review pass. It was self-verified with per-assertion DROP/TRIVIALIZE mutant pairs and with both broken routes driven live through production apply(), and the maintainer's re-review is the intended next check. I would rather say that than let one PASS line imply a review that did not happen.

Claimed parse impact

None. No file under crates/engine/src/parser is touched.

Scope Expansion

None. This is #7005's own review-response content, unchanged.

Validation Failures

Three LOW diagnostic findings, disclosed rather than fixed. A circuit breaker was set before the last round so further non-blocking findings would be disclosed instead of chased; these are its output. None is a correctness defect and none is reachable on the current fixture.

1. A comment says "a cleared ring minted nothing, so the beat is skipped". Closed by the review-response commit — that comment and the code it described are gone; the clear-and-rebuild case now has its own row.
2. The reach-guard clause labelled "Stated exactly" is the one that is not exact — it drops the minting qualifier, since the increment sits after both continue guards. Still open.
3. The widened arms interpolate only the beat number, so on a multi-frame beat they cannot report which frame in the slice failed. Still open — measured at this head, no frame index is interpolated into either arm's message.

From the review-response commit, disclosed unprompted. The mutant harness initially reported two vacuous PASSes: --exact matches the full libtest path, so a bare function name filtered every test out and libtest still exited 0. It was fixed by qualifying the path and adding a ran() == 1 positive control that reports a setup error on zero tests, and every figure was re-measured under the corrected harness. Separately, one mutant misses its intended target — the doubled-push mutation steps the ring length past the capacity check, so it flips the eviction row's reach guard rather than its shape assertion; the shape assertion's drop arm is a 2-for-2 eviction instead. Both are recorded rather than quietly dropped. Also: the "no tracked fixture reaches clear-and-rebuild naturally" claim was surveyed over 4 dumps, not all 10, and no case was constructed in which an address-keyed snapshot actually aliases — the Arc-clone choice rests on the allocator being free to reuse an address, not on an observed collision.

Honest limits. The underlying sync fix that #7005 landed remains causally inert on the current corpus — instrumenting the pre-fix tree measured zero stale samples at either sampler, with an always-true comparison of the same shape on the same line proving the instrument live, and a pure revert of that reorder still passes. This PR hardens the row that guards it; it does not change that.

CI Failures

None.

Summary by CodeRabbit

• Documentation

  • Clarified technical documentation for cycle detection and synchronized action behavior.

• Tests

  • Expanded regression coverage for synchronized frames, ring eviction, and ring clearing.
  • Improved validation of waiting states, priority submissions, frame retention, and sampler attribution.
  • Added coverage for same-beat state rebuilding and priority-driven scenarios.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 21797cc5-b114-4d1b-ba76-fe811160aa3c

📥 Commits

Reviewing files that changed from the base of the PR and between b90042d and f2df77c.

📒 Files selected for processing (1)

• crates/engine/tests/integration/loop_shortcut.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/engine/tests/integration/loop_shortcut.rs

---

📝 Walkthrough

Walkthrough

The change clarifies loop-detection semantics and updates synchronized-window regression coverage. Tests identify every frame minted during a beat, including frames affected by eviction or ring rebuilding, and validate synchronized priority state.

Changes

Loop sampling semantics

Layer / File(s)	Summary
Loop detection semantics crates/engine/src/analysis/resource.rs, crates/engine/src/game/engine.rs	Comments distinguish ring_delta_signature inputs from sampler-enforced properties, clarify Basis A comparisons and synchronization behavior, and update scan and producer references.
Synchronized frame tracking crates/engine/tests/integration/loop_shortcut.rs	The test tracks retained frames by Arc identity and validates each frame’s synchronized waiting state and authorized priority submitter.
Ring mutation regressions crates/engine/tests/integration/loop_shortcut.rs	New regressions cover capacity eviction and frames discarded and reminted during a same-beat clear-and-rebuild operation.

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

Possibly related PRs

• phase-rs/phase#6608: Related loop-shortcut regression and recurrence validation.
• phase-rs/phase#6838: Related loop-ring behavior and test coverage.
• phase-rs/phase#6886: Related loop-shortcut detection and regression tests.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the engine change to the answer-beat frame mechanism and its hardened test coverage.
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.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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/engine/tests/integration/loop_shortcut.rs`:
- Around line 11274-11306: Replace the length-based mint detection in the beat
loop around dump_drive_one_beat with complete Arc membership tracking: snapshot
every pre-beat loop_detect_ring entry via Arc::as_ptr, then after the beat
identify and validate every remaining frame whose pointer was not present in
that snapshot. Remove the back_after/grew assertion and ensure append, capacity
eviction, and clear-and-rebuild paths all pass every newly minted frame to the
existing validation logic, preserving production failure-path coverage.

🪄 Autofix

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: CHILL

Plan: Pro Plus

Run ID: 6c380cd0-2eb2-4a3f-a086-7b11e3cf6250

📥 Commits

Reviewing files that changed from the base of the PR and between dd6ab09 and 483ce27.

📒 Files selected for processing (3)

• crates/engine/src/analysis/resource.rs
• crates/engine/src/game/engine.rs
• crates/engine/tests/integration/loop_shortcut.rs

[github-actions]

Generated for head f2df77c5d78b4b1db8bf521fa66d727a496bb267.

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Review head: 483ce27d3e211ad63771a750b856827144626c66

[MED] The regression test does not validate every frame it claims to validate. Evidence: crates/engine/tests/integration/loop_shortcut.rs:11283-11330 detects only a changed back pointer, derives grew from net ring length, and validates rev().take(grew); crates/engine/src/types/game_state.rs:19664-19672 evicts the front before pushing at capacity. Why it matters: an evicting push changes the back while preserving length, causing the row to fail instead of checking the newly minted frame; a clear-and-rebuild beat can also have positive net growth smaller than its minted-frame count, leaving some new frames unvalidated. Suggested fix: snapshot the complete pre-beat Arc::as_ptr membership, then validate every remaining post-beat sample whose pointer was absent before the beat; do not infer minted frames from length.

This confirms the still-current CodeRabbit finding at #7023 (comment).

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[lgray]

🤖 AI text below 🤖

@matthewevans — both mechanisms confirmed at source and fixed. Pushed as b90042d66 (rebased onto current main; CI is running).

Your [MED] was right, and understated in one respect. At capacity record_loop_detect_sample pops the front then pushes, so the back changes while the length does not — which meant assert!(grew >= 1) failed the test on a legitimate beat rather than leaving the frame unvalidated. I had labelled that guard "fail closed"; it was not. A safety label is earned by a census of the production paths that reach the trip condition, and mine trips on an ordinary drive once the ring fills.

Fix. back(), grew, saturating_sub and the assertion are deleted rather than re-derived. The minted set is now the ring's pre/post membership difference, snapshotted as Arc clones rather than addresses — pop_front drops the evicted allocation before Arc::new can claim one of identical layout, so an address-keyed snapshot can alias. A new reach guard requires the difference to be non-empty on every minting beat.

Both routes are now live on real fixtures through production apply(), not argued:

• Eviction at capacity — dellian_emblem_conqueror_4p, ordinary drive. First evicting beat is 72: ring 16→16, minted 1, dropped 1, net 0. Three more inside 90 beats.
• Clear-and-rebuild — dina_conqueror_4p plus one SetAutoPass{UntilStackEmpty} at the first priority window with two accumulated frames. Beat 6: ring 2→1, dropped 2, minted 1, net 0. That payload is the client's "Resolve All"; it is absent from legal_actions_for_viewer, which is why it has to be dispatched by name.
• The pre-existing row's own board reaches neither — max ring 5 against a capacity of 16, zero evicting and zero clearing beats. That is now stated in its doc instead of left implicit, which is the omission that let the original defect through.

Each assertion has a drop arm and a trivialize arm flipping that assertion, including reach witnesses for both new rows.

Two things worth your attention rather than buried in the body. The mutant harness first reported two vacuous passes — --exact matches the full libtest path, so a bare function name filtered every test out and libtest exited 0. Fixed with a qualified path plus a ran() == 1 positive control, and all figures re-measured. And one mutant misses its intended target: the doubled-push mutation steps the length past the capacity check, so it flips the eviction row's reach guard rather than its shape assertion; the shape arm is a 2-for-2 eviction instead. Recorded in the body rather than dropped.

Two of the three LOW diagnostic findings I disclosed earlier are still open and still disclosed; the third is closed by this commit. The PR body is re-measured and re-bound to this head.

[coderabbitai]

🧹 Nitpick comments (2)

crates/engine/tests/integration/loop_shortcut.rs (2)

11514-11527: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse ring_membership_delta in the answer-beat loop.

ring_membership_delta re-implements the same Arc::ptr_eq membership filter that the answer-beat loop already writes inline at Lines 11312-11316. The two copies can drift, and only one of them is exercised by the two new regressions.

Make the helper return the minted frames and derive the counts from it, so all three call sites share one definition.

♻️ Proposed shape

-fn ring_membership_delta<T>(
+fn minted_frames<'a, T>(
+    before: &[std::sync::Arc<T>],
+    after: &'a std::collections::VecDeque<std::sync::Arc<T>>,
+) -> Vec<&'a std::sync::Arc<T>> {
+    after
+        .iter()
+        .filter(|f| !before.iter().any(|b| std::sync::Arc::ptr_eq(b, f)))
+        .collect()
+}
+
+fn ring_membership_delta<T>(
     before: &[std::sync::Arc<T>],
     after: &std::collections::VecDeque<std::sync::Arc<T>>,
 ) -> (usize, usize) {
-    let minted = after
-        .iter()
-        .filter(|f| !before.iter().any(|b| std::sync::Arc::ptr_eq(b, f)))
-        .count();
+    let minted = minted_frames(before, after).len();
     let dropped = before
         .iter()
         .filter(|b| !after.iter().any(|f| std::sync::Arc::ptr_eq(b, f)))
         .count();
     (minted, dropped)
 }

As per coding guidelines, "reuse existing helpers instead of writing duplicate string or collection logic".

🤖 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 `@crates/engine/tests/integration/loop_shortcut.rs` around lines 11514 - 11527,
Update ring_membership_delta to return the minted Arc frames rather than only
counts, then derive minted and dropped counts from that shared result as needed.
Replace the answer-beat loop’s inline Arc::ptr_eq membership filtering around
the existing call sites with this helper, ensuring all three call sites use one
membership definition.

Source: Coding guidelines

---

11693-11730: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Bind the clear-and-rebuild witness to the dispatched beat.

The loop dispatches SetAutoPass on one beat, but the witness predicate at Line 11719 accepts any beat that lost every pre-beat frame. A future fixture change or an unrelated clearing beat can satisfy the predicate, and the assertion message at Lines 11743-11750 would then attribute the clear to a SetAutoPass press that did not happen on that beat.

Record whether the current beat is the dispatched one, and require that flag in the witness predicate. The row then proves the route it names instead of relying on the measured "0 clearing beats" property of this dump.

♻️ Proposed fix

-        let outcome = if !fired
+        let dispatched_here = !fired
             && before.len() >= 2
-            && matches!(state.waiting_for, WaitingFor::Priority { .. })
-        {
+            && matches!(state.waiting_for, WaitingFor::Priority { .. });
+        let outcome = if dispatched_here {
             fired = true;
@@
         let (minted, dropped) = ring_membership_delta(&before, &state.loop_detect_ring);
-        if before.is_empty() || dropped != before.len() {
+        if !dispatched_here || before.is_empty() || dropped != before.len() {
             continue;
         }

As per path instructions, flag constructor or witness shortcuts that can silently mask the very bug a regression test claims to catch.

🤖 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 `@crates/engine/tests/integration/loop_shortcut.rs` around lines 11693 - 11730,
Track whether the current loop beat is the one that dispatches SetAutoPass in
the outcome branch, and require that flag alongside the existing ring-clearing
conditions before assigning clearing. Update the beat loop around the
SetAutoPass dispatch and the clearing witness predicate so the recorded witness
can only refer to that dispatched beat, preserving the existing outcome handling
and assertions.

Source: Path instructions

🤖 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.

Nitpick comments:
In `@crates/engine/tests/integration/loop_shortcut.rs`:
- Around line 11514-11527: Update ring_membership_delta to return the minted Arc
frames rather than only counts, then derive minted and dropped counts from that
shared result as needed. Replace the answer-beat loop’s inline Arc::ptr_eq
membership filtering around the existing call sites with this helper, ensuring
all three call sites use one membership definition.
- Around line 11693-11730: Track whether the current loop beat is the one that
dispatches SetAutoPass in the outcome branch, and require that flag alongside
the existing ring-clearing conditions before assigning clearing. Update the beat
loop around the SetAutoPass dispatch and the clearing witness predicate so the
recorded witness can only refer to that dispatched beat, preserving the existing
outcome handling and assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 044aebe8-8074-4ffc-bc4c-2a9c36dd31bd

📥 Commits

Reviewing files that changed from the base of the PR and between 152b368 and b90042d.

📒 Files selected for processing (3)

• crates/engine/src/analysis/resource.rs
• crates/engine/src/game/engine.rs
• crates/engine/tests/integration/loop_shortcut.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• crates/engine/src/analysis/resource.rs
• crates/engine/src/game/engine.rs

[matthewevans]

Review head: f2df77c

The prior membership-delta finding and both current-head test-witness comments are resolved. The answer-beat validator, eviction row, and clear/rebuild row now share the Arc-identity minted-frame definition; the clear/rebuild witness additionally requires the exact beat that dispatched SetAutoPass UntilStackEmpty.

Re-review found no remaining implementation findings.