fix: gate discard triggers on madness intervening-if (closes #5143) by RealDiligent · Pull Request #5146 · phase-rs/phase

RealDiligent · 2026-07-05T15:06:37Z

Summary

Fix Anje Falkenrath (and similar cards) firing their discard untap rider on every discard by hoisting the "if it has madness" intervening-if instead of dropping it.
Add TriggerCondition::EventObjectMatchesFilter plus KeywordKind::Madness so event-object predicates can gate on off-zone keyword grants.
Wire mtgish discard triggers (WhenAPlayerDiscardsACard, DiscardedCardPassesFilter) for the mtgish card-data path.

Closes #5143.

Test plan

Parser regression: trigger_intervening_if_discarded_card_has_madness
Engine unit test: event_object_madness_intervening_if_gates_discard_trigger
CI green on Rust lint + test shards

) Anje Falkenrath's untap rider was firing on every discard because the madness intervening-if was dropped at parse time. Add EventObjectMatchesFilter for event-object intervening-ifs, KeywordKind::Madness for off-zone keyword checks, and mtgish discard-trigger conversion for the mtgish card path. Co-authored-by: Cursor <cursoragent@cursor.com>

gemini-code-assist

Code Review

This pull request implements support for the "if it has madness" intervening-if condition on discard triggers (such as for Anje Falkenrath) by introducing a new EventObjectMatchesFilter trigger condition that falls back to Last Known Information (LKI) when necessary. It also updates the parser and importer to handle these discard-card filters. The review feedback correctly identifies a critical compilation error due to a duplicate definition of the discard_trigger function in crates/mtgish-import/src/convert/trigger.rs, as well as a violation of rule R1 in crates/engine/src/parser/oracle_trigger.rs where a verbatim .find() string search is used for parsing dispatch instead of nom combinators.

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 · 2026-07-05T15:08:20Z

+/// CR 701.9 + CR 603: Build a `Discarded` trigger with the player axis on
+/// `valid_target` and optional discarded-card predicates on `valid_card`.
+fn discard_trigger(
+    players: &Players,
+    cards: &CardsInHand,
+    idiom: &'static str,
+) -> ConvResult<TriggerDefinition> {
+    let mut def = TriggerDefinition::new(TriggerMode::Discarded);
+    if !matches!(players, Players::AnyPlayer) {
+        let controller = players_to_controller(players)?;
+        def.valid_target = Some(TargetFilter::Typed(
+            TypedFilter::default().controller(controller),
+        ));
+    }
+    match cards {
+        CardsInHand::AnyCard => {}
+        CardsInHand::SingleCardInHand(crate::schema::types::CardInHand::ThisCardInHand) => {
+            def.valid_card = Some(TargetFilter::SelfRef);
+        }
+        other => {
+            return Err(ConversionGap::EnginePrerequisiteMissing {
+                engine_type: "TriggerDefinition",
+                needed_variant: format!("{idiom} with discarded-card filter: CardsInHand::{other:?}"),
+            });
+        }
+    }
+    Ok(def)
+}


[CRITICAL] Duplicate definition of discard_trigger function. Evidence: crates/mtgish-import/src/convert/trigger.rs:878-905.\n\nWhy it matters: Defining the same function twice in the same module will cause a compilation error.\n\nSuggested fix: Remove the duplicate definition of discard_trigger.

gemini-code-assist · 2026-07-05T15:08:20Z

+    // discard triggers whose subject is the discarded card (Anje Falkenrath).
+    // MUST precede the zone-change-object "if it " arms below, which would
+    // otherwise mis-route this predicate.
+    if let Some(pos) = tp.find("if it has madness") {


[HIGH] Verbatim string search .find() used for parsing dispatch instead of nom combinators / parse_inner_condition. Evidence: crates/engine/src/parser/oracle_trigger.rs:3898.\n\nWhy it matters: This violates R1 (no .find for parsing dispatch) and the condition extraction rule requiring delegation to parse_inner_condition in oracle_nom/condition.rs.\n\nSuggested fix: Move the "if it has madness" parsing logic into parse_inner_condition in crates/engine/src/parser/oracle_nom/condition.rs using nom combinators, and delegate to it.

References

Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. Any new .find() used for parsing dispatch in non-test parser code is a violation. ^(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 and compose them using idiomatic combinator aggregates.

Co-authored-by: Cursor <cursoragent@cursor.com>

matthewevans

Thanks for working on the madness intervening-if path. I can't approve this head yet because there are two blocking issues that need to be cleaned up first.

crates/mtgish-import/src/convert/trigger.rs defines discard_trigger twice in the same module. The first copy starts at line 851 and the second starts at line 880, so this head cannot compile. The current CI failures line up with that local evidence. Please collapse the new discard-card handling into the existing helper rather than adding a duplicate definition.
crates/engine/src/parser/oracle_trigger.rs handles this condition with a one-off tp.find("if it has madness") check around line 3898. Parser dispatch in this repo needs to use the existing nom/combinator path or a shared condition parser so the grammar stays extensible. Please replace the verbatim phrase search with a composable parser path for the intervening-if clause.

Once those are fixed, the runtime test for the trigger condition is the right kind of boundary to keep: it should continue proving that a madness discard satisfies the condition and a non-madness discard does not.

Co-authored-by: Cursor <cursoragent@cursor.com>

matthewevans

Thanks for the quick update. The duplicate discard_trigger definition is fixed on this head.

I still can't approve yet because the parser-seam blocker remains: crates/engine/src/parser/oracle_trigger.rs still handles the madness intervening-if with tp.find("if it has madness") around line 3899. This is still a verbatim phrase search in parser dispatch rather than a composed parser/condition path, so it does not meet the parser architecture bar for a new trigger condition.

Please replace that branch with a nom/combinator path or a shared intervening-if condition parser, then keep the current positive/negative runtime boundary for EventObjectMatchesFilter.

Co-authored-by: Cursor <cursoragent@cursor.com>

github-actions · 2026-07-05T15:33:54Z

Parse changes introduced by this PR · 2 card(s), 3 signature(s) (baseline: main `c8198e7f56c7`)

1 card(s) · ability/Discard · added: `Discard (count=1, target=controller)`

Examples: Pollywog Symbiote

1 card(s) · trigger/Discarded · field `condition`: `∅` → `event object is with madness card`

Examples: Anje Falkenrath

1 card(s) · trigger/SpellCast · field `condition`: `∅` → `event object is with mutate card`

Examples: Pollywog Symbiote

RealDiligent · 2026-07-05T15:50:05Z

CI green on head 9b468e370 — workflow 28745811956:

Rust lint (fmt, clippy, parser gate) — success
Rust tests (shard 1/2, shard 2/2) — success
Card data (generate, validate, coverage) — success
Frontend (lint, type-check, test) — success
WASM compile check — success
Rust aggregate gate — success

matthewevans

Thanks for the latest update. The previous duplicate discard_trigger definition is fixed, and the old tp.find("if it has madness") dispatch blocker is gone. I still can't approve this head because the native parser is still a one-keyword branch instead of using the existing alternate-cost keyword parser seam.

[MED] The native parser still special-cases Anje's exact rider instead of using the existing alt-cost keyword parser seam. Evidence: crates/engine/src/parser/oracle_trigger.rs:4352 hardcodes tag("if it has madness"), while crates/engine/src/parser/oracle_nom/primitives.rs:700 already owns named alternate-cost keyword parsing via parse_alt_cost_keyword_name_to_kind and was not extended with KeywordKind::Madness. Why it matters: this keeps the prior wrong-seam risk alive as a one-card parser branch; the parse-diff currently shows exactly one Anje-only signature change, but the parser infrastructure has a reusable keyword-name axis for this class. Suggested fix: add Madness to parse_alt_cost_keyword_name_to_kind and parse if it has <alt-cost keyword> into the same HasKeywordKind event-object condition through that shared combinator.

Add Madness to parse_alt_cost_keyword_name_to_kind and parse "if it has <alt-cost keyword>" via the shared combinator instead of a hardcoded madness-only tag branch. Co-authored-by: Cursor <cursoragent@cursor.com>

RealDiligent · 2026-07-05T16:44:09Z

Addressed the alt-cost keyword seam feedback on head 3cc40a177:

Added KeywordKind::Madness to parse_alt_cost_keyword_name_to_kind in oracle_nom/primitives.rs
Replaced the hardcoded tag("if it has madness") branch with parse_event_object_has_alt_cost_keyword_intervening_if, which delegates the keyword name to the shared combinator

Ready for another look when you have a moment.

Co-authored-by: Cursor <cursoragent@cursor.com>

matthewevans

The prior parser-seam blocker looks addressed on this head: parse_event_object_has_alt_cost_keyword_intervening_if now delegates the keyword name through parse_alt_cost_keyword_name_to_kind, and Madness is registered there. I still can't approve/enqueue because the exact current head is failing the mechanical Rust lint gate.

[HIGH] Current head fails rustfmt. Evidence: crates/engine/src/parser/oracle_trigger.rs:3898 and crates/engine/src/parser/oracle_trigger.rs:4360; cargo fmt --all -- --check on head 3d64338f37f545ffaca835d3b3696ffcdcc736f8 rewrites both spots, and the live Rust lint (fmt, clippy, parser gate) job is failing for this head. Why it matters: this exact head cannot pass the required Rust lint gate, so it is not ready for handler approval/enqueue. Suggested fix: run cargo fmt --all, commit the resulting oracle_trigger.rs formatting, and repush.

Co-authored-by: Cursor <cursoragent@cursor.com>

matthewevans

The parser-seam issue remains resolved on this head, but the exact current head still fails the mechanical Rust formatting gate.

[HIGH] Current head fails rustfmt. Evidence: crates/engine/src/parser/oracle_trigger.rs:4359; cargo fmt --all -- --check on head 3952b06557744ee3a7ee4eda4d0e5310a50f9bb0 rewrites the EventObjectMatchesFilter construction. Why it matters: this exact head cannot pass the required Rust lint gate, so it is not ready for approval/enqueue. Suggested fix: run cargo fmt --all, commit the resulting oracle_trigger.rs formatting, and repush.

Co-authored-by: Cursor <cursoragent@cursor.com>

RealDiligent · 2026-07-05T17:45:11Z

CI green on head 20d442a36 — all workflow jobs success.

Matthew feedback: madness intervening-if now routes through shared parse_alt_cost_keyword_name_to_kind + event_object_has_alt_cost_keyword_condition helper (not a one-card madness tag branch).
rustfmt: helper filter: block reformatted; restored KeywordKind import dropped in an earlier cleanup commit.

Ready for re-review.

matthewevans

Maintainer sweep: reviewed current head after the follow-up fixes. The madness intervening-if gate is at the trigger scan/read-write seam, has a discriminating engine test, and current CI is green.

RealDiligent requested a review from matthewevans as a code owner July 5, 2026 15:06

gemini-code-assist Bot reviewed Jul 5, 2026

View reviewed changes

fix: cargo fmt for madness discard trigger PR

54ccb6b

Co-authored-by: Cursor <cursoragent@cursor.com>

matthewevans requested changes Jul 5, 2026

View reviewed changes

fix: restore CoreType import and cover EventObjectMatchesFilter matches

e1f1ee4

Co-authored-by: Cursor <cursoragent@cursor.com>

matthewevans requested changes Jul 5, 2026

View reviewed changes

fix: use nom combinator for madness intervening-if parser gate

9b468e3

Co-authored-by: Cursor <cursoragent@cursor.com>

matthewevans requested changes Jul 5, 2026

View reviewed changes

refactor: route discard intervening-if through alt-cost keyword parser

3cc40a1

Add Madness to parse_alt_cost_keyword_name_to_kind and parse "if it has <alt-cost keyword>" via the shared combinator instead of a hardcoded madness-only tag branch. Co-authored-by: Cursor <cursoragent@cursor.com>

fix: remove unused import and rustfmt oracle_trigger

3d64338

Co-authored-by: Cursor <cursoragent@cursor.com>

matthewevans requested changes Jul 5, 2026

View reviewed changes

fix: cargo fmt for alt-cost keyword intervening-if parser

3952b06

Co-authored-by: Cursor <cursoragent@cursor.com>

matthewevans requested changes Jul 5, 2026

View reviewed changes

RealDiligent and others added 3 commits July 6, 2026 01:04

fix: extract alt-cost keyword condition helper for rustfmt

facae40

Co-authored-by: Cursor <cursoragent@cursor.com>

fix: rustfmt alt-cost keyword condition helper for CI

9c0d652

Co-authored-by: Cursor <cursoragent@cursor.com>

fix: restore KeywordKind import for alt-cost helper

20d442a

Co-authored-by: Cursor <cursoragent@cursor.com>

matthewevans self-assigned this Jul 5, 2026

matthewevans added the bug Bug fix label Jul 5, 2026

matthewevans approved these changes Jul 5, 2026

View reviewed changes

matthewevans added this pull request to the merge queue Jul 5, 2026

matthewevans removed their assignment Jul 5, 2026

Merged via the queue into phase-rs:main with commit bcdde1f Jul 5, 2026
11 checks passed

Uh oh!

Uh oh!

Conversation

RealDiligent commented Jul 5, 2026

Summary

Test plan

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

matthewevans left a comment

Choose a reason for hiding this comment

Uh oh!

matthewevans left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

1 card(s) · ability/Discard · added: Discard (count=1, target=controller)

1 card(s) · trigger/Discarded · field condition: ∅ → event object is with madness card

1 card(s) · trigger/SpellCast · field condition: ∅ → event object is with mutate card

Uh oh!

RealDiligent commented Jul 5, 2026

Uh oh!

matthewevans left a comment

Choose a reason for hiding this comment

Uh oh!

RealDiligent commented Jul 5, 2026

Uh oh!

matthewevans left a comment

Choose a reason for hiding this comment

Uh oh!

matthewevans left a comment

Choose a reason for hiding this comment

Uh oh!

RealDiligent commented Jul 5, 2026

Uh oh!

matthewevans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Jul 5, 2026 •

edited

Loading

Parse changes introduced by this PR · 2 card(s), 3 signature(s) (baseline: main `c8198e7f56c7`)

1 card(s) · ability/Discard · added: `Discard (count=1, target=controller)`

1 card(s) · trigger/Discarded · field `condition`: `∅` → `event object is with madness card`

1 card(s) · trigger/SpellCast · field `condition`: `∅` → `event object is with mutate card`