ship/fix engine bind mass move damage total - #7277
Merged
Merged
Conversation
matthewevans
commented
Aug 12, 2026
matthewevans
commented
Member
- fix(engine): bind mass move damage to total
- test(parser): update Valakut lowered snapshot
matthewevans
enabled auto-merge
August 12, 2026 03:38
Contributor
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
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 |
matthewevans
force-pushed
the
ship/fix-engine-bind-mass-move-damage-total
branch
from
August 12, 2026 04:24
48306cf to
0f8bc8b
Compare
|
Generated for head Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 17, 2026
Both incumbent tests passed identically before and after the aggregate fix, which is why phase-rs#7277 shipped. Neither was measuring what its name claimed. `player_scope_discard_then_windfall_draws_greatest_discard_count` seeded 3-card libraries against hands of 3 and 1, so MAX 3 and SUM 4 both capped at 3 and the assertions held under either reading. Rebuild it over a shared board with 6-card libraries, where MAX 3, SUM 4, MIN 1 and the per-player reading 3/1 are four mutually distinguishable outcomes, and drive both aggregates through one builder so the Sum member is a same-board control for the Max member. `windfall_draw_uses_previous_discard_max_for_each_player` asserted the count via a `PreviousEffectAmount { .. }` wildcard, which matches every aggregate. Tighten it to the full literal so the parse is pinned to Max. Verified by revert probe rather than by inspection: reverting the resolver's Max arm turns the rebuilt test red with left [4, 4] / right [3, 3], while its Sum member on the same board stays green; reverting the combinator turns the wildcard test red on an assertion, not a compile error. Assisted-by: ClaudeCode:claude-opus-5
matthewevans
added a commit
to cuinhellcat/phase
that referenced
this pull request
Aug 18, 2026
…-player sum (phase-rs#7494) * fix(engine): draw the greatest single player's discard, not the cross-player sum Windfall's "the greatest number of cards a player discarded this way" resolved to the SUM across players: with hands 8/7/3/3 every player drew 21 instead of 8. `QuantityRef::PreviousEffectAmount` had no way to say *which* reduction to apply to the per-player table it reads, so every consumer got the total. Add an `aggregate: AggregateFunction` axis to the variant, mirroring the `DamageDealtThisTurn` precedent, and parameterize the resolver fold over it. `Sum` is the serde default and is elided, so the 147-card corpus projection is byte-identical except the three cards in the class (Windfall, Jace's Archivist, Whispering Madness). Parsing gains `parse_greatest_discarded_this_way`, a nom combinator covering the determiner-less and superlative-variant forms; the legacy `all_consuming` block that hard-coded the summed reading is deleted and its dispatcher delegates to the combinator, so the two readings can no longer disagree. Also corrects nine CR miscitations found while tracing the class (C1-C9): each cited a real rule for something it does not say. CR 120.6 is marked-damage persistence and never supported "the total amount dealt/lost/removed"; CR 107.1 does not license a maximizing extremum adjective. Every replacement number was greped and content-matched against docs/MagicCompRules.txt. CR 608.2h: the answer is determined only once, when the effect is applied. CR 608.2c + CR 608.2i: the "this way" back-reference and its look-back exception. CR 121.2c: the engine's APNAP serialization of the multiplayer draw is correct; only the leaked count was not. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): make the Windfall aggregate tests discriminating Both incumbent tests passed identically before and after the aggregate fix, which is why phase-rs#7277 shipped. Neither was measuring what its name claimed. `player_scope_discard_then_windfall_draws_greatest_discard_count` seeded 3-card libraries against hands of 3 and 1, so MAX 3 and SUM 4 both capped at 3 and the assertions held under either reading. Rebuild it over a shared board with 6-card libraries, where MAX 3, SUM 4, MIN 1 and the per-player reading 3/1 are four mutually distinguishable outcomes, and drive both aggregates through one builder so the Sum member is a same-board control for the Max member. `windfall_draw_uses_previous_discard_max_for_each_player` asserted the count via a `PreviousEffectAmount { .. }` wildcard, which matches every aggregate. Tighten it to the full literal so the parse is pinned to Max. Verified by revert probe rather than by inspection: reverting the resolver's Max arm turns the rebuilt test red with left [4, 4] / right [3, 3], while its Sum member on the same board stays green; reverting the combinator turns the wildcard test red on an assertion, not a compile error. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): freeze a draw clause's count once, per CR 608.2h `QuantityRef::PreviousEffectAmount` was re-read by every completed draw in a fan-out tail, so each draw re-stamped the shared scalar and every player after the first drew the wrong number: Windfall on hands 8/7/3/3 produced [5,5,5,5] where the rules require [5,8,8,8]. CR 608.2h fixes such a value "only once", when the spell or ability resolves -- not once per player the instruction fans out to. CR 608.2i's look-back exception is scoped to objects (zone, criteria), and so does not exempt the number. Admit `PreviousEffectAmount` to `collect_clause_minimum_refs` / `capture_clause_minimum_snapshot`, the existing CR 608.2h freeze mechanism, and read the snapshot before the live scalar in `game/quantity.rs`. The prompt census in `game/engine.rs` pins producer coordinates in `effects/mod.rs` by line; this commit's four hunks land above all three, so the pins move +24 uniformly. Re-pinned per that file's own drift-log protocol, with identity re-established rather than assumed: the 41-line window at each producer is sha256-identical to its old coordinate. Test-only, no production surface. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): count zero-contributors in the previous-effect table Review of the Windfall aggregate work surfaced seven findings; this commit applies all seven. The load-bearing one is a producer defect. The per-player table a completed instruction publishes is built from emitted events, so a player the clause applied to who contributed nothing -- an empty hand facing "each player discards their hand" -- emitted no event and was simply absent. An aggregate then reduced over a domain that omitted them. CR 608.2c: that player still discarded zero this way. The fill is now per player rather than all-or-nothing, extracted as `fill_zero_contributors`. Two of the three aggregates are blind to the omission, which is why it survived: Sum reads `last_effect_amount`, and Max cannot be raised by zeros. Only Min sees it -- hands 8/7/3/0 published {8,7,3} and answered 3 where the answer is 0. The defect is the reduction domain, not the Min arm. Also: a control that claimed to guard the cross-aggregate axis could not detect it (Syphon Mind builds no PreviousEffectAmount node at all) and is relabelled as the non-interference guard it actually is, with the aggregate axis discriminated at unit level where a populated table can be constructed; CR 120.6 struck from the condition peer it was still miscited on, matching the correction already applied to its QuantityRef twin; the categorical-boundary justification re-grounded on CR 608.2c/608.2i, since the Total channel is stamped by non-damage producers and has no CR 120 anchor; the clause_minimum_snapshot read added to the ability_scan enumeration whose stated purpose is to force re-classification; a unit sibling pair for the newly admitted freeze class; and the admission arm's unenforced precondition documented with the 44-card classification behind it. Every new test is revert-probed: each was made to fail on a value before being kept, and the probes isolate rather than overlap -- reverting the zero-fill reddens exactly one of its three tests. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): cover the zero-fill's production wire, and correct its census Delta re-review of the zero-contributor fix returned nine findings. The core fix was confirmed correct and at the right seam; this commit applies all nine. The load-bearing one is a test-coverage defect in my own work. All three `fill_zero_contributors_*` tests call the helper directly, so deleting the driver's call to it left the entire integration binary green -- including the zero-contributor integration test, which does reach the fill. The helper was tested; the wire was not. `player_scope_fan_out_publishes_a_zero_for_the_empty_handed_seat` now drives a real `player_scope: All` discard fan-out over four seats, hands 1/1/1/0, through `resolve_ability_chain` and asserts on the table the driver actually published. Removing the wire fails it on exactly the omission. The admission arm's corpus classification was wrong in a way that mattered. It claimed 4 cards hold the ref "in the scoped node itself" and the other 40 take the drain shape -- but those sets are not a partition. Measured: 44 cards carry both a `player_scope` and a `PreviousEffectAmount`; 3 hold it only in a condition; of the 41 quantity-position carriers, ALL hold it inside the scoped subtree, so the axis that discriminates is which effect carries it -- 38 `GainLife`, 3 `Draw`, 1 `LoseLife`, with Thorna and Twigtooth holding two and belonging to both of the old buckets at once. That correction also supplied the precondition audit the comment had asserted without performing. Thorna is the only retained-side carrier, hence the only card that could falsify "no card wants a per-iteration reading here." It does not: "each opponent loses X life ... where X is the number of counters removed this way" fixes X once for the whole clause, which is exactly the pre-clause value the freeze supplies. One behavioural fix rides along. On the interactive-pause path the fill's reduction domain was the full `matching_players`, so a pause after the first seat published a zero for three seats that had not yet had the chance to contribute. The domain is now narrowed to the seats that completed. Also: the Sum-vs-Max claim corrected (no card in the Sum class yields an integration-level discriminator -- the Max class does, and it is the first test in the file); a test comment that contradicted its own doc about whether the per-player table is populated; the clearing mechanism restated (the card's own draw tail takes the non-producer arm, not the player-action boundary); `install_previous_effect_counts_by_player`'s doc comment restored after the new helper captured it; CR 120.6 struck at its two remaining sibling sites, since marked-damage-until-cleanup does not govern a resolution-local carry-forward; the non-scoped install site's absent zero-fill documented; and five leftover debug `eprintln!`s removed. Census pins re-derived by content and confirmed by window hash against an off-by-one control: `:6816/:6893/:10148` to `:6828/:6905/:10171`. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): exclude the paused seat from the zero-fill's domain Final review at the rebased tip returned one MED and three LOW. This commit applies everything that belongs to this change; the MED is a pre-existing defect in the continuation machinery and is disclosed rather than repaired here (see below). The off-by-one was mine. `applied_domain_end = i + 1` included the seat that had just paused on a choice it has not answered, publishing it as a zero contributor when it has not yet had the chance to contribute at all. A `Min` read taken mid-pause answered 0 off that entry. The bound is `i`: only seats that COMPLETED before the pause belong to the reduction domain. Where a seat paused after its own producing clause it already holds a real entry, so the fill is a no-op for it either way. The comment above that line was worse than the code. It claimed the continuation would "extend" the domain. It does not: each resumed leg REPLACES the table -- `install_previous_effect_counts_by_player`'s `Some` arm assigns `last_effect_counts_by_player` outright, and `split_player_scope_chain` clears `player_scope` on the resumed legs, so every leg publishes only its own entry. Measured on four seats: `{P0:1, P1:0}` at the pause, `{P2:1}` after the next leg. That claim was the thing hiding the defect, so it is replaced with the measured behaviour and the shape of the repair. That defect is NOT introduced or widened here, measured rather than asserted: `last_effect_amount` is derived from the same table (`.values().sum()`), so the Sum class loses exactly the same counts and did so before this branch existed. It is reachable for this PR's three cards -- the forced whole-hand discard branch can still pause on a replacement choice, at a site that already documents its own related `EffectResolved` gap. Repairing it means making the per-clause table accumulate across continuation legs, which is resume-machinery work affecting every count-producing fan-out including the 38-card drain shape, and does not belong in a draw-count change. Also: `fmt_quantity_ref`'s `(_, Sum)` arm carries a "Must stay FIRST" comment that nothing enforced -- reordering it would silently move the coverage signature of every Excess-channel corpus card, reddening CI's coverage check with no indication of the cause. Five assertions now pin all four channel/aggregate renderings, including the order-dependent Excess+Sum pair. Both new tests were probed red before being kept: restoring `i + 1` gives `left: [(0,1),(1,0)] / right: [(0,1)]`, and moving the Excess arm above `(_, Sum)` gives `left: "excess amount from preceding effect" / right: "amount from preceding effect"`. Census pins re-measured after the last edit of the round, by content and confirmed by window hash against an off-by-one control: `:10324` to `:10347`. Only the third producer moved this time -- the first two are unchanged, because these edits land between them. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): correct a comment that cited the pre-fix numbers as measured Delta re-review of the previous commit found that its new comment quotes the per-player table as `{P0:1, P1:0}` at the pause -- which is the PRE-FIX value, byte-identical to the `left:` side of the probe that proves the `i`-not-`i+1` fix works, 85 lines below in that same commit. The comment therefore asserted, as measured fact about the tree it ships in, the exact behaviour that commit removes. A maintainer diagnosing the fill site would have read it, concluded the domain narrowing never took effect, and reverted or re-patched it. Re-measured on the tree the comment actually ships in: `[(0, 1)]` at the pause and `[(3, 1)]` once the continuation runs, with `last_effect_amount` reading `Some(1)` where an accumulating table would give 4. The corrected figures make the point stronger rather than weaker -- the remaining seats chain into ONE continuation leg, so seat 2's publication is replaced as well and the table is overwritten more than once, not merely truncated. The same two wrong figures were corrected in the PR body and the posterity issue. Two smaller corrections from the same review: The coverage guard was named for the four match arms while the pair space is two channels x three aggregates = six, and it asserted five of them, leaving `(Excess, Min)` unpinned. Renamed and completed to one assertion per pair, so the name's claim of completeness is literally true rather than true-of-the-arms. Recorded alongside it: rustc emits no `unreachable pattern` warning for the reorder this guard defends against, which is why the guard is needed at all. The `i`-not-`i+1` rationale said a seat that completed its producing clause already holds a real entry. `fill_zero_contributors` is `or_insert(0)`, so it is PRESENCE in the table, not completion, that makes the fill a no-op; a clause completing with a genuine zero emits no event and so holds no entry. Reworded to the property the code actually has. Census pins re-measured after the last edit: `:10347` to `:10352`, third producer only, window hash unchanged with both neighbours differing as controls. Assisted-by: ClaudeCode:claude-opus-5 * feat(engine): carry a paused discard batch across a replacement choice A forced whole-hand discard that pauses for a CR 616.1 replacement choice had nowhere to record what it still owed, so it abandoned the rest of the hand. Add the carrier that lets it resume, modelled on the sacrifice family that already solves this for the sibling producer: `PendingDiscardBatch` holds the owed cursor, `DiscardBatchCursor` types the selection mode (whole-hand vs random pool) instead of flagging it, and `PendingDiscardFanOut` carries the remaining-seat roster in APNAP order (CR 101.4) so the seat list survives the pause. The roster doubles as the clause identity and the final-leg signal, which is why no marker field is added to `ResolvedAbility` and no accumulator field is added beside `last_effect_counts_by_player`. All six registration surfaces move in lockstep: declaration, `Default`, the exhaustive partition destructure with a written classification note, the hand-written `PartialEq` conjunct (a parked batch is interaction state and must compare), `LIVE_EVENT_CARRIER_FIELDS`, and serde. The CR733 authority matrix gains the corresponding row, derived from a real census run rather than hand-written. Hidden information: `filter_state_for_viewer` is an allowlist-of-clears, so a new carrier defaults to leaked. The batch holds hand-zone object ids (CR 400.2), so it is cleared for every viewer. Verified before clearing that every projection caller is display-only and none resumes from a filtered state, since clearing a field the drain reads would be a silent breakage rather than a redaction. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): finish a discard batch that paused, and count what it discarded Three defects on the forced whole-hand route, all on the path Windfall, Jace's Archivist and Whispering Madness actually take: 1. The loop's `return Ok(())` exited mid-hand with no cursor, so the seat's remaining cards were never discarded — they stayed in hand for the rest of the game. Silent data loss. 2. No terminal `EffectResolved` was emitted, so that seat's count could not be derived from events at all. 3. The paused card was never counted even after the choice was answered: the gate-2 resume path emits `Discarded` only when a discard frame is present, and it is absent for all three of these cards. Both bail-outs now park a batch instead of dropping it, and `drain_pending_discard_batch` resumes it from the replacement-choice epilogue — ordered after the sacrifice drain and before the generic continuation drain, so a parked `after_scope` cannot run before the instruction feeding it has settled. `stamp_resumed_discard_if_unrecorded` closes facet 3. `publish_player_scope_clause_results` is extracted from the driver so the driver and the resumed batch share ONE publication rather than two racing ones. That is what makes the CR 608.2i look-back read a complete per-player table across the pause, and it is why no accumulator state is needed: a clause that publishes once needs nothing to merge. The production write sites for `last_effect_counts_by_player` therefore remain exactly four. The driver hands its remaining-seat roster to the batch behind an identity triple — source, seat, and not-already-handed-off — checked before the hand-off rather than inferred from payload shape, so a foreign batch falls through to the unchanged leg path. Also resets `cost_payment_failed_flag` per seat in the drain's fan-out loop (CR 101.3 + CR 608.2c): impossibility is a property of the part, so an earlier seat's mandatory failure must not leak into a later seat. This is the driver's own documented resumption boundary, previously missing on the resumed path. The CR 603.5 prompt census is re-pinned, not relaxed: twelve hunks at or above the coordinate sum to exactly +366, the producer window is sha256-identical, and the partition assert stayed green. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): drive the paused discard through the real cast pipeline Two arms on the existing aggregate file, both driving the production pipeline rather than a helper: cast Windfall into a four-seat board with hands 7/3/5/2, arranged so the seat that PAUSES holds the maximum. That placement is the whole point — a fixture where the paused seat is not the max cannot tell a complete table from a truncated one. The reference values are mutually distinct by construction, so no partial-table failure mode can coincide with the right answer: 7 is reachable only if the paused seat is in the table, 5 is the max without it, 2 is the last publication alone, and 6 is the value if the uncounted-redirected-card facet were left unrepaired. Arm B lands the paused card in exile, exercising the false arm of the graveyard guard. Both were run at the pre-fix tip first and were RED with the signature predicted in advance — 1 prompt, `drawn == [2,2,2,2]` — before any production line was written; they now read 7 prompts and `[7,7,7,7]`. One correction came out of running rather than deriving them: the predicted graveyard row had omitted the spell's own card, which lands in its controller's graveyard as the final part of resolution (CR 608.2n). The random-branch file covers the other cursor shape, with a pool of four and two sequential pauses, because a single-pick fixture could never exercise a cursor. Its redirect is narrowed to exclude the spell itself: with an unfiltered redirect the spell's own graveyard move overwrites the parked choice, which is a separate pre-existing defect recorded in the test's doc comment so a future reader sees why the narrowing exists. Assisted-by: ClaudeCode:claude-opus-5 * docs(ai): strike a CR tag from a detection heuristic that implements no rule `is_previous_amount` carried `CR 120.10`. That rule governs excess damage dealt to a permanent and how triggered abilities checking for it are evaluated; it says nothing about amounts left by a preceding effect, the total channel, or aggregate-agnostic detection — which is what the comment actually asserts. Same class as the `CR 120.6` miscitation this branch already struck one crate over. The rationale is correct and is kept verbatim as an engine invariant. It is the annotation that does not belong: an AI scoring heuristic implements no game rule, so per the workspace convention it carries no CR tag at all. Pinned by an `include_str!` guard asserting the rationale survives and the annotation form does not, so a future edit cannot quietly restore the tag or drop the reasoning. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): say what CR 608.2i's exception actually exempts CR 608.2i ends "This is an exception to 608.2h", and review read that as exempting a look-back from the snapshot rule outright -- which would make freezing `PreviousEffectAmount` contradictory rather than correct. Read in full, the exception is scoped to two things, both about objects: they "don't need to be currently in the zone" they were in, "nor do they need to currently meet the criteria described in the action". It relaxes where the objects must be standing, not when the number is determined, so CR 608.2h's "determined only once, when the effect is applied" still governs the value. The clause-snapshot doc already named all three rules but never said this, so the objection had nothing in-tree to answer it. Doc-only; no code change. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): let the authority publish the replacement chooser, and refuse to discard a card that is not in hand Three defects surfaced by independent implementation review of the paused discard batch. All are in this PR's own new code. The `Random` cursor's re-park re-derived the CR 616.1 chooser as `batch.player` instead of threading the seat `discard_at_random` had just computed, while the `All` arm 30 lines above threads its own correctly. Benign today, because a hand card's affected player is its controller -- but `replacement_choice_player`'s commander carve-out proves the engine already has chooser != affected-seat cases, and the moment one reaches a random discard the wrong seat is prompted. `RandomDiscardOutcome::NeedsReplacementChoice` now carries `chooser`, so both cursor arms read one contract and no call site re-derives it. The two cost-layer destructures take it as `_`: that layer never re-parks, so it has no prompt to keep in step. `route_discard` -- the single chokepoint every discard routes through, effect and cost, whole-hand and random -- now returns early when the card is not in a hand. CR 701.9a defines discarding as a move from hand to graveyard, so there is no event to propose. This became load-bearing with the parked batch: a cursor is a hand snapshot latched before an action boundary and drained after one, and `complete_discard_to_graveyard` lowers to a hard-coded `from: Hand`, so a card that moved in between would have been "discarded" out of whatever zone it now occupies. Un-paused callers build and consume their snapshot inside one action and cannot observe a difference. CR 800.4a: a seat that has left the game is dropped from the discard fan-out's not-yet-prompted roster, the same treatment `pending_scoped_library_search` already gets. `matching_players` is deliberately left whole -- CR 608.2f latches the reduction domain when the action begins being processed per subject, so a departed seat still contributes its truthful zero and pruning it would silently change a `Min` answer. The CR 603.5 prompt census pin moves `:10798 => :10803`, third producer only, re-derived by content after the last edit; the CR733 row's three reroute coordinates are re-derived from a fresh `cr733_mutation_census.py` run. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): make three pins discriminate the thing their messages claim Independent implementation review found one test arm that could not fail for the reason it named, one guard that a single character would slip past, and one deliberate asymmetry with nothing pinning it. `absent_pending_discard_batch_deserializes_as_none` built its fixture at `GameState::new_two_player`, whose `waiting_for` is already `Priority`, then asserted `Priority` after the round trip under the message "the restored state machine is intact, not orphaned mid-pause". It restated an input property. The save is now taken genuinely mid-pause -- a CR 616.1 `ReplacementChoice` is installed before serializing, with a reach guard asserting the input does not already satisfy the property -- and the assertion is that a batch-less mid-pause save round-trips its prompt VERBATIM, so the inconsistency stays observable to a caller instead of being silently rewritten into a plausible-looking state. The second revert probe records why the obvious "repair" would be wrong. The CR 120.10 strike guard asserted on `"CR 120.10:"`, so a re-added `// CR 120.10 both channels ...` without the colon would have passed. It now matches the annotation form -- a comment line whose first token is the citation -- which is also why a bare substring test cannot be used: the same window deliberately contains the prose recording that the tag was struck. The two adjacent post-replacement drains publish completion differently on purpose: sacrifice stamps `ThisWayCause::Sacrificed`, the discard drain 25 lines below stamps nothing, because the un-paused discard path does not stamp either and a stamping resume would give a paused discard provenance its own un-paused twin never has. Nothing pinned that. It is pinned as a source census rather than a behavioural assertion, and that is measured rather than lazy: `stamp_active_player_action_completion` early-returns without a `CompletePlayerAction` continuation frame, which a drain unit test does not have, so a behavioural assertion there would itself be vacuous. The census carries its own positive control -- half (a) proves the scan reaches a region that does stamp, so half (b)'s zero cannot be a scan that missed. Also records the scope of the CR 616.1 citations on `PendingDiscardBatch`: 616.1 governs the two-or-more-applicable case, while every pause this type carries in practice is the engine's apply-or-decline prompt for a single optional replacement, which 616.1 does not describe. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): widen a census window that guarded a line and a half, and retire two CR stretches Round 2 of independent review, on the previous round's own fixes. Four of six held; these are the two that did not, plus three annotation corrections. The drain-parity census sliced each match arm between two guessed markers, and the end marker for the second window sat INSIDE the arm: the "guarded" region was 36 characters of a five-line arm, so its named revert probe only flipped if a stamp landed as the arm's very first statement. Both windows are now closed by brace balance, each anchor must match exactly once (so a deleted arm cannot let the scan slide onto other text -- the doc comment no longer spells an anchor literally), and each window asserts its OWN non-degeneracy. That last one is the real lesson: the positive control proves the SACRIFICE region is real and says nothing about the extent of the DISCARD window, and the discard window is the one whose zero carries the claim. A positive control on region A does not license a negative on region B. Measured after: 281 chars / 6 lines, up from 36 chars / 1 line, and it now contains the arm's last statement. The mid-pause save/load test deserialized with bare `from_value::<GameState>`, bypassing `PersistedGameState::into_game_state()` -- which is where this repo puts load-time repairs, and therefore the only door the "helpful" repair its second revert probe warns about would ever come through. It now loads through that chokepoint, and asserts `candidate_count` as well as the variant and player, since a prompt rebuilt with different contents would otherwise pass. Two CR stretches retired, both the class caught earlier with CR 608.2b. CR 608.2f does not latch a reduction domain -- read in full it is simultaneity and APNAP ORDER, and both its examples are about ordering; the honest justification for leaving `matching_players` whole is PARITY with the un-paused driver, which also computes its domain once and never re-derives it, with CR 800.4i ("the effect uses the last known information about that player before they left the game") making the retained seat well-defined. And CR 800.4a is cited now only for what it says -- objects owned by a departing player leave the game, so a departed seat has no hand to discard. The CR 701.9a guard now retires the discard frame, exactly as the `Prevented` arm it is modelled on does; without that a `DiscardedCardMatchesFilter` frame leaks when every listed card has already moved. Its `Complete` return is imprecise on cost paths, but that imprecision is inherited from `Prevented` rather than introduced here, and the shape is recorded in place for whoever next touches `DiscardOutcome`. The comment's "single chokepoint every discard routes through" was false and is corrected: three callers reach `complete_discard_to_graveyard` directly, as resumes of an already-guarded proposal. Also: the prompt-census window digests now state the exact command that reproduces them. Review could not reproduce them from the obvious guesses, and a digest a reader cannot recompute is decoration rather than evidence. Verified: the documented rule reproduces all three and their off-by-one controls. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): pin two guards that nothing would have caught, and route a census through the shared comment rule The CR 701.9a "already left the hand" guard in `route_discard` and the CR 800.4a roster prune in `elimination.rs` both changed runtime behaviour with no test that went red if they were deleted. Review had verified they were correct, which is a different question from whether they were pinned. T-A asserts non-vacuity FIRST -- the in-hand card must actually be discarded, or an inert `route_discard` would satisfy the negative half by doing nothing at all -- then covers the frame half review found missing. That frame test nests two frames, which buys ARITY AND DIRECTION: it separates "retired one frame" from "emptied the stack", and catches a retirement that pops zero, two, or from the wrong end. It deliberately does NOT claim the guard retired the frame it was HANDED. That property is absent from the code rather than unmeasured -- `take_active_discard` pops the top when that top is a `Discard` frame, and `frame_id` is consulted only by a `debug_assert_eq!` -- so a test demanding it would red on HEAD. The doc records that instead of asserting it, and discloses the leak the qualifier implies: `retire_discard_frame` swallows `Err(UnexpectedTop)`, so a non-`Discard` top makes retirement a silent no-op. Unmeasured for reachability, and repairing it is a change to the resolution stack's error contract, not to this guard. T-B asserts an ASYMMETRY: a seat that leaves mid-pause is dropped from the iteration roster (CR 800.4a: its objects leave the game, so iterating it can only be a no-op) and KEPT in the reduction domain (CR 800.4i: last known information, contributing zero). The two lists look like duplicates, so the natural tidy-up prunes both -- which shrinks the domain and changes what a `Min` over it answers. `Min`, not `Max`: `Max` cannot be raised by zeros, as `fill_zero_contributors`' own doc records. One of the revert probes is exactly that tidy-up. The test pins the SHAPE of the two lists; the `Min` consequence is the reason the pin exists, not something it measures, and the doc comment now says so. `source_census::tests::no_source_reading_file_carries_a_private_comment_policy` was red on the parity census in `engine_replacement.rs`: it read Rust source with its own comment policy. That census's claim is a NEGATIVE, so a deleted stamp whose spelling survived in a trailing `//` would have HELD the zero and hidden the regression. Routed through `code_lines`; no probe is quoted because a better measurement exists -- the guard was red before and is green after. The routing is documented as a CLOSURE rather than a live defence: measured on this tree, raw and stripped text are identical for every quantity that census reads, so it discriminates nothing today. Also corrects a comment of my own that named the wrong arm: the two `Prevented` arms that retire the discard frame are in `complete_discard_to_graveyard` and in `resolve`'s specific-target loop, both above; `route_discard`'s own does not. Disclosed rather than repaired -- its reachability with a frame present was never measured. Six revert probes run and observed red, each with a real `test result: FAILED` rather than a bare non-zero exit, and each file restored with a sha256-verified copy. One is disclosed rather than counted clean: retiring the frame twice reds through `retire_discard_frame`'s own `debug_assert_eq!`, not through this test. `[profile.test] inherits = "dev"`, `[profile.release]` never sets `debug-assertions`, and no `--release` test invocation exists in the Tiltfile or any workflow, so the production assertion fires first in every venue this repo actually runs -- the test's own `expect` is unreachable there. Assisted-by: ClaudeCode:claude-opus-5 * fix(PR-7494): persist paused clause snapshot * test(PR-7494): target paused snapshot persistence * fix(PR-7494): persist Balance snapshot at discard choice * fix(PR-7494): bind a paused discard to its parked object incarnation `PendingDiscardBatch.paused_card` was a bare `ObjectId`, and the resume stamp accepted any hand departure carrying that id. CR 400.7 makes an object that changes zones a new object, so a same-id round trip let a later incarnation's departure settle a pause it never belonged to. The provenance was already on the wire. Every production `ZoneChangeRecord` is built by `GameObject::snapshot_for_zone_change` before the incarnation bump, so its `trigger_source_context.identity.reference` is exactly the pre-move occurrence. Retype `paused_card` to `ObjectIncarnationRef` — whose `#[serde(from = "ObjectIncarnationRefCompat")]` already carries the save migration for this exact CR 400.7 reason — pin it at each pause via `pin_paused_occurrence`, and match that occurrence rather than the id. A record with no context is legacy or hand-built and now fails closed, which is the policy `ZoneChangeRecord::trigger_source_context`'s own doc states: callers must not reconstruct a source from a current object. Ungating `entered_incarnation` was rejected rather than overlooked: `resolved_commands.rs` asserts a replay invariant that a non-battlefield destination must leave it `None`. The CR 603.5 prompt census pin moves `:11103` to `:11124`, a pure line shift. The producer's 9-line block hashes `bc850c67` at both coordinates and is re-found at exactly one place in the tree, so the entry moved rather than the set changing. Assisted-by: ClaudeCode:claude-opus-5 * fix(PR-7494): correct replacement rule annotations * fix(PR-7494): repair the census pin list and finish the CR 616.1 retirement The port across main resolved the CR 603.5 census conflict by keeping BOTH sides of the pin list — main's `:7267/:7344/:10624` and this branch's `:11124` — giving four `effects/mod.rs` entries. There are three producers in that file, so the vector could not match a census computed from source and it contradicted the `(5, 8, 28)` partition assert immediately above it. A union is the wrong merge for a list whose length is asserted: the entries are one coordinate per producer, not additive facts. Measured at `a9d9ec8c8`: left (from source) 5: mod.rs:7325, mod.rs:7402, mod.rs:11131, ... right (pinned) 6: mod.rs:7267, mod.rs:7344, mod.rs:10624, mod.rs:11124, ... Re-measured by digest, not arithmetic: each producer's 9-line block hashed at upstream/main (`f9098299`/`96338f0e`/`bc850c67`) is found at exactly one coordinate. The shift is non-uniform (+58/+58/+507), so adding a delta to all three would have written three wrong numbers. Also finishes the CR 616.1 retirement the review note asked for. The annotation fix reached effects/mod.rs and game_state.rs; the single-optional discard path spans six files, leaving nine assertions live — including windfall_greatest_discard_aggregate.rs, which said "P0 controls an OPTIONAL discard replacement" while citing the two-or-more rule. Replaced with CR 608.2c for instruction order and CR 614.6 where the point is that a replaced event never happens, both grep-verified. Pre-existing CR 616.1 citations elsewhere are untouched; only the nine this PR introduced are changed. Assisted-by: ClaudeCode:claude-opus-5 * test(PR-7494): annotate Balance snapshot rule Co-authored-by: lgray <lindsey.gray@gmail.com> * test(PR-7494): use typed Leng fixture assertion * fix(engine): resume replacement-paused discard lists * fix(PR-7494): reject duplicate discard selections Validate DiscardChoice card selections at the action boundary before recording a paused ordered cursor, and cover the real cast-to-choice pipeline. Co-authored-by: Lindsey Gray <lindsey.gray@gmail.com> --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.