fix(engine): Ur-Dragon softlock — propagate batched trigger subject count for EventContextAmount - #1050
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements batched trigger subject counting (CR 603.2c), allowing "one or more" triggers to correctly resolve "that many" based on the number of matching subjects in a firing event batch. It introduces subject_match_count to tracking structures, implements the counting logic in trigger_matchers.rs, and ensures this count is saved and restored across optional effect decision points. Feedback focuses on a logic error in the state restoration during cost-payment continuations, missing mandatory CR annotations, and incomplete event coverage in the subject extraction logic.
| let previous_trigger_match_count = state.current_trigger_match_count; | ||
| state.current_trigger_event = pending_event; | ||
| effects::drain_pending_continuation(state, &mut events); | ||
| state.current_trigger_event = previous_trigger_event; | ||
| state.current_trigger_match_count = previous_trigger_match_count; |
There was a problem hiding this comment.
[HIGH] The logic to restore current_trigger_match_count is incomplete. While it correctly saves and restores the previous_trigger_match_count, it fails to assign the pending match count to state.current_trigger_match_count before draining the continuation. This breaks the "mirroring" mentioned in the comment and will cause EventContextAmount to resolve incorrectly (likely to None or 0) during the resumed resolution of a batched trigger with a cost (CR 603.2c, CR 608.2).
let previous_trigger_event = state.current_trigger_event.clone();
let previous_trigger_match_count = state.current_trigger_match_count;
state.current_trigger_event = pending_event;
state.current_trigger_match_count = state.pending_optional_trigger_match_count.take();
effects::drain_pending_continuation(state, &mut events);
state.current_trigger_event = previous_trigger_event;
state.current_trigger_match_count = previous_trigger_match_count;| /// Subject `ObjectId`s carried by a single `GameEvent` for trigger filter | ||
| /// matching. Grows by event family as new "one or more <FILTER> <verb>" | ||
| /// patterns land. Variants without an object subject return an empty iterator. | ||
| fn trigger_event_subject_ids(event: &GameEvent) -> Box<dyn Iterator<Item = ObjectId> + '_> { |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for trigger_event_subject_ids. Per Rule R6, every rules-touching function must carry a CR <number> comment. This function implements the subject extraction logic required for CR 603.2c.
/// CR 603.2c: Subject ObjectIds carried by a single GameEvent for trigger filter matching.
fn trigger_event_subject_ids(event: &GameEvent) -> Box<dyn Iterator<Item = ObjectId> + '_> {References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| GameEvent::GameStarted | ||
| | GameEvent::TurnStarted { .. } | ||
| | GameEvent::PhaseChanged { .. } | ||
| | GameEvent::PriorityPassed { .. } | ||
| | GameEvent::SpellCopied { .. } | ||
| | GameEvent::XValueChosen { .. } | ||
| | GameEvent::AbilityActivated { .. } | ||
| | GameEvent::LifeChanged { .. } | ||
| | GameEvent::ManaAdded { .. } | ||
| | GameEvent::TappedForMana { .. } | ||
| | GameEvent::ManaPoolEmptied { .. } | ||
| | GameEvent::ManaRecolored { .. } | ||
| | GameEvent::PermanentTapped { .. } | ||
| | GameEvent::PlayerLost { .. } | ||
| | GameEvent::MulliganStarted | ||
| | GameEvent::CardsDrawn { .. } | ||
| | GameEvent::CardDrawn { .. } | ||
| | GameEvent::PermanentUntapped { .. } | ||
| | GameEvent::PermanentPhasedOut { .. } | ||
| | GameEvent::PermanentPhasedIn { .. } | ||
| | GameEvent::PlayerPhasedOut { .. } | ||
| | GameEvent::PlayerPhasedIn { .. } | ||
| | GameEvent::LandPlayed { .. } | ||
| | GameEvent::StackPushed { .. } | ||
| | GameEvent::StackResolved { .. } | ||
| | GameEvent::DamageCleared { .. } | ||
| | GameEvent::GameOver { .. } | ||
| | GameEvent::DamageDealt { .. } | ||
| | GameEvent::DamagePrevented { .. } | ||
| | GameEvent::SpellCountered { .. } | ||
| | GameEvent::CounterAdded { .. } | ||
| | GameEvent::CounterRemoved { .. } | ||
| | GameEvent::TokenCreated { .. } | ||
| | GameEvent::ObjectConjured { .. } | ||
| | GameEvent::CreatureDestroyed { .. } | ||
| | GameEvent::PermanentSacrificed { .. } | ||
| | GameEvent::EffectResolved { .. } | ||
| | GameEvent::Unattached { .. } | ||
| | GameEvent::BlockersDeclared { .. } | ||
| | GameEvent::CombatTaxPaid { .. } | ||
| | GameEvent::CombatTaxDeclined { .. } | ||
| | GameEvent::BecomesTarget { .. } | ||
| | GameEvent::VehicleCrewed { .. } | ||
| | GameEvent::Stationed { .. } | ||
| | GameEvent::Saddled { .. } | ||
| | GameEvent::ReplacementApplied { .. } | ||
| | GameEvent::Transformed { .. } | ||
| | GameEvent::DayNightChanged { .. } | ||
| | GameEvent::TurnedFaceUp { .. } | ||
| | GameEvent::CardsRevealed { .. } | ||
| | GameEvent::CombatDamageDealtToPlayer { .. } | ||
| | GameEvent::PlayerEliminated { .. } | ||
| | GameEvent::CrimeCommitted { .. } | ||
| | GameEvent::Cycled { .. } | ||
| | GameEvent::PlayerPerformedAction { .. } | ||
| | GameEvent::Regenerated { .. } | ||
| | GameEvent::CreatureSuspected { .. } | ||
| | GameEvent::BecamePrepared { .. } | ||
| | GameEvent::BecameUnprepared { .. } | ||
| | GameEvent::CaseSolved { .. } | ||
| | GameEvent::ClassLevelGained { .. } | ||
| | GameEvent::MonarchChanged { .. } | ||
| | GameEvent::CityBlessingGained { .. } | ||
| | GameEvent::DieRolled { .. } | ||
| | GameEvent::CoinFlipped { .. } | ||
| | GameEvent::RingTemptsYou { .. } | ||
| | GameEvent::RoomEntered { .. } | ||
| | GameEvent::RoomDoorUnlocked { .. } | ||
| | GameEvent::BecomesPlotted { .. } | ||
| | GameEvent::DungeonCompleted { .. } | ||
| | GameEvent::InitiativeTaken { .. } | ||
| | GameEvent::Firebend { .. } | ||
| | GameEvent::Airbend { .. } | ||
| | GameEvent::Earthbend { .. } | ||
| | GameEvent::Waterbend { .. } | ||
| | GameEvent::CompanionRevealed { .. } | ||
| | GameEvent::CompanionMovedToHand { .. } | ||
| | GameEvent::NinjutsuActivated { .. } | ||
| | GameEvent::KeywordAbilityActivated { .. } | ||
| | GameEvent::CreatureExploited { .. } | ||
| | GameEvent::EnergyChanged { .. } | ||
| | GameEvent::SpeedChanged { .. } | ||
| | GameEvent::PlayerCounterChanged { .. } | ||
| | GameEvent::ManaExpended { .. } | ||
| | GameEvent::Clash { .. } | ||
| | GameEvent::VoteCast { .. } | ||
| | GameEvent::VoteResolved { .. } | ||
| | GameEvent::PowerToughnessChanged { .. } | ||
| | GameEvent::CascadeMissed { .. } | ||
| | GameEvent::DebugActionUsed { .. } | ||
| | GameEvent::DebugPermissionGranted { .. } | ||
| | GameEvent::DebugPermissionRevoked { .. } => Box::new(std::iter::empty()), |
There was a problem hiding this comment.
[MEDIUM] Poor sibling coverage for object-based events in trigger_event_subject_ids. While it handles attackers and zone changes, it returns an empty iterator for many other common events that can be subjects of "one or more" triggers (e.g., DamageDealt, TokenCreated, CreatureDestroyed, PermanentSacrificed, PermanentTapped, PermanentUntapped). This will cause EventContextAmount to resolve to 0 for batched triggers on these events (Rule L2). Note that for events like DamageDealt, you should also ensure unique ObjectIds are counted if multiple events in a batch target the same object.
| .as_ref() | ||
| .and_then(crate::game::targeting::extract_amount_from_event) | ||
| .current_trigger_match_count | ||
| .map(|n| n as i32) |
…ct count - engine.rs: populate current_trigger_match_count from pending_optional_trigger_match_count before drain_pending_continuation so EventContextAmount resolves the correct "that many" in the resumed continuation (Gemini comment #1 — logic error in state restoration) - trigger_matchers.rs: add CR 603.2c doc annotation to trigger_event_subject_ids (comment phase-rs#2); extend match to return ObjectId arms for TokenCreated, CreatureDestroyed, PermanentSacrificed, PermanentTapped, PermanentUntapped, and DamageDealt (object target only, per CR 120.3) — previously these all fell through to std::iter::empty() (comment phase-rs#3) - quantity.rs: replace .map(|n| n as i32) with .map(u32_to_i32_saturating) for consistency with the rest of the file (comment phase-rs#4) Verified: cargo clippy --all-targets -- -D warnings (clean), cargo test -p engine (8956 passed) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
All four Gemini review comments have been addressed in commit 08e45c7. Comment #1 (HIGH — logic error in state restoration, engine.rs): Comment #2 (MEDIUM — missing CR annotation, trigger_matchers.rs:618): Comment #3 (MEDIUM — incomplete event coverage, trigger_matchers.rs): Comment #4 (MEDIUM — use u32_to_i32_saturating, quantity.rs:1225): Verified: |
Closes #707
Summary
Fixes two related game softlocks:
Primary (#707): The Ur-Dragon's attack trigger (
Whenever one or more Dragons you control attack, draw that many cards…) caused a stuck game.QuantityRef::EventContextAmountwas resolving to 0 for batched triggers becauseextract_amount_from_eventhas no amount forAttackersDeclaredevents. A newcurrent_trigger_match_countfield onGameStatecarries the filtered subject count through resolution;EventContextAmountnow checks this count first (CR 603.2c).Scope expansion: A second priority softlock was found and fixed during investigation — under turn-control effects (CR 723, e.g. Mindslaver),
handle_priority_passwas trackingpriority_player(the rewritten submitter) instead of the semantic seat, so the "all players pass in succession" condition (CR 117.4) could never be satisfied.Files changed
crates/engine/src/types/game_state.rs—current_trigger_match_countfieldcrates/engine/src/game/stack.rs— set/clear match count on batched trigger resolutioncrates/engine/src/game/quantity.rs—EventContextAmountprecedence: match count → event payload → fallbackscrates/engine/src/game/trigger_matchers.rs— new:count_trigger_subjects_in_batchcrates/engine/src/game/triggers.rs— propagate match count into stack entry clonescrates/engine/src/game/priority.rs/turn_control.rs— semantic seat tracking for priority passingcrates/engine/tests/integration/batched_trigger_subject_count.rs— 7 new integration testscrates/engine/tests/integration/turn_control_priority_softlock.rs— 2 new regression testsCR references
Track
Developer
LLM
Model: claude-sonnet-4-6
Thinking: medium
Verification
cargo fmt --all— cleancargo test -p engine— 8964 passed, 8 ignored, 0 failedScope Expansion
Turn-control priority softlock (CR 723) discovered during investigation of #707. Fixed in commit
58cea1754with dedicated integration testturn_control_priority_softlock.rs.Validation Failures
None.
CI Failures
None.