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 5 commits into
Conversation
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
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.
ea97ce6 to
271999b
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — current head 271999b4 does not compile.
The earlier damage-record routing issue is resolved: RequiredToAttack now reads the declaration-time snapshot through the damage-source matcher, and the new Firkraag runtime cases cover both required and voluntary attacks.
Blocker
Required CI reports E0063 for two AttackerInfo literals that omit the new required_to_attack field: crates/phase-ai/src/policies/anti_self_harm.rs:3273 and crates/phase-ai/src/search.rs:4992. This fails lint and both Rust-test shards, so the branch cannot be enqueued. Update every construction path (or use the established defaulting construction) and let current-head CI complete before re-requesting review.
Evidence: the required Rust lint and both Rust-test jobs for run 29717007499 fail at those exact initializers. Confidence: high.
…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
…lter_prop_reads_life Firkraag's "if that creature had to attack this combat" intervening-if relies on FilterProp::RequiredToAttack, which was missing from filter_prop_reads_life's exhaustive match, tripping clippy's non-exhaustive-match lint under -D warnings.
…tures Review follow-up: current head failed CI with E0063 — two AttackerInfo struct literals in phase-ai test code (anti_self_harm.rs, search.rs) were not updated for the new required_to_attack field. Replaced both literals with the established AttackerInfo::attacking_player defaulting constructor (their field values matched its semantics exactly), so future AttackerInfo fields cannot break these fixtures again. Verified locally in an isolated CARGO_TARGET_DIR: cargo fmt --all --check clean; cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -D warnings clean; engine lib suite 17515 passed; engine integration suite 3750 passed.
271999b to
ad485ed
Compare
|
Addressed the E0063 blocker: both remaining Verified locally in an isolated target dir: |
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a ChangesRequired-to-attack combat evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/types/events.rs (1)
619-626: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSnapshot
required_to_attackwith the damage event.This marks the property supported, but evaluation reads live
state.combat.attackersand the new bit exists only onAttackerInfo. If the damage source leaves combat before Firkraag’s CR 603.4 recheck (for example, it dies in combat damage), the condition becomes false despite having had to attack. Carry this bit into the damage-event/LKI snapshot and evaluate from it, or keep the shape unsupported until that plumbing exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/types/events.rs` around lines 619 - 626, Do not mark FilterProp::RequiredToAttack as Supported until its value is preserved in the damage-event/LKI snapshot and evaluated from that snapshot rather than live state.combat.attackers. Trace the damage event and snapshot types plus the RequiredToAttack evaluation path, carry AttackerInfo’s required-to-attack bit through them, and use the captured value during rechecks; otherwise remove this property from the supported match arm.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/parser/oracle_trigger.rs`:
- Around line 5175-5192: Replace the literal “if that creature had to attack
this combat” dispatch in the trigger-condition parser with a nom-combinator
grammar that parses the clause’s subject/pronoun and required-attack wording
compositionally. Lower the parsed typed event subject to
EventDamageSourceMatchesFilter using the existing RequiredToAttack filter, and
remove the verbatim full-string match while preserving the resulting trigger
semantics.
In `@crates/engine/tests/integration/issue_4732_firkraag_cunning_instigator.rs`:
- Around line 78-124: Extend the Firkraag integration tests around
firkraag_counter_and_draw_when_attacker_had_to_attack to cover creatures
required to attack by MustAttack and MustAttackPlayer, including the
player-scoped requirement. Build each scenario end to end, run combat, and
assert the required attacker deals damage while Firkraag receives a counter and
its controller draws; cover the relevant parameter range rather than only the
existing goad case.
In `@crates/phase-ai/src/policies/anti_self_harm.rs`:
- Around line 3279-3281: Update the test fixture around combat attacker setup to
exercise the production cast/pre-cast flow: replace GameAction::ChooseTarget
with a real cast/pre-cast candidate using an actual pump spell source object, so
AntiSelfHarmPolicy::score reaches score_pre_cast and pump_taps_blocker_penalty.
Remove the synthetic ObjectId(100) setup and assert the expected penalty
difference for the tapped attacker behavior.
---
Outside diff comments:
In `@crates/engine/src/types/events.rs`:
- Around line 619-626: Do not mark FilterProp::RequiredToAttack as Supported
until its value is preserved in the damage-event/LKI snapshot and evaluated from
that snapshot rather than live state.combat.attackers. Trace the damage event
and snapshot types plus the RequiredToAttack evaluation path, carry
AttackerInfo’s required-to-attack bit through them, and use the captured value
during rechecks; otherwise remove this property from the supported match arm.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3011019f-43ab-4e91-94bb-278f136f313a
📒 Files selected for processing (21)
crates/engine/src/ai_support/filter.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/combat.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/end_the_turn.rscrates/engine/src/game/effects/remove_from_combat.rscrates/engine/src/game/filter.rscrates/engine/src/game/layers.rscrates/engine/src/game/phasing.rscrates/engine/src/game/zones.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/types/ability.rscrates/engine/src/types/events.rscrates/engine/tests/integration/end_combat_phase.rscrates/engine/tests/integration/incredible_hulk_enrage_attacking.rscrates/engine/tests/integration/issue_4732_firkraag_cunning_instigator.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/najeela_extra_combat_grant_2898.rscrates/phase-ai/src/policies/anti_self_harm.rscrates/phase-ai/src/search.rs
| combat | ||
| .attackers | ||
| .push(AttackerInfo::attacking_player(attacker_id, PlayerId(1))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Drive the test through the tapped-attacker production path.
This fixture populates state.combat, but the test uses GameAction::ChooseTarget, so AntiSelfHarmPolicy::score never reaches score_pre_cast or pump_taps_blocker_penalty. It also uses the synthetic ObjectId(100) rather than an actual pump spell source. As a result, this test stays green even if the tapped-attacker behavior is broken. Use a real cast/pre-cast candidate and source object, then assert the relevant penalty difference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/phase-ai/src/policies/anti_self_harm.rs` around lines 3279 - 3281,
Update the test fixture around combat attacker setup to exercise the production
cast/pre-cast flow: replace GameAction::ChooseTarget with a real cast/pre-cast
candidate using an actual pump spell source object, so AntiSelfHarmPolicy::score
reaches score_pre_cast and pump_taps_blocker_penalty. Remove the synthetic
ObjectId(100) setup and assert the expected penalty difference for the tapped
attacker behavior.
Source: Path instructions
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: request changes — the current head fixes the earlier compile blocker, but it still loses the required-to-attack fact during the resolution-time intervening-if check and adds a new verbatim parser dispatch.
🔴 Blocker
crates/engine/src/game/effects/deal_damage.rs:728-769 snapshots damage-source characteristics into DamageRecord, but it does not carry AttackerInfo.required_to_attack. crates/engine/src/game/filter.rs:1570-1605 then reconstructs a synthetic source and routes the filter through the ordinary live-state evaluator, whose new FilterProp::RequiredToAttack arm at crates/engine/src/game/filter.rs:4183-4197 searches state.combat.attackers. A source leaving the battlefield before Firkraag's trigger resolves is removed from that list by crates/engine/src/game/zones.rs:461-465 / effects/remove_from_combat.rs:48-69, so the second CR 603.4 check becomes false even though the creature had to attack when attackers were declared. The official Firkraag rules update defines this as a declaration-time fact. Carry that fact in the damage-source event snapshot and make the damage-record matcher consume it; add a runtime case where the required attacker leaves before the trigger resolves, plus MustAttack and MustAttackPlayer sibling coverage.
crates/engine/src/parser/oracle_trigger.rs:5175-5192 adds the exact Oracle string "if that creature had to attack this combat" to try_extract_simple_condition. New parser dispatch must use the shared nom grammar, not a new verbatim table entry. Move this to a composable condition parser in parser/oracle_nom/condition.rs (including the pronoun/subject axis) and lower it to the existing typed EventDamageSourceMatchesFilter representation.
✅ Clean
The current head resolves the prior AttackerInfo construction failures with the existing defaulting constructor. The parse-diff artifact remains scoped to Firkraag alone, matching the PR claim.
Recommendation: request changes — implement the event-time combat snapshot and nom condition parser, then re-run the current-head runtime coverage.
…ot and parse the had-to-attack condition via the nom grammar The resolution-time CR 603.4 re-check of Firkraag's intervening-if read FilterProp::RequiredToAttack from live combat state; a required attacker leaving the battlefield before the trigger resolves is erased from state.combat.attackers by remove_from_combat, flipping the check false. Snapshot the declaration-time fact into DamageRecord (source_required_to_attack) at damage time and resolve the prop from that snapshot in matches_target_filter_on_damage_record_source, mirroring normalize_contextual_filter's resolve-then-delegate shape. Runtime coverage: required attacker dies with the trigger on the stack, plus MustAttack and MustAttackPlayer sibling requirements. Replace the verbatim try_extract_simple_condition table entry with a composable nom condition parser (subject / predicate / scope axes) in oracle_nom/condition.rs, lowering through a new StaticCondition::EventDamageSourceMatchesFilter carrier bridged 1:1 to the existing TriggerCondition::EventDamageSourceMatchesFilter.
|
Both blockers from the 2026-07-22 review are addressed at the current head. Snapshot carried through the damage record. New runtime coverage in
Composable condition parser. The verbatim Verification: issue_4732 file 5/5 green; full integration suite 3753 passed / 0 failed (2 ignored); engine lib suite green; |
|
Maintainer status for current head |
matthewevans
left a comment
There was a problem hiding this comment.
Current head 8da90b4 remains blocked.
- The tapped-attacker test constructs GameAction::ChooseTarget, which takes the target-scoring branch and never reaches score_pre_cast or pump_taps_blocker_penalty. It therefore cannot discriminate the claimed pre-cast behavior.
- AttackerInfo.required_to_attack and the duplicated DamageRecord snapshot remain raw serialized bools. This declaration-time state needs a typed domain representation before it becomes another parallel boolean contract.
Please replace the target-selection fixture with a real CastSpell pre-cast path and resolve the typed snapshot design, then request review again.
|
This PR remains blocked on the requested changes for its current head. Please address them and push an update within 7 days; otherwise, this PR will be automatically closed as stale. A new commit or substantive response will return it to review. |
|
Closing under the requested-changes expiry policy. The warning posted on August 2 for head |
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests