fix(engine): commit every axis a bounded loop-shortcut offer publishes

[lgray]

🤖 AI text below 🤖

Summary

A bounded loop-shortcut offer could publish a candidate set that its own accept then failed to commit: the user's 4-player capture (MODE2) accepted a Fixed(n) grant and the board moved on no axis. Root cause is a sampler blind spot — entries announced across a forced pre-priority window never appeared in any settle frame's stack, so the announced set was incomplete and the period boundary was measured against a premise that no longer held. This widens the sampler to the forced-window answer beat, gives the period boundary and the "is this may already answered" gate one authority each, discharges a loop's replacement obligations against the live board, and pins both user captures as fixtures.

Files changed

• crates/engine/src/game/engine.rs — forced-window answer sampling site; published_period_elapsed authority
• crates/engine/src/analysis/resource.rs — period-boundary and homogeneity notes re-derived against the two-site premise
• crates/engine/src/game/effects/mod.rs — upfront_optional_gate, the single authority for whether a may is already answered
• crates/engine/src/game/replacement.rs — replacement obligations discharged against the live board
• crates/engine/src/types/game_state.rs — comment-only: record_loop_detect_sample and loop_detect_ring docs re-derived
• crates/engine/tests/integration/fantastic_four_bounded_loop.rs — fix-bar rows and axis assertions
• crates/engine/tests/integration/loop_shortcut.rs — rows whose premise was the sampler's blind spot
• crates/engine/tests/fixtures/f4_user_mode2_accept_commits_nothing_4p.json.gz — the user capture this PR exists to fix
• crates/engine/tests/fixtures/f4_user_mode1_no_offer_4p.json.gz — the sibling capture (no offer)

Track

Developer

LLM

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

Implementation method (required)

Method: /engine-implementer

CR references

CR 732.2a, CR 732.2b, CR 732.2c (shortcut conformance and the published certificate), CR 603.5 (optional-trigger gate), CR 603.3, CR 603.3b, CR 603.12a, CR 614.1, CR 616.1, CR 608.2b, CR 608.2c, CR 608.2d, CR 704.5a, CR 701.34a, CR 121.2, CR 122.1, CR 117.3a, CR 104.4b, CR 101.4.

One correction worth calling out: a pre-existing comment cited CR 614.1a for "scopes a definition to its controller's events". 614.1a is verbatim "Effects that use the word 'instead'", which cannot cover skips, enters-with, turned-face-up, or virtual replacements. Corrected to CR 614.1. The three remaining 614.1a mentions in this diff are the corrective notes themselves, not new claims.

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.

• cargo test -p phase-engine --lib — test result: ok. 18501 passed; 0 failed; 6 ignored; 0 measured; 0 filtered out; finished in 48.96s

• cargo test -p phase-engine --test integration — test result: ok. 4513 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 211.14s

• cargo clippy --workspace --all-targets -- -D warnings — EXIT=0, zero warning/error lines

• cargo fmt --all — EXIT=0

• fantastic_four_bounded_loop::a1_the_users_accept_committed_nothing_board_now_commits_on_every_axis — ok (the fix bar: the user's accepted Fixed(n) now commits on life, library, counters and tokens, each against the offer's own published per-cycle signature rather than a constant)

Baseline movement reconciled, not waved through. These totals sit above the pre-rebase baselines (lib 18487, integration 4487) purely because of upstream: added #[test]/#[tokio::test] across dcb8f3808..b654513cb is crates/engine/src +14/−0 and crates/engine/tests +26/−0 (#6999 lib+11/int+2, #6996 lib+3/int+15, #6946 int+5, #6997 int+4, #6998 and #7001 zero). lib +14 and integration +26 are fully attributed; zero unexplained movement.

Non-vacuity of the new assertions. Every new assertion and guard was revert-probed and flips alone, as a typed assertion failure rather than a harness crash: ablating the sampler widening reproduces the user's symptom exactly (left: 0, right: -1 on P0's library); bypassing the seat loop flips the recovered exact counter equality (left: 0, right: 2); zeroing each published rate fires the life, library and counter anti-vacuity guards individually.

Gate A

Gate A PASS head=d51d3b6e0055ee54e13185d4f49977e41508cac6 base=b654513cb391203fa1add4dbc797dc21aa9f429e

Stated honestly: the gate's scope is crates/engine/src/parser, and this PR changes 0 files there (9 files in range, all outside it). 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:6049 — the settle-beat record_loop_detect_sample() call, the pre-existing authority for recording a loop-detect frame. The new forced-window site (:11192) mirrors its conjunct set, differing only in that answering_forced_window replaces resolved_this_beat.
• crates/engine/src/game/effects/mod.rs:5844 — optional_effect_is_infeasible, the pre-existing single authority for "this optional effect cannot be taken". upfront_optional_gate (:6013) extends that same seam rather than adding a parallel check, and preserves all three of its arms.

Final review-impl

Final review-impl PASS head=d51d3b6e0055ee54e13185d4f49977e41508cac6

Claimed parse impact

None.

Scope Expansion

None.

Validation Failures

None.

CI Failures

None.

Summary by CodeRabbit

• Bug Fixes

  • Improved loop detection and bounded shortcut handling across more decision and replacement scenarios.
  • Optional choices are now offered only when eligible and feasible.
  • Stored accepted choices are correctly recognized during loop certification, while declines remain safely handled.

• Tests

  • Expanded coverage for multiplayer loops, replacement effects, target selection, feasibility checks, and captured-game scenarios.
  • Updated bounded-loop expectations and published decision handling.

• Documentation

  • Clarified loop-sampling behavior and replacement-candidate rules.

[coderabbitai]

Warning

Review limit reached

@lgray, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 12 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 483644e2-aa56-4271-92e5-e810d7903ec6

📥 Commits

Reviewing files that changed from the base of the PR and between acfeaf0 and a6d1a0e.

⛔ Files ignored due to path filters (2)

• crates/engine/tests/fixtures/f4_user_mode1_no_offer_4p.json.gz is excluded by !**/*.gz
• crates/engine/tests/fixtures/f4_user_mode2_accept_commits_nothing_4p.json.gz is excluded by !**/*.gz

📒 Files selected for processing (7)

• crates/engine/src/analysis/resource.rs
• crates/engine/src/game/effects/mod.rs
• crates/engine/src/game/engine.rs
• crates/engine/src/game/replacement.rs
• crates/engine/src/types/game_state.rs
• crates/engine/tests/integration/fantastic_four_bounded_loop.rs
• crates/engine/tests/integration/loop_shortcut.rs

📝 Walkthrough

Walkthrough

The PR centralizes optional-gate evaluation, recognizes stored Accept choices during certification, adds forced-window loop sampling, measures candidate spans, and updates bounded-loop tests for published decision slots and declared victim data.

Changes

Loop certification and bounded shortcut behavior

Layer / File(s)	Summary
Optional gate authority and certification relief crates/engine/src/game/effects/mod.rs, crates/engine/src/analysis/resource.rs, crates/engine/src/game/engine.rs, crates/engine/src/game/replacement.rs	Optional gate checks use upfront_optional_gate. Stored Accept choices provide certification relief, while Decline remains fail-closed. Resolution checks live replacement state when required.
Candidate windows and dual-site loop sampling crates/engine/src/game/engine.rs, crates/engine/src/analysis/resource.rs, crates/engine/src/types/game_state.rs	The engine shares newest-first candidate-window traversal, uses published_period_elapsed, and records samples at settle and forced-window answer sites.
Fantastic Four bounded-loop validation crates/engine/tests/integration/fantastic_four_bounded_loop.rs, crates/engine/src/analysis/resource.rs	Tests validate published decision points, declared victim damage, exact fixed-count commits, replacement definitions, and measured cycle scaling.
Published decision-slot shortcut tests crates/engine/tests/integration/loop_shortcut.rs	Tests use engine-published DecisionSlot values and distinguish bounded offers with non-empty target schemas from explicitly staged empty schemas.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GameEngine
  participant GameState
  participant ResourceAnalysis
  participant IntegrationTests
  GameEngine->>GameState: record settle or forced-window samples
  GameState-->>GameEngine: provide ring frames and period state
  GameEngine->>ResourceAnalysis: provide measured candidate windows
  ResourceAnalysis-->>GameEngine: certify bounded loop
  GameEngine->>IntegrationTests: publish decision slots and cycle signature
  IntegrationTests-->>GameEngine: submit accepted fixed-count grant

Loading

Possibly related PRs

• phase-rs/phase#6608: Both modify loop-shortcut recurrence and candidate handling.
• phase-rs/phase#6838: Both modify loop-ring retention and forced-window handling.
• phase-rs/phase#6886: Both extend bounded-loop, pin-validation, and loop-certification logic.

Suggested labels: needs-maintainer

Suggested reviewers: matthewevans

🚥 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 describes the main fix: committing every axis published by a bounded loop-shortcut offer.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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: 4

🧹 Nitpick comments (3)

crates/engine/tests/integration/fantastic_four_bounded_loop.rs (1)

764-773: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reuse accept_all_opponents in r2a instead of the inline Accept loop.

Lines 764-773 duplicate the body of the new accept_all_opponents helper at lines 169-184. The helper also returns the responder count, which lets this row assert that the CR 732.2c window really opened for at least one seat. Replacing the inline loop removes the duplicate and keeps the local use engine::analysis::loop_check::ShortcutResponse; import unnecessary here.

♻️ Proposed refactor

-        while let WaitingFor::RespondToShortcut { player, .. } = state.waiting_for.clone() {
-            apply(
-                &mut state,
-                player,
-                GameAction::RespondToShortcut {
-                    response: ShortcutResponse::Accept,
-                },
-            )
-            .expect("each living opponent accepts (CR 732.2c)");
-        }
+        let responders = accept_all_opponents(&mut state);
+        assert!(
+            responders > 0,
+            "n={n}: at least one living opponent must have answered the CR 732.2c window"
+        );

🤖 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/fantastic_four_bounded_loop.rs` around lines
764 - 773, In the r2a setup, replace the inline WaitingFor::RespondToShortcut
Accept loop with the existing accept_all_opponents helper, capture its returned
responder count, and assert that at least one opponent responded to confirm the
CR 732.2c window opened. Remove the now-unused local ShortcutResponse import if
it is only referenced by this loop.

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

10939-10957: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

A board with a naturally empty live schema already exists in this file.

The doc discloses the reachability downgrade correctly and names the remedy: "a fixture whose live offer publishes nothing". drain_offer_schema_is_empty_until_lethal at line 4091 already asserts exactly that on reach_2p_optional_drain_offer — its live schema's points is empty with no staging at all.

Consider building this row on that board instead of staging points: vec![] onto the R5 offer. That restores natural reachability for the !offer.schema.points.is_empty() skipped path and removes the disclosed downgrade. The owner firewall does not depend on R5's 4-player seat set, so the smaller board should serve.

If you prefer to keep the staged form, no change is needed — the disclosure and the matched positive already make the row honest about what it proves.

As per path instructions: test adequacy is the highest-frequency contributor finding, and a row must drive the engine through its production pipeline.

🤖 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 10939 - 10957,
Update r28_a_the_owner_firewall_is_reached_on_an_empty_schema_offer_too to use
the naturally empty live-schema fixture reach_2p_optional_drain_offer, following
the drain_offer_schema_is_empty_until_lethal setup, instead of staging an empty
schema through r28_empty_schema_offer. Preserve the matched honest-declaration
assertion while driving the owner firewall through the production pipeline with
no schema-point staging.

Source: Path instructions

---

11048-11056: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Re-derive this comment: the mutation it justifies is gone, but the decode dependency is real.

r5_reach_offer() produces a per_cycle: None ShortcutProposal, so the decode still depends on that precondition. Update the comment to say the row does not mutate per_cycle because the fixture already carries none, and assert proposal.per_cycle.is_none() before round-tripping if this row must guard against future populated proposals.

🤖 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 11048 - 11056,
The comment above the ingress fixture is stale and must reflect that
r5_reach_offer() already creates a ShortcutProposal with per_cycle set to None.
Update it to explain that the row preserves the existing per_cycle value rather
than nulling it, and add an assertion on proposal.per_cycle.is_none() before
round-tripping if this fixture should reject future populated proposals.

🤖 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/src/game/effects/mod.rs`:
- Around line 6006-6007: Update the documentation comment for the
UpfrontOptionalGate prompt_player field to replace the incorrect CR 117.3a
citation with the governing optional-effect resolution rule, such as CR 608.2d,
while preserving the clarification that optional_prompt_player identifies the
player making the choice rather than necessarily the controller.

In `@crates/engine/src/game/engine.rs`:
- Around line 2644-2668: Review entry_publishes_pin_slots and its
upfront_optional_gate Probe path to determine whether current fixtures contain
optional, non-repeat, non-optional_for CastFromZone entries. If they do, avoid
repeated deep-clone dry-runs from optional_effect_is_infeasible by memoizing
results per beat using stack-entry identity, or expose the cost separately as
appropriate; otherwise document or validate the fixture assumption without
changing unrelated behavior.

In `@crates/engine/tests/integration/fantastic_four_bounded_loop.rs`:
- Around line 1775-1782: Make the source-name resolution in the match handling
`YieldTarget::ThisObject` panic when `state.objects.get(source_id)` returns
`None` instead of synthesizing an `obj<id>` name. Preserve the existing
cloned-name behavior for resolvable objects and the panic for unexpected yield
targets, so negative assertions cannot pass with an unresolvable source.
- Around line 777-787: Update the assertion in the bounded-loop test to cast
each library count to i64 before subtraction, matching the safe ordering used by
accept_a_fixed_grant. Also retain and use the bound _certificate’s published
per-cycle delta to compute the expected life and library changes for
i64::from(n), instead of hard-coding one per repetition.

---

Nitpick comments:
In `@crates/engine/tests/integration/fantastic_four_bounded_loop.rs`:
- Around line 764-773: In the r2a setup, replace the inline
WaitingFor::RespondToShortcut Accept loop with the existing accept_all_opponents
helper, capture its returned responder count, and assert that at least one
opponent responded to confirm the CR 732.2c window opened. Remove the now-unused
local ShortcutResponse import if it is only referenced by this loop.

In `@crates/engine/tests/integration/loop_shortcut.rs`:
- Around line 10939-10957: Update
r28_a_the_owner_firewall_is_reached_on_an_empty_schema_offer_too to use the
naturally empty live-schema fixture reach_2p_optional_drain_offer, following the
drain_offer_schema_is_empty_until_lethal setup, instead of staging an empty
schema through r28_empty_schema_offer. Preserve the matched honest-declaration
assertion while driving the owner firewall through the production pipeline with
no schema-point staging.
- Around line 11048-11056: The comment above the ingress fixture is stale and
must reflect that r5_reach_offer() already creates a ShortcutProposal with
per_cycle set to None. Update it to explain that the row preserves the existing
per_cycle value rather than nulling it, and add an assertion on
proposal.per_cycle.is_none() before round-tripping if this fixture should reject
future populated proposals.

🪄 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: e1eaea0e-2895-4961-9936-ef3f0300ef87

📥 Commits

Reviewing files that changed from the base of the PR and between b654513 and d51d3b6.

⛔ Files ignored due to path filters (2)

• crates/engine/tests/fixtures/f4_user_mode1_no_offer_4p.json.gz is excluded by !**/*.gz
• crates/engine/tests/fixtures/f4_user_mode2_accept_commits_nothing_4p.json.gz is excluded by !**/*.gz

📒 Files selected for processing (7)

• crates/engine/src/analysis/resource.rs
• crates/engine/src/game/effects/mod.rs
• crates/engine/src/game/engine.rs
• crates/engine/src/game/replacement.rs
• crates/engine/src/types/game_state.rs
• crates/engine/tests/integration/fantastic_four_bounded_loop.rs
• crates/engine/tests/integration/loop_shortcut.rs

[github-actions]

Generated for head a6d1a0e62ad24dd2eb3447a88f6c7643f9e3c644.

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Changes requested

1. High — forced-window certificate records the wrong state. In game/engine.rs:11185-11194, the sampled frame is recorded before state.waiting_for = wf. When wf is WaitingFor::Priority, is_forced_cascade_window only checks the non-priority side and the equality check preserves the prior waiting_for, so the certificate can record a forced window even though the returned state is priority. Route the returned wf and paired priority state through the existing synchronization authority before recording the sample, then assert both that the sampled frame is Priority and that the immediately following certificate is the exact expected state.

2. Medium — the optional-choice prompt cites the wrong rule. effects/mod.rs:6006-6007 (and the matching optional_prompt_player documentation) cites CR 117.3a, which covers receiving priority at the start of a step or phase. An optional choice while resolving an effect is governed by CR 608.2d; the local rules text at docs/MagicCompRules.txt:2795 is the relevant authority. Please correct both annotations.

3. High — absence test can pass vacuously. The helper in fantastic_four_bounded_loop.rs:1775-1782 substitutes obj<ID> when the expected source is absent. That makes the negative Sue-absence assertion at :2035-2040 green without proving that the expected source was present. Require the expected source with expect/panic before constructing the assertion.

These are current-head findings; no unrelated clone-cost or rate concerns are part of this review.

[matthewevans]

Changes requested

[HIGH] Synchronize the forced-window state before recording the loop sample. Evidence: crates/engine/src/game/engine.rs:11185-11194 receives wf as WaitingFor::Priority but records a loop sample before synchronizing it into state.waiting_for; record_loop_detect_sample snapshots that stale state in crates/engine/src/types/game_state.rs:19527-19535, despite the canonical synchronizer in crates/engine/src/game/public_state.rs:40-44. Why it matters: the loop certificate/ring can claim a forced window while the returned state is actually priority. Suggested fix: call sync_waiting_for(state, &wf) before sampling, and add production-fixture assertions for the newest sampled state, its priority player, and the resulting certificate.

[MED] Refresh the parse-diff sticky for this exact head. Evidence: the current <!-- coverage-parse-diff --> comment identifies d51d3b6e0055ee54e13185d4f49977e41508cac6, while this PR head is 8c5261740aefab7faad1d2f64de85d05a3f0cb6c. Why it matters: the engine/effects changes after that artifact have no current-head parse-surface evidence. Suggested fix: refresh the sticky and ensure it explicitly binds to 8c5261740aefab7faad1d2f64de85d05a3f0cb6c.

[matthewevans]

Current-head correction for 8c5261740aefab7faad1d2f64de85d05a3f0cb6c: the <!-- coverage-parse-diff --> artifact is explicitly generated for this head and reports no card-parse changes, so the parse-artifact requirement is satisfied. The only remaining requested change is the existing HIGH forced-window issue: synchronize sync_waiting_for before recording the forced-window frame/certificate so the sample reflects the returned priority state.

[lgray]

🤖 AI text below 🤖

Withdrawal: three verification figures in the PR body are stale for the current head

Self-caught while applying a push-boundary integrity check. Withdrawing explicitly rather than editing silently, so the record shows what was wrong and why.

Current head is 8c5261740aefab7faad1d2f64de85d05a3f0cb6c. The PR body's Verification section is bound to d51d3b6e0055ee54e13185d4f49977e41508cac6, two commits back.

body claim	measured at	true at 8c5261740
--test integration → 4513 passed … 211.14s	7841e1e93 (.item1repin-int.log, 21:28 UTC)	4514 passed; 0 failed; 2 ignored … 186.40s (.crfix-gates.log, 00:10 UTC)
--lib → 18501 passed … 48.96s	7841e1e93	count still 18501, but the run is a different one (… 46.34s)
Gate A PASS head=d51d3b6e0… / Final review-impl PASS head=d51d3b6e0…	d51d3b6e0	not re-derived at 8c5261740

Cause. The integration count moved because the review-response commit 8c5261740 added a regression row (published_point_names_panics_when_a_points_source_is_absent, the fix for the vacuous by-name absence assertion). So 4513 → 4514 is the expected consequence of that fix, not a regression — but the body still advertises the pre-fix number under the post-fix SHA, and the two head-bound attestations name a SHA that is no longer the head.

Nothing here is a test failure. Every gate is green at the current head; the defect is that published figures outlived the head they were measured at.

Correction plan. The forced-window synchronization change requested in review is in progress. Rather than bind figures to a head that is about to move again, I will re-run the full battery, Gate A, and a fresh review-impl at the new head, and replace the Verification, Gate A and Final review-impl sections wholesale — each explicitly bound to that SHA. This comment stands as the withdrawal of the three claims above.

Process note, since it generalises. The check I was applying compares a gate log's timestamp against the base commit's committer date, which catches pre-rebase numbers published under a post-rebase SHA. It does not catch this case: these numbers postdate the base correctly and are stale anyway, because the branch head moved forward twice after they were taken. A verification figure has to be bound to the head it was measured at and re-measured whenever that head moves — a rebase is only one of the ways it moves.

[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 11287-11304: Update the loop-detection test around
dump_drive_one_beat to require exactly one ring-frame addition per beat, and
fail rather than continue when the ring length does not increase. Capture and
validate the newly added frame directly instead of relying only on
loop_detect_ring.back(), while preserving the existing answer_mints and
settle_mints attribution and reach-guard expectations. Avoid constructor
shortcuts or fallback setup that could mask failures in the frame-recording
behavior.

🪄 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: 48c41dbd-d1a5-4fa2-9844-a59659a87c82

📥 Commits

Reviewing files that changed from the base of the PR and between d51d3b6 and f144bb3.

📒 Files selected for processing (4)

• crates/engine/src/game/effects/mod.rs
• crates/engine/src/game/engine.rs
• crates/engine/tests/integration/fantastic_four_bounded_loop.rs
• crates/engine/tests/integration/loop_shortcut.rs

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

• crates/engine/src/game/effects/mod.rs
• crates/engine/src/game/engine.rs

[matthewevans]

Maintainer hold for current head a6d1a0e: re-review found no code finding. Required CI is still running (Rust lint and test shards), and the coverage-parse-diff sticky has not yet been generated for this head. Approval/enqueue consideration resumes after both settle.

[matthewevans]

Approved current head a6d1a0e: the forced-window sample uses canonical sync_waiting_for state before recording; the current-head parse artifact reports no card-parse changes; required checks are green.

[lgray]

🤖 AI text below 🤖

Record-keeping pointer, no action needed here.

Three review-response commits for this PR were still in review when it merged. A push of them raced the merge — the branch ref moved, but GitHub never adopted them because the PR was already closing, so they did not enter 90c525eb7. They are now open as #7023 against current main, byte-identical content, rebased.

They matter because main currently carries what they fix: the mechanism this PR cites for answer-beat frame homogeneity is wrong in three in-tree sites (ring_delta_signature reads neither waiting_for nor priority_player — the real sensitivity is loop_states_equal_modulo_resources via impl PartialEq for GameState), one of those sites being an assertion message that would print a false reason on failure; four citations name a pre-rebase SHA that no longer resolves; CodeRabbit's Major on the answer-beat row's frame attribution is unclosed; and the row's mint detector is missing the is_some() guard its production counterpart has.

Also worth recording against this thread rather than only in the follow-up: the sync fix landed here is causally inert on the current corpus. Instrumenting the pre-fix tree measured zero stale samples at either sampler site, with an always-true comparison of the same shape emitted on the same line to prove the instrument was live, and a pure revert of the reorder passes the row added with it. It is latent-hazard hardening, correct and worth having, but this PR could not demonstrate the defect failing and no fixture reaching the divergence was constructed.

Earlier in this thread I withdrew the Verification, Gate A and Final review-impl figures as detached from the head they described. They are re-measured and re-bound in #7023 rather than restated here, since the head they belong to is that one.