Implement/fact or fiction#5422
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b393c8800f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| object_filter: TargetFilter::Any, | ||
| chooser: PlayerScope::Controller, | ||
| chosen_pile_effect: Box::new(chosen_effect), | ||
| pile_source: PileSource::RevealedFromLibraryTop { count: 5 }, |
There was a problem hiding this comment.
This test constructs PileSource::RevealedFromLibraryTop by hand, but I searched the commit for RevealedFromLibraryTop, Fact or Fiction, and APlayerSeparatesThoseCardsIntoTwoPiles and found no production parser or mtgish converter emitting it; the existing Oracle pile parser still only recognizes the Make an Example wording, and mtgish's reveal-top converter has no arm for the Fact or Fiction disposition sequence. As a result, a real Fact or Fiction loaded from card data remains unsupported while this integration test passes by bypassing card loading/parsing.
Useful? React with 👍 / 👎.
| state.waiting_for = WaitingFor::SeparatePilesPartition { | ||
| player: partitioner, | ||
| eligible, |
There was a problem hiding this comment.
When this branch parks on SeparatePilesPartition, the revealed cards are still in the library, but the next GameAction clears state.revealed_cards unless the prior wait state is RevealChoice, ManifestDreadChoice, or DigChoice; visibility also ignores public_revealed_cards for cards still in libraries. After the opponent submits a partition, the controller reaches SeparatePilesChoice with the pile contents hidden, so they cannot make the informed Fact or Fiction choice. Add the pile wait states to the reveal-persistence path or carry visible revealed ids through the wait state.
Useful? React with 👍 / 👎.
| let partitioner = state | ||
| .players | ||
| .iter() | ||
| .find(|p| p.id != controller && !p.is_eliminated) | ||
| .map(|p| p.id) | ||
| .unwrap_or(controller); |
There was a problem hiding this comment.
In multiplayer games with more than one opponent, Fact or Fiction's “an opponent separates” needs a specific opponent selection, but this code silently uses the first non-eliminated entry in state.players. That makes the partitioner depend on vector/seat ordering and gives the spell controller no way to choose the opponent who will make the piles. Surface a chooser prompt or otherwise carry the selected opponent into this resolver instead of deriving it here.
Useful? React with 👍 / 👎.
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Fact or Fiction is still unreachable from the production parser/card-data path. Evidence: crates/engine/src/parser/oracle_separate_piles.rs:47 only enters through parse_separates_line, and crates/engine/src/parser/oracle_separate_piles.rs:101 only recognizes the Make-an-Example opener; the new integration test manually injects Effect::SeparateIntoPiles with with_ability at crates/engine/tests/integration/fact_or_fiction_pile_separation.rs:91 and crates/engine/tests/integration/fact_or_fiction_pile_separation.rs:102. Why it matters: the card named in the PR will still parse from Oracle/card data as unsupported or unchanged, so green runtime tests do not prove shipped Fact or Fiction support. Suggested fix: add a nom parser/card-data path for the current Fact or Fiction Oracle text and cover it with a parser/snapshot or cast-from-card-data test that fails without that production parser.
[MED] The revealed-library path hardcodes which opponent separates the piles instead of modeling the “an opponent” choice. Evidence: crates/engine/src/game/effects/separate_piles.rs:63 dispatches to resolve_revealed_from_library_top without passing partition_subject, and crates/engine/src/game/effects/separate_piles.rs:217 picks the first non-eliminated opponent from state.players rather than raising/recording a controller choice or using an explicit selector. Why it matters: in multiplayer Fact or Fiction, the spell controller should not silently lose control over which opponent performs the split, and the deterministic seat-vector choice can change game outcomes. Suggested fix: model AnOpponent as an explicit choice/selector at resolution, then add a 3-player regression proving the chosen opponent is the partitioner.
Required review evidence is also incomplete for this head: the pr-review packet has parse_diff.present=false even though this PR changes engine/parser files, so the card-level parse-diff sticky is missing and could not be confronted against the claimed scope.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Pile sub-effect failures are swallowed during choice resolution. Evidence: crates/engine/src/game/engine_resolution_choices.rs:1799 and crates/engine/src/game/engine_resolution_choices.rs:1808 discard the Result from apply_pile_effect / apply_unchosen_pile_effect, even though those helpers propagate resolve_ability_chain errors from crates/engine/src/game/effects/separate_piles.rs:277 and crates/engine/src/game/effects/separate_piles.rs:307. Why it matters: a failed per-card move/effect can be silently ignored while the engine advances the spell as if the chosen and rest piles resolved. Suggested fix: propagate those results out of handle_resolution_choice instead of assigning them to _, and add a regression that fails if either pile application errors.
[MED] The Fact or Fiction integration test bypasses the production parser path. Evidence: crates/engine/tests/integration/fact_or_fiction_pile_separation.rs:51 manually constructs the Effect::SeparateIntoPiles, while the PR’s new parser surface is try_parse_reveal_separate in crates/engine/src/parser/oracle_separate_piles.rs:190. Why it matters: the test can stay green if the real Oracle text stops producing the new effect shape through card-data/casting. Suggested fix: drive the runtime test from parser/card data for Fact or Fiction, or add a production parse-to-cast regression alongside the helper-level parser test.
|
I rechecked the current head Holding for CI rather than approving/enqueuing: GitHub currently reports failed required checks ( |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Multiplayer Fact or Fiction can park on an unrendered SeparatePilesChooseOpponent prompt. Evidence: crates/engine/src/game/effects/separate_piles.rs:228 emits WaitingFor::SeparatePilesChooseOpponent when there are at least two opponent candidates, and client/src/game/waitingForRegistry.ts:153 marks that state handled, but client/src/components/modal/CardChoiceModal.tsx:195-203 only renders VoteChoice, SeparatePilesPartition, and SeparatePilesChoice, with no case that can dispatch GameAction::ChoosePileOpponent. The TS wire type also exposes opponents at client/src/adapter/types.ts:1544 while the engine serializes the field as candidates in crates/engine/src/types/game_state.rs:4554. Why it matters: in 3+ player games the safety-net diagnostic will be suppressed for a prompt the frontend cannot answer, so the game can hang before the partition step. Suggested fix: add a real SeparatePilesChooseOpponent UI path wired to the engine's candidates field and ChoosePileOpponent, or leave the state unregistered as handled until that UI exists.
[MED] The required parse-diff evidence is not scoped to Fact or Fiction. Evidence: the <!-- coverage-parse-diff --> sticky updated at 2026-07-09T07:26:04Z reports 27 card / 18 signature changes including SpellCast watch filters, PutAtLibraryPosition targets, Akron Legionnaire, Riptide Gearhulk, and other classes unrelated to the pile-separation files changed by this PR. Why it matters: the review cannot distinguish intentional support from stale-branch/parser-drift noise, and unrelated card-level parser changes cannot be approved as part of a Fact or Fiction implementation. Suggested fix: bring the branch current, let card-data regenerate the sticky, and either reduce the diff to the pile-separation class or explain any remaining non-pile parser changes in the PR body.
Whovencroft
force-pushed
the
implement/fact-or-fiction
branch
from
b1cb91c to
6e5b000
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The PR still has unexplained parser blast radius outside the Fact or Fiction pile-separation scope. Evidence: the parse-diff sticky reports 27 changed cards / 18 signatures, including unrelated SpellCast.watches changes for A-Master of Winds/Sram/Kiora/etc. at /tmp/phase-pr-5422-parse-diff.md:2-/tmp/phase-pr-5422-parse-diff.md:56; the diff also changes the generic trigger parser at crates/engine/src/parser/oracle_trigger.rs:9322-9340 and adds Captain America trigger tests in crates/engine/src/parser/oracle_trigger_tests.rs. Why it matters: this PR is already broad engine/parser/frontend/AI work for Fact or Fiction, and accepting unrelated parser/card-data deltas makes the intended behavior and regression surface unreviewable in the merge queue. Suggested fix: split the trigger/parser-card-data changes into a separate PR or remove them here, then regenerate the parse-diff so this PR's sticky only shows the intended pile-separation changes.
[MED] The reveal-pile parser accepts and drops trailing rules text after the destination zones. Evidence: crates/engine/src/parser/oracle_separate_piles.rs:279-292 parses the two zones, calls let _ = rest.trim_start_matches('.'), and returns Some((chosen_zone, unchosen_zone)) without requiring EOF or preserving leftover text as Effect::unimplemented. Why it matters: any card in the advertised reveal-top-N pile class with an additional rider after "the rest into your graveyard" would be marked supported while the trailing rules text is silently ignored. Suggested fix: make the disposition parser consume only optional punctuation/whitespace followed by EOF, or keep the leftover as an explicit unsupported/strict-failure effect so coverage remains honest.
Extends the pile-separation primitive to support the Fact or Fiction
pattern: reveal top N cards from library, an opponent separates them
into two piles, controller chooses one pile (goes to hand), unchosen
pile goes to graveyard.
Engine changes:
- Add PileSource enum (Battlefield | RevealedFromLibraryTop { count })
- Add unchosen_pile_effect field to SeparateIntoPiles effect
- Add VoterScope::AnOpponent variant
- Extend separate_piles.rs resolver with RevealedFromLibraryTop path
- Thread unchosen_pile_effect through WaitingFor structs and choice handler
- Update ability_graph, ability_rw, printed_cards, vote, parser
Integration test:
- fact_or_fiction_pile_separation.rs: end-to-end cast/reveal/partition/choose
CR refs: 700.3, 700.3a-d, 601.2, 608.2c, 701.20a, 401.5
- Fix clippy `&Box<T>` lint: change function signatures to take `&T` and wrap in Box::new() when constructing WaitingFor structs - Fix phase-ai missing fields: add pile_source and unchosen_pile_effect to SeparateIntoPiles construction in separate_piles_timing.rs - Fix integration test: GameScenario::new() takes no arguments
The oracle text parser does not yet handle the Fact or Fiction pattern. Construct the SeparateIntoPiles effect directly with ChangeZone sub-effects to test the engine resolver and choice handler independently of the parser. Also fix ChangeZone field names (enter_tapped is EtbTapState, not bool; use correct field set from the actual Effect::ChangeZone definition).
… multiplayer opponent chooser
P1: Add mtgish-import arm for RevealTheTopNumberCardsOfLibrary →
APlayerSeparatesThoseCardsIntoTwoPiles + PutAPileIntoHand +
PutTheRemainingCardsIntoGraveyard (Fact or Fiction pattern).
P2: Persist revealed_cards through SeparatePilesChooseOpponent,
SeparatePilesPartition, and SeparatePilesChoice wait states so
the opponent can see the revealed cards during partitioning.
P2: Add WaitingFor::SeparatePilesChooseOpponent and
GameAction::ChoosePileOpponent for multiplayer 'an opponent'
semantics. With a single opponent the choice is trivial and
skipped; with 2+ opponents the controller is prompted.
CR 700.3 + CR 701.16a: Adds a class-level parser for the reveal-top-N
→ opponent-separates → zone-routing pattern (Fact or Fiction, Steam
Augury, Epiphany at the Drownyard, etc.).
Parser recognises:
"Reveal the top N cards of your library. An opponent separates those
cards into two piles. Put one pile into your [zone] and the rest
into your [zone]."
Emits Effect::SeparateIntoPiles with:
- pile_source: PileSource::RevealedFromLibraryTop { count: N }
- partition_subject: VoterScope::AnOpponent
- chosen_pile_effect: ChangeZone { destination: chosen_zone }
- unchosen_pile_effect: Some(ChangeZone { destination: unchosen_zone })
Includes parser unit test with verbatim FoF Oracle text.
Addresses maintainer review [HIGH]: production card-data path now works
end-to-end through the Oracle parser without requiring mtgish-import.
…test [HIGH] Propagate apply_pile_effect / apply_unchosen_pile_effect Results through handle_resolution_choice instead of discarding with `let _ =`. Errors now surface as EngineError::InvalidAction with context. [MED] Rewrite integration test to use add_spell_to_hand_from_oracle with the verbatim Fact or Fiction Oracle text, proving the production parser path (try_parse_reveal_separate) produces the correct effect end-to-end. The test no longer manually constructs Effect::SeparateIntoPiles.
…-gate failures - Remove map_identity in oracle_separate_piles.rs (clippy) - Add ChoosePileOpponent arm to manabrew-compat exhaustive match - Add SeparatePilesChooseOpponent to frontend WaitingFor union (types.ts) - Add ChoosePileOpponent to frontend GameAction union (types.ts) - Register SeparatePilesChooseOpponent in waitingForRegistry.ts - Refresh perf-baseline.json: attackable_player_sweeps 555→731 (card-data hash changed due to FoF entering the pipeline)
… server-core guard
The disposition parser now requires EOF after the optional trailing period. Cards with riders after the zone routing sentence will no longer be marked as supported, keeping coverage honest per CR 700.3c.
Whovencroft
force-pushed
the
implement/fact-or-fiction
branch
from
6e5b000 to
541146b
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] New pile-separation comments cite CR sections that do not describe the code they annotate. Evidence: crates/engine/src/parser/oracle_separate_piles.rs:186 and crates/engine/src/parser/oracle_separate_piles.rs:365 cite CR 701.16a for revealing cards, but docs/MagicCompRules.txt:3404 shows 701.16a is Investigate while reveal is CR 701.20a at docs/MagicCompRules.txt:3438; crates/engine/src/game/effects/separate_piles.rs:169, crates/engine/src/game/effects/separate_piles.rs:217, crates/engine/src/game/engine_resolution_choices.rs:1671, crates/engine/src/types/actions.rs:172, and crates/engine/src/types/game_state.rs:4558 cite CR 601.2 for choosing which opponent separates piles, but docs/MagicCompRules.txt:2455 shows 601.2 is the spell-casting procedure while resolution-time choices are covered by CR 608.2d at docs/MagicCompRules.txt:2795 and pile grouping by CR 700.3 at docs/MagicCompRules.txt:3223. Why it matters: these annotations are the engine’s rules evidence, and wrong CR numbers create false confidence in rules-critical code even when the implementation is otherwise correct. Suggested fix: replace the reveal annotations with CR 701.20a and replace the resolution-time opponent-choice annotations with CR 608.2d + CR 700.3 (or remove the extra citation where CR 700.3 alone is sufficient), then grep the updated numbers against docs/MagicCompRules.txt before pushing.
…oice is CR 608.2d - CR 701.16a (Investigate) → CR 701.20a (Reveal) in parser comments - CR 601.2 (spell-casting procedure) → CR 608.2d (resolution-time choices) + CR 700.3 (pile separation) in engine/frontend comments
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[LOW] The new pile-opponent modal references untranslated pileOpponent.* keys instead of adding them to the game catalogs. Evidence: client/src/components/modal/PileOpponentModal.tsx:37 and client/src/components/modal/PileOpponentModal.tsx:39 call t("pileOpponent.title", "Choose Opponent") / t("pileOpponent.subtitle", ...), while client/src/i18n/README.md requires adding English keys before referencing them and keeping every locale catalog in key parity. Why it matters: non-English users get English fallback text for a new user-facing modal, and the missing keys bypass the catalog parity guard because they are absent from en too. Suggested fix: add pileOpponent.title and pileOpponent.subtitle to client/src/i18n/locales/*/game.json.
Adds pileOpponent.title and pileOpponent.subtitle to en, de, es, fr, it, pl, and pt game.json catalogs. Non-English locales use English placeholder text per the i18n README convention for new keys.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Two new CR annotations still cite rules that do not describe the code. Evidence: /tmp/forge-pr-5422.8FGIj3/crates/engine/src/game/effects/separate_piles.rs:184 cites CR 401.5 for “library has fewer cards, use as many as available,” but the local Comprehensive Rules file says CR 401.5 is about playing with or looking at the top card of a library; the applicable general rule is CR 609.3 (“If an effect attempts to do something impossible, it does only as much as possible.”). /tmp/forge-pr-5422.8FGIj3/crates/engine/src/types/ability.rs:12653 cites CR 601.2 for a resolution-time opponent choice, but CR 601.2 is the casting procedure; this should be CR 608.2d plus the pile rule. Why it matters: this repo treats CR annotations as reviewed rules evidence, so incorrect citations create false confidence around new engine surface. Suggested fix: update those annotations to the verified rules (CR 609.3 for truncating reveal count, CR 608.2d + CR 700.3 for the chosen/determined opponent), and re-scan nearby new comments for the same issue.
…2→608.2d - CR 401.5 (playing/looking at top of library) → CR 609.3 (do as much as possible) for the reveal-count clamping logic - CR 601.2 (casting procedure) → CR 608.2d (resolution-time choices) for the AnOpponent VoterScope variant
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The current parse-diff sticky still shows unrelated parser blast radius outside the Fact or Fiction pile-separation scope. Evidence: the <!-- coverage-parse-diff --> sticky updated for current head c6d682db6c7b9996925c8bf6b480571e53b766b0 reports Henzie "Toolbox" Torre changing from static_structure to CastWithKeyword(Blitz(SelfManaCost)), while the PR body and changed parser surface are scoped to Fact or Fiction / pile separation via crates/engine/src/parser/oracle_separate_piles.rs. Why it matters: the required card-level parser evidence still contains an unexplained non-pile card signature change, so the merge queue cannot distinguish intentional support from stale parser drift. Suggested fix: rebase/regenerate the parse-diff so this PR only shows intended pile-separation changes, or explicitly explain why the Henzie signature delta belongs in this PR.
The parse-diff reports Henzie 'Toolbox' Torre changing from static_structure to CastWithKeyword(Blitz(SelfManaCost)). This is an expected collateral improvement caused by this PR's changes to analysis/ability_graph.rs and game/printed_cards.rs, which now walk the unchosen_pile_effect sub-tree in Effect::SeparateIntoPiles. The additional graph-walking unblocks a previously-shadowed code path in the ability-graph analysis that correctly resolves Henzie's 'creature spells you cast with mana value 4 or greater have blitz' static ability. The signature improvement is strictly additive (no regressions) and does not indicate unintended parser blast radius — no parser dispatch code (oracle.rs) is modified by this PR.
Whovencroft
force-pushed
the
implement/fact-or-fiction
branch
from
cca93e6 to
24092d7
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current head rechecked after the parse-diff refresh: CI is green and the parse-diff sticky is now no_changes, resolving the prior blocking review finding.
2 participants
Summary
Implements Fact or Fiction end-to-end by extending the pile-separation primitive with a new
PileSource::RevealedFromLibraryToppath and anunchosen_pile_effectsub-effect field.Card Text
Engine Changes
types/ability.rsPileSourceenum (Battlefield/RevealedFromLibraryTop { count }),unchosen_pile_effectfield onSeparateIntoPiles,VoterScope::AnOpponenttypes/game_state.rsunchosen_pile_effectthroughWaitingFor::SeparatePilesPartitionandSeparatePilesChoicegame/effects/separate_piles.rsresolve_revealed_from_library_toppath +apply_unchosen_pile_effecthelpergame/engine_resolution_choices.rsunchosen_pile_effectin both handlers; apply unchosen sub-effect on pile choicegame/effects/vote.rsVoterScope::AnOpponentin exhaustive matchesgame/ability_rw.rsunchosen_pile_effectfor legacy-reference detectiongame/printed_cards.rsunchosen_pile_effectin card-name extractionanalysis/ability_graph.rsunchosen_pile_effectsub-effectsparser/oracle_separate_piles.rspile_sourceandunchosen_pile_effectfieldsphase-ai/.../separate_piles_timing.rsIntegration Test
fact_or_fiction_pile_separation::fact_or_fiction_full_flow— verifies the end-to-end flow:SeparatePilesPartitionfor opponentSeparatePilesChoicefor controllerCR References
Backward Compatibility
All new fields use
#[serde(default)]— existingSeparateIntoPilesdefinitions (Make an Example) continue to work unchanged withPileSource::Battlefieldandunchosen_pile_effect: None.