fix(engine): settle the answered copy-target choice so liminal copy-token ETB observers fire - #7020
Conversation
…oken ETB observers fire
A token entering the battlefield as a copy via a mid-entry choice (Embalm-as-copy)
was observed by zero enters-the-battlefield abilities.
`handle_copy_target_choice`'s liminal-resume branch never cleared the
already-answered `CopyTargetChoice`, so both of its tails echoed the spent prompt
via `if !Priority { return waiting_for }`. The action therefore never settled,
`apply_action` skipped `run_post_action_pipeline`, and the CR 603.6a entry pair
flushed by `finish_copy_target_choice_entry` never reached a priority boundary to
be scanned. The client also re-answered the spent prompt — the stale second
`CopyTargetChoice` disclosed in `token_zone_change_index.rs`, which is why that
route's reach-guard could only assert a `[..2]` slice of its prompt vector.
CR 614.12a makes the choice a single pre-entry event, so the prompt that asked for
it is spent; CR 603.3 owes the triggered abilities the priority boundary the echo
denied them. The clear is placed above the Token/Meld split because both
sub-branches share the echo tail, and mirrors three existing clears: this
handler's own non-liminal tail, `handle_persist_chosen_attribute_choice`, and
`effects::token::continue_liminal_copy_token_batch`.
Diagnosis note, because the obvious explanation was wrong: the deferred-entry
capture is empty on this route, but that is not the cause. It is empty on every
liminal copy-token route, including three sibling routes whose observers fire
correctly. A condition present in both the failing and the passing arms is not the
differentiator — settlement is. Instrumenting all nine exits of the branch over
the full suite gives 16 entries and 16 exits in both arms, with the four exits
carrying a genuine pause byte-identical; only the twelve that returned the spent
prompt now return `Priority`.
Tests: new `liminal_copy_token_etb_observers` suite covering the fired-exactly-once
row, the multi-observer ordering prompt, an opponent-controlled observer, and
unchanged controls for the permanent-spell copy route, a plain creature entry, and
a non-entry copy choice. Includes a premise-break arm that stages the observer
without its Oracle text and asserts every delta row — both controls included —
reads zero while the entry pins still hold, so no row can pass while reading
something other than the observer.
Assisted-by: ClaudeCode:claude-opus-5
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe engine now clears the consumed ChangesLiminal copy-token battlefield entry
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
There was a problem hiding this comment.
Approved. Current head 33879bf clears the consumed copy-target choice before both liminal completion tails; the current-head runtime suite exercises Embalm copy entry, ETB observer/order/controller behavior, and the prior stale-prompt regression. The SHA-bound parse artifact reports no parse changes.
🤖 AI text below 🤖
Summary
A token that enters the battlefield as a copy via a mid-entry choice (Embalm-as-copy is the live instance) was observed by zero enters-the-battlefield abilities.
handle_copy_target_choice's liminal-resume branch never cleared the already-answeredCopyTargetChoice, so both of its tails echoed the spent prompt, the action never settled toWaitingFor::Priority,apply_actionskippedrun_post_action_pipeline— and that pipeline is the only CR 603.6a trigger scan on this route. One statement clears the spent prompt; observers now fire exactly once.Predecessor context: this is the follow-up lane to #7012 (which fused battlefield-entry record+emit into one authority). #7012 moved where the entry pair is recorded and emitted; this PR fixes when the action carrying it settles, which is what decides whether anything scans it. The two are independent — this defect reproduces identically before and after #7012.
Files changed
crates/engine/src/game/engine_replacement.rs— the fix: clear the answeredCopyTargetChoiceat the top of the liminal-resume branch, above the Token/Meld split.crates/engine/tests/integration/liminal_copy_token_etb_observers.rs— new suite (7 tests) covering fired-exactly-once, multi-observer ordering, opponent-controlled observer, three unchanged controls, and a premise-break arm.crates/engine/tests/integration/main.rs—modregistration for the new suite.crates/engine/tests/integration/token_zone_change_index.rs— corrected a now-false doc claim, and tightened adrive.prompts[..2]slice assertion to the whole vector (that slice existed only to accommodate the stale prompt this PR removes).Track
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
Disclosed deviation: the flow used was planner subagent → plan document →
engine-implementation-executor→ independent adversarial review →review-implat the committed head. The canonical checkpoint-mode receipt artifact was not generated, so this is the skill's agent flow without its receipt. Stated rather than claimed clean.CR references
Added on the fix:
Touched in test documentation: CR 400.7, CR 603.2c, CR 603.3b, CR 603.6a, CR 608.2i, CR 616.1, CR 702.49c, CR 704.5f, CR 707.2c.
Every number above was verified two-step (resolves in
docs/MagicCompRules.txtand its text supports the site).CR 608.2iis a soft over-citation in a test doc — it describes the purpose of the ledger the assertion reads rather than a rule the test implements. Disclosed, left in place.Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo test -p phase-engine— 18514 lib + 12 + 9 + 4550 integration passed, 0 failedcargo test -p phase-engine --test integration liminal_copy_token— 11 passed, 0 failed, re-run at the rebased head and again at the committed headcargo test -p phase-ai— 2106 passed, 0 failedcargo clippy --workspace --all-targets -- -D warnings— rc=0cargo fmt --all --check— rc=0./scripts/check-parser-combinators.sh— Gate G PASS, Gate A PASS, Gate P PASSMutant evidence (per-assertion, two-sided, flipping different rows):
drive.promptsreturns to 3 entries with the stale third; controls and every premise-break row stay green.ZoneChangedre-parked intodeferred_entry_events) — 2 failures, firing the life-magnitude assertions (L1 and L3 each read 2 instead of 1); every prompt guard and both controls stay green.p1_break_every_delta_row_reads_zero, shipped in the suite) — with the observer staged without its Oracle text, every delta row reads 0 including both controls, while the entry pins still hold. No row can pass while reading something other than the observer.Exit-path enumeration for the fix's safety claim — all 9 exits of the edited branch instrumented over the full suite, both arms: 16 entries → 16 exits in both (equal counts prove no error return escaped). The 4 exits carrying a genuine pause are byte-identical with and without the fix; only the 12 that returned the spent prompt now return
Priority. No path that owed a player prompt returns one fewer.Honest residual: three
matches!(state.waiting_for, Priority)gates insidedrain_pending_continuationchange from skipped to run, each additionally gated on a frame that is absent on this route. No fixture reaches them, so this is recorded as unverified, not verified-safe.Gate A
Gate A PASS head=33879bfcde64fb4dcc78d0741140e2feba41ed87 base=4b34e5465eafa94bcd49dfe1a9275968be4300dc
Note: the gate resolves its base fork-relatively, so the printed
baseis older than this branch's actual base (acfeaf0786c7f1b39d0226ed965981348a2a5dd3). The examined range is therefore a superset of this diff — conservative, not vacuous. Reported exactly as the script printed it. This diff contains no parser changes.Anchored on
crates/engine/src/game/engine_replacement.rs:1576— the same clear insidehandle_persist_chosen_attribute_choice, which answers a sibling mid-entry choice on the same handler family.crates/engine/src/game/engine_replacement.rs:1958— this same handler's own non-liminal tail, which already clears the prompt before settling.crates/engine/src/game/effects/token.rs:1446—continue_liminal_copy_token_batchopens by settingPriorityin exactly this shape.(Line numbers generated by
grep -n/awkagainst this branch's tree at the committed head.)Stamp
Per-figure predicates: life deltas are
runner.life(seat)after minus before, same test; prompt counts are prompts raised and answered by arunner.actcall — not instrument lines; observers are constructed withadd_creature_from_oracle(a graft-based observer measures a different quantity and is not used here).Final review-impl
Final review-impl PASS head=33879bfcde64fb4dcc78d0741140e2feba41ed87
Review lineage, disclosed because the first pass did not pass.
review-implround 1 (headdb547bb9b) returned FAIL with three MED findings, none in the engine change: a proposed pinning test that was physically impossible (it named ninjutsu, which never creates a token) with an unsupporting CR citation; a reachability claim ("reached by no fixture") that a probe disproved; and a vacuous assertion in the premise-break arm, labelled as an axis but run on a board whose live counterpart reads the same value. All three were in prose or test-labelling written after the substantive review, and all three were fixed. Round 2 (this head) re-verified the fixes, hunted for siblings of both defect classes, re-checked all 8 cards against Scryfall and every CR citation, independently reproduced the MUTANT-DROP signature, and returned PASS with zero findings.Round 2 also measured a structural prediction of its own rather than reporting it: it expected the "nothing is stranded" claim to be impossible from code-reading, built a positive-controlled probe, and found round 1 correct and itself wrong. Recorded because it is the reason that claim is trustworthy.
Claimed parse impact
None.
Scope Expansion
One, ratified: the
drive.prompts[..2]slice assertion intoken_zone_change_index.rswas tightened to the whole vector. That slice existed only as an accommodation of the stale prompt this PR removes — of the 6 route tests in that file assertingdrive.prompts, it was the only one asserting a slice rather than the whole vector, which is the tell that led to this root cause. Closing it is in-scope; the new suite's ordering-prompt assertion inherits its diagnostic role.Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
Bug Fixes
Tests