Skip to content
142 changes: 141 additions & 1 deletion crates/engine/src/game/effects/draw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::game::quantity::resolve_quantity_with_targets;
use crate::game::replacement::{self, ReplacementResult};
use crate::game::static_abilities::prohibition_scope_matches_player;
use crate::types::ability::{Effect, EffectError, EffectKind, ResolvedAbility};
use crate::types::events::GameEvent;
use crate::types::events::{GameEvent, PlayerActionKind};
use crate::types::game_state::{DrawSequenceOrigin, GameState};
use crate::types::proposed_event::{AppliedReplacementKey, ProposedEvent};
use crate::types::statics::StaticMode;
Expand Down Expand Up @@ -446,6 +446,26 @@ fn resume_draw_sequence_outcome(
return DrawSequenceOutcome::Parked(ReplacementResult::Prevented);
};
state.last_effect_count = Some(frame.accumulated as i32);
// Record the drawing player exactly once per
// settled draw INSTRUCTION — the emission granularity is the whole draw, not
// the per-card unit that `apply_draw_after_replacement` settles. `frame.player`
// is the concrete drawer, so during a `player_scope: Opponent` fan-out (Cut a
// Deal) each scoped opponent's own instruction records that opponent, without
// relying on `ability.controller` rebinding. Gated on `frame.accumulated > 0`
// so an instruction that delivered no card (empty library, or every unit
// replaced away) records nothing because that player did not draw. The generic
// post-effect scan in `effects/mod.rs` folds this
// event into `player_actions_this_way` (a set — dedups the drawer for a
// multi-card draw) and `player_actions_this_turn` (a Vec — now one entry per
// draw event, not per card).
if frame.accumulated > 0 {
events.push(GameEvent::PlayerPerformedAction {
player_id: frame.player,
action: PlayerActionKind::Draw,
look_count: None,
scry_bottom_count: None,
});
}
match frame.origin {
DrawSequenceOrigin::Plain => {
// Intentionally no `EffectResolved { Draw }`: no trigger matcher consumes
Expand Down Expand Up @@ -668,6 +688,16 @@ pub fn apply_draw_after_replacement(
.expect("empty-library draw bookkeeping must have a live player and journal cause");
}

// CR 121.1 + CR 608.2c + CR 109.5: The `PlayerPerformedAction { Draw }`
// ledger emission is NOT made here. This helper settles ONE draw unit — the
// sequence driver (`resume_draw_sequence_outcome`) calls it once per card
// (count = 1), and the resumed-choice path in `engine_replacement` settles a
// single paused unit too — so recording here would push one event per card.
// Instead the drawing player is recorded exactly once per settled draw
// INSTRUCTION, at frame completion, gated on the instruction's true total.
// That keeps `player_actions_this_way` (a set) counting players who drew and
// makes `player_actions_this_turn` (a Vec) count draw events, not cards, so a
// future `PlayerActionsThisTurn { Draw }` consumer measures draws not cards.
drawn_count
}

Expand Down Expand Up @@ -1687,3 +1717,113 @@ mod tranche4_draw_pipeline_tests {
assert!(state.players[0].graveyard.contains(&drawn));
}
}

/// CR 121.1 + CR 608.2c + CR 109.5: The `PlayerPerformedAction { Draw }` ledger
/// emission fires once per settled draw INSTRUCTION, at draw-sequence completion
/// (`resume_draw_sequence_outcome`). These tests drive the REAL production driver
/// (`start_draw_sequence`), which internally delivers a multi-card draw
/// unit-by-unit (count = 1 per card) — the exact shape production uses — and
/// assert the emission granularity is per instruction, not per card. A direct
/// `apply_draw_after_replacement` call with `count: 2` is deliberately NOT used:
/// production never settles a multi-card draw in a single such call, so it would
/// exercise a shape the engine doesn't drive.
#[cfg(test)]
mod draw_this_way_ledger_tests {
use super::*;
use crate::game::scenario::{GameScenario, P0};

fn drew_action_events(events: &[GameEvent]) -> usize {
events
.iter()
.filter(|event| {
matches!(
event,
GameEvent::PlayerPerformedAction {
action: PlayerActionKind::Draw,
..
}
)
})
.count()
}

fn card_drawn_events(events: &[GameEvent]) -> usize {
events
.iter()
.filter(|event| matches!(event, GameEvent::CardDrawn { .. }))
.count()
}

/// CR 121.1 + CR 608.2c: A TWO-card draw driven by the production sequence
/// (`start_draw_sequence(.., 2, ..)`) delivers two cards (two `CardDrawn`
/// events) but records the drawing player with exactly ONE
/// `PlayerPerformedAction { Draw }` — the emission is per settled draw
/// instruction, not per card. Revert-failing anchor: moving the emit back
/// into the per-unit `apply_draw_after_replacement` makes `drew_action_events`
/// == 2 (one per card) and fails the final assertion. This is the emission
/// side of ruling #2 ("if an opponent drew more than one card this way … you
/// still draw only one card for that player"); the `player_actions_this_way`
/// `HashSet` is the second line of defence, validated end-to-end in the
/// `cut_a_deal_draw_this_way_count` integration suite.
#[test]
fn multi_card_instruction_records_player_once_via_sequence() {
let mut sc = GameScenario::new();
sc.add_card_to_library_top(P0, "Island");
sc.add_card_to_library_top(P0, "Mountain");
let mut state = sc.state;

let mut events = Vec::new();
start_draw_sequence(&mut state, P0, 2, &mut events);

assert_eq!(
card_drawn_events(&events),
2,
"the two-card instruction must deliver two cards (per-card CardDrawn)"
);
assert_eq!(
drew_action_events(&events),
1,
"but the draw-action ledger event must fire exactly once per instruction, \
not once per card (CR 608.2c ruling #2)"
);
}

/// CR 121.1: A draw instruction that delivers no card (empty library — no top
/// card enters the hand, so no draw occurs) emits no
/// `PlayerPerformedAction { Draw }`, so a player who doesn't draw is never
/// counted (CR 608.2c ruling #1). This is the `frame.accumulated > 0` gate.
#[test]
fn empty_library_instruction_records_nothing_via_sequence() {
let sc = GameScenario::new();
let mut state = sc.state;

let mut events = Vec::new();
start_draw_sequence(&mut state, P0, 1, &mut events);

assert_eq!(
card_drawn_events(&events),
0,
"empty library delivers no card"
);
assert_eq!(
drew_action_events(&events),
0,
"a draw that delivers nothing must not record the player (CR 608.2c ruling #1)"
);
}

/// CR 121.1: Baseline — a normal one-card draw instruction records the drawing
/// player once, so ordinary draws still populate the ledger.
#[test]
fn single_card_instruction_records_player_once_via_sequence() {
let mut sc = GameScenario::new();
sc.add_card_to_library_top(P0, "Plains");
let mut state = sc.state;

let mut events = Vec::new();
start_draw_sequence(&mut state, P0, 1, &mut events);

assert_eq!(card_drawn_events(&events), 1);
assert_eq!(drew_action_events(&events), 1);
}
}
20 changes: 18 additions & 2 deletions crates/engine/src/game/effects/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3622,8 +3622,24 @@ fn detach_after_player_scope_local_chain(
// "Each opponent may X and Y" makes the whole same-sentence X/Y clause
// optional for that opponent. Keep the continuation inside the scoped
// template so accepting the offer performs both instructions.
let next_is_optional_clause_continuation =
node.optional && next.sub_link == SubAbilityLink::ContinuationStep;
//
// CR 608.2c + CR 109.5: EXCEPT a child that carries its OWN distinct
// `player_scope` (a different population than this fan-out's) is a SEPARATE
// scoped instruction, not a continuation of this optional clause — e.g.
// Kwain, Itinerant Meddler's "each player may draw a card, then each player
// who drew a card this way gains 1 life": the GainLife is scoped to the
// drawers (`PerformedActionThisWay`), not to this "each player" (`All`)
// fan-out. Keeping it co-scoped would re-enter the fan-out driver once per
// outer iteration and re-count the incrementally growing this-way ledger,
// over-applying to the earlier-iterated players (a triangular over-gain). It
// must detach and resolve ONCE over its own population after the parent
// fan-out completes.
let next_is_optional_clause_continuation = node.optional
&& next.sub_link == SubAbilityLink::ContinuationStep
&& next
.player_scope
.as_ref()
.is_none_or(|child_scope| child_scope == scope);
// CR 701.23i + CR 701.24a: A shuffle explicitly scoped to players who
// searched this way is a once-after-all-searches tail, not the ordinary
// per-player SearchLibrary → ChangeZone → Shuffle continuation. Keep the
Expand Down
8 changes: 5 additions & 3 deletions crates/engine/src/game/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15961,9 +15961,11 @@ mod stage2_injector_tests {
// and is offered as a follow-up rather than taken unannounced mid-review.
// #6812 noted-mana support inserts two lines above all three producers:
// `:6210/:6287/:9475 => :6212/:6289/:9477`. The producers remain byte-identical.
"game/effects/mod.rs:6212".to_string(),
"game/effects/mod.rs:6289".to_string(),
"game/effects/mod.rs:9477".to_string(),
// #7018 adds the 16-line distinct-player-scope continuation gate above all
// three producers: `:6212/:6289/:9477 => :6228/:6305/:9493`.
"game/effects/mod.rs:6228".to_string(),
"game/effects/mod.rs:6305".to_string(),
"game/effects/mod.rs:9493".to_string(),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// 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.
Expand Down
45 changes: 45 additions & 0 deletions crates/engine/src/game/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,15 @@ fn should_exclude_event(event: &GameEvent, state: &GameState) -> bool {
{
true
}
// PlayerPerformedAction { Draw } is an internal ledger signal consumed by
// "for each player who drew a card this way" counting and
// the player-action trigger index), not a user-facing event. Unlike
// CardDrawn, which remains available as a HiddenInformation diagnostic,
// excluding it keeps the visible log from narrating internal ledger events.
GameEvent::PlayerPerformedAction {
action: crate::types::events::PlayerActionKind::Draw,
..
} => true,
// StackPushed/StackResolved are low-signal bookkeeping —
// the meaningful info is in SpellCast/AbilityActivated and EffectResolved
GameEvent::StackPushed { .. } | GameEvent::StackResolved { .. } => true,
Expand Down Expand Up @@ -1816,6 +1825,42 @@ mod tests {
));
}

#[test]
fn draw_player_action_is_excluded_but_other_actions_are_logged() {
use crate::types::events::PlayerActionKind;

let state = GameState::new_two_player(42);
// The Draw ledger signal must not reach the visible log —
// this assertion flips (entries.len() == 1) if the exclusion is reverted.
let draw_event = GameEvent::PlayerPerformedAction {
player_id: PlayerId(0),
action: PlayerActionKind::Draw,
look_count: None,
scry_bottom_count: None,
};
let draw_entries = resolve_log_entries(&[draw_event], &state, &state);
assert!(
draw_entries.is_empty(),
"PlayerPerformedAction {{ Draw }} is a ledger-only signal and must be excluded from the log"
);

// Reach-guard against an over-broad exclusion: a non-Draw player action
// (Scry) must still produce a log entry. Fails if someone excludes all
// PlayerPerformedAction variants instead of just Draw.
let scry_event = GameEvent::PlayerPerformedAction {
player_id: PlayerId(0),
action: PlayerActionKind::Scry,
look_count: Some(1),
scry_bottom_count: Some(0),
};
let scry_entries = resolve_log_entries(&[scry_event], &state, &state);
assert_eq!(
scry_entries.len(),
1,
"Non-Draw player actions must remain visible in the log"
);
}

#[test]
fn damage_dealt_non_combat_is_life_category() {
let event = GameEvent::DamageDealt {
Expand Down
75 changes: 75 additions & 0 deletions crates/engine/src/parser/oracle_effect/lower.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4424,6 +4424,20 @@ pub(super) fn strip_each_player_subject(text: &str) -> (Option<PlayerFilter>, St
return (Some(attr_scope), deconjugated);
}

// CR 608.2c + CR 109.5: A "who [verb]ed … this way" relative clause after
// "each player" / "each opponent" restricts the affected set to the players
// who performed the tracked action during THIS resolution (Kwain, Itinerant
// Meddler: "each player who drew a card this way gains 1 life" — only players
// who actually drew gain the life, so an opponent who declined the optional
// draw or had an empty library is excluded). Like the "who controls" /
// attribute clauses above, the relative clause MUST be consumed and reflected
// in the scope; dropping it would over-apply the effect to every player.
if let Some((action_scope, after_clause)) = strip_performed_action_this_way_clause(&scope, rest)
{
let deconjugated = subject::deconjugate_verb(&after_clause);
return (Some(action_scope), deconjugated);
}

// CR 508.6 + CR 104.3e: A "[source] attacked this turn" relative clause after
// "each player" / "each opponent" restricts the affected set to the players
// the ability source creature attacked this turn — Angel of Destiny: "each
Expand Down Expand Up @@ -5068,6 +5082,67 @@ fn strip_player_attribute_clause(
))
}

/// CR 608.2c + CR 109.5: Strip a "who [verb]ed … this way" relative clause after
/// an "each opponent"/"each player" subject. Returns
/// `PlayerFilter::PerformedActionThisWay` (carrying the base subject's relation
/// and the performed action, keyed at runtime on the `player_actions_this_way`
/// ledger that each settled search/investigate/draw populates) and the
/// verb-phrase remainder. Returns `None` when no such clause is present.
///
/// The this-way verb table is delegated whole to `parse_who_action_this_way`
/// (oracle_quantity.rs) — the same authority the quantity path
/// (`parse_action_this_way`) uses — so search, investigate, and draw stay one
/// building block across both the quantity and subject scopes. This function
/// adds only the subject-path concerns: deriving the relation from the base
/// subject and enforcing a non-empty verb-phrase residual. Kwain, Itinerant
/// Meddler ("each player who drew a card this way gains 1 life") is the
/// subject-scope sibling of Cut a Deal's quantity-path "for each opponent who
/// drew a card this way".
fn strip_performed_action_this_way_clause(
base: &PlayerFilter,
rest: &str,
) -> Option<(PlayerFilter, String)> {
use crate::types::ability::PlayerRelation;
let relation = match base {
PlayerFilter::Opponent => PlayerRelation::Opponent,
PlayerFilter::All => PlayerRelation::All,
PlayerFilter::Controller
| PlayerFilter::DefendingPlayer
| PlayerFilter::OpponentLostLife
| PlayerFilter::OpponentGainedLife
| PlayerFilter::HasLostTheGame
| PlayerFilter::OpponentDealtDamage { .. }
| PlayerFilter::OpponentAttacked { .. }
| PlayerFilter::OpponentAttackingEnchantedPlayer
| PlayerFilter::AllExcept { .. }
| PlayerFilter::HighestSpeed
| PlayerFilter::ZoneChangedThisWay
| PlayerFilter::PerformedActionThisWay { .. }
| PlayerFilter::OwnersOfCardsExiledBySource
| PlayerFilter::TriggeringPlayer
| PlayerFilter::OpponentOtherThanTriggering
| PlayerFilter::OpponentOfTriggeringPlayer
| PlayerFilter::OpponentOfTriggeringPlayerNotAttacked
| PlayerFilter::VotedFor { .. }
| PlayerFilter::ParentObjectTargetController
| PlayerFilter::ControlsCount { .. }
| PlayerFilter::PlayerAttribute { .. }
| PlayerFilter::ChosenPlayer { .. }
| PlayerFilter::ParentObjectTargetOwner
| PlayerFilter::TrackedSetPossessor { .. } => return None,
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.
let (remainder, action) =
crate::parser::oracle_quantity::parse_who_action_this_way(rest).ok()?;
let verb_phrase = remainder.trim_start();
if verb_phrase.is_empty() {
return None;
}
Some((
PlayerFilter::PerformedActionThisWay { relation, action },
verb_phrase.to_string(),
))
}

fn strip_linked_exile_owner_subject(text: &str) -> (Option<PlayerFilter>, String) {
let lower = text.to_lowercase();
let scope_rest = nom_on_lower(text, &lower, |i| {
Expand Down
Loading
Loading