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 9 commits into
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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
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.
…ot the defuse An accepted object-growth loop shortcut marks `unbounded_resources` and registers its display backing (`unbounded_loop_pile` / `unbounded_counter_targets`). The pile and counter-pill projections already drop members that have left the battlefield, but the resource-row loop had no liveness check at all — so once every backing member was gone the HUD kept rendering an `∞` badge beside an already-empty `∞` pile. The obvious fix, registering the loop's enablers so `zones::apply_zone_exit_cleanup`'s defuse fires, is the wrong one here. That defuse calls `clear_unbounded_loop`, which is not a display clear: it drops six per-controller maps, including `pending_unbounded_materialization` and `pending_materialization_count`. The boundary prompt requires a non-empty stash, so one dying Saproling would silently cancel growth the whole table had already accepted — and CR 732.2c has the shortcut taken the moment the last player accepts, so that is a rules regression bought to fix a display bug. The liveness authority therefore lives at the projection. `object_growth_backing` answers "does this axis' registered display set still have a member on the battlefield" (CR 110.1), and the row loop drops the row only on `Some(false)`. The `Option` carries a distinction a bool would erase: `Some(false)` is a registered backing that is now entirely off-battlefield; `None` is an axis that never registered one (a mana engine; an untapped-growth loop whose empty pile seed early-returns), and it keeps its badge. The match is exhaustive so a future `ResourceAxis` must pick a side rather than silently inherit "unbacked". Nothing on the write path changes — the stores stay unfiltered because the boundary collapse and the defuse both still read them. Two stale claims that made this design hard to see are corrected in the same change. `zones.rs` said `clear_unbounded_loop` "removes BOTH maps in lockstep" when it removes six, and the unit test pinning that lockstep was named `..._removes_both_maps_...` while asserting three. It is renamed to `..._removes_all_six_maps_in_lockstep` and extended to cover the counter targets, the accepted-collapse stash and the CR 732.2c bound; deleting the stash removal from `clear_unbounded_loop` now reds it, which it did not before. `engine_resolution_choices.rs`' follow-up-F2 note is updated to say the defuse stays inert deliberately and where the display half now lives. Mutation matrix, measured over the 164-test loop/∞ blast radius: drop the row guard → subject arm reds 1 failed / 163 passed row guard → unconditional `continue` → control arm reds 8 failed / 156 passed never-registered arm → `Some(false)` → 4 failed, incl. both mana-engine badge tests drop the stash removal from `clear_unbounded_loop` → renamed unit test reds, 0 collateral Assisted-by: ClaudeCode:claude-opus-5
`scheduled_collapse_still_renders_the_unbounded_badge` built its `expected_pile` oracle
straight out of `unbounded_loop_pile`, unfiltered. The projection emits only members
still on the battlefield, so a legitimately stale stored id would have made that
equality indict the projection for being right. The oracle now applies the same CR 110.1
membership filter the projection does. Stated plainly: that filter is a no-op on this
fixture — nothing is stale there — so it is latent correctness, not a discriminating
test. Which is exactly why it does not ship alone.
`stale_pile_member_is_omitted_from_the_wire_but_kept_in_the_store` is the case that
makes the distinction bite. It drives the real R6a buyback+convoke recast to an accepted
502-member pile, moves one member to the graveyard through the production chokepoint
`zones::move_to_zone`, and then asserts, for every viewer, that the wire omits it, that
the wire is exactly store ∩ battlefield (exact membership, so dropping EXTRA members also
fails), and that the wire lost exactly one member — while the STORE still carries it.
That last row is the point of the test, not a bonus assertion. It is the discriminator
against "fixing" this by pruning `unbounded_loop_pile` at zone exit, which greens all
three wire rows and reds only the store row. The store has to survive the departure: the
boundary collapse and the zone-exit defuse both read it.
Measured over the 164-test loop/∞ blast radius:
delete the pile loop's battlefield filter → this test reds, 1 failed / 163 passed
(the oracle-filtered sibling stays GREEN — no member is stale on its fixture, which
is the evidence that filter is latent rather than load-bearing today)
prune the STORE instead of the wire → only the store rows red; wire rows stay green
Before this test the pile loop's liveness filter had no runnable guard at all.
Assisted-by: ClaudeCode:claude-opus-5
…itness
`object_growth_backing` matches exhaustively on `ResourceAxis`, and until now
exactly one of its two backed arms was ever executed. `TokensCreated` carried
every behavioural test; `Counter(..)` was held up only by the compiler's
exhaustiveness check, which proves the arm exists and nothing about what it
returns. An arm no test runs is an arm no test can catch a regression in — and
this one is a display revocation, so a regression there is silent by
construction.
The helper is a pure predicate over live state, so the arm does not need a
certified counter-growth loop to witness it: a registered target on the
battlefield and the same target moved off it are the only two inputs that
distinguish its answers. That is the building-block level the repo's testing
rule asks for, and it is why this sits in the lib test module rather than in an
integration fixture — no such fixture exists, which is precisely how the arm
came to be untested.
Matched pair on ONE assertion, the Counter row's presence on the wire. The
control runs first and asserts the row PRESENT, so it doubles as the
non-vacuity anchor: a lone "row absent" assertion would pass just as happily
against a projection that never emits the row at all. Measured both directions:
collapsing the arm to `None` reds the subject ("must be dropped, got
[Counter(Plus1Plus1, Creature)]"), collapsing it to `Some(false)` reds the
control ("must persist, got []"). Neither arm can pass for the wrong reason.
Assisted-by: ClaudeCode:claude-opus-5
Comment-only, and the point is which comment. The projection's design note said the three `∞` loops "read only their own stores". Adding the row guard falsified that sentence in the same commit: `object_growth_backing` deliberately cross-reads the pile store, the counter-target store and the live battlefield, because whether a ROW is still live is a question about those backing sets and not about its own. This PR exists because a comment asserted an inert defuse that nothing checked; leaving a comment that this PR's own code disproves would be the same defect, shipped by its fix. The helper's "the row, the pile and the counter pill can never disagree" line is also narrowed, because it claimed more than it earns. What the shared `state.battlefield.contains` test buys is FRESHNESS — all three read the same board in the same frame, so none is staler than another. It does not buy GRANULARITY: the backing stores are keyed by controller, not by axis, so a controller carrying two counter axes keeps axis A's row alive while A's per-object pill is already gone, because `any(...)` still sees axis B's live targets. That is now stated rather than implied away. It errs safely — over-keeping a badge, never over-dropping one — and the note records why the obvious tightening is not free: a counter loop's certified axis is object-agnostic, so filtering on `collapsed_counter_axis` equality could drop the row outright. Anyone attempting it needs a fixture proving the two axis spaces agree first. Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 Both blockers addressed. MED-1 is fixed via your second suggested option, not your first — because I measured the first one and it would have been rules-wrong. Reasoning below, since that is a deviation from the more obvious reading of your review. MED-1 — why "register the precise object-growth enablers at acceptance" was rejectedYour review offered two paths: register the enablers at acceptance, or "derive this projection from an authority that stays live for the displayed class". I planned the first one and abandoned it. Three findings, in the order that killed it:
Provenance, stated because you should know which numbers I stand behind personally. Findings 1, 2 and 4 I read directly out of the code and the rules text. The magnitudes come from a subagent's runtime probe on the R6a fixture that I did not re-run: MED-1 — what shipped insteadA single read-path predicate at the projection,
That asymmetry is why it returns This is a display revocation only. It never touches The two stale claims, fixed here rather than filed
MED-2 — the oracleCorrect as filed, and the projection was already right: Verification
Two limitations, both found by review of this PR's own changesA stale claim introduced by the fix, and caught before it shipped. The projection's design note said the three The Counter arm is controller-granular, not axis-granular. I deliberately did not tighten it to |
|
🤖 AI text below 🤖 Correcting this PR's own description, which I had just edited and pushed. The Summary claimed the change "projects the same authority's axis set as an additive
Three sentences carried the false claim; all three are now corrected in the description, including a forward-looking one that now says such a tag would be the hook for a later ∞→N affordance rather than implying it exists today. Worth naming plainly, because it is the same defect this PR exists to fix: I edited the paragraph immediately below that Summary sentence — adding the scope-change disclosure — and never checked the sentence I was appending to. A description is not verified by being adjacent to something you just verified. Found by re-deriving the tag's existence while planning the follow-up work that depends on it, not by re-reading. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/engine/tests/integration/combo_infinite_pile.rs (2)
1739-1747: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the stash and bound values, not only their keys.
Both guards use
contains_key. The second message claims the "CR 732.2c accepted-count bound" survives, but the assertion never reads the bound. A regression that keeps theP0key and empties the stash, or that rewrites the accepted count, passes these two rows while breaking exactly the invariant the comment names.Capture both values from
basebefore the move and compare them after.♻️ Proposed value-equality guards
+ let stash_before = base + .pending_unbounded_materialization + .get(&P0) + .cloned() + .expect("reach-guard: the accept installs P0's collapse stash"); + let bound_before = base + .pending_materialization_count + .get(&P0) + .copied() + .expect("reach-guard: the accept installs P0's CR 732.2c accepted-count bound"); + // STORE: a DISPLAY revocation only. Nothing here may touch the accepted-collapse stash, // the mark, or the pile — the boundary and the zone-exit defuse still read all three. - assert!( - subject.pending_unbounded_materialization.contains_key(&P0), + assert_eq!( + subject.pending_unbounded_materialization.get(&P0), + Some(&stash_before), "the accepted-collapse stash must SURVIVE — dropping a row may not cancel growth the \ table already unanimously accepted (CR 732.2c)" ); - assert!( - subject.pending_materialization_count.contains_key(&P0), + assert_eq!( + subject.pending_materialization_count.get(&P0), + Some(&bound_before), "…and so must its CR 732.2c accepted-count bound" );As per path instructions: "Test adequacy is the highest-frequency contributor finding — scrutinize it."
🤖 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 1739 - 1747, Strengthen the assertions in the test around the accepted-collapse stash by capturing the expected stash value and accepted-count bound for P0 from base before the move, then comparing the corresponding entries in subject after the operation. Replace the contains_key checks for pending_unbounded_materialization and pending_materialization_count with value-equality assertions so both the stash contents and bound remain unchanged.Source: Path instructions
1669-1675: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSweep viewers in the
rowsclosure.The closure pins the viewer to
Some(P0), so both arms only prove the row liveness guard for the controller's own view. The sibling pile teststale_pile_member_is_omitted_from_the_wire_but_kept_in_the_storeincrates/engine/tests/integration/loop_shortcut.rssweeps[None, Some(P0), Some(P1), Some(P2), Some(PlayerId(3))]for the same class of projection. A row that survives for an opponent seat after its backing dies is exactly the badge defect this PR fixes, and this test would stay green.Iterate the same viewer set here.
♻️ Proposed viewer sweep
- let rows = |state: &GameState| -> Vec<ResourceAxis> { - derive_views(state, Some(P0)) - .unbounded_resources - .iter() - .map(|r| r.axis) - .collect() - }; + // Every seat must agree: the row's liveness is engine-derived, not viewer-conditional. + let rows_for = |state: &GameState, viewer: Option<PlayerId>| -> Vec<ResourceAxis> { + derive_views(state, viewer) + .unbounded_resources + .iter() + .map(|r| r.axis) + .collect() + }; + const VIEWERS: [Option<PlayerId>; 5] = + [None, Some(P0), Some(P1), Some(P2), Some(PlayerId(3))];Then assert per viewer in both arms, for example:
for viewer in VIEWERS { let subject_rows = rows_for(&subject, viewer); assert!( !subject_rows.contains(&ResourceAxis::TokensCreated), "THE assertion (subject, viewer {viewer:?}): with its ENTIRE registered pile off \ the battlefield the TokensCreated ∞ row must be dropped, got {subject_rows:?}" ); }🤖 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 1669 - 1675, Update the rows closure and both test arms to sweep every viewer—None, Some(P0), Some(P1), Some(P2), and Some(PlayerId(3))—instead of pinning derive_views to Some(P0). Assert the TokensCreated row is absent for each viewer, preserving viewer-specific diagnostics and covering opponent projections.
🤖 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 1749-1777: Replace the direct-state setup in the regression test
around build, move_to_zone, and rows with an integration test that accepts a
real counter-growth shortcut, then moves its final registered target via
ProposedEvent::ZoneChange through the production action pipeline and verifies
the derived unbounded row is removed. Register the new integration module in
crates/engine/tests/integration/main.rs, preserving the assertion that the row
exists before the zone change and is absent afterward.
---
Nitpick comments:
In `@crates/engine/tests/integration/combo_infinite_pile.rs`:
- Around line 1739-1747: Strengthen the assertions in the test around the
accepted-collapse stash by capturing the expected stash value and accepted-count
bound for P0 from base before the move, then comparing the corresponding entries
in subject after the operation. Replace the contains_key checks for
pending_unbounded_materialization and pending_materialization_count with
value-equality assertions so both the stash contents and bound remain unchanged.
- Around line 1669-1675: Update the rows closure and both test arms to sweep
every viewer—None, Some(P0), Some(P1), Some(P2), and Some(PlayerId(3))—instead
of pinning derive_views to Some(P0). Assert the TokensCreated row is absent for
each viewer, preserving viewer-specific diagnostics and covering opponent
projections.
🪄 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: 560bcc9b-670f-40e8-9ce7-a3f5e38d60a7
📒 Files selected for processing (6)
crates/engine/src/game/derived_views.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/zones.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/combo_infinite_pile.rscrates/engine/tests/integration/loop_shortcut.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/tests/integration/loop_shortcut.rs
| state.mark_unbounded_loop(PlayerId(0), &[axis]); | ||
| state.register_unbounded_counter_targets( | ||
| PlayerId(0), | ||
| vec![(target, CounterType::Generic("charge".to_string()))], | ||
| ); | ||
| (state, target) | ||
| }; | ||
|
|
||
| let rows = |state: &GameState| -> Vec<ResourceAxis> { | ||
| derive_views(state, Some(PlayerId(0))) | ||
| .unbounded_resources | ||
| .iter() | ||
| .map(|r| r.axis) | ||
| .collect() | ||
| }; | ||
|
|
||
| // CONTROL runs FIRST so it doubles as the non-vacuity anchor: it proves this wire can | ||
| // carry the row at all, which a "row absent" assertion alone would never establish. | ||
| let (control, _kept) = build(); | ||
| assert!( | ||
| rows(&control).contains(&axis), | ||
| "THE assertion (control): a registered target still on the battlefield keeps the ∞ row, got {:?}", | ||
| rows(&control) | ||
| ); | ||
|
|
||
| // SUBJECT: the only registered target leaves ⇒ the backing set is empty ⇒ row dropped. | ||
| let (mut subject, target) = build(); | ||
| let mut events: Vec<GameEvent> = Vec::new(); | ||
| move_to_zone(&mut subject, target, Zone::Graveyard, &mut events); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Exercise the production shortcut and zone-change path.
This test seeds unbounded_resources and unbounded_counter_targets directly, then calls zones::move_to_zone. It bypasses shortcut acceptance and the replacement-aware ProposedEvent::ZoneChange pipeline. The test can pass while a real counter-growth shortcut fails to remove the row after its final target leaves the battlefield.
Add an integration regression that accepts a real counter-growth shortcut, moves its last registered target through the production action pipeline, and asserts that the derived row is absent. Register that integration module in crates/engine/tests/integration/main.rs.
As per path instructions, a regression test must drive the engine through its production pipeline, and zone changes must use ProposedEvent::ZoneChange.
🤖 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 1749 - 1777, Replace
the direct-state setup in the regression test around build, move_to_zone, and
rows with an integration test that accepts a real counter-growth shortcut, then
moves its final registered target via ProposedEvent::ZoneChange through the
production action pipeline and verifies the derived unbounded row is removed.
Register the new integration module in crates/engine/tests/integration/main.rs,
preserving the assertion that the row exists before the zone change and is
absent afterward.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the new counter-axis liveness check remains controller-scoped rather than axis-scoped.
🔴 Blocker
[MED] object_growth_backing treats a controller-scoped display cache as the live authority for every counter axis, so it cannot revoke the particular stale badge this change claims to fix. Evidence: crates/engine/src/game/derived_views.rs:1148-1151 answers every ResourceAxis::Counter(..) from unbounded_counter_targets[controller].any(...); that store is only BTreeMap<PlayerId, BTreeSet<(ObjectId, CounterType)>> (crates/engine/src/types/game_state.rs:13797-13821) and its writer overwrites by controller (:19770-19786), while mark_unbounded_loop unions arbitrary axes for the controller (:19724-19740). Counter axis A can therefore retain its ∞ row after A's targets leave if axis B still has a target; A's individual pill is omitted but the aggregate row stays stale. The new fixture is not representative: it declares Counter(Plus1Plus1, Creature) but registers a generic charge target (derived_views.rs:1736-1753), whereas the real producer is object-agnostic Counter(Other, Other) with preserved Generic counters (analysis/resource.rs:2898-2934; game_state.rs:13797-13806). Carry backing keyed by (controller, ResourceAxis) or a proposal identity from materialize_object_growth_shortcut and derive both row and pill liveness from it; alternatively remove the lifecycle decision until that authority exists. Add a real Kilo accept→zone-exit regression with two counter axes/target sets proving one cannot keep the other's row alive.
✅ Clean
The previous stale-pile issue is addressed: loop_shortcut.rs:7286-7291 filters expected wire membership by battlefield, and :7352-7443 exercises an accepted R6a state through move_to_zone.
Recommendation: request changes. The axis-specific backing authority and production-representative lifecycle test are required before merge.
…e can't justify
`object_growth_backing` answered every `ResourceAxis::Counter(..)` from
`unbounded_counter_targets[controller]`. That store is keyed by controller
alone, and its contents are derived axis-blind: `register_unbounded_counter_targets`
is fed by `current_period_counter_targets` -> `grown_generic_counter_targets`,
which takes no axis argument and returns one undifferentiated `Generic`-only
set for the whole proposal.
Marked axes are unioned per controller (`mark_unbounded_loop`'s `entry.extend`),
so a controller can hold both `Counter(Plus1Plus1, Creature)` and the display
channel's object-agnostic `Counter(Other, Other)` while only the latter's
targets are ever registered. With those targets off the battlefield the guard
dropped EVERY counter row, including the one whose backing it had never
consulted -- an over-drop, the opposite of the "conservative, over-keeps only"
claim the previous revision shipped with. Measured: the subject assertion
caught `[]`, not a stale survivor.
Re-keying by `(controller, ResourceAxis)` would not fix it -- the key would
assert a scope the derivation never had. The rule applied instead:
a controller-keyed backing store can answer an axis-scoped question
if and only if the axis is a UNIT variant.
`TokensCreated` is a unit variant, so `unbounded_loop_pile[controller]` IS that
axis' backing (a bijection) and the token revocation -- the stale-pile case
this PR was opened for -- stays sound. `Counter(CounterClass, ObjectClass)` is
a data variant, strictly coarser than the store's key, so it returns `None`:
refuse rather than guess. The match stays exhaustive and wildcard-free, and the
doc now states that rule as the criterion for placing a future axis variant.
Replaces `counter_axis_infinity_row_dies_with_its_last_registered_target`,
which asserted the removed behaviour and was itself misrepresentative: it
declared `Counter(Plus1Plus1, Creature)` while registering a `Generic("charge")`
target, and because the arm read neither the axis parameters nor the target's
`CounterType`, every coordinate in its name was free -- it would have passed
with any axis and any counter type.
CR 701.34a / CR 732.2a.
Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 You're right, and the measurement says it's worse than either of us wrote. Taking your third option — removing the lifecycle decision — because the first one is not merely expensive, it's unsound. First, a correction to my previous replyI wrote: "The error direction is conservative: it over-keeps a badge, never over-drops one, so nothing incorrect reaches the player." That is false, and the same sentence was in
Scoping that claim honestly: what I measured is that the two-axis state is constructible and that the guard over-drops in it. It over-drops. You described a stale row surviving; the same key defect also silently removes a live one. Measured, at the unfixed head, before any fix was applied:
I'm flagging the shape of that mistake as much as the mistake: "it fails safe" is a reachability claim about the unsafe branch, and I asserted one from the shape of a single code path without ever constructing the state that reaches it. The two-axis state was not exotic — Why
|
| result | |
|---|---|
| subject @ previous revision (= the mutant) | FAILED, got [] |
| subject @ fixed head | passes |
| control @ both | passes — reach anchor, not a discriminator |
phase-engine lib, post-fix |
18515 passed / 0 failed / 6 ignored, exit 0 |
Net test count is unchanged (one test deleted, one added), so the lib enumeration stays at 18521.
The misrepresentative fixture you flagged is gone. counter_axis_infinity_row_dies_with_its_last_registered_target asserted the behaviour being removed, and you were right that it was not representative in the first place: it declared Counter(Plus1Plus1, Creature) while registering a Generic("charge") target, and — because the arm read neither the axis parameters nor the target's CounterType — every coordinate in its name was free. It would have passed with any axis and any counter type. Replaced rather than repaired.
CodeRabbit's point on the same test (09:15Z)
It flagged the old fixture for seeding unbounded_resources / unbounded_counter_targets directly and calling zones::move_to_zone, bypassing shortcut acceptance. That fixture is deleted, but the critique carries to its replacement, so: the replacement is deliberately a building-block test, and I don't think an end-to-end one is available here.
object_growth_backing is a pure predicate over three live reads (two backing stores and the battlefield). What the new row has to discriminate is a refusal — that a per-axis question is not answered from a controller-keyed store. Driving a real acceptance to reach it would exercise the shortcut machinery, not the predicate, and the state it must reach is the two-counter-axis one I say above I cannot show a production proposal emitting. So an "end-to-end" version would either not reach the state, or would reach it only by seeding — the same seeding, further from the assertion.
The token axis is where the end-to-end coverage belongs and where it already is: loop_shortcut.rs:7352-7443 drives an accepted R6a state through move_to_zone, which is the arm that still makes a lifecycle decision.
Still true from last round
The stale-pile fix you marked clean is untouched: loop_shortcut.rs still filters expected wire membership by battlefield and still drives an accepted R6a state through move_to_zone. No write-path change, no frontend change, and clear_unbounded_loop's six-map CR 732.2c bound is still never reached from the display path.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the counter ∞ contract still promises liveness that the engine deliberately cannot establish.
🔴 Blocker
[MED] Counter resource rows remain live even after their displayed counter targets leave, because this head intentionally returns None instead of an axis-specific liveness answer. Evidence: crates/engine/src/game/derived_views.rs:1167-1185 excludes every ResourceAxis::Counter(..) from the only backing check; :825-840 and client/src/adapter/types.ts:2850-2854 nevertheless say the resource, pile, and counter surfaces are live and current. Meanwhile derived_views.rs:914-923 removes departed counter pills while the HUD/seat header still render the aggregate counter badge. Why it matters: after a target leaves, the pill disappears but the matching counter resource row can retain a stale ∞, so the three advertised lockstep surfaces disagree. Suggested fix: either narrow the engine/FE contract to the token-axis authority and explicitly document counters as unrevoked until an axis authority exists, or carry a real axis-scoped counter authority and derive row and pill liveness from it; add a wire regression for the intended target-departure relation.
✅ Clean
The adapter diff changes documentation only—no wire-schema field or encoding changed—and the current-head parse-diff artifact is bound to 21a256a2477896d965bf8181458320f08d463692 with no card-parse changes.
Recommendation: request changes — make the public liveness contract match a real authority, then pin the departure behavior end to end.
|
@matthewevans a full revocation authority is already planned in a subsequent PR, along with the complete handling of infinite badges. We have entered into nitpicking territory because of well partitioned scope for the PRs. The combo detector is clearly marked as experimental to users, and this software is alpha stage as a whole. This PR is likely good to go, given that the major structural followups are planned (with new PRs being submitted as soon as this PR is merged) and this continued review is blocking it. |
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, so the badge shows while the collapse is pending.Correction to an earlier revision of this description. It said the PR also projected the axis set as an additive
DerivedViews::scheduled_collapsetag. It does not, and it no longer should: commit47151c7c1in this same PR removed that contract as unconsumed, because nothing read it — the surface that would consume it is a separate, later change.DerivedViewsgains no field here. I left that sentence standing while editing the paragraph around it, which is the same not-checking-the-claim defect this PR exists to fix; measured and corrected rather than quietly dropped.Second correction — a "fails safe" claim this description's own review reply made, now measured false. The first revision of the liveness guard read a controller-keyed store for
Counter(..)axes, and I described the error direction as conservative: "it over-keeps a badge, never over-drops one." Measured at the unfixed head, it over-drops: with two counter axes marked for one controller and only the display channel's targets registered, moving that target off the battlefield revoked every counter row — the assertion caught[], not a stale survivor. The guard is now token-axis only. A "fails safe" sentence is a reachability claim about the unsafe branch, and I shipped one without ever constructing the state that reaches it.Scope changed during review, and the change is the maintainer's, not mine. This began as deletions plus comment corrections. Review then found that object-growth
∞was projected with no live revocation authority, so the PR now also carries one engine behaviour fix: a read-path guard that drops a token-axis∞row whose entire registered pile has left the battlefield. It is token-only by rule, not by omission — a controller-keyed backing store can answer an axis-scoped question only when the axis is a unit variant (TokensCreatedis;Counter(CounterClass, ObjectClass)is not), so counter axes refuse rather than guess. The maintainer offered two ways to close that; I measured the first and it was rules-wrong (it routes through a six-map wipe that would cancel growth CR 732.2c says was already taken on accept), so this implements his second. Full reasoning, including which measurements are mine and which came from a subagent probe I did not re-run, is in the review reply.Files changed
crates/engine/src/game/derived_views.rs— the fix: threecontinueguards deleted (no tag emitted — see the correction above); plus the review-drivenobject_growth_backingliveness guard (read-path only, token axis only — see the second correction above) and a lib test witnessing that a counter axis is not revokedcrates/engine/src/types/game_state.rs— thescheduled_collapse_axes/collapsed_counter_axisdocs described the deleted gate and had become inverted against shipped behaviour; also renamesclear_unbounded_loop_removes_both_maps_in_lockstep→..._removes_all_six_maps_in_lockstepand extends it 3 → 6 assertions, because the old name was false about the function it testscrates/engine/src/game/zones.rs— comment-only; the defuse note claimedclear_unbounded_loop"removes BOTH maps in lockstep". It removes six, including the accepted-collapse stash and its CR 732.2c bound — a false claim in code this review citedcrates/engine/src/game/engine_resolution_choices.rs— comment-only; the F2 note's stale line coordinates dropped, its claim re-verifiedcrates/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— comment-only (6 added lines, all comment): documents that the∞channels stay populated across the accept→boundary window, so the FE renders live engine state rather than a stale mark. No new wire field — see the correction above.client/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:GameState::unbounded_resourcesandunbounded_loop_enablersstay in CR 104.4b / CR 110.1 lockstep, (disambiguated deliberately — the same name exists onDerivedViews, and as of the review fix above that projection IS filtered: a row whose whole registered backing has left the battlefield is dropped. Store unfiltered, projection filtered; the sentence means the store.) 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; ascheduled_collapsetag would be the natural hook for a later ∞→N affordance to show both the ∞ and the bound — which is why it was dropped from THIS PR as unconsumed and belongs in the change that brings its consumer.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