fix(engine): parse Firkraag's "if that creature had to attack this combat" intervening-if#5868
fix(engine): parse Firkraag's "if that creature had to attack this combat" intervening-if#5868tryeverything24 wants to merge 1 commit into
Conversation
…mbat" intervening-if Firkraag, Cunning Instigator: "Whenever a creature deals combat damage to one of your opponents, if that creature had to attack this combat, you put a +1/+1 counter on Firkraag and you draw a card." The intervening-if clause (CR 603.4) was silently dropped during trigger parsing (flagged by the card data's own SwallowedClause diagnostic), so the ability fired on ANY creature dealing combat damage to an opponent, including creatures that attacked purely by their controller's free choice. Adds FilterProp::RequiredToAttack, backed by a new AttackerInfo.required_to_attack snapshot taken in declare_attackers_with_bands BEFORE attackers are tapped (the must-attack predicate exempts tapped creatures per CR 508.1a, so a live recheck at combat-damage time would spuriously read false for every non-vigilance attacker). Wires the oracle clause to TriggerCondition::EventDamageSourceMatchesFilter over the new filter prop, reusing the same damage-source-matching mechanism as Mindblade Render's 'if any of that damage was dealt by a Warrior'. The zone-change look-back path (ZoneChangeCombatStatus) does not snapshot this bit -- no card currently exercises that path for this prop, so it fails closed (returns false) rather than guessing at an uncaptured value. Fixes phase-rs#4732
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for the intervening-if condition 'if that creature had to attack this combat' (specifically for Firkraag, Cunning Instigator) by introducing a new FilterProp::RequiredToAttack property and snapshotting the requirement status in AttackerInfo during attacker declaration. Feedback focuses on architectural compliance: replacing the new bool field on AttackerInfo with a typed enum to satisfy rule R2, avoiding verbatim string matching in the trigger parser in favor of nom combinators in oracle_nom/condition.rs per rule R1, and adding a mandatory CR annotation to the new match arm in filter.rs to comply with 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.
| /// CR 508.1a + CR 508.1d + CR 701.15b: True if this creature was under a | ||
| /// must-attack requirement (`StaticMode::MustAttack`/`MustAttackPlayer`, | ||
| /// or goad) at the moment attackers were declared this combat — i.e. it | ||
| /// "had to attack" rather than attacking by its controller's free choice. | ||
| /// Snapshotted in `declare_attackers_with_bands` BEFORE attackers are | ||
| /// tapped, because the requirement predicate | ||
| /// (`creature_must_attack_with_attackable_players_gated`) exempts tapped | ||
| /// creatures — re-deriving this live after declaration (e.g. at the combat | ||
| /// damage step) would spuriously read `false` for every non-vigilance | ||
| /// attacker. Backs `FilterProp::RequiredToAttack`. | ||
| #[serde(default)] | ||
| pub required_to_attack: bool, | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Violation of R2: Avoid new bool fields on structs. Evidence: crates/engine/src/game/combat.rs:208.
Why it matters: A bool field does not express the design space and violates the hard architectural rule R2.
Suggested fix: Replace the bool field with a typed enum like AttackRequirement.
/// CR 508.1a + CR 508.1d + CR 701.15b: The requirement status of this creature
/// at the moment attackers were declared this combat — i.e. whether it
/// "had to attack" or attacked by its controller's free choice.
/// Snapshotted in declare_attackers_with_bands BEFORE attackers are
/// tapped, because the requirement predicate
/// (creature_must_attack_with_attackable_players_gated) exempts tapped
/// creatures — re-deriving this live after declaration (e.g. at the combat
/// damage step) would spuriously read false for every non-vigilance
/// attacker. Backs FilterProp::RequiredToAttack.
#[serde(default)]
pub requirement: AttackRequirement,
}
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, Default)]
pub enum AttackRequirement {
#[default]
Voluntary,
Required,
}References
- R2. No bool fields — parameterize with existing typed enums. A bool field never expresses the design space; the project uses typed enums like ControllerRef, Comparator, PlayerScope, Option, and dedicated discriminated unions instead. (link)
| attack_target, | ||
| blocked: false, | ||
| band_id: None, | ||
| required_to_attack: false, | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Violation of R2: Initialize the new typed enum instead of the bool field. Evidence: crates/engine/src/game/combat.rs:223.
Why it matters: Adheres to the R2 architectural rule by using the typed enum.
Suggested fix: Initialize requirement to AttackRequirement::Voluntary.
| attack_target, | |
| blocked: false, | |
| band_id: None, | |
| required_to_attack: false, | |
| } | |
| attack_target, | |
| blocked: false, | |
| band_id: None, | |
| requirement: AttackRequirement::Voluntary, | |
| } |
References
- R2. No bool fields — parameterize with existing typed enums. (link)
| let mut info = AttackerInfo::new(*object_id, *target, defending_player); | ||
| info.required_to_attack = required_to_attack_ids.contains(object_id); | ||
| info |
There was a problem hiding this comment.
[MEDIUM] Violation of R2: Set the typed enum status based on the snapshot. Evidence: crates/engine/src/game/combat.rs:3169.
Why it matters: Adheres to the R2 architectural rule by using the typed enum.
Suggested fix: Set requirement to AttackRequirement::Required if the creature was required to attack.
let mut info = AttackerInfo::new(*object_id, *target, defending_player);
if required_to_attack_ids.contains(object_id) {
info.requirement = AttackRequirement::Required;
}
infoReferences
- R2. No bool fields — parameterize with existing typed enums. (link)
| FilterProp::RequiredToAttack => state.combat.as_ref().is_some_and(|combat| { | ||
| combat | ||
| .attackers | ||
| .iter() | ||
| .any(|a| a.object_id == object_id && a.required_to_attack) | ||
| }), |
There was a problem hiding this comment.
[MEDIUM] Violation of R2: Match against the typed enum instead of the bool field. Evidence: crates/engine/src/game/filter.rs:3692.
Why it matters: Adheres to the R2 architectural rule by using the typed enum.
Suggested fix: Compare requirement with AttackRequirement::Required.
| FilterProp::RequiredToAttack => state.combat.as_ref().is_some_and(|combat| { | |
| combat | |
| .attackers | |
| .iter() | |
| .any(|a| a.object_id == object_id && a.required_to_attack) | |
| }), | |
| FilterProp::RequiredToAttack => state.combat.as_ref().is_some_and(|combat| { | |
| combat | |
| .attackers | |
| .iter() | |
| .any(|a| a.object_id == object_id && a.requirement == AttackRequirement::Required) | |
| }), |
References
- R2. No bool fields — parameterize with existing typed enums. (link)
| ( | ||
| "if that creature had to attack this combat", | ||
| TriggerCondition::EventDamageSourceMatchesFilter { | ||
| filter: TargetFilter::Typed( | ||
| TypedFilter::creature().properties(vec![FilterProp::RequiredToAttack]), | ||
| ), | ||
| }, | ||
| ), |
There was a problem hiding this comment.
[MEDIUM] Violation of R1 and Condition Extraction: Avoid verbatim string matching in trigger parsers. Evidence: crates/engine/src/parser/oracle_trigger.rs:4967.
Why it matters: Verbatim string equality on full Oracle phrases is prohibited, and condition recognition must be delegated to parse_inner_condition in oracle_nom/condition.rs.
Suggested fix: Parse this clause using nom combinators in oracle_nom/condition.rs and delegate to it.
References
- R1. Nom combinators on the first pass — no exceptions. Verbatim string equality on full Oracle phrases is the single most prohibited pattern in the codebase. (link)
- Condition extraction: parse_inner_condition in oracle_nom/condition.rs is the single authority. Trigger and static parsers must delegate — never re-implement condition recognition. (link)
- 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
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
| // Group 5 (not-yet-supported): `ZoneChangeCombatStatus` / | ||
| // `capture_combat_status` do not currently snapshot a required-to-attack | ||
| // bit the way they do for `attacking_alone`/`blocking_alone` — Firkraag, | ||
| // Cunning Instigator's own "had to attack this combat" check runs via | ||
| // `EventDamageSourceMatchesFilter` over the live `DamageRecord` source | ||
| // (still in `state.combat` during the combat-damage step), not this | ||
| // zone-change look-back path, so no card currently exercises this arm. | ||
| // Conservative gap: fail closed rather than guess at a snapshot value | ||
| // that was never captured. | ||
| FilterProp::RequiredToAttack => false, |
There was a problem hiding this comment.
[MEDIUM] Violation of R6: Missing CR annotation for rules-touching code. Evidence: crates/engine/src/game/filter.rs:4812.
Why it matters: Every rules-touching line of engine code must carry a comment of the form CR <number>: <description> to ensure strict fidelity to the MTG Comprehensive Rules.
Suggested fix: Add a verified CR annotation comment (e.g., CR 508.1a + CR 508.1d + CR 701.15b) above the RequiredToAttack match arm.
| // Group 5 (not-yet-supported): `ZoneChangeCombatStatus` / | |
| // `capture_combat_status` do not currently snapshot a required-to-attack | |
| // bit the way they do for `attacking_alone`/`blocking_alone` — Firkraag, | |
| // Cunning Instigator's own "had to attack this combat" check runs via | |
| // `EventDamageSourceMatchesFilter` over the live `DamageRecord` source | |
| // (still in `state.combat` during the combat-damage step), not this | |
| // zone-change look-back path, so no card currently exercises this arm. | |
| // Conservative gap: fail closed rather than guess at a snapshot value | |
| // that was never captured. | |
| FilterProp::RequiredToAttack => false, | |
| // CR 508.1a + CR 508.1d + CR 701.15b: ZoneChangeCombatStatus / | |
| // capture_combat_status do not currently snapshot a required-to-attack | |
| // bit the way they do for attacking_alone/blocking_alone — Firkraag, | |
| // Cunning Instigator's own "had to attack this combat" check runs via | |
| // EventDamageSourceMatchesFilter over the live DamageRecord source | |
| // (still in state.combat during the combat-damage step), not this | |
| // zone-change look-back path, so no card currently exercises this arm. | |
| // Conservative gap: fail closed rather than guess at a snapshot value | |
| // that was never captured. | |
| FilterProp::RequiredToAttack => false, |
References
- R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . (link)
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: blocked — the new condition is wired to a damage-record matcher that currently makes every RequiredToAttack check false.
🔴 Blocker
crates/engine/src/game/triggers.rs:7096-7143 evaluates EventDamageSourceMatchesFilter through matches_target_filter_on_damage_record_source, rather than matching the live attacker. That path reaches crates/engine/src/game/filter.rs:4812-4821, where the newly added FilterProp::RequiredToAttack arm unconditionally returns false. Consequently the positive Firkraag case cannot trigger at either intervening-if check, even though combat.rs:3121-3132 captured the declaration-time fact; this aligns with the current failed Rust checks. CR 603.4 requires the condition to be true when the damage event occurs and again on resolution; CR 603.4 is verified in docs/MagicCompRules.txt:2592.
Please carry the declaration-time requirement through the existing DamageRecord source snapshot (types/game_state.rs:841-886, produced in game/effects/deal_damage.rs:693-750) and make the damage-record filter matcher consume that captured value. That preserves the established event-time source semantics and remains correct if the creature later changes characteristics or leaves combat. Add a direct positive/negative assertion at the damage-record matcher boundary in addition to the end-to-end Firkraag scenarios, so this routing mismatch cannot recur.
🟡 Non-blocking
The helper table in oracle_trigger.rs:4950-4974 is an existing legacy simple-condition seam, so I am not treating the bot's combinator concern as a separate blocker here. The snapshot must be completed first.
✅ Clean
The parse-diff artifact is scoped to exactly Firkraag, matching the PR claim, and the branch correctly recognizes that had to attack must be captured before non-vigilance attackers are tapped.
Recommendation: request changes — thread required_to_attack through the DamageRecord snapshot and prove both the matcher and Firkraag's production combat path.
2 participants
Fixes #4732.
Firkraag, Cunning Instigator: "Whenever a creature deals combat damage to one of your opponents, if that creature had to attack this combat, you put a +1/+1 counter on Firkraag and you draw a card."
The intervening-if clause (CR 603.4) was silently dropped during trigger parsing (flagged by the card data's own
SwallowedClausediagnostic), so the ability fired on ANY creature dealing combat damage to an opponent, including creatures that attacked purely by their controller's free choice.Root cause & fix
Adds
FilterProp::RequiredToAttack, backed by a newAttackerInfo.required_to_attacksnapshot taken indeclare_attackers_with_bandsbefore attackers are tapped — the must-attack predicate exempts tapped creatures per CR 508.1a, so a live recheck at combat-damage time would spuriously readfalsefor every non-vigilance attacker. Wires the oracle clause toTriggerCondition::EventDamageSourceMatchesFilterover the new filter prop, reusing the same damage-source-matching mechanism as Mindblade Render's "if any of that damage was dealt by a Warrior".Testing
Two discriminating integration tests: a goaded attacker (must-attack) triggers the ability; an unencumbered attacker (free choice) does not.
Verified locally (rebased onto main): fmt clean, clippy clean, 16672 lib tests pass, 3131 integration tests pass.