fix(parser): Weathered Wayfarer activation when opponent has more lands - #2835
Conversation
Parse "an opponent controls more [type] than you" as a PlayerCount ControlsCount check instead of aggregating all opponent permanents. Adds Weathered Wayfarer activation regression tests. Fixes phase-rs#859 Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Code Review
This pull request addresses issue #859 by updating the parser for "an opponent controls more [type] than you" to correctly perform an existential check over individual opponents rather than aggregating their permanents. It also adds integration tests for Weathered Wayfarer. Feedback on the changes highlights a missing three-player integration test to fully verify the existential logic, an incorrect Magic Comprehensive Rules (CR) citation in the comments, and an opportunity to use the existing inject_controller_you helper for more idiomatic Rust code.
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.
| !can_activate_ability_now(runner.state(), P0, wayfarer, 0), | ||
| "equal land counts must block activation" | ||
| ); | ||
| } |
There was a problem hiding this comment.
[HIGH] Missing 3-player integration test.
Why it matters: The PR description's test plan explicitly mentions verifying activation in a three-player game to ensure it requires a single opponent with more lands (existential check) rather than a combined opponent total, but no such test was implemented.
Suggested fix: Append a 3-player integration test to verify this behavior.
}
#[test]
fn weathered_wayfarer_blocked_in_three_player_game_when_no_single_opponent_has_more_lands() {
use engine::game::scenario::P2;
let mut scenario = GameScenario::new();
scenario.at_phase(Phase::PreCombatMain);
let wayfarer = scenario
.add_creature_from_oracle(P0, "Weathered Wayfarer", 1, 1, WEATHERED_WAYFARER)
.id();
scenario.add_basic_land(P0, ManaColor::White);
scenario.add_basic_land(P0, ManaColor::White);
scenario.add_basic_land(P1, ManaColor::Blue);
scenario.add_basic_land(P1, ManaColor::Blue);
scenario.add_basic_land(P2, ManaColor::Green);
let runner = scenario.build();
assert!(
!can_activate_ability_now(runner.state(), P0, wayfarer, 0),
"no single opponent has more lands than P0 (P1 has 2, P2 has 1, P0 has 2), even though aggregate is 3"
);
}| // CR 109.4: "an opponent controls more [type] than you" — existential over | ||
| // opponents (at least one opponent strictly exceeds your count), not an | ||
| // aggregate of all opponent permanents. Weathered Wayfarer, Tithe, etc. |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation.
Why it matters: CR 109.4 describes who has a controller on the stack or battlefield, which is unrelated to the definition of an opponent or existential checks.
Suggested fix: Update the citation to CR 102.2 (defining 'opponent') and/or CR 109.5 (defining 'you' and 'your').
| // CR 109.4: "an opponent controls more [type] than you" — existential over | |
| // opponents (at least one opponent strictly exceeds your count), not an | |
| // aggregate of all opponent permanents. Weathered Wayfarer, Tithe, etc. | |
| // CR 102.2 / CR 109.5: "an opponent controls more [type] than you" — existential over | |
| // opponents (at least one opponent strictly exceeds your count), not an | |
| // aggregate of all opponent permanents. Weathered Wayfarer, Tithe, etc. |
References
- Every rules-touching line of engine code must carry a comment of the form CR : . A CR annotation where the cited rule's body does not describe what the code is doing is a finding. (link)
| let you_filter = match &type_filter { | ||
| TargetFilter::Typed(tf) => { | ||
| TargetFilter::Typed(tf.controller(ControllerRef::Opponent)) | ||
| TargetFilter::Typed(tf.clone().controller(ControllerRef::You)) | ||
| } | ||
| other => other, | ||
| }; | ||
| let you_filter = match parse_type_phrase(type_text.trim()) { | ||
| (TargetFilter::Typed(tf), _) => { | ||
| TargetFilter::Typed(tf.controller(ControllerRef::You)) | ||
| } | ||
| (other, _) => other, | ||
| other => other.clone(), | ||
| }; |
There was a problem hiding this comment.
[MEDIUM] Non-idiomatic filter construction.
Why it matters: Manually matching and cloning type_filter to inject the controller is verbose and bypasses the codebase's standard inject_controller_you helper, which also ensures the filter is correctly restricted to the battlefield.
Suggested fix: Reuse the existing inject_controller_you helper.
let you_filter = inject_controller_you(type_filter.clone());References
- Idiomatic Rust pillar: uses the type system, ownership model, and standard library idioms to their fullest. Composable building blocks pillar: reuse existing helpers before writing new manual logic. (link)
The two 2-player tests pass identically under the old aggregate parse (one opponent's count == all opponents' counts), so neither discriminated the fix. The new 3-player test (two opponents with one land each vs your one land) fails under aggregate semantics and passes under existential — verified by reverting condition.rs to origin/main. Also cite CR 109.5 for the 'than you' anchor and swap the Tithe example (demonstrative 'that opponent', different parse path) for Land Tax.
Replaces the hand-rolled Typed-match/clone with the file's existing helper, which also pins the battlefield InZone restriction consistently with every other condition branch.
Maintainer review — FIXED, NOW READYVerdict: Right seam, right building block. Discrimination audit (empirical, via revert trick):
Gemini findings, confirmed and resolved (pushed
Note on "Fixes #859": the 2-player activation failure reported in #859 does not reproduce on current main even without this parser change (proven by the revert run) — the runtime blocker was most likely the castability gating fixed in #2788. This PR's real value is the multiplayer rules correction; the issue link is still fine since the parse is now CR-correct for the card. Verification: Not approving/merging — leaving that to the lead. |
matthewevans
left a comment
There was a problem hiding this comment.
Correct multiplayer-rules fix reusing the documented PlayerFilter::ControlsCount parameterization (no new variants). The added 3-player discriminating test (two opponents with 1 land each vs your 1) fails on revert — it pins the existential-vs-aggregate distinction the 2-player tests couldn't. CR 109.4/109.5 verified in the rules text. condition tests green, CI green.
Summary
Test plan
Fixes #859
Made with Cursor