Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions crates/engine/src/game/casting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14283,11 +14283,18 @@ fn can_cast_prepared_now_with_probe(
}
}

// CR 118.9 + CR 601.2f + CR 119.8: Graveyard/exile cast-permission statics
// that carry a pay-life extra-cost rider (Valgavoth alternative; Festival of
// Embers additional) must afford the life payment for the cast to be legal.
// The remove-counters extra-cost (Dawnhand) carries no life payment, so
// `find_pay_life_cost` returns `None` and this gate is a no-op for it.
// CR 118.3 + CR 118.9 + CR 601.2f + CR 601.2h + CR 119.8: Graveyard/exile
// cast-permission statics that carry a non-mana extra-cost rider (Valgavoth
// alternative pay-life; Festival of Embers additional pay-life; Dragon Man,
// Reformed Robot additional discard) must be able to pay that cost in full for
// the cast to be legal. Use the general affordability authority
// (`AbilityCost::is_payable`, mirroring the Flashback gate above) rather than a
// pay-life special case: `is_payable`'s PayLife arm calls the same
// `can_pay_life_cast_or_activation_cost`, so pay-life legality is unchanged,
// while discard/sacrifice/remove-counter riders are now correctly gated so
// legal actions never offer an unpayable cast (e.g. Dragon Man from an empty
// hand). Mode-agnostic: an unpayable Alternative or Additional cost both make
// the cast illegal.
{
// CR 601.2a: Bind the exile extra-cost rider to the source this cast
// commits to — the recorded `ExilePermission` source if elected, else the
Expand All @@ -14311,11 +14318,8 @@ fn can_cast_prepared_now_with_probe(
_ => None,
};
if let Some(extra) = static_extra {
if let Some(amount) = find_pay_life_cost(&extra.cost, state, player, prepared.object_id)
{
if !super::life_costs::can_pay_life_cast_or_activation_cost(state, player, amount) {
return false;
}
if !extra.cost.is_payable(state, player, prepared.object_id) {
return false;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}
Expand Down
31 changes: 30 additions & 1 deletion crates/engine/src/game/cost_payability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
use crate::types::ability::{
is_variable_remove_counter_cost_count, AbilityCost, Comparator, CounterCostSelection,
FilterProp, QuantityExpr, QuantityRef, TapCreaturesAggregateStat, TapCreaturesRequirement,
TargetFilter, TypedFilter,
TargetFilter, TypedFilter, EXILE_COST_X,
};
use crate::types::card_type::CoreType;
use crate::types::identifiers::ObjectId;
Expand Down Expand Up @@ -406,6 +406,13 @@ impl AbilityCost {
zone,
filter,
} => {
// CR 107.3a + CR 601.2b: X in this cost is chosen during
// announcement. X=0 is legal, so the pre-announcement
// affordability gate must not treat its compact sentinel as a
// literal count that can never be met.
if *count == EXILE_COST_X {
return true;
}
if matches!(filter, Some(TargetFilter::SelfRef)) {
// CR 118.3 + CR 602.1a: "Exile this <self>" as an
// activation cost needs the source available to pay that
Expand Down Expand Up @@ -1246,6 +1253,28 @@ mod tests {
);
}

#[test]
fn variable_exile_cost_is_payable_at_x_zero() {
let mut scenario = GameScenario::new();
let source = scenario.add_creature(P0, "Harvest Pyre", 0, 1).id();
let cost = AbilityCost::Exile {
count: EXILE_COST_X,
zone: Some(Zone::Graveyard),
filter: Some(TargetFilter::Typed(TypedFilter::new(TypeFilter::Instant))),
};

assert!(
cost.is_payable(&scenario.state, P0, source),
"X exile costs are payable at X=0 before any eligible card is selected"
);

scenario.add_spell_to_graveyard(P0, "Lightning Bolt", true);
assert!(
cost.is_payable(&scenario.state, P0, source),
"X exile costs stay payable when eligible cards can set X above zero"
);
}

Comment on lines +1256 to +1277

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Exercise the real cast and payment path.

Both assertions call AbilityCost::is_payable, so both pass through the new unconditional sentinel branch. The test does not verify X announcement, positive-X selection, actual exile payment, or card movement.

Add an integration test under crates/engine/tests/integration/ and register it in crates/engine/tests/integration/main.rs. Cover X=0 with no eligible cards and positive X through the real casting pipeline.

As per path instructions, “Tests belong under crates/engine/tests/integration/ and must be registered in integration/main.rs.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/game/cost_payability.rs` around lines 1256 - 1277, The
current unit test only checks AbilityCost::is_payable and bypasses the real
cast/payment flow. Add an integration test under
crates/engine/tests/integration/ that casts the ability with X=0 when no
eligible cards exist, then covers positive-X announcement and selection with an
eligible graveyard card, verifying payment and card movement; register the test
module in integration/main.rs.

Source: Path instructions

#[test]
fn loyalty_positive_is_always_payable() {
let state = new_state();
Expand Down
92 changes: 87 additions & 5 deletions crates/engine/src/parser/oracle_casting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use nom::combinator::{all_consuming, map, opt, value};
use nom::sequence::{preceded, terminated};
use nom::Parser;

use super::oracle_cost::parse_oracle_cost;
use super::oracle_cost::{parse_gerund_cost, parse_oracle_cost};
use super::oracle_util::{parse_mana_symbols, parse_ordinal, TextPair};
use crate::parser::oracle_condition::parse_restriction_condition;
use crate::types::ability::{
Expand Down Expand Up @@ -220,11 +220,40 @@ fn parse_self_flash_option(
}
}

if let Ok((after, _)) = tag::<_, _, OracleError<'_>>("by ").parse(rest) {
if let Some(cost_text) = after.strip_suffix(" in addition to paying its other costs") {
option = option.cost(parse_oracle_cost(cost_text));
return Some(option);
if let Some(((), after)) = nom_on_lower(rest, &rest.to_lowercase(), |input| {
value((), tag::<_, _, OracleError<'_>>("by ")).parse(input)
}) {
let after = after.trim();
let after_lower = after.to_lowercase();
// CR 601.2f: the trailing closer has the same independent axes as the
// graveyard permission parser: optional "paying " and either possessive
// pronoun. A malformed closer must decline the whole option rather than
// fall through to an uncosted flash grant.
let (cost_len, _) = nom_on_lower(after, &after_lower, |input| {
all_consuming(map(
(
terminated(
take_until::<_, _, OracleError<'_>>(" in addition to "),
tag(" in addition to "),
),
opt(tag("paying ")),
alt((tag("their other costs"), tag("its other costs"))),
opt(tag(".")),
),
|(cost, _, _, _)| cost.len(),
))
.parse(input)
})?;
// CR 601.2f: the rider names the additional cost as a GERUND ("by
// discarding a card") — de-gerund via the shared cost authority. A
// present-but-unmodeled cost declines the whole option, avoiding a
// strictly-more-permissive cost-less flash permission.
let cost = parse_gerund_cost(&after[..cost_len]);
if matches!(cost, AbilityCost::Unimplemented { .. }) {
return None;
}
option = option.cost(cost);
return Some(option);
}

if let Ok((after, _)) = tag::<_, _, OracleError<'_>>("if you ").parse(rest) {
Expand Down Expand Up @@ -1639,6 +1668,59 @@ Trample";
}
}

/// CR 601.2f: a self-flash rider that names its additional cost as a GERUND
/// ("as though it had flash by discarding a card in addition to paying its
/// other costs") must de-gerund the cost via the shared authority and carry a
/// concrete Discard cost — not the `Unimplemented` the old imperative-only
/// `parse_oracle_cost(cost_text)` produced. Class completeness for the
/// "cast … by <gerund> in addition to …" family alongside the graveyard rider.
#[test]
fn self_flash_by_gerund_additional_cost_carries_discard() {
for closer in [
"its other costs",
"their other costs",
"paying its other costs",
"paying their other costs",
] {
let option = parse_spell_casting_option_line(
&format!(
"You may cast this spell as though it had flash by discarding a card in addition to {closer}."
),
"Test Card",
)
.expect("self-flash rider should parse");
match option {
SpellCastingOption {
kind: crate::types::ability::SpellCastingOptionKind::AsThoughHadFlash,
cost: Some(AbilityCost::Discard { .. }),
condition: None,
} => {}
other => panic!("expected AsThoughHadFlash with a Discard cost, got {other:?}"),
}
}
}

/// The paired negative: an unmodeled gerund cost on the self-flash rider must
/// DECLINE the whole option (return `None`), mirroring the graveyard
/// `AdditionalCostRider::Unmodeled` decline — NOT emit a cost-less flash grant
/// (which coverage would falsely mark supported). Asserting `is_none()` is the
/// load-bearing, discriminating check: it flips to failure the instant the
/// guard falls through to the cost-less `Some(option)` tail. A `!matches!(cost,
/// Some(Unimplemented))` assertion would pass vacuously for that exact
/// (dishonest) `cost == None` outcome, so it cannot catch the regression.
#[test]
fn self_flash_by_unmodeled_gerund_declines_option() {
let option = parse_spell_casting_option_line(
"You may cast this spell as though it had flash by frobnicating a card in addition to paying its other costs.",
"Test Card",
);
assert!(
option.is_none(),
"an unmodeled gerund additional cost must decline the whole self-flash \
option (honest coverage gap), not emit a cost-less flash grant: {option:?}"
);
}

#[test]
fn alt_cost_sacrifice_typed_creature_arm() {
// Delraich — "sacrifice three black creatures"
Expand Down
135 changes: 135 additions & 0 deletions crates/engine/src/parser/oracle_cost.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,48 @@ pub fn parse_oracle_cost(text: &str) -> AbilityCost {
parse_oracle_cost_no_or(text)
}

/// CR 601.2f: Parse a GERUND-form cost phrase ("discarding a card", "paying 1
/// life", "sacrificing a creature") into an `AbilityCost` by de-conjugating the
/// leading verb to its imperative stem and delegating to [`parse_oracle_cost`],
/// the single cost authority.
///
/// The gerund construction appears in "cast … by <doing X> in addition to
/// (paying) its other costs" ADDITIONAL-cost riders (Festival of Embers pay-life;
/// Dragon Man, Reformed Robot discard; Demilich / Helbrute exile-from-graveyard)
/// and in the self-flash rider in `oracle_casting.rs`. English gerund→imperative
/// is irregular (pay→paying, discard→discarding, sacrifice→sacrificing[−e],
/// remove→removing[−e], exile→exiling[−e], tap→tapping[+p]), so it cannot be a
/// generic `strip_suffix("ing")`; each verb is one composed `value(stem,
/// tag(gerund))` arm. Extend by a single arm per cost verb, only once
/// `parse_oracle_cost` models its imperative.
///
/// Returns `AbilityCost::Unimplemented { .. }` when the leading verb is not a
/// modeled cost gerund OR the delegated imperative is itself unmodeled, so
/// callers can decline (or drop) rather than silently attach a wrong/absent cost.
pub(crate) fn parse_gerund_cost(phrase: &str) -> AbilityCost {
type E<'a> = super::oracle_nom::error::OracleError<'a>;
let original = phrase.trim();
let lower = original.to_lowercase();
// Compose one `value(stem, tag(gerund))` arm per cost verb — each maps a
// gerund onto the imperative stem `parse_oracle_cost` already recognizes.
let Some((stem, rest)) = nom_on_lower(original, &lower, |input| {
alt((
value("pay", tag::<_, _, E<'_>>("paying ")),
value("discard", tag("discarding ")),
value("sacrifice", tag("sacrificing ")),
value("tap", tag("tapping ")),
value("remove", tag("removing ")),
value("exile", tag("exiling ")),
))
.parse(input)
}) else {
return AbilityCost::Unimplemented {
description: original.to_string(),
};
};
parse_oracle_cost(&format!("{stem} {rest}"))
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/// True when a top-level ` or ` branch parsed to a concrete activation cost
/// rather than falling through to `Unimplemented` / `EffectCost`.
fn is_disjunctive_alt_cost(cost: &AbilityCost) -> bool {
Expand Down Expand Up @@ -1795,6 +1837,19 @@ fn extract_filter_zone(filter: &TargetFilter) -> Option<Zone> {
None
}
}),
// Recurse into composite filters so a multi-type source-zone cost carries
// the same top-level `zone` a single-type one would. "Exile four instant
// and/or sorcery cards from your graveyard" (Demilich) lowers to an
// `Or([Typed{Instant, InZone(Graveyard)}, Typed{Sorcery, InZone(Graveyard)}])`
// filter; without this, `zone` stayed `None` and the payment layer's
// no-zone default (`exile_cost_effective_zone`) looked in the hand instead
// of the graveyard, making the cost unpayable and the card uncastable.
// Every leg of these disjunctions names the same zone, so the first leg
// that yields a zone is authoritative.
TargetFilter::Or { filters } | TargetFilter::And { filters } => {
filters.iter().find_map(extract_filter_zone)
}
TargetFilter::Not { filter } => extract_filter_zone(filter),
_ => None,
}
}
Expand Down Expand Up @@ -1892,6 +1947,86 @@ mod tests {
assert_eq!(parse_oracle_cost("{T}"), AbilityCost::Tap);
}

/// CR 601.2f: `parse_gerund_cost` de-conjugates the gerund verb and delegates
/// to the single cost authority, so a gerund cost phrase lowers identically to
/// its imperative form across the whole verb class — and an unmodeled verb
/// stays honest `Unimplemented`. Tests the building block, not one card.
#[test]
fn gerund_cost_matches_imperative_authority() {
for (gerund, imperative) in [
("discarding a card", "discard a card"),
("paying 1 life", "pay 1 life"),
("sacrificing a creature", "sacrifice a creature"),
("sacrificing a Vehicle", "sacrifice a Vehicle"),
// CR 701.13a: the exile arm — Demilich / Helbrute cast-from-graveyard
// riders exile cards as an additional cost.
(
"exiling four instant and/or sorcery cards from your graveyard",
"exile four instant and/or sorcery cards from your graveyard",
),
(
"exiling another creature card from your graveyard",
"exile another creature card from your graveyard",
),
] {
assert_eq!(
parse_gerund_cost(gerund),
parse_oracle_cost(imperative),
"gerund {gerund:?} must lower like imperative {imperative:?}"
);
}
assert!(matches!(
parse_gerund_cost("sacrificing a Vehicle"),
AbilityCost::Sacrifice(SacrificeCost {
target: TargetFilter::Typed(TypedFilter { type_filters, .. }),
..
}) if type_filters == [TypeFilter::Subtype("Vehicle".to_string())]
));
// The required-for-this-fix arm is concretely a discard-a-card cost.
assert!(
matches!(
parse_gerund_cost("discarding a card"),
AbilityCost::Discard { .. }
),
"discarding a card must lower to a Discard cost"
);
// CR 701.13a: the exile arm lowers to a real graveyard Exile cost — the
// regression that turned Demilich/Helbrute from castable-with-dropped-cost
// into declined-and-uncastable is fixed at its root (the missing gerund).
assert!(
matches!(
parse_gerund_cost("exiling four instant and/or sorcery cards from your graveyard"),
AbilityCost::Exile {
count: 4,
zone: Some(Zone::Graveyard),
filter: Some(_),
}
),
"Demilich's exile-four rider must lower to an Exile-from-graveyard cost, got {:?}",
parse_gerund_cost("exiling four instant and/or sorcery cards from your graveyard")
);
assert!(
matches!(
parse_gerund_cost("exiling another creature card from your graveyard"),
AbilityCost::Exile {
count: 1,
zone: Some(Zone::Graveyard),
filter: Some(_),
}
),
"Helbrute's exile-another-creature rider must lower to an Exile-from-graveyard cost, got {:?}",
parse_gerund_cost("exiling another creature card from your graveyard")
);
// A verb the cost authority does not model stays honest.
assert!(
matches!(
parse_gerund_cost("frobnicating a card"),
AbilityCost::Unimplemented { .. }
),
"an unmodeled gerund verb must lower to Unimplemented"
);
}

#[test]
fn cost_explicit_count_continuation_with_unmodeled_rider_stays_unimplemented() {
// Terminal explicit-count guard: a "<N>=2 …" continuation whose object
Expand Down
2 changes: 1 addition & 1 deletion crates/engine/src/parser/oracle_static/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ mod prelude {
pub(super) use nom::sequence::{preceded, terminated};
pub(super) use nom::Parser;

pub(super) use super::super::oracle_cost::parse_oracle_cost;
pub(super) use super::super::oracle_cost::{parse_gerund_cost, parse_oracle_cost};
pub(super) use super::super::oracle_effect::subject::{
parse_restriction_modes, static_mode_needs_grant_propagation,
};
Expand Down
Loading
Loading