Fix Talion, the Kindly Lord chosen-number spell trigger (#1999)

[kiannidev]

Summary

Fixes #1999: Talion, the Kindly Lord was triggering on every opponent spell cast but never applying life loss or draw.

Root cause was twofold:

1. Condition/effect split — find_effect_boundary treated the comma in mana value, power, or toughness as the effect boundary, truncating the trigger condition and breaking spell-quality parsing.
2. Spell filter — The opponent-cast parser only recognized with mana value equal to the chosen number, not Talion’s full mana value, power, or toughness disjunction, so valid_card was never set and the trigger fired unconditionally.

Changes

• Teach find_effect_boundary to skip commas that continue a spell-stat disjunction (continues_spell_quality_disjunction).
• Add parse_spell_chosen_number_quality producing FilterProp::AnyOf over CMC / power / toughness == ChosenNumber.
• Stop re-splitting the opponent-cast spell clause on , (condition text is already effect-free).
• Parser and boundary tests for Talion’s oracle line.

Test plan

• cargo test -p engine --lib talion
• cargo test -p engine --lib find_effect_boundary_skips_spell_quality_comma
• Manual: ETB choose a number; opponent casts a spell matching MV, power, or toughness → 2 life loss + draw; non-matching spell → no trigger

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[matthewevans]

🤖 Architecture Review (automated)

Verdict: ⚠️ Changes requested

Seam: PASS — Correct layer (trigger parser), reuses existing FilterProp::{Cmc,PtComparison,AnyOf}, QuantityRef::ChosenNumber, PtStat/PtValueScope (all already in engine-inventory.json); no new variants, runtime consumers in game/filter.rs + game/quantity.rs confirmed (not parsed-but-unused).
Idiomatic: CONCERN — New continues_spell_quality_disjunction dispatches with raw starts_with("…")/contains("…") string literals, unlike its three sibling continuation detectors in the same block which all use nom tag().
Value: Class-level — suffix-table + disjunction generalizes the prior single-CMC path to the "mana value[, power, or toughness] equal to the chosen/that number" spell-quality class, not just Talion.
Reconciled with existing reviews: none — no Gemini review or inline comments present on this PR.

CR check: CR 202.3 (mana value) and CR 208.1 (creature P/T numbers) both verified present in docs/MagicCompRules.txt. Annotations accurate.

Findings

• [HIGH] crates/engine/src/parser/oracle_trigger.rs:22-29 (continues_spell_quality_disjunction) — Dispatches via trimmed.starts_with("power, or "), .starts_with("toughness, or "), .starts_with("or "), .contains("equal to the chosen number"), .contains("equal to that number"). These are string-literal .starts_with("/.contains(" forms with no allow-noncombinator marker, so they will trip scripts/check-parser-combinators.sh (SCOPE = crates/engine/src/parser; FORBIDDEN_METHODS matches \.starts_with\(" and \.contains\(", filtering only lines tagged allow-noncombinator). This is also the repo's chore: update coverage stats and badges #1 idiomatic rule (nom combinators for parsing dispatch) and is inconsistent with the three sibling detectors (continues_player_action_list, continues_disjunctive_zone_change_condition, continues_serial_event_condition) that all use tag::<_,_,OracleError>("or "). Fix: rewrite the connector dispatch with nom tag() mirroring the siblings; for the clause-content test, prefer matching the single clause segment (truncate at next , like continues_disjunctive_zone_change_condition does) rather than .contains() over the whole remainder — .contains() can match a later "chosen number" mention in the effect clause. (Note: the strip_suffix calls in parse_spell_chosen_number_quality are correctly marked allow-noncombinator — that sanctioned structural-suffix exception is fine; only the new continuation detector is the problem.)

No correctness or CR defects found beyond the above; the boundary trace for the Talion line is sound (comma-1 → power, or, comma-2 → or …equal to the chosen number, comma-3 → effect boundary), and the suffix table is class-level.

[kiannidev]

🤖 Architecture Review (automated)

Verdict: ⚠️ Changes requested

Seam: PASS — Correct layer (trigger parser), reuses existing FilterProp::{Cmc,PtComparison,AnyOf}, QuantityRef::ChosenNumber, PtStat/PtValueScope (all already in engine-inventory.json); no new variants, runtime consumers in game/filter.rs + game/quantity.rs confirmed (not parsed-but-unused). Idiomatic: CONCERN — New continues_spell_quality_disjunction dispatches with raw starts_with("…")/contains("…") string literals, unlike its three sibling continuation detectors in the same block which all use nom tag(). Value: Class-level — suffix-table + disjunction generalizes the prior single-CMC path to the "mana value[, power, or toughness] equal to the chosen/that number" spell-quality class, not just Talion. Reconciled with existing reviews: none — no Gemini review or inline comments present on this PR.

CR check: CR 202.3 (mana value) and CR 208.1 (creature P/T numbers) both verified present in docs/MagicCompRules.txt. Annotations accurate.

Findings

• [HIGH] crates/engine/src/parser/oracle_trigger.rs:22-29 (continues_spell_quality_disjunction) — Dispatches via trimmed.starts_with("power, or "), .starts_with("toughness, or "), .starts_with("or "), .contains("equal to the chosen number"), .contains("equal to that number"). These are string-literal .starts_with("/.contains(" forms with no allow-noncombinator marker, so they will trip scripts/check-parser-combinators.sh (SCOPE = crates/engine/src/parser; FORBIDDEN_METHODS matches \.starts_with\(" and \.contains\(", filtering only lines tagged allow-noncombinator). This is also the repo's chore: update coverage stats and badges #1 idiomatic rule (nom combinators for parsing dispatch) and is inconsistent with the three sibling detectors (continues_player_action_list, continues_disjunctive_zone_change_condition, continues_serial_event_condition) that all use tag::<_,_,OracleError>("or "). Fix: rewrite the connector dispatch with nom tag() mirroring the siblings; for the clause-content test, prefer matching the single clause segment (truncate at next , like continues_disjunctive_zone_change_condition does) rather than .contains() over the whole remainder — .contains() can match a later "chosen number" mention in the effect clause. (Note: the strip_suffix calls in parse_spell_chosen_number_quality are correctly marked allow-noncombinator — that sanctioned structural-suffix exception is fine; only the new continuation detector is the problem.)

No correctness or CR defects found beyond the above; the boundary trace for the Talion line is sound (comma-1 → power, or, comma-2 → or …equal to the chosen number, comma-3 → effect boundary), and the suffix table is class-level.

I updated code from the review

[matthewevans]

Maintainer follow-up pushed.

Changes made:

• Merged current origin/main into the PR branch.
• Kept the author’s nom-combinator cleanup for the Talion spell-quality comma boundary.
• Added a runtime cast/trigger regression that sets Talion’s chosen number and verifies the trigger fires for matching mana value, matching power, and matching toughness, but not a non-matching spell.

Verification run:

• cargo fmt --all
• ./scripts/check-parser-combinators.sh
• cargo test -p engine --lib talion -- --nocapture (6/6 focused tests)

I also rechecked the final diff for the handler security/instruction-path exclusions before pushing.

[matthewevans]

Maintainer review: approved after nom-parser cleanup, runtime trigger regression coverage, and final architecture pass.