fix(parser): UNSUPPORTED full-set one-off: Unleash the Flux#5682
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 implements support for tracking coin flip outcomes during resolution (CR 705.2) to enable repeating processes based on coin flips, introducing AbilityCondition::CoinFlipOutcome and ResolutionCoinFlip to store the most recent flip result. The review feedback correctly identifies a violation of Rule R2 of the repository style guide, which prohibits the use of bool fields in structs. It is recommended to refactor the won: bool field in ResolutionCoinFlip to use the existing CoinFlipResult enum, and update all associated instantiations, evaluations, and test assertions accordingly.
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.
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct ResolutionCoinFlip { | ||
| pub flipper: PlayerId, | ||
| pub won: bool, | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Avoid using bool fields in structs; use the existing CoinFlipResult enum instead.
Why it matters: This violates Rule R2 of the repository style guide, which prohibits bool fields in favor of strongly-typed enums to better express the design space.
Suggested fix: Replace pub won: bool with pub result: CoinFlipResult.
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | |
| pub struct ResolutionCoinFlip { | |
| pub flipper: PlayerId, | |
| pub won: bool, | |
| } | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | |
| pub struct ResolutionCoinFlip { | |
| pub flipper: PlayerId, | |
| pub result: crate::types::ability::CoinFlipResult, | |
| } |
References
- Rule R2: No bool fields — parameterize with existing typed enums. A bool field never expresses the design space; the project uses typed enums like ControllerRef, Comparator, PlayerScope, Option, and dedicated discriminated unions instead. (link)
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | ||
| flipper: player, | ||
| won, | ||
| }); |
There was a problem hiding this comment.
[MEDIUM] Update ResolutionCoinFlip instantiation to use the result field with CoinFlipResult instead of the won boolean.
Why it matters: This aligns with the refactoring of ResolutionCoinFlip to adhere to Rule R2 of the repository style guide.
Suggested fix: Map the won boolean to CoinFlipResult.
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | |
| flipper: player, | |
| won, | |
| }); | |
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | |
| flipper: player, | |
| result: if won { crate::types::ability::CoinFlipResult::Won } else { crate::types::ability::CoinFlipResult::Lost }, | |
| }); |
References
- Rule R2: No bool fields — parameterize with existing typed enums. (link)
| }); | ||
| // CR 705.2: record the kept flip's result for the flipper (not the source's | ||
| // controller) so a `WhileCondition` loop gate reads the surviving flip. | ||
| state.resolution_coin_flip = Some(ResolutionCoinFlip { flipper, won }); |
There was a problem hiding this comment.
[MEDIUM] Update ResolutionCoinFlip instantiation in resume_after_keep to use the result field with CoinFlipResult instead of the won boolean.
Why it matters: This aligns with the refactoring of ResolutionCoinFlip to adhere to Rule R2 of the repository style guide.
Suggested fix: Map the won boolean to CoinFlipResult.
state.resolution_coin_flip = Some(ResolutionCoinFlip {
flipper,
result: if won { crate::types::ability::CoinFlipResult::Won } else { crate::types::ability::CoinFlipResult::Lost },
});References
- Rule R2: No bool fields — parameterize with existing typed enums. (link)
| assert_eq!( | ||
| Some(recorded.won), | ||
| event_won, | ||
| "recorded result must match the CoinFlipped event" | ||
| ); |
There was a problem hiding this comment.
[MEDIUM] Update the test assertion to check recorded.result against CoinFlipResult::Won instead of checking recorded.won.
Why it matters: This aligns with the refactoring of ResolutionCoinFlip to adhere to Rule R2 of the repository style guide.
Suggested fix: Compare recorded.result with CoinFlipResult::Won.
| assert_eq!( | |
| Some(recorded.won), | |
| event_won, | |
| "recorded result must match the CoinFlipped event" | |
| ); | |
| assert_eq!( | |
| Some(recorded.result == crate::types::ability::CoinFlipResult::Won), | |
| event_won, | |
| "recorded result must match the CoinFlipped event" | |
| ); |
References
- Rule R2: No bool fields — parameterize with existing typed enums. (link)
| assert_eq!( | ||
| state.resolution_coin_flip, | ||
| Some(ResolutionCoinFlip { | ||
| flipper: PlayerId(0), | ||
| won: true, | ||
| }), | ||
| "final resolution flip is the winning flip for the controller" | ||
| ); |
There was a problem hiding this comment.
[MEDIUM] Update the test assertion to construct ResolutionCoinFlip with result: CoinFlipResult::Won instead of won: true.
Why it matters: This aligns with the refactoring of ResolutionCoinFlip to adhere to Rule R2 of the repository style guide.
Suggested fix: Use result: CoinFlipResult::Won.
| assert_eq!( | |
| state.resolution_coin_flip, | |
| Some(ResolutionCoinFlip { | |
| flipper: PlayerId(0), | |
| won: true, | |
| }), | |
| "final resolution flip is the winning flip for the controller" | |
| ); | |
| assert_eq!( | |
| state.resolution_coin_flip, | |
| Some(ResolutionCoinFlip { | |
| flipper: PlayerId(0), | |
| result: crate::types::ability::CoinFlipResult::Won, | |
| }), | |
| "final resolution flip is the winning flip for the controller" | |
| ); |
References
- Rule R2: No bool fields — parameterize with existing typed enums. (link)
| AbilityCondition::CoinFlipOutcome { result } => { | ||
| state.resolution_coin_flip.is_some_and(|f| { | ||
| f.flipper == ability.controller && f.won == matches!(result, CoinFlipResult::Won) | ||
| }) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Simplify the CoinFlipOutcome condition evaluation by directly comparing the result field of ResolutionCoinFlip with the expected result.
Why it matters: This aligns with the refactoring of ResolutionCoinFlip to adhere to Rule R2 of the repository style guide and simplifies the logic by avoiding boolean matching.
Suggested fix: Directly compare f.result with *result.
| AbilityCondition::CoinFlipOutcome { result } => { | |
| state.resolution_coin_flip.is_some_and(|f| { | |
| f.flipper == ability.controller && f.won == matches!(result, CoinFlipResult::Won) | |
| }) | |
| } | |
| AbilityCondition::CoinFlipOutcome { result } => { | |
| state.resolution_coin_flip.is_some_and(|f| { | |
| f.flipper == ability.controller && f.result == *result | |
| }) | |
| } |
References
- Rule R2: No bool fields — parameterize with existing typed enums. (link)
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | ||
| flipper: PlayerId(0), | ||
| won: false, | ||
| }); |
There was a problem hiding this comment.
[MEDIUM] Update the test setup to construct ResolutionCoinFlip with result: CoinFlipResult::Lost instead of won: false.
Why it matters: This aligns with the refactoring of ResolutionCoinFlip to adhere to Rule R2 of the repository style guide.
Suggested fix: Use result: CoinFlipResult::Lost.
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | |
| flipper: PlayerId(0), | |
| won: false, | |
| }); | |
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | |
| flipper: PlayerId(0), | |
| result: CoinFlipResult::Lost, | |
| }); |
References
- Rule R2: No bool fields — parameterize with existing typed enums. (link)
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | ||
| flipper: PlayerId(0), | ||
| won: true, | ||
| }); |
There was a problem hiding this comment.
[MEDIUM] Update the test setup to construct ResolutionCoinFlip with result: CoinFlipResult::Won instead of won: true.
Why it matters: This aligns with the refactoring of ResolutionCoinFlip to adhere to Rule R2 of the repository style guide.
Suggested fix: Use result: CoinFlipResult::Won.
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | |
| flipper: PlayerId(0), | |
| won: true, | |
| }); | |
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | |
| flipper: PlayerId(0), | |
| result: CoinFlipResult::Won, | |
| }); |
References
- Rule R2: No bool fields — parameterize with existing typed enums. (link)
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | ||
| flipper: PlayerId(1), | ||
| won: false, | ||
| }); |
There was a problem hiding this comment.
[MEDIUM] Update the test setup to construct ResolutionCoinFlip with result: CoinFlipResult::Lost instead of won: false.
Why it matters: This aligns with the refactoring of ResolutionCoinFlip to adhere to Rule R2 of the repository style guide.
Suggested fix: Use result: CoinFlipResult::Lost.
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | |
| flipper: PlayerId(1), | |
| won: false, | |
| }); | |
| state.resolution_coin_flip = Some(ResolutionCoinFlip { | |
| flipper: PlayerId(1), | |
| result: CoinFlipResult::Lost, | |
| }); |
References
- Rule R2: No bool fields — parameterize with existing typed enums. (link)
Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: blocked — the new coin-flip result state can outlive its resolution, and the affected card lacks a production-pipeline regression test; request changes before merge.
🔴 Blocker
crates/engine/src/game/effects/mod.rs:6062 clears resolution_coin_flip only at the start of a WhileCondition iteration, while crates/engine/src/game/effects/mod.rs:8633 reads it whenever CoinFlipOutcome is evaluated. The state is documented as resolution-scoped but can therefore survive a completed resolution and affect a later evaluation. CR 608.2c requires that the controller follows the instructions in their written order; the card text says, “each player sacrifices a nonland permanent, then you flip a coin. If you lose the flip, repeat this process.” Move the reset to the authoritative resolution lifetime, including continuation completion, rather than one repeat branch.
crates/engine/src/game/effects/flip_coin.rs:1617 proves only a bare flip/repeat, while crates/engine/src/parser/oracle_effect/tests.rs:41813 checks Unleash the Flux only as an AST. Neither test reaches the real each-player sacrifice choices, continuation resume, and repeat decision. Add a registered runtime scenario that drives those choices and demonstrates that an actual loss repeats the whole process and a win stops it.
🟡 Non-blocking
crates/engine/src/types/game_state.rs:1154 introduces ResolutionCoinFlip { won: bool }, even though crates/engine/src/types/ability.rs:17814 already defines CoinFlipResult. Reuse that typed domain vocabulary so the stored result and AbilityCondition::CoinFlipOutcome cannot drift.
✅ Clean
The current parse-diff is scoped to Unleash the Flux: it removes the previous Unimplemented lose branch and adds the intended coin-flip continuation. CR 705.2 was verified: only the player who flips the coin wins or loses it.
Recommendation: request changes — give the flip result a true resolution/continuation lifetime, add the interactive runtime regression, and store the existing CoinFlipResult type.
…-unsupported-full-set-one
…CoinFlipResult Address @matthewevans's CHANGES_REQUESTED review on Unleash the Flux: - Blocker: `resolution_coin_flip` could outlive its resolution — it was only cleared at each `WhileCondition` iteration start, so a stale flip from one resolution could wrongly satisfy a later `CoinFlipOutcome` gate. Clear it at the authoritative resolution-lifetime boundary (top-level `resolve_ability_chain` entry, depth == 0), which uniformly covers loop completion, bounded-cap exit, and the no-loop case (CR 608.2c + CR 705.2). The per-iteration clear remains as the intra-loop boundary. - Reuse the existing `CoinFlipResult` typed vocabulary for the stored result (`ResolutionCoinFlip { flipper, result }` instead of `won: bool`) so the written value and `AbilityCondition::CoinFlipOutcome`'s read predicate can never drift; add `CoinFlipResult::from_won` as the single bool->enum authority. - Add a registered runtime scenario (GameScenario + GameRunner) that drives the each-player nonland-permanent sacrifice choices, the flip continuation resume, and the repeat decision — proving an actual LOSS repeats the whole process and a WIN stops it — plus a focused unit test proving a stale flip does not leak into the next resolution. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approve — the current head resolves the requested coin-flip lifetime, typed-state, and runtime-coverage gaps; ready for the merge queue.
🔴 Blocker
- None. The prior requested changes are resolved on
ae42ff638d82c2769f36149486d67d6b930207d9.
🟡 Non-blocking
- None.
✅ Clean
crates/engine/src/game/effects/mod.rs:5983-6002establishes the top-level resolution lifetime and:6078-6085resets eachWhileConditioniteration, preventing both cross-resolution and cross-iteration leakage.crates/engine/src/types/game_state.rs:1157-1161stores the existingCoinFlipResult, andcrates/engine/src/game/effects/flip_coin.rs:87-90,464-467records it at both normal and kept-flip authorities.crates/engine/src/game/effects/flip_coin.rs:1741-1884drives the parsed Unleash the Flux ability through real sacrifice selections, a losing flip that repeats, and a winning flip that stops. Current CI is green and the parse artifact is exactly one card with two expected signatures.
Recommendation: approve and enqueue with the existing bug label.
2 participants
Summary
Fixes a parser misparse affecting 1 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED full-set one-off: Unleash the Flux
Cards corrected
Fix
Implemented the approved plan for cluster 99 (Unleash the Flux, a Planechase Phenomenon) surgically. The card previously misparsed: "…then you flip a coin. If you lose the flip, repeat this process." routed "repeat this process" into FlipCoin.lose_effect as Unimplemented{"empty"}, silently dropping the loop.
Built one class-level primitive across all layers: a resolution-scoped coin-flip-result condition feeding the existing RepeatContinuation::WhileCondition loop, plus a nom parser reroute.
Changes:
Result: Unleash the Flux parses with 0 Unimplemented and repeat_until = WhileCondition{ CoinFlipOutcome{Lost} }, sub_ability FlipCoin{win:None,lose:None}. Runtime proof: while_condition_lost_flip_loops_until_win drives resolve_ability_chain through the real WhileCondition loop and asserts the loop repeats while lost and stops on a win, with resolution_coin_flip recording the winning flip. Added 9 tests total (3 stripper, 2 directive incl. non-repeat regression, 1 full-card, flip-records, controller-relativity, loop runtime proof).
Verification (cargo run directly — worktree not under Tilt): cargo fmt clean; cargo clippy -p engine --all-targets -D warnings PASS (exit 0); cargo test -p engine 2732 passed/0 failed; full gen-card-data.sh + semantic-audit confirm the card is clean corpus-wide. Krark/Sin/Claim Jumper regression tests pass.
Judgement calls: (1) check-parser-combinators.sh exits 1, but that is a PRE-EXISTING condition — its fixed diff baseline is PR #904 (far behind HEAD fa199f8), so it flags string-dispatch accumulated across untouched files (oracle.rs/sequence.rs/oracle_trigger.rs). Confirmed none of the flagged lines are mine and my added parser lines contain zero string dispatch. (2) Runtime proof strategy: I proved the NEW primitive (flip→condition→WhileCondition loop, controller-relativity, field recording) via focused cast/resolution-pipeline tests rather than building the full multi-player-sacrifice + phenomenon-encounter integration harness, because that composition's remaining surface (player-scope sacrifice fan-out, pending_continuation-before-pending_repeat_until drain ordering) is pre-existing, separately-tested machinery that my change does not modify; the parse test proves the full-card structure and the loop test proves the mechanism. (3) gen-card-data.sh incidentally regenerated two TRACKED data files (known-tokens.toml −9032 lines, oracle-subtypes.json +1) from my local mtgjson/token data differing from the committed baseline — unrelated to the fix. I restored both to HEAD (isolated upstream/main worktree, no other agent's uncommitted work here). card-data.json and semantic-audit outputs are gitignored.
No STOP-AND-RETURN, no scope expansion, no new subsystem — extension only.
Files changed
CR references
Verification
cargo fmt --all— passcheck-parser-combinators.sh (base=upstream/main merge-base fa7f157)— passcargo clippy -p engine --all-targets -- -D warnings— passcargo test -p engine— passoracle-gen data --filter 'unleash the flux'— passCards confirmed re-parsed correctly: unleash the flux
🤖 Generated with Claude Code