fix(attach): honor protection attachment exemptions (#4964)

[bohdansolovie]

Summary

• Emit ProtectionDoesntRemoveControlledAttachments / ProtectionDoesntRemoveThisAura statics from protection-grant trailing text (CR 702.16p/n)
• Exempt already-attached controlled Auras/Equipment in attachment_illegality
• Preserve keyword-grant parsing by stripping exemption prose before map_keyword

Fixes #4964

Test plan

• cargo test -p engine --lib protection_chosen_color_drops_trailing
• cargo test -p engine --lib attachment_illegality_protection_exemption

[gemini-code-assist]

Code Review

This pull request implements CR 702.16n and CR 702.16p to exempt already-attached Auras and Equipment from protection-based detachment when the host carries a matching exemption static. While the implementation successfully adds the necessary static modes, attachment legality checks, and parser updates, several critical issues must be addressed. First, the parser changes in keyword_grant.rs violate Rule R1 by using verbatim string matching and .contains() instead of nom-based combinators. Second, the controller and source checks in attach.rs do not strictly adhere to the MTG Comprehensive Rules, as they incorrectly reference the host's controller instead of the protection-granting source's controller. Finally, a mandatory CR annotation comment is missing in attach.rs per Rule R6.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

[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)

[gemini-code-assist]

[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.

[gemini-code-assist]

[HIGH] Incorrect controller and source checks for protection attachment exemptions.

Why it matters: Under CR 702.16n, ProtectionDoesntRemoveThisAura only protects the specific Aura that is the source of the protection effect (not any Aura controlled by the host's controller). Under CR 702.16p, ProtectionDoesntRemoveControlledAttachments protects attachments controlled by the controller of the protection-granting Aura (def.controller or def.source_id's controller), not the host's controller (host.controller).

Suggested fix: Update the checks to correctly reference the static definition's source or controller.

References

1. Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. ^(link)

[gemini-code-assist]

[MEDIUM] Missing CR annotation comment.

Why it matters: Every rules-touching line of engine code must carry a comment of the form CR <number>: <description> [108] to ensure rules verification and traceability.

Suggested fix: Add a CR comment directly above the exemption check.

Suggested change

	if protection_doesnt_remove_attached_exemption(
	state,
	host_id,
	attachment_id,
	attacher_is_aura,
	attacher_is_equipment,
	) {
	return None;
	}
	// CR 702.16n / CR 702.16p: Exempt already-attached Auras/Equipment if the host has the matching exemption static.
	if protection_doesnt_remove_attached_exemption(
	state,
	host_id,
	attachment_id,
	attacher_is_aura,
	attacher_is_equipment,
	) {
	return None;
	}

References

1. R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . ^(link)

[matthewevans]

The ProtectionDoesntRemoveThisAura runtime check is too broad. In protection_doesnt_remove_attached_exemption, the ProtectionDoesntRemoveThisAura arm accepts any already-attached Aura controlled by the host controller once the host has that static. CR 702.16n says the exemption is for that specific Aura; another controlled Aura attached to the same creature should still be affected by protection.

Please make the “this Aura” exemption source/attachment-specific, or model it at the attachment/source level instead of as a host-wide static, and add a regression with two controlled Auras attached to the same protected host proving only the source Aura is exempt.

[github-actions]

Parse changes introduced by this PR · 2 card(s), 2 signature(s) (baseline: main 184c1304bb52)

1 card(s) · static/Continuous · field mods: grant Protection → grant Protection, ProtectionDoesntRemoveControlledAttachments

Examples: Benevolent Blessing

1 card(s) · static/Continuous · field mods: power +2, toughness +2, grant Protection → power +2, toughness +2, grant Protection, grant Protection, grant Protection, grant Protection, grant Protection, Prote…

Examples: Spectra Ward

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[matthewevans]

Requesting changes on the current head. The source-specific ProtectionDoesntRemoveThisAura issue is closer, but the current parser diff still shows an unrelated regression.

[MED] The current coverage-parse-diff sticky changes Spectra Ward from power +2, toughness +2, grant Protection to power +2, toughness +2, grant Protection, grant Protection, grant Protection, grant Protection, grant Protection. That is outside the Benevolent Blessing / attachment-exemption fix and means the keyword-grant trailing-prose splitter is still changing sibling protection text. Please keep the exemption parsing from duplicating Spectra Ward's protection grants, and add coverage that would fail on this parse-diff regression.

The existing runtime test is useful for the attachment exemption path, but it does not exercise the production Oracle parse shape that regressed here, so CI can still false-green on the parser blast radius.

[matthewevans]

Requesting changes on current head 57d3f1e02d7a2505f1057fba00b68563b2bab639.

[MED] The current coverage-parse-diff still shows parser blast radius outside the Benevolent Blessing attachment-exemption scope. Evidence: the sticky updated for this head still changes Spectra Ward from power +2, toughness +2, grant Protection to power +2, toughness +2, grant Protection, grant Protection, grant Protection, grant Protection, grant Protection, and also changes unrelated signatures such as ReduceAbilityCost affects fields, static_structure, orphaned_copy_retarget, Ravenous Trap, Teyo, and neighbor-attack cards. Why it matters: this PR is supposed to add CR 702.16n/p attachment exemptions, not rewrite unrelated parser output; the added Spectra Ward unit test calls parse_continuous_modifications on a shortened text fragment, so it does not cover the production card parse path that the sticky says is still regressing. Suggested fix: constrain the exemption splitter so the full card-data parse for Spectra Ward and the unrelated cards is unchanged, then add a production parse/card-data regression that would fail on the duplicated-protection sticky diff.

[matthewevans]

[MED] The current head still has parser blast radius outside the Benevolent Blessing / protection-attachment exemption scope. Evidence: the current coverage-parse-diff sticky updated at 2026-07-03T17:08:31Z still changes Spectra Ward from power +2, toughness +2, grant Protection to power +2, toughness +2, grant Protection, grant Protection, grant Protection, grant Protection, grant Protection, and also reports unrelated signature churn such as Mystic Barrier/Pramikon static-structure changes and multiple ReduceAbilityCost affects drops. Why it matters: this PR is supposed to model CR 702.16n/p attachment exemptions, not alter sibling protection parses or unrelated card signatures; merging it would make coverage less honest outside the claimed issue. Suggested fix: constrain the exemption splitter so only the recognized this Aura / Auras and Equipment you control that are already attached forms are peeled, then get the sticky down to the intended Benevolent Blessing / Ward-class deltas only.

[bohdansolovie]

Thanks @matthewevans — fixed the Spectra Ward blast radius on the new head (cbff90bc9).

Root cause. The pre-split exemption peel fired on any trailing "doesn't remove" sentence. Spectra Ward's production line is ... protection from each color. This effect doesn't remove Auras. (reminder) — peeling that tail cleaned protection from each color, which then let split_keyword_list / expand_protection_parts fan the leg out into the five WUBRG grants. That is the grant Protection → grant Protection ×5 diff in the sticky.

Fix. The peel is now gated on a recognized exemption (parse_protection_attachment_exemption_trailing(rest).is_some()), not on the mere presence of "doesn't remove". Spectra Ward's "...doesn't remove Auras." is not a modeled exemption, so it stays glued and the protection parse is byte-for-byte what origin/main produces (a single grant). I confirmed this against origin/main by diffing the production parse_static_line_multi output. The change now narrows the peel vs. the previous head, so non-exemption cards are parsed exactly as main.

Production coverage (via parse_static_line_multi, which strips reminder text exactly like the card-data export, so it exercises the same path the sticky diffs):

• spectra_ward_production_line_protection_not_duplicated_by_exemption_splitter — real Oracle line → exactly one protection grant, no exemption static (fails if it fans out to 5).
• benevolent_blessing_production_line_emits_controlled_attachment_exemption — real line → Protection(ChosenColor) + ProtectionDoesntRemoveControlledAttachments (this is the card whose "Auras and Equipment" inner and needs the pre-split peel).
• ward_of_lights_production_line_emits_this_aura_exemption — real line → Protection(ChosenColor) + source-specific ProtectionDoesntRemoveThisAura.

The earlier fragment test (... protection from the color of your choice.) was removed; it never exercised the each-color fan-out and so hid the regression. The unrelated signatures (ReduceAbilityCost / Teyo / Ravenous Trap) are pre-existing baseline noise — the keyword-grant change only touches recognized-exemption cards.

[matthewevans]

[HIGH] Protection attachment exemptions incorrectly override unrelated protection instances. Evidence: crates/engine/src/game/effects/attach.rs:606. Why it matters: CR 702.16n/p explicitly says other instances of protection from the same quality affect attached permanents normally, but this code first asks whether any protection keyword prevents the attachment and then exempts via a separate host/static scan at crates/engine/src/game/effects/attach.rs:607, while crates/engine/src/game/keywords.rs:384 has no source/provenance for the protection instance that matched. Suggested fix: bind the exemption to the specific protection-granting continuous effect/source/quality, and add a regression where Benevolent Blessing or Ward grants an exemption but another same-quality protection instance still detaches the Aura/Equipment.

[bohdansolovie]

Thanks @matthewevans — fixed the exemption-provenance issue on the new head (52b0e30e9).

Root cause. attachment_illegality asked two independent questions with no provenance link between them: (1) does protection_prevents_from(host, attachment) see any matching protection keyword (baked into host.keywords, source lost), and (2) does the host carry any ProtectionDoesntRemove* static. So once a host had a Benevolent Blessing / Ward exemption at all, every same-quality protection instance was exempted — including protection from an unrelated source that CR 702.16n/p says must still detach the Aura/Equipment.

Fix — bind the exemption to the specific granting effect. The exemption and the protection grant live on the same StaticDefinition (Benevolent Blessing parses to one Continuous static: mods: [AddKeyword(Protection(ChosenColor)), AddStaticMode(ProtectionDoesntRemoveControlledAttachments)]). That def is the provenance. attachment_exempt_from_protection now scans the functioning protection-granting static defs (battlefield_active_statics, re-running each affected filter against the host) and, for each def whose AddKeyword(Protection) matches the attachment's quality, asks whether that same def exempts this attachment:

• ProtectionDoesntRemoveThisAura → attachment.id == source_obj.id (CR 702.16n: only the source Aura itself).
• ProtectionDoesntRemoveControlledAttachments → attachment.controller == source_obj.controller (CR 702.16p: controller of the granting effect, per the earlier review too).

An attachment stays only if every matching protection instance comes from an effect that exempts it. Any non-exempting instance — a different effect, the host's intrinsic (printed) base_keywords, or a transient grant — detaches it. It's fail-closed: a baked protection quality on the host that can't be traced to an exempting granting effect (e.g. "gains protection until end of turn") is treated as non-exempt.

Coverage. The two prior exemption tests modeled protection as a bare baked keyword + a separate exemption static (exactly the no-provenance shape this review flagged); they now model protection as a granting continuous effect. New regression attachment_illegality_other_protection_instance_still_detaches_exempt_aura: a host enchanted by Benevolent Blessing (grants protection-white + controlled-attachment exemption) and a second white-protection Aura with no exemption — Benevolent Blessing is still detached (Some(AttachIllegality::Protection)). attachment_illegality_this_aura_exemption_is_source_specific still proves a sibling controlled Aura is not covered by the source Aura's this Aura exemption.

Scope. Runtime-only change (crates/engine/src/game/effects/attach.rs); no parser code touched, so no new coverage-parse-diff blast radius. Locally green: cargo fmt, scripts/check-parser-combinators.sh, cargo clippy -p engine --lib --tests, cargo test -p engine --lib (14801 passed), and cargo test -p engine --test '*'.

[matthewevans]

[HIGH] Transient same-quality protection grants can still be masked by the exempting protection grant. Evidence: crates/engine/src/game/effects/attach.rs:689 reconstructs protection provenance only from battlefield_active_statics, then crates/engine/src/game/effects/attach.rs:730 treats the baked host.keywords protection quality as covered if any exempting battlefield static granted that same ProtectionTarget. But transient continuous effects also feed the layer pipeline (crates/engine/src/game/layers.rs:3518), and identical protection keywords from different sources coalesce into one keyword entry (crates/engine/src/game/layers.rs:4673; existing test at crates/engine/src/game/layers.rs:14185). Why it matters: Benevolent Blessing plus a later transient "gains protection from white until end of turn" can still leave the Aura attached because the unexempted transient instance has no surviving provenance in host.keywords, contrary to CR 702.16n/p's "other instances" rule. Please make attachment_exempt_from_protection enumerate every matching protection instance from the same authorities the layer pipeline uses, including transient continuous effects, and require each source/effect instance to carry its own matching exemption.

The current parse-diff scope is clean from this pass; this blocker is the remaining runtime provenance issue.

[matthewevans]

[HIGH] Transient same-quality protection grants can still be masked by the exempting protection grant. Evidence: crates/engine/src/game/effects/attach.rs:689 reconstructs protection provenance only from battlefield_active_statics, while crates/engine/src/game/layers.rs:3518 feeds transient continuous effects into the same layer pipeline and crates/engine/src/game/layers.rs:4673 deduplicates identical Protection(Color(_)) values into one baked keyword; then crates/engine/src/game/effects/attach.rs:730 checks only whether the baked protection target is present in the exempted set. Why it matters: Benevolent Blessing plus a later transient “gains protection from white until end of turn” can still leave the Aura attached because the transient same-quality protection instance has no surviving provenance in host.keywords, contrary to CR 702.16n/p’s “other instances” rule. Suggested fix: enumerate active transient protection grants alongside the active static grants, or otherwise preserve per-effect protection provenance through the exemption check, and add a regression with an exempting Aura plus a same-quality transient protection grant.

[bohdansolovie]

Thanks — fixed in 751ecaf28. You were right: battlefield_active_statics alone missed transient grants, and since layers.rs deduplicates identical Protection(Color(_)) values into a single baked keyword, a same-quality transient grant had no surviving provenance in host.keywords, so the exempting static's baked target masked it.

Fix — enumerate transient protection grants alongside the static grants, with per-effect provenance:

• New authority layers::for_each_transient_protection_grant(state, affected_id, visit) walks state.transient_continuous_effects and surfaces each active protection-granting TCE together with its full sibling modification list. It reuses the exact gating from gather_transient_continuous_effects (UntilHostLeavesPlay source-on-battlefield, transient_duration_holds, source_condition_gate_passes, plus the recipient-context condition and the affected filter), so only grants that actually bake onto the recipient are surfaced.
• attach.rs now classifies static grants (via battlefield_active_statics) and transient grants through one shared ProtectionExemptionScan::classify_effect. Because the exemption AddStaticMode and the AddKeyword(Protection) are read off the same effect's modification list, the exemption binds only to its own grant — any other same-quality instance (printed keyword, a different static effect, or a transient grant) flips has_unexempted and detaches.

So Benevolent Blessing + a later "gains protection from white until end of turn" now correctly detaches the Aura per CR 702.16n/p's "other instances" rule. The baked-keyword protection_targets_matching pass is kept only as a fail-closed catch-all for exotic sources (e.g. copy effects) with no traceable exempting effect.

Regression: attachment_illegality_transient_protection_still_detaches_exempt_aura — exempting Benevolent Blessing plus an unexempted same-quality transient Protection(White) grant (added via add_transient_continuous_effect, Duration::UntilEndOfTurn); asserts the Aura is still detached (Some(AttachIllegality::Protection)).

Local: fmt, clippy -D warnings, parser-combinator gate, check-engine-authorities.sh origin/main, and cargo test -p engine --lib (14,802 passed) all green.

[matthewevans]

[MED] CR 702.16n’s “all Auras” exemption is still explicitly left unmodeled. Evidence: docs/MagicCompRules.txt:4067 says protection-granting Auras can say the effect does not remove either that specific Aura or all Auras, but crates/engine/src/parser/oracle_static/tests.rs:22239 asserts Spectra Ward’s “This effect doesn’t remove Auras” emits no exemption static at all. Why it matters: Spectra Ward-class protection still detaches matching Auras even though this PR is claiming CR 702.16n/p attachment-exemption support. Suggested fix: model the exemption scope explicitly, e.g. ThisAura, AllAuras, and the already-attached controlled Aura/Equipment scope, then add parser and runtime regressions for Spectra Ward.

[bohdansolovie]

Thanks @matthewevans — modeled the CR 702.16n all-Auras exemption on the new head (f2940111e).

Root cause. Spectra Ward's This effect doesn't remove Auras was the one gap in the exemption family: the trailing sentence was deliberately left unrecognized (to avoid the earlier blast radius), so the line lowered to a single inert Protection(CardType("each color")) grant with no exemption at all — Spectra Ward granted no functional color protection and still detached every matching Aura.

Fix — the scope is now modeled explicitly:

• New StaticMode::ProtectionDoesntRemoveAuras (+ StaticModeKind, Display/FromStr/as_keyword, static-abilities registry), alongside the existing ProtectionDoesntRemoveThisAura (CR 702.16n "this Aura") and ProtectionDoesntRemoveControlledAttachments (CR 702.16p).
• Parser: parse_protection_attachment_exemption_trailing now recognizes the terminal This effect doesn't remove Auras sentence. It's matched last (it's a prefix of the controlled-attachments form) and only as a complete trailing sentence (end-anchored), so Guardian Beast's …Auras already attached to those artifacts and the controlled form are excluded. Recognizing it lets the peel fire, so the recovered protection from each color correctly fans into the five WUBRG grants (CR 105.2) instead of the inert CardType grant.
• Runtime (attach.rs): the all-Auras form exempts any already-attached Aura of the granted quality (not just the source Aura or controlled ones), bound to that same granting effect. Equipment of the quality is still detached — the printed form names only Auras.

Parse-diff is clean. I merged the latest origin/main so the baseline is current main (this also drops the earlier behind-main contamination). Reproduced locally against the current-main coverage baseline — 2 cards, 2 signatures, both intended, zero unrelated churn:

Parse changes introduced by this PR · 2 card(s), 2 signature(s) (baseline: main 184c1304bb52)

Benevolent Blessing:
  grant Protection
  → grant Protection, ProtectionDoesntRemoveControlledAttachments

Spectra Ward:
  power +2, toughness +2, grant Protection
  → power +2, toughness +2, grant Protection ×5, ProtectionDoesntRemoveAuras

Guardian Beast (the only other doesn't remove Auras card in the corpus) is not in the diff — its non-terminal clause is rejected by the end-anchor.

Coverage (production-path + direct + runtime):

• spectra_ward_production_line_emits_all_auras_exemption — real Oracle line via parse_static_line_multi → five WUBRG grants + ProtectionDoesntRemoveAuras (fails if it reverts to the inert CardType("each color") grant / no exemption).
• protection_attachment_exemption_trailing_forms — direct coverage of all three forms plus the Guardian Beast guard and the controlled-vs-all-Auras ordering.
• attachment_illegality_all_auras_exemption_keeps_any_aura — a foreign-controlled white Aura stays attached (proving it's all-Auras, not controlled-only) while a white Equipment is still detached.
• Existing Benevolent Blessing / Ward / other-instance / transient-grant regressions still pass unchanged.

Local: cargo fmt --check, scripts/check-parser-combinators.sh, scripts/check-engine-authorities.sh origin/main, cargo clippy -p engine --lib --tests (-D warnings), and cargo test -p engine --lib (14,987 passed) all green.

[matthewevans]

Processed current head f2940111e91f8859ceef7691ff949968aac38c3f.

Holding this revision for current parser/card-data evidence before approval or enqueue. The head changed after the last coverage-parse-diff sticky update, and the current Card data check is still in progress, so the available parse-diff evidence is stale for this parser/engine change.

[matthewevans]

Thanks for pushing the CR 702.16n / 702.16p coverage forward. The parser/test direction and the provenance-aware attachment check are the right surface, but I don't think this should land with the new static modes shaped as three sibling variants.

The patch adds ProtectionDoesntRemoveThisAura, ProtectionDoesntRemoveControlledAttachments, and ProtectionDoesntRemoveAuras, then branches over those siblings in attachment_exempt_from_protection. Those are not separate rule concepts; they are one protection-attachment exemption with a scope/subject parameter. Please collapse this into a single parameterized StaticMode shape, with a typed scope enum for the three covered forms, and have the parser map each Oracle sentence to that scope. That keeps the CR 702.16n/p surface extensible without multiplying enum variants and exhaustive registry/display/from-str arms for each textual form.

Once that shape is parameterized, the existing discriminating tests for this-Aura, controlled attachments, all-Auras, unrelated same-quality protection, transient protection, and parse-diff cards look like the right proof set to keep.