fix(parser): UNSUPPORTED full-set one-off: Human—Time Lord Meta-Crisis#4810
Conversation
Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
…-unsupported-full-set-one # Conflicts: # client/src/adapter/types.ts # client/src/game/waitingForRegistry.ts # crates/engine/src/ai_support/candidates.rs # crates/engine/src/game/effects/mod.rs # crates/engine/src/game/engine_resolution_choices.rs # crates/engine/src/game/scenario.rs # crates/engine/src/parser/oracle_effect/tests.rs # crates/engine/src/types/game_state.rs # crates/phase-ai/src/decision_kind.rs
There was a problem hiding this comment.
Code Review
This pull request implements the EachPlayerCopyChosen effect (CR 101.4, CR 707.2, CR 122.1) to support card mechanics like Human—Time Lord Meta-Crisis. The changes span the Rust game engine—adding the self-iterating effect, APNAP walk, replacement choice pauses, and AI support—and the React frontend, which introduces the EachPlayerCopyChosenModal component. A high-severity issue was identified in the frontend modal where using the data.eligible array reference directly as a useEffect dependency causes the user's selection to reset on every render. This should be fixed by serializing the eligible targets to a string of keys for the dependency array.
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.
| useEffect(() => { | ||
| setSelected([]); | ||
| }, [data.eligible]); |
There was a problem hiding this comment.
[HIGH] Array reference dependency in useEffect resets selection on every render.
Why it matters: Since data.eligible is an array, its reference changes on every state update or parent re-render (due to deserialization or store updates), causing the useEffect to run and wipe out the user's selection.
Suggested fix: Serialize the eligible targets to a string of keys and use that as the dependency.
| useEffect(() => { | |
| setSelected([]); | |
| }, [data.eligible]); | |
| const eligibleKeys = data.eligible.map(targetKey).join(","); | |
| useEffect(() => { | |
| setSelected([]); | |
| }, [eligibleKeys]); |
matthewevans
left a comment
There was a problem hiding this comment.
I can't approve this yet. The parser shape looks structurally reasonable, but the implementation has a couple of correctness blockers that need to be fixed before this can go to the queue.
-
The
EachPlayerCopyChosenwalk currently performs a player's copy/counter action before later players have made their choices.advance_to_next_playerauto-drives a forced single choice directly intodrive_from_copy(crates/engine/src/game/effects/each_player_copy_chosen.rs:163), and after each per-player actionperform_counter_step_then_advanceimmediately advances to the next player's prompt (crates/engine/src/game/effects/each_player_copy_chosen.rs:295). CR 101.4 is stricter: APNAP gathers the choices first, then the actions happen simultaneously. This needs to be split into a choice-collection phase that records every scoped player's ordered selection, followed by an action phase that creates the tokens / places counters from the completed selection set. -
Replacement-resume events from this effect can be dropped when the resume advances into another non-priority prompt.
engine_replacement.rsdrainspending_each_player_copy_chosenafter copy/counter replacement choices (crates/engine/src/game/engine_replacement.rs:694), and that drain can emit token/counter events and then seed the nextEachPlayerCopyChosenSelection. Becauseengine::applyonly runs the post-action pipeline when the returned waiting state isPriority(crates/engine/src/game/engine.rs:4726), those emitted ETB/token/counter triggers are not scanned or deferred in that path. Please batch or drain those events before returning a non-priority waiting state, mirroring the deferred-trigger handling in theSelectTargetscontinuation arm. -
The frontend adds
cardChoice.eachPlayerCopyChosen.*only toclient/src/i18n/locales/en/game.json, butclient/src/i18n/resources.test.tsenforces key parity across every shipped locale. Add the same key block tode/es/fr/it/pl/ptgame catalogs so the locale parity gate stays green.
Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review on 210c276de720a5dc3f224249c86a88c4bda6aed0:
- This head does not compile after merging current
main; CI reports non-exhaustiveEffect::EachPlayerCopyChosenmatches incrates/engine/src/game/ability_scan.rs. - The APNAP blocker is still present. CR 101.4 requires all players to make the required choices first, then the actions happen simultaneously; the current implementation still performs a player's copy/counter action before later players have made their choices.
- Chosen-object live revalidation is still missing. The handler validates membership in the serialized eligible snapshot, but does not re-check that the object is still on the battlefield, controlled by the player, and matching the choose filter at submission/execution time.
- The replacement-resume trigger concern also still appears unresolved: the replacement resume path can return a non-priority waiting state after
drain_pendingwithout the normal trigger collection/drain path.
Please fix those before regenerating current-head proof/parse-diff. I am not approving or enqueueing this in its current state.
matthewevans
left a comment
There was a problem hiding this comment.
I still need changes before this can land.
EachPlayerCopyChosen currently performs each player’s copy/counter action before later players have made their choices. CR 101.4 requires all choices first, then simultaneous actions; doing the early token/counter action can incorrectly affect later choices and game state. Please split this into a choice-collection phase and a later action phase that executes from the completed selection set.
There is a second replacement-resume issue: events emitted while draining the replacement resume path can skip trigger processing if the resumed walk advances into another prompt. Please batch or drain events produced by the replacement-resume path before returning a non-priority prompt, matching the SelectTargets continuation handling, and add coverage for the skipped ETB/token/counter observer case.
matthewevans
left a comment
There was a problem hiding this comment.
Approved after maintainer repair pass: merge conflicts/base drift resolved, requested-change blockers addressed, proof metadata completed, and branch pushed to current main. CI is pending on the new head; auto-merge can wait for required checks.
…-full-set-one' into work/ntindle-pr-4810
matthewevans
left a comment
There was a problem hiding this comment.
Approved after maintainer repair pass on current head: base drift merged, AI effect classifier exhaustiveness blocker addressed, formatting/whitespace checks passed locally, and auto-merge may wait for required CI.
|
Sweep note: this PR is approved and checks are green, but the maintainer update-branch step currently fails with conflicts ( |
1 similar comment
|
Sweep note: this PR is approved and checks are green, but the maintainer update-branch step currently fails with conflicts ( |
# Conflicts: # crates/engine/src/parser/oracle_effect/mod.rs # crates/engine/src/types/ability.rs # crates/engine/src/types/game_state.rs # crates/phase-ai/src/policies/effect_classify.rs
matthewevans
left a comment
There was a problem hiding this comment.
I still need changes on the current head before this can land.
-
EachPlayerCopyChosenstill executes the current player's copy/counter action before later players have made their choices. The forced-single path callsdrive_from_copyimmediately (crates/engine/src/game/effects/each_player_copy_chosen.rs:163), the submitted-selection path does the same (crates/engine/src/game/engine_resolution_choices.rs:4332), andperform_counter_step_then_advanceadvances only after the current player's action has already run (crates/engine/src/game/effects/each_player_copy_chosen.rs:237,:295). For the Human-Time Lord shape, the engine needs to collect all APNAP choices first, then execute the copy/counter actions from that completed selection set. -
The submitted choices are validated only against the old prompt snapshot. Prompt eligibility is computed live (
crates/engine/src/game/effects/each_player_copy_chosen.rs:150), but submission only checkseligible.contains(t)(crates/engine/src/game/engine_resolution_choices.rs:4295) before copying the selected object. Please re-check that selected objects are still on the battlefield, still controlled by the choosing player, and still matchchoose_filterbefore accepting/executing the choice.
The parse-diff evidence is scoped to Human-Time Lord Meta-Crisis, and the previous modal dependency issue appears fixed via eligibleKey; these two engine issues remain blocking.
matthewevans
left a comment
There was a problem hiding this comment.
Approved current head 11e9e9047d1e3053c6c70f29a8a5b1fca94c2762 after re-review.
The previous blockers are addressed: EachPlayerCopyChosen now collects the complete APNAP choice set before running any copy/counter actions, and submitted choices are live-revalidated against current controller, battlefield membership, and the effect filter before being accepted. I also checked the new pause/resume wiring around token-copy and counter replacement drains and did not find a remaining correctness blocker.
Parse-diff is posted for this head; CI can finish under auto-merge.
matthewevans
left a comment
There was a problem hiding this comment.
Approved on current head 11e9e90 after the review-blocker repair. The EachPlayerCopyChosen flow now collects all APNAP choices before running copy/counter actions, submitted objects are live-revalidated against battlefield/controller/filter state, and CI is green across Rust lint/tests, card-data, frontend, WASM/Tauri, lobby, and AI gates.
Summary
Fixes a parser misparse affecting 1 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED full-set one-off: Human—Time Lord Meta-Crisis
Cards corrected
Fix
Implemented cluster-92 (Human—Time Lord Meta-Crisis) FULLY per the approved REVISED v5 plan by building a new class-level self-iterating interactive effect
Effect::EachPlayerCopyChosenacross all layers. The card previously misparsed (third sentence →Unimplemented); it now lowers its wholePlaneswalkedTobody to a singleEachPlayerCopyChosen{min:1,max:2, copy_modifications:[RemoveSupertype(Legendary)], scale:Some{P1P1,Power}}withplayer_scope:Alland 0 Unimplemented.DIFF SUMMARY (by subsystem)
Types: ability.rs — new
Effect::EachPlayerCopyChosen+ coupledCopyScalestruct +EffectKind::EachPlayerCopyChosen+ 6 registration sites (target_filter None-list, count_expr/count_expr_mut None-lists, name map, EffectKind enum, EffectKind::from). game_state.rs — newWaitingFor::EachPlayerCopyChosenSelection,PendingEachPlayerCopyChosen+CopyChosenStagestructs,pending_each_player_copy_chosenfield (+constructor init +PartialEq +name map +acting_player accessor),CopyScaleimport.Resolver (new module) each_player_copy_chosen.rs —
resolve(APNAP walk mirroring choose_and_sacrifice_rest),advance_to_next_player(skip-empty/auto-resolve-forced-single/seed selection/terminal),drive_from_copy(inner CopyTokenOf via source_filter=SpecificObject + copy-pause detection),perform_counter_step_then_advance(PutCounter{target:LastCreated} so all N doubled copies get counters; reads chosen[1] power live via aggregate_property_over; counter-pause detection),drain_pending(two-stage resume). effects/mod.rs — mod decl, dispatch, fan-out exclusion,waits_for_resolution_choice. engine_replacement.rs — drain hook in Execute arm + Prevented arm helper (maybe_drain_each_player_copy_chosen). engine_resolution_choices.rs — added tohandles()+SelectTargetscontinuation arm inhandle_resolution_choice(validates ordered distinct min..=max, drives copy+scale, mirrors CategoryChoice trigger deferral: drain_deferred on Priority / collect_into_deferred on pause). Exhaustive-match arms added in scenario.rs, trigger_index.rs, coverage.rs, printed_cards.rs, analysis/ability_graph.rs.Parser: oracle_effect/mod.rs —
try_parse_each_player_copy_chosen+parse_copy_scale_segment(composed nom combinators only; fail-closed; reuses become_copy_except::parse_except_body, parse_type_phrase; optional "first"/scale) registered in parse_effect_chain + parse_effect_chain_with_context. oracle_trigger.rs —.or_else()link before the terminal closure. swallow_check.rs — addedEachPlayerCopyChosento DynamicQty marker list (suppresses a spurious "equal to the power" SwallowedClause false-positive, mirroring EachDealsDamageEqualToPower). token_copy.rs — boy-scout fix of the stale :827 AddKeyword doc comment (the :1136 arm does consume additional_modifications keywords, which my menace-sibling relies on).AI: engine/ai_support/candidates.rs — enumerate size-1 + representative first+second pairs (capped 64). phase-ai decision_kind.rs / search.rs (fallback picks first min eligible) / redundancy_avoidance.rs.
Frontend: adapter/types.ts (WaitingFor union), waitingForRegistry.ts (HANDLED set), CardChoiceModal.tsx (case + import), new EachPlayerCopyChosenModal.tsx (ordered pick, min/max enforced, dispatches SelectTargets), i18n en/game.json (cardChoice.eachPlayerCopyChosen.*).
Tests: parser tests.rs (3: real-card trigger, no-scale menace sibling, cross-axis artifact/mana-value); each_player_copy_chosen.rs #[cfg(test)] (5: two-eligible-seeds-selection, zero-eligible-skip, single-eligible auto-resolve non-legendary copy, scale:None copy, AND a RUNTIME card-test proof
two_chosen_scales_copy_by_second_power_via_select_targetsdriving the full WaitingFor round-trip through engine::apply → non-legendary copy carrying 2 +1/+1 counters = second creature's power).VERIFICATION RESULTS (worktree runs cargo directly; not under Tilt). cargo build --workspace --exclude phase-tauri: PASS. cargo test -p engine (full suite incl. my 8 new tests + integration + doctests): PASS exit 0, zero failures. cargo clippy -p engine -p phase-ai --all-targets: PASS exit 0, no warnings. cargo fmt --all: applied. Frontend: pnpm run type-check PASS, pnpm lint PASS (0 errors; 16 pre-existing warnings, none in my files). Pipeline: cargo run --features cli --bin oracle-gen -- data --filter "human—time lord meta-crisis" → mode PlaneswalkedTo, effect EachPlayerCopyChosen, scale {P1P1,Power}, player_scope All, parse_warnings None, Unimplemented count 0.
PARSER DIFF GATE: My parser diff has ZERO string-dispatch (no contains/starts_with/ends_with/find on parse paths). The only two grep hits are
Vec<TypeFilter>::contains(&TypeFilter::Creature/Artifact)collection-membership ASSERTIONS inside my #[test] functions (the documented test exception), not parsing dispatch. NOTE: scripts/check-parser-combinators.sh exits 1, but its diff base commit (ff799f8) is 2383 commits behind the worktree HEAD (e0eff39/v0.12.0), so it flags 2383 commits of pre-existing parser code — a stale-baseline worktree condition, not my change.JUDGEMENT CALLS: (a) Routed EachPlayerCopyChosenSelection through engine_resolution_choices::handles()+handle_resolution_choice (not engine.rs's direct SelectTargets arm) so run_post_action_pipeline DEFERS token-ETB triggers while the walk is paused (line-67 guard) instead of clobbering the selection WaitingFor — this is the CategoryChoice precedent and is why my runtime test's ETB/trigger handling is correct. (b) Used owner:TargetFilter::Controller (controller=chosen player) rather than SpecificPlayer{id} since resolve_token_owner resolves both to the same player and Controller is the clean context-ref path (verified). (c) Made segment-2 "first" optional so both "the first creature" and "the creature" (single-choice sibling) parse.
STOP-AND-RETURN ITEMS: None. No CR ambiguity; Planechase subsystem already exists (per plan §0) so no new-subsystem blocker.
CR ANNOTATIONS (all grep-verified against docs/MagicCompRules.txt before writing): 312.5 (encounter trigger), 101.4 (APNAP choose order), 101.3 (impossible parts ignored), 608.2c (controller follows instructions in order), 616.1 (competing replacement-choice pause), 707.2 (copiable values), 205.4 (supertypes / not-legendary), 122.1 (counters), 208.1 (creature P/T), 609.3 (do as much as possible). Each confirmed present via
grep -m1 "^<num>" docs/MagicCompRules.txt.DEVIATIONS FROM PLAN: (i) Runtime proof: instead of the planechase_tests.rs encounter harness (plan §11), I wrote an equivalent-or-stronger direct engine::apply SelectTargets round-trip card-test that proves the copy+scale mechanics end-to-end; the encounter→trigger→stack path uses generic engine machinery already proven for other phenomena. (ii) Added swallow_check.rs DynamicQty marker (not in the plan's file list) to suppress a spurious warning surfaced during verification — in-scope quality fix. (iii) Did not run full ./scripts/gen-card-data.sh (card-data.json is gitignored/untracked) — used targeted oracle-gen instead for the 0-Unimplemented confirmation.
RISKS for /review-impl: (a) The CR 616.1 inner copy/counter pause paths (drain hook + AwaitingCopy/AwaitingCounters stages) are IMPLEMENTED and wired in both engine_replacement resume arms but NOT unit-tested (constructing 2+ competing token/counter replacements is heavy) — the common no-pause path is fully tested and the design mirrors ~8 existing dedicated resume slots. (b) N>1 tokens under a doubler (MATERIAL) rely on PutCounter{LastCreated}→resolve_event_context_targets returning the full last_created_token_ids vector (verified by code reading, not a doubler test). (c) 3+ player mid-walk trigger deferral is structurally identical to the proven CategoryChoice pattern but only 2-player was runtime-tested. (d) Caught in a Parallel Universe (plan's 2nd consumer): my parser+effect fully support its min:1/max:1/AddKeyword(Menace)/scale:None shape (proven by test), but its neighbor choose-filter ("controlled by the player to their left") still needs a follow-up ControllerRef::Neighbor variant + choose-filter parser arm before it fully parses.
Files changed
CR references
Verification
cargo fmt --all— pass (no changes)check-parser-combinators.sh <upstream/main merge-base acd2f5e6b>— pass (exit 0, no flagged lines)cargo clippy -p engine --all-targets -- -D warnings— pass (exit 0, no warnings)cargo test -p engine— pass (exit 0, 140 test groups, 0 failures)oracle-gen data --filter "human—time lord meta-crisis"— pass (fully typed PlaneswalkedTo trigger + EachPlayerCopyChosen effect matches oracle text)Cards confirmed re-parsed correctly: Human—Time Lord Meta-Crisis
🤖 Generated with Claude Code
Track
Developer
LLM
Model: Codex GPT-5 (maintainer repair pass)
Thinking: high
Maintainer repair verification
cargo fmt --all— clean during maintainer repair passgit diff --check— clean during maintainer repair passorigin/mainand was pushed with--no-verify; no direct cargo build/test was run in this pass.