fix(engine): show the ∞ badge while a loop collapse is merely scheduled - #7002
fix(engine): show the ∞ badge while a loop collapse is merely scheduled#7002lgray wants to merge 4 commits into
Conversation
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughScheduled unbounded resources, piles, and counters remain visible until deferred growth is applied at the phase boundary. Engine integration tests and client wire seam tests cover token, counter, resource, pile, and mana projections. ChangesScheduled collapse projections
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ShortcutAcceptance
participant DerivedViews
participant ClientWire
participant PhaseBoundary
ShortcutAcceptance->>DerivedViews: record deferred growth and scheduled axes
DerivedViews->>ClientWire: emit unbounded resources, piles, and counters
ClientWire-->>DerivedViews: decode and select projected data
PhaseBoundary->>DerivedViews: apply deferred growth and clear scheduled state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine/tests/integration/combo_infinite_pile.rs (1)
313-325: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAssert
scheduled_collapseafter the state reload.The test only reprojects
unbounded_pileafter deserialization. Ifpending_unbounded_materializationfailed to persist, this assertion would still pass whilederive_views(&reloaded, ...)omitted the newTokensCreatedscheduled tag. Assert that the reloaded view contains P0’sTokensCreatedentry inscheduled_collapse.Proposed test check
- let reloaded_set: BTreeSet<ObjectId> = derive_views(&reloaded, Some(P0)) + let reloaded_views = derive_views(&reloaded, Some(P0)); + let reloaded_set: BTreeSet<ObjectId> = reloaded_views .unbounded_pile .iter() .copied() .collect(); @@ assert_eq!(reloaded_set, oracle, ...); + assert!(reloaded_views.scheduled_collapse.iter().any(|row| { + row.player == P0 && row.axis == ResourceAxis::TokensCreated + }));As per path instructions, cover the full engine-to-wire-to-client path for player-visible fields.
🤖 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/combo_infinite_pile.rs` around lines 313 - 325, Extend the post-reload assertions in the test around reloaded_set to inspect the reloaded derive_views result’s scheduled_collapse and assert it contains P0’s TokensCreated entry. Reuse the same reloaded view rather than deriving it separately, while preserving the existing unbounded_pile oracle comparison.Source: Path instructions
crates/engine/tests/integration/loop_shortcut.rs (1)
7326-7336: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAssert scheduled tags through
derive_filtered_views.
derive_filtered_viewscallsderive_views(filtered_state, viewer). If filtering removes onlypending_unbounded_materialization, the current assertions still pass because the rows and pile remain present, but remote views losescheduled_collapse. Assert both P0LifeandTokensCreatedtags for every filtered viewer.Proposed test check
assert!( !views.unbounded_resources.is_empty() && !views.unbounded_pile.is_empty(), ... ); + assert!( + views.scheduled_collapse.iter().any(|row| row.axis == ResourceAxis::Life(P0)) + && views.scheduled_collapse.iter().any(|row| { + row.axis == ResourceAxis::TokensCreated + }), + "the viewer-FILTERED broadcast path preserves scheduled-collapse tags" + );As per path instructions, new player-visible engine fields require symmetric adapter-path coverage.
🤖 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 7326 - 7336, Update the assertions in the filtered-view loop around derive_filtered_views to verify that every viewer’s P0 row retains both the Life and TokensCreated scheduled tags, in addition to the existing non-empty checks. Assert these tags on the filtered derived view so the test covers scheduled-tag propagation through the adapter path.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.
Inline comments:
In `@client/src/adapter/types.ts`:
- Around line 2870-2883: Replace the incorrect CR 732.2c citation in the field
documentation near the scheduled-collapse type with a verified citation that
actually describes deferred unbounded growth, scheduled finite collapse, and CR
500.5 materialization; update
client/src/viewmodel/__tests__/unboundedWireSeam.test.ts lines 1-6 to use the
same corrected citation.
In `@client/src/viewmodel/__tests__/unboundedWireSeam.test.ts`:
- Around line 44-46: Extend the omit-when-empty coverage in the unbounded wire
seam test to include an engine-emitted empty scheduled-collapse fixture, then
assert the client-facing wire result omits the scheduled_collapse field. Ensure
the assertion would fail if the engine serialized scheduled_collapse as an empty
array, while preserving the existing unbounded_pile and unbounded_counters
checks.
In `@crates/engine/src/game/derived_views.rs`:
- Around line 815-845: Update the shortcut acceptance flow, including the logic
leading to GameState::scheduled_collapse_axes, to apply the proposal’s declared
iterations immediately when the final player accepts it, advancing the game to
the proposed ending point before exposing another priority state. Remove the
deferred-growth behavior and ensure loop_shortcut.rs no longer observes
WaitingFor::Priority between acceptance and endpoint resolution, while
preserving normal handling for shortcuts that are not fully accepted.
---
Outside diff comments:
In `@crates/engine/tests/integration/combo_infinite_pile.rs`:
- Around line 313-325: Extend the post-reload assertions in the test around
reloaded_set to inspect the reloaded derive_views result’s scheduled_collapse
and assert it contains P0’s TokensCreated entry. Reuse the same reloaded view
rather than deriving it separately, while preserving the existing unbounded_pile
oracle comparison.
In `@crates/engine/tests/integration/loop_shortcut.rs`:
- Around line 7326-7336: Update the assertions in the filtered-view loop around
derive_filtered_views to verify that every viewer’s P0 row retains both the Life
and TokensCreated scheduled tags, in addition to the existing non-empty checks.
Assert these tags on the filtered derived view so the test covers scheduled-tag
propagation through the adapter path.
🪄 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: c39221bd-76ef-4bdc-8331-e16a24f244e3
📒 Files selected for processing (10)
client/src/adapter/types.tsclient/src/test/fixtures/unbounded-counter-wire.jsonclient/src/test/fixtures/unbounded-token-wire.jsonclient/src/viewmodel/__tests__/unboundedWireSeam.test.tscrates/engine/src/game/derived_views.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/combo_infinite_pile.rscrates/engine/tests/integration/kilo_live_offer_from_real_dump.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rs
| // (3) omit-when-empty, engine-attested in BOTH directions. | ||
| expect("unbounded_pile" in counterWire).toBe(false); | ||
| expect("unbounded_counters" in tokenWire).toBe(false); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Test the omitted scheduled_collapse wire form.
These assertions prove omission only for unbounded_pile and unbounded_counters. Both new fixtures include scheduled_collapse. Add an engine-emitted empty fixture and assert that the client receives no scheduled_collapse field. This must fail if the engine emits scheduled_collapse: [] instead of omitting the optional field.
As per path instructions, omitted optional fields require full engine-to-wire-to-client coverage.
🤖 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 `@client/src/viewmodel/__tests__/unboundedWireSeam.test.ts` around lines 44 -
46, Extend the omit-when-empty coverage in the unbounded wire seam test to
include an engine-emitted empty scheduled-collapse fixture, then assert the
client-facing wire result omits the scheduled_collapse field. Ensure the
assertion would fail if the engine serialized scheduled_collapse as an empty
array, while preserving the existing unbounded_pile and unbounded_counters
checks.
Source: Path instructions
| // CR 732.2c (docs/MagicCompRules.txt:6394) says that once the last player has accepted, the | ||
| // shortcut IS taken and the game advances to the ending point the proposal named. THIS ENGINE | ||
| // DEFERS that: the growth is not applied at accept, it is parked until the next CR 500.5 | ||
| // boundary (docs/MagicCompRules.txt:2122), where `turns.rs:540-591` asks the controller to | ||
| // CHOOSE N in `[0, recorded bound]`. `turns.rs:553-556` documents that deferral in its own | ||
| // words as an engine tolerance no CR licenses. | ||
| // | ||
| // Filter the PROJECTION, never the store: `unbounded_resources` + | ||
| // `unbounded_loop_enablers` stay in CR 104.4b / CR 110.1 lockstep until the CR 500.5 | ||
| // boundary applies the growth, which is what keeps `zones::apply_zone_exit_cleanup`'s | ||
| // defuse armed in the meantime. | ||
| // So across accept -> boundary the game has NOT advanced, N is NOT determined, and no growth | ||
| // is on the board: what is there is still a certified-unbounded loop with no materialized | ||
| // bound. The projection therefore KEEPS the `∞` on every surface and TAGS it scheduled rather | ||
| // than hiding it. During that window the badge is load-bearing board-state information; hiding | ||
| // it erases the loop's identity from the display while the loop is still the truth of the | ||
| // board. | ||
| // | ||
| // FAIL-CLOSED: only axes a registered materialization really collapses are hidden, so an | ||
| // unregistered ∞ axis (a mana engine registers none) still renders. | ||
| // ONE authority (`GameState::scheduled_collapse_axes`), now exactly ONE consumer here: the | ||
| // `scheduled_collapse` tag below. The per-axis resource badge rows, the ∞ object pile and the | ||
| // ∞ counter pills are UNFILTERED — they read their own stores and no longer consult this set | ||
| // at all. (`clear_collapsed_materializations` is the authority's other caller, unchanged.) | ||
| // | ||
| // CLASS RULE for the hide-set: hide only axes whose growth is still DEFERRED; never hide an | ||
| // axis that is ALREADY MATERIALIZED and spendable right now. `Tokens` / `Counters` / `Life` | ||
| // are deferred by construction — the growth is not on the board until the boundary applies | ||
| // it — so an `∞` for them is exactly the lie this gate kills. A `DriveSequence` is the one | ||
| // item that does NOT name a deferral: its `collapsed_axes` is `proposal.unbounded`, i.e. | ||
| // EVERY axis of the whole loop, and a `Mana(_)` among them is live *now* — | ||
| // `mana_payment::refill_infinite_mana` tops that controller's pool back to | ||
| // The `Mana(_)` `retain` is preserved and REPURPOSED from hide-filter to tag-filter: it now | ||
| // means "tag only DEFERRED growth". `Tokens` / `Counters` / `Life` are deferred by | ||
| // construction — the growth is not on the board until the boundary applies it. A `Mana(_)` is | ||
| // not: `mana_payment::refill_infinite_mana` tops that controller's pool back to | ||
| // `INFINITE_MANA_PER_TYPE` off the STORE (which this projection deliberately never touches) | ||
| // after every action. Hiding it would show no `∞` beside a pool that keeps refilling: the | ||
| // same internally-inconsistent HUD as an `∞ Life` badge on a finite life total, inverted. | ||
| // CR 500.5: `turns::drain_pending_phase_transition_progress` clears the mana axis when the | ||
| // step/phase ends, and THAT is what legitimately ends the badge — not this projection. | ||
| // after every action, so that pool is live and spendable right now and tagging it "scheduled | ||
| // collapse" would mislabel it. SCOPE LIMIT: a mana ∞ does end — at the CR 500.5 step/phase | ||
| // end, via `turns::drain_pending_phase_transition_progress`, NOT via a materialization — so | ||
| // this tag deliberately UNDER-REPORTS "which ∞ rows will stop being ∞". It answers the | ||
| // narrower question: which ∞ rows name growth that is DEFERRED and will be cashed out by a | ||
| // registered materialization at the boundary. Widen this `retain` only for an axis that gains | ||
| // the same already-materialized property. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Resolve an accepted shortcut before priority resumes.
These lines make a post-accept, pre-endpoint state part of the public engine contract. loop_shortcut.rs then asserts WaitingFor::Priority in that interval. CR 732.2c requires an accepted shortcut to be taken and the game to advance to its proposed ending point. Do not defer the declared iterations to a later boundary. Apply them during acceptance before exposing another priority state. (media.wizards.com)
As per path instructions, enforce strict fidelity to the MTG Comprehensive Rules.
🤖 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/src/game/derived_views.rs` around lines 815 - 845, Update the
shortcut acceptance flow, including the logic leading to
GameState::scheduled_collapse_axes, to apply the proposal’s declared iterations
immediately when the final player accepts it, advancing the game to the proposed
ending point before exposing another priority state. Remove the deferred-growth
behavior and ensure loop_shortcut.rs no longer observes WaitingFor::Priority
between acceptance and endpoint resolution, while preserving normal handling for
shortcuts that are not fully accepted.
Sources: Path instructions, MCP tools
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — three correctness and contract gaps remain.
🔴 Blocker
[HIGH] scheduled_collapse is documented as behavior authorized by CR 732.2c even though the engine deliberately defers the shortcut. Evidence: crates/engine/src/game/derived_views.rs:815-827,864-870 and client/src/adapter/types.ts:2870-2884 label the accepted-but-not-applied state/tag with that rule; verified docs/MagicCompRules.txt:6378 instead says, “the shortcut is taken” and “The game advances to the last proposed ending point.” Why it matters: the public engine/client contract presents an acknowledged engine tolerance as rules behavior, so downstream consumers and future changes receive a false rules guarantee. Suggested fix: remove or narrow the CR 732.2c claims, retain only the verified CR 500.5 boundary references, and make the engine, client, and test prose consistently describe deferred state as an engine tolerance.
🟡 Non-blocking
[MED] The new tag is serialized and typed but has no production client consumer. Evidence: client/src/adapter/types.ts:2870-2884 defines scheduled_collapse, while the production consumers at client/src/hooks/usePlayerDesignations.ts:109, client/src/components/hud/BattlefieldPeekPopover.tsx:60, and client/src/viewmodel/gameStateView.ts:841 consume the existing resource/pile channels; the new references are fixtures/tests. Why it matters: this badge-only change adds a wire contract without delivering the advertised client affordance. Suggested fix: thread the tag into its intended production UI with a behavior test, or remove the unused contract from this change.
[MED] The persistence, filtering, and empty-omission contract for scheduled_collapse is not directly asserted. Evidence: crates/engine/tests/integration/combo_infinite_pile.rs:304-325 reloads then checks only unbounded_pile; crates/engine/tests/integration/loop_shortcut.rs:7326-7336 checks filtered rows/pile but not the tag; client/src/viewmodel/__tests__/unboundedWireSeam.test.ts:36-46 covers omission for the other optional channels while both added fixtures populate this field. Why it matters: a dropped pending-materialization value, viewer projection regression, or empty-array serialization can leave the new tag absent or contract-incompatible while the current assertions still pass. Suggested fix: assert the tag after deserialize, across derive_filtered_views for each viewer, and for an engine-emitted empty derived view that omits the field.
Recommendation: request changes — correct the rules/provenance language and either consume the new contract in production or keep it out of this badge-only change, with the missing persistence/filtering/empty-wire tests.
… the CR provenance Review response to phase-rs#7002. Remove `DerivedViews::scheduled_collapse` entirely — the field, its wire mirror, its emission loop, both goldens' entries and every assertion. It had zero production client consumers: the only readers were the fixtures and the new seam test. It is the hook for the ∞→N affordance, so it belongs in the PR that builds that UI, arriving with the code that reads it rather than one PR ahead of it. The badge fix does not depend on it — verified by re-injecting all three original hide guards, which reds five badge tests, each on its own assertion across the pile, pill and row channels. Correct the rules provenance. The deferred accept→boundary window is an engine deviation: pre-existing, deliberate, and licensed by no CR. Earlier prose here presented it as behaviour CR 732.2c authorizes, which inverts what that rule says — CR 732.2c fixes the count at accept and advances the game to the proposed ending point. CR 732.2c is retained only where it genuinely governs: the accepted N is a ceiling the collapse may not exceed, which the reducer enforces. What it never licensed was *hiding* a mark the store still carries. The ∞ mark is defended on engine-state and display-coherence grounds rather than by citation. Through the window the loop's enablers remain on the battlefield and `unbounded_resources` / `unbounded_loop_enablers` are held in deliberate CR 104.4b / CR 110.1 lockstep, which `zones::apply_zone_exit_cleanup` reads to defuse a capability whose enabler leaves. Filtering the projection while the store still said ∞ gave a HUD that contradicted its own engine, and it suppressed an already-materialized `Mana(_)` axis that `refill_infinite_mana` keeps topping off — a hidden badge beside a visibly refilling pool. No behaviour change in this commit: filtering comment lines out of the engine diff leaves only the deleted guards. Assisted-by: ClaudeCode:claude-opus-5
e6bb30b to
46bc936
Compare
|
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. |
|
🤖 AI text below 🤖 @matthewevans — thank you, both findings were correct and both are now addressed. Pushed as a separate commit ( [HIGH] CR provenance — you were right, and my first attempt at fixing it was wrongConceded. The deferred accept→boundary window is an engine deviation: pre-existing, deliberate, and licensed by no CR. The prose now says that at every site that describes it, in the same terms I want to be transparent that I initially tried to argue back, on the grounds that CR 732.1b — "the shortcut rules can be used to determine how many times those actions are repeated without having to actually perform them" — licensed the deferral. I put that argument through an independent adversarial review before posting it, and it does not hold. Three reasons, recorded here because they may be useful if this comes up again:
What I did keep, and would push back on gently: your remedy's first clause suggests removing the CR 732.2c claims. I've retained 732.2c in one specific role — the accepted N is a ceiling the collapse may not exceed, which the reducer actually enforces (an over-collapse is rejected; The ∞ mark is now defended without a citation at all, on engine-state and display-coherence grounds: through the window the loop's enablers remain on the battlefield and [MED] Unconsumed wire contract — conceded, removedYou were right: zero production consumers. The badge fix does not depend on it, and that is measured rather than asserted: re-injecting all three original hide guards reds five badge tests, each on its own assertion — [MED] Tag persistence / filtering / empty-omission assertionsDissolved by the removal — there is no longer a tag to assert. If the follow-up PR reintroduces the field, it will land with the persistence, per-viewer What this PR is nowThe entire engine behaviour delta is deletions. Filtering comment lines out of Verification at
|
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/engine/src/game/derived_views.rs`:
- Around line 796-836: Change the acceptance flow that records
pending_materialization_count so the accepted shortcut iterations are
materialized before transitioning to WaitingFor::Priority, rather than deferring
them to the next CR 500.5 boundary. Apply the declared changes across the
resource, pile, and counter channels, including the affected ∞ loops, and keep
scheduled-collapse bookkeeping consistent with clear_collapsed_materializations.
Update the scheduled-window tests to assert the resulting endpoint state instead
of the intermediate deferred state.
🪄 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: 3990c26b-2c33-4d17-9cb9-5f00d2a445b3
📒 Files selected for processing (10)
client/src/adapter/types.tsclient/src/test/fixtures/unbounded-counter-wire.jsonclient/src/test/fixtures/unbounded-token-wire.jsonclient/src/viewmodel/__tests__/unboundedWireSeam.test.tscrates/engine/src/game/derived_views.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/combo_infinite_pile.rscrates/engine/tests/integration/kilo_live_offer_from_real_dump.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- client/src/test/fixtures/unbounded-token-wire.json
- crates/engine/src/types/game_state.rs
- client/src/viewmodel/tests/unboundedWireSeam.test.ts
- crates/engine/tests/integration/combo_infinite_pile.rs
- crates/engine/tests/integration/loop_shortcut.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — this current head retains one incorrect CR provenance claim.
🔴 Blocker
[MED] The bookkeeping invariant is still presented as required by CR 104.4b / CR 110.1. Evidence: crates/engine/src/game/derived_views.rs:829-831 says unbounded_resources and unbounded_loop_enablers “stay in CR 104.4b / CR 110.1 lockstep”; the same provenance appears in the test assertions at crates/engine/tests/integration/loop_shortcut_mana_engine.rs:1185 and crates/engine/tests/integration/loop_shortcut.rs:7209,7335. The verified rules say, respectively: CR 104.4b, “If a game … enters a ‘loop’ of mandatory actions … the game is a draw”; and CR 110.1, “A permanent is a card or token on the battlefield” and defines when it ceases to be one. Neither rule requires these two engine bookkeeping maps to remain synchronized. Why it matters: the comments and test diagnostics turn an implementation choice into a false rules guarantee, so later changes can treat map synchronization as rules-mandated rather than preserving it for the actual engine consumer. Suggested fix: describe this as an engine-state invariant required by the zone-exit defuse/boundary behavior, and reserve CR citations for the behavior each cited rule actually governs.
✅ Clean
The prior scheduled_collapse contract/provenance findings were addressed at this head; this request is limited to the remaining map-lockstep citation.
Recommendation: request changes — remove the CR 104.4b / CR 110.1 provenance from the bookkeeping-lockstep claim and its matching test prose.
The CR 732.2c hide-gate in `derive_views` suppressed every ∞ surface for the whole accept→CR-500.5-boundary window: the per-axis resource badge rows, the ∞ object pile and the ∞ counter pills each `continue`d when a pending materialization stash named their axis. That window covers every detected infinite loop in production, so `DerivedViews::unbounded_pile` was empty for every token loop and `unbounded_counters` for every counter loop. Measured on two real 4p dumps — Witherbloom/Sprout Swarm and Kilo/Freed from the Real/Pentad Prism — whose `derived` blocks carry none of the ∞ keys while their stores are populated and `waitingFor` is still `Priority`. Delete the three guards and project the same authority's axis set as an additive, omit-when-empty `DerivedViews::scheduled_collapse` TAG instead. The badge stays visible while the collapse is pending; the tag tells a surface that a finite N is already fixed, and is the hook a later ∞→N affordance can read. The `Mana(_)` retain is preserved and repurposed from hide-filter to tag-filter: mana is already materialized and spendable, so it renders ∞ untagged. The store is still never filtered, so `unbounded_resources` and `unbounded_loop_enablers` stay in CR 104.4b / CR 110.1 lockstep — which is what keeps `zones::apply_zone_exit_cleanup`'s defuse armed — and `clear_collapsed_materializations` still ends both the ∞ and the tag at the CR 500.5 boundary. Cross-seam coverage for both loop families: two engine-emitted wire goldens (token and counter) drive a new client suite through `groupByName`, `familyOf` and `useUnboundedCounterTypes`, pinning the real dump → `derive_views` → serde → client path instead of fabricating derived shapes on each side. Assisted-by: ClaudeCode:claude-opus-5
… the CR provenance Review response to phase-rs#7002. Remove `DerivedViews::scheduled_collapse` entirely — the field, its wire mirror, its emission loop, both goldens' entries and every assertion. It had zero production client consumers: the only readers were the fixtures and the new seam test. It is the hook for the ∞→N affordance, so it belongs in the PR that builds that UI, arriving with the code that reads it rather than one PR ahead of it. The badge fix does not depend on it — verified by re-injecting all three original hide guards, which reds five badge tests, each on its own assertion across the pile, pill and row channels. Correct the rules provenance. The deferred accept→boundary window is an engine deviation: pre-existing, deliberate, and licensed by no CR. Earlier prose here presented it as behaviour CR 732.2c authorizes, which inverts what that rule says — CR 732.2c fixes the count at accept and advances the game to the proposed ending point. CR 732.2c is retained only where it genuinely governs: the accepted N is a ceiling the collapse may not exceed, which the reducer enforces. What it never licensed was *hiding* a mark the store still carries. The ∞ mark is defended on engine-state and display-coherence grounds rather than by citation. Through the window the loop's enablers remain on the battlefield and `unbounded_resources` / `unbounded_loop_enablers` are held in deliberate CR 104.4b / CR 110.1 lockstep, which `zones::apply_zone_exit_cleanup` reads to defuse a capability whose enabler leaves. Filtering the projection while the store still said ∞ gave a HUD that contradicted its own engine, and it suppressed an already-materialized `Mana(_)` axis that `refill_infinite_mana` keeps topping off — a hidden badge beside a visibly refilling pool. No behaviour change in this commit: filtering comment lines out of the engine diff leaves only the deleted guards. Assisted-by: ClaudeCode:claude-opus-5
…variant The maintainer's remaining review finding: the `unbounded_resources` <-> `unbounded_loop_enablers` lockstep was documented as required by CR 104.4b / CR 110.1. Verified against the rules text, neither governs it — CR 104.4b is the mandatory-loop draw rule and CR 110.1 defines a permanent. The lockstep is an engine-state invariant held for exactly one consumer: `zones::apply_zone_exit_cleanup` reads the enabler map to defuse a capability whose enabler leaves, so a desynced store leaves that defuse unarmed. Swept the whole defect class rather than only the two sites this PR introduced. Of the four cited, two were mine and two predate the branch (9169d8f); the same sentence also lived at five pre-existing sites in `game_state.rs` — including the one this PR copied. Fixing only the copies leaves the template. `zones.rs:559` is deliberately preserved: it cites CR 104.4b / CR 110.1 / CR 700.4 for the defuse *behaviour* — an enabling permanent leaving the battlefield — which CR 110.1 genuinely governs ("it stops being a permanent as it's moved to another zone"). Purging every occurrence would be the same defect mirrored; the remedy is to reserve citations for what each rule governs. Also corrects `derived_views.rs`, where under the heading "the two CRs this code does rely on, each for what it actually governs" the CR 500.5 bullet read "the boundary where the deferred growth is applied". CR 500.5 governs until-end-of- step expiry and mana-pool emptying; it is the timing landmark and the reason a `Mana(_)` infinity ends there, but it does not license cashing out deferred token/life/counter growth at that moment. That placement is part of the same uncited engine deviation the block already describes four lines above. No CR number is added: the added-line CR set is empty; the removed set is {CR 104.4b, CR 110.1}. The two `src` edits are comment-only (code lines cmp-identical once trailing comments are stripped, with a positive control); the two test edits change assert message strings only. Assisted-by: ClaudeCode:claude-opus-5
46bc936 to
cfe2b04
Compare
|
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. |
|
🤖 AI text below 🤖 @matthewevans — the CR 104.4b / CR 110.1 finding was correct and is fixed. Pushed as a third commit so you can diff just this round. I also have a correction to make about my own previous verification numbers, unprompted — see the last section. They were real measurements published under the wrong SHA. [MED] CR 104.4b / CR 110.1 on the bookkeeping lockstep — conceded, and swept as a classYou were right. I re-read both rules rather than taking the finding on faith: CR 104.4b is the mandatory-loop draw rule ("If a game … somehow enters a 'loop' of mandatory actions … the game is a draw"), CR 110.1 defines a permanent and when it stops being one. Neither requires two engine maps to stay synchronized. The lockstep is an engine-state invariant with exactly one consumer — Swept the defect class, not just the sites you cited. Of your four, two were introduced by this PR and two predate the branch ( One site is deliberately preserved: Found while fixing yours: the same defect class on CR 500.5, which your review had passedApplying your test to the rest of the PR turned up one more, in
CR 500.5 governs until-end-of-step/phase expiry and mana-pool emptying. It is the timing landmark, and it genuinely explains why a Mechanical invariants for this commit
Correction: my previous verification numbers were published under the wrong SHAThe verification block in my last comment reported The drift reconciles exactly at every step, which is how I know this is the whole story rather than a symptom of something else. The counts moved twice, both times purely from upstream commits arriving under the branch:
Nothing about the code was affected; every gate was green then and is green now. The defect was in the evidence-to-SHA binding — the part you cannot check independently, so the part that has to be right. This PR's body already discloses this same hazard at an earlier rebase: I named the risk once and then walked into it at the next one, which is why the fix here is mechanical rather than a promise to be careful. Every figure below is bound to the SHA it was measured at, with the command and the counting unit stated so you can re-derive rather than trust. Verification — all figures measured at head
|
| Figure | Command | What counts as one unit |
|---|---|---|
| 18514 passed / 0 failed / 6 ignored / 0 filtered out | cargo test -p phase-engine --lib |
one #[test] fn run by the libtest harness — not assertions |
| 4543 passed / 0 failed / 2 ignored / 0 filtered out | cargo test -p phase-engine --test integration |
one #[test] fn in the single integration binary; the 0 filtered out in that same line is the evidence the run was unfiltered |
| exit 0, 0 warnings | cargo clippy --workspace --all-targets -- -D warnings |
exit code; unit is one diagnostic, and -D warnings promotes any warning to an error |
| exit 0 | cargo fmt --all -- --check |
exit code; unit is one file that would be reformatted |
both rows ... ok |
same integration binary, filtered to the fix's own two rows | named tests — unregistered_axis_still_renders_its_infinity_badge and scheduled_collapse_still_renders_the_unbounded_badge, listed individually rather than inferred from a passing total |
| exit 0 | npx tsc -b --noEmit --force |
exit code; -b, not -p, which is vacuous on this solution config |
| 4 passed / 4, and full client suite 2522 passed across 286 files | npx vitest run src/viewmodel/__tests__/unboundedWireSeam.test.ts, then npx vitest run |
one it case |
Each gate log records HEAD_AT_MEASURE, BASE_AT_MEASURE and DIRTY_FILES inside the log itself, so the binding is recorded next to the measurement rather than asserted afterwards.
Both real 4p dump families (Witherbloom/Sprout Swarm tokens, Kilo/Freed from the Real/Pentad Prism counters) remain pinned end to end. Note this round was re-measured after rebasing onto #7008 (deterministic hash-collection serialization), which changes state-map deserialize paths — my fixtures load real dumps, so that was the specific risk being checked, not a formality.
Still not done, disclosed
No in-browser verification of a live 4p game. Acceptance remains engine-seam and wire-level for both dump families; the rendered result should be confirmed in a playtest before this is called fixed.
I can't formally re-request review from a fork, so: whenever you have time.
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/engine/tests/integration/loop_shortcut.rs`:
- Around line 7211-7216: Strengthen the R6a integration test around the relevant
acceptance flow by capturing P0’s life before acceptance and asserting it is
unchanged afterward, rather than only checking life > 0. Validate the filtered
view contains both expected resource axes and all expected pile members, and
assert unbounded_loop_enablers remains populated when enabler lockstep is part
of the contract. Ensure the test exercises the boundary failure path prevented
by the fix, covering the related assertions near the additional referenced
block.
🪄 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: 04fddeb3-dc3e-4009-a44a-8f698cca3a4f
📒 Files selected for processing (10)
client/src/adapter/types.tsclient/src/test/fixtures/unbounded-counter-wire.jsonclient/src/test/fixtures/unbounded-token-wire.jsonclient/src/viewmodel/__tests__/unboundedWireSeam.test.tscrates/engine/src/game/derived_views.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/combo_infinite_pile.rscrates/engine/tests/integration/kilo_live_offer_from_real_dump.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- client/src/test/fixtures/unbounded-token-wire.json
- client/src/test/fixtures/unbounded-counter-wire.json
- client/src/adapter/types.ts
- crates/engine/tests/integration/loop_shortcut_mana_engine.rs
- crates/engine/tests/integration/combo_infinite_pile.rs
- crates/engine/src/types/game_state.rs
- client/src/viewmodel/tests/unboundedWireSeam.test.ts
- crates/engine/tests/integration/kilo_live_offer_from_real_dump.rs
| // The growth is UNMATERIALIZED: the accepted count has not been applied, so P0's life is still | ||
| // a concrete number. The ∞ row beside it reports the live loop mark, not the current total. | ||
| let life = state.players.iter().find(|p| p.id == P0).unwrap().life; | ||
| assert!( | ||
| life > 0, | ||
| "the axis the badge lies about is a finite life total, got {life}" | ||
| "the ∞-badged axis still shows an unmaterialized, finite life total, got {life}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the R6a test prove each stated condition.
life > 0 also passes if the 200 accepted iterations are applied before the boundary. Nonempty filtered collections also pass if filtering removes one required axis or pile member.
Capture P0’s life before acceptance and assert that it is unchanged after acceptance. Assert that the filtered view contains both expected resource axes and the expected pile members. Assert unbounded_loop_enablers remains populated if enabler lockstep is part of this contract.
As per path instructions, “A test must exercise the FAILURE path the fix prevents.”
Also applies to: 7269-7275
🤖 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 7211 - 7216,
Strengthen the R6a integration test around the relevant acceptance flow by
capturing P0’s life before acceptance and asserting it is unchanged afterward,
rather than only checking life > 0. Validate the filtered view contains both
expected resource axes and all expected pile members, and assert
unbounded_loop_enablers remains populated when enabler lockstep is part of the
contract. Ensure the test exercises the boundary failure path prevented by the
fix, covering the related assertions near the additional referenced block.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — three current-head merge blockers remain.
🔴 Blockers
[MED] The projection claims a revocable, live unbounded capability without registering the object-growth enablers that its zone-exit defuse reads. Evidence: crates/engine/src/game/derived_views.rs:819-837,849-890 says the enabler/resources lockstep keeps the capability live and projects the unbounded values; crates/engine/src/game/engine.rs:4716-4719 only calls mark_unbounded_loop; and crates/engine/src/game/engine_resolution_choices.rs:2411-2418 documents that object growth never calls register_unbounded_loop_enablers, making the zone-exit gate inert for this class. Why it matters: an object-growth enabler can leave before the boundary while the displayed capability remains marked and cannot be revoked by the claimed live authority. Suggested fix: capture and register the precise enablers at object-growth acceptance and add a zone-exit-before-boundary regression, or derive the projection from a true live authority instead.
[MED] The R6a assertions do not prove the claimed unmaterialized state or complete viewer projection. Evidence: crates/engine/tests/integration/loop_shortcut.rs:7211-7217 checks only life > 0, which also passes after materialization, and :7265-7275 checks only nonempty resource/pile collections. Why it matters: a partial projection that loses an expected axis or pile member, or a premature life change, can still satisfy these tests. Suggested fix: capture P0’s baseline and assert equality after acceptance; assert the exact expected axes and pile memberships for every filtered viewer; and add an enabler reach guard if the live-enabler contract is claimed.
[MED] The required parse-diff evidence is stale for this engine-source head. Evidence: the only <!-- coverage-parse-diff --> sticky comment is bound to 46bc93691e34f6ca6bcdf88c5ac42088163b5b0d, while the live head is cfe2b042f0c458011825b4d134dbd465d08d5082 and Card data is still in progress. Why it matters: the current engine delta has no head-bound card-level artifact, so its parser/coverage impact cannot yet be verified. Suggested fix: let current-head CI finish and provide the regenerated head-bound parse-diff artifact before the next approval review.
Recommendation: request changes — establish a real live authority for the displayed capability, strengthen the discriminating regression coverage, and wait for current-head parse-diff evidence.
|
Correction to my current-head changes-requested review: I retract only its stale/missing parse-diff point. The The other two MED blockers remain unchanged: the object-growth live/revocable display lacks registered enabler authority, and the R6a projections remain non-discriminating. |
…discriminate
Three maintainer findings on the current head.
(1) The projection claimed the displayed ∞ capability was revocable via
`zones::apply_zone_exit_cleanup`. It is not, for this class. That defuse is gated
on a non-empty `unbounded_loop_enablers`, and the only production writer of that
map is the Interactive Path-C arm; `materialize_object_growth_shortcut` never
registers enablers, so the gate never matches an object-growth mark — i.e. never
for the token and counter families this projection exists to display.
`engine_resolution_choices.rs` already documents that gap in those words and
tracks it as a pre-existing deferred follow-up. The comment now says what the
projection does NOT claim, and discloses the consequence rather than burying it:
because the defuse is inert here, an enabler leaving between accept and boundary
leaves a stale ∞ that this projection now renders where the previous gate
happened to hide it. That is a pre-existing engine gap surfaced by a display fix.
The lockstep sentence is downgraded to match — for this class the enabler map is
empty, so the invariant is vacuously satisfied and is load-bearing only for the
Path-C class that populates it.
(2) Two assertions in the R6a test did not discriminate. `life > 0` also passes
after materialization, so it could not pin the unmaterialized state its own
comment claimed; it now captures a baseline before the accept and asserts
equality. The per-viewer filtered-broadcast row asserted only non-emptiness, so a
partial projection that dropped an axis or a pile member still passed; it now
asserts the exact axis set and the full pile membership, both derived from the
store so the expectation cannot drift from what the accept wrote, plus a control
that the expectations are themselves non-trivial.
Each strengthened assertion is proven discriminating by an isolated mutant that
reds it by name: redacting one axis from the filtered clone only (left
{Life(P0)} vs right {Life(P0), TokensCreated}) — deliberately mutating
`filter_state_for_viewer` rather than the shared projection, so the pre-existing
unfiltered assertion cannot dominate and mask the new row; dropping one pile
member while leaving the pile non-empty; and simulating premature materialization
(746 vs 546).
No production behaviour changes: the engine delta is comment-only.
Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 @matthewevans — all three addressed. Pushed as a fourth commit ( [MED] 1 — the revocability claim. Conceded; it was false for exactly this classYou were right, and the tree already said so. Measured:
My comment nevertheless leaned on that defuse as the live authority keeping the mark honest. That is the second time in this PR I cited a mechanism for something it does not govern (after CR 104.4b / CR 110.1), so I have stopped treating it as a wording slip: the failure is asserting that some named authority backs a claim without checking the authority actually reaches the case. The comment now states what the projection does not claim, and discloses the consequence instead of burying it: because the defuse is inert for this class, an enabler leaving between accept and boundary leaves a stale ∞ — and this projection now renders it where the previous gate happened to hide it. That is a pre-existing engine gap being surfaced by a display fix, and it is the strongest argument for doing the enabler-registration work. The lockstep sentence is downgraded to match: for this class the enabler map is empty, so the invariant is vacuously satisfied here and is load-bearing only for the Path-C class that populates it. On your suggested fix — I did the third option and want to be explicit about it. I did not register enablers at object-growth acceptance. That is an engine behaviour change inside a display-only PR, the gap is pre-existing and already tracked, and doing it here would put new revocation logic in a change whose entire engine delta is otherwise deletions and comments. What I did instead is stop the projection claiming an authority it does not have. If you would rather this PR carry the registration + a zone-exit-before-boundary regression, say so and I will do it — I would just rather you choose that deliberately than have me widen scope on my own initiative. [MED] 2 — the R6a assertions did not discriminate. Fixed, and provenBoth were as weak as you said, and both are mine:
Each strengthened row is proven discriminating by an isolated mutant that reds it, by name:
One methodology note, because my first attempt at that first mutant was worthless. I originally mutated the shared projection to drop the axis. The test went red — but at [MED] 3 — parse-diff evidenceNothing to argue: the artifact was bound to Verification — measured at head
|
| Figure | Command | What counts as one unit |
|---|---|---|
| 18514 passed / 0 failed / 6 ignored / 0 filtered out | cargo test -p phase-engine --lib |
one #[test] fn run by the libtest harness — not assertions |
| 4543 passed / 0 failed / 2 ignored / 0 filtered out | cargo test -p phase-engine --test integration |
one #[test] fn in the single integration binary; the 0 filtered out in that line is the evidence the run was unfiltered |
| exit 0, 0 warnings | cargo clippy --workspace --all-targets -- -D warnings |
exit code; one unit is a diagnostic, -D warnings promotes any warning to an error |
| exit 0 | cargo fmt --all -- --check |
exit code; one unit is a file that would be reformatted |
Counts are identical to the previous round, which is the expected result and worth stating: this round strengthened existing assertions and added no test functions, so a changed total would have meant something unintended happened.
The engine delta this round is comment-only; the rest is test assertions.
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/engine/tests/integration/loop_shortcut.rs`:
- Around line 7279-7283: Update the expected_pile construction in the loop
shortcut test to retain only IDs for which state.battlefield.contains(id) is
true, matching derive_views’ live-object filtering before comparing with
got_pile. Add or preserve a stale pile-member case so the test exercises the
prevented failure path.
🪄 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: ac1fbc42-3962-4ed4-b7f6-fb4ec95a8f0a
📒 Files selected for processing (2)
crates/engine/src/game/derived_views.rscrates/engine/tests/integration/loop_shortcut.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/game/derived_views.rs
| let expected_pile: std::collections::BTreeSet<ObjectId> = state | ||
| .unbounded_loop_pile | ||
| .values() | ||
| .flat_map(|ids| ids.iter().copied()) | ||
| .collect(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply the production live-object filter to expected_pile.
derive_views emits only pile members that remain in state.battlefield; it drops stale stored IDs before populating DerivedViews::unbounded_pile. (raw.githubusercontent.com)
This oracle currently includes every stored ID. A valid state with a stale pile member will make this test fail even when production returns the correct projection. Filter the expected set with state.battlefield.contains(id) before comparing it with got_pile.
Proposed fix
let expected_pile: std::collections::BTreeSet<ObjectId> = state
.unbounded_loop_pile
.values()
.flat_map(|ids| ids.iter().copied())
+ .filter(|id| state.battlefield.contains(id))
.collect();As per path instructions, “A test must exercise the FAILURE path the fix prevents.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let expected_pile: std::collections::BTreeSet<ObjectId> = state | |
| .unbounded_loop_pile | |
| .values() | |
| .flat_map(|ids| ids.iter().copied()) | |
| .collect(); | |
| let expected_pile: std::collections::BTreeSet<ObjectId> = state | |
| .unbounded_loop_pile | |
| .values() | |
| .flat_map(|ids| ids.iter().copied()) | |
| .filter(|id| state.battlefield.contains(id)) | |
| .collect(); |
🤖 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 7279 - 7283,
Update the expected_pile construction in the loop shortcut test to retain only
IDs for which state.battlefield.contains(id) is true, matching derive_views’
live-object filtering before comparing with got_pile. Add or preserve a stale
pile-member case so the test exercises the prevented failure path.
Sources: Path instructions, MCP tools
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — two current-head correctness gaps remain.
🔴 Blockers
[MED] Object-growth infinity is still projected unconditionally even though this path has no registered live enabler authority. Evidence: crates/engine/src/game/derived_views.rs:825-839,880-885 explicitly describes the object-growth zone-exit defuse as inert yet renders its stored infinity; crates/engine/src/game/engine.rs:4716-4718 only marks the unbounded loop; and crates/engine/src/game/zones.rs:566-575 revokes only controllers whose registered enabler set contains the departing object. Why it matters: an object-growth enabler can leave before the boundary while the engine continues to display a stale infinity that it has no live authority to revoke. Suggested fix: register the precise object-growth enablers at acceptance and add a pre-boundary zone-exit regression, or derive this projection from an authority that stays live for the displayed class.
[MED] The R6a expected pile does not model the projection’s stale-member filtering. Evidence: crates/engine/tests/integration/loop_shortcut.rs:7279-7283 copies every stored pile ID, while crates/engine/src/game/derived_views.rs:880-885 emits only IDs still on the battlefield, then the test requires equality at loop_shortcut.rs:7301-7306. Why it matters: a legitimate stale stored ID makes the regression assertion fail even though the production projection correctly omits it, so the test does not express the real contract. Suggested fix: filter the test expectation by battlefield membership, and add a dedicated stale-member case if this omission is intended to be part of the contract.
Recommendation: request changes — establish a live revocation authority for object-growth infinity and align the regression oracle with the projection’s battlefield filter.
2 participants
🤖 AI text below 🤖
Summary
The CR 732.2c hide-gate in
derive_viewssuppressed every ∞ surface for the whole accept→CR-500.5-boundary window, so a detected infinite loop showed no ∞ badge anywhere in production. This deletes the three guards and projects the same authority's axis set as an additiveDerivedViews::scheduled_collapsetag instead — the badge shows while the collapse is pending, and the tag tells a surface that a finite N is already fixed.Files changed
crates/engine/src/game/derived_views.rs— the fix: threecontinueguards deleted,scheduled_collapsetag emittedcrates/engine/src/types/game_state.rs— comment-only; thescheduled_collapse_axes/collapsed_counter_axisdocs described the deleted gate and had become inverted against shipped behaviourcrates/engine/tests/integration/combo_infinite_pile.rs— truth-maintenance + token wire-golden emittercrates/engine/tests/integration/loop_shortcut.rs— truth-maintenance + re-anchored discriminatorscrates/engine/tests/integration/loop_shortcut_mana_engine.rs— truth-maintenancecrates/engine/tests/integration/kilo_live_offer_from_real_dump.rs— truth-maintenance + counter wire-golden emitterclient/src/adapter/types.ts— wire mirror for the new fieldclient/src/viewmodel/__tests__/unboundedWireSeam.test.ts— new cross-seam suiteclient/src/test/fixtures/unbounded-token-wire.json,unbounded-counter-wire.json— new, engine-emitted goldensTrack
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/CR 500.5/CR 104.4b/CR 110.1/CR 701.34a/CR 122.1/CR 400.1/CR 400.7/CR 704.5d— all grep-verified againstdocs/MagicCompRules.txt. The comment-onlygame_state.rsedit introduced zero new CR numbers (added-line CR set equals removed-line set) and every pre-existing number in the touched blocks was re-verified rather than assumed.What the rule change actually is
CR 732.2c fixes the finite N at accept, and the previous code read that as "the axis is already bounded, so ∞ is a lie." But this engine does not advance to the ending point at accept — it defers to a CR 500.5 boundary prompt where the player names N (
turns.rs, an explicitly documented engine tolerance, not a rules entitlement). During that window the loop is still the truth of the board, so hiding the badge erased the loop's identity from the display while it was still real. The store was never the problem and is still never filtered:unbounded_resourcesandunbounded_loop_enablersstay in CR 104.4b / CR 110.1 lockstep, which is what keepszones::apply_zone_exit_cleanup's defuse armed, andclear_collapsed_materializationsstill ends both the ∞ and the tag at the boundary.The
Mana(_)retain is preserved and repurposed from hide-filter to tag-filter: mana is already materialized and spendable (refill_infinite_manare-tops the pool off the store), so it renders ∞ untagged. The tag therefore deliberately under-reports "which ∞ rows will stop being ∞" — a mana ∞ ends at CR 500.5, not via a materialization. That scope limit is documented at both the field and the call site.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—18471 passed; 0 failed; 6 ignoredcargo test -p phase-engine --test integration(unfiltered) —4492 passed; 0 failed; 2 ignoredat rebase base378b6b485; the trajectory across the change was4480/0/2(base) →4473 passed / 7 failed(production edit alone) →4480/0/2(with the test edits), and the 7 reds were exactly the 7 predicted flip rowscargo clippy --workspace --all-targets -- -D warnings— exit 0, 0 warningscargo fmt --all -- --check— exit 0, 0-byte lognpx tsc -b --noEmit --force— exit 0, with a must-fail control (TS2339) proving the gate can faileslint— 0vitest(new seam suite) — 6it/ 13 numbered assertions, 6 passedWire goldens — byte-identical, reproduced exactly by re-running the emitters, md5-stable across 3 independent processes
10 revert-probe arms, each preceded by a green full-arm control, covering all three deleted guards independently plus the tag loop, the
Mana(_)retain, and two wrong-implementation mutantsCI-delegated (disclosed, not implied): the full local battery was run at rebase base
378b6b485. The tip was then rebased forward two commits tod7524b348; the drift catalog was re-derived fresh at that boundary and is empty on all 10 touched paths, so per the repo's CI-delegation policy the re-run is delegated to GitHub Actions rather than repeated locally. Gate A was re-run at the current head.Not done (disclosed): no in-browser verification of a live 4p game. Acceptance here is engine-seam and wire-level for both loop families; the rendered result should be confirmed in a playtest before this is called fixed.
Gate A
Gate A PASS head=e6bb30b7ada196b35d5d51804b8566104ba3a7bb base=d7524b348a437cab35c504b26e162983b02d6660
Run with an explicit
upstream/mainbase: the script defaults tomerge-base origin/main HEAD, andoriginis a fork, which yields a vacuous pass. Range is non-empty (1 commit, 10 files) — but 0 of those files are in Gate A'scrates/engine/src/parserscope, so for this change the gate is trivially satisfied. Reported as run-and-passed, not as evidence of anything.Anchored on
crates/engine/src/game/derived_views.rs:387— the existingUnboundedResourceViewprojection pattern this reuses (same struct, sameattribution_playerauthority, same omit-when-empty serde shape); the new field adds no typecrates/engine/src/types/game_state.rsclear_collapsed_materializations— the authority's other caller, unchanged, and the reason tagging-in-the-projection rather than filtering-the-store is load-bearingFinal review-impl
Final review-impl PASS head=e6bb30b7ada196b35d5d51804b8566104ba3a7bb
Pipeline: 3 independent plan-review rounds (R1 REVISE 2 blocking → R2 REVISE 1 blocking + 11 → R3 verification 12/12 resolved, 1 blocking + 5), then execution, then an independent implementation review (1 blocking, comment-only, since fixed and re-verified). No step reviewed its own output.
Evidence the fix is real, on two real games
Both families are user-reported 4p game dumps, not synthetic scenarios, and both are covered by fixtures already in-tree:
derivedbeforederivedafterunbounded_resources={0:[TokensCreated]},unbounded_loop_pile={0:[407]}[{0,TokensCreated}]+ pile[407]+ tagunbounded_resources={0:[Counter(Other,Other)]},unbounded_counter_targets={0:[[402,"charge"]]}[{0,Counter(Other,Other)}]+ counters{402:[charge]}+ tagBetween them the two families exercise all three gate consumers (rows, pile, pills). Verified through the real
derive_viewsfor viewersNone/P0/P1and through the realderive_filtered_viewsbroadcast path.The cross-seam test class was genuinely missing before this: the engine-side test proved the counter pill only by hand-clearing the materialization stash, and the client tests fabricated
derivedshapes directly. The new goldens are engine-emitted with a reproducible regeneration path and drive the real client consumers (groupByName,familyOf,useUnboundedCounterTypes), so the dump →derive_views→ serde → client path is pinned end to end instead of asserted on each side independently.Claimed parse impact
None.
Scope Expansion
One file, comment-only.
crates/engine/src/types/game_state.rswas outside the plan's frozen scope, and the executor correctly refused to touch it. It was then widened deliberately, restricted to comment lines, because the docs onscheduled_collapse_axes— the single authority this change repurposes — still described the deleted hide-gate and read as the exact inverse of shipped behaviour, withderived_views.rspointing readers straight at them. The doc's consumer census was also stale: it listed the ∞ counter-pill projection as a caller ofcollapsed_counter_axis, and this change removed that call (three production callers remain, all ingame_state.rs).Proven comment-only by three independent instruments: 61 changed lines, none failing a comment-prefix filter (positive control: the same filter reports 35 code lines in
derived_views.rs); and stripping all comment lines from before and after leavescmp-identical 19328-line files.Validation Failures
None.
CI Failures
None.
Accepted, disclosed regression
By construction this restores a display the previous gate was written to kill: an
∞badge beside a finite, growing total —∞ Lifenext to a real life total, an∞pile whose members are countable, an∞charge pill on a Pentad Prism that really holds 4 counters. That is the intended trade. Players want to see that the loop is live while it is live, and the board reads more clearly with the loop's identity on screen; thescheduled_collapsetag is the hook a later ∞→N affordance can use to show both the ∞ and the bound.Follow-up (pre-existing, not introduced here)
Two counter render sites do not subscribe to
useUnboundedCounterTypes(AttackTargetPicker/StackLabel, andDialogAttachmentCard), so they show no ∞ pill either before or after this change. Neither affects either dump family — both render their pills through a site that does subscribe. Tracked as a separate follow-up rather than scope-crept into this PR.Related PR
The amount-prompt half of this workstream is now open as #7019 (client-only: one shared sanitized
numeric box for the
PayAmountChoice/ChooseXValue/ Assist prompts). It does not depend on this PR— verified, not assumed:
git merge-base --is-ancestorreports none of this PR's commits are in thatbranch, and it touches no
crates/file.The dependency runs the other way and applies to a third PR, still unopened: the
∞ → Nbadge affordanceneeds a rendered ∞ badge to attach to, which is what this PR provides. So the landing order is
#7019 (independent, any time) → this PR → the badge affordance.
Summary by CodeRabbit
Bug Fixes
Tests