fix(engine): Eriette of the Charmed Apple scoped attack restriction (#2030)

[kiannidev]

Summary

Fixes #2030 — Eriette of the Charmed Apple was not enforcing its combat restriction or was mis-parsed.

Root cause: Subject-led lines like Each creature that's enchanted by an Aura you control can't attack you or planeswalkers you control. failed parse_subject_combat_rule_static (trailing defended scope left unconsumed), then fell through to the ~ can't attack dispatcher and became a SelfRef CantAttack on Eriette herself. Scoped attack prohibitions also were not stored or enforced at declaration time.

Changes:

• Parse that's enchanted by an Aura you control via strip_attachment_relative_clause + HasAttachment on the subject filter.
• Capture attack_defended on StaticDefinition for flat can't attack you [or planeswalkers you control].
• Enforce scoped CantAttack in declare_attackers via StaticCheckContext::attack_target; skip scoped statics in attacker eligibility queries when no target is known.
• Shared restrictions::attack_target_matches_defended_scope for combat tax and flat restrictions.

Test plan

• cargo test -p engine --lib eriette_charmed_apple_static_and_trigger_parse
• cargo test -p engine --lib eriette_scoped_cant_attack_blocks_only_defender_scope
• Manual: Eriette on battlefield, aura on opponent creature — cannot attack you/your PW; can attack other players in multiplayer
• Manual: end step with N auras — each opponent loses N life, you gain N

[gemini-code-assist]

Code Review

This pull request implements support for scoped attack prohibitions (CR 508.1d), such as those on "Eriette of the Charmed Apple", by adding an attack_defended field to StaticDefinition and checking it during attacker validation and static ability resolution. It also updates the parser to handle subject-led combat restrictions and attachment-relative clauses. Feedback on this PR points out a violation of Rule R1 in evasion.rs, where string manipulation (contains and find) is used for parsing dispatch instead of nom combinators, and suggests a nom-based refactoring.

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.

[matthewevans]

🤖 Architecture Review (automated)

Verdict: ⚠️ Changes requested

Seam: PASS — attack_defended scope-matching centralized in restrictions.rs as a shared pub(crate) helper, reused by both compute_combat_tax and check_static_ability; HasAttachment reuses the existing filter.rs building block rather than reinventing.
Idiomatic: CONCERN — new parse_subject_combat_rule_static block dispatches on lower.contains/lower.find + manual byte-slicing instead of the nom path that already exists three lines below it (the repo's #1 non-negotiable parser mandate).
Value: Covers the CLASS of scoped remote-CantAttack statics (Eriette + any " can't attack you [or planeswalkers you control]" line, with attachment-relative-clause subjects), not just Eriette — attack_defended: Option<AttackTargetFilter> is a reusable axis and is genuinely consumed at runtime (declaration + eligibility skip + tax).
Reconciled with existing reviews:

• Gemini [HIGH] evasion.rs:996/1010 — string contains/find for parsing dispatch: CONFIRMED. The added block (if !lower.contains("can't attack or block") { if let Some(marker_idx) = lower.find("can't attack") { ... &text[..marker_idx] ... &text[marker_idx..] } }) is exactly the prohibited dispatch pattern, and parse_subject_combat_rule_static already runs nom_primitives::scan_preceded(&lower, parse_combat_rule_static_predicate_nom) immediately below. The duplicate string path only exists because parse_combat_rule_static_predicate_nom discards the Option<AttackTargetFilter> (it routes through parse_cant_attack_combat_predicate_nom, which drops the scope). The fix is to thread the scope through the existing nom path, not to add a second string-based dispatch. Gemini's suggested scan_preceded(&lower, parse_cant_attack_rule_static_predicate_nom) is the right shape.

Findings

• [HIGH] crates/engine/src/parser/oracle_static/evasion.rs (new block at top of parse_subject_combat_rule_static) — lower.contains("can't attack or block") + lower.find("can't attack") + &text[..marker_idx]/&text[marker_idx..] slicing is parsing dispatch via string methods, violating the repo's non-negotiable nom-combinator mandate. A nom path for the identical line already exists below. Fix: recover the defended scope through the existing scan_preceded + parse_cant_attack_rule_static_predicate_nom (which now returns Option<AttackTargetFilter>) and call .attack_defended(defended), deleting the string block. Confirms Gemini.
• [NIT] crates/engine/src/parser/oracle_static/shared.rs strip_attachment_relative_clause — five verbatim strip_suffix literals enumerate that is/are × enchanted by an aura / equipped by an equipment you control. strip_suffix for grammar normalization is permitted by convention, but this is a 2×2 (×kind) product that would compose more cleanly via chained strip_suffix on the that is/that are/that's head + the attachment-kind tail, mirroring the "compose, don't enumerate permutations" guideline. Not blocking.

Notes (verified, no action): CR 508.1d, 508.1h, 506.3, 303.4, 301.5 all confirmed against docs/MagicCompRules.txt. attack_defended is read at runtime (declaration guard in declare_attackers_with_bands, scoped-skip in check_static_ability, eligibility is_none() filters in validate_attackers/get_valid_attacker_ids/has_potential_attackers, and compute_combat_tax) — not a parsed-but-unused field. Eligibility-vs-declaration split is correct: scoped statics are skipped when attack_target is None so the creature stays eligible to attack other players, and enforced only when a target is declared. The attack_defended: None churn across ~20 sites is the unavoidable cost of adding a struct field with no builder default; acceptable.

[kiannidev]

🤖 Architecture Review (automated)

Verdict: ⚠️ Changes requested

Seam: PASS — attack_defended scope-matching centralized in restrictions.rs as a shared pub(crate) helper, reused by both compute_combat_tax and check_static_ability; HasAttachment reuses the existing filter.rs building block rather than reinventing. Idiomatic: CONCERN — new parse_subject_combat_rule_static block dispatches on lower.contains/lower.find + manual byte-slicing instead of the nom path that already exists three lines below it (the repo's #1 non-negotiable parser mandate). Value: Covers the CLASS of scoped remote-CantAttack statics (Eriette + any " can't attack you [or planeswalkers you control]" line, with attachment-relative-clause subjects), not just Eriette — attack_defended: Option<AttackTargetFilter> is a reusable axis and is genuinely consumed at runtime (declaration + eligibility skip + tax). Reconciled with existing reviews:

• Gemini [HIGH] evasion.rs:996/1010 — string contains/find for parsing dispatch: CONFIRMED. The added block (if !lower.contains("can't attack or block") { if let Some(marker_idx) = lower.find("can't attack") { ... &text[..marker_idx] ... &text[marker_idx..] } }) is exactly the prohibited dispatch pattern, and parse_subject_combat_rule_static already runs nom_primitives::scan_preceded(&lower, parse_combat_rule_static_predicate_nom) immediately below. The duplicate string path only exists because parse_combat_rule_static_predicate_nom discards the Option<AttackTargetFilter> (it routes through parse_cant_attack_combat_predicate_nom, which drops the scope). The fix is to thread the scope through the existing nom path, not to add a second string-based dispatch. Gemini's suggested scan_preceded(&lower, parse_cant_attack_rule_static_predicate_nom) is the right shape.

Findings

• [HIGH] crates/engine/src/parser/oracle_static/evasion.rs (new block at top of parse_subject_combat_rule_static) — lower.contains("can't attack or block") + lower.find("can't attack") + &text[..marker_idx]/&text[marker_idx..] slicing is parsing dispatch via string methods, violating the repo's non-negotiable nom-combinator mandate. A nom path for the identical line already exists below. Fix: recover the defended scope through the existing scan_preceded + parse_cant_attack_rule_static_predicate_nom (which now returns Option<AttackTargetFilter>) and call .attack_defended(defended), deleting the string block. Confirms Gemini.
• [NIT] crates/engine/src/parser/oracle_static/shared.rs strip_attachment_relative_clause — five verbatim strip_suffix literals enumerate that is/are × enchanted by an aura / equipped by an equipment you control. strip_suffix for grammar normalization is permitted by convention, but this is a 2×2 (×kind) product that would compose more cleanly via chained strip_suffix on the that is/that are/that's head + the attachment-kind tail, mirroring the "compose, don't enumerate permutations" guideline. Not blocking.

Notes (verified, no action): CR 508.1d, 508.1h, 506.3, 303.4, 301.5 all confirmed against docs/MagicCompRules.txt. attack_defended is read at runtime (declaration guard in declare_attackers_with_bands, scoped-skip in check_static_ability, eligibility is_none() filters in validate_attackers/get_valid_attacker_ids/has_potential_attackers, and compute_combat_tax) — not a parsed-but-unused field. Eligibility-vs-declaration split is correct: scoped statics are skipped when attack_target is None so the creature stays eligible to attack other players, and enforced only when a target is declared. The attack_defended: None churn across ~20 sites is the unavoidable cost of adding a struct field with no builder default; acceptable.

Thanks for the architecture review — agreed on the [HIGH] finding.

Removed the duplicate string dispatch in parse_subject_combat_rule_static. Subject-led lines now use a single scan_preceded(&lower, parse_combat_rule_static_predicate_with_defended_nom), which threads Option from parse_cant_attack_rule_static_predicate_nom / parse_cant_attack_defended_scope_nom into .attack_defended(...).

parse_rule_static_predicate_nom now composes the same combinator (dropping scope only where callers need a bare RuleStaticPredicate). Reordered the combat alt so can't attack or block precedes can't attack, fixing regressions where partial can't attack matches left or block and fell through to SelfRef (e.g. enchanted-creature auras).

Runtime behavior unchanged: attack_defended still enforced at declare attackers and skipped in eligibility when no target is in context. Happy to take another look.

[matthewevans]

Pushed maintainer follow-up in b633e2ed2 after merging current origin/main.

What changed:

• Kept the author’s nom-based parser refactor and scoped attack_defended runtime enforcement.
• Extended the Eriette runtime regression to a 3-player setup so the enchanted creature is proven still able to attack another player while blocked from attacking the restricted defender scope.

Verification run locally with the shared target dir:

• cargo fmt --all
• git diff --check
• CARGO_TARGET_DIR=/Users/matt/dev/forge.rs-pr-target RUSTC_WRAPPER= cargo test -p engine --lib eriette_scoped_cant_attack_blocks_only_defender_scope -- --nocapture

I am leaving broad validation to GitHub CI.

[matthewevans]

Pushed 82c0386fc5 after #2259 merged into main.

What changed:

• Merged latest origin/main into this branch.
• No new fix(engine): Eriette of the Charmed Apple scoped attack restriction (#2030) #2261 behavior changes beyond the previously pushed Eriette runtime coverage.

Re-ran focused verification:

• git diff --check
• CARGO_TARGET_DIR=/Users/matt/dev/forge.rs-pr-target RUSTC_WRAPPER= cargo test -p engine --lib eriette_scoped_cant_attack_blocks_only_defender_scope -- --nocapture

[matthewevans]

Maintainer review: approved after follow-up in b633e2e and latest-main merge in 82c0386.

Evidence checked:

• Correct seam: scoped attack restriction state is parsed into StaticDefinition.attack_defended and enforced in combat declaration/static checks, not patched in the UI or a card-specific path.
• Idiomatic at the seam: the author replaced the earlier string parser dispatch with the existing nom static-combat predicate path; runtime scope matching uses the shared restrictions helper.
• Discriminating coverage: the updated Eriette runtime test now uses three players and proves the enchanted creature can attack another player while still being blocked from attacking the restricted defender scope.
• Verification: GitHub CI is green on the latest head; local focused test eriette_scoped_cant_attack_blocks_only_defender_scope also passed with the shared target dir.