Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 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
200 changes: 200 additions & 0 deletions crates/engine/src/game/effects/attach.rs
Original file line number Diff line number Diff line change
Expand Up @@ -596,18 +596,111 @@ pub(crate) fn attachment_illegality(
// being attached to the protected permanent.
// CR 702.16d: Protection from a quality prevents Equipment or Fortifications
// of that quality from being attached to the protected permanent.
// CR 702.16c–702.16d: Protection from a quality prevents new attachments of
// that quality, but CR 702.16n/702.16p exempt already-attached Auras and
// Equipment named by the protection-granting effect.
if let (Some(host), Some(attachment)) = (
state.objects.get(&host_id),
state.objects.get(&attachment_id),
) {
if crate::game::keywords::protection_prevents_from(host, attachment) {
if protection_doesnt_remove_attached_exemption(
state,
host_id,
attachment_id,
attacher_is_aura,
attacher_is_equipment,
) {
return None;
}
Comment thread
matthewevans marked this conversation as resolved.
Outdated
return Some(AttachIllegality::Protection);
}
}

None
}

/// CR 702.16n / CR 702.16p: already-attached Auras/Equipment exempt from
/// protection-based detachment when the host carries the matching exemption
/// static from the protection-granting effect.
fn protection_doesnt_remove_attached_exemption(
state: &GameState,
host_id: ObjectId,
attachment_id: ObjectId,
attacher_is_aura: bool,
attacher_is_equipment: bool,
) -> bool {
let Some(host) = state.objects.get(&host_id) else {
return false;
};
if !host.attachments.contains(&attachment_id) {
return false;
}

let host_has_this_aura =
crate::game::functioning_abilities::active_static_definitions(state, host).any(|def| {
matches!(
def.mode,
crate::types::statics::StaticMode::ProtectionDoesntRemoveThisAura
)
});
let host_has_controlled =
crate::game::functioning_abilities::active_static_definitions(state, host).any(|def| {
matches!(
def.mode,
crate::types::statics::StaticMode::ProtectionDoesntRemoveControlledAttachments
)
});

if host_has_this_aura
&& attacher_is_aura
&& aura_grants_protection_attachment_exemption(
state,
attachment_id,
crate::types::statics::StaticMode::ProtectionDoesntRemoveThisAura,
)
{
return true;
}

if host_has_controlled && (attacher_is_aura || attacher_is_equipment) {
let Some(attachment) = state.objects.get(&attachment_id) else {
return false;
};
return host.attachments.iter().any(|&grantor_id| {
aura_grants_protection_attachment_exemption(
state,
grantor_id,
crate::types::statics::StaticMode::ProtectionDoesntRemoveControlledAttachments,
) && state
.objects
.get(&grantor_id)
.is_some_and(|grantor| grantor.controller == attachment.controller)
});
}

false
}

fn aura_grants_protection_attachment_exemption(
state: &GameState,
aura_id: ObjectId,
mode: crate::types::statics::StaticMode,
) -> bool {
let Some(aura) = state.objects.get(&aura_id) else {
return false;
};
crate::game::functioning_abilities::active_static_definitions(state, aura).any(|def| {
def.modifications.iter().any(|m| {
matches!(
m,
crate::types::ability::ContinuousModification::AddStaticMode { mode: m }
if *m == mode
)
})
})
}

/// CR 301.5 + CR 303.4 + CR 701.3a: True unless `host_id` is forbidden by a
/// positive "can be attached only to {filter}" restriction on `attachment_id`.
///
Expand Down Expand Up @@ -873,6 +966,113 @@ mod tests {
assert!(!can_attach_to_object(&state, aura, creature));
}

#[test]
fn attachment_illegality_protection_exemption_keeps_attached_controlled_aura() {
// CR 702.16p (issue #4964): Benevolent Blessing — protection from the
// chosen color must not detach your already-attached Auras/Equipment.
let mut state = setup();
let aura = spawn_with_subtype(&mut state, "Benevolent Blessing", "Aura");
{
let obj = state.objects.get_mut(&aura).unwrap();
obj.card_types.core_types.push(CoreType::Enchantment);
obj.color.push(crate::types::mana::ManaColor::White);
}
let creature = spawn_creature(&mut state, "Bear");
state
.objects
.get_mut(&creature)
.unwrap()
.attachments
.push(aura);
state.objects.get_mut(&creature).unwrap().keywords.push(
crate::types::keywords::Keyword::Protection(
crate::types::keywords::ProtectionTarget::Color(
crate::types::mana::ManaColor::White,
),
),
);
state
.objects
.get_mut(&aura)
.unwrap()
.static_definitions
.push(StaticDefinition::continuous().modifications(vec![
crate::types::ability::ContinuousModification::AddStaticMode {
mode: StaticMode::ProtectionDoesntRemoveControlledAttachments,
},
]));
state
.objects
.get_mut(&creature)
.unwrap()
.static_definitions
.push(StaticDefinition::new(
StaticMode::ProtectionDoesntRemoveControlledAttachments,
));

assert_eq!(attachment_illegality(&state, aura, creature), None);
assert!(can_attach_to_object(&state, aura, creature));
}

/// CR 702.16n (issue #4964 review): the "this Aura" exemption
/// (`ProtectionDoesntRemoveThisAura`, Pentarch Ward class) is
/// source-specific — it protects ONLY the Aura that granted it. A second
/// controlled Aura attached to the same protected host is still detached by
/// protection. Two controlled Auras on one protected host: the granting Aura
/// is exempt, the sibling Aura is not.
#[test]
fn attachment_illegality_this_aura_exemption_is_source_specific() {
let mut state = setup();

// Granting Aura: carries the "doesn't remove this Aura" grant AND is the
// aura that supplies the host's protection static.
let granting_aura = spawn_with_subtype(&mut state, "Pentarch Ward", "Aura");
{
let obj = state.objects.get_mut(&granting_aura).unwrap();
obj.card_types.core_types.push(CoreType::Enchantment);
obj.color.push(crate::types::mana::ManaColor::White);
obj.static_definitions
.push(StaticDefinition::continuous().modifications(vec![
crate::types::ability::ContinuousModification::AddStaticMode {
mode: StaticMode::ProtectionDoesntRemoveThisAura,
},
]));
}

// Sibling Aura: same controller, plain white Aura, NO exemption grant.
let sibling_aura = spawn_with_subtype(&mut state, "Pacifism", "Aura");
{
let obj = state.objects.get_mut(&sibling_aura).unwrap();
obj.card_types.core_types.push(CoreType::Enchantment);
obj.color.push(crate::types::mana::ManaColor::White);
}

let creature = spawn_creature(&mut state, "Bear");
{
let obj = state.objects.get_mut(&creature).unwrap();
obj.attachments.push(granting_aura);
obj.attachments.push(sibling_aura);
obj.keywords
.push(crate::types::keywords::Keyword::Protection(
crate::types::keywords::ProtectionTarget::Color(
crate::types::mana::ManaColor::White,
),
));
// Host carries the applied "this Aura" static from the granting Aura.
obj.static_definitions.push(StaticDefinition::new(
StaticMode::ProtectionDoesntRemoveThisAura,
));
}

// Granting Aura is exempt; sibling Aura is still removed by protection.
assert_eq!(attachment_illegality(&state, granting_aura, creature), None);
assert_eq!(
attachment_illegality(&state, sibling_aura, creature),
Some(AttachIllegality::Protection),
"a sibling controlled Aura must NOT share the source Aura's CR 702.16n exemption"
);
}

#[test]
fn attachment_illegality_cant_be_enchanted_blocks_aura() {
// CR 303.4c: other applicable effects can make an Aura's host illegal.
Expand Down
7 changes: 7 additions & 0 deletions crates/engine/src/game/static_abilities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,13 @@ pub fn build_static_registry() -> HashMap<StaticMode, StaticAbilityHandler> {
// CR 704.5j: LegendRuleDoesntApply — affected permanents are excluded from
// the legend-rule SBA. Runtime enforcement is in sba.rs::legend_rule_exempt().
registry.insert(StaticMode::LegendRuleDoesntApply, handle_rule_mod);
// CR 702.16n / CR 702.16p: protection attachment exemptions — enforced in
// effects/attach.rs::attachment_illegality via active static scan.
registry.insert(StaticMode::ProtectionDoesntRemoveThisAura, handle_rule_mod);
registry.insert(
StaticMode::ProtectionDoesntRemoveControlledAttachments,
handle_rule_mod,
);
// CR 702.179e: Card-specific rule modification allowing speed to exceed 4.
registry.insert(StaticMode::SpeedCanIncreaseBeyondFour, handle_rule_mod);
// CR 609.4b: "You may spend mana as though it were mana of any color."
Expand Down
65 changes: 60 additions & 5 deletions crates/engine/src/parser/oracle_static/keyword_grant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1400,13 +1400,27 @@ pub(crate) fn parse_continuous_modifications(text: &str) -> Vec<ContinuousModifi
}
}
} else if let Some(keyword_text) = extract_keyword_clause(&unquoted_text) {
for part in split_keyword_list(keyword_text.trim().trim_end_matches('.')) {
let (keyword_only, trailing_exemption) =
match super::oracle_nom::bridge::split_once_on_lower(
keyword_text,
&keyword_text.to_lowercase(),
". ",
) {
Some((first, rest)) if trailing_mentions_protection_attachment_exemption(rest) => {
(first, Some(rest.trim()))
}
_ => (keyword_text, None),
};
Comment on lines +1581 to +1593

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.

high

[HIGH] Verbatim string matching and .contains() used for parsing dispatch.

Why it matters: Using .contains() for parsing dispatch in non-test parser code violates Rule R1 [33] and bypasses the robust nom-based parser, creating fragile matches.

Suggested fix: Use a dedicated nom parser to recognize the trailing exemption prose.

        let (keyword_only, trailing_exemption) = 
            match super::oracle_nom::bridge::split_once_on_lower(
                keyword_text,
                &keyword_text.to_lowercase(),
                ". ",
            ) {
                Some((first, rest)) if parse_protection_exemption(rest.trim()).is_ok() => {
                    (first, Some(rest.trim()))
                }
                _ => (keyword_text, None),
            };
References
  1. R1. Nom combinators on the first pass — no exceptions. Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. (link)

for part in split_keyword_list(keyword_only.trim().trim_end_matches('.')) {
push_grant_clause_modifications(
&mut modifications,
part.as_ref(),
where_x_expression.as_deref(),
);
}
if let Some(trailing) = trailing_exemption {
push_protection_attachment_exemption_modifications(&mut modifications, trailing);
}
}

// CR 613.1f: Pre-quote keyword recovery for compound lines like Swashbuckler's
Expand Down Expand Up @@ -1461,11 +1475,12 @@ pub(crate) fn push_grant_clause_modifications(
// parse_quoted_ability_modifications at :798) before extract_keyword_clause
// runs, so any ". " here can only introduce a trailing inert prose sentence
// (e.g. Benevolent Blessing's SBA-exemption "This effect doesn't remove ...").
// Drop it so the keyword sentence reaches map_keyword clean.
let part =
// Strip it from the keyword token, but emit the matching protection-attachment
// exemption static when recognized.
let (part, trailing_exemption) =
match super::oracle_nom::bridge::split_once_on_lower(part, &part.to_lowercase(), ". ") {
Some((first, _)) => first,
None => part,
Some((first, rest)) => (first, Some(rest.trim())),
None => (part, None),
};

let part_trimmed = part.trim().trim_end_matches('.');
Expand Down Expand Up @@ -1538,6 +1553,9 @@ pub(crate) fn push_grant_clause_modifications(

if let Some(kw) = map_keyword(part_trimmed) {
modifications.push(ContinuousModification::AddKeyword { keyword: kw });
if let Some(trailing) = trailing_exemption {
push_protection_attachment_exemption_modifications(modifications, trailing);
}
return;
}

Expand Down Expand Up @@ -1565,6 +1583,43 @@ pub(crate) fn push_grant_clause_modifications(
}
}

fn trailing_mentions_protection_attachment_exemption(text: &str) -> bool {
nom_primitives::scan_contains(text, "doesn't remove")
|| nom_primitives::scan_contains(text, "does not remove")
}

fn parse_protection_attachment_exemption_trailing(text: &str) -> Option<StaticMode> {
let lower = text.trim().to_ascii_lowercase();
let mut this_aura = alt((
tag::<_, _, OracleError<'_>>("this effect doesn't remove this aura"),
tag("this effect does not remove this aura"),
tag("doesn't remove this aura"),
tag("does not remove this aura"),
));
if this_aura.parse(lower.as_str()).is_ok() {
return Some(StaticMode::ProtectionDoesntRemoveThisAura);
}
let mut controlled = alt((
tag::<_, _, OracleError<'_>>("this effect doesn't remove auras and equipment you control"),
tag("this effect does not remove auras and equipment you control"),
tag("doesn't remove auras and equipment you control"),
tag("does not remove auras and equipment you control"),
));
if controlled.parse(lower.as_str()).is_ok() {
return Some(StaticMode::ProtectionDoesntRemoveControlledAttachments);
}
None
}

fn push_protection_attachment_exemption_modifications(
modifications: &mut Vec<ContinuousModification>,
trailing: &str,
) {
if let Some(mode) = parse_protection_attachment_exemption_trailing(trailing) {
modifications.push(ContinuousModification::AddStaticMode { mode });
}
}
Comment on lines +1865 to +1872

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.

high

[HIGH] Verbatim string matching used for parsing Oracle phrases.

Why it matters: Using verbatim string matching for compound phrases bypasses the robust nom-based parser and creates fragile matches, violating the repository rule on Oracle phrase parsing.

Suggested fix: Decompose compound phrases into modular, reusable parsers for constituent parts (such as the subject, negation, and target) and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.

fn parse_protection_exemption(input: &str) -> nom::IResult<&str, StaticMode> {
    use nom::{
        branch::alt,
        bytes::complete::tag_no_case,
        combinator::value,
        sequence::tuple,
    };

    let effect_parser = tag_no_case("this effect ");
    let negation_parser = alt((tag_no_case("doesn't remove "), tag_no_case("does not remove ")));

    let this_aura = value(
        StaticMode::ProtectionDoesntRemoveThisAura,
        tag_no_case("this aura")
    );

    let controlled_attachments = value(
        StaticMode::ProtectionDoesntRemoveControlledAttachments,
        tag_no_case("auras and equipment you control that are already attached to it")
    );

    let (input, (_, _, mode)) = tuple((effect_parser, negation_parser, alt((this_aura, controlled_attachments))))(input)?;
    Ok((input, mode))
}

fn push_protection_attachment_exemption_modifications(
    modifications: &mut Vec<ContinuousModification>,
    trailing: &str,
) {
    if let Ok((_, mode)) = parse_protection_exemption(trailing) {
        modifications.push(ContinuousModification::AddStaticMode { mode });
    }
}
References
  1. R1. Nom combinators on the first pass — no exceptions. Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. (link)
  2. Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.


/// Extract quoted ability text from Oracle text and parse each into a typed AbilityDefinition.
///
/// Quoted abilities like `"{T}: Add two mana of any one color."` are parsed by splitting
Expand Down
Loading
Loading