fix(parser): scope "attacks enchanted player" trigger to the attached player (Curse of Predation #4744)

[shin-core]

Summary

Fixes #4744. Curse of Predation — "Whenever a creature attacks enchanted player, put a +1/+1 counter on it." — parsed to a bare Attacks trigger with no defender scope, so it fired whenever any creature attacked anyone. The curse handed out +1/+1 counters for every attack on the board, not just attacks against the enchanted player.

Root cause

The attack-trigger parser's parse_attack_target combinator recognized " you", " a player", " a planeswalker", " a battle" — but had no arm for " enchanted player", so the qualifier was silently dropped and the trigger had no defender scope.

Fix (parser-only — the runtime already supports it)

• Add the " enchanted player" arm (type axis = AttackTargetFilter::Player).
• Set valid_target = TargetFilter::AttachedTo for it — player_matches_filter already resolves AttachedTo to the Aura's attached player (source.attached_to.as_player()).

No engine/type change. The runtime path is already proven: the existing attacks_trigger_matches_player_host_for_attached_to_target test shows an AttachedTo-scoped Attacks trigger fires only when the enchanted player is the defender and not when another player is attacked. The parser now feeds that tested path.

Class

The Curse cycle's attack triggers (Curse of Predation, Curse of Bloodletting, …) — any "whenever … attacks enchanted player".

Tests

• Parser (trigger_attacks_enchanted_player_scopes_to_attached_player): Curse of Predation parses to Attacks + attack_target_filter = Player + valid_target = AttachedTo (mirrors the existing "attacks you" → Controller test).
• Runtime coverage is provided by the pre-existing attacks_trigger_matches_player_host_for_attached_to_target (enchanted player attacked → matches; other player attacked → does not).

Verification

• cargo check -p engine / cargo clippy -p engine — clean
• Suites pass: parser::oracle_trigger (985), game::trigger_matchers (254) — zero failures.

CR 303.4a (Aura attached to a player) + CR 508.1a (attack declaration) + CR 109.4 (defended player identity).

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request adds support for parsing the 'enchanted player' defender scope for Curse Aura triggers, mapping it to TargetFilter::AttachedTo to resolve Issue #4744. The review feedback suggests extending this to support modern Oracle text ('enchanted player or a planeswalker enchanted player controls') by using modular, composable parsers rather than a single verbatim string match, and adding corresponding test coverage.

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] Support modern Oracle text for Curses using modular parsers.

Why it matters: The official Oracle text for Curse of Predation and all other attack-triggered Curses has been updated to "Whenever a creature attacks enchanted player or a planeswalker enchanted player controls...". Parsing only "enchanted player" will cause the parser to fail on the actual Oracle text of these cards due to unparsed trailing text before the comma.

Suggested fix: Avoid matching the entire compound phrase verbatim with a single long tag. Instead, decompose the phrase into modular, reusable parsers for its constituent parts (such as "enchanted player" and "a planeswalker enchanted player controls") and compose them using idiomatic combinators to prevent fragile matches and maintainability issues.

                alt((
                    value(
                        AttackTargetFilter::PlayerOrPlaneswalker,
                        preceded(
                            tag(" enchanted player"),
                            tag(" or a planeswalker enchanted player controls")
                        )
                    ),
                    value(
                        AttackTargetFilter::Player,
                        tag(" enchanted player")
                    )
                ))

References

1. Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. ^(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 and compose them using idiomatic combinator aggregates.

[gemini-code-assist]

[HIGH] Add test coverage for modern Oracle text.

Why it matters: Ensures that the parser correctly handles the modern Oracle text of Curses ("enchanted player or a planeswalker enchanted player controls") and correctly maps it to AttackTargetFilter::PlayerOrPlaneswalker with TargetFilter::AttachedTo.

#[test]
fn trigger_attacks_enchanted_player_scopes_to_attached_player() {
    let def = parse_trigger_line(
        "Whenever a creature attacks enchanted player, put a +1/+1 counter on it.",
        "Curse of Predation",
    );
    assert_eq!(def.mode, TriggerMode::Attacks);
    assert_eq!(def.attack_target_filter, Some(AttackTargetFilter::Player));
    assert_eq!(
        def.valid_target,
        Some(TargetFilter::AttachedTo),
        "'attacks enchanted player' must scope the defender to the attached player"
    );
}

#[test]
fn trigger_attacks_enchanted_player_or_planeswalker_scopes_to_attached_player() {
    let def = parse_trigger_line(
        "Whenever a creature attacks enchanted player or a planeswalker enchanted player controls, put a +1/+1 counter on it.",
        "Curse of Predation",
    );
    assert_eq!(def.mode, TriggerMode::Attacks);
    assert_eq!(
        def.attack_target_filter,
        Some(AttackTargetFilter::PlayerOrPlaneswalker)
    );
    assert_eq!(
        def.valid_target,
        Some(TargetFilter::AttachedTo),
        "'attacks enchanted player or a planeswalker...' must scope the defender to the attached player"
    );
}

[github-actions]

Parse changes introduced by this PR · 9 card(s), 6 signature(s) (baseline: main 9d82a1d0cc7f)

5 card(s) · trigger/Attacks · field valid target: ∅ → attached permanent

Examples: Curse of Chaos, Curse of Inertia, Curse of Predation (+2 more)

2 card(s) · ability/SeparateIntoPiles · added: SeparateIntoPiles

Examples: Sphinx of Uthuun, Unesh, Criosphinx Sovereign

1 card(s) · ability/DamageAll · added: DamageAll (amount=1, filter=shares color with reference other than target creature)

Examples: Wojek Embermage

1 card(s) · ability/DamageAll · added: DamageAll (amount=2, filter=shares color with reference other than target creature)

Examples: Cleansing Beam

1 card(s) · ability/RevealTop · removed: RevealTop (count=4, player=controller)

Examples: Unesh, Criosphinx Sovereign

1 card(s) · ability/RevealTop · removed: RevealTop (count=5, player=controller)

Examples: Sphinx of Uthuun

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

[matthewevans]

Thanks for this — the diagnosis is right and the fix lands at the right seam. A few things need to change before it can merge, but the shape of the change is correct.

What's right

The seam. You added " enchanted player" as an arm in the parse_attack_target alternation (crates/engine/src/parser/oracle_trigger.rs:8720) instead of special-casing Curse of Predation. That is exactly the call this repo wants: one arm in an existing alt() gives the whole card class a defender scope. Your parse-diff sticky confirms the blast radius is precisely the class — trigger/Attacks · field valid target: ∅ → attached permanent across Curse of Chaos, Curse of Inertia, Curse of Predation and two more. Five cards fixed, not one.

The runtime-support claim. I verified it rather than taking it on faith. TargetFilter::AttachedTo really is handled at crates/engine/src/game/trigger_matchers.rs:662, where it resolves source.attached_to, calls .as_player(), and compares against the defending player. So "parser-only, the runtime already supports it" is accurate. One note for the next reader: there are two functions named player_matches_filter in this codebase — filter.rs:5304 takes a &str filter, trigger_matchers.rs:641 takes a &TargetFilter. Your comment at oracle_trigger.rs:8772 describes the behavior of the latter correctly; naming the file in the comment would save someone a grep.

The two DamageAll rows on your parse-diff sticky (Wojek Embermage, Cleansing Beam) are baseline drift from #5470, which merged after your sticky's baseline commit. They have nothing to do with this change — I grepped your diff and it contains zero occurrences of those symbols. Please don't chase them.

Blocking: the test doesn't test the fix

trigger_attacks_enchanted_player_scopes_to_attached_player in oracle_trigger_tests.rs asserts def.valid_target == Some(TargetFilter::AttachedTo). That proves the parser emits the filter. It proves nothing about whether the trigger actually fires.

That gap matters more here than it usually would, because this change inverts the failure mode. Before your patch the trigger had no defender scope and fired on every attack anywhere. After it, the trigger fires only when source.attached_to.as_player() resolves to the defending player. Enchant-player attachment is only partially supported in this engine. If a Curse's attachment does not populate attached_to as a player, these five cards go from firing too often to never firing at all — a worse bug than the one being fixed, and one that no parser-shape assertion can detect. Your test would stay green through that regression.

What I need is a discriminating runtime test that drives the real pipeline:

1. Put Curse of Predation onto the battlefield enchanting player B.
2. Have a creature attack player B; assert the +1/+1 counter is placed.
3. Have a creature attack a different player; assert no counter is placed.

Step 3 is what makes it discriminating — without it the test passes both before and after your change. Put it in crates/engine/tests/integration/ and add the mod line to tests/integration/main.rs; a new top-level file under tests/ is rejected by the no_top_level_test_binaries guard.

If it turns out enchant-player attachment isn't wired for Curses, that is exactly the thing we need to learn before this merges, and this test is how we learn it.

Non-blocking: the CR annotations are misapplied

I grep-verified all three against docs/MagicCompRules.txt. None of them say what the comments imply:

• CR 303.4a (oracle_trigger.rs:8716, :8770) reads: "An Aura spell requires a target, which is defined by its enchant ability." It says nothing about attachment scope or the enchanted player. The anchor you want is CR 303.4e: "An Aura's controller is separate from the enchanted object's controller or the enchanted player." A sibling comment a few lines below the AttachedTo arm in trigger_matchers.rs already cites 303.4e for this exact concept.
• CR 508.1a (oracle_trigger.rs:8716) reads: "The active player chooses which creatures that they control, if any, will attack…" It governs choosing attackers, not the defending player.
• CR 109.4 (oracle_trigger.rs:8770) reads: "Only objects on the stack or on the battlefield have a controller." It does not scope a trigger to an attached player.

Per CLAUDE.md, a wrong CR number is worse than no CR number: it creates false confidence that the code was checked, against the wrong rule. Every CR number has to be grep-verified against docs/MagicCompRules.txt before it's written.

Non-blocking: wrong card cited

Both new comments (oracle_trigger.rs:8719 and :8774) name Curse of Bloodletting as an example of "attacks enchanted player". Its actual Oracle text is:

Enchant player / If a source would deal damage to enchanted player, it deals double that damage to that player instead.

That's a damage replacement effect with no attack trigger at all. The real examples are Curse of Predation, Curse of Chaos and Curse of Inertia — the same cards your own parse-diff sticky lists. CLAUDE.md asks us to verify the card against Scryfall, not only the rule.

---

The seam and the parser change look right. Fix the CR citations, swap the card example, and add the runtime test that would fail if AttachedTo never resolves, and this should be a quick turnaround.

[shin-core]

Thanks @matthewevans — all addressed in 1af093f.

Blocking — discriminating runtime test. Added curse_of_predation_attacks_enchanted_player_4744.rs under tests/integration/ (with the main.rs mod line). It drives the real pipeline exactly as you asked:

1. Curse of Predation enchants P1; a creature attacks P1 → +1/+1 counter IS placed on the attacker.
2. A creature attacks a different player → no counter.

Both pass — so the answer to the question you flagged (does AttachedTo actually resolve, or do these cards now never fire?) is confirmed: enchant-player attachment is wired and the trigger fires correctly. Step 3 is what makes it discriminating — it fails before the patch (fires on any attack) and would fail if the fix over-corrected to never-fires.

Non-blocking — CR annotations. You're right, all three were misapplied. Grep-verified and corrected to CR 303.4e ("an Aura's controller is separate from the enchanted player") — the same anchor the sibling comment in trigger_matchers.rs uses. Also named game/trigger_matchers.rs in the comment so the next reader doesn't have to disambiguate the two player_matches_filter functions.

Non-blocking — wrong card. Fixed: Curse of Bloodletting is a damage-replacement with no attack trigger. Swapped to the real class — Curse of Predation, Curse of Chaos, Curse of Inertia (verified via Scryfall).

On Gemini's "or a planeswalker enchanted player controls" suggestion: I checked the current Scryfall Oracle text for these curses and it's plain "attacks enchanted player" — no planeswalker clause — so I did not add that variant (per CLAUDE.md, verify the card, not the paraphrase). Happy to add it if there's a printing I'm missing.

fmt/clippy clean; parser trigger test + the new integration test green.

[matthewevans]

Approved. The follow-up in 1af093fd is exactly what I asked for, and it resolves all three findings from my earlier review.

The runtime test is the reason this is now mergeable. curse_of_predation_attacks_enchanted_player_4744.rs pins both failure directions, which matters more here than it usually would. The fix inverts the failure mode: before it, the trigger had no defender scope and fired on every attack anywhere; after it, the new risk is the opposite, where attached_to.as_player() never resolves and the trigger fires for nobody. A test that only proved the trigger stopped over-firing would have passed just as happily against a trigger that was dead. curse_of_predation_counter_placed_when_enchanted_player_attacked attaches to P1, has P0 attack P1, and asserts the counter lands, so it fails if AttachedTo never resolves. curse_of_predation_no_counter_when_non_enchanted_player_attacked attaches to P1, has P1 attack P0, and asserts no counter, so it fails pre-fix. The positive case doubles as the reach-guard that keeps the negative from passing vacuously, because it proves the trigger can fire in that fixture shape. That is the property I was looking for.

The test also settles the open question that justified the block. I flagged uncertainty about whether enchant-player attachment was wired for Curses at all, and engine::game::effects::attach::attach_to_player answers it directly. That is resolved by evidence now rather than by assertion, which is the right way to close it.

CR annotation. Replacing 303.4a / 508.1a / 109.4 with the single CR 303.4e is correct. 303.4a covers an Aura spell's target on the stack, which is a different moment from the trigger's defender scope, so it was doing no work where it sat. 303.4e carries the separation between the Aura's controller and the enchanted player, which is the property the AttachedTo scope actually depends on.

Card citations. Good correction from Curse of Bloodletting, which is a damage-doubling replacement effect and has no attack trigger to scope, to Curse of Chaos and Curse of Inertia.

Disambiguating player_matches_filter by naming game/trigger_matchers.rs was a good catch on your part. A same-named function taking a &str lives in filter.rs, and that collision briefly made my original comment look wrong to me when I went to re-check it. Naming the module removes the ambiguity for the next reader.

Two housekeeping notes, neither worth a push:

• The doc comment in oracle_trigger_tests.rs points at curse_of_predation_scopes_counter_to_enchanted_player, but the tests landed as curse_of_predation_counter_placed_when_enchanted_player_attacked and curse_of_predation_no_counter_when_non_enchanted_player_attacked. Cosmetic, and I would rather not spin CI for it. Worth folding into your next touch of that file.
• The extra rows on the coverage sticky (SeparateIntoPiles on Sphinx of Uthuun and Unesh, DamageAll on Wojek Embermage and Cleansing Beam, the RevealTop removals) are baseline drift from #5489 and #5470, which merged after your sticky's baseline commit. They have nothing to do with this change. Please do not chase them. The rows that belong to you are the five Curse cards gaining valid target: ∅ → attached permanent, which is the intended effect.

Enqueuing for merge. Thanks for the thorough second pass.