diff --git a/crates/sage-engine/data/catalog/resplendent_angel.json b/crates/sage-engine/data/catalog/resplendent_angel.json index 6fe071ca..14e66a89 100644 --- a/crates/sage-engine/data/catalog/resplendent_angel.json +++ b/crates/sage-engine/data/catalog/resplendent_angel.json @@ -58,6 +58,25 @@ ] } ] + }, + { + "type": "activated", + "cost": [ + { + "kind": "mana", + "mana": "{3}{W}{W}{W}" + } + ], + "effects": [ + { + "kind": "pump_self", + "power": 2, + "toughness": 2, + "keywords": [ + "lifelink" + ] + } + ] } ] } diff --git a/crates/sage-engine/data/catalog/skyrider_patrol.json b/crates/sage-engine/data/catalog/skyrider_patrol.json index 140780b2..85bfd162 100644 --- a/crates/sage-engine/data/catalog/skyrider_patrol.json +++ b/crates/sage-engine/data/catalog/skyrider_patrol.json @@ -40,12 +40,10 @@ "kind": "put_counters", "target": "another_creature_you_control", "counter": "plus_one_plus_one", - "count": 1 - }, - { - "kind": "grant_keyword", - "target": "another_creature_you_control", - "keyword": "flying" + "count": 1, + "keywords": [ + "flying" + ] } ] } diff --git a/crates/sage-engine/src/ability/effect.rs b/crates/sage-engine/src/ability/effect.rs index 132ced29..7bfb320e 100644 --- a/crates/sage-engine/src/ability/effect.rs +++ b/crates/sage-engine/src/ability/effect.rs @@ -307,6 +307,25 @@ pub enum Effect { /// card that names two creatures gives each of them the same X. #[serde(default)] count_amount: Option, + /// Keyword abilities granted to the **same** target(s) until end of turn, applied + /// at CR 613 layer 6 exactly as [`Effect::Pump`]'s are. Empty for the ordinary + /// counter placement, which is nearly every card. + /// + /// Here for the reason they are on [`Effect::Pump`], and it is the reason rather + /// than an analogy: one effect declares one target group, so a card printed as + /// `put a +1/+1 counter on another target creature you control, **and that + /// creature** gains flying until end of turn` has to be one effect. Authored as a + /// counter beside a standalone [`Effect::GrantKeyword`] it would advertise two + /// independent slots and let a player counter one creature while a different one + /// gained flying — which is exactly what Skyrider Patrol did before this field + /// existed (issue #821). The catalog validator now refuses that shape outright + /// ([`Violation::TwoTargetsOfOneClass`](crate::Violation)). + /// + /// The counter and the grant have different lifetimes and are meant to: a counter + /// stays on the permanent, and the keyword is gone at cleanup (CR 514.2). One + /// printed sentence, two durations, one target. + #[serde(default)] + keywords: Vec, }, /// Give the single creature this effect targets `+power`/`+toughness` /// **until end of turn** — the pump-spell verb (e.g. `Target creature gets @@ -525,11 +544,29 @@ pub enum Effect { /// not a *target* (CR 115.1), so this chooses nothing, fills no slot, and can /// never fizzle. A source that has left the battlefield by the time the ability /// resolves is simply not there to modify, and the effect does nothing. + /// + /// The optional `keywords` are granted to that same source, at CR 613 layer 6 and for + /// the same until-end-of-turn duration — the self row of what [`Effect::Pump`] carries + /// for a target and [`Effect::GrantKeywordAll`] for a class. `{3}{W}{W}{W}: Until end + /// of turn, this creature gets +2/+2 and gains lifelink` is **one** printed sentence + /// about one permanent, so it is one effect with one CR 613.7 timestamp. + /// + /// Deliberately not spelled through [`Effect::AlterAbilitiesSelf`], which is the + /// verb for a clause that *subtracts* — `loses defender and gains flying`, and its + /// `lose_all`. Reaching for the lose-all-abilities verb to say "gains lifelink" reads + /// as a card doing something it does not do, and the absence of any other way to say + /// it is why Resplendent Angel's third ability was missing from the catalog + /// altogether (issue #821). PumpSelf { /// The signed amount added to the source's power until end of turn. power: i32, /// The signed amount added to the source's toughness until end of turn. toughness: i32, + /// Keyword abilities granted to the source until end of turn. Empty for the + /// ordinary self-pump that only changes numbers. Duplicate grants are redundant, + /// not additive (CR 613.1f). + #[serde(default)] + keywords: Vec, }, /// Change what abilities **this ability's own source** has until end of turn at /// CR 613 **layer 6** — `loses defender and gains flying until end of turn`, or diff --git a/crates/sage-engine/src/ability/tests.rs b/crates/sage-engine/src/ability/tests.rs index dd02d78f..18251b42 100644 --- a/crates/sage-engine/src/ability/tests.rs +++ b/crates/sage-engine/src/ability/tests.rs @@ -466,6 +466,7 @@ fn issue_149_put_counters_round_trips_with_both_kinds() { serde_json::from_str::(plus).unwrap(), Effect::PutCounters { count_amount: None, + keywords: Vec::new(), targets: crate::ability::TargetCount::Exactly(1), target: TargetSpec::AnyCreature, counter: CounterKind::PlusOnePlusOne, @@ -477,6 +478,7 @@ fn issue_149_put_counters_round_trips_with_both_kinds() { serde_json::from_str::(minus).unwrap(), Effect::PutCounters { count_amount: None, + keywords: Vec::new(), targets: crate::ability::TargetCount::Exactly(1), target: TargetSpec::AnyCreature, counter: CounterKind::MinusOneMinusOne, diff --git a/crates/sage-engine/src/apply/cast/effects.rs b/crates/sage-engine/src/apply/cast/effects.rs index 1a983569..76cfb596 100644 --- a/crates/sage-engine/src/apply/cast/effects.rs +++ b/crates/sage-engine/src/apply/cast/effects.rs @@ -496,7 +496,11 @@ pub(crate) fn apply_effect( // not a target (CR 115.1) and so was never chosen. A source that has left the // battlefield is not there to modify, and the effect simply does nothing — // the same no-op a fizzled target produces, without the fizzle. - Effect::PumpSelf { power, toughness } => { + Effect::PumpSelf { + power, + toughness, + keywords, + } => { if let Some(id) = permanent_source { if state.battlefield.iter().any(|p| p.id == id) { let stamp = state.mint_id(); @@ -509,6 +513,17 @@ pub(crate) fn apply_effect( }, duration: Duration::UntilEndOfTurn, }); + // The same source, in the same breath, at layer 6 instead of 7c — + // the self-referential half of what `Effect::Pump` does for a target. + for keyword in keywords { + let stamp = state.mint_id(); + state.static_effects.push(StaticEffect { + source: stamp, + affects: EffectAffects::SpecificPermanent(id), + modification: Modification::GrantKeyword(*keyword), + duration: Duration::UntilEndOfTurn, + }); + } } } } diff --git a/crates/sage-engine/src/apply/cast/targeted.rs b/crates/sage-engine/src/apply/cast/targeted.rs index ec0d0722..9c04d3a4 100644 --- a/crates/sage-engine/src/apply/cast/targeted.rs +++ b/crates/sage-engine/src/apply/cast/targeted.rs @@ -183,6 +183,7 @@ pub(crate) fn apply_targeted_effect( counter, count, count_amount, + keywords, .. } => { // The printed number, or the one the game supplies (CR 608.2) — read here @@ -202,6 +203,20 @@ pub(crate) fn apply_targeted_effect( }; if let Target::Permanent(id) = target { state.put_counters_on_permanent(id, *counter, count, db); + // A keyword the same sentence grants the same creature, at CR 613 layer + // 6 and until end of turn — the counter stays and the keyword does not, + // which is the two durations one printed sentence really has. The + // permanent has been re-checked as a legal target by the caller + // (CR 608.2b); one that has since left is skipped by the `if let` above. + for keyword in keywords { + let stamp = state.mint_id(); + state.static_effects.push(StaticEffect { + source: stamp, + affects: EffectAffects::SpecificPermanent(id), + modification: Modification::GrantKeyword(*keyword), + duration: Duration::UntilEndOfTurn, + }); + } } } // Pump the targeted creature until end of turn (CR 514.2): add a diff --git a/crates/sage-engine/src/apply/combat/strike/dies_tests.rs b/crates/sage-engine/src/apply/combat/strike/dies_tests.rs index 1d9c0559..766f229f 100644 --- a/crates/sage-engine/src/apply/combat/strike/dies_tests.rs +++ b/crates/sage-engine/src/apply/combat/strike/dies_tests.rs @@ -96,6 +96,7 @@ fn issue_151_dies_trigger_fires_from_a_minus_one_counter_toughness_drop() { lurker, vec![Effect::PutCounters { count_amount: None, + keywords: Vec::new(), targets: crate::ability::TargetCount::Exactly(1), target: TargetSpec::AnyCreature, counter: CounterKind::MinusOneMinusOne, diff --git a/crates/sage-engine/src/card/helpers.rs b/crates/sage-engine/src/card/helpers.rs index e5ebf065..fc9880b0 100644 --- a/crates/sage-engine/src/card/helpers.rs +++ b/crates/sage-engine/src/card/helpers.rs @@ -531,6 +531,7 @@ mod tests { event: TriggerCondition::SelfEntersBattlefield, effects: vec![Effect::PutCounters { count_amount: None, + keywords: Vec::new(), targets: crate::ability::TargetCount::Exactly(1), target: TargetSpec::AnyCreature, counter: CounterKind::PlusOnePlusOne, @@ -550,6 +551,7 @@ mod tests { crate::card::tests::card_named(&inline, "test_wither").spell_effects, vec![Effect::PutCounters { count_amount: None, + keywords: Vec::new(), targets: crate::ability::TargetCount::Exactly(1), target: TargetSpec::AnyCreature, counter: CounterKind::MinusOneMinusOne, diff --git a/crates/sage-engine/src/resolve/tests.rs b/crates/sage-engine/src/resolve/tests.rs index 3cc863d9..b302fa1f 100644 --- a/crates/sage-engine/src/resolve/tests.rs +++ b/crates/sage-engine/src/resolve/tests.rs @@ -424,6 +424,7 @@ fn issue_149_put_counters_ability_lands_on_its_target_cr_122() { origin: AbilityOrigin::Activated, effects: vec![Effect::PutCounters { count_amount: None, + keywords: Vec::new(), targets: crate::ability::TargetCount::Exactly(1), target: TargetSpec::AnyCreature, counter: CounterKind::PlusOnePlusOne, diff --git a/crates/sage-engine/tests/m19_ability_removal.rs b/crates/sage-engine/tests/m19_ability_removal.rs index f181d13c..25551a64 100644 --- a/crates/sage-engine/tests/m19_ability_removal.rs +++ b/crates/sage-engine/tests/m19_ability_removal.rs @@ -464,3 +464,71 @@ fn cr_613_1f_a_keyword_gained_in_the_same_clause_survives_the_loss_of_everything "a granted keyword is not an entry in the ability list" ); } + +// ----- Resplendent Angel: layer 6 adding, on the same row ------------------- + +/// The additive counterpart, and the reason it is not spelled with the verb above. +/// +/// `{3}{W}{W}{W}: Until end of turn, this creature gets +2/+2 and gains lifelink` was +/// **missing from the catalog entirely** (#819, #821), and the shape of the vocabulary is +/// why: the only self-directed keyword *addition* was `alter_abilities_self`'s `gain`, +/// whose sibling field is `lose_all`. Reaching for the lose-all-abilities verb to say +/// "gains lifelink" reads as a card doing something it does not do, so a `pump_self` +/// carries its own keywords now — one printed sentence, one effect, one CR 613.7 +/// timestamp. +#[test] +fn issue_821_the_angel_pumps_and_grants_in_one_breath() { + let db = db(); + let mut state = main_phase(&db, "forest"); + let angel = place(&mut state, &db, "resplendent_angel", PlayerId(0)); + + let before = characteristics(&state, angel, &db); + assert_eq!((before.power, before.toughness), (Some(3), Some(3))); + assert!(before.keywords.contains(&Keyword::Flying), "printed flying"); + assert!(!before.keywords.contains(&Keyword::Lifelink)); + + let state = activate(&state, &db, angel, 1); + + let after = characteristics(&state, angel, &db); + assert_eq!( + (after.power, after.toughness), + (Some(5), Some(5)), + "a 3/3 with +2/+2" + ); + assert!(after.keywords.contains(&Keyword::Lifelink), "and lifelink"); + assert!( + after.keywords.contains(&Keyword::Flying), + "and the flying it printed — this grants, it does not replace, which is the \ + whole difference from the removal verb above" + ); +} + +/// Both halves are `until end of turn` and both are gone at cleanup (CR 514.2), with +/// nothing written onto the permanent to undo (ADR 0005). +#[test] +fn issue_821_the_angels_pump_and_its_lifelink_wear_off_together() { + let db = db(); + let mut state = main_phase(&db, "forest"); + let angel = place(&mut state, &db, "resplendent_angel", PlayerId(0)); + + let state = activate(&state, &db, angel, 1); + assert!(characteristics(&state, angel, &db) + .keywords + .contains(&Keyword::Lifelink)); + + let next_turn = settle_until(&state, &db, |s| s.turn == 2 && s.step == Step::Upkeep); + let after = characteristics(&next_turn, angel, &db); + assert_eq!( + (after.power, after.toughness), + (Some(3), Some(3)), + "the pump is gone" + ); + assert!( + !after.keywords.contains(&Keyword::Lifelink), + "and so is the keyword it came with" + ); + assert!( + after.keywords.contains(&Keyword::Flying), + "the printed keyword was never touched" + ); +} diff --git a/crates/sage-engine/tests/m19_pay_for_a_trigger.rs b/crates/sage-engine/tests/m19_pay_for_a_trigger.rs index 70f932c1..acc67c7f 100644 --- a/crates/sage-engine/tests/m19_pay_for_a_trigger.rs +++ b/crates/sage-engine/tests/m19_pay_for_a_trigger.rs @@ -183,14 +183,24 @@ fn an_unpayable_offer_is_not_posed() { assert!(state.stack.is_empty()); } -/// Skyrider Patrol buys a two-effect ability, and both effects land on the one creature -/// its single slot named. +/// Skyrider Patrol buys **one** effect for **one** creature: `put a +1/+1 counter on +/// another target creature you control, and that creature gains flying until end of +/// turn` names one creature and then points back at it. +/// +/// The ability was authored as a counter beside a standalone keyword grant, which is two +/// effects each declaring the same target class — so it advertised two slots and this +/// test filled both of them with the same creature, which is how it stayed green while +/// the card was wrong (issue #821). A second ally is on the board now, and the slot count +/// is asserted rather than assumed. #[test] -fn skyrider_patrol_buys_a_counter_and_a_keyword_for_one_creature() { +fn issue_821_skyrider_patrol_buys_a_counter_and_a_keyword_for_one_creature() { let db = db(); let mut state = main_phase(&db); let patrol = place(&mut state, &db, "skyrider_patrol", PlayerId(0)); let ally = place(&mut state, &db, "onakke_ogre", PlayerId(0)); + // A second creature the ability could have aimed at, so "one slot" is a claim that + // can fail rather than a board with only one answer available. + let bystander = place(&mut state, &db, "centaur_courser", PlayerId(0)); // Two lands to pay {G}{U} with. The pool empties between steps, so the mana has to be // made *while the offer is owed* (CR 605.3a) — which is the only time it is legal. let forest = place(&mut state, &db, "forest", PlayerId(0)); @@ -225,12 +235,29 @@ fn skyrider_patrol_buys_a_counter_and_a_keyword_for_one_creature() { let state = apply_action(&state, &Action::AnswerConfirm { accept: true }, &db); let ability = pending_trigger_target_choice(&state).expect("the ability owes targets"); + + // One slot, not two. A second target would be a card asking a question it does not + // print, and the answer would let a player counter one creature and fly another. + let two_targets = apply_action( + &state, + &Action::ChooseTriggerTargets { + ability, + mode: None, + targets: vec![Target::Permanent(ally), Target::Permanent(bystander)], + }, + &db, + ); + assert_eq!( + two_targets, state, + "the ability declares one target group, so a two-target answer is refused" + ); + let state = apply_action( &state, &Action::ChooseTriggerTargets { ability, mode: None, - targets: vec![Target::Permanent(ally), Target::Permanent(ally)], + targets: vec![Target::Permanent(ally)], }, &db, ); @@ -240,6 +267,14 @@ fn skyrider_patrol_buys_a_counter_and_a_keyword_for_one_creature() { let stats = characteristics(&state, ally, &db); assert_eq!(stats.power, Some(5), "a 4/2 with a +1/+1 counter"); assert!(stats.keywords.contains(&Keyword::Flying), "and flying"); + // Both halves landed on the one creature the slot named, and nothing reached the + // other one — which is the whole of what "and that creature" means. + let untouched = characteristics(&state, bystander, &db); + assert_eq!(untouched.power, Some(3), "the bystander took no counter"); + assert!( + !untouched.keywords.contains(&Keyword::Flying), + "and gained no flying" + ); // "Another" is the source-relative class: the Patrol is not one of its own targets. assert_eq!( characteristics(&state, patrol, &db).power, diff --git a/crates/sage-server/src/rules_text/effects.rs b/crates/sage-server/src/rules_text/effects.rs index 6cbf20e9..aba66980 100644 --- a/crates/sage-server/src/rules_text/effects.rs +++ b/crates/sage-server/src/rules_text/effects.rs @@ -213,6 +213,7 @@ pub(super) fn effect_clause(source: &str, effect: &Effect) -> String { counter, count, count_amount, + keywords, } => { // "X +1/+1 counters, where X is your life total" — the letter in quantity // position and the source named after it, which is how a card prints an @@ -222,13 +223,27 @@ pub(super) fn effect_clause(source: &str, effect: &Effect) -> String { Some(_) => format!("X {} counters", crate::rules_text::counter_symbol(*counter)), }; let clause = format!("put {what} on {}", target_phrase(*target, *targets)); - match count_amount { + let clause = match count_amount { None => clause, Some(amount) => format!( "{clause}, where X is {}", amount_noun(amount, PlayerRef::Controller) ), + }; + // A keyword the same sentence grants goes to the same creature, and the card + // says so with a pronoun rather than by naming the class twice — "and **that + // creature** gains flying until end of turn". The pronoun is what the second + // clause is for; repeating the noun would read as a second target, which is + // precisely the thing the one-effect shape exists to prevent. + if keywords.is_empty() { + return clause; } + let words: Vec<&str> = keywords.iter().map(|&kw| keyword_word(kw)).collect(); + format!( + "{clause}, and {} gains {} until end of turn", + counter_pronoun(*targets), + list_words(&words) + ) } // One effect, one target, one sentence: the keywords a pump also grants are // granted to the same creature, so they read as a second verb on the same @@ -306,8 +321,21 @@ pub(super) fn effect_clause(source: &str, effect: &Effect) -> String { ), // A self-referential effect names the source by name, so the sentence reads // the way the card does rather than as an anonymous "this". - Effect::PumpSelf { power, toughness } => { - format!("{source} gets {power:+}/{toughness:+} until end of turn") + // The self row of what `Effect::Pump` says for a target: one subject, one + // duration, and the keywords the same sentence grants read as a second verb on + // it rather than as a sentence of their own. + Effect::PumpSelf { + power, + toughness, + keywords, + } => { + let mut clauses = vec![format!("gets {power:+}/{toughness:+}")]; + if !keywords.is_empty() { + let words: Vec<&str> = keywords.iter().map(|&kw| keyword_word(kw)).collect(); + clauses.push(format!("gains {}", list_words(&words))); + } + let verbs = list_words(&clauses.iter().map(String::as_str).collect::>()); + format!("{source} {verbs} until end of turn") } Effect::RestrictSelf { restriction } => { format!("{source} {} this turn", restriction_predicate(restriction)) @@ -1076,6 +1104,21 @@ fn entering_noun(filter: &EnteringFilter) -> String { /// A target group as the phrase a card writes it in: `target creature` for the ordinary /// single-target effect, `each of up to two target creatures` for the one that may name /// fewer than it allows. +/// The pronoun a counter-and-grant sentence refers back to its own target with — `that +/// creature` for one, `those creatures` for a group. +/// +/// The whole point of the pronoun is that it is **not** the noun: a card that says "put a +/// +1/+1 counter on another target creature you control, and *that creature* gains +/// flying" names one creature and then points at it. Writing the class out twice would +/// read as a second target, which is the misauthoring +/// [`Violation::TwoTargetsOfOneClass`](sage_engine::Violation) exists to refuse. +fn counter_pronoun(count: TargetCount) -> &'static str { + match count { + TargetCount::Exactly(1) | TargetCount::UpTo(1) => "that creature", + _ => "those creatures", + } +} + fn target_phrase(spec: TargetSpec, count: TargetCount) -> String { match count { TargetCount::Exactly(1) => target_noun(spec), diff --git a/crates/sage-server/src/rules_text/tests.rs b/crates/sage-server/src/rules_text/tests.rs index 108c85de..af109d7d 100644 --- a/crates/sage-server/src/rules_text/tests.rs +++ b/crates/sage-server/src/rules_text/tests.rs @@ -1253,7 +1253,40 @@ fn a_life_gained_condition_states_its_threshold_only_when_there_is_one() { assert_eq!( text_of(&db, "resplendent_angel"), "Flying\nAt the beginning of each end step, if you gained five or more life \ - this turn, you create a 4/4 white Angel creature token with flying and vigilance." + this turn, you create a 4/4 white Angel creature token with flying and \ + vigilance.\n{3}{W}{W}{W}: Resplendent Angel gets +2/+2 and gains lifelink \ + until end of turn." + ); +} + +/// One printed sentence about the **source** is one clause with one subject and one +/// duration — the self row of what a pump-and-grant says for a target (issue #821). +#[test] +fn issue_821_a_self_pump_that_also_grants_reads_as_one_sentence() { + let db = bundled(); + let text = text_of(&db, "resplendent_angel"); + assert_eq!( + text.lines().last(), + Some("{3}{W}{W}{W}: Resplendent Angel gets +2/+2 and gains lifelink until end of turn."), + "the pump and the grant share one subject and one duration" + ); +} + +/// A counter and the keyword the same sentence grants the same creature: the second +/// clause points back with a **pronoun**, because writing the class out twice is how a +/// card with one target comes to advertise two slots (issue #821). +#[test] +fn issue_821_a_counter_and_its_grant_name_the_creature_once() { + let db = bundled(); + let text = text_of(&db, "skyrider_patrol"); + assert_eq!( + text.lines().last(), + Some( + "At the beginning of combat on your turn, you may pay {G}{U}. When you do, \ + put a +1/+1 counter on another target creature you control, and that \ + creature gains flying until end of turn." + ), + "one target named once, and pointed back at" ); } diff --git a/docs/card-schema.md b/docs/card-schema.md index 07d4a38d..af562ef8 100644 --- a/docs/card-schema.md +++ b/docs/card-schema.md @@ -154,7 +154,35 @@ keywords are read (combat legality, evasion, damage, view projection, generated One effect declares one target group, so two effects would advertise two independent slots and let a player pump one creature while a different one gained flying. Author - the two-effect form only when the card really names two targets. + the two-effect form only when the card really names two targets — the engine supports + two same-class slots and pairs them positionally, so nothing refuses the shape; it is + the *card* that decides, and Skyrider Patrol shipped with two slots for one printed + target because nobody checked (issue #821). +- The same rule, on the two other subjects a sentence can have: + - **The source.** `{3}{W}{W}{W}: Until end of turn, this creature gets +2/+2 and gains + lifelink` is one `pump_self` carrying `keywords`, never a `pump_self` beside an + `alter_abilities_self`: + + ```json + {"kind": "pump_self", "power": 2, "toughness": 2, "keywords": ["lifelink"]} + ``` + + `alter_abilities_self` is the verb for a clause that **subtracts** — *loses defender + and gains flying*, and `lose_all`. Reaching for the lose-all-abilities verb to say + "gains lifelink" reads as a card doing something it does not do, and the absence of + any other way to say it is why Resplendent Angel's third ability was missing from the + catalog altogether. + - **A counter's target.** `put a +1/+1 counter on another target creature you control, + and that creature gains flying until end of turn` is one `put_counters` carrying + `keywords`: + + ```json + {"kind": "put_counters", "target": "another_creature_you_control", + "counter": "plus_one_plus_one", "count": 1, "keywords": ["flying"]} + ``` + + The counter stays on the permanent and the keyword is gone at cleanup: one printed + sentence, two durations, one target. A `restrictions` list rides beside `keywords` on the same effect, for the same reason and with the same until-end-of-turn duration — including the one *requirement* in that