Fix Mishra copy-token ETB replacements and applied keys#5475
Conversation
github-actions
Bot
added
the
needs-maintainer
![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 introduces 'liminal' token objects to handle complex copy-token scenarios where choices must be made before a token is committed to the battlefield, and transitions the replacement pipeline's applied tracking from ReplacementId to AppliedReplacementKey to keep different replacement types distinct. The review feedback highlights a critical bug where token initialization is skipped if counter placement pauses, a violation of Rule R2 due to the use of a bool field (enters_attacking) on LiminalEntry, and violations of Rule R6 due to missing CR annotations in token_copy.rs and replacement.rs.
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 name: String, | ||
| pub source_id: ObjectId, | ||
| pub controller: PlayerId, | ||
| pub enters_attacking: bool, |
There was a problem hiding this comment.
[MEDIUM] Avoid bool fields on structs. Evidence:
crates/engine/src/types/game_state.rs:6386.
Why it matters: Rule R2 of the style guide prohibits usingboolfields on structs, requiring typed enums to better express the design space.
Suggested fix: Replaceenters_attacking: boolwith a typed enum likeCombatEntryState.
References
- Rule R2: No bool fields — parameterize with existing typed enums. A bool field never expresses the design space; the project uses typed enums instead. (link)
| }) | ||
| } | ||
|
|
||
| pub(crate) fn apply_immediate_copy_token_modifications_to_object( |
There was a problem hiding this comment.
[MEDIUM] Missing CR annotation for copiable characteristics modifications. Evidence:
crates/engine/src/game/effects/token_copy.rs:1325.
Why it matters: Rule R6 of the style guide requires every rules-touching line of engine code to carry a verifiedCR <number>: <description>comment.
Suggested fix: Add a comment citing CR 707.2 for copiable characteristics.
| pub(crate) fn apply_immediate_copy_token_modifications_to_object( | |
| /// CR 707.2: When copying an object, the copy acquires the copiable values of the original object's characteristics. | |
| pub(crate) fn apply_immediate_copy_token_modifications_to_object( |
References
- Rule R6: CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| { | ||
| return false; | ||
| } | ||
| if is_liminal_source { |
There was a problem hiding this comment.
[MEDIUM] Missing CR annotation for liminal replacement candidates. Evidence:
crates/engine/src/game/replacement.rs:4738.
Why it matters: Rule R6 of the style guide requires every rules-touching line of engine code to carry a verifiedCR <number>: <description>comment.
Suggested fix: Add a comment citing CR 614.12a for replacement effects applying to entering objects.
| if is_liminal_source { | |
| if is_liminal_source { | |
| // CR 614.12a: Some replacement effects apply to an object entering the battlefield from another zone. | |
| if rid.source == ObjectId(0) |
References
- Rule R6: CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . (link)
Parse changes introduced by this PR · 7 card(s), 4 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed at head e35991cb. All required checks green. Parse-diff sticky reports no_changes, consistent with a non-parser engine change.
Seam: correct. For a token copy's own as-enters replacement to fire, the token must exist before it reaches the battlefield and route through the replacement pipeline. ProposedEvent::TokenEntry + liminal (uncommitted) token objects threaded through replacement.rs / engine_replacement.rs / token_copy.rs is the layer that owns replacement effects, and CR 614.12a is the right rule. This is not a symptom patch at an effect handler. The mishra_shaped_copy_token_surfaces_liminal_enter_as_copy_choice test parses real Oracle text, drives apply_as_current(ChooseReplacement) → apply_as_current(ChooseTarget), and asserts the liminal token is absent from state.objects until the choice commits — discriminating and revert-failing. token_entry_matches_liminal_self_moved_replacement_only pairs a positive with a genuinely non-vacuous negative. Good work.
One blocking finding.
HIGH — counter pause in commit_liminal_token_entry_with_event_emission leaves an irrecoverable half-initialized permanent
crates/engine/src/game/effects/token.rs:1210-1235
let Some(mut entry) = state.liminal_entries.remove(&entry_ref) else { return true; }; // 1210: entry moved into a local
entry.object.tapped = enter_tapped.resolve(entry.object.tapped);
let owner = entry.object.owner;
state.objects.insert(entry_ref, entry.object); // 1215: now on the battlefield
zones::add_to_zone(state, entry_ref, Zone::Battlefield, owner);
for (counter_type, counter_count) in enter_with_counters.iter().chain(entry.enter_with_counters.iter()) {
if *counter_count > 0
&& !super::counters::add_counter_with_replacement(state, owner, entry_ref, counter_type.clone(), *counter_count, events)
{
state.last_created_token_ids = entry.created_ids.clone();
return false; // 1233: bare return; `entry` drops here
}
}If add_counter_with_replacement parks a WaitingFor (returns false), the function bare-returns. entry was already remove()d from state.liminal_entries into a local, so it is dropped and unrecoverable — while the object is already inserted into state.objects and added to the battlefield at 1215-1216.
Everything below the loop is skipped: inject_predefined_token_abilities / inject_resolved_token_abilities (1239/1241), mark_layers_entered (1243), record_battlefield_entry (1244), record_token_created (1245), enters_attacking (1247), attach_to (1250), push_committed_token_entry_events (1262), and the sacrifice_at UntilEndOfCombat delayed trigger (1270-1288).
Result: a permanent sits on the battlefield with no abilities, no layer registration, no battlefield-entry record, no entry events, and no sacrifice-at delayed trigger — and nothing can ever finish it, because its LiminalEntry is gone. For a Mishra-created token copy with sacrifice_at: UntilEndOfCombat, the token also never gets sacrificed.
The asymmetry is the tell. The caller already knows this hazard exists — it just guards the wrong pause site. In crates/engine/src/game/engine_replacement.rs:
if !super::effects::token::commit_liminal_token_entry_with_event_emission(state, resume.event, events, false) {
return Ok(state.waiting_for.clone()); // 1164: pause here → no post-actions registered, entry already dropped
}
...
let mut counter_pause_post_actions = Vec::new(); // 1182: built AFTER the call above
counter_pause_post_actions.push(PendingCounterPostAction::EmitCommittedCopyTokenEntry { .. }); // 1185
counter_pause_post_actions.push(PendingCounterPostAction::ContinueCopyTokenCreation { .. }); // 1195
finish_copy_target_choice_entry(state, source_id, events, counter_pause_post_actions)? // 1204: only THIS pause resumesEmitCommittedCopyTokenEntry / ContinueCopyTokenCreation are constructed at 1185/1195 — after the commit call at 1158 — so they only cover a counter pause inside finish_copy_target_choice_entry at 1204. A pause originating inside commit_liminal_token_entry_with_event_emission at 1158 returns at 1164 with no resume registered. The same applies to the other caller, token.rs:1105.
Reachability: apply_single_replacement extends ProposedEvent::TokenEntry { enter_with_counters, .. } with modifiers.etb_counters, so a replacement can populate the counters a liminal copy token enters with. If placing one of those counters then hits an optional/choice counter-replacement, the pause fires. Narrow, but real — and it corrupts authoritative state rather than failing loudly.
Suggested fix: mirror the existing pattern. Before the counter loop can pause, register the continuation — either re-insert entry into state.liminal_entries and resume via a PendingLiminalEntryResume, or push EmitCommittedCopyTokenEntry + the remaining-init post-actions through append_pending_counter_post_actions so the skipped initialization completes on resume.
Test gap: none of mishra_shaped_copy_token_surfaces_liminal_enter_as_copy_choice, token_entry_matches_liminal_self_moved_replacement_only, or liminal_entry_state_serializes_but_is_filtered_from_viewers drives the pause arm — every fixture enters with an empty or non-pausing enter_with_counters, so only the no-pause shortcut arm runs. Please add a fixture whose liminal TokenEntry.enter_with_counters triggers a choice-bearing counter replacement, asserting the token ends up fully initialized (abilities injected, entry event emitted, sacrifice-at trigger registered) after the choice resolves. That test should fail on the current code.
LOW — missing CR annotations on new rules-bearing helpers
token_copy.rs (apply_immediate_copy_token_modifications_to_object, copy_token_modifications_are_liminal_immediate) and the is_liminal_source guard in replacement.rs are rules-bearing but carry no // CR comment. Per CLAUDE.md, annotation is required, not optional. CR 707.2 (copiable values) and CR 614.12a (self-affecting as-enters replacements) are the relevant rules.
Refuting one bot finding
The Gemini review flags LiminalEntry.enters_attacking: bool under the no-bool-flags rule. Not actionable here. enters_attacking: bool is the established shape across TokenSpec, CopyTokenSpec, and PendingCopyTokenBatch, with 30+ existing sites. Converting this single instance to a typed enum would make it inconsistent with its own sibling structs. If the enters_attacking family is migrated, migrate it together — as its own PR.
Gemini's two CR-annotation comments are folded into the LOW above. Its CRITICAL is the HIGH finding, confirmed against this head.
Scope note (not blocking)
The ReplacementId → AppliedReplacementKey migration (splitting the ObjectId(0) sentinel into distinct Floating / StepEndMana variants) is a real, coherent, well-tested improvement with a legacy-compat deserializer — but it is separable from the Mishra fix, and the Object-keyed liminal tokens don't strictly need it. Every one of the 19 touched files is justified, so this isn't drive-by churn; the title just materially undersells a workspace-wide type migration. Please call it out in the PR description so the blast radius is legible to the next reader.
parthmishra
changed the title
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
The HIGH bug from my last review is fixed, and fixed properly. Thank you for the rework.
The blocker is resolved
Previously the counter-pause path in commit_liminal_token_entry_with_event_emission dropped the LiminalEntry and skipped token init. On this head the ordering is right: state.objects.insert(entry_ref, entry.object) and zones::add_to_zone(...) both run before the counter loop, so a pause from add_counter_with_replacement can no longer strand the entry. The remaining counters are stashed as PendingCounterAddition::Object, and finalization is queued as the first post-action so entry events and token init still fire on resume. state.last_created_token_ids is restored on the pause path too. That's the shape I was hoping for.
MED — a bool flag, where this PR's own enum is the right type
CLAUDE.md is explicit that a boolean isn't composable and that an existing typed enum should be used instead. This PR introduces exactly that enum in types/game_state.rs:
pub enum TokenEntryEventEmission {
Emit,
Suppress,
}and then exposes bool on both pub(crate) entry points anyway, converting at the top of the callee:
pub(crate) fn commit_liminal_token_entry_with_event_emission(
…
emit_entry_events: bool, // <-- here
) -> bool
let finalization = liminal_token_entry_finalization_action(
entry_ref,
&entry,
if emit_entry_events { TokenEntryEventEmission::Emit }
else { TokenEntryEventEmission::Suppress }, // <-- and immediately undone
);Downstream, liminal_token_entry_finalization_action already takes entry_events: TokenEntryEventEmission and matches on it. So the bool exists only to be translated back into the enum one frame later, and at the call site it reads as a bare literal:
commit_liminal_token_entry_with_post_actions(state, event, events, true, post_actions)Nothing at that call site says what true means. Please thread TokenEntryEventEmission through both commit_liminal_token_entry_* signatures and delete the if/else. It's a mechanical change and it makes the call sites self-documenting. You clearly know this — LiminalTokenAbilityInjection sits right next to it in the same enum block and gets it right.
Scope note — I have not reviewed the whole diff
I want to be straight with you rather than imply a pass I didn't do. This is +1942/-173 across 19 files, and the bulk of it is core replacement machinery:
| file | change |
|---|---|
game/effects/token.rs |
+557/-8 |
game/effects/token_copy.rs |
+441/-5 |
game/replacement.rs |
+264/-73 |
game/engine_replacement.rs |
+240/-15 |
types/proposed_event.rs |
+228/-37 |
I verified the counter-pause fix and read the new enum block; I have not yet reviewed the replacement-effect and ProposedEvent reshaping, and I'm not going to rubber-stamp it at the tail of a sweep. Fix the bool and I'll give the replacement machinery a dedicated pass — that's the part that decides this PR, and it deserves undivided attention.
CI is green on d3b5ef50, which is a good sign but not a substitute for that read.
|
Addressed the latest requested change in |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the rework — both prior blockers are resolved on this head:
- Counter-pause HIGH (fixed). The liminal-token finalize path now inserts the object and
zones::add_to_zone(..., Battlefield, ...)before the counter loop, and the loop capturescounters_to_apply[counter_index + 1..]intoContinueLiminalCopyTokenBatchso a mid-loop pause resumes cleanly instead of dropping the entry. Correct ordering. - Bool-flag MED (fixed).
emit_entry_events: boolis gone; the typedTokenEntryEventEmission { Emit, Suppress }now threads through the entry paths and is matched withmatches!(entry_events, TokenEntryEventEmission::Emit). Good — that's the parameterize-don't-proliferate call.
One nit to fold into your next push: TokenEntryEventEmission has no doc comment. Every neighboring enum in this module carries a /// explaining its variants (and a CR cite where a rule is at stake) — please add a one-liner on the enum saying when Suppress is used (copy-token batch members that emit their entry event once at batch finalize, if I'm reading it right).
Holding formal approval only because the full replacement re-keying (AppliedReplacementKey::object(...) in proposed_event.rs) and the token_copy.rs batch logic deserve a dedicated review pass beyond the two blockers above — that's on me, not a request for changes. CI is still running; once it's green I'll do the machinery pass. No action needed from you except the doc-comment nit.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Both prior blockers are resolved and all 13 required checks are green on head e300decfe2 (Rust lint, both test shards, card-data coverage gate, WASM, Tauri, Frontend, Lobby all success; CLEAN/MERGEABLE).
- Counter-pause HIGH (fixed): the liminal-token finalize path inserts the object and
zones::add_to_zone(..., Battlefield, ...)before the counter loop, and capturescounters_to_apply[counter_index + 1..]so a mid-loop ETB-counter pause resumes exactly once. - Bool-flag MED (fixed):
emit_entry_events: boolis now the typedTokenEntryEventEmission { Emit, Suppress }, threaded through the commit APIs with explicit variants at call sites. - Doc-comment nit addressed: the new enum is CR-annotated
603.6a + 111.1(both grep-verified).
Dedicated pass over the 1940-line machinery (AppliedReplacementKey re-keying, drive_copy_token_batches → finalize_copied_token) is clean, and the once-only resume is guarded by the discriminating regression test paused_copy_token_add_counter_on_enter_preserves_remaining_batch. Approving and enqueuing. Thank you for the careful rework.
matthewevans
added
bug
2 participants
Summary
Fixes Mishra, Eminent One copy tokens for artifact copies that enter as a copy of something else, including Cursed Mirror / Machine God's Effigy class effects.
This PR also includes the applied-replacement identity migration needed to make the liminal token-entry path safe across object-backed, floating, and step-end mana replacements.
Review update: fixes the counter-pause finalization bug found at head
e35991cbby registering a pending liminal-entry finalization before any ETB-counter replacement can pause, then resuming copy-token batch draining after the counter choice completes.Review update 2: fixes the requested enum-threading change from head
d3b5ef50.TokenEntryEventEmissionnow threads throughcommit_liminal_token_entry_with_event_emissionandcommit_liminal_token_entry_with_post_actions, and call sites pass explicitEmit/Suppressvariants instead of bool literals. I also re-reviewedtoken.rs,token_copy.rs,replacement.rs,engine_replacement.rs, andproposed_event.rsagainstAI-CONTRIBUTOR.md's bool/enums, seam, applied-key, and runtime-test guidance.Merged latest
origin/mainthrough2f8e17bb9before this update.Closes #5450.
Files changed
CR references
Track
Developer
LLM
Tier: Frontier
Model: codex-5.5
Thinking: high
Verification
cargo fmt --all- clean./scripts/check-parser-combinators.sh- clean (exit 0; no stdout/stderr)cargo test -p engine paused_liminal_copy_token_counter_finalizes_entry_after_choice --no-fail-fast- cleancargo test -p engine mishra_shaped_copy_token_surfaces_liminal_enter_as_copy_choice --no-fail-fast- cleancargo test -p engine token_entry_matches_liminal_self_moved_replacement_only --no-fail-fast- cleancargo test -p engine proposed_event --no-fail-fast- cleancargo clippy-strict- cleancargo test -p engine --no-fail-fast- clean, 0 failures; engine lib and integration/doc-test suites completed successfully[rust] cargo fmt --check[rust] cargo clippy[rust] card-data-validate release check[rust] parser combinator gate[rust] engine parser tests(550 passed)[rust] phase-ai lib tests(1264 passed, 8 ignored)[cards] oracle-gen[cards] card-data-validate[cards] coverage-report[cards] coverage regression check(baseline 31478/current 31478; 0 engine regressions, 0 coverage-honesty regressions)[client] pnpm lintbecause local ignored generated Tauri build output underclient/src-tauri/targetwas present andeslint .traversed those generated assets. Removed the ignored local build output; no tracked files changed.pnpm --dir client lint- exit 0, warnings onlypnpm --dir client run type-check- cleanScope Expansion
Introduced reusable liminal token-entry infrastructure so token-copy objects can exist for replacement matching before they are committed to the battlefield, plus typed applied-replacement keys to keep object, floating, and step-end mana replacement identity distinct.
Validation Failures
None.
CI Failures
None.
Gate A
Command:
./scripts/check-parser-combinators.shExit code: 0; the script produced no stdout/stderr.
Anchored on
CreateTokenevents into normal vs copy-token creation paths.