diff --git a/crates/engine/src/database/synthesis.rs b/crates/engine/src/database/synthesis.rs index dee7adee44..1d79c3fe0a 100644 --- a/crates/engine/src/database/synthesis.rs +++ b/crates/engine/src/database/synthesis.rs @@ -6009,6 +6009,7 @@ fn build_extort_trigger() -> TriggerDefinition { amount: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, player: TargetFilter::Controller, @@ -15058,6 +15059,7 @@ mod extort_synthesis_tests { amount: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, player: TargetFilter::Controller, diff --git a/crates/engine/src/game/ability_scan.rs b/crates/engine/src/game/ability_scan.rs index 7003baa522..a542a50d52 100644 --- a/crates/engine/src/game/ability_scan.rs +++ b/crates/engine/src/game/ability_scan.rs @@ -2178,7 +2178,21 @@ fn scan_quantity_ref(x: &QuantityRef, mode: ScanMode) -> Axes { }, }, QuantityRef::ExiledFromHandThisResolution => Axes::NONE, - QuantityRef::PreviousEffectAmount { .. } => Axes::NONE, + // CR 608.2c + CR 608.2i: every channel and every aggregate reads + // resolution-local state — `last_effect_amount` / + // `last_effect_excess_amount` / `last_effect_counts_by_player` / + // `clause_minimum_snapshot`, the last read FIRST (`game/quantity.rs`, + // the `PreviousEffectAmount` arm) as the CR 608.2h frozen value. All are + // cleared at depth-0 chain entry (`resolve_ability_chain`); `apply()` + // additionally clears `last_effect_count` and the per-player table at + // every player action. None is a triggering-event characteristic + // (event), a board-scoped mutable aggregate a sibling copy could mutate + // (sibling), or a player-level per-turn projected resource (projected). + // Destructured without `..` so a future field forces re-classification. + QuantityRef::PreviousEffectAmount { + channel: _, + aggregate: _, + } => Axes::NONE, QuantityRef::PreviousEffectCount => Axes::NONE, QuantityRef::LifeLostThisTurn { player } => { let mut acc = Axes { diff --git a/crates/engine/src/game/casting_tests.rs b/crates/engine/src/game/casting_tests.rs index 93962714c8..a146983461 100644 --- a/crates/engine/src/game/casting_tests.rs +++ b/crates/engine/src/game/casting_tests.rs @@ -7064,6 +7064,7 @@ fn x_spell_doubled_lose_life_drains_opponents_and_gains_controller() { amount: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, player: TargetFilter::Controller, diff --git a/crates/engine/src/game/coverage.rs b/crates/engine/src/game/coverage.rs index 9de5dc2442..d76cacfd25 100644 --- a/crates/engine/src/game/coverage.rs +++ b/crates/engine/src/game/coverage.rs @@ -1588,7 +1588,28 @@ fn fmt_quantity_ref(qty: &QuantityRef) -> String { format!("# of counter kinds among {}", fmt_target(filter)) } QuantityRef::VoteCount { choice_index } => format!("# of votes for choice {choice_index}"), - QuantityRef::PreviousEffectAmount { .. } => "amount from preceding effect".into(), + QuantityRef::PreviousEffectAmount { channel, aggregate } => match (channel, aggregate) { + // Byte-identical to the pre-change string, so no existing card's + // coverage signature moves. Must stay FIRST: the Excess-channel + // corpus cards are all `Sum` and must keep hitting this arm. + (_, AggregateFunction::Sum) => "amount from preceding effect".into(), + // CR 120.10: excess damage is "equal to the difference" beyond lethal — + // one amount per damaged permanent, never a per-player tally. Naming a + // "single player's" extremum over it would describe a reduction that + // never happened. (The per-player table the Total channel publishes is + // an engine structure; no CR governs its shape, so none is cited for it.) + // No parser path builds that pair today; the arm exists so the renderer + // stays honest if one ever does. + (crate::types::ability::DamageChannel::Total, AggregateFunction::Max) => { + "greatest single player's amount from preceding effect".into() + } + (crate::types::ability::DamageChannel::Total, AggregateFunction::Min) => { + "least single player's amount from preceding effect".into() + } + (crate::types::ability::DamageChannel::Excess, _) => { + "excess amount from preceding effect".into() + } + }, QuantityRef::PreviousEffectCount => "count from preceding effect".into(), QuantityRef::TrackedSetSize => "cards moved".into(), QuantityRef::FilteredTrackedSetSize { filter, .. } => { @@ -15970,4 +15991,52 @@ mod tests { "CantHaveKeyword(Flying) should be covered by is_data_carrying_static()" ); } + /// The `fmt_quantity_ref` `PreviousEffectAmount` arms are ORDER-DEPENDENT: + /// the `(_, Sum)` arm must stay first so every Excess-channel corpus card + /// (all of which are `Sum`) keeps rendering the pre-change string. Nothing + /// enforced that ordering — reordering the arms would silently move the + /// coverage signature of every Excess card, reddening CI's coverage check + /// with no indication of the cause. rustc emits NO `unreachable pattern` + /// warning for the reorder, so the compiler will not catch it either. These + /// six assertions -- one per channel/aggregate pair -- are that guard. + #[test] + fn previous_effect_amount_renders_every_channel_aggregate_pair() { + use crate::types::ability::{AggregateFunction, DamageChannel}; + let render = |channel, aggregate| { + fmt_quantity_ref(&QuantityRef::PreviousEffectAmount { channel, aggregate }) + }; + + // Order-dependent: `(_, Sum)` is matched before the Excess catch-all, so + // the Excess+Sum pair renders the SUM string, not the excess one. + assert_eq!( + render(DamageChannel::Total, AggregateFunction::Sum), + "amount from preceding effect" + ); + assert_eq!( + render(DamageChannel::Excess, AggregateFunction::Sum), + "amount from preceding effect", + "the (_, Sum) arm must stay FIRST: Excess+Sum is the shape the corpus \ + actually holds, and it must keep the pre-change signature" + ); + assert_eq!( + render(DamageChannel::Total, AggregateFunction::Max), + "greatest single player's amount from preceding effect" + ); + assert_eq!( + render(DamageChannel::Total, AggregateFunction::Min), + "least single player's amount from preceding effect" + ); + assert_eq!( + render(DamageChannel::Excess, AggregateFunction::Max), + "excess amount from preceding effect" + ); + // The pair space is 2 channels x 3 aggregates = 6, which is more than the + // four match arms; `(Excess, Min)` routes through the same catch-all as + // `(Excess, Max)` and is asserted so the name's claim of completeness is + // literally true rather than true-of-the-arms. + assert_eq!( + render(DamageChannel::Excess, AggregateFunction::Min), + "excess amount from preceding effect" + ); + } } diff --git a/crates/engine/src/game/effects/discard.rs b/crates/engine/src/game/effects/discard.rs index 57ae94f86c..ccab4574ab 100644 --- a/crates/engine/src/game/effects/discard.rs +++ b/crates/engine/src/game/effects/discard.rs @@ -11,7 +11,7 @@ use crate::types::ability::{ }; use crate::types::events::GameEvent; use crate::types::game_state::GameState; -use crate::types::identifiers::ObjectId; +use crate::types::identifiers::{ObjectId, ObjectIncarnationRef, LEGACY_INCARNATION}; use crate::types::player::PlayerId; use crate::types::proposed_event::{AppliedReplacementKey, ProposedEvent}; use crate::types::zones::Zone; @@ -104,7 +104,7 @@ pub(crate) fn complete_discard_to_graveyard( return DiscardOutcome::Complete; } ReplacementResult::NeedsChoice(player) => { - // CR 616.1: The event retains `discard_frame` on the paused + // CR 614.1: The replacement-effect pipeline retains `discard_frame` on the paused // ZoneChange. Generic replacement resume returns to terminal zone // delivery, which appends the exact result and emits bookkeeping. return DiscardOutcome::NeedsReplacementChoice(player); @@ -155,6 +155,73 @@ pub(crate) fn hand_off_recruit_discard_result( true } +/// Park what this seat's discard instruction still owes so the replacement +/// resume can finish it. +/// +/// CR 614.1: a replacement effect can pause this instruction while it is being +/// applied. CR 701.9a is what is still owed: each remaining card must still be +/// moved from its owner's hand to their graveyard. This is the SINGLE AUTHORITY +/// for "this batch paused" — both selection modes park through it, so the two +/// cannot drift on what a parked batch means. +/// +/// Deliberately private and called ONLY from `resolve`, the effect layer. The +/// cost layer owns its own typed cursor (`PendingCostMoveResume:: +/// RandomDiscardUnlessPayment`), because it additionally owes an unless-payment +/// this carrier knows nothing about; sharing one carrier across the two would +/// launder a cost payment into an effect, which is exactly what [`DiscardCause`] +/// exists to make unrepresentable. +#[allow(clippy::too_many_arguments)] +fn park_discard_batch( + state: &mut GameState, + player: PlayerId, + cursor: crate::types::game_state::DiscardBatchCursor, + source_id: ObjectId, + effect_kind: EffectKind, + paused_card: ObjectIncarnationRef, + discard_frame: Option, + preceding_events: Vec, + completion: crate::types::game_state::PendingDiscardBatchCompletion, +) { + let paused_events = preceding_events.clone(); + state.pending_discard_batch = Some(Box::new(crate::types::game_state::PendingDiscardBatch { + player, + cursor, + completion, + source_id, + effect_kind, + paused_card, + discard_frame, + // The `player_scope` driver installs the fan-out remainder, if any, + // as it unwinds — this layer only knows about one seat. + fan_out: None, + preceding_events, + })); + crate::game::engine_resolution_choices::defer_observer_triggers_for_paused_choice( + state, + &paused_events, + 0, + ); +} + +/// CR 400.7: pin the occurrence a replacement pause parked, while the card is +/// still in its pre-move zone. +/// +/// A pause is only ever raised for a live hand card, so the lookup cannot +/// legitimately miss. The fallback pins `LEGACY_INCARNATION`, which no live +/// object can carry — the resume match then fails closed instead of letting a +/// bare `ObjectId` settle the pause against whichever occurrence happens to be +/// leaving the hand. +pub(crate) fn pin_paused_occurrence( + state: &GameState, + object_id: ObjectId, +) -> ObjectIncarnationRef { + state + .objects + .get(&object_id) + .map(ObjectIncarnationRef::from_object) + .unwrap_or_else(|| ObjectIncarnationRef::of(object_id, LEGACY_INCARNATION)) +} + /// CR 701.9a: To discard a card, move it from owner's hand to their graveyard. /// If targets specify specific cards, discard those; otherwise discard from end of hand. pub fn resolve( @@ -175,6 +242,12 @@ pub fn resolve( ), _ => None, }); + // CR 608.2i: the terminal count window for this instruction starts here. + // Everything this node emits before a replacement-application pause is + // carried into the parked batch so the reunited window is exactly what the + // un-paused path would have published. The `player_scope` driver widens it + // to the whole clause's span when the pause interrupted a fan-out. + let events_before_self = events.len(); // CR 701.9b + CR 608.2d: Peel `UpTo` from the count expression to derive // the upper-bound expression and the may-pick-fewer flag. Plain // `QuantityExpr` means a mandatory count; wrapped in `UpTo` means the @@ -319,7 +392,7 @@ pub fn resolve( || (object_bound_discard && parent_reveal_choice_found_nothing) { // Discard specific targeted cards - for obj_id in specific_targets { + for (index, obj_id) in specific_targets.iter().copied().enumerate() { let obj = state .objects .get(&obj_id) @@ -363,6 +436,23 @@ pub fn resolve( crate::game::replacement::replacement_choice_waiting_for( player, state, ); + park_discard_batch( + state, + player_id, + crate::types::game_state::DiscardBatchCursor::Ordered { + remaining: specific_targets[index + 1..] + .iter() + .filter_map(|id| state.objects.get(id)) + .map(ObjectIncarnationRef::from_object) + .collect(), + }, + ability.source_id, + EffectKind::from(&ability.effect), + pin_paused_occurrence(state, obj_id), + discard_frame, + events[events_before_self..].to_vec(), + crate::types::game_state::PendingDiscardBatchCompletion::Standard, + ); return Ok(()); } } @@ -405,6 +495,23 @@ pub fn resolve( ReplacementResult::NeedsChoice(player) => { state.waiting_for = crate::game::replacement::replacement_choice_waiting_for(player, state); + park_discard_batch( + state, + player_id, + crate::types::game_state::DiscardBatchCursor::Ordered { + remaining: specific_targets[index + 1..] + .iter() + .filter_map(|id| state.objects.get(id)) + .map(ObjectIncarnationRef::from_object) + .collect(), + }, + ability.source_id, + EffectKind::from(&ability.effect), + pin_paused_occurrence(state, obj_id), + discard_frame, + events[events_before_self..].to_vec(), + crate::types::game_state::PendingDiscardBatchCompletion::Standard, + ); return Ok(()); } } @@ -444,28 +551,48 @@ pub fn resolve( // CR 701.9a: this is a resolving effect, so Library-of-Leng-class // replacements DO apply — `DiscardCause::Effect`. // - // PRE-EXISTING GAP (unchanged by the extraction, called out so the - // asymmetry with the cost caller below is not mistaken for an - // oversight): a replacement choice mid-batch drops the remaining - // picks, because the effect layer has no batch cursor to resume - // through. The returned cursor is therefore ignored here. The cost - // caller DOES persist it, since it additionally owes a pending - // unless-payment that would otherwise never settle. - if matches!( - discard_at_random( + // CR 614.1: a replacement-application choice mid-batch parks the + // cursor `discard_at_random` returns rather than dropping it; + // `drain_pending_discard_batch` (effects/mod.rs) finishes the + // remaining picks and publishes the terminal marker. The COST caller + // persists the same cursor in its own carrier, because it + // additionally owes an unless-payment this layer has no business + // settling. + if let RandomDiscardOutcome::NeedsReplacementChoice { + remaining_eligible, + remaining_count, + paused_card, + // `discard_at_random` already set `waiting_for` from this value + // and this path parks without re-setting it, so there is + // nothing here to keep in step. The drain that RE-parks does + // consume it. + chooser: _, + } = discard_at_random( + state, + RandomDiscardRequest { + player: discard_player, + source_id: ability.source_id, + count, + eligible: hand_cards, + cause: DiscardCause::Effect, + discard_frame, + }, + events, + ) { + park_discard_batch( state, - RandomDiscardRequest { - player: discard_player, - source_id: ability.source_id, - count, - eligible: hand_cards, - cause: DiscardCause::Effect, - discard_frame, + discard_player, + crate::types::game_state::DiscardBatchCursor::Random { + pool: remaining_eligible, + remaining: remaining_count, }, - events, - ), - RandomDiscardOutcome::NeedsReplacementChoice { .. } - ) { + ability.source_id, + EffectKind::from(&ability.effect), + paused_card, + discard_frame, + events[events_before_self..].to_vec(), + crate::types::game_state::PendingDiscardBatchCompletion::Standard, + ); return Ok(()); } } else if hand_cards.is_empty() { @@ -473,7 +600,7 @@ pub fn resolve( } else if !up_to && hand_cards.len() <= count { // Forced discard — no choice needed, discard all eligible cards. // When up_to=true, always present the choice (player may discard fewer). - for obj_id in &hand_cards { + for (i, obj_id) in hand_cards.iter().enumerate() { if let DiscardOutcome::NeedsReplacementChoice(player) = discard_caused_by_effect_with_source_and_frame( state, @@ -486,8 +613,26 @@ pub fn resolve( { state.waiting_for = crate::game::replacement::replacement_choice_waiting_for(player, state); - // Known limitation: EffectResolved is not emitted when replacement - // choice interrupts forced-discard (same systemic gap as sacrifice). + // CR 614.1 + CR 701.9a: park the un-iterated tail instead of + // abandoning it. `hand_cards[i + 1..]` and not `[i..]`: the + // paused card is settled by the replacement itself, exactly + // as `discard_at_random`'s cursor documents. The terminal + // `EffectResolved` below is unreachable from here, so the + // drain emits it — see `drain_pending_discard_batch`. + park_discard_batch( + state, + discard_player, + crate::types::game_state::DiscardBatchCursor::All { + remaining: hand_cards[i + 1..].to_vec(), + }, + ability.source_id, + EffectKind::from(&ability.effect), + // CR 400.7: the pause parks the pre-move occurrence. + pin_paused_occurrence(state, *obj_id), + discard_frame, + events[events_before_self..].to_vec(), + crate::types::game_state::PendingDiscardBatchCompletion::Standard, + ); return Ok(()); } } @@ -617,6 +762,26 @@ pub(crate) enum RandomDiscardOutcome { remaining_eligible: Vec, /// Picks still owed AFTER the paused one resolves. remaining_count: usize, + /// The card whose replacement raised the choice. CR 614.6: the replaced + /// event never happens and a modified event happens instead, so this + /// card was still discarded and the effect layer's drain needs its + /// identity to stamp the terminal `Discarded` the resumed zone-change + /// arm cannot emit. The cost layer does not consume it. + /// + /// CR 400.7: the PRE-move occurrence, captured while the card is still + /// in hand. The drain settles the pause against this exact occurrence + /// leaving the hand, so a later same-id occurrence cannot claim it. + paused_card: ObjectIncarnationRef, + /// The replacement pipeline's selected chooser. Published by this + /// authority rather than re-derived at the call site, because it is NOT + /// always the discarding player — see the commander carve-out in + /// `replacement_choice_player`, where the choice belongs to a seat other + /// than the affected one. A re-parking caller that assumed + /// `request.player` would prompt the wrong seat the moment such a case + /// reaches a random discard. Mirrors the `chooser` the single-card + /// `DiscardOutcome::NeedsReplacementChoice` already carries, + /// so both cursor arms read one contract. + chooser: PlayerId, }, } @@ -702,6 +867,12 @@ pub(crate) fn discard_at_random( // The paused pick is settled by the replacement itself, so the // resumed batch owes only the picks after it. remaining_count: count - pick - 1, + // CR 400.7: pinned before the redirect moves it, so the resume + // settles against this occurrence and not a later same-id one. + paused_card: pin_paused_occurrence(state, obj_id), + // Same value this function just set `waiting_for` from, so a + // re-parking caller cannot drift from the prompt actually shown. + chooser, }; } } @@ -736,6 +907,52 @@ fn route_discard( discard_frame: Option, events: &mut Vec, ) -> DiscardOutcome { + // CR 701.9a: "To discard a card, move it from its owner's hand to that + // player's graveyard." A card that is not in a hand cannot be discarded, so + // there is no event to propose. + // + // Placed here because every *proposed* discard routes through this function + // — effect and cost layers, whole-hand and random cursors — so one guard + // covers them all. (Not the same as every discard: three callers reach + // `complete_discard_to_graveyard` directly, at `:397` here and in + // `engine_replacement.rs` / `engine_payment_choices.rs`. Those are RESUMES of + // an event this function already proposed and guarded, which is why they are + // not a hole — but the claim is "every proposal", not "every discard".) + // + // It became load-bearing with the parked batch: a cursor is a hand snapshot + // latched BEFORE an action boundary and drained after one, so anything that + // moved a listed card in between would otherwise be "discarded" out of + // whatever zone it now occupies — `complete_discard_to_graveyard` lowers to + // a hard-coded `from: Hand`. Un-paused callers build and consume their + // snapshot inside one action and cannot observe a difference. + // + // Modelled on this file's `Prevented` arms, which are its existing answer to + // "the card never left the hand, so no discard occurred": both retire the + // frame and report `Complete`. Retiring matters — a + // `DiscardedCardMatchesFilter` frame left active would leak when every + // listed card has already moved. + // + // WHICH arms, stated because an earlier revision of this comment named the + // wrong one: the two that retire are in `complete_discard_to_graveyard` and + // in `resolve`'s specific-target loop, both ABOVE. This function's own + // `Prevented` arm below does NOT retire — an inherited asymmetry left + // untouched, since whether that arm is reachable at all with a frame present + // was not measured here, and writing a fix for an unmeasured path is how the + // wrong-arm claim got in. + // + // `Complete` is a known imprecision INHERITED from those arms, not introduced + // here: `DiscardOutcome` has no "nothing happened" variant, so a cost caller + // reads `Complete` as paid. A prevented discard already launders an unpayable + // cost the same way (CR 118.3 wants all-or-nothing). Fixing it means a third + // variant threaded through every caller, which is a change this PR has no + // mandate for and no test for; the shape is recorded here rather than in a + // commit message so the next person to touch `DiscardOutcome` finds it. + if state.objects.get(&object_id).map(|obj| obj.zone) != Some(Zone::Hand) { + if let Some(frame_id) = discard_frame { + retire_discard_frame(state, frame_id); + } + return DiscardOutcome::Complete; + } let proposed = ProposedEvent::Discard { player_id: player, object_id, @@ -1046,6 +1263,8 @@ mod random_discard_authority_tests { let RandomDiscardOutcome::NeedsReplacementChoice { remaining_eligible, remaining_count, + paused_card, + chooser, } = outcome else { panic!("expected a replacement pause, got {outcome:?}"); @@ -1059,6 +1278,38 @@ mod random_discard_authority_tests { 3, "the un-picked pool excludes only the paused card" ); + // The cursor's two halves must agree on WHICH card paused: the reported + // paused card is the one missing from the un-picked pool. + assert!( + hand.contains(&paused_card.object_id) + && !remaining_eligible.contains(&paused_card.object_id), + "the paused card must be a hand card that left the un-picked pool" + ); + // CR 400.7: the pin is the PRE-move occurrence, so it must still name + // the live hand card. A pin taken after the redirect would carry the + // bumped incarnation and never match the departure it is meant to settle. + assert_eq!( + Some(paused_card), + state + .objects + .get(&paused_card.object_id) + .map(ObjectIncarnationRef::from_object), + "the parked pin must equal the live pre-move occurrence" + ); + // The published chooser must be the seat this authority actually + // prompted. A re-parking caller reads `chooser` to rebuild the + // prompt, so if the two ever disagree the wrong seat is asked. Compared + // against `waiting_for` rather than against the request's player, + // because agreeing with the request is the very assumption this pins + // against — the drain used to re-derive it that way. + let prompted = match &state.waiting_for { + crate::types::game_state::WaitingFor::ReplacementChoice { player, .. } => *player, + other => panic!("expected an installed ReplacementChoice, got {other:?}"), + }; + assert_eq!( + chooser, prompted, + "the outcome's chooser must equal the seat `waiting_for` was built from" + ); } /// Caller contract (documented on the authority): a pool shorter than @@ -1073,6 +1324,159 @@ mod random_discard_authority_tests { assert_eq!(outcome, RandomDiscardOutcome::Completed); assert_eq!(discarded(&state, &hand).len(), 2); } + + /// CR 701.9a: "To discard a card, move it from its owner's hand to that + /// player's graveyard." A card that is no longer in a hand when its + /// proposal is reached cannot be discarded, so `route_discard` must propose + /// nothing for it. + /// + /// The real shape is a parked batch — a cursor latches a hand snapshot + /// BEFORE an action boundary and drains after one, so a listed card can have + /// left the hand in between, and `complete_discard_to_graveyard` lowers to a + /// hard-coded `from: Hand`. Staged directly here rather than through the + /// batch machinery so a failure names the guard and not the driver. + /// + /// NON-VACUITY is the first assertion, not the second: an inert + /// `route_discard` that discarded nothing at all would satisfy the negative + /// half. The in-hand card must actually be discarded for the moved card's + /// silence to mean anything. + /// + /// REVERT PROBE (RUN, not reasoned): delete the `!= Some(Zone::Hand)` early + /// return at the top of `route_discard`. Observed first failure is the + /// `discarded_ids` assertion, which goes `[stays]` -> `[stays, moved]`. + /// + /// The `relowered` assertion below is therefore DOMINATED under that probe — + /// it never gets to run. It is kept deliberately, and its scope is stated + /// here rather than left implied: it covers a DIFFERENT failure, one that + /// lowers the hand -> graveyard `ZoneChange` while suppressing the + /// `Discarded` push. No probe in this lane exercises that one, and this + /// fixture passes `discard_frame: None`, so it cannot reach the frame-borne + /// route where that split is what actually happens today. + #[test] + fn route_discard_skips_a_card_that_already_left_the_hand() { + let (mut state, hand) = hand_of(42, 2); + let (stays, moved) = (hand[0], hand[1]); + let mut setup = Vec::new(); + crate::game::zones::move_to_zone(&mut state, moved, Zone::Graveyard, &mut setup); + assert_eq!( + state.objects[&moved].zone, + Zone::Graveyard, + "reach guard: the card under test must genuinely be out of the hand" + ); + + let mut events = Vec::new(); + for card in [stays, moved] { + route_discard(&mut state, card, PlayerId(0), None, true, None, &mut events); + } + + let discarded_ids: Vec = events + .iter() + .filter_map(|e| match e { + GameEvent::Discarded { object_id, .. } => Some(*object_id), + _ => None, + }) + .collect(); + assert_eq!( + discarded_ids, + vec![stays], + "the in-hand card must be discarded (non-vacuity) and the already-moved \ + card must produce no discard" + ); + let relowered = events + .iter() + .filter(|e| { + matches!( + e, + GameEvent::ZoneChanged { object_id, from: Some(Zone::Hand), .. } + if *object_id == moved + ) + }) + .count(); + assert_eq!( + relowered, 0, + "no hand -> graveyard move may be lowered for a card that was not in a hand" + ); + } + + /// The frame half of the same guard: a `DiscardedCardMatchesFilter` frame is + /// opened by `resolve` for the whole instruction, so bailing out of a listed + /// card without retiring it leaves an active frame owning nothing — and + /// `active_discard` is LIFO, so the next operation reads it as its own. + /// + /// TWO frames are installed, and what that buys is ARITY AND DIRECTION, not + /// identity: a single-frame fixture cannot separate "retired one frame" from + /// "emptied the stack", while nesting catches a retirement that pops zero, + /// pops two, or pops from the wrong end. + /// + /// It does NOT establish that the guard retired the frame it was HANDED, and + /// an earlier revision of this doc claimed it did. The fixture hands the + /// guard the frame already on top, so "retire the handed frame" and "retire + /// the top" are one action here — and they are one action in PRODUCTION too: + /// `retire_discard_frame` calls `take_active_discard`, which pops the top + /// WHEN THAT TOP IS A `Discard` FRAME — returning `Err(UnexpectedTop)` + /// otherwise — with `frame_id` consulted only by a `debug_assert_eq!`. + /// The id-keyed property is therefore ABSENT FROM THE CODE rather than + /// merely unmeasured, so a test demanding it would red on HEAD. Recorded + /// here instead of asserted: a failing test for a property the design does + /// not claim is noise, not coverage. + /// + /// DISCLOSED, NOT REPAIRED, because the qualifier above is load-bearing: + /// `retire_discard_frame` swallows that `Err` (and the empty case) in an + /// `if let Ok(Some(..))`, so retirement is BEST-EFFORT. If a non-`Discard` + /// frame sits on top when this guard fires, the retirement silently no-ops + /// and the frame survives owning nothing — precisely the hazard the first + /// paragraph of this doc names. Its reachability was not measured, and + /// making retirement total is a change to the resolution stack's error + /// contract rather than to this guard. Same disposition as `route_discard`'s + /// own non-retiring `Prevented` arm. + /// + /// REVERT PROBES (RUN): delete the `retire_discard_frame` call from inside + /// the guard, keeping the early return — reds at this test's own assertion. + /// Calling it TWICE also reds, but through `retire_discard_frame`'s + /// `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 runs. + #[test] + fn route_discard_retires_the_frame_for_a_card_that_left_the_hand() { + let (mut state, hand) = hand_of(7, 1); + let card = hand[0]; + let mut setup = Vec::new(); + crate::game::zones::move_to_zone(&mut state, card, Zone::Graveyard, &mut setup); + + let outer = state.resolution_stack.begin_discard(Some(ObjectId(499))); + let frame = state.resolution_stack.begin_discard(Some(ObjectId(500))); + assert_eq!( + state + .resolution_stack + .active_discard() + .expect("reach guard: a frame must be active before the call") + .id, + frame, + "reach guard: the INNER frame must be the one on top, or the pop below proves nothing" + ); + + let mut events = Vec::new(); + route_discard( + &mut state, + card, + PlayerId(0), + None, + true, + Some(frame), + &mut events, + ); + + assert_eq!( + state + .resolution_stack + .active_discard() + .expect("exactly one frame may be retired, leaving the outer one active") + .id, + outer, + "the guard must retire EXACTLY ONE frame, popped from the top: the outer frame survives" + ); + } } #[cfg(test)] diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 39554af2d7..288cf51a19 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -22,11 +22,11 @@ use crate::types::ability::{ use crate::types::ability::{AttackScope, AttackSubject}; use crate::types::events::{GameEvent, PlayerActionKind}; use crate::types::game_state::{ - AutoMayChoice, CastOfferKind, ClauseMinimumSnapshot, DayNight, GameState, LKISnapshot, - ManaAbilityResume, MayTriggerAutoChoiceKey, PendingContinuation, PendingCopyTokenBatch, - PendingCostMoveResume, PendingPlayerScopeSacrificeChoice, - PendingPlayerScopeSacrificeCompletion, PendingPlayerScopeSacrificeFollowUp, WaitingFor, - ZoneChangeRecord, + AutoMayChoice, CastOfferKind, ClauseMinimumSnapshot, DayNight, DiscardBatchCursor, GameState, + LKISnapshot, ManaAbilityResume, MayTriggerAutoChoiceKey, PendingContinuation, + PendingCopyTokenBatch, PendingCostMoveResume, PendingDiscardBatchCompletion, + PendingPlayerScopeSacrificeChoice, PendingPlayerScopeSacrificeCompletion, + PendingPlayerScopeSacrificeFollowUp, WaitingFor, ZoneChangeRecord, }; use crate::types::identifiers::{ObjectId, TrackedSetId}; use crate::types::mana::ManaCost; @@ -4457,12 +4457,14 @@ fn detach_after_multi_target_player_local_chain( tail } -/// CR 608.2e: Collect cross-player equalization quantity references from a -/// `QuantityExpr`. These are the refs whose value would shift as an APNAP -/// fan-out mutates the board — `ControlledByEachPlayer` (battlefield extremum) -/// and `HandSize { AllPlayers }` (hand extremum). The per-player `left` operand -/// of a `Difference` is intentionally NOT collected: it must re-resolve per -/// iterating player. +/// CR 608.2h + CR 608.2e: Collect the quantity references whose answer is +/// determined only once, when the clause is applied (608.2h), across an APNAP +/// fan-out that is one action processed simultaneously (608.2e). Three classes +/// are admitted: `ControlledByEachPlayer` (battlefield extremum), +/// `HandSize { AllPlayers }` (hand extremum), and `PreviousEffectAmount` +/// (a look-back at a COMPLETED instruction's result — see the arm below). The +/// per-player `left` operand of a `Difference` is intentionally NOT collected: +/// it must re-resolve per iterating player. fn collect_clause_minimum_refs<'a>(expr: &'a QuantityExpr, out: &mut Vec<&'a QuantityRef>) { match expr { QuantityExpr::Ref { qty } => { @@ -4472,6 +4474,57 @@ fn collect_clause_minimum_refs<'a>(expr: &'a QuantityExpr, out: &mut Vec<&'a Qua | QuantityRef::HandSize { player: PlayerScope::AllPlayers { .. } } + // CR 608.2h: "the answer is determined only once, when the + // effect is applied." A `PreviousEffectAmount` is a look-back + // (CR 608.2i) at a COMPLETED instruction's result — it has no + // per-iteration reading at all (that reading is + // `EventContextAmount`, `game/quantity.rs`'s + // `QuantityRef::EventContextAmount` arm), so every channel + // and every aggregate is clause-frozen. Without this, each + // player's own completed action re-stamps the shared scalar + // (`install_previous_effect_counts_by_player`'s post-stamp → + // `previous_effect_amount_from_events`' `Effect::Draw` arm) + // and a later player inherits an earlier player's DELIVERED + // count — Windfall with a short library drew [5,5,5,5] + // instead of [5,8,8,8]. CR 608.2e supports it (one action, + // processed simultaneously); CR 121.2c confirms the + // SERIALIZATION of the multiplayer draw is itself correct — + // only the leaked count is not. + // + // PRECONDITION, unenforced by construction: this admission is + // unconditional, and `capture_clause_minimum_snapshot` walks + // the WHOLE scoped sub-chain, so it rests on the convention + // stated above — `PreviousEffectAmount` is clause-wide, + // `EventContextAmount` is per-iteration. A sub-link RETAINED + // inside the scoped template whose `PreviousEffectAmount` is + // meant to read *that iteration's* preceding effect would be + // frozen at the pre-clause value instead. + // + // No card does this today, measured over the corpus: 44 CARDS + // carry both a `player_scope` and a `PreviousEffectAmount` + // somewhere. 3 of them (Parallax Nexus/Tide/Wave) hold it + // only OUTSIDE the scoped subtree, in a condition, so they + // never reach this arm. The other 41 hold it inside the + // scoped subtree in a quantity position — 42 NODES, because + // Thorna and Twigtooth holds two. (Node and card counts + // differ here; an earlier revision of this comment conflated + // them and mis-partitioned the result.) + // + // The split that discriminates is which effect carries the + // ref: 38 `GainLife` — the drain tail, which DETACHES because + // `effect_has_iteration_bound_recipient` has no `GainLife` + // arm — 3 `Draw` (Windfall, Jace's Archivist, Whispering + // Madness), and 1 `LoseLife` (Thorna). + // + // Thorna is the only RETAINED-side carrier, so it is the one + // card that could falsify the precondition — 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. If a future card needs a per-iteration reading + // here, it wants `EventContextAmount`, not a guard on this + // arm. + | QuantityRef::PreviousEffectAmount { .. } ) { out.push(qty); } @@ -4497,11 +4550,14 @@ fn collect_clause_minimum_refs<'a>(expr: &'a QuantityExpr, out: &mut Vec<&'a Qua } } -/// CR 608.2e (§8): Capture this `player_scope` link's equalization extrema -/// against the board as it stands NOW — before the APNAP fan-out begins. The -/// snapshot is stored on `state.clause_minimum_snapshot` and consulted by the -/// `ControlledByEachPlayer` / `HandSize { AllPlayers }` resolver arms so every -/// player in the fan-out sees the same pre-clause minimum. +/// CR 608.2h + CR 608.2e (§8): Capture this `player_scope` link's clause-frozen +/// quantities against the board as it stands NOW — before the APNAP fan-out +/// begins — because the answer is determined only once, when the effect is +/// applied (608.2h), and the fan-out is one action processed simultaneously +/// (608.2e). The snapshot is stored on `state.clause_minimum_snapshot` and +/// consulted by the `ControlledByEachPlayer` / `HandSize { AllPlayers }` / +/// `PreviousEffectAmount` resolver arms so every player in the fan-out sees the +/// same pre-clause value. /// /// Always overwrites `state.clause_minimum_snapshot` — to `Some` when the /// clause carries a cross-player extremum, to `None` otherwise. This makes @@ -4522,8 +4578,10 @@ fn collect_clause_minimum_refs<'a>(expr: &'a QuantityExpr, out: &mut Vec<&'a Qua /// structural rather than relying on the three cards' clauses using /// pairwise-distinct `QuantityRef` keys. fn capture_clause_minimum_snapshot(state: &mut GameState, scoped_template: &ResolvedAbility) { - // CR 608.2e: values are locked when the clause starts resolving, so each - // clause must capture against its own pre-clause board. + // CR 608.2h + CR 608.2e: the answer is determined only once, when the + // effect is applied, and the clause's fan-out is one action processed + // simultaneously — so values are locked when the clause starts resolving + // and each clause must capture against its own pre-clause board. // // Per-link reset: clear any previous clause's snapshot before resolving so // the live-resolve below sees a clean slate and a stale value is never @@ -8513,17 +8571,24 @@ fn previous_effect_excess_amount_from_events( (excess > 0).then_some(excess) } +/// The per-player table a completed instruction leaves behind for a later +/// look-back (CR 608.2i), derived from the terminal event window. +/// +/// Keyed by `EffectKind` rather than `&Effect`: both selections this function +/// makes — which effects are count producers, and how each producer's counts are +/// derived — are kind-level facts, and a batch parked across a replacement +/// pause holds only the kind. Passing the kind is what lets the paused and +/// un-paused paths share ONE count authority instead of growing a second, +/// drifting counter. fn previous_effect_counts_by_player_from_events( - effect: &Effect, + kind: EffectKind, source_id: ObjectId, events: &[GameEvent], ) -> Option> { - let kind = match effect { - Effect::Discard { .. } | Effect::DiscardCard { .. } | Effect::ChangeZoneAll { .. } => { - EffectKind::from(effect) - } + match kind { + EffectKind::Discard | EffectKind::DiscardCard | EffectKind::ChangeZoneAll => {} _ => return None, - }; + } // CR 608.2c: An effect's terminal marker bounds exactly its own completed // instruction. The supplied slice is already scoped to the current parent // or player-scope pass; events later in that slice belong to later work and @@ -8540,11 +8605,11 @@ fn previous_effect_counts_by_player_from_events( })?; let mut counts = HashMap::new(); - match effect { + match kind { // CR 701.9a: `Discarded::source_id` is the causal source authority. // A same-window discard from another effect cannot be attributed to // this instruction merely because it happened before this marker. - Effect::Discard { .. } | Effect::DiscardCard { .. } => { + EffectKind::Discard | EffectKind::DiscardCard => { for event in &events[..=resolved_index] { if let GameEvent::Discarded { player_id, @@ -8562,7 +8627,7 @@ fn previous_effect_counts_by_player_from_events( // the move event. This collects all cards moved by the completed // ChangeZoneAll instruction, including zero-card players as an empty // map when the terminal marker is present. - Effect::ChangeZoneAll { .. } => { + EffectKind::ChangeZoneAll => { for event in &events[..=resolved_index] { if let GameEvent::ZoneChanged { record, .. } = event { *counts.entry(record.owner).or_insert(0) += 1; @@ -8574,6 +8639,32 @@ fn previous_effect_counts_by_player_from_events( Some(counts) } +/// CR 608.2c: give every player the clause applied to an entry in the +/// completed-instruction table, defaulting a non-contributor to zero. +/// +/// The table is built from emitted events, so a player who contributed nothing — +/// an empty hand facing "each player discards their hand" — emits no event and +/// would otherwise be absent. They still discarded zero *this way*, and the +/// table is what an aggregate reduces over, so an omission is a wrong reduction +/// domain rather than a missing convenience. +/// +/// The omission is invisible to two of the three aggregates, which is why it +/// survived: `Sum` reads `last_effect_amount` (and adding zeros could not move a +/// sum anyway) and `Max` cannot be raised by zeros. Only `Min` sees it — hands +/// 8/7/3/**0** publish `{8,7,3}` and answer 3 where the answer is 0. The defect +/// is the domain, not the `Min` arm. +/// +/// Existing entries are never overwritten: a player who contributed 3 keeps 3. +fn fill_zero_contributors( + mut counts_by_player: HashMap, + matching_players: &[PlayerId], +) -> HashMap { + for player in matching_players.iter().copied() { + counts_by_player.entry(player).or_insert(0); + } + counts_by_player +} + /// CR 608.2c: Install the terminal-window per-player counts for a completed /// instruction. `Some(empty)` is a real zero-result producer and must replace /// an older table; `None` means this effect has no such count channel, so clear @@ -8606,6 +8697,97 @@ fn install_previous_effect_counts_by_player( } } +/// Publish one COMPLETED `player_scope` clause's terminal results: the +/// per-player table (zero-filled over `zero_fill_domain`), the scalar/excess +/// fallback, the tracked set, and `last_zone_changed_ids`. +/// +/// CR 608.2f: a clause is one action taken on multiple players; when a +/// replacement-application choice makes it non-simultaneous it is processed per +/// player, but it stays ONE action and therefore has exactly ONE terminal +/// result. This function is extracted so a clause that finished inside the +/// driver and a clause that finished inside a resumed +/// [`drain_pending_discard_batch`] publish through the same authority and +/// cannot drift. +/// +/// `scoped_events` is the clause's full event span. The driver passes its live +/// slice; a resumed batch passes its pre-pause span reunited with the resumed +/// action's buffer. +fn publish_player_scope_clause_results( + state: &mut GameState, + outer: &ResolvedAbility, + scoped_template: &ResolvedAbility, + zero_fill_domain: &[PlayerId], + after_scope_needs_linked_exile: bool, + scoped_events: &[GameEvent], +) { + let counts_by_player = previous_effect_counts_by_player_from_events( + EffectKind::from(&scoped_template.effect), + scoped_template.source_id, + scoped_events, + ); + let counts_by_player = + counts_by_player.map(|counts| fill_zero_contributors(counts, zero_fill_domain)); + if !install_previous_effect_counts_by_player(state, counts_by_player, false) { + if let Some(amount) = + previous_effect_amount_from_events(state, scoped_template, scoped_events) + { + state.last_effect_amount = Some(amount); + // CR 120.10: stamp the resolution-local excess channel alongside + // the running total so a follow-up "if excess damage was dealt + // this way" condition reads overkill-beyond-lethal. CR 120.6 was + // cited for that total and is struck: it governs damage MARKED on + // a creature until the cleanup step, not the amount one clause + // leaves for a later clause in the same resolution — that + // carry-forward is CR 608.2c. + let excess = + previous_effect_excess_amount_from_events(state, scoped_template, scoped_events); + state.last_effect_excess_amount = excess; + } + } + let affected_with_causes = + if next_sub_needs_tracked_set(outer) || after_scope_needs_linked_exile { + affected_objects_with_causes( + state, + scoped_template, + &scoped_template.effect, + scoped_events, + ) + } else { + Vec::new() + }; + let affected_ids: Vec = affected_with_causes.iter().map(|(id, _)| *id).collect(); + if after_scope_needs_linked_exile { + for id in &affected_ids { + if state + .objects + .get(id) + .is_some_and(|obj| obj.zone == crate::types::zones::Zone::Exile) + { + crate::game::exile_links::push_tracked_by_source(state, *id, outer.source_id); + } + } + } + // CR 608.2c: After a `player_scope: All` sacrifice clause completes, + // publish the full scoped event slice so downstream "if you sacrificed + // a permanent this way" / ZoneChangedThisWay gates see every player's + // sacrifice — not only the last iteration's overwrite of + // `last_zone_changed_ids`. + let mut ids: Vec = scoped_events + .iter() + .filter_map(|event| match event { + GameEvent::ZoneChanged { object_id, .. } + | GameEvent::PermanentSacrificed { object_id, .. } => Some(*object_id), + _ => None, + }) + .collect(); + ids.sort_unstable_by_key(|id| id.0); + ids.dedup(); + state.last_zone_changed_ids = ids; + if next_sub_needs_tracked_set(outer) { + publish_tracked_set_with_causes(state, affected_with_causes); + } +} + fn effect_consumes_event_context_amount(effect: &Effect) -> bool { let mut consumes = false; effect.for_each_quantity_expr(&mut |quantity| { @@ -9255,6 +9437,413 @@ pub(crate) fn drain_pending_player_scope_sacrifice_after_replacement( } } +pub(crate) enum PendingDiscardBatchOutcome { + /// No batch was parked; nothing was done. + Idle, + /// The batch (or the fan-out behind it) paused again. `state.waiting_for` + /// carries the new prompt. + PausedForReplacement, + /// The whole instruction settled and published its terminal results. + Completed, +} + +/// Finish a discard instruction that a replacement-application choice parked +/// mid-batch, and publish its terminal result ONCE. +/// +/// CR 614.1: the replacement application pauses the event as it happens. CR +/// 608.2f is why the remainder belongs here and not on the generic continuation +/// queue: the clause is one action taken on several players, processed per +/// player only because it could not be processed simultaneously — so it still +/// has exactly one terminal result. +/// +/// Composability: an arbitrary number of sequential re-pauses compose, because +/// each resume re-enters this same function through the same hook. +pub(crate) fn drain_pending_discard_batch( + state: &mut GameState, + events: &mut Vec, +) -> Result { + let Some(mut batch) = state.pending_discard_batch.take() else { + return Ok(PendingDiscardBatchOutcome::Idle); + }; + + stamp_resumed_discard_if_unrecorded(state, &batch, events); + + // Finish what this seat still owes. The cursor is replaced with an empty + // one so a re-park below installs a fresh remainder rather than mutating a + // borrowed value. + let cursor = std::mem::replace( + &mut batch.cursor, + DiscardBatchCursor::All { + remaining: Vec::new(), + }, + ); + match cursor { + DiscardBatchCursor::All { remaining } => { + for (i, obj_id) in remaining.iter().enumerate() { + if let discard::DiscardOutcome::NeedsReplacementChoice(chooser) = + discard::discard_caused_by_effect_with_source_and_frame( + state, + *obj_id, + batch.player, + Some(batch.source_id), + batch.discard_frame, + events, + ) + { + batch.cursor = DiscardBatchCursor::All { + remaining: remaining[i + 1..].to_vec(), + }; + // CR 400.7: pin the pre-move occurrence, matching the park + // the `Random` arm below already receives from + // `discard_at_random`. + batch.paused_card = discard::pin_paused_occurrence(state, *obj_id); + repark_discard_batch(state, batch, events, chooser); + return Ok(PendingDiscardBatchOutcome::PausedForReplacement); + } + } + } + DiscardBatchCursor::Random { pool, remaining } => { + if let discard::RandomDiscardOutcome::NeedsReplacementChoice { + remaining_eligible, + remaining_count, + paused_card, + chooser, + } = discard::discard_at_random( + state, + discard::RandomDiscardRequest { + player: batch.player, + source_id: batch.source_id, + count: remaining, + eligible: pool, + cause: discard::DiscardCause::Effect, + discard_frame: batch.discard_frame, + }, + events, + ) { + batch.cursor = DiscardBatchCursor::Random { + pool: remaining_eligible, + remaining: remaining_count, + }; + batch.paused_card = paused_card; + // The chooser comes from the authority that raised the choice, + // exactly as the `All` arm above threads its own. + // It was `batch.player` here, which happens to agree today + // because a hand card's `affected_player` is its controller — + // but `replacement_choice_player`'s commander carve-out proves + // the engine already has cases where chooser != affected seat, + // and re-deriving at the call site is how those drift. + repark_discard_batch(state, batch, events, chooser); + return Ok(PendingDiscardBatchOutcome::PausedForReplacement); + } + } + DiscardBatchCursor::Ordered { remaining } => { + for (i, card) in remaining.iter().enumerate() { + if !card.is_current(state) + || state.objects.get(&card.object_id).map(|object| object.zone) + != Some(Zone::Hand) + { + continue; + } + let player = state.objects[&card.object_id].owner; + if let discard::DiscardOutcome::NeedsReplacementChoice(chooser) = + discard::discard_caused_by_effect_with_source_and_frame( + state, + card.object_id, + player, + Some(batch.source_id), + batch.discard_frame, + events, + ) + { + batch.cursor = DiscardBatchCursor::Ordered { + remaining: remaining[i + 1..].to_vec(), + }; + batch.player = player; + batch.paused_card = *card; + repark_discard_batch(state, batch, events, chooser); + return Ok(PendingDiscardBatchOutcome::PausedForReplacement); + } + } + } + } + + if matches!( + &batch.completion, + PendingDiscardBatchCompletion::DiscardChoice { .. } + ) { + let mut window = batch.preceding_events.clone(); + window.extend_from_slice(events); + finalize_discard_choice_completion(state, &batch.completion, batch.discard_frame, &window); + } + + // CR 608.2c: the terminal marker the pre-pause action could not emit, + // because it returned from inside the batch loop. Without it this seat's + // count is underivable — `previous_effect_counts_by_player_from_events` + // early-returns at its `rposition`. + events.push(GameEvent::EffectResolved { + kind: batch.effect_kind, + source_id: batch.source_id, + subject: None, + }); + + // CR 608.2f + CR 101.4: run the clause's remaining seats, in the APNAP + // order latched at the pause. + if let Some(fan_out) = batch.fan_out.take() { + let fan_out = *fan_out; + let initial_waiting_for = state.waiting_for.clone(); + for (i, pid) in fan_out.remaining_players.iter().enumerate() { + let mut scoped = (*fan_out.scoped_template).clone(); + // CR 608.2c + CR 101.3: each scoped iteration is a fresh + // sub-resolution of the scoped template, so the cost-payment-failed + // signal is per-iteration. This is the same resumption boundary the + // driver's own loop resets; without it an earlier seat's mandatory + // failure (an empty-handed seat's `count == 0 && !up_to` arm) leaks + // into a later seat's `IfCurrentScopeSucceeded` read, for cards like + // Refurbished Familiar and Aclazotz, Deepest Betrayal. + state.cost_payment_failed_flag = false; + scoped.set_original_controller_recursive(fan_out.original_controller); + scoped.set_controller_recursive(*pid); + scoped.set_scoped_player_recursive(*pid); + resolve_ability_chain(state, &scoped, events, 1)?; + if state.waiting_for == initial_waiting_for { + continue; + } + // This seat paused. If it parked its OWN discard batch, move the + // clause remainder onto that batch so the instruction still ends in + // one publication. + if let Some(next) = state.pending_discard_batch.as_mut() { + if next.fan_out.is_none() + && next.source_id == fan_out.scoped_template.source_id + && next.player == *pid + { + let mut window = std::mem::take(&mut batch.preceding_events); + window.extend_from_slice(events); + next.preceding_events = window; + next.fan_out = Some(Box::new(crate::types::game_state::PendingDiscardFanOut { + remaining_players: fan_out.remaining_players[i + 1..].to_vec(), + ..fan_out.clone() + })); + return Ok(PendingDiscardBatchOutcome::PausedForReplacement); + } + } + // BOUNDARY (measured, and deliberately not repaired here): the seat + // paused on something that is not a batch pause — an interactive + // `WaitingFor::DiscardChoice`, or any other resolution choice. Hand + // the remaining seats back to the generic continuation queue exactly + // as the driver does, and publish NOTHING: those legs each publish + // node-locally, which is the pre-existing behaviour this change does + // not extend to the interactive path. + let mut tail: Option> = None; + for &remaining_pid in fan_out.remaining_players[i + 1..].iter().rev() { + let mut remaining_scoped = (*fan_out.scoped_template).clone(); + remaining_scoped.set_original_controller_recursive(fan_out.original_controller); + remaining_scoped.set_controller_recursive(remaining_pid); + remaining_scoped.set_scoped_player_recursive(remaining_pid); + remaining_scoped.sub_link = SubAbilityLink::SequentialSibling; + if let Some(prev) = tail { + super::ability_utils::append_to_sub_chain(&mut remaining_scoped, *prev); + } + tail = Some(Box::new(remaining_scoped)); + } + if tail.is_some() { + append_to_pending_continuation(state, tail); + } + return Ok(PendingDiscardBatchOutcome::PausedForReplacement); + } + + // CR 608.2i: the look-back window is everything this instruction did, + // on both sides of the pause. `preceding_events` was copied rather than + // drained, so `events` still holds only the resumed action's own span. + let mut window = std::mem::take(&mut batch.preceding_events); + window.extend_from_slice(events); + publish_player_scope_clause_results( + state, + &fan_out.outer, + &fan_out.scoped_template, + &fan_out.matching_players, + fan_out.after_scope_needs_linked_exile, + &window, + ); + // CR 608.2h: the clause has completed, so clear its frozen values before + // the parked tail runs — a following `player_scope` clause captures its + // own snapshot against the post-this-clause board. + state.clause_minimum_snapshot = None; + return Ok(PendingDiscardBatchOutcome::Completed); + } + + // Single-subject discard: no fan-out, so no reduction domain to zero-fill. + // Mirrors the non-`player_scope` publication site, whose `preserve` argument + // is provably irrelevant here — it is read only on the `None` arm, and the + // marker pushed above guarantees `Some`. + let mut window = std::mem::take(&mut batch.preceding_events); + window.extend_from_slice(events); + install_previous_effect_counts_by_player( + state, + previous_effect_counts_by_player_from_events(batch.effect_kind, batch.source_id, &window), + false, + ); + if !matches!( + &batch.completion, + PendingDiscardBatchCompletion::DiscardChoice { .. } + ) { + state.last_zone_changed_ids = window + .iter() + .filter_map(|e| match e { + GameEvent::ZoneChanged { object_id, .. } => Some(*object_id), + _ => None, + }) + .collect(); + } + Ok(PendingDiscardBatchOutcome::Completed) +} + +/// Finish the choice-specific bookkeeping that must precede a discard effect's +/// terminal marker, whether the selected cards settled synchronously or after +/// one or more replacement choices. +pub(crate) fn finalize_discard_choice_completion( + state: &mut GameState, + completion: &PendingDiscardBatchCompletion, + discard_frame: Option, + events: &[GameEvent], +) { + let PendingDiscardBatchCompletion::DiscardChoice { chosen } = completion else { + return; + }; + let discarded_to_graveyard: Vec = events + .iter() + .filter_map(|event| match event { + GameEvent::ZoneChanged { + object_id, + to: Zone::Graveyard, + .. + } => Some(*object_id), + _ => None, + }) + .collect(); + if !discarded_to_graveyard.is_empty() { + state.last_zone_changed_ids = discarded_to_graveyard.clone(); + publish_tracked_set_with_causes( + state, + discarded_to_graveyard + .into_iter() + .map(|id| (id, Some(ThisWayCause::Discarded))) + .collect(), + ); + } + if !chosen.is_empty() { + if let Some(frame) = state.active_ability_continuation_frame_mut() { + frame + .pending + .chain + .set_optional_effect_performed_recursive(true); + } + } + if let Some(frame_id) = discard_frame { + discard::hand_off_recruit_discard_result(state, frame_id); + } + if let Some(snapshot) = parent_referent_context_from_events(state, events) { + if let Some(frame) = state.active_ability_continuation_frame_mut() { + frame + .pending + .chain + .set_effect_context_object_recursive(snapshot); + } + } + state.last_effect_count = Some(chosen.len() as i32); +} + +/// Re-park a batch that paused again, carrying the resumed action's span into +/// the pre-pause window so the terminal count still covers the whole +/// instruction. +fn repark_discard_batch( + state: &mut GameState, + mut batch: Box, + events: &[GameEvent], + chooser: PlayerId, +) { + let mut window = std::mem::take(&mut batch.preceding_events); + window.extend_from_slice(events); + batch.preceding_events = window; + state.pending_discard_batch = Some(batch); + state.waiting_for = crate::game::replacement::replacement_choice_waiting_for(chooser, state); +} + +/// Stamp the terminal `Discarded` for the card whose replacement just resolved, +/// when the resume path could not emit one. +/// +/// CR 614.6: "If an event is replaced, it never happens. A modified event occurs +/// instead." A hand → graveyard `Moved` redirect (Rest in Peace class) therefore +/// still discarded the card per CR 701.9a, and a Madness redirect explicitly +/// does (CR 702.35a: "that player discards it, but exiles it instead of putting +/// it into their graveyard"). But that resume returns through terminal zone +/// delivery, which emits `Discarded` only for a provenance-framed discard — so +/// for every unframed discard the card leaves the hand and is never counted. +/// +/// The already-emitted guard is what makes the OTHER gate idempotent: a +/// `ReplacementEvent::Discard` pause (Library of Leng class) resumes through +/// `complete_discard_to_graveyard`, which does emit the event. This is the +/// direct analogue of the sacrifice batch's `!completion.sacrificed.contains(id)` +/// guard. +fn stamp_resumed_discard_if_unrecorded( + state: &mut GameState, + batch: &crate::types::game_state::PendingDiscardBatch, + events: &mut Vec, +) { + let paused = batch.paused_card; + let card = paused.object_id; + let already_recorded = events.iter().any(|event| { + matches!( + event, + GameEvent::Discarded { object_id, .. } if *object_id == card + ) + }); + if already_recorded { + return; + } + // CR 400.7: "An object that moves from one zone to another becomes a new + // object with no memory of, or relation to, its previous existence." The + // departure that settles this pause is the parked occurrence leaving the + // hand — not any hand departure that happens to reuse the `ObjectId`. A + // same-id round trip (the card returns to hand and leaves again) produces a + // later occurrence, and stamping this batch's `Discarded` from it would + // credit the discard to an object the pause never parked. + // + // The departing occurrence is already on the wire: every production record + // is built by `GameObject::snapshot_for_zone_change` BEFORE the incarnation + // bump, so `trigger_source_context.identity` is exactly the pre-move + // occurrence and its `expected_zone` is the zone it left. A record without + // that context is legacy/hand-built; it fails closed here rather than + // falling back to the id, which is the same policy the record's own doc + // states ("Callers must not reconstruct a source from a current object"). + let left_hand = events.iter().any(|event| { + matches!( + event, + GameEvent::ZoneChanged { + from: Some(crate::types::zones::Zone::Hand), + record, + .. + } if record + .trigger_source_context() + .is_some_and(|context| context.identity.reference == paused) + ) + }); + if !left_hand { + return; + } + crate::game::restrictions::record_discard(state, batch.player); + // CR 702.187b: the Mayhem marker is stamped only when the card actually + // landed in the graveyard — a redirect leaves it elsewhere, matching the + // un-paused path's own condition. + if state.objects.get(&card).map(|o| o.zone) == Some(crate::types::zones::Zone::Graveyard) { + crate::game::restrictions::record_card_discarded(state, card); + } + events.push(GameEvent::Discarded { + player_id: batch.player, + object_id: card, + source_id: Some(batch.source_id), + }); +} + /// Resolve an ability and follow its sub_ability chain using typed nested structs. /// No SVar lookup, no parse_ability(). The depth is bounded by the data structure. /// CR 608.2c: True when `condition` is a quantity comparison awaiting a @@ -10081,6 +10670,37 @@ fn resolve_chain_body( let initial_waiting_for = state.waiting_for.clone(); let mut paused = false; + // CR 608.2c: the zero-fill's reduction domain is the set of players the + // clause has actually applied to. A mid-fan-out pause leaves the tail + // unresolved, so filling them as zero would publish a contribution they + // have not had the chance to make. Narrow the domain to the players who + // COMPLETED before the pause — the pausing player is excluded too: they + // are sitting on a choice they have not answered, so a `Min` read taken + // mid-pause must not see them as a zero contributor. A seat that already + // holds an entry is unaffected either way — `fill_zero_contributors` is + // `or_insert(0)`, so it is PRESENCE in the table, not completion, that + // makes the fill a no-op for them. + // + // WHAT THIS DOES NOT FIX, measured on the tree this comment ships in: + // each resumed continuation leg REPLACES the table rather than extending + // it — `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 each leg publishes only + // its own entry. A four-seat fan-out pausing on seat 1 measures + // `[(0, 1)]` at the pause and `[(3, 1)]` once the continuation runs — the + // remaining seats chain into ONE leg, so even seat 2's publication is + // replaced before the fan-out ends, and `last_effect_amount` reads + // `Some(1)` where an accumulating table would give 4. That is + // PRE-EXISTING and + // not specific to an aggregate: `last_effect_amount` is derived from the + // same table (`.values().sum()`), so the `Sum` class loses the same + // counts. It is reachable here because the forced whole-hand discard + // branch can still pause on a replacement choice + // (`effects/discard.rs`, which documents its own related + // `EffectResolved` gap at that site). Repairing it means making the + // per-clause table accumulate across continuation legs, which is + // resume-machinery work well outside a draw-count change. + let mut applied_domain_end = matching_players.len(); // CR 608.2e: each clause's equalization minimum is fixed when that // clause begins; the snapshot is per `player_scope` link, captured // before fan-out (the board is now exactly the clause's pre-clause @@ -10122,6 +10742,54 @@ fn resolve_chain_body( if after_scope_needs_linked_exile { mark_exile_choice_tracks_by_source(state, ability.source_id); } + // CR 608.2f: this fan-out paused because THIS seat's discard + // batch is parked. The clause's remaining seats belong to that + // batch, not to the generic continuation queue, so the whole + // instruction publishes ONE per-player table instead of one + // table per resumed leg. Mirrors + // `start_player_scope_sacrifice_choices`, which likewise keeps + // `remaining_players` on its pending state and parks only the + // unscoped tail. + // + // The identity triple is checked BEFORE the hand-off and never + // inferred from payload shape: a batch parked by a different + // source, by a different seat, or one that a nested clause has + // already handed off, fails a conjunct and the driver falls + // through to the ordinary per-seat leg path below, unchanged. + let handed_to_discard_batch = + state.pending_discard_batch.as_ref().is_some_and(|batch| { + batch.source_id == scoped_template.source_id + && batch.player == *pid + && batch.fan_out.is_none() + }); + if handed_to_discard_batch { + if let Some(batch) = state.pending_discard_batch.as_mut() { + // Widen the batch's pre-pause window from this seat's + // own emissions to the whole clause's span: the earlier + // seats' discards are part of the same instruction. + // Copied, not drained — the pre-pause action still + // returns them to its caller. + batch.preceding_events = events[scoped_events_before..].to_vec(); + batch.fan_out = + Some(Box::new(crate::types::game_state::PendingDiscardFanOut { + scoped_template: Box::new(scoped_template.clone()), + outer: Box::new(ability.clone()), + original_controller: controller, + remaining_players: matching_players[i + 1..].to_vec(), + matching_players: matching_players.clone(), + after_scope_needs_linked_exile, + })); + } + // Only the unscoped tail goes to the generic continuation; + // the per-seat legs do not exist on this path. + if after_scope.is_some() { + append_to_pending_continuation(state, after_scope.clone()); + } + // Deliberately skips the clause postlude below: the batch + // owns that publication now, and running it here as well + // would publish a truncated table first. + return Ok(()); + } let remaining = &matching_players[i + 1..]; let mut tail = after_scope.clone(); // Build continuation chain for remaining players in APNAP order. @@ -10160,85 +10828,22 @@ fn resolve_chain_body( if tail.is_some() { append_to_pending_continuation(state, tail); } + // `i`, not `i + 1`: player `i` is the one who just paused, so + // they have NOT completed the clause and must not be filled as + // a zero contributor. + applied_domain_end = i; paused = true; break; } } - let scoped_events = &events[scoped_events_before..]; - let counts_by_player = previous_effect_counts_by_player_from_events( - &scoped_template.effect, - scoped_template.source_id, - scoped_events, - ); - // CR 608.2c: A completed scoped count producer that moved/discarded - // nothing still produced a zero for every player in this fan-out. Keep - // that provenance distinct from the absence of a count producer: the - // nonempty zero table takes precedence over an enclosing scalar event - // when the detached scoped "that many" consumer resolves. - let counts_by_player = counts_by_player.map(|mut counts_by_player| { - if counts_by_player.is_empty() { - counts_by_player.extend(matching_players.iter().copied().map(|player| (player, 0))); - } - counts_by_player - }); - if !install_previous_effect_counts_by_player(state, counts_by_player, false) { - if let Some(amount) = - previous_effect_amount_from_events(state, &scoped_template, scoped_events) - { - state.last_effect_amount = Some(amount); - // CR 120.10: stamp the resolution-local excess channel alongside the - // CR 120.6 total so a follow-up "if excess damage was dealt this way" - // condition reads overkill-beyond-lethal. - let excess = previous_effect_excess_amount_from_events( - state, - &scoped_template, - scoped_events, - ); - state.last_effect_excess_amount = excess; - } - } - let affected_with_causes = - if next_sub_needs_tracked_set(ability) || after_scope_needs_linked_exile { - affected_objects_with_causes( - state, - &scoped_template, - &scoped_template.effect, - scoped_events, - ) - } else { - Vec::new() - }; - let affected_ids: Vec = affected_with_causes.iter().map(|(id, _)| *id).collect(); - if after_scope_needs_linked_exile { - for id in &affected_ids { - if state - .objects - .get(id) - .is_some_and(|obj| obj.zone == crate::types::zones::Zone::Exile) - { - crate::game::exile_links::push_tracked_by_source(state, *id, ability.source_id); - } - } - } - // CR 608.2c: After a `player_scope: All` sacrifice clause completes, - // publish the full scoped event slice so downstream "if you sacrificed - // a permanent this way" / ZoneChangedThisWay gates see every player's - // sacrifice — not only the last iteration's overwrite of - // `last_zone_changed_ids`. - let mut ids: Vec = scoped_events - .iter() - .filter_map(|event| match event { - GameEvent::ZoneChanged { object_id, .. } - | GameEvent::PermanentSacrificed { object_id, .. } => Some(*object_id), - _ => None, - }) - .collect(); - ids.sort_unstable_by_key(|id| id.0); - ids.dedup(); - state.last_zone_changed_ids = ids; - if next_sub_needs_tracked_set(ability) { - publish_tracked_set_with_causes(state, affected_with_causes); - } + publish_player_scope_clause_results( + state, + ability, + &scoped_template, + &matching_players[..applied_domain_end], + after_scope_needs_linked_exile, + &events[scoped_events_before..], + ); if !paused { // CR 608.2e: this `player_scope` clause has completed. Clear its // frozen values before running any following instruction; if the @@ -11318,10 +11923,15 @@ fn resolve_chain_body( // many" chains (Tolarian Winds) stamp `last_effect_count`. let parent_events = &events[events_before..]; let counts_by_player = previous_effect_counts_by_player_from_events( - &ability.effect, + EffectKind::from(&ability.effect), ability.source_id, parent_events, ); + // No `fill_zero_contributors` here, unlike the `player_scope` loop: the + // reduction domain of a fan-out is the set of players the clause applied to, + // and this path has no such set to fill from — a bare effect applies to whom + // its own target names, and a player who emitted no event was never in the + // domain rather than being a zero contributor within it. let preserve_counts_for_current_consumer = ability.player_scope.is_none() && effect_consumes_event_context_amount(&ability.effect); if !install_previous_effect_counts_by_player( @@ -11332,8 +11942,11 @@ fn resolve_chain_body( if let Some(amount) = previous_effect_amount_from_events(state, ability, parent_events) { state.last_effect_amount = Some(amount); // CR 120.10: stamp the resolution-local excess channel alongside the - // CR 120.6 total so a follow-up "if excess damage was dealt this way" - // condition reads overkill-beyond-lethal. + // running total so a follow-up "if excess damage was dealt this way" + // condition reads overkill-beyond-lethal. CR 120.6 was cited for that + // total and is struck: it governs damage MARKED on a creature until + // the cleanup step, not the amount one clause leaves for a later + // clause in the same resolution — that carry-forward is CR 608.2c. let excess = previous_effect_excess_amount_from_events(state, ability, parent_events); state.last_effect_excess_amount = excess; } @@ -18781,29 +19394,782 @@ mod tests { ))); } - #[test] - fn previous_effect_amount_for_damage_ignores_counter_side_effects() { - let mut state = GameState::new_two_player(42); - let battle_id = create_object( - &mut state, - CardId(1), - PlayerId(1), - "Test Siege".to_string(), - Zone::Battlefield, + /// CR 608.2c: the PRODUCER half of the zero-contributor fix. A player the + /// clause applied to who emitted no event still discarded zero *this way* + /// and must hold an entry, or an aggregate reduces over a domain that omits + /// them. + /// + /// Board 8/7/3/**0**: P3's empty hand emits no discard event, so the + /// event-built table arrives as `{8,7,3}`. Discriminating on the axis that + /// matters — `Min` over the filled table is 0, over the unfilled one 3. + /// (`Sum` and `Max` are provably blind to the omission, which is why this + /// needs its own test rather than riding an existing one.) + #[test] + fn fill_zero_contributors_adds_the_absent_player_as_zero() { + let mut counts = HashMap::new(); + counts.insert(PlayerId(0), 8); + counts.insert(PlayerId(1), 7); + counts.insert(PlayerId(2), 3); + let seats = [PlayerId(0), PlayerId(1), PlayerId(2), PlayerId(3)]; + + let unfilled_min = counts.values().copied().min(); + let filled = fill_zero_contributors(counts, &seats); + + let mut rows: Vec<(u8, i32)> = filled.iter().map(|(p, n)| (p.0, *n)).collect(); + rows.sort(); + assert_eq!( + rows, + vec![(0, 8), (1, 7), (2, 3), (3, 0)], + "the absent player is present holding 0" ); - { - let battle = state.objects.get_mut(&battle_id).unwrap(); - battle.card_types.core_types.push(CoreType::Battle); - battle.defense = Some(5); - battle.base_defense = Some(5); - battle.counters.insert(CounterType::Defense, 5); + assert_eq!(unfilled_min, Some(3), "control: unfilled, Min answers 3"); + assert_eq!( + filled.values().copied().min(), + Some(0), + "filled, Min answers 0 — the whole point of the fix" + ); + } + + /// Contributors are never overwritten, and an already-complete table is + /// unchanged — so the fill cannot corrupt the common case it runs on every + /// time. + #[test] + fn fill_zero_contributors_preserves_existing_counts() { + let mut counts = HashMap::new(); + counts.insert(PlayerId(0), 5); + counts.insert(PlayerId(1), 2); + let seats = [PlayerId(0), PlayerId(1)]; + + let filled = fill_zero_contributors(counts, &seats); + + assert_eq!(filled.get(&PlayerId(0)).copied(), Some(5)); + assert_eq!(filled.get(&PlayerId(1)).copied(), Some(2)); + assert_eq!(filled.len(), 2, "no phantom entries added"); + } + + /// The previously-handled case still behaves identically: an entirely empty + /// table becomes an all-zero table, one entry per matching player. + #[test] + fn fill_zero_contributors_fills_an_entirely_empty_table() { + let seats = [PlayerId(0), PlayerId(1), PlayerId(2)]; + let filled = fill_zero_contributors(HashMap::new(), &seats); + let mut rows: Vec<(u8, i32)> = filled.iter().map(|(p, n)| (p.0, *n)).collect(); + rows.sort(); + assert_eq!(rows, vec![(0, 0), (1, 0), (2, 0)]); + } + + /// CR 608.2c: the PRODUCTION wire, not the helper. The three tests above call + /// `fill_zero_contributors` directly, so they stay green even if the driver + /// stops calling it — this one drives a real `player_scope` fan-out through + /// `resolve_ability_chain` and reads the table the driver actually published. + /// + /// Four seats, hands 1/1/1/**0**: the empty-handed seat emits no discard event + /// and is therefore absent from the event-derived table. It is still a player + /// the clause applied to, so the published reduction domain must carry it as a + /// zero rather than omit it. + #[test] + fn player_scope_fan_out_publishes_a_zero_for_the_empty_handed_seat() { + let mut state = GameState::new(FormatConfig::standard(), 4, 42); + for seat in 0..3u8 { + create_object( + &mut state, + CardId(10 + u64::from(seat)), + PlayerId(seat), + format!("P{seat} Card"), + Zone::Hand, + ); } + // PlayerId(3) is dealt no card: the zero contributor under test. - let sub = ResolvedAbility::new( - Effect::GainLife { - amount: QuantityExpr::Ref { + let mut ability = ResolvedAbility::new( + Effect::Discard { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::ScopedPlayer, + selection: crate::types::ability::CardSelectionMode::Chosen, + unless_filter: None, + filter: None, + }, + vec![], + ObjectId(100), + PlayerId(0), + ); + ability.player_scope = Some(PlayerFilter::All); + + let mut events = Vec::new(); + resolve_ability_chain(&mut state, &ability, &mut events, 0).unwrap(); + + let mut rows: Vec<(u8, i32)> = state + .last_effect_counts_by_player + .iter() + .map(|(p, n)| (p.0, *n)) + .collect(); + rows.sort(); + assert_eq!( + rows, + vec![(0, 1), (1, 1), (2, 1), (3, 0)], + "the driver must publish the empty-handed seat as a zero contributor, \ + not omit it from the reduction domain" + ); + } + + /// CR 608.2c: the zero-fill's domain on a PAUSED fan-out. Seat 1 holds two + /// cards facing a "discard a card" fan-out, so its iteration stops on a + /// `DiscardChoice` it has not answered. Seat 0 completed; seats 1..3 did not. + /// + /// The bound is `i`, not `i + 1`: publishing the pausing seat as a zero says + /// it contributed nothing, when in fact it has not yet been given the chance + /// to contribute — a `Min` read taken mid-pause would answer 0 off that. + /// + /// This pins the domain only. It deliberately does NOT assert that the table + /// survives the continuation: each resumed leg replaces it rather than + /// extending it, which is pre-existing and documented at the fill site. + #[test] + fn paused_fan_out_excludes_the_seat_that_has_not_answered_its_choice() { + let mut state = GameState::new(FormatConfig::standard(), 4, 42); + // Seat 0: exactly one card — a forced discard, no choice, completes. + // Seat 1: two cards — must choose, so the fan-out pauses here. + for (seat, cards) in [(0u8, 1u32), (1, 2), (2, 1), (3, 1)] { + for n in 0..cards { + create_object( + &mut state, + CardId(100 + u64::from(seat) * 10 + u64::from(n)), + PlayerId(seat), + format!("P{seat} Card {n}"), + Zone::Hand, + ); + } + } + + let mut ability = ResolvedAbility::new( + Effect::Discard { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::ScopedPlayer, + selection: crate::types::ability::CardSelectionMode::Chosen, + unless_filter: None, + filter: None, + }, + vec![], + ObjectId(100), + PlayerId(0), + ); + ability.player_scope = Some(PlayerFilter::All); + + let mut events = Vec::new(); + resolve_ability_chain(&mut state, &ability, &mut events, 0).unwrap(); + + assert!( + matches!( + state.waiting_for, + crate::types::game_state::WaitingFor::DiscardChoice { .. } + ), + "reach guard: the fan-out must actually be paused on seat 1's choice, \ + got {:?}", + state.waiting_for + ); + + let mut rows: Vec<(u8, i32)> = state + .last_effect_counts_by_player + .iter() + .map(|(p, n)| (p.0, *n)) + .collect(); + rows.sort(); + assert_eq!( + rows, + vec![(0, 1)], + "only the seat that COMPLETED before the pause belongs to the domain; \ + the paused seat has not had the chance to contribute and must not be \ + published as a zero" + ); + } + + // --------------------------------------------------------------------- + // The discard-batch carrier (CR 608.2c parked order + CR 614.6 replacement). + // --------------------------------------------------------------------- + + /// A `player_scope: All` "each player discards a card" clause template. + fn scoped_discard_one(source_id: ObjectId) -> ResolvedAbility { + let mut ability = ResolvedAbility::new( + Effect::Discard { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::ScopedPlayer, + selection: crate::types::ability::CardSelectionMode::Chosen, + unless_filter: None, + filter: None, + }, + vec![], + source_id, + PlayerId(0), + ); + ability.player_scope = Some(PlayerFilter::All); + ability + } + + fn deal_hand(state: &mut GameState, seat: u8, cards: u32) -> Vec { + (0..cards) + .map(|n| { + create_object( + state, + CardId(500 + u64::from(seat) * 10 + u64::from(n)), + PlayerId(seat), + format!("P{seat} Card {n}"), + Zone::Hand, + ) + }) + .collect() + } + + fn park_batch( + state: &mut GameState, + source_id: ObjectId, + player: PlayerId, + remaining: Vec, + fan_out: Option>, + ) { + state.pending_discard_batch = + Some(Box::new(crate::types::game_state::PendingDiscardBatch { + player, + cursor: DiscardBatchCursor::All { remaining }, + completion: crate::types::game_state::PendingDiscardBatchCompletion::Standard, + source_id, + effect_kind: EffectKind::Discard, + paused_card: crate::types::identifiers::ObjectIncarnationRef::of( + ObjectId(9_999_999), + 0, + ), + discard_frame: None, + fan_out, + preceding_events: Vec::new(), + })); + } + + fn fan_out_of( + source_id: ObjectId, + remaining_players: Vec, + matching_players: Vec, + ) -> Box { + let template = scoped_discard_one(source_id); + let mut scoped = template.clone(); + scoped.player_scope = None; + Box::new(crate::types::game_state::PendingDiscardFanOut { + scoped_template: Box::new(scoped), + outer: Box::new(template), + original_controller: PlayerId(0), + remaining_players, + matching_players, + after_scope_needs_linked_exile: false, + }) + } + + /// CR 608.2c: the terminal marker the pre-pause action could not emit, + /// because it returned from inside the batch loop. + /// + /// Without it the seat's count is underivable — + /// `previous_effect_counts_by_player_from_events` early-returns at its + /// `rposition`, so `install_previous_effect_counts_by_player` takes the arm + /// that CLEARS the table. The window spans the pause: one pre-pause discard + /// carried in `preceding_events` plus the two the drain still owes. + /// + /// REVERT PROBE (RUN, not reasoned): delete the + /// `events.push(GameEvent::EffectResolved { .. })` in + /// `drain_pending_discard_batch`. Observed first failure — "exactly one + /// terminal marker, matching the un-paused path's one per seat / left: 0 / + /// right: 1". The `last_effect_count` assertion below is downstream of that + /// one and never gets to run, so it is the marker count that discriminates. + #[test] + fn drained_discard_batch_emits_its_terminal_marker_and_counts_across_the_pause() { + let mut state = GameState::new_two_player(42); + let source = ObjectId(100); + let hand = deal_hand(&mut state, 0, 2); + let already_discarded = ObjectId(9_001); + + park_batch(&mut state, source, PlayerId(0), hand.clone(), None); + state + .pending_discard_batch + .as_mut() + .unwrap() + .preceding_events = vec![GameEvent::Discarded { + player_id: PlayerId(0), + object_id: already_discarded, + source_id: Some(source), + }]; + + let mut events = Vec::new(); + let outcome = drain_pending_discard_batch(&mut state, &mut events).unwrap(); + + assert!( + matches!(outcome, PendingDiscardBatchOutcome::Completed), + "the batch owed two cards and no replacement intervened" + ); + // Reach guard: the two owed cards really were discarded, so the count + // below cannot be a stale read from a run that did nothing. + assert_eq!( + hand.iter() + .filter(|id| state.objects[id].zone == Zone::Graveyard) + .count(), + 2, + "reach guard: the drain must finish the parked cursor" + ); + assert_eq!( + events + .iter() + .filter(|e| matches!( + e, + GameEvent::EffectResolved { kind: EffectKind::Discard, source_id: s, .. } + if *s == source + )) + .count(), + 1, + "exactly one terminal marker, matching the un-paused path's one per seat" + ); + assert_eq!( + state.last_effect_count, + Some(3), + "the published count spans the pause: 1 pre-pause discard + 2 owed" + ); + } + + /// Drive one `ObjectId` through hand → graveyard → hand → graveyard with the + /// production zone authority, returning the live occurrence pinned before + /// each hand departure together with that departure's real `ZoneChanged`. + /// + /// Both records are produced by `GameObject::snapshot_for_zone_change`, so + /// the identity under test is the one production writes, not a hand-built + /// stand-in. The batch itself is still parked directly because no card in + /// the corpus returns a card to hand mid-instruction — see the PR notes. + fn hand_departures_across_a_round_trip( + state: &mut GameState, + card: ObjectId, + ) -> [(crate::types::identifiers::ObjectIncarnationRef, GameEvent); 2] { + let mut departures = Vec::new(); + for to in [Zone::Graveyard, Zone::Hand, Zone::Graveyard] { + let before = + crate::types::identifiers::ObjectIncarnationRef::from_object(&state.objects[&card]); + let from_hand = state.objects[&card].zone == Zone::Hand; + let mut moved = Vec::new(); + crate::game::zones::move_to_zone(state, card, to, &mut moved); + if from_hand { + let event = moved + .into_iter() + .find(|event| { + matches!( + event, + GameEvent::ZoneChanged { object_id, from: Some(Zone::Hand), .. } + if *object_id == card + ) + }) + .expect("a hand departure emits its ZoneChanged"); + departures.push((before, event)); + } + } + let [first, second]: [_; 2] = departures + .try_into() + .unwrap_or_else(|_| panic!("the round trip makes exactly two hand departures")); + assert_ne!( + first.0.incarnation, second.0.incarnation, + "reach guard: the round trip must really advance the incarnation, \ + otherwise the two arms below are the same test twice" + ); + [first, second] + } + + /// CR 400.7: "An object that moves from one zone to another becomes a new + /// object with no memory of, or relation to, its previous existence." + /// + /// The parked batch pins the occurrence whose replacement paused. After a + /// same-`ObjectId` round trip, a LATER occurrence's hand departure must not + /// settle that pause — stamping it would credit the parked discard to an + /// object the batch never parked. The matched positive arm proves the pin + /// still accepts its own departure, so the negative arm is a discriminator + /// and not a blanket refusal to stamp. + /// + /// REVERT PROBE (RUN, not reasoned): restore the bare-id predicate in + /// `stamp_resumed_discard_if_unrecorded` — + /// `GameEvent::ZoneChanged { object_id, from: Some(Zone::Hand), .. } if + /// *object_id == card`. Observed failure — "a later incarnation's hand + /// departure must not settle this pause / left: 1 / right: 0". The positive + /// arm keeps passing under the revert, which is what makes the negative arm + /// the discriminating one. + #[test] + fn resumed_discard_stamp_rejects_a_later_incarnation_of_the_paused_card() { + let stamped_discards = |state: &mut GameState, + card: ObjectId, + pin: crate::types::identifiers::ObjectIncarnationRef, + departure: GameEvent| { + let source = ObjectId(100); + park_batch(state, source, PlayerId(0), Vec::new(), None); + state.pending_discard_batch.as_mut().unwrap().paused_card = pin; + let mut events = vec![departure]; + drain_pending_discard_batch(state, &mut events).unwrap(); + events + .iter() + .filter(|event| { + matches!(event, GameEvent::Discarded { object_id, .. } if *object_id == card) + }) + .count() + }; + + let mut state = GameState::new_two_player(42); + let card = create_object( + &mut state, + CardId(4_000), + PlayerId(0), + "Round Tripper".to_string(), + Zone::Hand, + ); + let [(first_pin, first_departure), (later_pin, later_departure)] = + hand_departures_across_a_round_trip(&mut state, card); + + // Negative arm: the pause parked the FIRST occurrence; the resume window + // carries only the LATER occurrence's departure. + assert_eq!( + stamped_discards(&mut state, card, first_pin, later_departure), + 0, + "a later incarnation's hand departure must not settle this pause" + ); + + // Positive arm: the same pin, offered its own departure, still stamps. + assert_eq!( + stamped_discards(&mut state, card, first_pin, first_departure.clone()), + 1, + "the parked occurrence's own departure must still stamp exactly one \ + Discarded, or the negative arm above proves nothing" + ); + + // The later pin is equally bound: it accepts its own departure and not + // the earlier one, so the predicate is an equality on the occurrence + // rather than an ordering test. + assert_eq!( + stamped_discards(&mut state, card, later_pin, first_departure), + 0, + "an earlier incarnation's departure must not settle a later pause" + ); + } + + /// CR 608.2c + CR 101.3: the drain's per-seat resumption boundary. + /// + /// `cost_payment_failed_flag` is per-iteration. Seat 1 is empty-handed, so + /// its mandatory discard fails (`discard.rs`'s `count == 0 && !up_to` arm) + /// and raises the flag; seat 2 then succeeds. Without the reset, seat 1's + /// failure leaks into seat 2's `IfCurrentScopeSucceeded` read — the same + /// leak the driver's own loop resets against for Refurbished Familiar and + /// Aclazotz, Deepest Betrayal. + /// + /// REVERT PROBE (RUN, not reasoned): delete + /// `state.cost_payment_failed_flag = false;` from the drain's fan-out loop. + /// Observed failure — "an earlier seat's mandatory failure must not leak + /// into a later seat". + #[test] + fn drained_fan_out_resets_the_cost_payment_failure_between_seats() { + let mut state = GameState::new(FormatConfig::standard(), 4, 42); + let source = ObjectId(100); + // Seat 1 empty (raises the flag); seat 2 holds one card, so its forced + // discard succeeds. The roster deliberately ends on the succeeding seat. + let seat2 = deal_hand(&mut state, 2, 1); + + park_batch(&mut state, source, PlayerId(0), Vec::new(), None); + state.pending_discard_batch.as_mut().unwrap().fan_out = Some(fan_out_of( + source, + vec![PlayerId(1), PlayerId(2)], + vec![PlayerId(0), PlayerId(1), PlayerId(2), PlayerId(3)], + )); + + let mut events = Vec::new(); + drain_pending_discard_batch(&mut state, &mut events).unwrap(); + + // Reach guards: both seats really ran. Without them the `false` below + // could hold vacuously on a roster that was never iterated. + assert_eq!( + state.objects[&seat2[0]].zone, + Zone::Graveyard, + "reach guard: the later seat's forced discard must have run" + ); + assert!( + state.players[1].hand.is_empty(), + "reach guard: the earlier seat must be the empty-handed one" + ); + assert!( + !state.cost_payment_failed_flag, + "an earlier seat's mandatory failure must not leak into a later seat" + ); + } + + /// CR 800.4a ("all objects (see rule 109) owned by that player leave the + /// game …") vs CR 800.4i ("the effect uses the last known information about + /// that player before they left the game"): a seat that leaves mid-pause is + /// dropped from the ITERATION roster and kept in the reduction DOMAIN. + /// + /// The asymmetry is the whole point of the test: the two lists look like + /// duplicates, so the natural "tidy-up" is to prune both. Latching the + /// domain is PARITY with the un-paused driver (which derives it once at + /// clause entry) rather than a rule; CR 800.4i is what keeps the departed + /// seat well-defined in it. + /// + /// WHAT THIS PINS, stated precisely because the honest scope is narrower + /// than the motivation: it pins the SHAPE of the two lists after an + /// elimination, and nothing downstream of them. The consequence that makes + /// the shape matter — a domain short one zero-contributor changes what a + /// `Min` over it answers (`fill_zero_contributors`; `Sum` and `Max` are + /// blind to zeros) — is NOT exercised here: no seat in this fixture holds a + /// hand, so the drain never runs. Treat that consequence as the reason the + /// pin exists, not as something this test measures. + /// + /// Lives here rather than beside the prune so it can reuse the fan-out + /// fixture; `elimination.rs` carries a pointer to it at the prune site. + /// + /// REVERT PROBES (both RUN, not reasoned): + /// * delete `fan_out.remaining_players.retain(..)` in `elimination.rs` + /// -> the roster assertion fails; + /// * add a matching `fan_out.matching_players.retain(..)` beside it + /// -> the domain assertion fails. + #[test] + fn eliminating_a_seat_prunes_the_paused_roster_but_not_its_reduction_domain() { + let mut state = GameState::new(FormatConfig::standard(), 4, 42); + let source = ObjectId(100); + let seats = vec![PlayerId(1), PlayerId(2), PlayerId(3)]; + + park_batch(&mut state, source, PlayerId(0), Vec::new(), None); + state.pending_discard_batch.as_mut().unwrap().fan_out = + Some(fan_out_of(source, seats.clone(), seats.clone())); + + let mut events = Vec::new(); + crate::game::elimination::eliminate_player(&mut state, PlayerId(2), &mut events); + + let fan_out = state + .pending_discard_batch + .as_ref() + .expect("the batch survives an unrelated seat leaving") + .fan_out + .as_ref() + .expect("so does its fan-out"); + assert_eq!( + fan_out.remaining_players, + vec![PlayerId(1), PlayerId(3)], + "CR 800.4a: a departed seat's objects leave the game, so it has no \ + hand left and iterating it can only be a no-op" + ); + assert_eq!( + fan_out.matching_players, seats, + "CR 800.4i: the reduction domain is latched at the pause and keeps \ + the departed seat, whose truthful contribution is zero" + ); + } + + /// CR 608.2f: BOUNDARY. A later seat that pauses on something which is NOT + /// a batch pause — here an interactive `WaitingFor::DiscardChoice` — hands + /// the remaining seats back to the generic continuation queue exactly as the + /// driver does, and leaves no stale batch live. + /// + /// That interactive route is the one this change deliberately does NOT + /// repair; this test pins that it is handed back cleanly rather than + /// corrupted. + /// + /// REVERT PROBE: delete the leg-rebuild loop in the drain's non-batch pause + /// fallback. The remaining seats are silently dropped and + /// `active_ability_continuation().is_some()` fails. + #[test] + fn drained_fan_out_returns_an_interactive_seat_to_the_continuation_path() { + let mut state = GameState::new(FormatConfig::standard(), 4, 42); + let source = ObjectId(100); + // Seat 1 holds two cards facing "discard a card": it must choose. + deal_hand(&mut state, 1, 2); + deal_hand(&mut state, 2, 1); + deal_hand(&mut state, 3, 1); + + park_batch(&mut state, source, PlayerId(0), Vec::new(), None); + state.pending_discard_batch.as_mut().unwrap().fan_out = Some(fan_out_of( + source, + vec![PlayerId(1), PlayerId(2), PlayerId(3)], + vec![PlayerId(0), PlayerId(1), PlayerId(2), PlayerId(3)], + )); + + let mut events = Vec::new(); + let outcome = drain_pending_discard_batch(&mut state, &mut events).unwrap(); + + assert!( + matches!(outcome, PendingDiscardBatchOutcome::PausedForReplacement), + "an unfinished clause must report a pause, not completion" + ); + assert!( + matches!(state.waiting_for, WaitingFor::DiscardChoice { .. }), + "reach guard: seat 1 must actually be sitting on its choice, got {:?}", + state.waiting_for + ); + assert!( + state.pending_discard_batch.is_none(), + "no stale batch may be left live once the clause left this path" + ); + assert!( + state.active_ability_continuation().is_some(), + "seats 2 and 3 must be returned to the generic continuation queue" + ); + } + + /// MULTI-AUTHORITY. The driver hand-off's identity triple must reject a + /// batch that is not the one this clause's seat just parked. + /// + /// The reachable hostile shape: the running clause's seat pauses on an + /// INTERACTIVE `DiscardChoice` (which parks no batch) while an unrelated + /// batch already sits in the single-slot carrier. Without the triple the + /// driver would hand this clause's roster to that stranger. + /// + /// REVERT PROBES (all three RUN, not reasoned), one per conjunct in the + /// driver's `handed_to_discard_batch` predicate. Each independently reddens + /// exactly one arm, and all three land on the SAME assertion — the + /// sentinel-roster one — with only the arm label differing: + /// (a) delete `batch.source_id == scoped_template.source_id` → observed + /// "foreign_source: a parked batch's roster must not be overwritten by + /// this clause / left: [PlayerId(1), PlayerId(2), PlayerId(3)] / + /// right: [PlayerId(3)]". + /// (b) delete `batch.player == *pid` → same assertion, "foreign_seat:". + /// (c) delete `batch.fan_out.is_none()` → same assertion, + /// "already_handed_off:". + #[test] + fn hand_off_identity_triple_rejects_a_foreign_batch() { + // Sentinel roster, distinguishable from the clause's real remainder + // [P1, P2, P3], so an unwanted hand-off is visible. + const SENTINEL: [PlayerId; 1] = [PlayerId(3)]; + + for arm in ["foreign_source", "foreign_seat", "already_handed_off"] { + let mut state = GameState::new(FormatConfig::standard(), 4, 42); + let clause_source = ObjectId(100); + // Seat 0 holds two cards facing "discard a card": it must choose, + // so the fan-out pauses WITHOUT parking a batch of its own. + deal_hand(&mut state, 0, 2); + deal_hand(&mut state, 1, 1); + deal_hand(&mut state, 2, 1); + deal_hand(&mut state, 3, 1); + + let (stale_source, stale_player, stale_fan_out) = match arm { + "foreign_source" => (ObjectId(999), PlayerId(0), None), + "foreign_seat" => (clause_source, PlayerId(1), None), + _ => ( + clause_source, + PlayerId(0), + Some(fan_out_of( + clause_source, + SENTINEL.to_vec(), + SENTINEL.to_vec(), + )), + ), + }; + park_batch( + &mut state, + stale_source, + stale_player, + Vec::new(), + stale_fan_out, + ); + + let mut events = Vec::new(); + resolve_ability_chain( + &mut state, + &scoped_discard_one(clause_source), + &mut events, + 0, + ) + .unwrap(); + + assert!( + matches!(state.waiting_for, WaitingFor::DiscardChoice { .. }), + "{arm}: reach guard — the clause must actually pause on seat 0's choice, \ + got {:?}", + state.waiting_for + ); + let batch = state + .pending_discard_batch + .as_ref() + .unwrap_or_else(|| panic!("{arm}: the stale batch must still be parked")); + assert_eq!( + batch.source_id, stale_source, + "{arm}: the stale batch's identity must be untouched" + ); + match &batch.fan_out { + None => assert_ne!( + arm, "already_handed_off", + "the already-handed-off arm must keep its own fan-out" + ), + Some(fan_out) => assert_eq!( + fan_out.remaining_players, SENTINEL, + "{arm}: a parked batch's roster must not be overwritten by this clause" + ), + } + assert!( + state.active_ability_continuation().is_some(), + "{arm}: the driver must fall through to the ordinary per-seat leg path" + ); + } + } + + /// Kind-keying `previous_effect_counts_by_player_from_events` is + /// behaviour-preserving: the producer set is exactly Discard / DiscardCard / + /// ChangeZoneAll, and nothing else opens a count window. + /// + /// REVERT PROBE (RUN, not reasoned): add `EffectKind::Draw` to the producer + /// arm of the opening `match kind`. Observed failure is NOT a test assertion + /// — it is the production `unreachable!("producer kind was selected above")` + /// in the inner match, because opening the outer gate without adding the + /// matching inner arm makes the two disagree. Still discriminating (the run + /// goes red on Draw's row and cannot go green), but a reader should expect a + /// panic from production rather than an `assert!` message. + #[test] + fn count_authority_producer_set_is_closed_over_effect_kind() { + let source = ObjectId(10); + for kind in [ + EffectKind::Discard, + EffectKind::DiscardCard, + EffectKind::ChangeZoneAll, + ] { + assert!( + previous_effect_counts_by_player_from_events( + kind, + source, + &[resolved_event(kind, source)], + ) + .is_some(), + "{kind:?} is a count producer" + ); + } + for kind in [ + EffectKind::Draw, + EffectKind::LoseLife, + EffectKind::DealDamage, + ] { + assert!( + previous_effect_counts_by_player_from_events( + kind, + source, + &[resolved_event(kind, source)], + ) + .is_none(), + "{kind:?} publishes no per-player table" + ); + } + } + + #[test] + fn previous_effect_amount_for_damage_ignores_counter_side_effects() { + let mut state = GameState::new_two_player(42); + let battle_id = create_object( + &mut state, + CardId(1), + PlayerId(1), + "Test Siege".to_string(), + Zone::Battlefield, + ); + { + let battle = state.objects.get_mut(&battle_id).unwrap(); + battle.card_types.core_types.push(CoreType::Battle); + battle.defense = Some(5); + battle.base_defense = Some(5); + battle.counters.insert(CounterType::Defense, 5); + } + + let sub = ResolvedAbility::new( + Effect::GainLife { + amount: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, player: TargetFilter::Controller, @@ -23187,8 +24553,20 @@ mod tests { assert_eq!(state.players[1].graveyard.len(), 1); } - #[test] - fn player_scope_discard_then_windfall_draws_greatest_discard_count() { + /// CR 608.2c + CR 701.9a + CR 121.2: the discard→draw "this way" + /// back-reference, exercised on ONE board across both aggregates. + /// + /// Libraries hold 6 (not 3, as the vacuous predecessor did) so neither + /// aggregate is library-capped: hands 3/1 make MAX 3, SUM 4, MIN 1 and the + /// per-player reading 3/1 four mutually distinguishable outcomes. The + /// predecessor seeded 3-card libraries, so MAX 3 and SUM 4 both capped at 3 + /// and its assertions held under either aggregate — which is why #7277 + /// shipped with the bug. + /// + /// Returns `(hands, graveyards)` per seat. + fn run_player_scope_discard_then_draw( + aggregate: AggregateFunction, + ) -> (Vec, Vec) { let mut state = GameState::new_two_player(42); for i in 0..3 { create_object( @@ -23198,6 +24576,8 @@ mod tests { format!("P0 Hand {i}"), Zone::Hand, ); + } + for i in 0..6 { create_object( &mut state, CardId(60 + i), @@ -23243,6 +24623,7 @@ mod tests { count: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate, }, }, target: TargetFilter::Controller, @@ -23257,10 +24638,45 @@ mod tests { let mut events = Vec::new(); resolve_ability_chain(&mut state, &ability, &mut events, 0).unwrap(); - assert_eq!(state.players[0].hand.len(), 3); - assert_eq!(state.players[1].hand.len(), 3); - assert_eq!(state.players[0].graveyard.len(), 3); - assert_eq!(state.players[1].graveyard.len(), 1); + ( + vec![state.players[0].hand.len(), state.players[1].hand.len()], + vec![ + state.players[0].graveyard.len(), + state.players[1].graveyard.len(), + ], + ) + } + + /// V6 discriminator — CR 608.2c + CR 608.2i: `Max` reads the GREATEST single + /// player's discard (3), not the cross-player total. Flip this test's + /// `aggregate` to `Sum` and the hands become `[4, 4]` ⇒ FAILS. That field + /// swap on an otherwise byte-identical board IS the discrimination evidence; + /// neither member compiles at BASE, where the field does not exist. + #[test] + fn player_scope_discard_then_draw_greatest_uses_max_aggregate() { + let (hands, graveyards) = run_player_scope_discard_then_draw(AggregateFunction::Max); + assert_eq!( + hands, + vec![3, 3], + "Max must draw the greatest single player's discard (3), not the sum (4)" + ); + // CR 701.9a reach guard: the discard step really ran and moved 3 and 1 + // cards to the graveyards, so this cannot pass on an unresolved chain. + assert_eq!(graveyards, vec![3, 1], "both players must have discarded"); + } + + /// V6 same-board control — the `Sum` default keeps reading the cross-player + /// total (4) on the identical board, so the Max assertion above is pinned by + /// a measured contrast rather than by a single reading. + #[test] + fn player_scope_discard_then_draw_total_uses_sum_aggregate() { + let (hands, graveyards) = run_player_scope_discard_then_draw(AggregateFunction::Sum); + assert_eq!( + hands, + vec![4, 4], + "Sum must draw the cross-player total (3 + 1), the pre-change behaviour" + ); + assert_eq!(graveyards, vec![3, 1], "both players must have discarded"); } /// CR 608.2c + CR 118.12 + CR 701.9: Read the Runes — draw X, then for @@ -23754,6 +25170,7 @@ mod tests { count: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, target: TargetFilter::Controller, @@ -30310,14 +31727,17 @@ mod tests { }, ]; - let counts = - previous_effect_counts_by_player_from_events(&discard_count_effect(), source, &events) - .expect("the exact discard terminal marker is present"); + let counts = previous_effect_counts_by_player_from_events( + EffectKind::from(&discard_count_effect()), + source, + &events, + ) + .expect("the exact discard terminal marker is present"); assert_eq!(counts, HashMap::from([(PlayerId(0), 1)])); assert!( previous_effect_counts_by_player_from_events( - &discard_count_effect(), + EffectKind::from(&discard_count_effect()), source, &[resolved_event(EffectKind::ChangeZoneAll, source)], ) @@ -30326,7 +31746,7 @@ mod tests { ); assert!( previous_effect_counts_by_player_from_events( - &discard_count_effect(), + EffectKind::from(&discard_count_effect()), source, &[resolved_event(EffectKind::Discard, other_source)], ) @@ -30335,7 +31755,7 @@ mod tests { ); assert!( previous_effect_counts_by_player_from_events( - &discard_count_effect(), + EffectKind::from(&discard_count_effect()), source, &events[..2], ) @@ -30357,7 +31777,11 @@ mod tests { zone_changed_event(ObjectId(2), PlayerId(1)), ]; assert_eq!( - previous_effect_counts_by_player_from_events(&effect, source, &before_then_after), + previous_effect_counts_by_player_from_events( + EffectKind::from(&effect), + source, + &before_then_after + ), Some(HashMap::from([(PlayerId(0), 1)])), "zone changes after the final marker belong to later work" ); @@ -30369,14 +31793,18 @@ mod tests { resolved_event(EffectKind::ChangeZoneAll, source), ]; assert_eq!( - previous_effect_counts_by_player_from_events(&effect, source, &two_same_source_moves), + previous_effect_counts_by_player_from_events( + EffectKind::from(&effect), + source, + &two_same_source_moves + ), Some(HashMap::from([(PlayerId(0), 1), (PlayerId(1), 1)])), "the final same-source marker aggregates the completed scoped moves" ); assert_eq!( previous_effect_counts_by_player_from_events( - &effect, + EffectKind::from(&effect), source, &[resolved_event(EffectKind::ChangeZoneAll, source)], ), @@ -30932,6 +32360,7 @@ mod tests { count: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, target: TargetFilter::Controller, @@ -31249,6 +32678,7 @@ mod tests { amount: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, target: None, @@ -31264,6 +32694,7 @@ mod tests { count: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, target: TargetFilter::Controller, @@ -31340,6 +32771,7 @@ mod tests { count: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, target: TargetFilter::SelfRef, diff --git a/crates/engine/src/game/elimination.rs b/crates/engine/src/game/elimination.rs index 1acfcb15b7..5ab2273f8c 100644 --- a/crates/engine/src/game/elimination.rs +++ b/crates/engine/src/game/elimination.rs @@ -913,6 +913,37 @@ fn do_eliminate( } } } + // CR 800.4a: "all objects … owned by that player leave the game", so a + // departed seat has no hand left to discard and iterating it can only be a + // no-op. Drop it from the discard fan-out's not-yet-prompted roster — the + // same treatment the scoped-library-search roster above already gets. + // + // `matching_players` is deliberately NOT pruned, and the reason is PARITY + // rather than a rule: the un-paused driver computes its reduction domain + // once at clause entry and never re-derives it, so a paused clause that + // pruned would answer differently from an identical unpaused one — which is + // precisely the divergence this repair exists to remove. CR 800.4i is what + // makes the retained seat well-defined: "the effect uses the last known + // information about that player before they left the game." The seat's + // truthful contribution is zero, and dropping it would silently change a + // `Min` answer. + // + // (Deliberately NOT cited: CR 608.2f, which an earlier revision leaned on. + // Read in full it is about simultaneity and APNAP ORDER — it latches no + // domain, and both its examples are about ordering. Same class of stretch as + // the CR 608.2b citation removed from `discard.rs`.) + // + // PINNED BY `effects/mod.rs`'s + // `eliminating_a_seat_prunes_the_paused_roster_but_not_its_reduction_domain`, + // which lives there to reuse the fan-out fixture. It asserts BOTH halves, + // so pruning the second list too is a red test rather than a silent change. + if let Some(fan_out) = state + .pending_discard_batch + .as_mut() + .and_then(|batch| batch.fan_out.as_mut()) + { + fan_out.remaining_players.retain(|seat| *seat != player); + } if let Some(crate::types::game_state::PendingBatchDeliveries { completion: Some(crate::types::game_state::BatchCompletion::LibrarySearchDeliverySettled { diff --git a/crates/engine/src/game/engine.rs b/crates/engine/src/game/engine.rs index dbd76fead8..612ae90237 100644 --- a/crates/engine/src/game/engine.rs +++ b/crates/engine/src/game/engine.rs @@ -19401,9 +19401,35 @@ mod stage2_injector_tests { // #7496's parked Cipher frame extends the exhaustive resume dispatch above // all three producers. It adds six lines without assigning an optional-effect // prompt, so the measured production producers move uniformly by `+6`. - "game/effects/mod.rs:7267".to_string(), - "game/effects/mod.rs:7344".to_string(), - "game/effects/mod.rs:10624".to_string(), + // Incarnation-pin round and the port across main, adjudicated together + // because the port is what moved the pre-port coordinates. + // + // The port's conflict resolution kept BOTH sides of this list: main's + // `:7267/:7344/:10624` and the branch's `:11124`, giving FOUR + // `effects/mod.rs` entries. There are only THREE producers in that file, + // so the union could not match a census computed from source, and it + // contradicted the `(5, 8, 28)` partition assert directly above. A union + // is the wrong merge for a list whose LENGTH is asserted: these entries + // are not additive facts, they are one coordinate per producer. + // + // Re-measured in the ported tree BY DIGEST, not by arithmetic: each + // producer's 9-line block was hashed at `upstream/main` + // (`f9098299`/`96338f0e`/`bc850c67`) and each digest is found at exactly + // ONE coordinate here. `:7267/:7344/:10624` => `:7325/:7402/:11131`. + // + // The shift is NON-UNIFORM (`+58/+58/+507`): this branch's insertions are + // not all above the first producer, and the third sits below every + // discard-carrier and test addition. `bc850c67` is byte-identical to its + // value before the port — the same digest that pinned this producer at + // `:11103` and `:11124` earlier in this log — which is the evidence that + // it MOVED rather than being replaced. + // #7494 finish: `:11131 => :11229`, +98. The ordered-discard + // resume/finalization helpers are above this existing producer; + // they do not mint an optional-effect prompt. The census above + // still finds exactly the same five production producers. + "game/effects/mod.rs:7325".to_string(), + "game/effects/mod.rs:7402".to_string(), + "game/effects/mod.rs:11229".to_string(), // UNMOVED across the rebase, and that is itself evidence the SET did not // move: a census that had gained or lost a producer would not leave this // entry both byte-identical AND at the same coordinate. diff --git a/crates/engine/src/game/engine_payment_choices.rs b/crates/engine/src/game/engine_payment_choices.rs index d0ad8a2df8..c6445b35ee 100644 --- a/crates/engine/src/game/engine_payment_choices.rs +++ b/crates/engine/src/game/engine_payment_choices.rs @@ -937,6 +937,15 @@ pub(super) fn handle_unless_payment( crate::game::effects::discard::RandomDiscardOutcome::NeedsReplacementChoice { remaining_eligible, remaining_count, + // Effect-layer field: the parked EFFECT batch stamps + // the paused card's terminal `Discarded`. A cost + // payment publishes no such ledger, so this caller + // has nothing to do with it. + paused_card: _, + // Likewise effect-layer: `discard_at_random` already + // set `waiting_for` from this seat, and this caller + // never re-parks, so it has no prompt to keep in step. + chooser: _, } => { state.pending_cost_move_resume = Some(PendingCostMoveResume::RandomDiscardUnlessPayment(Box::new( @@ -2115,6 +2124,9 @@ pub(super) fn resume_random_discard_unless_payment( crate::game::effects::discard::RandomDiscardOutcome::NeedsReplacementChoice { remaining_eligible, remaining_count, + // Effect-layer fields — see the sibling site above. + paused_card: _, + chooser: _, } => { state.pending_cost_move_resume = Some(PendingCostMoveResume::RandomDiscardUnlessPayment(Box::new( diff --git a/crates/engine/src/game/engine_replacement.rs b/crates/engine/src/game/engine_replacement.rs index d665ae1897..b86c6b2309 100644 --- a/crates/engine/src/game/engine_replacement.rs +++ b/crates/engine/src/game/engine_replacement.rs @@ -1036,6 +1036,40 @@ pub(super) fn handle_replacement_choice( } } + // CR 608.2c: a discard instruction parked mid-batch by a + // replacement-application choice finishes what it still owes BEFORE + // any parked continuation runs. The resolving effect follows its + // instructions in written order, so later instructions cannot resume + // until this action has settled and published its terminal result. + if matches!(waiting_for, WaitingFor::Priority { .. }) + && state.pending_discard_batch.is_some() + { + match effects::drain_pending_discard_batch(state, events) + .map_err(|error| EngineError::InvalidAction(error.to_string()))? + { + effects::PendingDiscardBatchOutcome::Idle => {} + effects::PendingDiscardBatchOutcome::PausedForReplacement => { + waiting_for = state.waiting_for.clone(); + super::engine_resolution_choices::defer_observer_triggers_for_paused_choice( + state, + events, + replacement_action_event_start, + ); + } + effects::PendingDiscardBatchOutcome::Completed => { + effects::drain_pending_continuation(state, events); + if !matches!(state.waiting_for, WaitingFor::Priority { .. }) { + waiting_for = state.waiting_for.clone(); + super::engine_resolution_choices::defer_observer_triggers_for_paused_choice( + state, + events, + replacement_action_event_start, + ); + } + } + } + } + if matches!(waiting_for, WaitingFor::Priority { .. }) && (state.active_ability_continuation().is_some() || state.active_change_zone_frame().is_some()) @@ -7683,4 +7717,144 @@ mod tests { "half (iii)'s own positive control: with no dispatch live the same direct call retires it" ); } + + /// CR 701.9a vs CR 701.21a: the two adjacent post-replacement drains in + /// `handle_choose_replacement` publish their completion DIFFERENTLY, and the + /// asymmetry is deliberate. The sacrifice drain stamps + /// `ThisWayCause::Sacrificed`; the discard drain immediately below stamps + /// nothing, because the UN-paused discard path does not stamp either + /// (`grep stamp_active_player_action_completion effects/discard.rs` -> 0), so + /// a stamping resume would give a paused discard a provenance its own + /// un-paused twin never has. + /// + /// Nothing else pins this. A reader "fixing the inconsistency" between two + /// blocks twenty-five lines apart would silently change `Discarded`-this-way + /// provenance for the whole class. + /// + /// A SOURCE census rather than a behavioural assertion, and that is measured: + /// `stamp_active_player_action_completion` early-returns unless an active + /// ability continuation frame holds a `CompletePlayerAction` chain, so in a + /// drain unit test -- which has neither -- adding the call would change no + /// observable state and a behavioural assertion would itself be vacuous. + /// + /// THE WINDOWS ARE BRACE-BALANCED, NOT END-ANCHORED, and that is the whole + /// difficulty of this instrument. An earlier revision ended each slice at a + /// guessed marker; the marker for the second window happened to sit inside + /// the arm, so the "guarded" region collapsed to **36 characters of a + /// five-line arm** and the named revert probe only flipped if a stamp landed + /// as the arm's very first statement. A census whose window is wrong reports + /// a zero that means nothing, and it reports it silently. + /// + /// Three defences, because a negative result needs all of them, plus one + /// CLOSURE that is deliberately not counted among them: + /// 0. the text scanned is the CODE half only, via the shared + /// `source_census` authority. MEASURED INERT on today's tree: raw and + /// stripped text are byte-identical for every quantity this test reads + /// (both anchors and the needle at 1, sacrifice window 716 chars, + /// discard window 281, `drain_pending_continuation` present in both — + /// those two char counts are a SNAPSHOT and will rot on any edit to + /// either arm body; the durable claim is the raw/stripped IDENTITY, not + /// the numbers), + /// which is exactly what `source_census.rs` predicts of any census that + /// scans the real tree. It discriminates nothing here and is listed + /// apart from 1-3 for that reason: it closes a shape not currently + /// present rather than catching one that is; + /// 1. each anchor must match EXACTLY ONCE in the file, so a deleted + /// production arm cannot let the scan retarget some other text (this + /// doc comment deliberately never spells an anchor literally); + /// 2. each window is closed by brace balance, so it always spans the whole + /// arm body; + /// 3. each window asserts its OWN non-degeneracy. The positive control on + /// the sacrifice arm proves the file and that anchor are real, but it + /// says nothing about the extent of the DISCARD window -- and the + /// discard window is the one whose zero carries the claim. + /// + /// REVERT PROBES: + /// * add a stamp call ANYWHERE in the discard arm -- first statement, last + /// statement, or nested inside its `if` -- and half (b) fails. Only the + /// brace-balanced window makes all three positions equivalent. + /// * delete the sacrifice arm's existing stamp -> half (a) fails. + /// * delete either arm outright -> the exactly-once assertion fails, rather + /// than the scan silently sliding onto other text. + #[test] + fn the_resumed_discard_drain_does_not_stamp_a_completion_while_its_sacrifice_sibling_does() { + /// The arm body that follows `anchor`, delimited by brace balance. + fn arm_body<'a>(source: &'a str, anchor: &str) -> &'a str { + assert_eq!( + source.matches(anchor).count(), + 1, + "anchor {anchor:?} must identify exactly one site; a second \ + occurrence lets a deleted arm retarget the scan silently" + ); + let after = source.find(anchor).expect("anchor present") + anchor.len(); + let open = after + + source[after..] + .find('{') + .expect("the arm opens a block after its anchor"); + let mut depth = 0usize; + for (offset, ch) in source[open..].char_indices() { + match ch { + '{' => depth += 1, + '}' => { + depth -= 1; + if depth == 0 { + return &source[open..=open + offset]; + } + } + _ => {} + } + } + panic!("unbalanced braces after {anchor:?}"); + } + + // Routed through the shared comment authority rather than scanned raw, + // and this census is exactly the case that module exists for: half (b) + // is a NEGATIVE, so a deleted stamp whose spelling survived in a + // trailing `//` inside the discard arm would HOLD the zero and hide the + // regression, while a comment merely naming the needle would flip it red + // on a pure prose edit. + // + // SCOPE, because "stripping comments" would overclaim: `code_lines` + // removes whole-line and trailing `//` comments and a LEADING block + // comment. The interior lines of a multi-line `/* … */` survive and are + // still scanned — `source_census.rs` says so in its own doc. So a block + // comment inside the discard arm naming the needle would still flip + // half (b) red. That residue is fail-CLOSED (spurious red, never a + // missed site), which is the direction a census may fail in. + let source = &crate::source_census::code_lines(include_str!("engine_replacement.rs")); + let needle = concat!("stamp_active_player_action_", "completion("); + let sacrifice_anchor = concat!("PendingPlayerScopeSacrifice", "Outcome::Completed {"); + let discard_anchor = concat!("PendingDiscardBatch", "Outcome::Completed =>"); + + let sacrifice = arm_body(source, sacrifice_anchor); + let discard = arm_body(source, discard_anchor); + + // (0) Non-degeneracy, asserted per window. Both arms run a multi-line + // body ending in a `drain_pending_continuation` call guarded by a + // `waiting_for` re-check, so a window that cannot see that call has not + // spanned its arm and any verdict drawn from it is worthless. + for (label, window) in [("sacrifice", sacrifice), ("discard", discard)] { + assert!( + window.contains("drain_pending_continuation"), + "{label} window must span its whole arm body; it stops before the \ + arm's last statement, so a scan of it proves nothing (got {} chars)", + window.len() + ); + } + + // (a) Positive control: proves the instrument finds a stamp where one + // exists. Necessary but NOT sufficient for (b) -- different window. + assert!( + sacrifice.contains(needle), + "(a) positive control: the sacrifice arm must stamp, or this scan is \ + reading the wrong region and (b)'s zero would mean nothing" + ); + + // (b) The claim. + assert!( + !discard.contains(needle), + "(b) the resumed discard must publish exactly what the un-paused \ + discard publishes -- no CompletePlayerAction stamp" + ); + } } diff --git a/crates/engine/src/game/engine_resolution_choices.rs b/crates/engine/src/game/engine_resolution_choices.rs index af4909b401..aa677b3b58 100644 --- a/crates/engine/src/game/engine_resolution_choices.rs +++ b/crates/engine/src/game/engine_resolution_choices.rs @@ -5,7 +5,7 @@ use rand::seq::SliceRandom; use crate::types::ability::{ AbilityCost, ChoiceType, ChosenAttribute, DigRestOrder, Effect, EffectKind, GuessOutcome, - LibraryPosition, QuantityExpr, QuantityRef, ResolvedAbility, TargetRef, ThisWayCause, + LibraryPosition, QuantityExpr, QuantityRef, ResolvedAbility, TargetRef, }; use crate::types::actions::{GameAction, LearnOption, OutsideGameSelection}; use crate::types::events::GameEvent; @@ -631,6 +631,24 @@ fn batch_or_drain_observer_triggers( } } +/// CR 603.2 + CR 603.3b: Preserve triggers from events that occurred while a +/// resolution choice paused; they trigger now and wait for the next priority +/// window's APNAP placement rather than being lost with the action's event slice. +pub(crate) fn defer_observer_triggers_for_paused_choice( + state: &mut GameState, + events: &[GameEvent], + event_start: usize, +) { + let trigger_events: Vec = events[event_start..] + .iter() + .filter(|event| !matches!(event, GameEvent::PhaseChanged { .. })) + .cloned() + .collect(); + if !trigger_events.is_empty() { + super::triggers::collect_triggers_into_deferred(state, &trigger_events); + } +} + /// CR 603.2 + CR 603.3b + CR 701.23: after a search tutor's put/shuffle /// continuation drains, collect ETB/dies/discards observers before this /// `SelectCards` action reaches its priority checkpoint. The ordinary @@ -4674,6 +4692,15 @@ pub(super) fn handle_resolution_choice( } } + // CR 608.2d: A resolving player can't choose one eligible card + // more than once to satisfy a multi-card discard selection. + let unique_chosen: HashSet = chosen.iter().copied().collect(); + if unique_chosen.len() != chosen.len() { + return Err(EngineError::InvalidAction( + "Selected cards must be distinct".to_string(), + )); + } + let current_hand: std::collections::HashSet = state .players .iter() @@ -4694,8 +4721,14 @@ pub(super) fn handle_resolution_choice( } } + let chosen_refs = chosen + .iter() + .filter_map(|id| state.objects.get(id)) + .map(crate::types::identifiers::ObjectIncarnationRef::from_object) + .collect::>(); + let events_before_effect = events.len(); - for &card_id in &chosen { + for (index, &card_id) in chosen.iter().enumerate() { if let effects::discard::DiscardOutcome::NeedsReplacementChoice(choice_player) = effects::discard::discard_caused_by_effect_with_source_and_frame( state, @@ -4708,100 +4741,43 @@ pub(super) fn handle_resolution_choice( { state.waiting_for = super::replacement::replacement_choice_waiting_for(choice_player, state); + state.pending_discard_batch = Some(Box::new( + crate::types::game_state::PendingDiscardBatch { + player, + cursor: crate::types::game_state::DiscardBatchCursor::Ordered { + remaining: chosen_refs[index + 1..].to_vec(), + }, + completion: + crate::types::game_state::PendingDiscardBatchCompletion::DiscardChoice { + chosen: chosen_refs, + }, + source_id, + effect_kind, + paused_card: crate::types::identifiers::ObjectIncarnationRef::of( + card_id, + state.objects[&card_id].incarnation, + ), + discard_frame, + fan_out: None, + preceding_events: events[events_before_effect..].to_vec(), + }, + )); + defer_observer_triggers_for_paused_choice(state, events, events_before_effect); return Ok(action_result_outcome(events, state.waiting_for.clone())); } } let events_after_move = events.len(); - // CR 608.2e + CR 608.2c: APNAP discard steps accumulate into one - // tracked set. The discard handler is the single authority for - // recording the cards it moved — `discard_as_cost_with_source` - // runs outside `resolve_effect`, so its non-interactive sibling's - // `next_sub_needs_tracked_set` publish never fires for it. Publish - // the cards that reached the graveyard here; `chain_tracked_set_id` - // is preserved across the per-opponent continuation pause, so each - // opponent's publish extends the same set and the "draw a card for - // each card discarded this way" tail reads the union. - // CR 701.9c: only graveyard-bound cards count — a replacement - // redirect (Madness) to another zone is excluded by the filter. - let discarded_to_graveyard: Vec = events[events_before_effect..] - .iter() - .filter_map(|ev| match ev { - GameEvent::ZoneChanged { - object_id, - to: Zone::Graveyard, - .. - } => Some(*object_id), - _ => None, - }) - .collect(); - if !discarded_to_graveyard.is_empty() { - // CR 608.2c: A `ZoneChangedThisWay` reflexive gate ("When you - // discard a card this way, …" — Talion's Messenger, The Ancient - // One) reads `last_zone_changed_ids`. The synchronous resolve path - // populates that ledger from the discard's `ZoneChanged` events - // (`effects/mod.rs`), but a discard that paused for an interactive - // `DiscardChoice` (hand > 1) moves the chosen card HERE, after the - // parent effect already returned. Re-publish the just-moved cards - // into the ledger so the deferred gate, re-evaluated when the - // stashed continuation drains, sees the discarded objects. - state.last_zone_changed_ids = discarded_to_graveyard.clone(); - // CR 701.9a + CR 608.2c: stamp these members with the producer - // action `Discarded` so a `caused_by: Some(Discarded)` "discarded - // this way" consumer counts them while a `caused_by: None` - // consumer still reads the whole id-only set. The cause is the - // action, independent of final zone (CR 614.6). - let with_causes = discarded_to_graveyard - .into_iter() - .map(|id| (id, Some(ThisWayCause::Discarded))) - .collect(); - effects::publish_tracked_set_with_causes(state, with_causes); - } - - // CR 608.2c: "discard a card. If you do, [effect]" — the IfYouDo - // sub_ability condition evaluates against optional_effect_performed. - // Set it on the stashed continuation before draining so the gate - // evaluates true when at least one card was actually discarded. - // Mirrors the recursive AutoMayChoice::Accept path in effects/mod.rs. - if !chosen.is_empty() { - if let Some(frame) = state.active_ability_continuation_frame_mut() { - frame - .pending - .chain - .set_optional_effect_performed_recursive(true); - } - } - - // CR 701.9a + CR 608.2c: A Recruit discard that paused for card - // selection now has its terminal LKI result in the operation-owned - // frame. Stamp that result only onto the deferred direct child before - // the continuation drains; the ordinary parent→child hand-off clears - // it again for grandchildren. - if let Some(frame_id) = discard_frame { - effects::discard::hand_off_recruit_discard_result(state, frame_id); - } - - // CR 608.2c + CR 400.7j: A reflexive sub deferred across this - // interactive discard may name the discarded card anaphorically — - // "When you discard a card this way, target player mills cards equal - // to ITS mana value" (The Ancient One). The synchronous resolve path - // captures that referent via `parent_referent_context_from_events` - // (`effects/mod.rs`); the interactive path moves the card here, after - // the parent returned, so capture it now and stamp it onto the stashed - // continuation. The discarded card is in the public graveyard, so its - // characteristics are read live. Mirrors the `EffectZoneChoice` path. - if let Some(snapshot) = - effects::parent_referent_context_from_events(state, &events[events_before_effect..]) - { - if let Some(frame) = state.active_ability_continuation_frame_mut() { - frame - .pending - .chain - .set_effect_context_object_recursive(snapshot); - } - } - - state.last_effect_count = Some(chosen.len() as i32); + let completion = + crate::types::game_state::PendingDiscardBatchCompletion::DiscardChoice { + chosen: chosen_refs, + }; + effects::finalize_discard_choice_completion( + state, + &completion, + discard_frame, + &events[events_before_effect..], + ); events.push(GameEvent::EffectResolved { kind: effect_kind, source_id, diff --git a/crates/engine/src/game/quantity.rs b/crates/engine/src/game/quantity.rs index cf104fddca..8b6153f2e9 100644 --- a/crates/engine/src/game/quantity.rs +++ b/crates/engine/src/game/quantity.rs @@ -3982,23 +3982,78 @@ fn resolve_ref( } count } - // CR 608.2c: Numeric result from the preceding effect in a sub_ability chain. - // The resolver stamps this from the parent effect's semantic event class. - // - // CR 120.6 / CR 120.10: `channel` picks WHICH tally the preceding effect - // left behind. Both are stamped by the damage effects and cleared at - // depth-0, so the two channels are read from the same resolution scope — - // this arm only chooses between them. Mirrors the condition peer - // `AbilityCondition::PreviousEffectAmount`, which already reads both. - QuantityRef::PreviousEffectAmount { channel } => match channel { - // CR 120.6: the total amount dealt/lost/removed. - DamageChannel::Total => state.last_effect_amount.unwrap_or(0), - // CR 120.10: only the damage dealt BEYOND lethal — "the amount of - // excess damage dealt to that creature this way" (Goblin - // Negotiation, Hell to Pay, Lacerate Flesh), "that excess damage" - // (Contest of Claws). 0 when the preceding effect dealt no excess. - DamageChannel::Excess => state.last_effect_excess_amount.unwrap_or(0), - }, + // CR 608.2c + CR 608.2i: a "this way" look-back at the numeric result of + // the preceding instruction in this resolution. `channel` selects WHICH + // tally that instruction left behind; `aggregate` selects how the + // per-player table is reduced to the one number this reference reads. + QuantityRef::PreviousEffectAmount { channel, aggregate } => { + // CR 608.2h + CR 608.2e: if this clause's `player_scope` link + // captured a snapshot, the answer was determined ONCE when the + // effect was applied (608.2h) and the whole fan-out is one action + // processed simultaneously (608.2e) — so every player in the + // fan-out must read that frozen value, not the scalar their own + // completed action re-stamped. CR 121.2c: the serialization of the + // multiplayer draw is itself correct; only the leaked count is not. + // Mirrors the snapshot-first shape of the `HandSize { AllPlayers }` + // and `ControlledByEachPlayer` arms. Placed before the channel + // match because the snapshot is keyed on the WHOLE `QuantityRef` + // (`ClauseMinimumSnapshot::get`), so it is channel- and + // aggregate-correct by key. + if let Some(v) = state + .clause_minimum_snapshot + .as_ref() + .and_then(|s| s.get(qty)) + { + return v; + } + match channel { + DamageChannel::Total => { + let total = state.last_effect_amount.unwrap_or(0); + let per_player = state.last_effect_counts_by_player.values().copied(); + match aggregate { + AggregateFunction::Sum => total, + // An absent table means the producer published NO per-player + // breakdown: only `Effect::Discard | DiscardCard | + // ChangeZoneAll` populate it; every other producer takes the + // `None` arm in `install_previous_effect_counts_by_player`, + // which clears it. For a SINGLE-subject producer the scalar + // IS the extremum, so the fallback is exact. For a + // MULTI-subject non-count producer — `Effect::DamageEachPlayer`, + // `Effect::DamageAll`, `Effect::LoseLife` under `player_scope` + // — the scalar is a cross-player SUM and a Max read would + // over-report. Unreachable today, measured: the Scryfall + // census (2026-08-15) returns exactly 3 cards in the Max + // class and all 3 follow an `Effect::Discard`, a count + // producer. The real-zero case is also safe: the discard + // fan-out zero-fills an empty producer table with one entry + // per matching player, so a discard-of-nothing yields a + // non-empty all-zero table (Max = 0), never the fallback. + // + // The mirror-image hazard is a STALE PRESERVED table, not + // an absent one: `install_previous_effect_counts_by_player` + // KEEPS the prior table on its `None` arm when + // `preserve_counts_for_current_consumer` (`player_scope.is_none() + // && effect_consumes_event_context_amount`). In a chain + // A(count producer) -> B(EventContextAmount consumer, no + // player_scope) -> C(PreviousEffectAmount{Max}), C would + // fold A's table while `Sum` reads B's re-stamped scalar. + // Unreachable for the closed Max/Min class, measured: all 3 + // class cards are `Discard{All} -> Draw{PEA}` with the + // consumer in the IMMEDIATELY following link, so no B can + // interpose; and `Sum` is unaffected either way because it + // reads `last_effect_amount`, exactly as before this change. + AggregateFunction::Max => per_player.max().unwrap_or(total), + AggregateFunction::Min => per_player.min().unwrap_or(total), + } + } + // CR 120.10: only the damage dealt BEYOND lethal — "the amount of + // excess damage dealt to that creature this way" (Goblin + // Negotiation, Hell to Pay, Lacerate Flesh), "that excess damage" + // (Contest of Claws). A scalar channel with no per-player table, + // so every aggregate reduces to it. 0 when no excess was dealt. + DamageChannel::Excess => state.last_effect_excess_amount.unwrap_or(0), + } + } // Read the preceding continuation-local effect count directly. // An unavailable count resolves to zero. QuantityRef::PreviousEffectCount => state.last_effect_count.unwrap_or(0), @@ -17524,6 +17579,110 @@ mod tests { assert_eq!(resolve_quantity(&state, &qty, PlayerId(0), ObjectId(0)), 5); } + #[test] + fn previous_effect_amount_prefers_clause_snapshot() { + // CR 608.2h: the third class admitted to the clause freeze. The live + // tally is deliberately set to a DIFFERENT value than the frozen one, so + // the assertion fails if the snapshot read is removed or ordered after + // the channel match. This is the unit-level peer of the integration + // test `windfall_short_library_does_not_shrink_later_players_draws`, + // where the live value is what a completed draw re-stamped. + let mut state = GameState::new_two_player(42); + let qref = QuantityRef::PreviousEffectAmount { + channel: DamageChannel::Total, + aggregate: AggregateFunction::Max, + }; + let mut snap = crate::types::game_state::ClauseMinimumSnapshot::default(); + snap.insert(qref.clone(), 8); + state.clause_minimum_snapshot = Some(snap); + // Live state says 5 — the post-fan-out value the freeze must override. + state.last_effect_amount = Some(5); + state.last_effect_counts_by_player.insert(PlayerId(0), 5); + let qty = QuantityExpr::Ref { qty: qref }; + assert_eq!(resolve_quantity(&state, &qty, PlayerId(0), ObjectId(0)), 8); + } + + #[test] + fn previous_effect_amount_live_when_no_snapshot() { + // The fallback arm: with no clause snapshot the ref reads live state, so + // `Max` over the per-player table {P0:8, P1:3} is 8. Pairs with the test + // above — together they show the snapshot is PREFERRED, not the only + // path, so a fix that always returned the snapshot would fail here. + let mut state = GameState::new_two_player(42); + state.last_effect_amount = Some(11); + state.last_effect_counts_by_player.insert(PlayerId(0), 8); + state.last_effect_counts_by_player.insert(PlayerId(1), 3); + let qty = QuantityExpr::Ref { + qty: QuantityRef::PreviousEffectAmount { + channel: DamageChannel::Total, + aggregate: AggregateFunction::Max, + }, + }; + assert_eq!(resolve_quantity(&state, &qty, PlayerId(0), ObjectId(0)), 8); + } + + /// All three reductions of ONE table must be mutually distinguishable, or a + /// test that pins any of them proves nothing about the others. Table + /// {8,7,3} with `last_effect_amount` 18: Sum 18 / Max 8 / Min 3 — three + /// distinct values, so each assertion below fails if its arm is swapped for + /// either sibling. + #[test] + fn previous_effect_amount_aggregates_are_mutually_distinct() { + let mut state = GameState::new_two_player(42); + state.last_effect_amount = Some(18); + for (p, n) in [(0, 8), (1, 7), (2, 3)] { + state.last_effect_counts_by_player.insert(PlayerId(p), n); + } + let read = |agg| { + resolve_quantity( + &state, + &QuantityExpr::Ref { + qty: QuantityRef::PreviousEffectAmount { + channel: DamageChannel::Total, + aggregate: agg, + }, + }, + PlayerId(0), + ObjectId(0), + ) + }; + assert_eq!(read(AggregateFunction::Sum), 18, "Sum reads the total"); + assert_eq!(read(AggregateFunction::Max), 8, "Max reads the greatest"); + assert_eq!(read(AggregateFunction::Min), 3, "Min reads the least"); + } + + /// CR 608.2c: a player the clause applied to who contributed NOTHING still + /// contributed zero *this way*, so the table must carry them. + /// + /// This is the resolver half of the producer fix: given a table that + /// includes the zero-contributor, `Min` must be 0. The producer half — that + /// the table actually gets that entry — is + /// `windfall_empty_hand_player_is_in_the_per_player_table`. + /// + /// Board 8/7/3/**0**. Before the producer fix the table omitted the + /// zero-contributor and published {8,7,3}, so `Min` answered 3. `Max` is + /// immune to the omission (zeros cannot raise a maximum) and `Sum` reads + /// `last_effect_amount`, which is why the shipped Max class never saw it. + #[test] + fn previous_effect_amount_min_counts_the_zero_contributor() { + let mut state = GameState::new_two_player(42); + state.last_effect_amount = Some(18); + for (p, n) in [(0, 8), (1, 7), (2, 3), (3, 0)] { + state.last_effect_counts_by_player.insert(PlayerId(p), n); + } + let qty = QuantityExpr::Ref { + qty: QuantityRef::PreviousEffectAmount { + channel: DamageChannel::Total, + aggregate: AggregateFunction::Min, + }, + }; + assert_eq!( + resolve_quantity(&state, &qty, PlayerId(0), ObjectId(0)), + 0, + "the empty-handed player discarded 0 this way; the minimum is 0, not 3" + ); + } + #[test] fn hand_size_all_players_min_live_when_no_snapshot() { // Without a snapshot, `HandSize { AllPlayers { Min } }` resolves live — diff --git a/crates/engine/src/game/visibility.rs b/crates/engine/src/game/visibility.rs index 6aa57d3ec3..f47b88d271 100644 --- a/crates/engine/src/game/visibility.rs +++ b/crates/engine/src/game/visibility.rs @@ -251,6 +251,20 @@ pub fn filter_state_for_viewer(state: &GameState, viewer: PlayerId) -> GameState // The replacement-resume cursor is server authority and can retain private // object IDs and last-known snapshots from a cost payment. filtered.pending_cost_move_resume = None; + // The EFFECT layer's twin of the cursor above, redacted for the same + // reason: `PendingDiscardBatch` retains the object IDs of cards still in a + // hand (a hidden zone, CR 400.2), the instruction's pre-pause event span, + // and full `ResolvedAbility` clones of the paused clause. The projected + // `WaitingFor::ReplacementChoice` is the complete viewer-facing interaction + // surface, so no viewer — including the choosing player — needs the carrier + // itself. Viewer projections are display-only clones; the authoritative + // state the drain resumes from is never filtered. + filtered.pending_discard_batch = None; + // CR 608.2h: a paused player-scope clause retains its frozen aggregate in + // authoritative state so save/restore resumes the same application. The + // value can encode hidden-zone information (for example, hand sizes), so it + // belongs with the private discard cursor rather than any viewer payload. + filtered.clause_minimum_snapshot = None; // Deferred life-cost owners can embed a complete PendingCast, including // hidden card and target context. The projected WaitingFor is the only // viewer-facing interaction surface. @@ -6174,6 +6188,77 @@ mod tests { ); } + /// CR 400.2 + CR 608.2c: `pending_discard_batch` is the EFFECT layer's twin + /// of the cost cursor above. It retains the object IDs of cards still in a + /// HAND — a hidden zone — plus the instruction's pre-pause event span, so it + /// must be absent from every viewer projection, including the projection of + /// the player who owns the live replacement prompt. `hide_card` is an + /// allowlist, so a new state carrier defaults to LEAKED and this has to be + /// measured rather than assumed. + /// + /// REVERT PROBE (RUN, not reasoned): delete + /// `filtered.pending_discard_batch = None;` from `filter_state_for_viewer`. + /// Observed first failure — "viewer PlayerId(0) must not receive the parked + /// discard batch". The later per-viewer and wire-string assertions never run + /// (the first panic ends the test), so it is that one which discriminates. + #[test] + fn parked_discard_batch_is_absent_from_every_viewer_projection() { + let mut state = GameState::new_two_player(42); + let hidden = create_object( + &mut state, + CardId(70_007), + PlayerId(0), + "Hand Secret".to_string(), + Zone::Hand, + ); + state.pending_discard_batch = + Some(Box::new(crate::types::game_state::PendingDiscardBatch { + player: PlayerId(0), + cursor: crate::types::game_state::DiscardBatchCursor::All { + remaining: vec![hidden], + }, + completion: crate::types::game_state::PendingDiscardBatchCompletion::Standard, + source_id: ObjectId(9_300), + effect_kind: crate::types::ability::EffectKind::Discard, + paused_card: crate::types::identifiers::ObjectIncarnationRef::of(hidden, 0), + discard_frame: None, + fan_out: None, + preceding_events: Vec::new(), + })); + state.clause_minimum_snapshot = + Some(crate::types::game_state::ClauseMinimumSnapshot::default()); + + let authoritative = serde_json::to_string(&state.pending_discard_batch) + .expect("the authoritative batch serializes"); + assert!( + authoritative.contains(&hidden.0.to_string()), + "reach guard: the authoritative carrier really does hold the hand card's ID" + ); + + for viewer in [PlayerId(0), PlayerId(1)] { + let view = filter_state_for_viewer(&state, viewer); + assert!( + view.pending_discard_batch.is_none(), + "viewer {viewer:?} must not receive the parked discard batch" + ); + assert!( + view.clause_minimum_snapshot.is_none(), + "viewer {viewer:?} must not receive the paused clause's private aggregate" + ); + let wire = serde_json::to_string(&view).expect("the filtered snapshot serializes"); + assert!( + !wire.contains("\"pendingDiscardBatch\":{") + && !wire.contains("\"pending_discard_batch\":{"), + "viewer {viewer:?}'s snapshot must not serialize the carrier at all" + ); + } + + assert!( + state.pending_discard_batch.is_some(), + "filtering must not alter the authoritative server carrier" + ); + } + /// CR 605.4a + CR 117.3c (plan Step 6): the triggered-mana continuation and /// the trigger-construction priority recipient are trusted persistence /// authority. They must survive an authoritative round trip exactly, and diff --git a/crates/engine/src/parser/oracle_effect/assembly.rs b/crates/engine/src/parser/oracle_effect/assembly.rs index 0d7e734e11..866818ac2e 100644 --- a/crates/engine/src/parser/oracle_effect/assembly.rs +++ b/crates/engine/src/parser/oracle_effect/assembly.rs @@ -2242,6 +2242,7 @@ pub(crate) fn assemble_effect_chain(ir: &EffectChainIr) -> AbilityDefinition { if let Effect::DamageEachPlayer { amount, .. } = def.effect.as_mut() { amount.rebind_event_context_amount(&QuantityRef::PreviousEffectAmount { channel: DamageChannel::Total, + aggregate: AggregateFunction::Sum, }); } } diff --git a/crates/engine/src/parser/oracle_effect/imperative.rs b/crates/engine/src/parser/oracle_effect/imperative.rs index 2de2b63ec4..72693dc966 100644 --- a/crates/engine/src/parser/oracle_effect/imperative.rs +++ b/crates/engine/src/parser/oracle_effect/imperative.rs @@ -1848,6 +1848,7 @@ pub(super) fn parse_targeted_action_ast( count = QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: crate::types::ability::AggregateFunction::Sum, }, }; } diff --git a/crates/engine/src/parser/oracle_effect/mana.rs b/crates/engine/src/parser/oracle_effect/mana.rs index 17b62ed0f1..3744258e9f 100644 --- a/crates/engine/src/parser/oracle_effect/mana.rs +++ b/crates/engine/src/parser/oracle_effect/mana.rs @@ -3508,6 +3508,7 @@ mod tests { QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: crate::types::ability::AggregateFunction::Sum, } }, "for-each tail must dispatch to PreviousEffectAmount" diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index b71037af14..c5d2876e59 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -12575,15 +12575,27 @@ fn windfall_draw_uses_previous_discard_max_for_each_player() { .as_ref() .expect("expected draw continuation"); assert_eq!(sub.player_scope, Some(PlayerFilter::All)); - assert!(matches!( - &*sub.effect, - Effect::Draw { - count: QuantityExpr::Ref { - qty: QuantityRef::PreviousEffectAmount { .. } - }, - target: TargetFilter::Controller, - } - )); + // CR 608.2c + CR 608.2i: the superlative names the cross-player reduction, + // so the draw count must carry `Max` — the whole point of this test's name. + // A `{ .. }` wildcard here pinned NOTHING and passed at BASE while Windfall + // drew the cross-player SUM. Revert the combinator's `Max` to `Sum`, or drop + // the `oracle_quantity.rs` delegation, and this FAILS. + assert!( + matches!( + &*sub.effect, + Effect::Draw { + count: QuantityExpr::Ref { + qty: QuantityRef::PreviousEffectAmount { + channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Max, + } + }, + target: TargetFilter::Controller, + } + ), + "expected PreviousEffectAmount {{ Total, Max }}, got {:?}", + sub.effect + ); } /// CR 608.2c + CR 701.9 + CR 118.12: Read the Runes — draw X, then for each @@ -38569,6 +38581,7 @@ fn for_each_prefix_pump_threads_self_ref_target() { Some(QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }), "repeat_for should scale by counters removed in the activation cost" diff --git a/crates/engine/src/parser/oracle_effect/token.rs b/crates/engine/src/parser/oracle_effect/token.rs index fd9cf21dfb..dbda946870 100644 --- a/crates/engine/src/parser/oracle_effect/token.rs +++ b/crates/engine/src/parser/oracle_effect/token.rs @@ -1803,6 +1803,7 @@ mod tests { QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: crate::types::ability::AggregateFunction::Sum, }, }, ), diff --git a/crates/engine/src/parser/oracle_nom/condition.rs b/crates/engine/src/parser/oracle_nom/condition.rs index 8a1524ec10..75adf03677 100644 --- a/crates/engine/src/parser/oracle_nom/condition.rs +++ b/crates/engine/src/parser/oracle_nom/condition.rs @@ -9910,6 +9910,7 @@ pub fn parse_you_draw_this_way_condition(input: &str) -> OracleResult<'_, Abilit lhs: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, comparator: Comparator::GE, diff --git a/crates/engine/src/parser/oracle_nom/quantity.rs b/crates/engine/src/parser/oracle_nom/quantity.rs index c1d6de9e94..c40b93c4d6 100644 --- a/crates/engine/src/parser/oracle_nom/quantity.rs +++ b/crates/engine/src/parser/oracle_nom/quantity.rs @@ -778,6 +778,7 @@ fn parse_excess_damage_ref(input: &str) -> OracleResult<'_, QuantityRef> { value( QuantityRef::PreviousEffectAmount { channel: DamageChannel::Excess, + aggregate: AggregateFunction::Sum, }, ( opt(alt((tag("the amount of "), tag("the ")))), @@ -797,6 +798,35 @@ fn parse_excess_damage_ref(input: &str) -> OracleResult<'_, QuantityRef> { .parse(input) } +/// CR 608.2c + CR 608.2i: "the greatest number of cards a player discarded this +/// way" — a look-back read of the completed discard instruction whose +/// SUPERLATIVE names the cross-player reduction. Windfall, Jace's Archivist, +/// Whispering Madness (Scryfall census 2026-08-15: exactly these three, +/// identical clause; zero "least/fewest" counterparts exist). +/// +/// The superlative is the AGGREGATE AXIS and must be REPORTED, not consumed and +/// thrown away: the legacy `oracle_quantity.rs` arm matched `greatest|highest` +/// and emitted a bare (Sum-equivalent) ref, so a four-player board with hands +/// 8/7/3/3 drew 21 — the cross-player SUM — instead of 8. Reuses the shipped +/// `parse_max_extremum_adjective` so `greatest`, `highest` and `largest` stay +/// ONE axis rather than three enumerated phrases. +pub(crate) fn parse_greatest_discarded_this_way(input: &str) -> OracleResult<'_, QuantityRef> { + value( + QuantityRef::PreviousEffectAmount { + channel: DamageChannel::Total, + aggregate: AggregateFunction::Max, + }, + ( + opt(tag("the ")), + parse_max_extremum_adjective, + tag(" number of cards "), + opt(alt((tag("a player "), tag("any player ")))), + tag("discarded this way"), + ), + ) + .parse(input) +} + /// CR 701.22a + CR 701.22d: "the number of cards looked at while scrying this /// way" — the effective (post-clamp) look count of the scry that fired the /// enclosing "whenever you scry" trigger (Elrond, Master of Healing: "put a @@ -1447,7 +1477,7 @@ fn parse_the_number_of(input: &str) -> OracleResult<'_, QuantityRef> { parse_number_of_inner(rest) } -/// CR 107.1: The maximizing extremum adjective. Oracle text prints several +/// The maximizing extremum adjective. Oracle text prints several /// interchangeable superlatives for the same `AggregateFunction::Max` /// ("greatest power", "highest mana value"); they are one axis, not one phrase /// each. Verdant Rejuvenation prints "highest". @@ -12180,4 +12210,57 @@ mod tests { "targeted of-form must stay TargetObjectManaValue, got {q:?}" ); } + + /// V7b — CR 608.2c + CR 608.2i: the widened "greatest number of cards a + /// player discarded this way" grammar, exercised where it lives. + /// + /// Both widenings over the deleted legacy arm are pinned here: the + /// superlative axis (`largest`, which the legacy `alt((greatest, highest))` + /// rejected) and the now-optional determiner. Revert + /// `parse_max_extremum_adjective` to `alt((greatest, highest))` and the + /// first two FAIL; make `tag("the ")` mandatory and the first FAILS. + #[test] + fn greatest_discarded_this_way_reports_the_max_aggregate() { + let max_ref = QuantityRef::PreviousEffectAmount { + channel: DamageChannel::Total, + aggregate: AggregateFunction::Max, + }; + + // Determiner-less AND widened adjective, both at once. + assert_eq!( + parse_greatest_discarded_this_way( + "largest number of cards a player discarded this way" + ) + .expect("determiner-less widened form must bind"), + ("", max_ref.clone()) + ); + assert_eq!( + parse_greatest_discarded_this_way( + "the largest number of cards any player discarded this way" + ) + .expect("widened adjective with determiner must bind"), + ("", max_ref.clone()) + ); + // The shipped production phrase. + assert_eq!( + parse_greatest_discarded_this_way( + "the greatest number of cards a player discarded this way" + ) + .expect("production Windfall phrase must bind"), + ("", max_ref) + ); + } + + /// V7b negative — the combinator cannot capture the superlative-free + /// `TrackedSetSize` phrase. Direct proof that adding this arm does not + /// steal "the number of cards a player discarded this way", which parses to + /// a tracked-set shape elsewhere. + #[test] + fn greatest_discarded_this_way_rejects_the_superlative_free_phrase() { + assert!( + parse_greatest_discarded_this_way("the number of cards a player discarded this way") + .is_err(), + "no superlative means no aggregate axis — must not match" + ); + } } diff --git a/crates/engine/src/parser/oracle_quantity.rs b/crates/engine/src/parser/oracle_quantity.rs index 57bcded329..61631b8024 100644 --- a/crates/engine/src/parser/oracle_quantity.rs +++ b/crates/engine/src/parser/oracle_quantity.rs @@ -165,6 +165,7 @@ pub(crate) fn parse_quantity_ref_with_context( if try_parse_counters_removed_this_way(rest) { return Some(QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, }); } } @@ -1267,7 +1268,7 @@ fn parse_owned_cards_in_zones_quantity( Ok((rest, expr)) } -/// CR 608.2c + CR 120.6 / CR 120.10: "[the] this way", reporting +/// CR 608.2c + CR 120.10: "[the] this way", reporting /// WHICH damage channel the phrase named. /// /// The damage arm carries a channel because "excess" is an independent qualifier @@ -1782,14 +1783,17 @@ pub(crate) fn parse_event_context_quantity(text: &str) -> Option { // - counter-removal chains: "counters removed", "counter removed" // (Sensational Spider-Man's "stun counters removed this way"; // `state.last_effect_amount` is stamped by the preceding RemoveCounter). - // PreviousEffectAmount reads `state.last_effect_amount` (CR 120.6) or + // PreviousEffectAmount reads `state.last_effect_amount` or // `state.last_effect_excess_amount` (CR 120.10), whichever channel the phrase // named — the combinator reports it rather than the caller assuming `Total`. // Assuming Total here is precisely what made "the excess damage dealt this // way" gain the FULL damage instead of the overkill (Razor Rings). if let Ok((_, channel)) = parse_previous_effect_amount_this_way(lower) { return Some(QuantityExpr::Ref { - qty: QuantityRef::PreviousEffectAmount { channel }, + qty: QuantityRef::PreviousEffectAmount { + channel, + aggregate: AggregateFunction::Sum, + }, }); } @@ -1812,21 +1816,14 @@ pub(crate) fn parse_event_context_quantity(text: &str) -> Option { }); } - if nom::combinator::all_consuming(( - tag::<_, _, OracleError<'_>>("the "), - alt((tag("greatest "), tag("highest "))), - tag("number of cards "), - nom::combinator::opt(alt((tag("a player "), tag("any player ")))), - tag("discarded this way"), - )) - .parse(lower) - .is_ok() - { - return Some(QuantityExpr::Ref { - qty: QuantityRef::PreviousEffectAmount { - channel: crate::types::ability::DamageChannel::Total, - }, - }); + // CR 608.2c + CR 608.2i: "the greatest number of cards a player discarded + // this way" — the superlative IS the aggregate axis and is REPORTED by the + // combinator, never matched and discarded. Grammar lives in + // `oracle_nom/quantity.rs` per the parser skill's single-authority + // doctrine; `Ok(("", …))` is the same full-consumption requirement the + // deleted `all_consuming` tuple expressed. + if let Ok(("", qty)) = nom_quantity::parse_greatest_discarded_this_way(lower) { + return Some(QuantityExpr::Ref { qty }); } // CR 614.1a: "that much/many [noun] (plus|minus) N" — Offset over the @@ -3211,6 +3208,7 @@ fn parse_for_each_clause_with_they_controller( { return Some(QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, }); } @@ -3334,6 +3332,7 @@ fn parse_for_each_clause_with_they_controller( if try_parse_counters_removed_this_way(&lower) { return Some(QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, }); } // CR 608.2c + CR 400.7: "nontoken creature you controlled that was @@ -4522,6 +4521,7 @@ mod tests { qty, QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, "{phrase:?} ({note})" ); @@ -4535,6 +4535,7 @@ mod tests { qty, QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, } ); } @@ -5920,7 +5921,7 @@ mod tests { ); } - /// CR 120.6: the TOTAL channel. Every phrase here names an unqualified + /// The TOTAL channel. Every phrase here names an unqualified /// numeric result — no "excess" qualifier — so it reads `last_effect_amount`. #[test] fn parse_event_context_quantity_previous_effect_this_way_variants() { @@ -5935,6 +5936,7 @@ mod tests { Some(QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }), "phrase {phrase:?} must map to PreviousEffectAmount on the TOTAL channel" @@ -5942,6 +5944,66 @@ mod tests { } } + /// V7a — CR 608.2c + CR 608.2i: the PRODUCTION path reports the aggregate. + /// + /// `parse_event_context_quantity` is `pub(crate)`, so this guard must live + /// in-crate. It pins the delegation added at the deleted legacy block's + /// exact position: the superlative form must now carry + /// `AggregateFunction::Max` instead of the bare (Sum-equivalent) ref that + /// made Windfall draw the cross-player SUM. Revert the combinator's `Max` + /// to `Sum` and all three positives FAIL. + #[test] + fn parse_event_context_quantity_greatest_discarded_this_way_reports_max() { + for phrase in [ + "the greatest number of cards a player discarded this way", + "the highest number of cards a player discarded this way", + "the greatest number of cards any player discarded this way", + ] { + assert_eq!( + parse_event_context_quantity(phrase), + Some(QuantityExpr::Ref { + qty: QuantityRef::PreviousEffectAmount { + channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Max, + }, + }), + "phrase {phrase:?} must report the MAX aggregate, not a bare ref" + ); + } + } + + /// V7a paired negatives — the superlative-free neighbours keep their + /// MEASURED shapes, so the new delegation cannot have stolen them. Each + /// `.expect(…)`s first, so neither can pass on a parse failure. + #[test] + fn parse_event_context_quantity_superlative_free_discard_phrases_are_unchanged() { + let bare = parse_event_context_quantity("the number of cards discarded this way") + .expect("superlative-free bare form must still parse"); + assert!( + matches!( + bare, + QuantityExpr::Ref { + qty: QuantityRef::FilteredTrackedSetSize { + caused_by: Some(ThisWayCause::Discarded), + .. + } + } + ), + "bare form must stay a filtered tracked-set read, got {bare:?}" + ); + + let per_player = + parse_event_context_quantity("the number of cards a player discarded this way") + .expect("superlative-free per-player form must still parse"); + assert_eq!( + per_player, + QuantityExpr::Ref { + qty: QuantityRef::TrackedSetSize + }, + "per-player superlative-free form must stay a tracked-set read" + ); + } + #[test] fn parse_event_context_quantity_opponents_dealt_damage_counts_event_players() { for phrase in [ @@ -5989,6 +6051,7 @@ mod tests { Some(QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Excess, + aggregate: AggregateFunction::Sum, }, }), "phrase {phrase:?} names EXCESS damage (CR 120.10) and must read the \ diff --git a/crates/engine/src/parser/oracle_trigger_tests.rs b/crates/engine/src/parser/oracle_trigger_tests.rs index 8d1fcc1f7b..ecc00adb23 100644 --- a/crates/engine/src/parser/oracle_trigger_tests.rs +++ b/crates/engine/src/parser/oracle_trigger_tests.rs @@ -19077,6 +19077,7 @@ fn trigger_coalition_relic_charge_counter_drain() { QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: crate::types::ability::DamageChannel::Total, + aggregate: AggregateFunction::Sum, } }, "for-each tail must dispatch to PreviousEffectAmount" @@ -28677,6 +28678,7 @@ fn valakut_exploration_end_step_trigger_hoists_gate_and_keeps_damage_shape() { qty: QuantityRef::PreviousEffectAmount { channel: DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, player_filter: PlayerFilter::Opponent, diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 189ecc215c..7cedebef5c 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -6976,7 +6976,7 @@ pub enum QuantityRef { /// behind — the same axis, and the same `DamageChannel`, already carried by /// the condition peer [`AbilityCondition::PreviousEffectAmount`]: /// - /// - [`DamageChannel::Total`] (default): the total amount (CR 120.6), via + /// - [`DamageChannel::Total`] (default): the total amount, via /// `GameState::last_effect_amount`. Every non-damage producer (life lost, /// counters removed, cards drawn) stamps only this channel. /// - [`DamageChannel::Excess`]: the EXCESS amount (CR 120.10) — damage dealt @@ -6987,14 +6987,56 @@ pub enum QuantityRef { /// /// A sibling `PreviousEffectExcessAmount` variant would be the textbook /// sibling-cluster smell: the channel is a leaf parameterization of one - /// structural axis, and it lies wholly inside CR 120 (120.6 total / - /// 120.10 excess), so it is a parameterization, not a new leaf. + /// structural axis, so it is a parameterization, not a new leaf. + /// + /// The categorical boundary is CR 608.2c / CR 608.2i — this reference's OWN + /// section. Both channels are readings of the same look-back at the one + /// completed instruction; they differ only in which tally that instruction + /// left behind. (An earlier revision justified the boundary as "both + /// channels lie wholly inside CR 120". That is wrong and is struck: CR 120 + /// is Damage, while the `Total` channel is stamped by non-damage producers — + /// life lost, counters removed, cards drawn — as three lines above already + /// say. CR 120.10 is cited only where it does apply, for what "excess" + /// means.) /// /// `Total` is serde-elided, so every pre-existing serialized card is - /// byte-identical. + /// byte-identical — a parse-diff fidelity property, not a save-compatibility + /// promise. PreviousEffectAmount { #[serde(default, skip_serializing_if = "is_total_damage_channel")] channel: DamageChannel, + /// CR 608.2c + CR 608.2i: how the completed instruction's per-player + /// result table (`GameState::last_effect_counts_by_player`) is reduced + /// to the one number this look-back reference reads. + /// + /// - `Sum` (default): the cross-player TOTAL, read from + /// `GameState::last_effect_amount` — which + /// `install_previous_effect_counts_by_player` + /// (`game/effects/mod.rs`) stamps as the sum of the table, and the + /// ONLY channel a non-per-player producer (damage, life, counters, + /// draw, die roll) leaves behind. Byte-identical to the + /// pre-`aggregate` behaviour for every existing consumer. + /// - `Max`: the GREATEST single player's contribution — Windfall, + /// Jace's Archivist, Whispering Madness ("draws cards equal to the + /// greatest number of cards a player discarded this way"). Scryfall + /// census 2026-08-15 returns exactly these three. + /// - `Min`: no printed card uses it (same census: zero + /// "least/fewest … this way" cards). Present because + /// `AggregateFunction` is a shared 3-valued enum matched + /// exhaustively; the arm is a one-line `.min()`, not a stub. + /// + /// The `Excess` channel (CR 120.10) publishes a scalar, not a table + /// (`GameState::last_effect_excess_amount`), so on that channel + /// Max/Min/Sum of the single value coincide — degenerate by + /// construction, not silently ignored. + /// + /// `Sum` is serde-elided, so every pre-existing serialized card, + /// scenario and IR snapshot is byte-identical. + #[serde( + default = "default_sum_aggregate", + skip_serializing_if = "is_sum_aggregate" + )] + aggregate: AggregateFunction, }, /// Engine bookkeeping for the immediately preceding resolution-local effect /// count. This reads `GameState::last_effect_count` directly, defaults an @@ -7202,8 +7244,8 @@ pub enum QuantityRef { source: Box, target: Box, #[serde( - default = "default_damage_aggregate", - skip_serializing_if = "is_default_damage_aggregate" + default = "default_sum_aggregate", + skip_serializing_if = "is_sum_aggregate" )] aggregate: AggregateFunction, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -15457,7 +15499,7 @@ fn default_counter_transfer_mode() -> CounterTransferMode { CounterTransferMode::Move } -fn default_damage_aggregate() -> AggregateFunction { +fn default_sum_aggregate() -> AggregateFunction { AggregateFunction::Sum } @@ -15526,7 +15568,7 @@ fn default_most_prevalent_scope() -> ControllerRef { ControllerRef::You } -fn is_default_damage_aggregate(a: &AggregateFunction) -> bool { +fn is_sum_aggregate(a: &AggregateFunction) -> bool { matches!(a, AggregateFunction::Sum) } @@ -21440,19 +21482,27 @@ pub enum AbilityCondition { comparator: Comparator, rhs: QuantityExpr, }, - /// CR 608.2c + CR 120.6 + CR 120.10: Compares the numeric result tracked from + /// CR 608.2c + CR 120.10: Compares the numeric result tracked from /// the previous instruction in the same resolution against `rhs`. The /// `channel` selects which resolution-local tally is read: - /// - `DamageChannel::Total` (default): the *total* amount (CR 120.6) via + /// - `DamageChannel::Total` (default): the total amount via /// `last_effect_amount` — the same channel that feeds - /// `QuantityRef::PreviousEffectAmount` / `EventContextAmount`. + /// `QuantityRef::PreviousEffectAmount` / `EventContextAmount`. Every + /// non-damage producer (life lost, counters removed, cards drawn) stamps + /// only this channel, so no CR 120 rule governs it; the look-back itself + /// is CR 608.2c. /// - `DamageChannel::Excess`: the *excess* amount (CR 120.10) via /// `last_effect_excess_amount` — damage dealt beyond lethal /// ("if excess damage was dealt … this way"). + /// + /// CR 120.6 was cited here for the `Total` channel and is struck: it governs + /// marked damage persisting until cleanup, not an amount left behind by a + /// preceding effect. Mirrors the identical correction on the `QuantityRef` + /// peer — the two must not disagree about the same channel. PreviousEffectAmount { comparator: Comparator, rhs: QuantityExpr, - /// CR 120.6 / CR 120.10: which resolution-local channel to compare + /// CR 608.2c / CR 120.10: which resolution-local channel to compare /// against. Reuses the committed `DamageChannel`; `Total` is serde-elided /// so every existing card is byte-identical. #[serde(default, skip_serializing_if = "is_total_damage_channel")] @@ -30173,7 +30223,7 @@ mod tests { let modern_total = QuantityRef::DamageDealtThisTurn { source: Box::new(TargetFilter::Any), target: Box::new(TargetFilter::Any), - aggregate: default_damage_aggregate(), + aggregate: default_sum_aggregate(), group_by: None, damage_kind: default_damage_kind(), channel: DamageChannel::Total, @@ -30214,7 +30264,7 @@ mod tests { let modern_excess = QuantityRef::DamageDealtThisTurn { source: Box::new(TargetFilter::Any), target: Box::new(TargetFilter::Any), - aggregate: default_damage_aggregate(), + aggregate: default_sum_aggregate(), group_by: None, damage_kind: default_damage_kind(), channel: DamageChannel::Excess, diff --git a/crates/engine/src/types/game_state.rs b/crates/engine/src/types/game_state.rs index a4032e41f2..176f9e361f 100644 --- a/crates/engine/src/types/game_state.rs +++ b/crates/engine/src/types/game_state.rs @@ -4058,6 +4058,196 @@ pub enum PendingPlayerScopeSacrificeFollowUp { Exploit { exploiter: ObjectId }, } +/// One discard instruction, parked mid-batch while an optional replacement +/// application awaits its apply-or-decline choice. +/// +/// This is [`PendingPlayerScopeSacrificeChoice`]'s sibling one layer down, and +/// it carries the same two things across the pause: the **cursor** is what the +/// instruction still owes, `preceding_events` is what it has already done, and +/// the two are reunited into one terminal window when the batch settles. Read +/// that type first — every mechanism here is its, with the two deliberate +/// divergences noted on `preceding_events` and in `drain_pending_discard_batch`. +/// +/// CR 614.1: replacement effects apply as events happen. This batch preserves +/// the cursor and already-produced events while the selected optional +/// replacement is applied or declined, then resumes the same discard +/// instruction. +/// +/// The companion `GameState::clause_minimum_snapshot` persists with this batch: +/// a save taken mid-pause must resume the same CR 608.2h application with its +/// original frozen value, rather than determine it again after restore. +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +pub struct PendingDiscardBatch { + /// The discarding seat whose batch paused. + pub player: PlayerId, + /// What this seat still owes. + pub cursor: DiscardBatchCursor, + /// Work that must run exactly once after the cursor has fully settled. + /// + /// An interactive discard choice normally finalizes in its response + /// handler. A replacement can interrupt that handler mid-selection, so its + /// completion belongs to the same typed carrier as the remaining cards. + #[serde(default)] + pub completion: PendingDiscardBatchCompletion, + /// The object that caused the discard. Together with `effect_kind` and + /// `player` this is the batch's identity: the driver hand-off below refuses + /// any batch whose identity does not match the clause it is running. + pub source_id: ObjectId, + /// The terminal marker this batch must emit once it settles. Held as + /// `EffectKind` rather than `Effect` because both `Effect::Discard` and + /// `Effect::DiscardCard` route here and the count authority + /// (`previous_effect_counts_by_player_from_events`) selects on kind alone. + pub effect_kind: EffectKind, + /// The card whose replacement is being chosen right now. + /// + /// CR 614.6: "If an event is replaced, it never happens. A modified event + /// occurs instead." A hand → graveyard `Moved` redirect therefore still + /// discarded the card (CR 701.9a), but the resumed zone-change arm emits no + /// `GameEvent::Discarded` for an unframed discard, so the drain stamps one + /// from this id. Without it the paused card is the one card that silently + /// leaves the ledger even though the batch resumed correctly. + /// + /// CR 400.7: pinned as an incarnation reference, not a bare `ObjectId`. The + /// pause parks the pre-move occurrence; the departure that settles it is + /// that same occurrence leaving the hand. A later same-`ObjectId` occurrence + /// (the card returned to hand and left again) is a different object and must + /// not be able to satisfy this pause — see + /// `stamp_resumed_discard_if_unrecorded`, which matches the departing + /// occurrence carried on the zone-change record rather than the id alone. + pub paused_card: ObjectIncarnationRef, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub discard_frame: Option, + /// CR 608.2f: the clause and the seats it has not reached, installed by the + /// `player_scope` driver when this pause interrupted its fan-out. `None` + /// for a single-subject discard, which owns no fan-out. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub fan_out: Option>, + /// The events this instruction emitted BEFORE the pause. + /// + /// COPIED, not drained — the one place this type diverges from + /// `PendingPlayerScopeSacrificeCompletion::deferred_events`. That sibling + /// drains because its terminal co-departure stamping (CR 603.10a look-back + /// zone-change triggers) has to rewrite the live event buffer. Discard does + /// no co-departure stamping, so draining would reorder client-visible + /// events across two actions for no rules benefit; copying yields the + /// identical terminal count window with no observable change. + /// + /// CR 608.2i: at completion the window is `preceding_events ++ events`, read + /// by the same authority the un-paused path uses. That rule's precondition + /// holds here — "if such an effect requires information from the game about + /// an object or group of objects, **and that effect is not taking any + /// actions on those objects**" — because a count ledger takes no actions on + /// the cards it counts. A Madness redirect (CR 702.35a: "that player + /// discards it, but exiles it instead") therefore counts here exactly as it + /// counts on the un-paused path, by construction rather than by a second + /// hand-written rule. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub preceding_events: Vec, +} + +/// What a paused discard batch still owes, by selection mode. +/// +/// CR 701.9b: "By default, effects that cause a player to discard a card allow +/// the affected player to choose which card to discard. Some effects, however, +/// require a random discard …" — the two modes differ only in how the next card +/// is chosen, which is the axis `CardSelectionMode` already names on +/// `Effect::Discard`. One parameterized cursor rather than two pending states. +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +#[serde(tag = "type")] +pub enum DiscardBatchCursor { + /// CR 701.9a: every remaining card of a forced whole-hand discard, in hand + /// order. Excludes the card whose replacement paused — that one is settled + /// by the replacement itself. + All { remaining: Vec }, + /// CR 701.9b: `remaining` further picks drawn at random from `pool`. + /// Mirrors `RandomDiscardOutcome::NeedsReplacementChoice`'s payload exactly. + Random { + pool: Vec, + remaining: usize, + }, + /// An announced ordered list of cards. Unlike `All`, these may belong to + /// different owners and their exact pre-move occurrences are part of the + /// instruction's identity. + Ordered { + remaining: Vec, + }, +} + +/// Terminal work coupled to a parked discard cursor. +#[derive(Clone, Debug, Default, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +#[serde(tag = "type")] +pub enum PendingDiscardBatchCompletion { + #[default] + Standard, + /// A player already selected these cards through `WaitingFor::DiscardChoice`. + /// Keep their incarnation references so neither an id-reused object nor a + /// later hand card can satisfy the original selection after a pause. + DiscardChoice { chosen: Vec }, +} + +/// The remainder of a `player_scope` discard clause whose fan-out was +/// interrupted. +/// +/// CR 608.2f: "Some spells and abilities include actions taken on multiple +/// players … If the action can't be processed simultaneously, it's instead +/// processed considering each affected player or object individually. APNAP +/// order is used to make the primary determination of the order of those +/// actions." A replacement choice is exactly what makes the discard action +/// non-simultaneous — but it is still ONE action, so the roster is held by the +/// batch rather than pushed onto `pending_continuation`, which is what lets the +/// whole clause publish ONE per-player table. +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +pub struct PendingDiscardFanOut { + /// The scoped template — `player_scope` already removed by + /// `split_player_scope_chain`. + pub scoped_template: Box, + /// The outer ability, which the shared clause postlude consults for + /// tracked-set and linked-exile decisions. + pub outer: Box, + pub original_controller: PlayerId, + /// CR 101.4: seats not yet iterated, in APNAP order. + pub remaining_players: Vec, + /// The clause's full reduction domain, for the terminal zero-fill. Latched + /// at the pause and never re-derived, for PARITY rather than by rule: the + /// un-paused driver computes this domain once at clause entry and never + /// re-derives it either, so a paused clause that re-derived would answer + /// differently from an identical un-paused one — the exact divergence this + /// carrier exists to prevent. CR 800.4i is what keeps a seat that has since + /// left the game well-defined here: "the effect uses the last known + /// information about that player before they left the game", and its + /// truthful contribution is zero. `elimination.rs` therefore prunes + /// `remaining_players` and deliberately leaves this list whole. + /// + /// (An earlier revision cited CR 608.2f for the latching. It does not + /// support it: read in full, 608.2f is about simultaneity and APNAP ORDER, + /// and both its examples are about ordering.) + pub matching_players: Vec, + /// CR 607.2a: carried across the pause so the terminal publication makes the + /// same linked-exile decision the un-paused postlude would. + /// + /// ORDERING NOTE for the driver's hand-off (`resolve_chain_body`), recorded + /// here rather than at the call site so the CR 603.5 prompt-census pin in + /// `engine.rs` — which pins a line coordinate in `effects/mod.rs` BELOW that + /// call site — does not have to be re-derived for a comment. The hand-off is + /// placed AFTER `mark_exile_choice_tracks_by_source` so the batch pause route + /// keeps the exact side-effect the ordinary per-seat leg route already had; + /// placing it before would make the two pause routes for one clause diverge. + /// Only that side-effect differs between the placements — this flag itself + /// survives either way, which is what this field is for. + /// + /// UNREACHABLE in the corpus, measured rather than assumed. A python walk of + /// `data/card-data.json` (35798 cards) finds 0 `player_scope` clause nodes + /// that carry BOTH a discard effect and a linked-exile consumer tag + /// (`LINKED_EXILE_CONSUMER_TAGS`, `game/exile_links.rs`) anywhere in the + /// clause subtree. The subtree strictly CONTAINS the tail + /// `split_player_scope_chain` detaches, so the filter over-approximates the + /// real condition and the zero is sound. Three positive controls on the same + /// walk, all non-zero: 1530 `player_scope` clause nodes of any effect, 202 of + /// those carrying a discard effect, 290 cards carrying a linked-exile tag. + /// That is why the flag-true x batch-pause combination has no fixture. + pub after_scope_needs_linked_exile: bool, +} + /// CR 101.4 + CR 701.23i: APNAP state for a self-library search instruction /// whose selected cards are delivered only after every searching player has /// made their private choice. The original spell's controller remains on @@ -8979,6 +9169,10 @@ fn visit_persisted_live_zone_changed_records( "consumed_before_priority_trigger_events", "pending_attack_trigger_events", "pending_player_scope_sacrifice_choice", + // `PendingDiscardBatch::preceding_events` holds this turn's `ZoneChanged` + // records, whose `turn_zone_change_index` must be rebound on load — + // exactly the reason its sacrifice sibling is listed above. + "pending_discard_batch", "stack", "waiting_for", "resolution_stack", @@ -14493,22 +14687,43 @@ impl StackEntryKind { } } -/// CR 608.2e: A clause-local snapshot of an equalization minimum/maximum, -/// frozen when a `player_scope` link begins so every player in that clause's -/// APNAP fan-out resolves its disposal count against the same pre-clause board. +/// CR 608.2h + CR 608.2e: A clause-local snapshot of a quantity whose answer is +/// determined only once, when the effect is applied (608.2h), frozen when a +/// `player_scope` link begins so every player in that clause's APNAP fan-out — +/// one action processed simultaneously (608.2e) — resolves against the same +/// pre-clause board. /// /// Balance's three clauses ("sacrifice lands", "discard cards", "sacrifice /// creatures") each compute an independent extremum at a different time. The /// `player_scope` driver re-resolves the effect's `count` expression on every /// per-player iteration; without a snapshot, after APNAP player 0 sacrifices /// down to the minimum, player 1 would recompute a smaller minimum. The -/// snapshot freezes only the cross-player aggregate (`ControlledByEachPlayer` / -/// `HandSize { AllPlayers }`); the per-player `left` operand still re-resolves -/// per iteration, which is correct. +/// snapshot freezes only the three clause-frozen classes +/// (`ControlledByEachPlayer` / `HandSize { AllPlayers }` / +/// `PreviousEffectAmount`, the last being a CR 608.2i look-back at a completed +/// instruction's result); the per-player `left` operand still re-resolves per +/// iteration, which is correct. +/// +/// CR 608.2i ends "This is an exception to 608.2h", which invites the reading +/// that a look-back is exempt from the snapshot rule outright. It is not, and +/// the distinction is what makes freezing `PreviousEffectAmount` correct rather +/// than contradictory. 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; it says nothing +/// about WHEN the number is determined. So 608.2h's "determined only once, when +/// the effect is applied" still governs the value — which is precisely the rule +/// a per-seat re-read violates. Were it otherwise, the pre-fix behaviour (every +/// seat re-stamping the shared scalar as its own draw completed) would have +/// been correct, and Windfall would rightly pay out the last discard rather +/// than the greatest. /// -/// Transient — never serialized. Captured before a `player_scope` link's -/// fan-out and cleared when the link completes, so the next clause re-enters -/// the driver with `None` and re-captures against the post-clause board. +/// Resolution-scoped, but persisted while a choice pauses the resolution. +/// Captured before a `player_scope` link's fan-out and cleared when the link +/// completes, so the next clause re-enters the driver with `None` and +/// re-captures against the post-clause board. A save during a replacement +/// choice must retain this frozen answer: the resumed clause is still the same +/// application of the effect, not a new time to determine it. /// /// # Single-cell invariant /// @@ -14525,7 +14740,7 @@ impl StackEntryKind { /// snapshot would be silently corrupted by the inner capture. At that point /// this field MUST become a `Vec` stack with /// push/pop bracketing each `player_scope` link entry/exit. -#[derive(Debug, Clone, Default)] +#[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct ClauseMinimumSnapshot { /// Reduced cross-player aggregates keyed by the originating quantity /// reference, so multiple distinct refs in one clause do not collide. @@ -16627,6 +16842,16 @@ declare_game_state! { /// `EffectZoneChoice`. #[serde(default, skip_serializing_if = "Option::is_none")] pub pending_player_scope_sacrifice_choice: Option, + /// CR 608.2c + CR 701.9a: a discard instruction parked by a + /// replacement-application choice. See [`PendingDiscardBatch`], whose doc + /// records why CR 616.1 does not govern this path. + /// + /// Boxed: `GameState` is moved by value through the phase-server action and + /// AI paths under a hard stack budget (`types/game_state_size.rs`), and this + /// payload is populated only during a pause — the shape that file says to + /// box. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub pending_discard_batch: Option>, /// CR 401.4: Remaining per-owner library-order batches for a mass /// `ChangeZoneAll` instruction paused on `EffectZoneChoice`. #[serde(default, skip_serializing_if = "Option::is_none")] @@ -16862,18 +17087,32 @@ declare_game_state! { /// preceding count-producing effect in the current ability chain. Used by /// carried-subject continuations like "Each player discards ..., then draws /// that many ..." after all players have completed the discard pass. + /// + /// CR 608.2i is what makes such a continuation legal at all, and its + /// precondition is worth quoting rather than paraphrasing: an effect may + /// look back at a previous game state "if such an effect requires + /// information from the game about an object or group of objects, **and + /// that effect is not taking any actions on those objects**". This table is + /// a pure count ledger — it takes no actions on the cards it counts — so the + /// exception to CR 608.2h holds. A consumer that instead wanted to ACT on + /// the counted objects would not be covered by 608.2i and must not be built + /// on this field. #[serde(default, skip_serializing_if = "HashMap::is_empty")] #[serde(serialize_with = "crate::types::deterministic_serde::hash_map")] pub last_effect_counts_by_player: HashMap, - /// CR 608.2e: Clause-local equalization snapshot. Each `player_scope` link - /// (e.g. a Balance clause) captures its cross-player extremum here before - /// the APNAP fan-out begins and clears it when the link completes, so every - /// player in that clause resolves against the same pre-clause board. The - /// per-link lifecycle is deliberately narrower than `last_vote_ballots`' - /// per-chain reset — three Balance clauses are three links in one chain and - /// must each snapshot independently. Transient. - #[serde(skip)] + /// CR 608.2h + CR 608.2e: Clause-local snapshot of the quantities whose + /// answer is determined only once, when the effect is applied. Each + /// `player_scope` link (e.g. a Balance clause, or Windfall's draw link) + /// captures its cross-player extremum or `PreviousEffectAmount` look-back + /// here before the APNAP fan-out begins and clears it when the link + /// completes, so every player in that clause resolves against the same + /// pre-clause board. The per-link lifecycle is deliberately narrower than + /// `last_vote_ballots`' per-chain reset — three Balance clauses are three + /// links in one chain and must each snapshot independently. Resolution- + /// scoped, but serialized across a paused resolution so the frozen value + /// survives authoritative save/restore. + #[serde(default, skip_serializing_if = "Option::is_none")] pub clause_minimum_snapshot: Option, /// CR 400.7 + CR 608.2c: Number of cards exiled from a hand by the most recent @@ -21377,6 +21616,7 @@ impl GameState { merged_card_component_route: None, resolution_coin_flip: None, pending_player_scope_sacrifice_choice: None, + pending_discard_batch: None, pending_mass_library_order_choice: None, pending_scoped_library_search: None, pending_library_search_delivery: None, @@ -23350,7 +23590,14 @@ fn _gamestate_partition_is_total(s: &GameState) { // explicitly. It is `None` at every sample beat (cleared whenever `waiting_for == // Priority`, effects/mod.rs:759) or a constant direct-assigned count across a real // copy-token loop, so COMPARING never suppresses a legitimate loop's detection. + // - `pending_discard_batch`: COMPARED (hand-written `impl PartialEq` conjunct) — a + // paused discard-batch interaction state, the direct sibling of + // `pending_player_scope_sacrifice_choice` above and classified identically. Its cursor + // only SHRINKS as the batch drains and it is `None` outside a pause, so a differing + // value is correctly not a fixed-point repeat and COMPARING it can never suppress a + // legitimate loop's detection. pending_player_scope_sacrifice_choice: _, + pending_discard_batch: _, pending_mass_library_order_choice: _, pending_scoped_library_search: _, pending_library_search_delivery: _, @@ -23564,6 +23811,7 @@ impl PartialEq for GameState { && self.resolution_coin_flip == other.resolution_coin_flip && self.pending_player_scope_sacrifice_choice == other.pending_player_scope_sacrifice_choice + && self.pending_discard_batch == other.pending_discard_batch && self.pending_mass_library_order_choice == other.pending_mass_library_order_choice && self.pending_scoped_library_search == other.pending_scoped_library_search @@ -25604,6 +25852,184 @@ mod tests { } } + /// A mid-pause discard batch carrying one current-turn `ZoneChanged`. + /// The event is the point: `preceding_events` is a LIVE event carrier, so + /// its records must be rebound on load like every other one. + fn parked_discard_batch(record: ZoneChangeRecord) -> Box { + Box::new(PendingDiscardBatch { + player: PlayerId(0), + cursor: DiscardBatchCursor::All { + remaining: vec![ObjectId(9_301), ObjectId(9_302)], + }, + completion: PendingDiscardBatchCompletion::Standard, + source_id: ObjectId(9_300), + effect_kind: crate::types::ability::EffectKind::Discard, + paused_card: ObjectIncarnationRef::of(ObjectId(9_303), 0), + discard_frame: None, + fan_out: None, + preceding_events: vec![persisted_zone_change_event(record)], + }) + } + + /// Registration surfaces 4 and 5 for `pending_discard_batch`: the + /// hand-written `PartialEq` conjunct, and membership in + /// `LIVE_EVENT_CARRIER_FIELDS`. + /// + /// REVERT PROBES: + /// * delete `"pending_discard_batch"` from `LIVE_EVENT_CARRIER_FIELDS` → + /// the visitor reaches no record and `erased > 0` fails inside + /// `erase_persisted_event_occurrence_fields`. + /// * delete the `self.pending_discard_batch == other.pending_discard_batch` + /// conjunct → the `assert_ne!` below sees two states as equal. + #[test] + fn parked_discard_batch_is_a_live_event_carrier_and_a_compared_field() { + let mut state = GameState::new_two_player(42); + state.turn_number = 19; + let record = persisted_zone_change_record(ObjectId(9_101), 19, 0); + state.zone_changes_this_turn.push_back(record.clone()); + state.pending_discard_batch = Some(parked_discard_batch(record)); + + let bare = { + let mut bare = state.clone(); + bare.pending_discard_batch = None; + bare + }; + assert_ne!( + state, bare, + "a paused discard batch is interaction state and must be COMPARED" + ); + + let mut persisted = serde_json::to_value(PersistedGameState::Raw(Box::new(state.clone()))) + .expect("fixture serializes"); + // Asserts internally that the traversal reached at least one record — + // which it can only do if the field is a declared live-event carrier. + erase_persisted_event_occurrence_fields(persisted_state_payload_mut(&mut persisted)); + let restored = serde_json::from_value::(persisted) + .expect("the parked batch's records reconcile") + .into_game_state(); + let batch = restored + .pending_discard_batch + .as_ref() + .expect("the parked batch survives the round trip"); + let GameEvent::ZoneChanged { record, .. } = &batch.preceding_events[0] else { + panic!("the fixture stores one ZoneChanged"); + }; + assert_eq!( + (record.recorded_turn_number, record.turn_zone_change_index), + (19, 0), + "the carried record is rebound to this turn's ledger on load" + ); + assert_eq!( + batch.cursor, + DiscardBatchCursor::All { + remaining: vec![ObjectId(9_301), ObjectId(9_302)], + }, + "the cursor round-trips unchanged" + ); + } + + /// Registration surface 1's save-compat property: a save written before this + /// field existed — or by any writer that skipped it — must load as `None` + /// and leave the state machine intact, rather than failing deserialization. + /// + /// NOT a revert probe for `#[serde(default)]`. MEASURED, not assumed: + /// deleting `default` from the declaration leaves this test and all 195 + /// `types::game_state::` tests green, because serde's derive already maps a + /// missing `Option` field to `None`. The attribute is redundant TODAY and is + /// kept only for symmetry with the sibling pause carriers + /// (`pending_player_scope_sacrifice_choice`, + /// `pending_mass_library_order_choice`, `pending_scoped_library_search`), + /// so a later reader does not "restore" it without this measurement. + /// + /// This is therefore a characterization test of the save-compat contract, + /// not a discriminator for the attribute. What it does discriminate is the + /// contract itself: make the field non-`Option` and `from_value` errors on + /// the `expect` below — which is also why `default` must NOT be sold as + /// insurance for that change. On a non-`Option` field it would fabricate a + /// `Default` for a missing key instead of failing loudly. + /// The state-machine half is pinned against a save taken GENUINELY + /// mid-pause. An earlier revision built this fixture at + /// `GameState::new_two_player`, whose `waiting_for` is already `Priority`, + /// and then asserted `Priority` after the round trip — restating an input + /// property, so it could not fail for the reason its message named. The + /// prompt below is therefore installed before serializing, which is the + /// only shape in which "the batch is missing but the prompt says paused" + /// can arise at all. + /// + /// REVERT PROBES: + /// * change `waiting_for` back to the default `Priority` before + /// serializing → the `ReplacementChoice` assertion below stops + /// discriminating (it passes for the wrong reason); the `matches!` on + /// the restored prompt fails outright, which is what makes the + /// mid-pause input load-bearing rather than decorative. + /// * add a load-time "repair" that resets `waiting_for` to `Priority` + /// when `pending_discard_batch` is absent, in + /// `PersistedGameState::into_game_state()` → the same assertion fails. + /// The load below deliberately goes through that chokepoint rather than + /// bare `from_value::`, because it is where this repo puts + /// load-time repairs; deserializing the struct directly would leave this + /// probe guarding a door no repair would ever come through. That repair + /// would be WRONG: it silently discards a real prompt and converts a + /// detectable inconsistency into a plausible-looking state. + #[test] + fn absent_pending_discard_batch_deserializes_as_none() { + let mut state = GameState::new_two_player(42); + state.turn_number = 19; + let record = persisted_zone_change_record(ObjectId(9_101), 19, 0); + state.zone_changes_this_turn.push_back(record.clone()); + state.pending_discard_batch = Some(parked_discard_batch(record)); + // The prompt a parked batch is waiting on. Without it the save is not + // mid-pause and this test measures nothing. CR 616.1 is accurate for THIS + // fixture, which builds a two-candidate ordering prompt; the production + // discard pause is a single optional apply-or-decline, which CR 616.1 does + // not govern. The carrier does not read the prompt's arity, so the + // persistence measured here is identical either way. + state.waiting_for = WaitingFor::ReplacementChoice { + player: PlayerId(0), + candidate_count: 2, + candidates: Vec::new(), + }; + assert!( + !matches!(state.waiting_for, WaitingFor::Priority { .. }), + "reach guard: the INPUT must not already satisfy the property under test" + ); + + // Loaded through `PersistedGameState::into_game_state()`, NOT bare + // `from_value::`. That chokepoint is where this repo puts + // load-time repairs, so it is the only door a "helpfully" reset + // `waiting_for` would come through; deserializing the struct directly + // would leave the second revert probe below guarding a door nobody uses. + let mut wire = serde_json::to_value(PersistedGameState::Raw(Box::new(state))) + .expect("fixture serializes"); + assert!( + wire.as_object_mut() + .expect("a raw persisted state is an object") + .remove("pending_discard_batch") + .is_some(), + "reach guard: the field must actually have been serialized to remove" + ); + + let restored = serde_json::from_value::(wire) + .expect("an absent parked batch defaults to None") + .into_game_state(); + assert!(restored.pending_discard_batch.is_none()); + assert!( + matches!( + restored.waiting_for, + WaitingFor::ReplacementChoice { + player, + candidate_count, + .. + } if player == PlayerId(0) && candidate_count == 2 + ), + "a batch-less mid-pause save must round-trip its prompt VERBATIM — player \ + AND candidate_count, since asserting only the variant would pass for a \ + prompt rebuilt with different contents — so the inconsistency stays \ + observable to a caller instead of being silently rewritten into a \ + plausible-looking state" + ); + } + #[test] fn persisted_zone_change_collision_rebinds_only_unique_ledger_records() { let mut state = normal_trigger_firing_fixture(); diff --git a/crates/engine/tests/fixtures/cr733/authority_matrix.json.gz b/crates/engine/tests/fixtures/cr733/authority_matrix.json.gz index f0916909f0..55302318ce 100644 Binary files a/crates/engine/tests/fixtures/cr733/authority_matrix.json.gz and b/crates/engine/tests/fixtures/cr733/authority_matrix.json.gz differ diff --git a/crates/engine/tests/integration/balance_equalization.rs b/crates/engine/tests/integration/balance_equalization.rs index 691d5b1db6..7cc5f3d605 100644 --- a/crates/engine/tests/integration/balance_equalization.rs +++ b/crates/engine/tests/integration/balance_equalization.rs @@ -21,14 +21,17 @@ use engine::game::ability_utils::build_resolved_from_def; use engine::game::effects::resolve_ability_chain; use engine::game::engine::apply; +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; use engine::game::zones::create_object; use engine::parser::oracle_effect::parse_effect_chain; use engine::types::ability::{AbilityKind, ResolvedAbility}; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; use engine::types::format::FormatConfig; -use engine::types::game_state::{GameState, WaitingFor}; +use engine::types::game_state::{GameState, PersistedGameState, WaitingFor}; use engine::types::identifiers::{CardId, ObjectId}; +use engine::types::mana::ManaCost; +use engine::types::phase::Phase; use engine::types::player::PlayerId; use engine::types::zones::Zone; @@ -358,3 +361,67 @@ fn balance_three_player_interactive_fan_out_equalizes() { ); } } + +/// CR 608.2h: Balance's cross-player hand-size minimum is determined once when +/// the effect is applied; persisting it across the pause preserves that value. +/// +/// Balance's discard choice pauses after its cross-player hand-size minimum has +/// been frozen. The authoritative save/restore path must preserve that +/// still-live value: resume continues the same application rather than +/// determining a new minimum. +/// +/// Discriminating: P0's three-card hand must discard down to P1's one-card +/// minimum. The test saves only after the real cast pipeline reaches the +/// `DiscardChoice` that selects P0's two discards; removing the snapshot's +/// serde support leaves the restored reach guard empty before the resumed +/// production path runs. +#[test] +fn balance_save_during_discard_choice_preserves_frozen_hand_minimum() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + for i in 0..3 { + scenario.add_card_to_hand(P0, &format!("P0 hand card {i}")); + } + scenario.add_card_to_hand(P1, "P1 hand card"); + let balance = scenario + .add_spell_to_hand_from_oracle(P0, "Balance", false, BALANCE_ORACLE) + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + + runner.cast(balance).resolve(); + assert!( + matches!(runner.state().waiting_for, WaitingFor::DiscardChoice { .. }) + && runner.state().clause_minimum_snapshot.is_some(), + "reach guard: the production cast must park Balance's discard choice with its frozen hand minimum" + ); + + let saved = serde_json::to_string(&PersistedGameState::capture(runner.state().clone())) + .expect("the authoritative paused state serializes"); + let restored: PersistedGameState = + serde_json::from_str(&saved).expect("the authoritative paused state deserializes"); + let mut runner = GameRunner::from_state(restored.into_game_state()); + assert!( + runner.state().clause_minimum_snapshot.is_some(), + "the paused discard clause's frozen hand minimum must survive authoritative restore" + ); + + let mut prompts = 0; + while let WaitingFor::DiscardChoice { cards, count, .. } = runner.state().waiting_for.clone() { + runner + .act(GameAction::SelectCards { + cards: cards.into_iter().take(count).collect(), + }) + .expect("selecting Balance's required discards must resume the cast"); + prompts += 1; + } + runner.advance_until_stack_empty(); + + assert_eq!(prompts, 1, "P0's two required discards share one choice"); + assert_eq!(hand_len(runner.state(), P0), 1, "P0 must discard down to 1"); + assert_eq!( + hand_len(runner.state(), P1), + 1, + "P1 was already at the minimum" + ); +} diff --git a/crates/engine/tests/integration/chain_of_smog_copy.rs b/crates/engine/tests/integration/chain_of_smog_copy.rs index cd390ad81b..fc801f7be0 100644 --- a/crates/engine/tests/integration/chain_of_smog_copy.rs +++ b/crates/engine/tests/integration/chain_of_smog_copy.rs @@ -24,7 +24,7 @@ use engine::game::scenario::{GameScenario, P0, P1}; use engine::game::scenario_db::GameScenarioDbExt; use engine::types::ability::{CopyRetargetPermission, Effect}; use engine::types::actions::GameAction; -use engine::types::game_state::WaitingFor; +use engine::types::game_state::{PersistedGameState, WaitingFor}; use engine::types::identifiers::ObjectId; use engine::types::mana::{ManaType, ManaUnit}; use engine::types::phase::Phase; @@ -285,6 +285,185 @@ fn chain_of_smog_copy_controlled_by_targeted_player_and_retargeted() { ); } +#[test] +fn chain_of_smog_discard_choice_resumes_selected_tail_after_library_of_leng() { + let Some(db) = load_db() else { + return; + }; + + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let smog = scenario.add_real_card(P0, "Chain of Smog", Zone::Hand, db); + scenario.add_real_card(P1, "Library of Leng", Zone::Battlefield, db); + for _ in 0..3 { + scenario.add_card_to_hand(P1, "Mountain"); + } + + let mut runner = scenario.build(); + engine::game::rehydrate_game_from_card_db(runner.state_mut(), db); + add_mana(&mut runner, P0, &[ManaType::Black, ManaType::Colorless]); + let card_id = runner.state().objects[&smog].card_id; + runner + .act(GameAction::CastSpell { + object_id: smog, + card_id, + targets: vec![], + payment_mode: CastPaymentMode::Auto, + }) + .expect("cast Chain of Smog"); + runner + .act(GameAction::SelectTargets { + targets: vec![engine::types::ability::TargetRef::Player(P1)], + }) + .expect("target P1"); + + for _ in 0..16 { + match runner.state().waiting_for.clone() { + WaitingFor::DiscardChoice { count, cards, .. } => { + runner + .act(GameAction::SelectCards { + cards: cards.into_iter().take(count).collect(), + }) + .expect("select two cards to discard"); + break; + } + WaitingFor::Priority { .. } => { + runner + .act(GameAction::PassPriority) + .expect("advance Chain of Smog"); + } + waiting_for => panic!("expected discard choice, got {waiting_for:?}"), + } + } + + assert!(matches!( + runner.state().pending_discard_batch.as_deref(), + Some(engine::types::game_state::PendingDiscardBatch { + completion: engine::types::game_state::PendingDiscardBatchCompletion::DiscardChoice { chosen }, + .. + }) if chosen.len() == 2 + )); + let saved = serde_json::to_string(&PersistedGameState::capture(runner.state().clone())).expect( + "paused selected discard serializes through the authoritative persistence envelope", + ); + let restored: PersistedGameState = serde_json::from_str(&saved) + .expect("paused selected discard restores through the authoritative persistence envelope"); + let restored = restored.into_game_state(); + assert!(matches!( + restored.pending_discard_batch.as_deref(), + Some(engine::types::game_state::PendingDiscardBatch { + completion: engine::types::game_state::PendingDiscardBatchCompletion::DiscardChoice { chosen }, + .. + }) if chosen.len() == 2 + )); + let mut runner = engine::game::scenario::GameRunner::from_state(restored); + + let mut replacement_events = Vec::new(); + for _ in 0..2 { + let WaitingFor::ReplacementChoice { candidates, .. } = runner.state().waiting_for.clone() + else { + panic!("each selected card must reach Library of Leng's replacement choice"); + }; + let decline = candidates + .iter() + .position(|candidate| candidate.description == "Decline") + .expect("Library of Leng supplies a decline choice"); + let result = runner + .act(GameAction::ChooseReplacement { index: decline }) + .expect("decline Library of Leng"); + replacement_events.extend(result.events); + } + + assert!(runner.state().pending_discard_batch.is_none()); + assert_eq!(runner.state().last_effect_count, Some(2)); + assert_eq!( + replacement_events + .iter() + .filter(|event| matches!( + event, + engine::types::events::GameEvent::EffectResolved { + kind: engine::types::ability::EffectKind::Discard, + source_id, + .. + } if *source_id == smog + )) + .count(), + 1, + "the resumed selected discard emits its terminal marker exactly once" + ); + assert!(matches!( + runner.state().waiting_for, + WaitingFor::OptionalEffectChoice { player: P1, .. } + )); + assert_eq!( + hand_size(&runner, P1), + 1, + "both selected cards must discard before Chain of Smog's copy continuation" + ); +} + +#[test] +fn chain_of_smog_discard_choice_rejects_a_duplicate_card_submission() { + let Some(db) = load_db() else { + return; + }; + + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let smog = scenario.add_real_card(P0, "Chain of Smog", Zone::Hand, db); + for _ in 0..3 { + scenario.add_card_to_hand(P1, "Mountain"); + } + + let mut runner = scenario.build(); + engine::game::rehydrate_game_from_card_db(runner.state_mut(), db); + add_mana(&mut runner, P0, &[ManaType::Black, ManaType::Colorless]); + let card_id = runner.state().objects[&smog].card_id; + runner + .act(GameAction::CastSpell { + object_id: smog, + card_id, + targets: vec![], + payment_mode: CastPaymentMode::Auto, + }) + .expect("cast Chain of Smog"); + runner + .act(GameAction::SelectTargets { + targets: vec![engine::types::ability::TargetRef::Player(P1)], + }) + .expect("target P1"); + + let (count, duplicate) = loop { + match runner.state().waiting_for.clone() { + WaitingFor::DiscardChoice { count, cards, .. } => break (count, cards[0]), + WaitingFor::Priority { .. } => { + runner + .act(GameAction::PassPriority) + .expect("advance Chain of Smog"); + } + waiting_for => panic!("expected discard choice, got {waiting_for:?}"), + } + }; + assert_eq!(count, 2, "Chain of Smog requires two distinct discards"); + + let error = runner + .act(GameAction::SelectCards { + cards: vec![duplicate, duplicate], + }) + .expect_err("one card cannot satisfy Chain of Smog's two-card discard"); + assert!(matches!( + error, + engine::game::EngineError::InvalidAction(message) + if message == "Selected cards must be distinct" + )); + assert!(matches!( + runner.state().waiting_for, + WaitingFor::DiscardChoice { count: 2, .. } + )); + assert_eq!(runner.state().objects[&duplicate].zone, Zone::Hand); + assert_eq!(hand_size(&runner, P1), 3); +} + // --------------------------------------------------------------------------- // Runtime: the copy is itself a Chain of Smog and carries the same nested // optional copy — accepting the re-offered copy must produce a diff --git a/crates/engine/tests/integration/coalition_relic_integration.rs b/crates/engine/tests/integration/coalition_relic_integration.rs index ea673a4e5e..36d754b977 100644 --- a/crates/engine/tests/integration/coalition_relic_integration.rs +++ b/crates/engine/tests/integration/coalition_relic_integration.rs @@ -34,8 +34,8 @@ use engine::game::effects; use engine::game::scenario::{GameScenario, P0}; use engine::game::zones::create_object; use engine::types::ability::{ - AbilityCondition, AbilityKind, DamageChannel, Effect, ManaContribution, ManaProduction, - QuantityExpr, QuantityRef, ResolvedAbility, TargetFilter, TargetRef, + AbilityCondition, AbilityKind, AggregateFunction, DamageChannel, Effect, ManaContribution, + ManaProduction, QuantityExpr, QuantityRef, ResolvedAbility, TargetFilter, TargetRef, }; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; @@ -64,9 +64,10 @@ fn build_coalition_relic_drain(controller: PlayerId, source: ObjectId) -> Resolv produced: ManaProduction::AnyOneColor { count: QuantityExpr::Ref { // CR 608.2c: the counters-removed count is a TOTAL-channel - // amount (CR 120.6) — the excess channel is damage-only. + // amount — the excess channel is damage-only. qty: QuantityRef::PreviousEffectAmount { channel: DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, color_options: vec![ diff --git a/crates/engine/tests/integration/excess_damage_quantity_channel.rs b/crates/engine/tests/integration/excess_damage_quantity_channel.rs index 70368cd095..706b63f36e 100644 --- a/crates/engine/tests/integration/excess_damage_quantity_channel.rs +++ b/crates/engine/tests/integration/excess_damage_quantity_channel.rs @@ -31,7 +31,7 @@ use engine::game::quantity::resolve_quantity; use engine::game::scenario::{GameScenario, P0, P1}; use engine::parser::parse_oracle_text; -use engine::types::ability::{DamageChannel, Effect, QuantityExpr, QuantityRef}; +use engine::types::ability::{AggregateFunction, DamageChannel, Effect, QuantityExpr, QuantityRef}; use engine::types::game_state::{CastOfferKind, WaitingFor}; use engine::types::mana::ManaCost; use engine::types::phase::Phase; @@ -152,6 +152,7 @@ fn total_channel_is_unchanged_and_still_reads_last_effect_amount() { let total = QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }; let resolved = resolve_quantity(runner.state(), &total, P0, source); diff --git a/crates/engine/tests/integration/issue_6858_draw_that_many_discard.rs b/crates/engine/tests/integration/issue_6858_draw_that_many_discard.rs index 190ecbccc6..c1d6305354 100644 --- a/crates/engine/tests/integration/issue_6858_draw_that_many_discard.rs +++ b/crates/engine/tests/integration/issue_6858_draw_that_many_discard.rs @@ -17,8 +17,8 @@ use engine::game::scenario::{GameRunner, GameScenario, P0}; use engine::types::ability::{ - AbilityDefinition, AbilityKind, CardSelectionMode, DamageChannel, Effect, QuantityExpr, - QuantityRef, TargetFilter, + AbilityDefinition, AbilityKind, AggregateFunction, CardSelectionMode, DamageChannel, Effect, + QuantityExpr, QuantityRef, TargetFilter, }; use engine::types::actions::GameAction; use engine::types::game_state::WaitingFor; @@ -63,6 +63,7 @@ fn draw_then_discard_that_many(draw: Effect) -> AbilityDefinition { count: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, target: TargetFilter::Controller, diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 15a8e4bdff..d234cd72ea 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -1383,6 +1383,7 @@ mod wheel_of_misfortune_secret_numbers; mod where_x_coverage_runtime; mod where_x_quantity_channel_binds; mod where_x_totality_guard; +mod windfall_greatest_discard_aggregate; mod winding_way_reveal_partition_2931; mod witchs_oven_food_tokens; mod xantid_swarm_defending_player_cant_cast; diff --git a/crates/engine/tests/integration/random_discard_cost_replacement_resume.rs b/crates/engine/tests/integration/random_discard_cost_replacement_resume.rs index fbdb1031c8..68c47e3857 100644 --- a/crates/engine/tests/integration/random_discard_cost_replacement_resume.rs +++ b/crates/engine/tests/integration/random_discard_cost_replacement_resume.rs @@ -164,6 +164,14 @@ fn random_discard_cost_resumes_its_payment_after_an_accepted_replacement() { runner.state().pending_cost_move_resume.is_some(), "the unless-payment continuation must be persisted while the choice is open" ); + // NEGATIVE, paired with the positive reach guard directly above: the EFFECT + // layer's batch carrier must stay empty for a COST payment. The assertion is + // non-vacuous precisely because the line above proves a random discard batch + // really did pause here — only the layer differs. + assert!( + runner.state().pending_discard_batch.is_none(), + "a cost-layer random discard must not park an EFFECT batch (DiscardCause::Cost)" + ); let accept_idx = candidates .iter() .position(|c| c.description == "Accept") @@ -316,3 +324,82 @@ fn random_discard_cost_with_no_cards_still_sacrifices() { "nothing may be left parked" ); } + +/// Hymn to Tourach's printed Oracle text (Scryfall, verified verbatim +/// 2026-08-16) — the EFFECT layer's multi-pick random discard, and the only +/// selection mode besides the forced whole-hand branch that can lose cards to a +/// mid-batch pause. +const HYMN_TO_TOURACH: &str = "Target player discards two cards at random."; + +/// CR 701.9b + CR 608.2c: an EFFECT-caused random discard that pauses on its +/// first pick must still make its second one. +/// +/// This is the random sibling of the forced whole-hand arm in +/// `windfall_greatest_discard_aggregate.rs`. The gate-2 `Moved` redirect fires +/// on every VICTIM card, so the batch pauses on pick 1, resumes, pauses again on +/// pick 2, and resumes — two prompts, two cards gone. +/// +/// FIXTURE NOTE (measured, not assumed). The redirect is narrowed with +/// `Not { SpecificObject { id: hymn } }` rather than left at `valid_card: None`. +/// An unnarrowed redirect also watches the SPELL's own CR 608.2n stack → +/// graveyard move, and that move happens while the first pick's choice is still +/// parked: `pending_replacement` is a single slot, so the spell's move overwrote +/// the victim's parked choice and the victim never left the hand. Measured on +/// this fixture at this tip — prompt #0's `pending_replacement` was +/// `ZoneChange { object_id: , from: Stack, to: Graveyard }`, and the +/// first-picked victim stayed in `Zone::Hand` for the rest of the game. The +/// overwrite was already in place at prompt #0, i.e. before any resume code +/// runs, so it is a single-slot `pending_replacement` defect independent of the +/// batch cursor this test covers. It is NOT repaired here; the fixture excludes +/// the spell instead of silently absorbing it. +/// +/// Discriminating: at the pre-fix tip the effect layer threw the returned cursor +/// away, so exactly ONE card left the hand and exactly ONE prompt was raised. +/// The prompt count is the reach guard — a run that raised no prompt never +/// exercised the pause path and could satisfy a bare zone check for the wrong +/// reason. +#[test] +fn effect_random_discard_finishes_its_batch_after_a_replacement_pause() { + let mut scenario = GameScenario::new(); + scenario.at_phase(engine::types::phase::Phase::PreCombatMain); + let hymn = scenario + .add_spell_to_hand_from_oracle(P0, "Hymn to Tourach", false, HYMN_TO_TOURACH) + .with_mana_cost(engine::types::mana::ManaCost::zero()) + .id(); + scenario + .add_creature(P1, "Graveyard Warden", 1, 1) + .with_replacement_definition(optional_graveyard_exile_replacement().valid_card( + TargetFilter::Not { + filter: Box::new(TargetFilter::SpecificObject { id: hymn }), + }, + )); + let hand: Vec = (0..4) + .map(|i| scenario.add_card_to_hand(P1, &format!("Victim Card {i}"))) + .collect(); + let mut runner = scenario.build(); + + runner.cast(hymn).target_player(P1).resolve(); + + let mut prompts = 0; + for _ in 0..16 { + let WaitingFor::ReplacementChoice { candidates, .. } = runner.state().waiting_for.clone() + else { + break; + }; + let decline = candidates + .iter() + .position(|c| c.description == "Decline") + .expect("a Decline option"); + prompts += 1; + runner + .act(GameAction::ChooseReplacement { index: decline }) + .expect("declining the redirect must be accepted"); + } + runner.advance_until_stack_empty(); + + assert_eq!( + (prompts, moved_out_of_hand(&runner, &hand)), + (2, 2), + "both random picks must be made across the pause (prompts, cards gone)" + ); +} diff --git a/crates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs b/crates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs index 16172bf626..e46e4134f6 100644 --- a/crates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs +++ b/crates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs @@ -3,7 +3,9 @@ //! tests drive the real Oracle parser and trigger-resolution pipeline. use engine::game::scenario::{GameScenario, P0, P1}; -use engine::types::ability::{DamageChannel, Effect, PlayerFilter, QuantityExpr, QuantityRef}; +use engine::types::ability::{ + AggregateFunction, DamageChannel, Effect, PlayerFilter, QuantityExpr, QuantityRef, +}; use engine::types::game_state::{ExileLink, ExileLinkKind}; use engine::types::identifiers::ObjectId; use engine::types::phase::Phase; @@ -62,6 +64,7 @@ fn assert_queued_total_damage_continuation( amount: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: DamageChannel::Total, + aggregate: AggregateFunction::Sum, }, }, player_filter: PlayerFilter::Opponent, diff --git a/crates/engine/tests/integration/windfall_greatest_discard_aggregate.rs b/crates/engine/tests/integration/windfall_greatest_discard_aggregate.rs new file mode 100644 index 0000000000..b4d51e3a08 --- /dev/null +++ b/crates/engine/tests/integration/windfall_greatest_discard_aggregate.rs @@ -0,0 +1,796 @@ +//! Windfall's cross-player MAX aggregate — "the greatest number of cards a +//! player discarded this way". +//! +//! Oracle (Scryfall, verified verbatim 2026-08-15): +//! "Each player discards their hand, then draws cards equal to the greatest +//! number of cards a player discarded this way." +//! +//! Class: Windfall, Jace's Archivist, Whispering Madness — identical text. +//! +//! CR 608.2e: the discard action is processed simultaneously for every player, +//! then the draw action reads that completed action's result. +//! CR 608.2h: the draw count is determined ONCE, when the draw action is +//! applied — not re-derived per player as the fan-out proceeds. +//! CR 608.2i: that determination is a look-back at the already-completed +//! discard action, the exception to CR 608.2h this clause relies on. +//! CR 701.9a: to discard a card is to move it from hand to graveyard. +//! CR 121.2: drawing N cards is N individual card draws. +//! +//! The regression this pins: the engine reduces the per-player discard counts to +//! ONE untyped scalar whose aggregate lived on the PRODUCER. With the producer +//! set to a cross-player SUM, Windfall drew 8+7+3+3 = 21 for every player +//! instead of the greatest single player's 8. + +use engine::game::engine::apply; +use engine::game::scenario::{GameRunner, GameScenario, Outcome, P0, P1}; +use engine::types::ability::{ + AbilityDefinition, AbilityKind, Effect, ReplacementDefinition, ReplacementMode, + ResolvedAbility, TargetFilter, TargetRef, +}; +use engine::types::actions::GameAction; +use engine::types::card_type::CoreType; +use engine::types::game_state::{PersistedGameState, WaitingFor}; +use engine::types::identifiers::ObjectId; +use engine::types::mana::ManaCost; +use engine::types::phase::Phase; +use engine::types::player::PlayerId; +use engine::types::replacements::ReplacementEvent; +use engine::types::zones::Zone; + +const WINDFALL: &str = "Each player discards their hand, then draws cards equal to the greatest number of cards a player discarded this way."; + +/// Syphon Mind's shape — the NON-superlative "discarded this way" neighbour. +/// +/// This does NOT guard the aggregate axis, and an earlier revision of this file +/// claimed that it did. Syphon Mind parses to `FilteredTrackedSetSize` and +/// carries no `PreviousEffectAmount` node at all, so it is structurally +/// incapable of detecting a change to `QuantityRef::PreviousEffectAmount`'s +/// aggregate — measured: it stays green under BOTH the aggregate revert and the +/// clause-freeze revert. What it does guard is real and worth keeping: that the +/// superlative combinator did not STEAL the non-superlative phrasing, i.e. this +/// card still reaches `FilteredTrackedSetSize` and still sums. +/// +/// The aggregate axis is guarded at unit level instead — see +/// `game/quantity.rs`'s `previous_effect_amount_live_when_no_snapshot` and +/// `previous_effect_amount_aggregates_are_mutually_distinct`. Measured: no card +/// in the **Sum class** yields a clean integration-level Sum-vs-Max +/// discriminator — the Max class does, and it is the first test in this file. +const SYPHON_MIND: &str = + "Each other player discards a card. You draw a card for each card discarded this way."; + +/// Blood Tithe — the drain shape, and the class the corpus actually populates. +/// Measured: 44 cards carry both a `player_scope` and a `PreviousEffectAmount` +/// somewhere; 3 hold it only outside the scoped subtree, in a condition. Of the +/// 41 that hold it inside, in a quantity position, 38 carry it on a `GainLife` — +/// this `LoseLife` → `GainLife { PreviousEffectAmount }` form. +/// +/// Unlike Syphon Mind this DOES build `PreviousEffectAmount`, with `aggregate` +/// absent and therefore `Sum`. CR 119.3: an effect causing a player to gain or +/// lose life adjusts that life total accordingly — one rule covers both +/// directions here. "The life lost this way" is the cross-player TOTAL, 9. +/// +/// It is a REACH guard, not an aggregate discriminator: `Effect::LoseLife` +/// publishes no per-player table, so `Max`/`Min` fall back to the total and all +/// three reductions coincide at 9. Measured, not reasoned — see the degeneracy +/// note on the test itself. +const BLOOD_TITHE: &str = + "Each opponent loses 3 life. You gain life equal to the life lost this way."; + +const P2: PlayerId = PlayerId(2); +const P3: PlayerId = PlayerId(3); +const SEATS: [PlayerId; 4] = [P0, P1, P2, P3]; + +/// Deep enough that no draw in these tests is library-limited. +const LIBRARY_DEPTH: usize = 60; + +fn seed_library(scenario: &mut GameScenario, player: PlayerId, n: usize) { + for i in 0..n { + scenario.add_card_to_library_top(player, &format!("Filler {i}")); + } +} + +fn seed_hand(scenario: &mut GameScenario, player: PlayerId, n: usize) { + for i in 0..n { + scenario.add_card_to_hand(player, &format!("Hand Filler {i}")); + } +} + +fn zone_len(outcome: &Outcome, player: PlayerId, zone: Zone) -> usize { + let p = outcome + .state() + .players + .iter() + .find(|p| p.id == player) + .expect("player exists"); + match zone { + Zone::Hand => p.hand.len(), + Zone::Library => p.library.len(), + Zone::Graveyard => p.graveyard.len(), + other => panic!("zone_len does not cover {other:?}"), + } +} + +/// CR 608.2e + CR 121.2: four seats, hands 8/7/3/3 (the USER-reported board). +/// CR 608.2h: the greatest number of cards any one player discarded is 8 and is +/// determined once when the draw action is applied, so EVERY player draws +/// exactly 8. +/// +/// P0's eight are the cards held BESIDE Windfall: CR 601.2a removes the spell +/// from hand when the cast commits to the stack, so it is not itself discarded. +/// +/// Non-vacuous and discriminating: the four hand sizes make MAX (8), SUM (21), +/// MIN (3), and per-player (8/7/3/3) four mutually distinguishable outcomes, so +/// the assertion fails under every wrong aggregate, not merely the one that +/// shipped. The graveyard assertion is the reach guard — it proves the discard +/// step actually ran, so a spell that failed to parse or resolve cannot pass a +/// bare hand-size check for the wrong reason. +#[test] +fn windfall_draws_the_greatest_single_players_discard_not_the_cross_player_sum() { + let mut scenario = GameScenario::new_n_player(4, 42); + scenario.at_phase(Phase::PreCombatMain); + for (seat, hand) in SEATS.iter().zip([8usize, 7, 3, 3]) { + seed_hand(&mut scenario, *seat, hand); + seed_library(&mut scenario, *seat, LIBRARY_DEPTH); + } + let windfall = scenario + .add_spell_to_hand_from_oracle(P0, "Windfall", false, WINDFALL) + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(windfall).resolve(); + + let drawn: Vec = SEATS + .iter() + .map(|p| LIBRARY_DEPTH - zone_len(&outcome, *p, Zone::Library)) + .collect(); + let hands: Vec = SEATS + .iter() + .map(|p| zone_len(&outcome, *p, Zone::Hand)) + .collect(); + let graveyards: Vec = SEATS + .iter() + .map(|p| zone_len(&outcome, *p, Zone::Graveyard)) + .collect(); + + // CR 701.9a reach guard: every player really did discard their whole hand. + assert!( + graveyards[0] >= 8 && graveyards[1] >= 7 && graveyards[2] >= 3 && graveyards[3] >= 3, + "reach guard: each player's hand must have reached the graveyard, got {graveyards:?}" + ); + assert_eq!( + drawn, + vec![8, 8, 8, 8], + "each player draws the GREATEST single-player discard (8), not the cross-player sum (21)" + ); + assert_eq!( + hands, + vec![8, 8, 8, 8], + "each hand holds exactly the freshly drawn cards" + ); +} + +/// NON-INTERFERENCE, not an aggregate guard. Syphon Mind in a four-player game: +/// the three other players each discard one card and the controller draws 3. +/// +/// What this discriminates: that the superlative combinator did not swallow the +/// non-superlative "discarded this way" phrasing — this card must still reach +/// `FilteredTrackedSetSize` and still sum. What it does NOT discriminate: the +/// aggregate axis. Syphon Mind builds no `PreviousEffectAmount` node, so it +/// cannot see a change to that ref's `aggregate` and stays green under both +/// revert arms. The cross-aggregate guard lives at unit level, in +/// `game/quantity.rs`'s `previous_effect_amount_aggregates_are_mutually_distinct` +/// and `previous_effect_amount_live_when_no_snapshot` — no card in the **Sum +/// class** gives a clean integration-level Sum-vs-Max discriminator. (The Max +/// class does: `windfall_draws_the_greatest_single_players_discard_not_the_cross_player_sum` +/// above separates MAX 8 / SUM 21 / MIN 3. The gap is specific to the Sum class, +/// whose producers publish no per-player table for an aggregate to reduce over.) +#[test] +fn syphon_mind_shape_still_draws_the_cross_player_total() { + let mut scenario = GameScenario::new_n_player(4, 42); + scenario.at_phase(Phase::PreCombatMain); + for seat in SEATS { + seed_hand(&mut scenario, seat, 1); + seed_library(&mut scenario, seat, LIBRARY_DEPTH); + } + let syphon = scenario + .add_spell_to_hand_from_oracle(P0, "Syphon Mind", false, SYPHON_MIND) + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(syphon).resolve(); + + let drawn = LIBRARY_DEPTH - zone_len(&outcome, P0, Zone::Library); + let opponents_discarded: usize = [P1, P2, P3] + .iter() + .map(|p| zone_len(&outcome, *p, Zone::Graveyard)) + .sum(); + + // Reach guard: the discard step ran for all three opponents (CR 701.9a). + assert_eq!( + opponents_discarded, 3, + "reach guard: each of the three other players must discard one card" + ); + assert_eq!( + drawn, 3, + "controller draws one per card discarded across all opponents (sum), not the max (1)" + ); +} + +/// CR 608.2c: a zero-contributor board must not disturb the Max class. +/// +/// Board 8/7/3/**0** — P3 has an empty hand, so "each player discards their +/// hand" emits no discard event for them and the event-built table arrives as +/// `{8,7,3}` with P3 absent. The producer fills that gap with a 0 so an +/// aggregate reduces over every subject. +/// +/// SCOPE — this asserts the NON-REGRESSION half only: the greatest discard is +/// still 8, so every player including the empty-handed one still draws 8. It +/// does NOT assert the table's contents, and deliberately so: by the time +/// `outcome.state()` is readable the table is already `[]` regardless of the +/// fix. An earlier revision asserted on it and failed with `left: []` — an +/// INSTRUMENT failure, not a fix failure. +/// +/// The clearer is this card's OWN draw tail, not the player-action boundary: +/// `Effect::Draw` is not a count producer, so its postlude calls +/// `install_previous_effect_counts_by_player(.., None, ..)` and takes the arm +/// that clears — inside the same resolution, long before `apply()`'s +/// start-of-action clear could matter. Two tests bracket this: a bare `Discard` +/// fan-out with no tail leaves the table populated +/// (`game/effects/mod.rs`'s `player_scope_fan_out_publishes_a_zero_for_the_empty_handed_seat`), +/// and adding the draw tail — this test — empties it. +/// +/// The table's contents are therefore asserted where they survive, at unit +/// level: that same production-wire test pins the zero entry, and +/// `game/quantity.rs`'s `previous_effect_amount_min_counts_the_zero_contributor` +/// pins `Min` at 0 filled versus 3 unfilled. +#[test] +fn windfall_zero_contributor_board_still_draws_the_greatest() { + let mut scenario = GameScenario::new_n_player(4, 42); + scenario.at_phase(Phase::PreCombatMain); + seed_hand(&mut scenario, P0, 8); + seed_hand(&mut scenario, P1, 7); + seed_hand(&mut scenario, P2, 3); + // P3: no hand at all — the zero contributor. + for seat in SEATS { + seed_library(&mut scenario, seat, LIBRARY_DEPTH); + } + let windfall = scenario + .add_spell_to_hand_from_oracle(P0, "Windfall", false, WINDFALL) + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(windfall).resolve(); + + let drawn: Vec = SEATS + .iter() + .map(|p| LIBRARY_DEPTH - zone_len(&outcome, *p, Zone::Library)) + .collect(); + let graveyards: Vec = SEATS + .iter() + .map(|p| zone_len(&outcome, *p, Zone::Graveyard)) + .collect(); + + // CR 701.9a reach guard: the discard step ran, and P3 really contributed + // nothing — without this, an all-8 draw could pass on a board that never + // had a zero contributor at all. + assert_eq!( + graveyards[3], 0, + "reach guard: P3 must be the zero contributor, got {graveyards:?}" + ); + assert!( + graveyards[0] >= 8, + "reach guard: the discard step must have run, got {graveyards:?}" + ); + assert_eq!( + drawn, + vec![8, 8, 8, 8], + "non-regression: the greatest discard is still 8, so every player draws 8" + ); +} + +/// REACH + non-regression guard for the drain class — NOT an aggregate +/// discriminator. Read the measured degeneracy below before trusting it as one. +/// +/// Blood Tithe in a four-player game: each of the three opponents loses 3 life, +/// so "the life lost this way" is 3 + 3 + 3 = 9 (CR 119.3) and the controller +/// gains 9. This is the shape 38 of the 41 quantity-position corpus carriers +/// take (see `BLOOD_TITHE`'s note for the full split), so it is the widest +/// non-regression this file has. +/// +/// WHAT IT DISCRIMINATES, measured by sentinel probe: the ref is genuinely +/// reached — forcing an early `return 999` at the top of the +/// `QuantityRef::PreviousEffectAmount` arm moves this card to 1019 life. So a +/// change that stopped routing the drain class through that arm fails here. +/// +/// WHAT IT DOES **NOT** DISCRIMINATE: the aggregate axis. `Effect::LoseLife` +/// publishes no per-player breakdown — only `Discard` / `DiscardCard` / +/// `ChangeZoneAll` populate `last_effect_counts_by_player` — so the table is +/// EMPTY here and `Max`/`Min` both fall back to `unwrap_or(total)`. All three +/// reductions coincide: +/// +/// Sum -> 9 Max -> 9 Min -> 9 (degenerate) +/// +/// Measured, not reasoned: forcing `AggregateFunction::Sum => per_player.max() +/// .unwrap_or(total)` leaves this test green at 29. An earlier revision of this +/// comment claimed `Max -> 3` and that a global flip would fail here. That was +/// wrong, and it is the same error as the Syphon Mind control above — a +/// discriminating claim derived from the parse tree and never revert-probed. +/// +/// The aggregate axis IS discriminated, at unit level where a populated table +/// can be constructed directly: `game/quantity.rs`'s +/// `previous_effect_amount_live_when_no_snapshot` asserts `Max` = 8 over +/// `{P0:8, P1:3}` with `last_effect_amount` = 11, so `Sum` fails it. +#[test] +fn blood_tithe_drain_still_gains_the_cross_player_total() { + let mut scenario = GameScenario::new_n_player(4, 42); + scenario.at_phase(Phase::PreCombatMain); + for seat in SEATS { + seed_library(&mut scenario, seat, LIBRARY_DEPTH); + } + let tithe = scenario + .add_spell_to_hand_from_oracle(P0, "Blood Tithe", false, BLOOD_TITHE) + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(tithe).resolve(); + + let life: Vec = SEATS + .iter() + .map(|p| { + outcome + .state() + .players + .iter() + .find(|pl| pl.id == *p) + .expect("player exists") + .life + }) + .collect(); + + // Reach guard: the loss step actually ran for all three opponents, so the 9 + // really is a three-way total and not a single 3 read off a fan-out that + // never happened. (It is NOT evidence about the per-player table, which is + // empty here — see the degeneracy note above.) + assert_eq!( + &life[1..], + &[17, 17, 17], + "reach guard: each of the three opponents loses exactly 3 (CR 119.3)" + ); + assert_eq!( + life[0], 29, + "controller gains the cross-player TOTAL life lost (9) via \ + PreviousEffectAmount — a reach guard for the 38-card drain class, not an \ + aggregate discriminator (see the degeneracy note above)" + ); +} + +/// PROBE for the second, independent defect the code map surfaced: the draw +/// tail keeps `player_scope: All` and re-fans-out, and each player's completed +/// draw re-stamps the shared scalar with that player's DELIVERED count. So a +/// player whose library ran short does not just draw fewer cards — they +/// redefine how many every LATER player draws. +/// +/// CR 608.2h: the draw action's count is determined only once, when the +/// effect is applied — one player's short library cannot change another +/// player's count. CR 608.2e: the whole fan-out is one action processed +/// simultaneously. CR 121.2c: the SERIALIZATION (the active player performs +/// all of their draws first, then each other player in turn order) is itself +/// rules-correct — only the leaked count is not. +/// +/// Discriminating: P0's library holds 5, everyone else 60. Correct = [5,8,8,8]. +/// Leaked-delivered-count = [5,5,5,5]. The two differ on three seats. +#[test] +fn windfall_short_library_does_not_shrink_later_players_draws() { + let mut scenario = GameScenario::new_n_player(4, 42); + scenario.at_phase(Phase::PreCombatMain); + for (seat, hand) in SEATS.iter().zip([8usize, 7, 3, 3]) { + seed_hand(&mut scenario, *seat, hand); + seed_library( + &mut scenario, + *seat, + if *seat == P0 { 5 } else { LIBRARY_DEPTH }, + ); + } + let windfall = scenario + .add_spell_to_hand_from_oracle(P0, "Windfall", false, WINDFALL) + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + let outcome = runner.cast(windfall).resolve(); + + let drawn: Vec = SEATS + .iter() + .map(|p| { + let depth = if *p == P0 { 5 } else { LIBRARY_DEPTH }; + depth - zone_len(&outcome, *p, Zone::Library) + }) + .collect(); + assert_eq!( + drawn, + vec![5, 8, 8, 8], + "P0's short library caps only P0; every later player still draws the greatest discard (8)" + ); +} + +// --------------------------------------------------------------------------- +// The replacement-pause arms (CR 608.2c: replacement effects may modify an +// instruction's actions). A replacement choice interrupts the discard fan-out +// mid-batch; the clause must still publish ONE complete per-player table. +// --------------------------------------------------------------------------- + +/// Library of Leng, verbatim Scryfall (re-fetched 2026-08-16 via +/// `curl -s 'https://api.scryfall.com/cards/named?exact=Library%20of%20Leng' | jq -r .oracle_text`). +/// +/// Line 2 parses to a `ReplacementEvent::Discard` definition in +/// `ReplacementMode::Optional` with `valid_card: Typed { controller: You }` +/// (`parser/oracle_replacement.rs`'s `parse_discard_to_library_top_replacement`), +/// so the engine raises an Accept/Decline prompt for its controller's discards +/// and for nobody else's. That is what makes this fixture's pause count +/// predictable: exactly one prompt per card P0 discards. +const LIBRARY_OF_LENG: &str = "You have no maximum hand size.\nIf an effect causes you to discard a card, discard it, but you may put it on top of your library instead of into your graveyard."; + +/// Hands beside Windfall. The MAXIMUM sits on P0 — the seat whose batch pauses — +/// so the aggregate is only correct if that seat is present AND complete in the +/// published table. Reference values over `{P0:7, P1:3, P2:5, P3:2}`: +/// MAX 7, MAX-without-P0 5, MAX-with-P0's-paused-card-uncounted 6, last-seat 2. +/// Four mutually distinct numbers, one per failure mode. +const PAUSED_HANDS: [usize; 4] = [7, 3, 5, 2]; + +fn seed_hand_ids(scenario: &mut GameScenario, player: PlayerId, n: usize) -> Vec { + (0..n) + .map(|i| scenario.add_card_to_hand(player, &format!("Hand Filler {player:?} {i}"))) + .collect() +} + +fn state_zone_len(runner: &GameRunner, player: PlayerId, zone: Zone) -> usize { + let p = runner + .state() + .players + .iter() + .find(|p| p.id == player) + .expect("player exists"); + match zone { + Zone::Hand => p.hand.len(), + Zone::Library => p.library.len(), + Zone::Graveyard => p.graveyard.len(), + other => panic!("state_zone_len does not cover {other:?}"), + } +} + +/// Answer every `ReplacementChoice` the board raises with the named option, +/// returning how many were answered. The count is the reach guard for every +/// assertion below: a run that raised no prompt never exercised the pause path +/// at all, and would pass a bare zone-count check for the wrong reason. +fn answer_every_replacement_choice(runner: &mut GameRunner, description: &str) -> usize { + for (prompts, _) in (0..64).enumerate() { + let WaitingFor::ReplacementChoice { candidates, .. } = runner.state().waiting_for.clone() + else { + return prompts; + }; + let index = candidates + .iter() + .position(|c| c.description == description) + .unwrap_or_else(|| { + panic!( + "no {description:?} option among {:?}", + candidates + .iter() + .map(|c| c.description.clone()) + .collect::>() + ) + }); + runner + .act(GameAction::ChooseReplacement { index }) + .expect("ChooseReplacement must be accepted"); + } + panic!("the replacement-choice loop never terminated"); +} + +/// Every observable of the paused clause, asserted as ONE value so a failure +/// prints the whole signature rather than the first divergent field. +#[derive(Debug, PartialEq, Eq)] +struct PausedFanOutSignature { + prompts: usize, + graveyards: Vec, + drawn: Vec, + hands: Vec, +} + +/// Rest in Peace class, made OPTIONAL so it surfaces an Accept/Decline choice. +/// Copied in shape from `random_discard_cost_replacement_resume.rs`'s +/// `optional_graveyard_exile_replacement`; narrowed per call site with +/// `valid_card`. +fn optional_graveyard_exile_replacement() -> ReplacementDefinition { + ReplacementDefinition::new(ReplacementEvent::Moved) + .destination_zone(Zone::Graveyard) + .mode(ReplacementMode::Optional { decline: None }) + .execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::ChangeZone { + origin: None, + destination: Zone::Exile, + target: TargetFilter::SelfRef, + owner_library: false, + enter_transformed: false, + enters_under: None, + enter_tapped: engine::types::zones::EtbTapState::Unspecified, + enters_attacking: false, + up_to: false, + enter_with_counters: vec![], + conditional_enter_with_counters: vec![], + enters_modified_if: None, + face_down_profile: None, + }, + )) +} + +/// ARM A — gate 1 (`ReplacementEvent::Discard`, Library of Leng), the fan-out +/// discriminator. +/// +/// P0 controls an OPTIONAL discard replacement, so every one of P0's seven +/// discards raises an apply-or-decline choice. Deliberately NOT cited to +/// CR 616.1: that rule governs choosing among two or more competing +/// replacements, and this arm has exactly one. CR 614.6 is what makes the +/// applied-or-declined event resolve as it does. CR 608.2f: the discard action is taken on +/// four players and cannot be processed simultaneously once it pauses, so it is +/// processed per player — but it is still ONE action, and the look-back +/// (CR 608.2i) that feeds the draw clause must see every seat's contribution. +/// +/// Discriminating: with `{P0:7, P1:3, P2:5, P3:2}` the correct MAX is 7, and 7 +/// is unreachable under every partial-table failure mode — a table missing P0 +/// yields 5, a table holding only the last resumed leg yields 2, and a table +/// where P0's paused card went uncounted yields 6. +/// +/// Reach guards, both inside the asserted signature: `prompts == 7` proves the +/// pause path really ran seven times (a zero-prompt run would trivially satisfy +/// a graveyard check), and `graveyards == [8, 3, 5, 2]` proves all four seats +/// discarded their whole hands. P0's 8 is seven discards PLUS Windfall itself: +/// CR 608.2n — "As the final part of an instant or sorcery spell's resolution, +/// the spell is put into its owner's graveyard." That is the same reason the +/// first test in this file asserts `>= 8` rather than `== 8`. +#[test] +fn windfall_paused_mid_fan_out_still_draws_the_greatest() { + let mut scenario = GameScenario::new_n_player(4, 42); + scenario.at_phase(Phase::PreCombatMain); + for (seat, hand) in SEATS.iter().zip(PAUSED_HANDS) { + seed_hand_ids(&mut scenario, *seat, hand); + seed_library(&mut scenario, *seat, LIBRARY_DEPTH); + } + let leng = scenario + .add_creature_from_oracle(P0, "Library of Leng", 1, 1, LIBRARY_OF_LENG) + .as_artifact() + .id(); + let windfall = scenario + .add_spell_to_hand_from_oracle(P0, "Windfall", false, WINDFALL) + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + + // Fixture self-checks — the prop must be what the derivation assumes. + assert_eq!( + runner.state().objects[&leng].card_types.core_types, + vec![CoreType::Artifact], + "the Leng prop must be an artifact, not a creature" + ); + assert_eq!( + runner.state().objects[&leng].replacement_definitions.len(), + 1, + "Library of Leng's second line must parse to exactly one replacement" + ); + + // The cast driver stops at the first ReplacementChoice it is not told how + // to answer; from there this test drives the prompts itself so it can count + // them. + runner.cast(windfall).resolve(); + assert!( + runner.state().pending_discard_batch.is_some(), + "reach guard: the first Library of Leng prompt must park Windfall's live discard batch" + ); + let saved = serde_json::to_string(&PersistedGameState::capture(runner.state().clone())) + .expect("parked Windfall state serializes through the authoritative persistence envelope"); + let restored: PersistedGameState = serde_json::from_str(&saved) + .expect("parked Windfall state restores through the authoritative persistence envelope"); + let restored = restored.into_game_state(); + assert!( + restored.pending_discard_batch.is_some(), + "the live discard cursor must survive save and restore before its replacement choice" + ); + let mut runner = GameRunner::from_state(restored); + let prompts = answer_every_replacement_choice(&mut runner, "Decline"); + runner.advance_until_stack_empty(); + + let observed = PausedFanOutSignature { + prompts, + graveyards: SEATS + .iter() + .map(|p| state_zone_len(&runner, *p, Zone::Graveyard)) + .collect(), + drawn: SEATS + .iter() + .map(|p| LIBRARY_DEPTH - state_zone_len(&runner, *p, Zone::Library)) + .collect(), + hands: SEATS + .iter() + .map(|p| state_zone_len(&runner, *p, Zone::Hand)) + .collect(), + }; + + assert_eq!( + observed, + PausedFanOutSignature { + prompts: 7, + graveyards: vec![8, 3, 5, 2], + drawn: vec![7, 7, 7, 7], + hands: vec![7, 7, 7, 7], + }, + "a replacement pause must not truncate the batch (prompts/graveyards) nor split \ + the clause's per-player table (drawn/hands)" + ); +} + +#[test] +fn replacement_resumed_targeted_discard_preserves_the_announced_multi_owner_tail() { + let mut scenario = GameScenario::new_n_player(3, 42); + scenario.at_phase(Phase::PreCombatMain); + let first = scenario.add_card_to_hand(P1, "First Target"); + let second = scenario.add_card_to_hand(P2, "Second Target"); + let source = scenario + .add_spell_to_hand(P0, "Targeted Discard", false) + .id(); + scenario + .add_creature(P0, "Graveyard Warden", 1, 1) + .with_replacement_definition( + optional_graveyard_exile_replacement() + .valid_card(TargetFilter::SpecificObject { id: first }), + ); + let mut runner = scenario.build(); + let ability = ResolvedAbility::new( + Effect::DiscardCard { + count: 2, + target: TargetFilter::SpecificObject { id: first }, + }, + vec![TargetRef::Object(first), TargetRef::Object(second)], + source, + P0, + ); + let mut initial_events = Vec::new(); + engine::game::effects::resolve_ability_chain( + runner.state_mut(), + &ability, + &mut initial_events, + 0, + ) + .expect("the announced targeted discard resolves to its first replacement choice"); + + assert!(matches!( + runner.state().waiting_for, + WaitingFor::ReplacementChoice { .. } + )); + assert!(matches!( + runner.state().pending_discard_batch.as_deref().map(|batch| &batch.cursor), + Some(engine::types::game_state::DiscardBatchCursor::Ordered { remaining }) + if remaining.len() == 1 && remaining[0].object_id == second + )); + + let result = apply( + runner.state_mut(), + P1, + GameAction::ChooseReplacement { index: 0 }, + ) + .expect("accept the first target's graveyard redirect"); + + assert_eq!(runner.state().objects[&first].zone, Zone::Exile); + assert_eq!(runner.state().objects[&second].zone, Zone::Graveyard); + assert!(runner.state().pending_discard_batch.is_none()); + assert_eq!( + result + .events + .iter() + .filter( + |event| matches!(event, engine::types::events::GameEvent::EffectResolved { + kind: engine::types::ability::EffectKind::DiscardCard, + source_id: event_source, + .. + } if *event_source == source) + ) + .count(), + 1, + "the resumed ordered target list emits its terminal marker exactly once" + ); +} + +/// Every observable of the gate-2 arm, asserted as ONE value. +#[derive(Debug, PartialEq, Eq)] +struct GateTwoSignature { + prompts: usize, + p0_graveyard: usize, + redirected_card_zone: Zone, + exiled_total: usize, + drawn: Vec, +} + +/// ARM B — gate 2 (`ReplacementEvent::Moved` on the inner hand → graveyard +/// move), the discriminator for the paused card's OWN count. +/// +/// CR 614.6: a replaced event never happens; the modified event happens +/// instead — the card is still discarded (CR 701.9a) and must still be counted. +/// The gate-2 resume returns through terminal zone delivery, which emits no +/// `GameEvent::Discarded` for an unframed discard, so the paused card is the one +/// card that can silently vanish from the table even after the batch resumes. +/// +/// Discriminating: exactly one card in the game can prompt (`valid_card` is a +/// `SpecificObject`), the redirect is ACCEPTED, and P0's counted discards are 7 +/// while P0's graveyard tops out at 7 (six discards + Windfall) because the +/// seventh went to exile. If the paused card is uncounted the aggregate is 6, +/// not 7 — the only arm in this file that separates that facet from the batch +/// truncation arm above. +/// +/// Reach guards, inside the asserted signature: `prompts == 1` proves the pause +/// happened, and `redirected_card_zone == Exile` / `exiled_total == 1` prove the +/// redirect was actually applied (on a board where it silently did not apply, +/// the card would be in the graveyard and the count would be right for the +/// wrong reason). +#[test] +fn windfall_counts_a_card_redirected_out_of_the_graveyard_mid_batch() { + let mut scenario = GameScenario::new_n_player(4, 42); + scenario.at_phase(Phase::PreCombatMain); + let mut p0_hand = Vec::new(); + for (seat, hand) in SEATS.iter().zip(PAUSED_HANDS) { + let ids = seed_hand_ids(&mut scenario, *seat, hand); + if *seat == P0 { + p0_hand = ids; + } + seed_library(&mut scenario, *seat, LIBRARY_DEPTH); + } + let redirected = p0_hand[3]; + // Hosted on P1 so it cannot be confused with the discarding seat's own + // permanents; narrowed to a single card so the prompt count is exactly 1. + scenario + .add_creature(P1, "Graveyard Warden", 1, 1) + .with_replacement_definition( + optional_graveyard_exile_replacement() + .valid_card(TargetFilter::SpecificObject { id: redirected }), + ); + let windfall = scenario + .add_spell_to_hand_from_oracle(P0, "Windfall", false, WINDFALL) + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + + runner.cast(windfall).resolve(); + let prompts = answer_every_replacement_choice(&mut runner, "Accept"); + runner.advance_until_stack_empty(); + + let observed = GateTwoSignature { + prompts, + p0_graveyard: state_zone_len(&runner, P0, Zone::Graveyard), + redirected_card_zone: runner.state().objects[&redirected].zone, + exiled_total: runner + .state() + .objects + .values() + .filter(|o| o.zone == Zone::Exile) + .count(), + drawn: SEATS + .iter() + .map(|p| LIBRARY_DEPTH - state_zone_len(&runner, *p, Zone::Library)) + .collect(), + }; + + assert_eq!( + observed, + GateTwoSignature { + prompts: 1, + p0_graveyard: 7, + redirected_card_zone: Zone::Exile, + exiled_total: 1, + drawn: vec![7, 7, 7, 7], + }, + "a card redirected out of the graveyard mid-batch was still discarded \ + (CR 614.6 + CR 701.9a) and must still be counted" + ); +} diff --git a/crates/phase-ai/src/policies/x_cast_gate.rs b/crates/phase-ai/src/policies/x_cast_gate.rs index 63af64899f..ac3b4c6c41 100644 --- a/crates/phase-ai/src/policies/x_cast_gate.rs +++ b/crates/phase-ai/src/policies/x_cast_gate.rs @@ -401,6 +401,7 @@ mod tests { amount: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: engine::types::ability::DamageChannel::Total, + aggregate: engine::types::ability::AggregateFunction::Sum, }, }, player: TargetFilter::Controller, @@ -772,6 +773,7 @@ mod tests { count: QuantityExpr::Ref { qty: QuantityRef::PreviousEffectAmount { channel: engine::types::ability::DamageChannel::Total, + aggregate: engine::types::ability::AggregateFunction::Sum, }, }, target: TargetFilter::Controller, diff --git a/crates/phase-ai/src/policies/x_reference.rs b/crates/phase-ai/src/policies/x_reference.rs index f0326de2d9..4e3b06be3e 100644 --- a/crates/phase-ai/src/policies/x_reference.rs +++ b/crates/phase-ai/src/policies/x_reference.rs @@ -354,9 +354,18 @@ fn is_cost_x_paid(qty: &QuantityRef) -> bool { } fn is_previous_amount(qty: &QuantityRef) -> bool { - // CR 120.6 / CR 120.10: both channels (total and excess) are amounts left by - // the preceding effect, so the AI's X-reference detection treats them alike — - // it cares that the value is chain-derived, not which tally it came from. + // Both channels (total and excess) are amounts left by the preceding + // effect, so the AI's X-reference detection treats them alike — it cares + // that the value is chain-derived, not which tally it came from, and every + // aggregate reduces the same table, so the detection is aggregate-agnostic + // too. + // + // The former CR 120.10 tag is STRUCK, not relocated. Read in full, that + // rule scopes triggered abilities that check whether a permanent has been + // dealt EXCESS DAMAGE; it says nothing about amounts one effect leaves for + // the next, and nothing about aggregate-agnostic detection. An AI scoring + // heuristic implements no game rule and needs no CR annotation. The + // rationale above is kept verbatim. matches!(qty, QuantityRef::PreviousEffectAmount { .. }) } @@ -406,3 +415,46 @@ fn filter_prop_references_x(prop: &FilterProp) -> bool { _ => false, } } + +#[cfg(test)] +mod tests { + /// The `CR 120.10` strike on `is_previous_amount` is load-bearing, so it is + /// asserted rather than left to review. Read in full, CR 120.10 governs + /// triggered abilities that check whether a permanent has been dealt excess + /// damage — it does not govern "amounts left by the preceding effect", and + /// an AI scoring heuristic implements no game rule at all. + /// + /// Reads this file's own source so the assertion is about the annotation as + /// shipped, not about a value re-derived from it. + /// + /// REVERT PROBE (RUN, not reasoned): restore the tag, i.e. change the + /// comment's first line back to `// CR 120.10: both channels (total and + /// excess) are amounts left by`. Observed failure — "an AI scoring heuristic + /// implements no game rule, so it carries no CR annotation". + #[test] + fn previous_amount_detection_carries_its_rationale_without_a_cr_tag() { + let source = include_str!("x_reference.rs"); + let start = source + .find("fn is_previous_amount(") + .expect("the detection helper exists"); + let body = &source[start..start + 900]; + + assert!( + body.contains("chain-derived"), + "the rationale for treating both channels alike must survive the strike" + ); + // Matched on the ANNOTATION FORM, not on one punctuation variant: an + // earlier revision asserted only on `"CR 120.10:"`, which a re-added + // `// CR 120.10 both channels …` (no colon) would have slipped past — + // while the surrounding window deliberately contains the prose "The + // former CR 120.10 tag is STRUCK", so a bare substring test cannot be + // used either. Any comment line whose first token after `//` is the + // citation is a restored annotation. + assert!( + !body + .lines() + .any(|line| line.trim_start().starts_with("// CR 120.10")), + "an AI scoring heuristic implements no game rule, so it carries no CR annotation" + ); + } +}