fix(engine): attach token copies of Auras to a host (CR 303.4f) - #7303
Conversation
A token that is a copy of an Aura entered unattached and was swept by the CR 704.5m SBA before the player saw it — no host prompt, no copy. The CR 303.4f consult now runs on both copy-token entry paths, CR 303.4g un-creates a token with no legal host, and a `LastCreated`-gated rider no longer resolves eagerly mid-prompt. Fixes phase-rs#5904. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAura copy-token entry resolves host legality before creation records. Pending host choices preserve entry state and resume finalization. Unhosted Auras follow origin-specific outcomes. Condition traversal and SBA handling support the continuation flow. ChangesAura copy-token entry
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CopyTokenEntry
participant ZonePipeline
participant PendingCounterPostAction
participant CopyTokenFinalization
CopyTokenEntry->>ZonePipeline: discover and resolve Aura host
ZonePipeline-->>CopyTokenEntry: Attached, NoLegalHost, or NeedsChoice
CopyTokenEntry->>PendingCounterPostAction: save entry tail when choice is pending
PendingCounterPostAction->>CopyTokenFinalization: resume after host choice
CopyTokenFinalization-->>CopyTokenEntry: finalize entry and publish created IDs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/engine_replacement.rs (1)
2103-2141: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve the liminal copy continuation across the Aura-host prompt.
Lines 2103-2111 identify that
NeedsChoicereturnsOk(Some(..))beforehandle_copy_target_choiceruns itscopy_continuationtail. A future liminal copy token that realizes as a multi-host Aura will skip remaining token creation and created-ID publication after the player selects a host.Store and resume this tail with the
ReturnAsAuraTargetcontinuation. Do not rely on the current card pool to make this branch safe.As per path instructions, “A latent bug behind a guard or unreached branch is still a finding.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/engine_replacement.rs` around lines 2103 - 2141, Preserve the liminal copy path’s copy_continuation and committed-token-entry tail when resolve_entering_aura_attachment returns NeedsChoice. Extend ReturnAsAuraTarget’s stored continuation state so handle_copy_target_choice resumes that tail after the player selects a legal host, including remaining token creation and created-ID publication. Keep the existing behavior for non-choice attachment outcomes unchanged.Source: Path instructions
🔇 Additional comments (9)
crates/engine/src/game/zone_pipeline.rs (1)
1921-1943: LGTM!Also applies to: 1959-2053, 2055-2240
crates/engine/src/types/game_state.rs (1)
5311-5332: LGTM!Also applies to: 5397-5409
crates/engine/src/game/effects/token_copy.rs (3)
952-973: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
⚠️ Unverified finding
Sandbox verification was unavailable.The modification pause on this route can still drop
etb_counters.
ContinueCopyTokenEntryAfterAuraHostwas added precisely becauseApplyCopyTokenModificationsAndFinalizeresumes after ETB counters are applied and therefore skips them. Theapply_token_modificationspause at Line 952 has the same shape as the Aura-host pause: it happens BEFORE theetb_countersloop at Line 980, and it parks onlyContinueCopyTokenCreationfor the rest of the batch. Ifapply_token_modificationsinternally stashesApplyCopyTokenModificationsAndFinalize, this token's CR 306.5b copied loyalty and CR 614.1c "enters with N counters" self-replacements are lost on resume.This route is exactly the one with non-empty
etb_counters:liminal_immediateat Line 529 requiresetb_counters.is_empty(), so every copy that reaches Line 952 has counters to seed.The condition depends on what
apply_token_modificationsparks, which is not in the provided context. Confirm the continuation, and if it resumes throughApplyCopyTokenModificationsAndFinalize, carry the remainingetb_countersthrough the sameCopyTokenEntryTailthis PR introduced.
704-722: 🗄️ Data Integrity & Integration | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that
uncreate_unentered_aura_tokenreverses everythingcreate_objectrecorded.On this route the token was minted by
zones::create_objectat Line 651, not by the liminal insert path.create_objectmay write more thanstate.objectsandstate.battlefield— for example a CR 400.7zone_changes_this_turnrow or abattlefield_entries_this_turnrow. The CR 303.4g arm claims "no battlefieldZoneChanged, nocreated_idsrow, and nothing in any graveyard", but the integration testnon_liminal_copy_with_no_legal_host_is_not_createdasserts onlyobjects.len(),TokenCreated, the battlefield list, and the graveyard.If
create_objectwrites an entry-history row, a CR 608.2i look-back condition such as "you had a creature enter this turn" would observe a token that CR 303.4g says was never created.Confirm the rollback covers those ledgers, and add the assertion to the test if it does not.
747-816: LGTM!Also applies to: 881-915, 1065-1124
crates/engine/src/game/effects/counters.rs (1)
689-702: LGTM!crates/engine/tests/integration/yenna_aura_token_copy.rs (2)
999-1054: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
⚠️ Unverified finding
Sandbox verification was unavailable.Two paths through the new resume state have no coverage.
The suite is strong: every negative has a paired positive reach-guard, and
no_legal_host_means_the_token_is_not_createdseeds a stalelast_created_token_idsso the "not created" verdict cannot pass vacuously. Two gaps remain, both specific to code this PR added.First, the non-liminal batch tail.
continue_copy_token_entry_after_aura_hostcallsapply_copy_token_after_replacementfortail.remaining_countwhen it is non-zero (token_copy.rsLines 1089-1103), and folds the result throughextend_copy_batch_created_ids. This test usescount: 1, soremaining_countis always0and that branch never runs.two_copies_each_choose_their_own_hostcovers a two-token batch, but Cooped Up has no entry counters, so it takes the liminal route instead. Add a Fylgja fixture withcount: QuantityExpr::Fixed { value: 2 }and assert both tokens attach to their own chosen host, both carry four healing counters, andlast_created_token_idscontains both ids.Second, the parked tail is serializable.
CopyTokenEntryTailandPendingCounterPostAction::ContinueCopyTokenEntryAfterAuraHostboth deriveSerialize/Deserialize, so an open CR 303.4f host prompt is resumable state that survives a reconnect. The sibling paused continuation incounters.rshas a wire round-trip test (counter_moves_queue_reparks_and_roundtrips_v2_at_replacement_choice). Add the equivalent here: halt at the prompt withHostAnswers::HaltAtPrompt, round-trip the state throughResolutionStateWire, then answer the prompt on the restored state and assert the attach and the four counters both land.
1-59: LGTM!Also applies to: 96-221, 223-339, 341-452, 454-535, 537-583, 585-649, 651-720, 722-796, 798-869, 871-940, 942-997, 1056-1090
crates/engine/tests/integration/main.rs (1)
1040-1040: LGTM!
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/effects/token.rs`:
- Around line 1640-1708: Update LiminalEntry and the liminal entry flow to carry
a pre-entry disposition for card-backed Auras with no legal host. Cancel such
entries before state.objects.insert, zone insertion, token-creation journaling,
or entry events; keep them in their current zone, except send stack-originating
Auras to their owner’s graveyard. Preserve the existing token NotCreated path
and unattached behavior only where CR 303.4g permits it.
In `@crates/engine/src/game/filter.rs`:
- Around line 1514-1522: Make LastCreated dependency traversal exhaustive: in
filter_contains_last_created, recurse through ChosenDamageSource and explicitly
handle every TargetFilter variant, removing the wildcard arm while preserving
recursive handling for Not, Or, And, and TrackedSetFiltered. In
crates/engine/src/game/effects/mod.rs lines 2885-2905, update the quantity-ref
traversal to explicitly enumerate every QuantityRef variant and remove its
wildcard arm so all population-bearing variants reach
quantity_ref_population_filter.
---
Outside diff comments:
In `@crates/engine/src/game/engine_replacement.rs`:
- Around line 2103-2141: Preserve the liminal copy path’s copy_continuation and
committed-token-entry tail when resolve_entering_aura_attachment returns
NeedsChoice. Extend ReturnAsAuraTarget’s stored continuation state so
handle_copy_target_choice resumes that tail after the player selects a legal
host, including remaining token creation and created-ID publication. Keep the
existing behavior for non-choice attachment outcomes unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1baeb70c-3172-4584-b6e0-79db0b46a5d1
📒 Files selected for processing (13)
crates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/effects/token_copy.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/filter.rscrates/engine/src/game/sba.rscrates/engine/src/game/zone_pipeline.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/battlefield_entry_authority_census.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/yenna_aura_token_copy.rs
# Conflicts: # crates/engine/src/game/engine.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the token path is close, but the card-backed liminal Aura no-host case still violates the entry rule.
🔴 Blocker
[MED] Card-backed liminal Auras can enter unattached when there is no legal host. Evidence: crates/engine/src/game/effects/token.rs:1640-1708 selects EnterUnattached for every non-token entrant, inserts it into objects and the battlefield, then accepts NoLegalHost. The grep-verified CR 303.4g says: “If an Aura is entering the battlefield and there is no legal object or player for it to enchant, the Aura remains in its current zone, unless that zone is the stack…” This applies to LiminalEntryKind::Meld (crates/engine/src/types/game_state.rs:13704-13712); it cannot enter and then be left for the CR 704.5m SBA. Why it matters: the game observes an entry that the rule says never occurs. Suggested fix: carry an explicit pre-entry outcome on LiminalEntry, cancel before insertion, journaling, and entry events, with the stack-origin exception to owner graveyard, and add a runtime no-host Meld regression.
🟡 Non-blocking
[LOW] The new LastCreated dependency traversal is not exhaustive. Evidence: crates/engine/src/game/filter.rs:1514-1522 recurses through Not, And/Or, and TrackedSetFiltered, but its wildcard misses TargetFilter::ChosenDamageSource { filter: Some(..) }; crates/engine/src/game/effects/mod.rs:2885-2905 likewise silently classifies unlisted QuantityRef variants. Why it matters: a nested LastCreated can bypass required prompt deferral today, and future population-reading variants can silently do the same. Suggested fix: recurse through the optional ChosenDamageSource filter and explicitly enumerate both known enums so additions require a classification decision.
Recommendation: request changes on this current head. Please fix the pre-entry CR 303.4g outcome and the dependency traversal, then we can review the updated head.
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
Review round on phase-rs#7303. The CR 303.4f/g consult read the entrant's pre-entry self, so an Aura meld result got no host choice at all; it now reads the liminal projection (CR 614.12). One authority decides the CR 303.4g disposition, and it denies the entry before insertion instead of leaving it to the CR 704.5m SBA. Both `LastCreated` traversals are now wildcard-free, and the Aura-host pause parks its caller's batch tail.
|
Pushed Blocker (CR 303.4g pre-entry) — fixed, but the seam you cited isn't the one Meld uses.
the ZoneChange consult read
I used Tests: Named narrowing, not fixed here: Traversals — fixed, and they were already wrong, not just future-fragile. CodeRabbit's continuation point — threaded, not argued away. The pause now parks CI: the
|
|
Maintainer ported this PR across current The prior requested-changes review was attached to |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/effects/mod.rs (1)
3073-3085: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExtend
condition_depends_on_last_createdto live filter conditions.
TargetMatchesFilter,SourceMatchesFilter, andControllerControlsMatchingevaluate filters against live state, whereTargetFilter::LastCreatedreadsstate.last_created_token_ids. The walker misses these variants and does not recurse throughConditionInstead, which evaluates its inner condition. During suspended token entry, the gated continuation can therefore read a stale ledger and be skipped. Add the applicable arms and regression tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/effects/mod.rs` around lines 3073 - 3085, Extend condition_depends_on_last_created to inspect filters in TargetMatchesFilter, SourceMatchesFilter, and ControllerControlsMatching, and recurse into the inner condition of ConditionInstead. Detect TargetFilter::LastCreated through the existing filter-matching helper, preserve current recursive handling, and add regression tests covering each live-filter variant and ConditionInstead during suspended token entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/zone_pipeline.rs`:
- Around line 3573-3583: The CR 303.4g substitute graveyard placements bypass
destination replacements. In
crates/engine/src/game/zone_pipeline.rs#L3573-L3583, route the rewritten
Zone::Graveyard event through the replacement-aware pipeline while carrying the
existing applied set; in crates/engine/src/game/effects/token.rs#L1559-L1572,
replace the three raw zones calls with the same replacement-aware graveyard
delivery, preserving raw uncreate_unentered_aura_token handling.
---
Outside diff comments:
In `@crates/engine/src/game/effects/mod.rs`:
- Around line 3073-3085: Extend condition_depends_on_last_created to inspect
filters in TargetMatchesFilter, SourceMatchesFilter, and
ControllerControlsMatching, and recurse into the inner condition of
ConditionInstead. Detect TargetFilter::LastCreated through the existing
filter-matching helper, preserve current recursive handling, and add regression
tests covering each live-filter variant and ConditionInstead during suspended
token entry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4765f157-6a4d-45e7-8b81-c08be1d1930f
📒 Files selected for processing (7)
crates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/filter.rscrates/engine/src/game/meld_tests.rscrates/engine/src/game/zone_pipeline.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/game/engine.rs
Only the third producer moved (`:9725 ⇒ :9721`); the other two did not, which locates the deletion between them. Re-read at the new coordinate and sha256-identical to its old one. Coordinate evidence only — this round adds no prompt.
…into pr/7303-maintainer-port # Conflicts: # crates/engine/src/game/engine.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review: e8db46c
Request changes:
- CR 303.4g’s stack-origin substitute outcome is delivered as a direct Battlefield-to-Graveyard rewrite in zone_pipeline after the original entry replacement consult has completed. That bypasses a fresh Moved graveyard-destination replacement (for example Rest in Peace / Leyline of the Void), so the Aura incorrectly reaches the graveyard. The liminal card-backed sibling also uses raw zone mutations for the same disposition. Route this new graveyard placement through the replacement-aware pipeline without re-consulting the already-applied battlefield-entry replacements, and add a discriminating redirect regression. engine_replacement.rs already treats an analogous substitute graveyard fallback as a fresh replacement-aware event.
CI for this exact head is currently queued; that is not the reason for this request.
|
Head Shard 4/4 failed on infrastructure, not on the diff: I do not have rerun rights on the repo — could someone re-run that job? |
Review round 2 on phase-rs#7303. The stack-origin denial rewrote the approved event's destination, so a board-wide Moved graveyard redirect (Rest in Peace) never saw it; it is now a fresh consulted move carrying the applied set. The entrant projection also reaches attach legality and the non-liminal consult, so both copy seams judge the same object, and the `LastCreated` traversals lose their last wildcards.
|
Head Blocker (CR 303.4g graveyard placement) — the stack-origin site is fixed; the liminal sibling is not, and I want that decision reviewed rather than assumed. Stack-origin: no longer a destination rewrite. It proposes a fresh Liminal sibling ( The Non-liminal copy seam consulted before the CR 707.9 exceptions — fixed. It now consults an entrant projection with Player hosts had no attach-legality check at all. Pre-existing, but this PR made it consequential: an illegal player counted as a legal host suppressed the new CR 303.4g denial, so the Curse copy got created and swept. Now filtered through Wildcards. A doc claim in my own last round was false and is corrected. Kept as is, with reasons in code: the Known residual, named rather than hidden: the act half of the non-liminal attach still re-checks legality against the stored object, before Census: re-derived, uniform +93 (
Shard 4/4 was still red on the previous head for infrastructure reasons ( |
|
Head You were right, and it was the residual I disclosed last round rather than fixed. The decide half and the act half derived the attachment side independently — that is what let them disagree. The fix names the authority instead of re-deriving it, so the disagreement is no longer representable. Typed authority ( The decide half now states which object it used ( Multi-host resume: Absent-safety for your three pre-existing callers is by construction, not by care: only a On your point 3 (apply the exceptions before the consult): I re-checked and rejected it, but the previous round's stated reason was the weaker one. The idempotent/pausable split is genuinely order-safe. What kills it is the journal contract — the birth records I also rejected "trust the offered Tests, all through
The one thing I could not close, stated plainly. The player-host half is structurally fixed but has no production-reachable discriminating input, and I did not manufacture one. Housekeeping. New
Two process notes. (1) This push used |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Card-backed unhosted Aura entries bypass graveyard-destination replacements. Evidence: crates/engine/src/game/effects/token.rs:1580-1592 removes the liminal battlefield projection and adds the object directly to its owner's graveyard; the card-backed ProposedEvent::TokenEntry no-host arm calls it around 1751-1763. Why it matters: although today's producer builds token entrants, the public TokenEntry contract also admits a card-backed entrant; when that allowed state has no legal Aura host, CR 303.4g sends it to its owner's graveyard, and applicable Moved replacements such as Rest in Peace or Leyline of the Void must still be able to redirect that placement. The raw move skips that replacement pipeline. Suggested fix: either represent a replacement-aware from-no-zone graveyard placement, preserving the absence of a false battlefield-entry event, or narrow TokenEntry so card-backed entrants are unrepresentable; add a regression with a graveyard redirect and assert both the redirected destination and no battlefield-entry event.
A `TokenEntry` entrant is a CR 111.1 marker in no zone, so CR 303.4g's card dispositions cannot apply to it. `LiminalEntrant` carries that as a witness instead of an expected flag, which deletes the raw graveyard placement rather than teaching it about replacements. Wire form is unchanged; the reachable card-backed class keeps its consulted move.
|
Head
The narrowing is a witness type, not a flag check:
Why not your first option, concretely. I verified the claim rather than repeating it: The rules also don't treat the two options symmetrically. CR 303.4g's card dispositions are both phrased against the zone the Aura enters from ("remains in its current zone, unless that zone is the stack"). A On the regression you asked for. On the
Assertions that legitimately changed. New-field sweep: neither a One soft spot I'd rather name than have found:
Same two process notes as before: pushed with |
Maintainer hold — port requiredThe prior requested change is resolved at I am not asking you to rebase. The PR is currently The current head has only Contributor trust, Superagent Security Scan, and CodeRabbit results. There are no current Rust/frontend Actions or current parse-diff sticky evidence to rely on. Those will be rechecked after the port; no approval or enqueue is appropriate before then. |
# Conflicts: # crates/engine/src/game/engine.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer ported the stale-base conflict at head 2df16bff944ea4000be818594501bff44c2dbab3.
The only textual conflict was the unrelated optional-effect prompt census in game/engine.rs; I retained current main's authoritative coordinates and preserved the Aura-token implementation. The previous projection-attachment review remains clean on the port. This head is held pending fresh CI, parse-diff, and CodeRabbit feedback; no approval or queue action yet.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer correction at d9912081213d568c1ec91fb15c05afc0004955e3 re-pins the stage-2 optional-effect census to the three current, byte-identical producer sites in game/effects/mod.rs (6548, 6625, and 9820). The prior failed Rust shard was coordinate-only after the maintainer port; all other reported checks were green.
Holding this exact head for fresh CI, parser artifact output, and automated-review feedback before any approval or merge-queue action.
Maintainer hold — current-main port awaiting CIMaintainer ported the reviewed implementation from The sole textual conflict was the OptionalEffect census in |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head maintainer review: port conflict resolution preserved the reviewed Aura-entry authority; current required CI and current-head parse artifact are green. Approving for merge queue.
Merge current origin/main and preserve both the phase-rs#7303 entering-Aura census shift and the random-discard continuation census change. The stage-2 producer was re-derived by content at game/engine.rs:12018.
2 participants
Summary
A token that's a copy of an Aura entered unattached, so the CR 704.5m SBA binned it before the player saw it — no host prompt, no copy. Yenna, Redtooth Regent copying
Cooped Upproduced nothing. The CR 303.4f consult now runs on both copy-token entry paths.Fixes #5904.
Files changed
game/zone_pipeline.rs—EnteringAuraAttachment::Resolvedsplit intoAttached/NoLegalHost;resolve_entering_aura_attachmentdecomposed into pureentering_aura_hosts+apply_entering_aura_hosts.game/effects/token.rs— CR 303.4f consult on the liminal copy path;UnhostedAuraEntry;uncreate_unentered_aura_token.game/effects/token_copy.rs— same consult on the non-liminal path; entry tail extracted tofinish_non_liminal_copy_token_entry.game/effects/mod.rs,game/filter.rs—condition_depends_on_last_createddefers aLastCreated-gated sub-ability past the prompt.types/game_state.rs,game/effects/counters.rs—ContinueCopyTokenEntryAfterAuraHost+CopyTokenEntryTail.game/sba.rs— CR 704.4:pending_replacement_pauses_sba→mid_resolution_entry_pauses_sba.game/engine_replacement.rs,game/engine.rs— callers and census pin.tests/integration/yenna_aura_token_copy.rs(new, 13 tests),main.rs,battlefield_entry_authority_census.rs.Track
Non-developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 303.4f— controller chooses the host as the Aura enters. The consult, on both copy paths.CR 303.4g— no legal host and the Aura is a token → it isn't created. Distinct from CR 704.5m: nothing is journaled, noTokenCreated, nolast_created_token_idsrow.CR 303.4d— an Aura that's also a creature enchants nothing. Left as an SBA case; the token IS created, then dies.CR 704.4— SBAs ignore what happens during resolution. Widened so an open host prompt doesn't expose the entrant to CR 704.5m.CR 608.2c— theLastCreated-gated rider defers instead of evaluating mid-prompt.Also touched: 111.1, 111.2, 111.7, 111.10, 122.6a, 306.5b, 400.7, 508.4, 603.2c, 603.7, 608.2i, 614.1c, 614.12, 616.1, 704.5a, 704.5m, 707, 707.2, 707.9. All verified against
docs/MagicCompRules.txt.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— 18912 + 4857 + 21 + 9 passed, 0 failed./scripts/check-engine-authorities.sh— Gate B PASS (42 hits / 26 rows), Gate D PASScargo clippy --workspace --all-targets -- -D warnings— cleancargo fmt --all— applied.githooks/pre-push— full gate, all stepsRevert-discrimination, measured: patching out
condition_depends_on_last_createdfails onlymulti_host_copy_prompts_and_attaches; narrowingmid_resolution_entry_pauses_sbafails onlyan_open_host_prompt_does_not_expose_the_token_to_sbas; dropping the CR 303.4g un-creation fails only the two..._is_not_createdtests.Pre-existing, not from this diff:
cargo test --workspacefailsmtgish-import::manifest_coverage::every_list_field_is_in_ordering_manifest—(SpellContext, parent_target_iteration_members)is missing fromORDERING_MANIFESTatad7ba57already.mtgish/is dormant, so left alone.Gate A
Gate A PASS head=79b9e1a0b08b74ed1011548c56eb8e0b545e4f73 base=81c54e325d4cb7ae5e925d18bad3b98ae85d52af
Anchored on
crates/engine/src/game/zone_pipeline.rs:3261— theattach_to.is_none()CR 303.4f consult on the ZoneChange entry path. The reference implementation this change mirrors onto the token paths;ProposedEvent::TokenEntryhas noattach_tofield, which is why tokens never reached it.crates/engine/src/game/effects/token.rs:1785— the enter-with-counters mid-token-entry pause. Same carrier (stash_pending_counter_additions+PendingCounterPostAction), samefalsereturn, same resume door.Final review-impl
Not clean — one MED finding accepted rather than fixed. See Scope Expansion.
Claimed parse impact
None. No parser change.
Scope Expansion
None. The reverse, disclosed deliberately:
/review-implfound the sibling seam — plainEffect::Tokenwith a specified host that fails to bind — carries the same silent-vanish under CR 303.4i. Left out on purpose: different rule, different seam (apply_create_token_after_replacement), and 47 of the 48 Aura-typedEffect::Tokenspecs in the shipped pool bind their host correctly. The one that doesn't (Questing Cosplayer) is a parser defect —"and attach it to target creature"is dropped — so fixing it here would build the wrong thing. Filed separately as #7302.Validation Failures
Open review findings, all disclosed, none fixed:
Auraafter the consult. Unreachable for shipped cards (needs a type-changing exception co-occurring with entry counters).EffectResolved { kind: ChangeZone }from the sharedReturnAsAuraTargethandler. Trigger-inert (trigger_index.rsmaps that kind to no trigger keys); log noise only. Not forked, because the pre-existing callers share that handler.// CR 733on the journal annotations does not describe CR 733 ("Handling Illegal Actions"). Pre-existing repo-wide convention at 50+ sites; continued rather than unilaterally renamed.can_attach_to_objectreads the attachment fromstate.objects, so for a liminal entrant the CR 701.3a / CR 702.16c halves see pre-entry characteristics. Permissive-only, and strictly better than the prior no-consult behaviour. Closing it means threading the entrant projection throughattach.rs— a wider single-authority change than this seam should make.crates/engine/src/game/engine.rs's CR 603.5 census pin is line-exact and has now drifted twice (once on yourmainmerge, once on this round). Re-derived, not guessed: all three producers verified sha256-identical at their new coordinates. A function+content-hash anchor would end this drift class.CI Failures
None.
Summary by CodeRabbit
Bug Fixes
Tests