refactor(engine): extract the CR 603.3d uncommitted-trigger removal authority - #6664
Merged
Conversation
…uthority
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 #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.
matthewevans
enabled auto-merge
July 26, 2026 11:45
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 (3)
📝 WalkthroughWalkthroughChangesThe PR adds a centralized helper for removing uncommitted pending trigger entries and uses it across modal-trigger, target-selection, and no-target cleanup paths. Pending trigger cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 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. |
Merged
matthewevans
added a commit
to minion1227/phase
that referenced
this pull request
Jul 26, 2026
…e-rs#6667) CR 603.3d: "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." The engine reaches that removal through the "push first, choose second" invariant: a triggered ability is PUT on the stack and only then are its choices gathered, with `pending_trigger_entry` marking the entry whose slots are still unfilled. Declining an optional MODAL trigger before its mode choice abandons one, and phase-rs#6664 funnelled all six sites through a single authority. This journals that authority. TWO OUTCOMES, both mutating, which is why `removed` is an `Option` rather than a bare entry. The authority consumes `pending_trigger_entry` UNCONDITIONALLY and only then decides whether to pop: * guard holds — cursor consumed AND the entry leaves with both side tables * guard fails — cursor consumed and nothing else, because the cursor outlived its entry (another path already removed it) A command modelling only the first would leave a replay of the second holding a `pending_trigger_entry` the real execution had cleared — a divergence needing no forged journal, only an honest replay. The applier therefore also REFUSES a `removed: None` record whose predecessor still has the entry on top: replaying there would clear the cursor and strand the entry, a state no execution produces. The removed side-table VALUES are deliberately not recorded. Contrast `ResolvedStackEntryFinalizeCommand`, which records `expected_old_paid_facts` because it INSTALLS a value and must verify what it overwrites. This command only removes rows keyed on the recorded entry's own id — nothing is installed and nothing re-derived, so there is no invariant a recorded value would pin, and carrying `Vec<GameEvent>` batches would widen every journal entry for nothing. The applier compares the popped entry WHOLE rather than by id. `ObjectId` is reused across a replay, so an id-only match would discard a divergent entry that merely shares an id and report success. Fixture note, because three plausible cards do NOT reach this authority and each failure is silent. Measured: * `you may choose one — ...` reaches it; entry popped. * `choose one — ...` reaches mid-construction, never removed (no may-offer to decline). Control. * `you may destroy target creature` never reaches it at all — a "you may" on an effect is resolved under CR 608.2d when the ability RESOLVES, so the ability is fully constructed when pushed and declining it makes it do nothing rather than removing it. Revert probes, each watched go red and restored: 1. journal call removed -> 0 passed / 3 failed, `left: 0, right: 1`. Proves every test reaches the production journal path. 2. applier matches by id instead of whole entry -> 2 passed / 1 failed, only `removal_rejects_a_divergent_predecessor`. Proves that test is discriminating rather than incidentally coupled. CR 603.3d and CR 603.3c grep-verified against docs/MagicCompRules.txt. Engine integration suite 4064 passed, 0 failed. `clippy -p engine --lib` clean. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
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.
The "push first, choose second" invariant puts a triggered ability on the stack
before its choices are gathered (see
GameState::pending_trigger_entry), so theentry is live while a
WaitingForfills its slots. When those choices cannot becompleted, 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.
One of the six was already inside
drop_mid_construction_pending_trigger, sothis is one existing authority plus five sites bypassing it — not six orphans.
The five are in
begin_pending_trigger_target_selection(x4) andresolve_random_modal_trigger.Routing the five INTO
drop_mid_construction_pending_triggerwould have beenwrong: that function additionally clears
pending_trigger, which the fivedeliberately 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 itand then clears
pending_trigger, and the five call it directly. One authorityfor 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_factsorstack_trigger_event_batchesbehind would strand rows against an id no longer on the stack.
Deliberately NOT folded in:
triggers.rs's recovery path clears both side tableswith 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 #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.
Summary by CodeRabbit