refactor(engine): extract the CR 707.10 copy-onto-stack authority - #6647
Merged
Conversation
Four copy effects hand-rolled their own `state.stack.push_back` plus a
manual `GameEvent::StackPushed`, bypassing any shared push path:
`copy_spell`, `cast_copy_of_card`, `paradigm`, and `epic`.
They are not deviations from `stack::push_to_stack` that should be
funnelled into it — putting an object onto the stack (CR 405.1 /
CR 601.2a) and copying one onto it (CR 707.10) are distinct sub-families
that disagree on both source-referential stamps:
* CR 701.27f generation — a copy captures the source's generation at
copy-creation time and must overwrite what the copied ability inherited
from the original's earlier push. `push_to_stack` guards its stamp with
`is_none()` so a delayed triggered ability's creation-time generation
survives firing; applying that guard to a copy leaves it comparing
against the original's generation and silently no-opping.
* Force-block binding — a copied ability already carries the
`force_block_attacker` its original was bound to, sourced from the
trigger's captured `trigger_source` provenance. Re-running
`bind_force_block_source_recursive` would overwrite that exact
choice-time referent with a live `state.objects` rescan, contradicting
CR 707.10b ("a copy of an ability has the same source as the original").
So this adds `stack::push_copy_to_stack` as the copy-family sibling
rather than merging the two. It stamps the CR 701.27f generation
unconditionally (lifted verbatim from `copy_spell`, including its
activated/triggered gate — CR 701.27f covers only "an activated or
triggered ability of a permanent", so the stamp is correctly inert for
the three spell-copy callers), deliberately does not re-bind force-block,
and emits `StackPushed` exactly once. Each of the four hand-rolled
emissions was positionally equivalent, so event order is unchanged.
Behavior is unchanged; the new test pins the CR 701.27f semantics that
were previously implicit in one caller and are now the authority's
contract. It drives a real double activation plus a real Lithoform Engine
copy, and fails (generation 1, not 2) if the stamp is re-guarded with
`is_none()`.
Production `state.stack.push_back` sites are now exactly two, one per
family authority, both in `game/stack.rs`.
matthewevans
enabled auto-merge
July 26, 2026 04:34
Contributor
|
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 (7)
📝 WalkthroughWalkthroughCopied spells and abilities now use ChangesCopied ability stack flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
added a commit
to mike-theDude/phase
that referenced
this pull request
Jul 26, 2026
…uthority (phase-rs#6664) The "push first, choose second" invariant puts a triggered ability on the stack before its choices are gathered (see `GameState::pending_trigger_entry`), so the entry is live while a `WaitingFor` fills its slots. When those choices cannot be completed, CR 603.3d says the ability is simply removed from the stack. That removal was written raw at SIX sites as a byte-identical seven-line block (verified by whole-block comparison, not a sampled window): the pop, plus the two per-entry side tables that settle with it. if let Some(entry_id) = state.pending_trigger_entry.take() { if state.stack.back().map(|e| e.id) == Some(entry_id) { state.stack.pop_back(); state.stack_paid_facts.remove(&entry_id); state.stack_trigger_event_batches.remove(&entry_id); } } One of the six was already inside `drop_mid_construction_pending_trigger`, so this is one existing authority plus five sites bypassing it — not six orphans. The five are in `begin_pending_trigger_target_selection` (x4) and `resolve_random_modal_trigger`. Routing the five INTO `drop_mid_construction_pending_trigger` would have been wrong: that function additionally clears `pending_trigger`, which the five deliberately do not, so it would have been a silent behaviour change at five call sites. Instead the shared seven lines become `stack::pop_uncommitted_pending_trigger_entry`, the existing authority calls it and then clears `pending_trigger`, and the five call it directly. One authority for the mutation, no semantic change anywhere. The two side tables are cleared inside the authority rather than by callers because they are keyed on the entry and settle WITH the pop — a removal that dropped the entry but left `stack_paid_facts` or `stack_trigger_event_batches` behind would strand rows against an id no longer on the stack. Deliberately NOT folded in: `triggers.rs`'s recovery path clears both side tables with no pop, because its entry has already vanished from the stack. Same two lines, different operation. Prerequisite for journaling the CR733 stack-pop family, which requires exactly one authority per family — the same shape phase-rs#6647 used for the CR 707.10 copy push. No journaling here; this is the extraction only. CR 603.3d grep-verified against docs/MagicCompRules.txt: "If a choice is required when the triggered ability goes on the stack but no legal choices can be made for it, or if a rule or a continuous effect otherwise makes the ability illegal, the ability is simply removed from the stack." Behaviour-preserving: engine integration suite 4057 passed, 0 failed. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four copy effects hand-rolled their own
state.stack.push_backplus amanual
GameEvent::StackPushed, bypassing any shared push path:copy_spell,cast_copy_of_card,paradigm, andepic.They are not deviations from
stack::push_to_stackthat should befunnelled into it — putting an object onto the stack (CR 405.1 /
CR 601.2a) and copying one onto it (CR 707.10) are distinct sub-families
that disagree on both source-referential stamps:
copy-creation time and must overwrite what the copied ability inherited
from the original's earlier push.
push_to_stackguards its stamp withis_none()so a delayed triggered ability's creation-time generationsurvives firing; applying that guard to a copy leaves it comparing
against the original's generation and silently no-opping.
force_block_attackerits original was bound to, sourced from thetrigger's captured
trigger_sourceprovenance. Re-runningbind_force_block_source_recursivewould overwrite that exactchoice-time referent with a live
state.objectsrescan, contradictingCR 707.10b ("a copy of an ability has the same source as the original").
So this adds
stack::push_copy_to_stackas the copy-family siblingrather than merging the two. It stamps the CR 701.27f generation
unconditionally (lifted verbatim from
copy_spell, including itsactivated/triggered gate — CR 701.27f covers only "an activated or
triggered ability of a permanent", so the stamp is correctly inert for
the three spell-copy callers), deliberately does not re-bind force-block,
and emits
StackPushedexactly once. Each of the four hand-rolledemissions was positionally equivalent, so event order is unchanged.
Behavior is unchanged; the new test pins the CR 701.27f semantics that
were previously implicit in one caller and are now the authority's
contract. It drives a real double activation plus a real Lithoform Engine
copy, and fails (generation 1, not 2) if the stamp is re-guarded with
is_none().Production
state.stack.push_backsites are now exactly two, one perfamily authority, both in
game/stack.rs.Summary by CodeRabbit
Bug Fixes
Tests