test(engine): runtime regression for Riptide Gearhulk optional per-opponent targets (#5994)#6034
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the per_opponent_target_fanout_min function to correctly handle optional target fanouts by scanning for prefixes at word boundaries, ensuring that abilities do not fizzle when a target is declined. The reviewer correctly identified that the implementation of a manual parsing loop violates the project's architectural rule requiring the use of nom combinators for parser dispatch and provided a refactored, idiomatic solution.
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.
| pub(super) fn per_opponent_target_fanout_min(text: &str) -> usize { | ||
| let mut remaining = text; | ||
| while !remaining.is_empty() { | ||
| if let (_, Some(spec)) = strip_optional_target_prefix(remaining) { | ||
| return if spec.min_is_fixed_zero() { 0 } else { 1 }; | ||
| } | ||
| // allow-noncombinator: structural word-boundary advance on a runtime ' ' | ||
| // separator (the documented scan idiom), not dispatch against a literal. | ||
| remaining = remaining | ||
| .find(' ') | ||
| .map_or("", |idx| remaining[idx + 1..].trim_start()); | ||
| } | ||
| 1 | ||
| } |
There was a problem hiding this comment.
This manual parsing loop should be refactored to use nom combinators. Per the repository rules, we avoid manual string slicing and verbatim matching for Oracle phrases to ensure maintainability and robustness. Please use the existing nom-based primitives instead.
| pub(super) fn per_opponent_target_fanout_min(text: &str) -> usize { | |
| let mut remaining = text; | |
| while !remaining.is_empty() { | |
| if let (_, Some(spec)) = strip_optional_target_prefix(remaining) { | |
| return if spec.min_is_fixed_zero() { 0 } else { 1 }; | |
| } | |
| // allow-noncombinator: structural word-boundary advance on a runtime ' ' | |
| // separator (the documented scan idiom), not dispatch against a literal. | |
| remaining = remaining | |
| .find(' ') | |
| .map_or("", |idx| remaining[idx + 1..].trim_start()); | |
| } | |
| 1 | |
| } | |
| pub(super) fn per_opponent_target_fanout_min(text: &str) -> usize { | |
| let parser = |i: &str| -> nom::IResult<&str, MultiTargetSpec, OracleError<'_>> { | |
| let (rem, spec_opt) = strip_optional_target_prefix(i); | |
| if let Some(spec) = spec_opt { | |
| Ok((rem, spec)) | |
| } else { | |
| Err(nom::Err::Error(OracleError::new( | |
| i, | |
| nom::error::ErrorKind::Tag, | |
| ))) | |
| } | |
| }; | |
| if let Some((spec, _)) = nom_primitives::scan_at_word_boundaries(text, parser) { | |
| if spec.min_is_fixed_zero() { 0 } else { 1 } | |
| } else { | |
| 1 | |
| } | |
| } |
References
- Avoid verbatim string equality for parsing Oracle phrases; use modular, reusable nom-based parsers and combinators to improve maintainability.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — two correctness/verification blockers remain on 3ef0bf405d0d0f85c8990d875394b1101dcb1b46.
🟡 Medium
-
Correct the CR citation. Evidence:
crates/engine/src/parser/oracle_effect/lower.rs:3436andcrates/engine/src/parser/oracle_effect/tests.rs:46610cite CR 115.10 for optional target count. The local Comprehensive Rules source atdocs/MagicCompRules.txt:886-890says 115.10 only distinguishes objects affected without targeting; it does not establish thatup to one targetpermits zero choices. Remove 115.10 (retaining the verified relevant CR 601.2c is sufficient) or replace it with a verified rule that supports the claim. -
Add a production target-selection/resolution regression. Evidence:
crates/engine/src/parser/oracle_effect/tests.rs:46688calls onlyparse_oracle_textand assertsMultiTargetSpec.min; it does not exercise the reported behavior that declining a per-opponent target must not fizzle Riptide Gearhulk's ETB. Drive the real trigger pipeline throughWaitingFor::TriggerTargetSelection, including the per-opponent player/object slots: prove one opponent can be selected while another object's optional slot is declined, and prove all optional object slots can be declined while the ability resolves. The test must fail if theminlowering reverts to 1.
✅ Clean
The verb-general optional-prefix scan is at the shared per-opponent fanout lowering seam and follows the repository's documented word-boundary scan idiom; the direct parser coverage spans put, exile, and gain control.
Please address both findings, then rerun the current-head parser/CI evidence. I did not apply a maintainer fixup: the required runtime scenario needs a verified player/object/None selection sequence, and the PR explicitly identifies related fanout-resolution behavior as out of scope.
Parse changes introduced by this PR · 4 card(s), 4 signature(s) (baseline: main
|
|
This PR remains blocked by the requested changes on |
3ef0bf4 to
5e5bad5
Compare
…n + fix CR citation Address the two review blockers on phase-rs#6034. 1. CR citation. `per_opponent_target_fanout_min`'s doc and the Riptide parser test cited CR 115.10, which governs objects/players AFFECTED without being targeted -- it does not establish that "up to one target" permits zero choices. Replace it with the verified CR 601.2c: a spell/ability "with a variable number of targets" lets the player announce how many targets they will choose, which is the rule that makes "up to one target" optional. (Verified against docs/MagicCompRules.txt:2461.) 2. Runtime regression. Add crates/engine/tests/integration/riptide_gearhulk_5994.rs, driving Riptide's real ETB through the cast pipeline into WaitingFor::TriggerTargetSelection with two opponents: - `riptide_declining_all_per_opponent_targets_does_not_fizzle` declines every per-opponent target and asserts the ETB resolves back to Priority (not a stall) with both permanents untouched. - `riptide_selects_one_opponent_target_and_declines_the_other` chooses one opponent's permanent (moved to its owner's library) while declining the other's optional slot (untouched), and asserts the ability resolves. Both are non-vacuous: verified they PASS with the fix and FAIL when `per_opponent_target_fanout_min` is forced back to a mandatory min of 1 -- on that revert Riptide lowers to a non-targeted `EffectZoneChoice` that stalls mid-resolution (never reaching Priority) and the chosen permanent never moves, so each assertion flips. Refs phase-rs#5994
📝 WalkthroughWalkthroughAdds Riptide Gearhulk integration coverage for optional per-opponent nonland-permanent targets, including cases where all targets are declined and where only one opponent’s permanent is moved to its owner’s library. ChangesRiptide Gearhulk regression coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…opponent targets (phase-rs#5994) The parser fix for phase-rs#5994 landed via phase-rs#6565 (the verb-general `per_opponent_target_fanout_min` scan). This adds the runtime target-selection regression that main was still missing, driving Riptide's real ETB through the cast pipeline into `WaitingFor::TriggerTargetSelection` with two opponents: - `riptide_declining_all_per_opponent_targets_does_not_fizzle` declines every optional per-opponent target and asserts the ETB resolves back to `Priority` (both permanents untouched) rather than stalling mid-resolution. - `riptide_selects_one_opponent_target_and_declines_the_other` chooses one opponent's permanent (moved to its owner's library) while declining the other's optional slot (untouched), and asserts the ability resolves. Both are non-vacuous: they pass on current main and fail if `per_opponent_target_fanout_min` is forced back to a mandatory min of 1 — on that revert Riptide lowers to a non-targeted `EffectZoneChoice` that stalls (never reaching `Priority`) and the chosen permanent never moves, so the `final_waiting_for()` and zone assertions each flip. Refs phase-rs#5994
5e5bad5 to
cf8c27a
Compare
galuis116
changed the title
|
Update: while rebasing to address your two blockers, I found that #6565 (merged) already landed the parser fix for #5994 � it rewrote Rather than push a now-redundant duplicate, I've repurposed this PR to just the runtime regression you asked for (
Both are non-vacuous: they pass on current The CR-citation blocker is moot now that the parser code is Happy to close this instead if you'd rather not carry a test-only PR; the coverage seemed worth keeping given you'd asked for the runtime scenario. |
2 participants
What this is now
The parser fix for #5994 landed independently via #6565 (merged), which rewrote
per_opponent_target_fanout_minwith the verb-generalscan_at_word_boundariesscan. #5994 is closed. This PR is repurposed to just the runtime target-selection regression � coveragemainwas still missing � rebased onto currentmain.Tests
crates/engine/tests/integration/riptide_gearhulk_5994.rsdrives Riptide Gearhulk's real ETB through the cast pipeline intoWaitingFor::TriggerTargetSelectionwith two opponents, each controlling a nonland permanent:riptide_declining_all_per_opponent_targets_does_not_fizzle� declines every optional per-opponent target; asserts the ETB resolves back toPriority(both permanents untouched), i.e. it does not stall or fizzle.riptide_selects_one_opponent_target_and_declines_the_other� chooses one opponent's permanent (moved to its owner's library third from the top) while declining the other's optional slot (untouched); asserts the ability resolves.Both are non-vacuous: they pass on current
mainand fail ifper_opponent_target_fanout_minis forced back to a mandatorymin: 1. On that revert Riptide lowers to a non-targetedEffectZoneChoicethat stalls short ofPriority, and the chosen permanent never moves � so thefinal_waiting_for()and zone assertions each flip.cargo fmt,clippy -p engine --tests, and the new integration tests are green.Refs #5994
Summary by CodeRabbit
Bug Fixes
Tests