fix(parser): existential opponent control activation restrictions - #3002
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements parsing and resolution for activation restrictions where an opponent controls more (or at least N more) of a specific card type than you, addressing issues related to cards like Weathered Wayfarer and Isolated Watchtower. Feedback suggests refactoring the parser logic in crates/engine/src/parser/oracle_nom/condition.rs to avoid nested if let Ok chains and fragile verbatim parsing by using modular nom combinators, adhering to Rule R1 of the style guide.
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.
| if let Ok((rest2, _)) = tag::<_, _, OracleError<'_>>("controls at least ").parse(rest) { | ||
| if let Ok((rest3, n)) = parse_number(rest2) { | ||
| if let Ok((rest4, _)) = tag::<_, _, OracleError<'_>>(" more ").parse(rest3) { | ||
| if let Ok((rest5, type_text)) = | ||
| take_until::<_, _, OracleError<'_>>(" than you").parse(rest4) | ||
| { | ||
| let (rest5, _) = tag(" than you").parse(rest5)?; | ||
| let (type_filter, _) = parse_type_phrase(type_text.trim()); | ||
| let you_filter = inject_controller_you(type_filter.clone()); | ||
| return Ok(( | ||
| rest5, | ||
| StaticCondition::QuantityComparison { | ||
| lhs: QuantityExpr::Ref { | ||
| qty: QuantityRef::PlayerCount { | ||
| filter: PlayerFilter::ControlsCount { | ||
| relation: PlayerRelation::Opponent, | ||
| filter: type_filter, | ||
| comparator: Comparator::GE, | ||
| count: Box::new(QuantityExpr::Offset { | ||
| inner: Box::new(QuantityExpr::Ref { | ||
| qty: QuantityRef::ObjectCount { | ||
| filter: you_filter, | ||
| }, | ||
| }), | ||
| offset: n as i32, | ||
| }), | ||
| }, | ||
| }, | ||
| }, | ||
| comparator: Comparator::GE, | ||
| rhs: QuantityExpr::Fixed { value: 1 }, | ||
| }, | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Avoid nested if let Ok chains and fragile verbatim parsing for Oracle phrases.
Why it matters: Nested if let Ok blocks create a "pyramid of doom" that is hard to read. Furthermore, parsing compound Oracle phrases using verbatim string matches or fragile sequential tags bypasses the robust nom-based parser. Instead, we should decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates.
Suggested fix: Refactor the parsing logic to decompose the phrase into modular constituent parsers and compose them using idiomatic nom combinators, avoiding nested if let Ok chains.
let parse_at_least_more = |i| {
let (i, _) = parse_controls_at_least(i)?;
let (i, n) = parse_number(i)?;
let (i, _) = parse_more(i)?;
let (i, type_filter) = parse_type_phrase(i)?;
let (i, _) = parse_than_you(i)?;
Ok((i, (n, type_filter)))
};
if let Ok((rest2, (n, type_filter))) = parse_at_least_more(rest) {
let you_filter = inject_controller_you(type_filter.clone());
return Ok((
rest2,
StaticCondition::QuantityComparison {
lhs: QuantityExpr::Ref {
qty: QuantityRef::PlayerCount {
filter: PlayerFilter::ControlsCount {
relation: PlayerRelation::Opponent,
filter: type_filter,
comparator: Comparator::GE,
count: Box::new(QuantityExpr::Offset {
inner: Box::new(QuantityExpr::Ref {
qty: QuantityRef::ObjectCount {
filter: you_filter,
},
}),
offset: n as i32,
}),
},
},
},
comparator: Comparator::GE,
rhs: QuantityExpr::Fixed { value: 1 },
},
));
}References
- Rule R1: Every new parser dispatch under
crates/engine/src/parser/must use nom 8.0 combinators or delegate to existing helpers. Avoid nestedif let Okmatching chains (pyramid of doom) for parsing dispatch. (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 to prevent combinatorial explosion and improve maintainability.
487fd83 to
ca217a4
Compare
# Conflicts: # crates/engine/tests/fixtures/integration_cards.json # crates/engine/tests/integration/main.rs
matthewevans
left a comment
There was a problem hiding this comment.
Approved after maintainer cleanup and re-review on the current-base head.
Evidence checked:
- The fix is at the activation restriction / parsed condition seam: Weathered Wayfarer-style gates are stored in
activation_restrictionsasRequiresCondition, not on the resolution-timeconditionfield. - The cross-player count semantics are existential per opponent, not aggregate across all opponents; three-player tests cover the combined-opponents false-positive case.
- I tightened the parser after review: the new
at least N more [type] than youpath is now a composed helper, and malformed type phrases fail closed instead of falling through to the genericcontrols at least N ...arm. - The generated fixture conflict from the branch being behind was resolved by taking current
origin/mainand grafting only the PR'sweathered wayfarerfixture entry. - CR 109.4, 109.5, 602.5b, and 603.4 citations were checked against
docs/MagicCompRules.txt.
Verification run locally on the pushed head:
cargo test -p engine --lib opponent_controls_more -- --nocapturecargo test -p engine --lib opponent_controls_at_least -- --nocapturecargo test -p engine --lib rejects_unknown_type_phrase -- --nocapturecargo test -p engine --test integration weathered_wayfarer -- --nocapture./scripts/check-parser-combinators.shgit diff --check
Summary
"Activate only if an opponent controls more lands than you"restriction is enforced viaactivation_restrictions, not the top-levelconditionfield (which is intentionallyNonefor resolution-time conditions)."an opponent controls at least N more [type] than you"(Isolated Watchtower), placed before the genericcontrols+parse_ge_thresholdarm so"at least two more lands"is not mis-parsed as"at least two"+ type"more lands than you"."an opponent [verb]"arms inparse_restriction_condition.Context
The Discord report looked at
abilities[0].condition: nulland concluded the restriction was dropped. Activation gates belong inactivation_restrictionsasRequiresCondition { condition: Some(...) }per CR 602.5b. Weathered Wayfarer was already correct in card-data after #2835/#859; this PR hardens the class and adds card-database integration coverage for the production hydration path.Test plan
cargo test -p engine --test integration weathered_wayfarercargo test -p engine --lib opponent_controls_at_leastcargo test -p engine --lib parses_activate_only_if_opponentcard-dataregen (Isolated Watchtower export will pick up the new parse on next pipeline run; parser unit tests cover AST shape today)