fix(engine): scope the ∞ revocation by axis, and keep the row an accepted collapse earned - #7048
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThe engine now publishes structured collapse metadata and complete per-object counter projections. The client renders these projections across cards, badges, loyalty totals, attack labels, and battlefield grouping. Tests cover shortcut ceilings, accepted-collapse persistence, counter backing, wire output, localization, and interaction limits. ChangesCollapse and counter projection
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
|
Maintainer disposition: no branch update was applied. This draft is still stacked directly on open #7045 (current parent d b6b400804118ae5fab20715b3e20da5ff7a77c1). A merge simulation against current main has textual conflicts; merging main into the child now would combine parent and child conflict resolution, invalidate the head-bound evidence, and add churn before the stack base lands. Please keep this PR draft. After #7045 merges, rebase this child onto the resulting main, regenerate the head-bound verification and parse evidence, and request review. No security hard-stop was found in the current diff. |
d19e9a4 to
a11f2be
Compare
|
Maintainer hold at current head Keep this PR as a draft: it remains stacked on open #7045 ( |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
…ill be asked Re-applies PR phase-rs#7048's rules content onto the data model phase-rs#7045 left behind. phase-rs#7045 squash-merged as ea87712 and, after the commit this branch forked from, moved the collapse state off the row: `UnboundedResourceView` is now `{player, axis}` and a `(player, family)`-keyed `UnboundedFamilyView` carries `FamilyCollapseState`. The four fixes below are therefore re-sited, not replayed. THE ACCEPTANCE GATE (CR 732.2c). A row is withheld only when the collapse has NOT been accepted and its object backing is registered-and-gone. The gate reads a new FACT set, `accepted_collapse_axes`, never the display-filtered announcement set — those answer different questions, and coupling them would let a rendering rule revoke a row for a collapse the table unanimously agreed to. They coincide today only because `object_growth_backing` returns `None` for `Mana(_)`; that is an accident between two functions, not an invariant either states. THE COUNTER ARM. `object_growth_backing` answers `Counter(..)` by deriving each registered `(ObjectId, CounterType)` pair's own axis through `collapsed_counter_axis` — the same bridge the stash uses, so a display set and the store that collapses it cannot disagree about what an axis is. A bearer that ceased to exist derives `Counter(_, Other)` (CR 400.7), stops matching, and the axis falls to `None`. Fail-open is required: the drift test forbids the wildcard that would over-drop. ONE COUNTER DERIVATION, NOT TWO. The ∞ display registration was partitioned by `generic_counter_is_growable` — the ω-cover's `Generic`-only rule — while the boundary materializes the strictly wider beneficial partition, so a +1/+1-growing loop collapsed correctly and never rendered ∞. Display now projects from the same `current_period_counter_growth` the stash consumes, which also deletes a duplicate period drive; `current_period_counter_targets` and `grown_generic_counter_targets` go with it. THE PROMPTED SEAT (CR 732.2a). `FamilyCollapseState::Scheduled` carries the seat that will be asked to name N, captured at the controller key before `attribution_player` may replace `player` with the victim. It lives INSIDE the variant rather than beside it because `derive_views` folds families through exactly one `merge`: a sibling field would need a second parallel accumulator whose merge rule no type enforces. Inside, `Unscheduled ⊔ Scheduled → Mixed` drops the seat structurally, and two disagreeing controllers meet to `None` on a flat lattice, so the fold is order-independent. The badge compares it against `usePlayerId()` gated by `useSpectatorMode()` — not `usePerspectivePlayerId()`, which returns the controlled seat under a turn-control effect and would address the copy to the wrong player. Wire shape changes: `Scheduled`'s serde payload goes from a bare string tag to an object. `DerivedViews` is never persisted, so there is no save migration, but an out-of-repo consumer on the old enum fails loudly rather than silently defaulting. `GameState::unbounded_counter_targets` is serialized and its content widens to the beneficial partition; `#[serde(default)]` keeps old saves loadable, though the ∞ pill set such a save restores is the pre-widening one. Known-open, disclosed rather than implied fixed: a mixed stash (≥2 accepts by one controller) can still suppress an axis removal — that pre-exists here for `Generic` pairs and this change enlarges its domain rather than creating it. The `CardPreview` F2 gap is likewise unchanged; its dangling symbol reference is repointed because the function it named is deleted, not because the gap closed. Assisted-by: ClaudeCode:claude-opus-4.8
An infinite object-growth loop shortcut published a ceiling of
MAX_SHORTCUT_CYCLES but seeded its own iteration_count at Fixed(1), so the
finite resolution a player accepted yielded exactly one counter no matter
what they chose. Reported from a real 4-player game (kilo + Freed from the
Real + Relic + Pentad); the capture is committed as the regression fixture
and drives the test through the production load path.
CR 732.2a: the shortcut proposal must state the number of iterations it is
offering. CR 732.2c: the count the players agree to is the count that is
taken. The producer now seeds the schema with the very ceiling it publishes,
so the offer, the collapse boundary, and the interaction picker all name the
same number.
THE SECOND DECLARE AUTHORITY. materialize_loop_shortcut_response maps
(AcceptSuggested, Fixed { suggested, .. }) to the declared count, and that
path is live today via the server's handle_full_game_submission and the WASM
submit_interaction_js. Post-fix it emits the byte-identical DeclareShortcut
the React echo already produced; fixing the producer without this moving
would have been the defect.
PROMPT WORDING. The count line reads "Repeat at most once." / "Repeat at
most N times." — the cap is an upper bound, not a promise, and the wording
now says so. English is written into all seven locale catalogs to satisfy
the key-set parity gate without inventing translations.
DISCLOSED, NOT FIXED HERE. Resolving the full 1000 cycles takes 67.572 s in
a debug build (super-linear; release unmeasured). The cap stays at 1000 and
the O(N) counter-side replay is a scheduled follow-up — see LEDGER 5. The
wire-visible `suggested` value changes 1 -> 1000 for this offer class; no
field or variant shape changes.
Assisted-by: ClaudeCode:claude-opus-5
a11f2be to
c9afcd8
Compare
|
🤖 AI text below 🤖 Addressing the maintainer hold at Precondition. #7045 merged as 1. Rebased onto the resulting 2. All head-bound verification regenerated at 3. Parse evidence regenerated. The coverage-parse-diff sticky is now bound to One thing worth flagging in the resolution, because it did not present as a conflict. The conflict was in the CR 603.5 prompt-census drift log. Both sides independently moved the pinned producer That the pin is load-bearing was measured rather than asserted: restoring git's auto-merged The same shape appeared a second time: an upstream-owned assertion in
Full evidence, plus the saved-game/wire-surface and performance disclosures, is in the updated PR body. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/components/card/__tests__/CardPreview.test.tsx`:
- Around line 625-638: The engine pipeline must expose a renderable target for
beneficial counter growth when grown_beneficial_counter_deltas detects a 0→1
transition absent from the live obj.counters, covering +1/+1, loyalty, and
defense counters. Materialize or otherwise publish the engine-derived target
before adapter output so CardPreview can render the unbounded marker without
client-side state inference, and add a regression test covering this 0→1 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: ed44c26e-7ff4-4dc0-8476-632e00deb396
⛔ Files ignored due to path filters (1)
crates/engine/tests/fixtures/kilo_freed_relic_pentad_max_of_one_4p.json.gzis excluded by!**/*.gz
📒 Files selected for processing (26)
client/src/adapter/types.tsclient/src/components/card/__tests__/CardPreview.test.tsxclient/src/components/hud/HudBadges.tsxclient/src/components/hud/__tests__/UnboundedBadge.test.tsxclient/src/components/modal/__tests__/LoopShortcutModal.test.tsxclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsonclient/src/test/fixtures/unbounded-counter-wire.jsonclient/src/test/fixtures/unbounded-token-wire.jsonclient/src/viewmodel/__tests__/unboundedWireSeam.test.tscrates/engine/src/analysis/resource.rscrates/engine/src/game/derived_views.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/interaction.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_counter_growth.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rs
|
🤖 AI text below 🤖 @coderabbitai — the mechanism is correct, and my first reading of it was wrong. Recording both, because the way it was wrong is the interesting part. I initially refuted this from Two corrections to the finding's framing, both measured: It is pre-existing, not introduced here. The gap and its characterization test are both present at this PR's base But you are right that this PR makes it worse, and that is the part worth having flagged. Collapsing the display channel and the batched-collapse δ stash onto one derivation — which is what makes the ∞ pills and the growth that actually lands unable to disagree — leaves the mechanism untouched (same frame diff, same BEFORE clone) while enlarging the affected class from Not fixed in this PR, and deliberately. The repair is an engine display-channel change — publish an engine-derived render target, or materialize it before adapter output — which is outside this PR's frozen scope, and I agree with your framing that the client must not infer it: the existing gap test asserts precisely that the frontend must not synthesize a counter row the engine says does not exist. Your suggested acceptance criterion is the right one, so it is recorded as written: an engine-pipeline regression test driving the Now carrying a scheduled Tier M backlog slot rather than living only as an in-code comment, and disclosed in the PR body under the saved-game/wire disclosures with the widened domain named explicitly. |
|
Tip For best results, initiate chat on the files or code changes.
I agree with the updated scope. The display gap predates this PR. This PR widens the affected domain to The client must not synthesize the missing row. The fix belongs in the engine display channel or in adapter materialization. A regression test must drive the simulated The scheduled Tier M backlog slot and the PR disclosure are sufficient for this PR. Would you like me to open a GitHub follow-up issue that records the required engine change and acceptance criterion? 🐇🔍 ✏️ Learnings added
You are interacting with an AI system. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head changes requested
Reviewed c9afcd8cce088473cc45cd239788c32cae5469c1.
[HIGH] The engine still publishes counter ∞ targets that the rendered object cannot display. derived_views.rs:1331-1350 forwards every registered (object, counter-type) target as long as the object remains on the battlefield, but it does not publish or materialize a corresponding renderable counter row. The existing CardPreview regression at client/src/components/card/__tests__/CardPreview.test.tsx:625-639 documents the resulting 0 → 1 failure: the period is driven on clone frames, so a beneficial counter may be registered while the live object has no entry in obj.counters, and all display modes iterate that live map. The frontend must not infer game state to paper over this.
This head widens that unresolved gap from Generic counters to the full beneficial partition: +1/+1, loyalty, and defense in addition to Generic. Fix it at the engine/derived-view seam by publishing a renderable counter target or materializing it before adapter output, and add an engine-pipeline regression that drives the 0 → 1 path and proves the adapter-visible state is renderable for the marked counter. The current-head CodeRabbit review independently confirms this requirement.
[LOW] Update the wire-contract description in client/src/adapter/types.ts. Its unbounded_counters documentation still describes only preserved Generic counters, while the engine contract now covers the full beneficial materializable partition. Align the comment with the actual engine-owned contract.
Keep auto-merge and merge-queue entry disabled until the engine-owned display contract and regression are in place.
The engine registered an (object, counter-type) pair as an infinity display
target but published only the type. Every client display mode draws pills by
iterating the object's own `counters` map, so a pair an accepted loop pumps
from 0 -> 1 -- registered while the live object still carries none -- had no
row to draw and rendered nowhere. Widening the registration from Generic to
the full beneficial-materializable partition enlarged the affected class
without touching that mechanism.
`DerivedViews::unbounded_counters` now carries `UnboundedCounterView
{ counter, count }` rather than a bare `CounterType`, with the count read from
the live object at projection time. The row is self-sufficient: the display
neither infers that a row exists nor computes its count, so the client stays a
display layer and the existing regression's prohibition on client-side
synthesis remains in force.
CR 122.1: a counter is a marker on an object, so a row names an
(object, counter) pair rather than a stored quantity; the absent-means-zero
convention mirrors `analysis::resource::grown_beneficial_counter_deltas` so
producer and projector share one definition of "absent".
CR 110.1 / CR 122.2: a row drops when the object leaves the battlefield.
CR 306.5c: a planeswalker's loyalty is its loyalty-counter count, so loyalty
totals render the infinity marker. CR 606.4 / CR 606.5 keep activation-cost
badges out, an activation cost being a number of counters to pay rather than a
total.
The regression drives the production path -- ActivateAbility, DeclareShortcut,
RespondToShortcut::Accept, materialize_object_growth_shortcut,
register_unbounded_counter_targets, derive_views -- and asserts the 0 -> 1 row
both in `DerivedViews` and in the serialized adapter envelope, with a sibling
test pinning a nonzero count so neither can pass vacuously. Reverting the fix
reds exactly the two 0 -> 1 tests and leaves every N -> N+1 sibling green.
Assisted-by: ClaudeCode:claude-opus-5
`unbounded_counter_targets` is keyed per seat, so its `BTreeSet` dedupes within a controller and never across controllers. Two accepted loops pumping the same (object, counter) pair each held their own entry, and the projector pushed both. Before this PR that surfaced as one pill, because the display iterated the object's own counter map and applied the mark as a membership test; publishing rows made the duplicate visible as two pills sharing one React key, since every render site keys on the counter type alone. The rows are byte-identical -- `count` is read from `state.objects` keyed only by `(id, ct)`, with no seat input -- so collapsing them is a deduplication, not a choice of whose row wins. Flattening into a `BTreeSet` also drops a nesting level; its `(ObjectId, CounterType)` ordering is what the single-seat case already produced, so no existing assertion moves. Fixed at the seam that owns the row set: deduplicating engine-published game state in the display layer is the inversion this codebase forbids. Also corrects two doc self-contradictions this PR introduced -- the field and projector comments still described publishing counter *types* and marking an existing pill -- and re-splits a compound citation so CR 122.1 carries the marker-on-an-object half, CR 110.1 the permanent-on-the-battlefield half, and CR 122.2 the zone-change half. Assisted-by: ClaudeCode:claude-opus-5
The dedupe added in the previous commit had no over-collapse control: the only two-seat registration in the tree registered the same pair twice, so narrowing the set key -- or the merged multi-seat ordering, which nothing asserted -- would have redded nothing. The fixture now registers a shared pair from two seats and a distinct pair from one, with unequal counts so a collapse that kept the wrong row cannot coincide, and asserts both surviving rows in `(ObjectId, CounterType)` order. Measured in both directions: narrowing the key to `ObjectId` drops to one row, removing the dedupe yields three. Also corrects two doc defects this PR introduced. `NO-SURFACE-IS-FILTERED invariant` was cited at two sites and stated at neither -- and on inspection named a different proposition at each, plus a third one elsewhere in the file, so the name is dropped and each site now says its own claim. And the field and projector comments described the published set as "PRESERVED BENEFICIAL", where "preserved" is this repo's term for `is_monotone_loop_resource() == false`, which intersects the published partition only at `Generic`; both now read "BENEFICIAL-MATERIALIZABLE", the predicate the production path actually filters on. No production behavior changes: `derived_views.rs` is comment-only here. Assisted-by: ClaudeCode:claude-opus-5
Resolves the CR 603.5 prompt-census pin conflict in `mod stage2_injector_tests`. Both incoming coordinates were stale, each correct only for its own side: this branch's `:11837` counts the search-observer -7, the axis-scoped -7 and the CR 500.5 +23; upstream's `:11858` counts the Ward continuation +13 and the durable-knowledge hooks +24. The shifts are independent and compose, so accepting either verbatim would have been wrong by 37 or by 16. Resolved by locating the producer by content -- the line whose sha256 with its trailing newline is 8a544e87..5cc7d63, matching exactly one line under a whole-file scan -- and only then computing 11828 -7 -7 +23 +13 +24 = 11874 as a check, which agrees. The pin is proven load-bearing: mutating it to upstream's :11858 fails the census test, whose own producer scan independently reports :11874. Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 Both findings from your review of [HIGH] fixed at the engine seam. The regression is One thing I want to be explicit about rather than let the diff imply: the row's Fixing this also surfaced a defect of my own that was not in your review: the registration store is per-seat, so it deduped within a seat and never across seats, which would have published a duplicate row for a pair two seats both registered. Fixed by collapsing at the source, with a both-directions negative control so the test cannot pass by collapsing everything. [LOW] fixed. This head merges The merge produced one real conflict, in the CR 603.5 prompt-census drift log, and both incoming numbers were stale — each correct only for its own side. I resolved it by content, not by accepting the 3-way result: the pinned producer was located by sha256 of the line at I left one inherited comment inside that drift log byte-identical even though it reads in upstream's voice — it is #7094's own provenance prose, and after squash it resolves correctly against Worth your attention, inherited not introduced: Verification at |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine/src/game/engine.rs (1)
5839-5847: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPass the typed boundary instead of a derived
bool.
resume_get_player_counters_unless_paymentreceivesmatches!(boundary, CostMoveDrainBoundary::ReplacementDelivered { .. }). The callee then cannot distinguishReplacementPreventedfromPriorityBoundary, and a future third boundary silently maps tofalse.CostMoveDrainBoundaryis already in scope and carries the same information with more meaning. Pass it and let the callee match exhaustively.♻️ Proposed signature change
- engine_payment_choices::resume_get_player_counters_unless_payment( - state, - events, - matches!(boundary, CostMoveDrainBoundary::ReplacementDelivered { .. }), - )? + engine_payment_choices::resume_get_player_counters_unless_payment(state, events, boundary)?As per coding guidelines: "any new
boolstruct field orboolvariant payload where a typed enum ... would carry the same information with more meaning".🤖 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/engine.rs` around lines 5839 - 5847, Update the call to `resume_get_player_counters_unless_payment` in the `pending_cost_move_resume` branch to pass the typed `CostMoveDrainBoundary` value instead of the derived `matches!` boolean. Adjust the callee’s signature and internal handling to match the boundary enum exhaustively, preserving distinct behavior for `ReplacementDelivered`, `ReplacementPrevented`, and `PriorityBoundary`.Source: Coding guidelines
crates/engine/src/game/derived_views.rs (1)
816-826: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftAvoid re-running
filter_state_for_viewerjust to collectdisplay_visible_to_viewerids.
wrapis on the broadcast serialization path and callsfilter_state_for_viewer, which clones the fullGameStateand applies every redaction pass, but only consumes one boolean per object. Move identity-display projection to a shared predicate/helper that can computedisplay_visible_object_idswithout cloning the authoritive state.🤖 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 816 - 826, Refactor the display_visible_object_ids computation in wrap to use a shared identity-display predicate/helper instead of calling filter_state_for_viewer and cloning the full GameState. Preserve the existing viewer-specific filtering semantics by applying the helper per object, and reuse that helper wherever display visibility is determined.
🧹 Nitpick comments (1)
client/src/adapter/types.ts (1)
2731-2734: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMatch
UnboundedCounterView.countertoCounterType.
GameObject.counterskeys arePartial<Record<CounterType, number>>, and existing counter fields useCounterType.UnboundedCounterView.counteris already type-checked against those same serde strings, so it can keep the typedCounterTypeunion instead of narrowing the API tostring.♻️ Proposed type tightening
export interface UnboundedCounterView { - counter: string; + counter: CounterType; count: number; }🤖 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/adapter/types.ts` around lines 2731 - 2734, Update UnboundedCounterView.counter from string to CounterType, matching the CounterType-based typing used by GameObject.counters and existing counter fields while preserving the current serialized counter values.
🤖 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/hooks/useUnboundedCounterRows.ts`:
- Around line 67-81: Move the counter-row projection out of the
useUnboundedCounterRows hook and into the engine, exposing one complete typed
projection that includes finite rows, unbounded status, and loyalty-total
status. Update the hook and its consumers to render this engine-provided
projection directly, removing client-side joining, duplicate filtering,
isUnbounded computation, and counter-name interpretation such as “loyalty”;
preserve empty-row behavior through the engine projection.
---
Outside diff comments:
In `@crates/engine/src/game/derived_views.rs`:
- Around line 816-826: Refactor the display_visible_object_ids computation in
wrap to use a shared identity-display predicate/helper instead of calling
filter_state_for_viewer and cloning the full GameState. Preserve the existing
viewer-specific filtering semantics by applying the helper per object, and reuse
that helper wherever display visibility is determined.
In `@crates/engine/src/game/engine.rs`:
- Around line 5839-5847: Update the call to
`resume_get_player_counters_unless_payment` in the `pending_cost_move_resume`
branch to pass the typed `CostMoveDrainBoundary` value instead of the derived
`matches!` boolean. Adjust the callee’s signature and internal handling to match
the boundary enum exhaustively, preserving distinct behavior for
`ReplacementDelivered`, `ReplacementPrevented`, and `PriorityBoundary`.
---
Nitpick comments:
In `@client/src/adapter/types.ts`:
- Around line 2731-2734: Update UnboundedCounterView.counter from string to
CounterType, matching the CounterType-based typing used by GameObject.counters
and existing counter fields while preserving the current serialized counter
values.
🪄 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: 7f0b6b68-15de-41c8-b034-ec865cf1b9ca
📒 Files selected for processing (29)
client/src/adapter/types.tsclient/src/components/board/PermanentCard.tsxclient/src/components/board/__tests__/PermanentCard.test.tsxclient/src/components/card/ArtCropCard.tsxclient/src/components/card/CardPreview.tsxclient/src/components/card/__tests__/ArtCropCard.test.tsxclient/src/components/card/__tests__/CardPreview.test.tsxclient/src/components/ui/LoyaltyBadge.tsxclient/src/components/ui/__tests__/LoyaltyBadge.test.tsxclient/src/hooks/useUnboundedCounterRows.tsclient/src/hooks/useUnboundedCounterTypes.tsclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsonclient/src/test/fixtures/unbounded-counter-wire.jsonclient/src/viewmodel/__tests__/cardProps.test.tsclient/src/viewmodel/__tests__/unboundedWireSeam.test.tscrates/engine/src/game/derived_views.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_resolution_choices.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_counter_growth.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rs
💤 Files with no reviewable changes (1)
- client/src/hooks/useUnboundedCounterTypes.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- client/src/i18n/locales/it/game.json
- client/src/i18n/locales/es/game.json
- client/src/i18n/locales/pt/game.json
- client/src/i18n/locales/pl/game.json
- crates/engine/tests/integration/loop_shortcut_mana_engine.rs
- client/src/test/fixtures/unbounded-counter-wire.json
- crates/engine/src/game/engine_resolution_choices.rs
- client/src/i18n/locales/de/game.json
- crates/engine/src/types/game_state.rs
- client/src/i18n/locales/en/game.json
- client/src/i18n/locales/fr/game.json
- crates/engine/tests/integration/combo_infinite_pile.rs
| return useMemo(() => { | ||
| if (engineRows.length === 0) { | ||
| if (!objectCounters) return EMPTY_ROWS; | ||
| const finiteOnly = Object.entries(objectCounters) | ||
| .filter((entry): entry is [string, number] => entry[1] != null) | ||
| .map(([type, count]) => ({ type, count, isUnbounded: false })); | ||
| return finiteOnly.length === 0 ? EMPTY_ROWS : finiteOnly; | ||
| } | ||
| const marked = new Set(engineRows.map((r) => r.counter)); | ||
| return [ | ||
| ...engineRows.map((r) => ({ type: r.counter, count: r.count, isUnbounded: true })), | ||
| ...Object.entries(objectCounters ?? {}) | ||
| .filter((entry): entry is [string, number] => entry[1] != null && !marked.has(entry[0])) | ||
| .map(([type, count]) => ({ type, count, isUnbounded: false })), | ||
| ]; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Move counter-row projection into the engine.
This hook synthesizes finite rows from GameObject.counters, computes isUnbounded, and filters duplicate counter types. Its consumers also infer that "loyalty" changes the loyalty-total display. These are game-data projection and rules semantics in client/src.
Publish one complete typed counter-display projection from the engine. Include finite rows, unbounded status, and loyalty-total status. Make the client render that projection without joining, filtering, or interpreting counter names.
As per path instructions, “The frontend is a display layer, never a logic layer” and “any computation, derivation, filtering, or inference of GAME data inside client/src/” must move to the engine.
🤖 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/hooks/useUnboundedCounterRows.ts` around lines 67 - 81, Move the
counter-row projection out of the useUnboundedCounterRows hook and into the
engine, exposing one complete typed projection that includes finite rows,
unbounded status, and loyalty-total status. Update the hook and its consumers to
render this engine-provided projection directly, removing client-side joining,
duplicate filtering, isUnbounded computation, and counter-name interpretation
such as “loyalty”; preserve empty-row behavior through the engine projection.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current head 4f0554f53d93028d313266f03519d5536aafcacb
[HIGH] The new frontend hook derives counter display state instead of rendering an engine-owned projection. Evidence: client/src/hooks/useUnboundedCounterRows.ts:67-81 joins DerivedViews::unbounded_counters with GameObject.counters, filters entries, deduplicates them using Set, orders the result, and assigns the isUnbounded display interpretation. That path also decides how counter types such as loyalty are represented.
Why it matters: this violates the display-only boundary and creates a second, incomplete authority for which counters a permanent displays and how they are classified. Any engine rule or derived-view change can now leave the UI silently stale or semantically wrong even while its wire types still compile.
Suggested fix: have the engine publish a complete per-object counter-display projection (all rendered counter rows, count, and unbounded/loyalty presentation semantics as typed engine data) in derived views, then make the hook/selectors render that projection without joining, filtering, deduplicating, or interpreting GameObject.counters. Add engine and wire/adapter coverage for the complete projection; UI tests should only assert rendering of that data.
CI is still incomplete (Rust lint and both Rust test shards are pending). The parse-diff sticky is stale additional evidence; neither condition replaces this architecture block.
DerivedViews::unbounded_counters becomes counter_display, keyed by ObjectId
and carrying every rendered counter row rather than only the infinity-marked
counter types. Each row is CounterRowView { counter, count, magnitude }, with
CounterMagnitude { Finite, Unbounded } typed rather than a bool, and rows are
pre-partitioned into ObjectCounterDisplay { pills, loyalty } so the display
layer selects, filters and interprets nothing.
Row key is (ObjectId, CounterType) per CR 122.1, so one pair cannot produce
two rows and the client-side dedupe/shadow rule ceases to exist rather than
moving. Infinity annotation existence stays gated on the loop stores plus live
battlefield membership (CR 122.2 + CR 110.1); finite row existence gates on the
object's own counters, deferring to zones::counters_persist_on_move for the
CR 113.6b carve-out rather than re-deriving a second, weaker zone rule.
Extends the NO SURFACE IS FILTERED BY THE SCHEDULE invariant to the widened
projection: the schedule may annotate a row, never admit or withhold one.
Assisted-by: ClaudeCode:claude-opus-5
Adds the four-arm CR 122.2 test driving a real accepted counter-growth loop: a populated row, the store still holding the pair after the bearer leaves the battlefield, the row gone, and the bearer's counter map empty. The store arm is what separates 'the projection gated it' from 'the store was wiped'. Retargets the boundary-collapse assertion rather than renaming it: that frame leaves the bearer on the battlefield carrying real counters, so the widened projection emits a finite row where the old channel emitted nothing. It is now the only test of the infinity-clears-while-the-finite-row-survives transition. Replaces the goldens' filter_map key list with an exact key-set assertion. The old form dropped a misspelled key silently and the drift compare then agreed with itself. Assisted-by: ClaudeCode:claude-opus-5
…g it The hook joined the engine channel with GameObject.counters, filtered entries, deduplicated them with a Set, ordered the result and assigned the unbounded display interpretation - a second, incomplete authority for what a permanent displays. It now reads the engine's rows and nothing else. Deletes the obj.counters fallback deliberately: a state envelope missing derived now renders no pills at all, where it previously still rendered the finite half. Restoring a fallback would restore the authority this deletes. Assisted-by: ClaudeCode:claude-opus-5
…rows groupKey hashed obj.counters but carried no term for the engine's counter rows, so permanents differing only in their infinity marks collapsed into one group whose representative spoke for members it disagreed with. It now keys on the projection entry, applying groupKey's existing contract - collapse only what renders identically - to the field that was missing. groupByName's new parameter is required rather than defaulted so the compiler enumerates every call site; a default would have left BattlefieldPeekPopover silently on the old behavior with no type error. Assisted-by: ClaudeCode:claude-opus-5
…e projection
Closes the review MED that partition D introduced. `groupAttackers` began keying
attacker-stack identity on the engine's `counter_display` projection, but
`AttackTargetPicker` still built its chips from `Object.entries(obj.counters)`
and rendered no unbounded mark. Two same-named attackers with byte-identical
`counters` where only one carried an unbounded row therefore split into two
stacks with identical name, identical chips and identical `x1` — a split with no
visible cause, in the attack-assignment modal.
`StackLabel` now reads `pillsOf(useCounterDisplay(stack.ids[0]))`, so the chips
come from the same rows the grouping keyed on and the split explains itself.
`ids[0]` is exactly the object `representative` is defined from (combat.ts), so
the hook call is unconditional. This also fixes a pre-existing defect in the same
lines: an unbounded-marked attacker rendered as plain in that modal.
`objectCounterChips` is deleted rather than rewired. It re-derived what the
engine projection already published — dropping zero-count entries (CR 122.1),
excluding the loyalty total (CR 306.5c) and ordering the rows — making it a
fifth client-side counter authority of exactly the kind this PR is removing.
The unbounded spelling mirrors `PermanentCard` verbatim; no new one is invented.
Also in this round:
- Split a doc block in `derived_views.rs` that documented `derive_views` while
sitting on `pending_payment_remaining`. The misattachment predates this PR,
but this PR extended the block, so it is closed here. Pure doc move: 12 lines,
identical multiset, zero code tokens touched.
- Corrected the follow-up ledger in `useCounterDisplay.ts`. It undercounted the
surviving `obj.counters` readers and mischaracterized them. `CardChoiceModal`
enumerates counters removable as a cost (CR 118.3) and `DebugCardContextMenu`
reads the map its own buttons mutate; both legitimately need the raw map,
including loyalty, and are not pending display conversions. Its claim that
FU-B was blocked on a missing test file was also false — that test exists.
- Typed `CounterRowView.counter` as `CounterType` rather than `string`, matching
the Rust field. Safe because the TS union carries a `(string & {})` arm.
Assisted-by: ClaudeCode:claude-opus-4.8
|
🤖 AI text below 🤖 Fixed at The projection is now engine-owned and complete. One consequence worth calling out explicitly, because it is a deliberate behavior change. There is no fallback to Your point about a second authority going stale was load-bearing — our own review found a live instance of exactly that. Keying attacker-stack identity on the new projection while Coverage. Engine-side, Verification at this head: Two findings from the review bots I deliberately did not fix, both measured pre-existing and disclosed in the PR body rather than folded in: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/engine/tests/integration/combo_infinite_pile.rs (1)
288-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the four channel names into one shared constant.
The guard is correct, and the union with
kilo_live_offer_from_real_dump's twin guard does span all four names today. The coverage depends on both files keeping identical request arrays, and only the comments enforce that. If an edit changes one array,filter_mapdrops the name silently in that file and the paired guard in the other file still passes — the exact failure mode this guard was added to catch.A single
pub(crate) const WIRE_GOLDEN_CHANNELS: [&str; 4]in a shared test module, used by both emitters, makes the coupling a compile-time fact instead of a comment.🤖 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 288 - 311, Extract the four channel names into a shared `pub(crate) const WIRE_GOLDEN_CHANNELS: [&str; 4]` in the common test module, then update this guard and the twin guard in `kilo_live_offer_from_real_dump` to derive their expected `BTreeSet` from that constant. Use the same constant for both emitters’ request arrays so the channel set and requested channels cannot drift independently.
🤖 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 3025-3028: Update the consumer contract comment near the pills
ordering rules to state that zero-count entries are dropped only during the
finite pass; preserve that unbounded rows from unbounded_counter_targets may
retain count: 0, including the published 0-to-1 case described by
CounterRowView.
In `@client/src/components/controls/__tests__/AttackTargetPicker.test.tsx`:
- Around line 208-251: Update the projection-only lore pill in the test using
the derived counter_display fixture to a nonzero count, such as 1, and update
its corresponding rendered-text assertion from lore x0 to lore x1. Preserve the
coverage proving the UI renders projection-only rows and does not fall back to
the raw counters map, without requiring zero-count rows.
---
Nitpick comments:
In `@crates/engine/tests/integration/combo_infinite_pile.rs`:
- Around line 288-311: Extract the four channel names into a shared `pub(crate)
const WIRE_GOLDEN_CHANNELS: [&str; 4]` in the common test module, then update
this guard and the twin guard in `kilo_live_offer_from_real_dump` to derive
their expected `BTreeSet` from that constant. Use the same constant for both
emitters’ request arrays so the channel set and requested channels cannot drift
independently.
🪄 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: 21102770-3188-44f1-a5f7-c8003f74ec33
📒 Files selected for processing (24)
client/src/adapter/types.tsclient/src/components/board/PermanentCard.tsxclient/src/components/board/__tests__/PermanentCard.test.tsxclient/src/components/card/ArtCropCard.tsxclient/src/components/card/CardPreview.tsxclient/src/components/card/__tests__/ArtCropCard.test.tsxclient/src/components/card/__tests__/CardPreview.test.tsxclient/src/components/controls/AttackTargetPicker.tsxclient/src/components/controls/__tests__/AttackTargetPicker.test.tsxclient/src/components/hud/BattlefieldPeekPopover.tsxclient/src/hooks/useCounterDisplay.tsclient/src/test/fixtures/unbounded-counter-wire.jsonclient/src/utils/combat.tsclient/src/viewmodel/__tests__/battlefieldGrouping.test.tsclient/src/viewmodel/__tests__/cardProps.test.tsclient/src/viewmodel/__tests__/unboundedWireSeam.test.tsclient/src/viewmodel/battlefieldProps.tsclient/src/viewmodel/cardProps.tsclient/src/viewmodel/gameStateView.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_counter_growth.rs
💤 Files with no reviewable changes (1)
- client/src/viewmodel/cardProps.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- client/src/viewmodel/tests/cardProps.test.ts
- client/src/test/fixtures/unbounded-counter-wire.json
- client/src/components/card/ArtCropCard.tsx
- client/src/components/board/PermanentCard.tsx
- client/src/viewmodel/tests/unboundedWireSeam.test.ts
- crates/engine/src/types/game_state.rs
- client/src/components/card/tests/CardPreview.test.tsx
| it("renders counter chips from the engine projection, never the raw counters map (CR 122.1)", () => { | ||
| // The raw map and the projection DISAGREE on every axis: a differing count, a row the map | ||
| // does not carry at all, and a map entry the projection dropped. Only a StackLabel reading | ||
| // `derived.counter_display` can satisfy all three, so any reintroduced `obj.counters` read | ||
| // (or a join back to it) fails here. | ||
| const goblin = makeCreature(101, "Goblin"); | ||
| useGameStore.setState({ | ||
| gameState: buildGameState({ | ||
| players: buildPlayers([0, 1, 2]), | ||
| seat_order: [0, 1, 2], | ||
| objects: buildObjectMap({ ...goblin, counters: { charge: 99, stun: 2 } }), | ||
| derived: { | ||
| counter_display: { | ||
| "101": { | ||
| pills: [ | ||
| { counter: "charge", count: 4 }, | ||
| // CR 122.1: engine-supplied row with NO entry in the raw map — a pair pumped | ||
| // 0 -> 1 is registered while the object still carries none, so it is | ||
| // unreachable by any client-side derivation from `obj.counters`. | ||
| { counter: "lore", count: 0 }, | ||
| ], | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| }); | ||
| render( | ||
| <AttackTargetPicker | ||
| validTargets={TARGETS} | ||
| selectedAttackers={[101]} | ||
| onConfirm={vi.fn()} | ||
| onCancel={vi.fn()} | ||
| />, | ||
| ); | ||
| enterDistribute(); | ||
|
|
||
| // Projection count wins over the raw map's disagreeing count. | ||
| expect(screen.getAllByText("charge x4").length).toBeGreaterThan(0); | ||
| expect(screen.queryAllByText("charge x99")).toHaveLength(0); | ||
| // Projection-only row renders even though the raw map has no such key. | ||
| expect(screen.getAllByText("lore x0").length).toBeGreaterThan(0); | ||
| // Raw-map-only entry the projection dropped must NOT render. | ||
| expect(screen.queryAllByText("stun x2")).toHaveLength(0); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not require a finite zero-count counter pill.
Line 227 creates a finite lore row with count: 0. Line 248 requires the UI to render lore x0. The PR contract says the engine omits zero-count rows. This test can preserve an invalid display state.
Use a nonzero projection-only row, such as lore: 1, to retain the no-fallback coverage. As per PR objectives, counter-display rows omit zero-count entries.
🤖 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/components/controls/__tests__/AttackTargetPicker.test.tsx` around
lines 208 - 251, Update the projection-only lore pill in the test using the
derived counter_display fixture to a nonzero count, such as 1, and update its
corresponding rendered-text assertion from lore x0 to lore x1. Preserve the
coverage proving the UI renders projection-only rows and does not fall back to
the raw counters map, without requiring zero-count rows.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current head 3a34a1b18b11c9267422477ed484a13bd4b2e995
[HIGH] DialogAttachmentCard still bypasses the engine-owned counter-display projection. Evidence: client/src/components/hud/DialogAttachmentCard.tsx:115-118 enumerates and filters raw obj.counters, then :212-221 renders those derived finite pills. It cannot render an engine-projected Unbounded pill. The new ledger in client/src/hooks/useCounterDisplay.ts:47-54 explicitly identifies this as the sole remaining display conversion.
Why it matters: the renderer reintroduces a second frontend authority for counter visibility/classification and silently drops engine-projected unbounded display state.
Suggested fix: consume the per-object pills projection at this renderer with no raw-map fallback or filter. Add rendering coverage for both a finite pill and an Unbounded pill.
[HIGH] Current-head parser-impact evidence is absent. The only <!-- coverage-parse-diff --> sticky is bound to prior head 4f0554f53d93028d313266f03519d5536aafcacb, not this head. Publish a current-head artifact and account for its result before re-review. Rust lint and both Rust test shards are also still pending.
[LOW] Projection documentation/fixtures should distinguish the zero-value cases precisely. Only a zero Finite row is omitted; zero Unbounded is valid and must remain displayable. Correct the docs/types accordingly and make the zero-Finite fixture nonzero so it proves the finite rendering path rather than an omission path.
Closes the maintainer's remaining display-authority finding. `DialogAttachmentCard` enumerated and filtered raw `obj.counters` and rendered those derived pills, so it could not show an engine-projected `Unbounded` row — the same second-authority defect this PR exists to remove, at the last site still holding it. It now reads `pillsOf(useCounterDisplay(objectId))` with no raw-map fallback and no filter, and renders `∞` using the spelling the other four sites already share. The hook is placed with the other hooks, above `if (!obj) return null` — at the natural spot beside the old counters read it would sit after that early return and be a conditional hook. Also corrects a documentation claim that was wrong in a way a test was hiding. `counter_display_views` drops zero-count rows only in its finite pass; the unbounded pass publishes a live count, so a `count: 0` Unbounded row is legitimate and must stay displayable — it is exactly the `0 -> 1` case this PR set out to render. The consumer contract in `adapter/types.ts` claimed zero-count entries were dropped outright. The picker fixture had encoded the same misunderstanding: it seeded a zero-count row with no `magnitude`, so serde defaulted it to `Finite` — a shape the engine provably never emits — and asserted it rendered. That row is now nonzero, so the test proves the finite rendering path instead of an omission path, and the zero-count Unbounded case gets real coverage in the attachment tests instead. Finally, the two wire-golden emitters hard-coded the same four channel names with only comments coupling them, so editing one could silently drop a name there while the paired guard in the other still passed. Both now derive from a single `WIRE_GOLDEN_CHANNELS` const, which makes the coupling a compile-time fact: a typo in the shared list now fails both files. Assisted-by: ClaudeCode:claude-opus-4.8
…t could not A zero-count `Unbounded` row is legitimate and must stay displayable — it is the `0 -> 1` case this PR set out to render, and the reason the engine drops zero rows only in its finite pass. Three of the five subscribed render sites had a fixture that would catch a `count > 0` filter deleting those rows. Two did not. The picker lost its guard in this PR. Its fixture had seeded a zero-count row with no `magnitude`, so serde defaulted it to `Finite` — a shape the engine provably never emits — and the previous commit correctly made it nonzero. But the right replacement was a *valid* zero row, not none: removing it left the site unable to detect the regression its own comments warn about. It now carries a zero-count `Unbounded` row asserted as `∞`. The art-crop gap is older than this PR and is closed here too, since it is one fixture in a file already changed and it completes an invariant this PR is what establishes. Both new assertions were probed with the mutant they exist to catch, and each is the sole failure under it, so neither rides another assertion's coverage. The picker's separate projection-vs-raw-map discriminator was re-checked and still fails under a join-back mutant. Assisted-by: ClaudeCode:claude-opus-4.8
The fixture seeded `[charge(Finite), lore(Finite), quest(Unbounded)]`, which is the
exact reverse of what `counter_display_views` produces for that content: the
unbounded pass runs first, then the finite rows in `CounterType` declaration order,
where `Lore` precedes `Generic("charge")`. `derived_views` states that contract
directly — "`Unbounded` rows lead, then `CounterType`'s declaration `Ord` inside
each class" — so the PR was shipping a documented order alongside a fixture
demonstrating its reverse.
Nothing was failing: every assertion in that test is order-independent, and the
fixture's contents are unchanged. It is fixed because row order is load-bearing
downstream — `battlefieldProps` folds it into `groupKey` — and a fixture depicting
a frame the engine cannot emit invites the wrong conclusion about whether order
matters. A comment now names the authority so the next row added does not
re-scramble it.
No order-asserting UI test accompanies this, deliberately. Order is already pinned
engine-side, where `loop_counter_growth` asserts a real projection's row sequence;
asserting it again at a render site would put a second authority for counter
display back in the display layer, which is what this PR removes.
Assisted-by: ClaudeCode:claude-opus-4.8
|
🤖 AI text below 🤖 Fixed at [HIGH] DialogAttachmentCard. Converted. It now reads That was the last display holdout. The two remaining [HIGH] Parse-impact evidence. You were right that the sticky was bound to the prior head when you reviewed. CI refreshed it about six minutes later, and the three checks you flagged as pending — Rust lint and both test shards — have since passed. That sticky is now stale again by one head, since this push moves it; CI will republish against [LOW] Zero-value precision. Correct, and it was load-bearing in a way I did not expect. The docs are fixed: zero-count rows are dropped only in the finite pass, and a But making that fixture nonzero removed the site's only zero-row assertion, and our own review caught it: the right replacement was a valid zero row, not none. Measured with the mutant it exists to catch — One more fixture correction fell out of that review. The picker seeded its pills as Verification at this head: |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved at current head 519e999a3ab1ba437be86271568d9fce4f7f88a9: the counter-display projection is now consumed by the remaining renderer, current-head parse evidence reports no parser impact, and required CI is green.
2 participants
🤖 AI text below 🤖
Summary
Fixes both maintainer HIGHs by moving the entire counter-display projection into the engine.
The first HIGH (
c9afcd8cc): the engine published counter-∞targets the rendered object could not display.derive_viewsforwarded every registered(object, counter-type)pair while the object stayed on the battlefield but published only the type, so a pair an accepted loop pumps0 → 1— registered while the live object carries none — had nothing to render from.The second HIGH (
4f0554f53): the fix for the first one put the join in the wrong layer.useUnboundedCounterRowsjoinedDerivedViews::unbounded_counterswithGameObject.counters, filtered entries, deduplicated with aSet, ordered the result and assigned display meaning — a display layer computing game state. This head deletes that hook.DerivedViewsnow publishescounter_display: HashMap<ObjectId, ObjectCounterDisplay>, pre-partitioned by where each row renders: the engine drops zero-count entries in its finite pass (CR 122.1), splits the loyalty total out of the pill strip (CR 306.5c), dedupes across seats and orders the rows.useCounterDisplayis one keyed lookup that joins nothing, filters nothing and sorts nothing.UnboundedCounterView's boolean-ish shape is replaced by a typedCounterMagnitude { Finite, Unbounded }.There is deliberately no fallback to
objects[id].counters. A frame arriving with noderivednow renders no counter pills, where the deleted hook rendered the finite ones — the correct outcome of removing a second authority, and the reason a dropped-derivedadapter regression now fails visibly instead of silently half-correct.All five counter render sites now consume that projection.
DialogAttachmentCardwas the last holdout — it enumerated and filtered the raw map and so could not render an engine-projectedUnboundedpill at all; it now readspillswith no fallback and no filter. Two raw-map readers remain and are deliberate, permanent exclusions rather than pending work:DebugCardContextMenuis a counter editor reading the map its own +/− buttons mutate (loyalty included), andCardChoiceModalenumerates counters removable as a cost (CR 118.3), where loyalty is legal and anUnboundedmagnitude is meaningless.The zero-value contract is also stated precisely. Zero-count rows are dropped only in the projection's finite pass; the unbounded pass publishes a live count, so a
count: 0Unboundedrow is legitimate — it is exactly the0 → 1case this PR exists to render. The consumer docs previously claimed zero-count entries were dropped outright.Also in this head: the attack picker renders from the same projection.
groupAttackersbegan keying attacker-stack identity oncounter_display, butAttackTargetPickerstill built chips fromObject.entries(obj.counters)and rendered no∞— so two same-named attackers differing only in∞split into two stacks with identical name, chips and×1, a split with no visible cause.StackLabelnow reads the projection, which both explains the split and fixes a pre-existing defect in those lines: an∞-marked attacker previously rendered as plain there.objectCounterChipsis deleted rather than rewired — it re-derived exactly what the engine already published, making it a fifth client-side counter authority of the kind this PR removes.And the branch's original content: re-applying the
∞-revocation rules work onto the data model #7045 left behind (UnboundedResourceViewis now{player, axis}, with a(player, family)-keyedUnboundedFamilyViewcarryingFamilyCollapseState), plus a HOT playtest bugfix — an infinite object-growth loop shortcut published a ceiling ofMAX_SHORTCUT_CYCLESbut seeded its owniteration_countatFixed(1), so a finite resolution the player accepted yielded exactly one counter regardless of what they chose (CR 732.2a/732.2c).Files changed
client/src/adapter/types.tsclient/src/components/board/PermanentCard.tsxclient/src/components/board/__tests__/PermanentCard.test.tsxclient/src/components/card/ArtCropCard.tsxclient/src/components/card/CardPreview.tsxclient/src/components/card/__tests__/ArtCropCard.test.tsxclient/src/components/card/__tests__/CardPreview.test.tsxclient/src/components/controls/AttackTargetPicker.tsxclient/src/components/controls/__tests__/AttackTargetPicker.test.tsxclient/src/components/hud/BattlefieldPeekPopover.tsxclient/src/components/hud/DialogAttachmentCard.tsxclient/src/components/hud/HudBadges.tsxclient/src/components/hud/__tests__/DialogAttachmentCard.test.tsxclient/src/components/hud/__tests__/UnboundedBadge.test.tsxclient/src/components/modal/__tests__/LoopShortcutModal.test.tsxclient/src/components/ui/LoyaltyBadge.tsxclient/src/components/ui/__tests__/LoyaltyBadge.test.tsxclient/src/hooks/useCounterDisplay.tsclient/src/hooks/useUnboundedCounterTypes.tsclient/src/i18n/locales/{de,en,es,fr,it,pl,pt}/game.jsonclient/src/test/fixtures/unbounded-counter-wire.jsonclient/src/test/fixtures/unbounded-token-wire.jsonclient/src/utils/combat.tsclient/src/viewmodel/battlefieldProps.tsclient/src/viewmodel/cardProps.tsclient/src/viewmodel/gameStateView.tsclient/src/viewmodel/__tests__/battlefieldGrouping.test.tsclient/src/viewmodel/__tests__/cardProps.test.tsclient/src/viewmodel/__tests__/unboundedWireSeam.test.tscrates/engine/src/analysis/resource.rscrates/engine/src/game/derived_views.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/interaction.rscrates/engine/src/types/game_state.rscrates/engine/tests/fixtures/kilo_freed_relic_pentad_max_of_one_4p.json.gzcrates/engine/tests/integration/combo_infinite_pile.rscrates/engine/tests/integration/kilo_live_offer_from_real_dump.rscrates/engine/tests/integration/loop_counter_growth.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rs(47 paths at
117b430c2..9340075ad; the seven locale catalogs are collapsed into one line above.client/src/hooks/useUnboundedCounterRows.tsdoes not appear because it was both created and deleted inside this range — it exists at neither endpoint.)Track
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 104.4b, CR 110.1, CR 113.6b, CR 118.3, CR 119.3, CR 122.1, CR 122.2, CR 122.3, CR 306.5c, CR 400.7, CR 406.3, CR 500.5, CR 601.2a, CR 602.2, CR 605.3a, CR 606.4, CR 701.34a, CR 702.62b, CR 704, CR 704.5a, CR 704.5c, CR 732, CR 732.2a, CR 732.2b, CR 732.2c, CR 903.10a
Measured from added lines across the full PR range, not recalled — 26 distinct numbers. Every one was then confirmed to exist in
docs/MagicCompRules.txt, with controls in both directions: a bogusCR 999.99returns 0 hits from the same instrument (so the sweep is not stuck-true) andCR 122.1returns a hit (so it is not stuck-false).Verification
All results measured at the current head
9340075add0d4ae9f846243d3ae08631ee35bb02, in a distinct detached worktree with an isolatedCARGO_HOME/target, with start and end attestations recording detached state (git symbolic-ref -q HEADexit 1, empty stdout),HEAD, and a clean tree.cargo fmt --all -- --check— exit 0cargo clippy -p phase-engine --all-targets -- -D warnings— exit 0, 0 warningscargo test -p phase-engine— 23,281 passed; 0 failed; 15 ignored across all test targetspnpm run type-check— exit 0pnpm lint— exit 0, 0 errors (31 warnings, all pre-existing and none in the changed files)pnpm test -- --run(full suite) — 294 files passed (3 skipped), 2657 tests passed, 0 failed (12 todo)./scripts/check-parser-combinators.sh 117b430c2…—Gate G PASS,Gate A PASS(exit 0)game.json(the only namespace this PR touches), 1890 leaf keys each, identical key-set digest per localeThe i18n count is reconciled rather than asserted, from two directions that must agree. Directly: the base
117b430c2carries 1888 leaf keys and this branch adds 2 → 1890. Via the common ancestor:b5b8f4ecfcarries 1882, upstream added 6 and this branch 2, and 1882 + 6 + 2 = 1890. Both match the measured per-locale total, which is what rules out a key silently landing in six catalogs but not the seventh. The comparison instrument was checked against a planted extra key and reports a different digest, so the seven-way agreement is discriminating rather than a constant.Regressions proving the fixes
plus_one_counter_growth_registers_a_target_the_bearer_does_not_yet_carrydrives the real engine pipeline through an accepted counter-growth loop to the declared offer and asserts the adapter-visible state carries a row for a(object, counter)pair whose live bearer has no entry for it — the exact0 → 1case the first HIGH named. It fails on the pre-fix shape, because aVec<CounterType>has no row to carry and the assertion has nothing to read.two_seats_collapse_the_shared_pair_and_keep_the_distinct_onecovers the cross-seat case: the registration store is per-seat, so it deduped within a seat and never across seats, which would have published a duplicate row. It is matched by a both-directions negative control so it cannot pass by collapsing everything.counter_display_publishes_nothing_a_viewer_cannot_already_readpins the widened channel.counter_displayis keyed by every object in every zone rather than battlefield-only, so it is the one place this PR could leak hidden information. Its first arm asserts the redacted name really was redacted, so the test cannot pass by failing to reach the interesting state.DialogAttachmentCardgains rendering coverage for both a finite pill and anUnboundedone. Both were checked against the pre-fix file and fail there, so neither can pass on the raw-map path.The zero-count
Unboundedcase is now guarded at every site that renders pills. Measured with the mutant it exists to catch —.filter(r => r.count > 0)applied at all five render sites — the suites fail 5 of 5; before this work they failed 3 of 5, because two sites had no zero-row fixture at all. One of those gaps was introduced by this PR while fixing the zero-value wording (the replacement for an invalid zero-Finitefixture should have been a valid zero row, not none) and the other predates it; both are closed. Each new assertion is the sole failure under that mutant at its site, so none is riding another's coverage.For the picker, two tests added at this head. One seeds a state where
obj.countersandcounter_displaydisagree and asserts the chips follow the projection — it cannot be satisfied by any raw-map read. The other is the reported defect itself: two same-named attackers with byte-identicalcounterswhere only one carries an∞row must render visibly distinct. Three revert probes were run rather than two: reverting the projection read fails both; reverting only the∞ternary fails the second alone; and revertinggroupAttackers' fourth argument also fails the second alone — so it discriminates both halves of the fix, not just the rendering.Merge note (this head merges
upstream/main, it does not rebase)The PR's base moved to
117b430c2while this work was in review. I merged rather than rebased, deliberately: the head-bound evidence battery is re-owed at the new tip under either option so rebasing buys nothing measurable, this branch was already force-pushed once and a second rewrite of a published tip is not something to do casually, the repo squash-merges so the merge commit never reachesmain, and stable SHAs keep in-progress review diffs stable. Merge parents:883565893(branch) +117b430c2(upstream).upstream/mainhas since advanced a further 7 commits tod46667fc8. This head is not merged up to it, deliberately: a conflict check (git merge-tree, no worktree mutation) reports a clean merge and GitHub reportsMERGEABLE/CLEAN, so merging would buy no conflict resolution while invalidating the entire head-bound evidence battery above.The earlier merge produced exactly one real conflict, in the CR 603.5 prompt-census drift log, and it is worth stating because both incoming numbers were stale — each correct only for its own side. It was resolved by content, not by accepting the 3-way result: the pinned producer line was located by sha256 (
8a544e878d…5cc7d63, hashed with its trailing newline, verified unique under a whole-file scan), with arithmetic used only as a cross-check. That the pin is load-bearing was measured, not asserted: mutating it fails the test, and the mutation was reverted and the revert verified.Saved-game and wire surface disclosure
unbounded_counters: HashMap<ObjectId, Vec<CounterType>>is replaced bycounter_display: HashMap<ObjectId, ObjectCounterDisplay>, whereObjectCounterDisplay { pills, loyalty }is pre-partitioned and eachCounterRowView { counter, count, magnitude }carries a typedCounterMagnitude.magnitudeis#[serde(skip_serializing_if)]on theFinitedefault, so the common row stays two fields on the wire.phase-serverviaderive_filtered_views,engine-wasmviawrap_filtered,manabrew-compatviafilter_state_for_viewer), and the test named above pins it.GameState::unbounded_counter_targetsis serialized; the widened registration changes saved-game content for accepted counter-growth loops.#[serde(default)]keeps old saves loadable; the∞pill set an old save restores is the pre-widening (Generic-only) one.suggestedvalue changes1 → 1000for the unbounded object-growth offer class.GameState'sPartialEqcomparesproduct_knowledge_statewhilenormalize_for_loopdoes not clear it, so loop-state equality can fail to certify for a real class. Every link in that chain pre-exists at117b430c2— this merge inherits it, it is not introduced here. It gates the feature's upstream trigger for a real class; the paths tested in this PR are unaffected.engine.rs:5839-5847passesmatches!(boundary, CostMoveDrainBoundary::ReplacementDelivered { .. })where the typed enum is already in scope — measured at 0 changed lines on those symbols across this PR, despite the file being touched elsewhere. AndClientGameStateRef::wrapclones the fullGameStateviafilter_state_for_viewerto collect one bool per object on the broadcast path — present verbatim at the merge base (117b430c2:711-713). Both are real; both are outside this PR's frozen scope, and the second needs a rules answer (whether the predicate is evaluable independently of the redaction passes that set the flag) rather than a mechanical refactor.claude-opus-4.8on the earliest,claude-opus-5after). This PR squash-merges, so the landing trailer is the squash message. Recorded rather than amended — amending would rewrite the head this evidence is bound to.Gate A
Gate A PASS head=9340075add0d4ae9f846243d3ae08631ee35bb02 base=117b430c21fa66ba36040d60495aca7bbc08ef44
The base is stated explicitly because the gate script defaults to
git merge-base origin/main HEAD, and a stale forkorigin/mainsilently yields the wrong base — here it would have reportedc44a4512e, which is 7 commits behind. It was passed rather than defaulted.Non-vacuity: the compared range is non-empty (47 changed paths) and contains 0 files under
crates/engine/src/parser/, so the gate passes because this change touches no parser dispatch, not because the range was empty.Anchored on
crates/engine/src/game/derived_views.rs:53—CommanderDamageView { victim, commander, damage }, documented as "a single commander-damage badge the HUD renders". The engine already publishes the badge's number as a row so the client never sums damage itself;CounterRowView { counter, count, magnitude }is the same move at the same seam.crates/engine/src/game/derived_views.rs:584—prospective_storm_counts: HashMap<ObjectId, u32>, the existingObjectId-keyed, engine-computed count the client renders directly. It is the precedent for keying renderable per-object quantities off the derived view rather than offobjects[..].counters.Both anchors exist unchanged at the base commit (
:53and:478in the base blob).Final review-impl
Final review-impl PASS head=9340075add0d4ae9f846243d3ae08631ee35bb02
Seven review rounds ran against immutable base-to-candidate diffs, each in a fresh context. Every round but the last filed at least one finding, and each was closed in a further increment rather than argued away. The final round returned no findings with every re-verification claim confirmed by execution rather than by reading.
Process errors made during this run were recorded rather than buried, because several nearly banked false evidence. A hand-rolled
git status --porcelain -zparser reported 10 records where 19 existed, making a scope check vacuous rather than clean. A--exacttest pin probe ran0 testsand read as green. A cross-copy CR comparison reported that all 21 citations differed — a CRLF artifact (9367\rbytes in one copy); after normalization 19 were identical and 2 were real. A 100% failure rate needs a negative control exactly as a 0% rate needs a positive one. Most recently, a CR sweep in a fresh worktree returned "all missing" becausedocs/MagicCompRules.txtis gitignored and had not been fetched there — a vacuous zero that reads identically to "every citation is hallucinated"; it is why the CR verification above carries both controls.Claimed parse impact
None.
Measured, not assumed. The engine source hash differs between base and candidate (
2e5d3e8dd906698b→e0df6d4634c75bce), as expected since the range touches files undercrates/engine/src/— so a full parser projection was forced (projection_forced_reason=SOURCE_HASH_DIFFERENCE) rather than skipped. Both sides were projected directly from the same pinned read-onlyAtomicCards.json(sha25601b46792…):card-data.json72090407f1c4be3a…The base-built comparator run once against both projections reports
oracle_changed: 0,clusters: [],added_cards: [],removed_cards: []— "No card-parse changes detected."Two controls keep this from being a stuck instrument. Positive: the same pipeline produces a different
card-data.jsonagainst different engine source — upstream's own commits moved it2ae5a041… → 72090407…, so byte-identity here is a measurement, not a constant. Negative: all three binary pairs (oracle-gen,coverage-report,coverage-parse-diff) differ by both size and sha256 across sides, so the identical output is not a stale-binary artifact.oracle_changedreports1at this head (0previously), and it was chased rather than waved off, because the comparatorcontinues past such cards and a carve-out can therefore mask a real diff. The key is"fast": two distinct faces share it once card names are lowercased into a last-wins map, and 30 such duplicate keys exist, so each side retained a different face underCardDatabase::face_iterHashMap ordering. It masks nothing, proven independently of the comparator — both sides' card-entry multisets are identical (35657 each) and every aggregate is equal.coverage-data.jsondiffers for the same documented reason, with every asymmetric row sitting exactly at the truncation tie boundary of a top-50 list, which is why the identity claim rests on the byte-reproduciblecard-data.jsoninstrument instead.Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
New Features
Bug Fixes