From c5024c97678be3ffdcbac252e235cd8ed145fdab Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 21 Jul 2026 15:48:21 +0000 Subject: [PATCH] fix(parser): parse "Activate only during an opponent's upkeep" (Trade Caravan) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trade Caravan's activated ability "Remove two currency counters from ~: Untap target basic land. Activate only during an opponent's upkeep." dropped its timing restriction to `Effect::Unimplemented`, leaving the ability activatable at any time. `parse_activation_during_role_gate` handled "during an opponent's turn" and "during your upkeep" but not "during an opponent's upkeep". Rather than add a `DuringOpponents*` sibling to `ActivationRestriction` (which expresses opponent-turn scope via `RequiresCondition { Not(IsYourTurn) }`, not a dedicated variant), compose from building blocks: - Add `ParsedCondition::IsDuringUpkeep` (CR 503.1) — a scope-free upkeep-step predicate, orthogonal to every existing `ParsedCondition` variant. - Map "during an opponent's upkeep" to `RequiresCondition { And([Not(IsYourTurn), IsDuringUpkeep]) }` (CR 602.5b + CR 102.1 + CR 503.1), reusing the existing opponent-turn composition idiom. - Evaluate `IsDuringUpkeep` against `state.phase == Phase::Upkeep`; classify it memo-safe in the AI condition filter (reads only apply()-constant state). Removes Trade Caravan from the parser-misparse backlog (root cause 28). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01HwvJgpQpXqutChWLLLGB8E --- crates/engine/src/ai_support/filter.rs | 2 + crates/engine/src/game/restrictions.rs | 51 ++++++++++++++++++++++++ crates/engine/src/parser/oracle.rs | 25 ++++++++++++ crates/engine/src/parser/oracle_tests.rs | 47 ++++++++++++++++++++++ crates/engine/src/types/ability.rs | 9 +++++ docs/parser-misparse-backlog.md | 3 +- 6 files changed, 135 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/ai_support/filter.rs b/crates/engine/src/ai_support/filter.rs index 837f7d6aff..7c2fadf8ef 100644 --- a/crates/engine/src/ai_support/filter.rs +++ b/crates/engine/src/ai_support/filter.rs @@ -918,6 +918,8 @@ fn condition_reads_only_memo_safe_state(c: &ParsedCondition) -> bool { | ParsedCondition::CardsLeftYourGraveyardThisTurnAtLeast { .. } | ParsedCondition::PlayerCountAtLeast { .. } | ParsedCondition::HasCityBlessing + // CR 503.1: reads only `state.phase`, apply()-constant global state. + | ParsedCondition::IsDuringUpkeep | ParsedCondition::IsYourTurn => true, } } diff --git a/crates/engine/src/game/restrictions.rs b/crates/engine/src/game/restrictions.rs index fcc586e577..62e5e17153 100644 --- a/crates/engine/src/game/restrictions.rs +++ b/crates/engine/src/game/restrictions.rs @@ -1518,6 +1518,9 @@ pub(crate) fn evaluate_condition( ParsedCondition::HasCityBlessing => state.city_blessing.contains(&player), // CR 102.1: "The active player is the player whose turn it is." ParsedCondition::IsYourTurn => state.active_player == player, + // CR 503.1: The game is currently in the upkeep step. Player scope, if + // any, is composed by the caller via `And([Not(IsYourTurn), ..])`. + ParsedCondition::IsDuringUpkeep => state.phase == Phase::Upkeep, // CR 601.3d + CR 608.2c: "if it targets a [filter]" — gates a casting // permission on the chosen targets of the in-flight spell. Read from // `state.pending_cast.ability.targets` when targets have been committed. @@ -3092,6 +3095,54 @@ mod tests { )); } + #[test] + fn opponents_upkeep_activation_condition_gates_on_step_and_scope() { + // CR 602.5b + CR 102.1 + CR 503.1: "Activate only during an opponent's + // upkeep" is the composed restriction the parser emits for Trade Caravan — + // `And([Not(IsYourTurn), IsDuringUpkeep])`. Drive the real + // `evaluate_condition` across the full turn-scope × step matrix so the gate + // holds only in an opponent's upkeep step. + let mut state = crate::types::game_state::GameState::new_two_player(42); + let condition = ParsedCondition::And { + conditions: vec![ + ParsedCondition::Not { + condition: Box::new(ParsedCondition::IsYourTurn), + }, + ParsedCondition::IsDuringUpkeep, + ], + }; + let activator = PlayerId(0); + + // Opponent's turn, upkeep step -> allowed. + state.active_player = PlayerId(1); + state.phase = Phase::Upkeep; + assert!(evaluate_condition( + &state, + activator, + ObjectId(1), + &condition + )); + + // Opponent's turn, non-upkeep step -> denied (IsDuringUpkeep false). + state.phase = Phase::PreCombatMain; + assert!(!evaluate_condition( + &state, + activator, + ObjectId(1), + &condition + )); + + // Your turn, upkeep step -> denied (Not(IsYourTurn) false). + state.active_player = PlayerId(0); + state.phase = Phase::Upkeep; + assert!(!evaluate_condition( + &state, + activator, + ObjectId(1), + &condition + )); + } + #[test] fn evaluates_creatures_you_control_total_power_condition() { let mut state = crate::types::game_state::GameState::new_two_player(42); diff --git a/crates/engine/src/parser/oracle.rs b/crates/engine/src/parser/oracle.rs index 0d9a98cbdf..04dd27d50b 100644 --- a/crates/engine/src/parser/oracle.rs +++ b/crates/engine/src/parser/oracle.rs @@ -6781,6 +6781,13 @@ fn parse_activation_during_role_gate(i: &str) -> OracleResult<'_, ActivationRest tag::<_, _, OracleError<'_>>("as a sorcery"), ), value(ActivationRestriction::AsInstant, tag("as an instant")), + value( + opponents_upkeep_activation_restriction(), + alt(( + tag("during an opponent's upkeep"), + tag("during an opponents upkeep"), + )), + ), value( opponents_turn_activation_restriction(), alt(( @@ -7010,6 +7017,24 @@ fn opponents_turn_activation_condition() -> ParsedCondition { } } +/// CR 602.5b + CR 102.1 + CR 503.1: "Activate only during an opponent's upkeep" +/// gates activation to the upkeep step of a turn where the activator is not the +/// active player (Trade Caravan). Composed from the same `Not(IsYourTurn)` +/// opponent-turn leaf as `opponents_turn_activation_condition` plus the +/// `IsDuringUpkeep` step predicate, so the opponent scope reuses the existing +/// composition idiom instead of a dedicated `DuringOpponents*` restriction +/// sibling. +fn opponents_upkeep_activation_restriction() -> ActivationRestriction { + ActivationRestriction::RequiresCondition { + condition: Some(ParsedCondition::And { + conditions: vec![ + opponents_turn_activation_condition(), + ParsedCondition::IsDuringUpkeep, + ], + }), + } +} + pub(super) fn strip_activated_constraints(text: &str) -> (String, ActivatedConstraintAst) { let mut remaining = text.trim().trim_end_matches('.').trim().to_string(); let mut constraints = ActivatedConstraintAst::default(); diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index c768d5d951..2529b76926 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -248,6 +248,53 @@ fn activated_ability_opponent_turn_restriction_uses_not_your_turn_condition() { ); } +/// CR 602.5b + CR 102.1 + CR 503.1: "Activate only during an opponent's upkeep" +/// (Trade Caravan) composes the opponent-turn scope (`Not(IsYourTurn)`) with the +/// upkeep-step predicate (`IsDuringUpkeep`) inside a single `RequiresCondition`, +/// reusing the same composition idiom as the bare opponent-turn gate above rather +/// than a dedicated `DuringOpponents*` restriction sibling. Before the fix the +/// tail dropped to `Effect::Unimplemented` and the ability was activatable at any +/// time. +#[test] +fn activated_ability_opponents_upkeep_restriction_composes_scope_and_step() { + let r = parse( + "Remove two currency counters from ~: Untap target basic land. \ + Activate only during an opponent's upkeep.", + "Trade Caravan", + &[], + &["Creature"], + &["Human", "Nomad"], + ); + + assert_eq!(r.abilities.len(), 1, "got {:#?}", r.abilities); + let ability = &r.abilities[0]; + assert!( + !matches!(ability.effect.as_ref(), Effect::Unimplemented { .. }) + && ability + .sub_ability + .as_ref() + .is_none_or(|sub| !matches!(sub.effect.as_ref(), Effect::Unimplemented { .. })), + "expected no unimplemented fallback, got {:#?}", + ability + ); + + let expected = ActivationRestriction::RequiresCondition { + condition: Some(ParsedCondition::And { + conditions: vec![ + ParsedCondition::Not { + condition: Box::new(ParsedCondition::IsYourTurn), + }, + ParsedCondition::IsDuringUpkeep, + ], + }), + }; + assert!( + ability.activation_restrictions.contains(&expected), + "expected composed opponent's-upkeep restriction, got {:?}", + ability.activation_restrictions + ); +} + /// CR 508.1: a STANDALONE combat-window activation gate — "Activate only before /// attackers are declared" / "Activate only before combat" (Arcum's Whistle, /// Arcum's Sleigh) with no "during " first half — must map to the enforced diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 41cb7523c9..1ffffce138 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -7600,6 +7600,15 @@ pub enum ParsedCondition { /// ability-resolution layer), the same way `ParsedCondition::And` mirrors /// `AbilityCondition::And`. IsYourTurn, + /// CR 503.1: True when the game is currently in the upkeep step. A + /// turn-structure predicate with NO player scope — it asks only "is it an + /// upkeep step", not whose. Player scope composes at the restriction layer: + /// "during an opponent's upkeep" is `And([Not(IsYourTurn), IsDuringUpkeep])` + /// (CR 102.1 fixes the active player from the turn), mirroring how + /// opponent-turn activation timing reuses `Not(IsYourTurn)` (see + /// `opponents_turn_activation_condition`) rather than a dedicated + /// `DuringOpponents*` restriction sibling. + IsDuringUpkeep, /// CR 601.3d + CR 702.8a + CR 608.2c: The in-flight spell being cast targets at /// least one object that matches `filter`. Gates a target-dependent casting /// permission (Timely Ward — "you may cast this spell as though it had flash if diff --git a/docs/parser-misparse-backlog.md b/docs/parser-misparse-backlog.md index d3bfd7c824..3130248b0b 100644 --- a/docs/parser-misparse-backlog.md +++ b/docs/parser-misparse-backlog.md @@ -5071,7 +5071,7 @@ This is the prioritized "fix N root causes → unlock M cards" backlog: the top -### 28. Trigger/activation timing or ordinal restriction dropped (14 cards) +### 28. Trigger/activation timing or ordinal restriction dropped (13 cards) **Signature.** A timing/scope restriction (OnlyDuringYourTurn / OncePerTurn / 'during an opponent's turn' / Nth-spell ordinal / cast-timing) is null; the constraint tail is not parsed. @@ -5090,7 +5090,6 @@ This is the prioritized "fix N root causes → unlock M cards" backlog: the top - Shadowheart, Sharran Cleric - Skarrgan Hellkite - Tomb Tyrant -- Trade Caravan - Uthros Research Craft - Uthros, Titanic Godcore