feat(engine): journal the CR 603.3d uncommitted-trigger removal - #6667
Merged
Conversation
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 #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.
matthewevans
enabled auto-merge
July 26, 2026 12:59
Contributor
📝 WalkthroughWalkthroughThe PR adds a resolved journal command for uncommitted triggered-ability removal, records pop and no-pop outcomes, validates replay invariants, updates replay dispatch, and adds integration coverage for exact, divergent, and cursor-only removal cases. ChangesUncommitted trigger removal
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TriggerResolution
participant Stack
participant Journal
participant ReplayState
TriggerResolution->>Stack: decline optional trigger
Stack->>Stack: consume cursor and conditionally pop entry
Stack->>Journal: record removal outcome
ReplayState->>Stack: apply recorded command
Stack->>Stack: validate predecessor invariants
Stack-->>ReplayState: updated stack and cursor state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/integration/cr733_resolved_trigger_removal.rs`:
- Around line 238-286: The test currently hand-constructs a no-pop removal
instead of exercising the production guard-failure path. Extend the fixture
around a_removal_that_popped_nothing_still_clears_the_cursor to remove the
pending-trigger entry through a real replacement/SBA sweep or competing decline
before invoking pop_uncommitted_pending_trigger_entry, then obtain and replay
the resulting no-pop command. Keep assertions that the cursor clears and the
stack remains unchanged, while ensuring the production path—not a manually built
ResolvedUncommittedTriggerRemovalCommand—produces the record.
🪄 Autofix (Beta)
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: 0c5adf83-508c-4e2d-8f2d-8116eec790b4
📒 Files selected for processing (7)
crates/engine/src/game/stack.rscrates/engine/src/types/mod.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/cr733_resolved_commands_p2.rscrates/engine/tests/integration/cr733_resolved_draw.rscrates/engine/tests/integration/cr733_resolved_trigger_removal.rscrates/engine/tests/integration/main.rs
Comment on lines
+238
to
+286
| fn a_removal_that_popped_nothing_still_clears_the_cursor() { | ||
| let state = declined_optional_trigger_state(); | ||
| let commands = removals(&state); | ||
| let popping = &commands[0]; | ||
|
|
||
| let no_pop = ResolvedUncommittedTriggerRemovalCommand { | ||
| consumed_entry_id: popping.consumed_entry_id, | ||
| removed: None, | ||
| resulting_depth: state.stack.len(), | ||
| cause: popping.cause, | ||
| }; | ||
|
|
||
| // Predecessor whose top IS that entry: a no-pop record is a divergence. | ||
| let mut still_present = pre_removal_state(&state, popping); | ||
| assert!( | ||
| matches!( | ||
| apply_resolved_uncommitted_trigger_removal(&mut still_present, &no_pop), | ||
| Err(ResolvedUncommittedTriggerRemovalReplayInvariantError::DepthMismatch { .. }) | ||
| | Err( | ||
| ResolvedUncommittedTriggerRemovalReplayInvariantError::UnexpectedRemovableEntry( | ||
| _ | ||
| ) | ||
| ) | ||
| ), | ||
| "a no-pop record must not apply where the entry is still removable" | ||
| ); | ||
| assert_eq!( | ||
| still_present.pending_trigger_entry, | ||
| Some(popping.consumed_entry_id), | ||
| "the rejected replay must not have consumed the cursor" | ||
| ); | ||
|
|
||
| // Predecessor where the entry has already gone: the same record applies and | ||
| // clears the cursor without touching the stack. | ||
| let mut already_gone = state.clone(); | ||
| already_gone.pending_trigger_entry = Some(popping.consumed_entry_id); | ||
| let depth_before = already_gone.stack.len(); | ||
| apply_resolved_uncommitted_trigger_removal(&mut already_gone, &no_pop) | ||
| .expect("a no-pop removal replays where the entry is already gone"); | ||
| assert_eq!( | ||
| already_gone.pending_trigger_entry, None, | ||
| "the cursor is consumed even when nothing was popped" | ||
| ); | ||
| assert_eq!( | ||
| already_gone.stack.len(), | ||
| depth_before, | ||
| "a no-pop removal must not touch the stack" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
No production-path coverage for the "guard fails" branch.
a_removal_that_popped_nothing_still_clears_the_cursor proves apply_resolved_uncommitted_trigger_removal's None branch replays correctly, but the no_pop command is hand-constructed (Lines 243-248) rather than journaled from a real scenario. Nothing in this file drives pop_uncommitted_pending_trigger_entry (stack.rs Lines 302-309) through a genuine "cursor outlived its entry" case — i.e., the state.stack.back().map(|e| e.id) == Some(entry_id) guard evaluating to false in production. If that guard's condition were ever flipped or the entry lookup broken, no test here would catch it, since only the replay side is exercised for this outcome.
Consider adding a fixture where the pending-trigger entry is removed via a different path (e.g., a replacement/SBA sweep, or a second decline racing the same cursor) before pop_uncommitted_pending_trigger_entry runs, so the "no-pop" record is observed coming out of the real authority rather than assembled by hand.
As per path instructions, "A test must exercise the FAILURE path the fix prevents and drive the engine through its production pipeline... Flag constructor shortcuts... that can silently mask the very bug a regression test claims to catch."
🤖 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/tests/integration/cr733_resolved_trigger_removal.rs` around
lines 238 - 286, The test currently hand-constructs a no-pop removal instead of
exercising the production guard-failure path. Extend the fixture around
a_removal_that_popped_nothing_still_clears_the_cursor to remove the
pending-trigger entry through a real replacement/SBA sweep or competing decline
before invoking pop_uncommitted_pending_trigger_entry, then obtain and replay
the resulting no-pop command. Keep assertions that the cursor clears and the
stack remains unchanged, while ensuring the production path—not a manually built
ResolvedUncommittedTriggerRemovalCommand—produces the record.
Source: Path instructions
Parse changes introduced by this PR✓ No card-parse changes detected. |
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.
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_entrymarking the entry whose slots are stillunfilled. Declining an optional MODAL trigger before its mode choice abandons
one, and #6664 funnelled all six sites through a single authority. This journals
that authority.
TWO OUTCOMES, both mutating, which is why
removedis anOptionrather than abare entry. The authority consumes
pending_trigger_entryUNCONDITIONALLY andonly then decides whether to pop:
its entry (another path already removed it)
A command modelling only the first would leave a replay of the second holding a
pending_trigger_entrythe real execution had cleared — a divergence needing noforged journal, only an honest replay. The applier therefore also REFUSES a
removed: Nonerecord whose predecessor still has the entry on top: replayingthere 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 recordsexpected_old_paid_factsbecause 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.
ObjectIdisreused 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 (nomay-offer to decline). Control.
you may destroy target creaturenever reaches it at all — a "you may" onan 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:
left: 0, right: 1. Provesevery test reaches the production journal path.
removal_rejects_a_divergent_predecessor. Proves that test isdiscriminating 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 --libclean.Summary by CodeRabbit
New Features
Bug Fixes
Tests