ship/cr733 attach - #6644
Merged
Merged
Conversation
The three attachment authorities (attach_to, attach_to_player, unattach) wrote their graph mutation raw, so a CR 733 retained-prefix replay had no record of who was attached to whom. They are the same mutation parameterized by the resulting host, so they share one ResolvedAttachmentCommand rather than three sibling variants: Some(Object), Some(Player), and None (unattach) are leaf values of the Option<AttachTarget> the object already stores. CR 613.7e + CR 701.3c: attaching to a different host draws a NEW timestamp that orders the attachment in the layer system, while a same-host re-attach (CR 701.3b) and an unattach draw none. resulting_timestamp is therefore Some exactly when the authority drew one, and apply_resolved_attachment installs that value instead of re-drawing from next_timestamp. Journaling at the three authorities covers every production attach site for free, including the zone delivery tail's enters-attached path.
…mmands The zone delivery tail retagged entering permanents twice with raw writes, so a CR 733 retained-prefix replay lost both facts. CR 110.2a controller override: apply_battlefield_entry_controller_override wrote five fields raw. Four are persistent state (the object's base_controller and controller, plus the zone_changes_this_turn and battlefield_entries_this_turn snapshot controllers) and are now one ResolvedControllerOverrideCommand. The fifth, the in-flight ZoneChanged event, stays outside the command: events are transient carriers consumed by the same resolution, not state a replay reconstructs. Both snapshot POSITIONS are recorded rather than re-found, mirroring ResolvedZoneChangeCommand::turn_zone_change_index -- CR 400.7 permits the same object to hold several entries in one turn, so a replay-time last-match scan could retag a different one. CR 603.6a provenance stamp: entered_via_ability_source was the last bare field write at the delivery-tail site, with no owning authority at all. zones::stamp_battlefield_entry_provenance is that authority, paired with apply_resolved_entry_provenance. Without it a replay would let anti-recursion intervening-ifs re-trigger off their own output. They stay separate families rather than one parameterized entry retag: CR 110.2a and CR 603.6a are different rule sections the engine resolves independently.
zones::cease_object is the only production path that deletes an object outright, and the CR 704.5d state-based sweep is its single caller. It wrote state.objects.remove raw, so a retained-prefix replay left the token alive in a zone the rules had already swept it from. Ceasing to exist is deliberately NOT a zone change (CR 400.7) -- no event is emitted and no whenever-exiled trigger fires -- so it cannot ride the zone-change family and needs its own command. No characteristics are recorded: replay removes the object the retained prefix already reconstructed rather than rebuilding a deleted one.
matthewevans
enabled auto-merge
July 26, 2026 02:24
Contributor
📝 WalkthroughWalkthroughThe PR adds resolved-command journaling and replay for attachment changes, controller overrides, entry provenance, object cessation, and player departure. It adds typed contracts, invariant validation, gameplay integration, replay dispatch, and CR733 integration tests. ChangesCR733 resolved command journaling and replay
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GameResolution
participant ResolvedRulesJournal
participant ReplayApplier
GameResolution->>ResolvedRulesJournal: record resolved command
ResolvedRulesJournal->>ReplayApplier: provide serialized command
ReplayApplier->>ReplayApplier: validate preconditions and apply exact state change
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/zones.rs`:
- Around line 1744-1747: The absent-object branch in the surrounding
SBA/object-cease function must not call remove_from_zone without journaling.
Since the caller reads the object immediately beforehand, assert or otherwise
require the object to exist and remove this fallback; alternatively, record the
zone-list removal through the existing ObjectCease command path before
returning.
- Around line 2035-2050: Strengthen the validation around
command.zone_change_index and command.battlefield_entry_index to verify the
indexed record’s object_id matches command.object before any controller
mutation. Keep the existing bounds checks and return MissingZoneChangeRecord or
MissingBattlefieldEntryRecord for both out-of-range and identity mismatches.
🪄 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: ca9173d1-045f-47dc-9958-7cfeea1a17e8
📒 Files selected for processing (10)
crates/engine/src/game/effects/attach.rscrates/engine/src/game/zone_pipeline.rscrates/engine/src/game/zones.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/cr733_resolved_attachment.rscrates/engine/tests/integration/cr733_resolved_commands_p2.rscrates/engine/tests/integration/cr733_resolved_draw.rscrates/engine/tests/integration/cr733_resolved_entry_retags.rscrates/engine/tests/integration/cr733_resolved_object_cease.rscrates/engine/tests/integration/main.rs
Comment on lines
+1744
to
+1747
| let Some(object) = state.objects.get(&object_id) else { | ||
| remove_from_zone(state, object_id, zone, owner); | ||
| return; | ||
| }; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Absent-object fallback performs an unjournaled persistent write.
When the object map entry is already gone but the owner's zone list still holds the id, remove_from_zone mutates persistent state and the function returns without recording a command. A retained-prefix replay then leaves that id in the zone list — the exact divergence the ObjectCease family exists to close. Either assert the object is present (the SBA sweep is the only caller and reads it immediately before), or record the zone-list removal.
🤖 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/zones.rs` around lines 1744 - 1747, The absent-object
branch in the surrounding SBA/object-cease function must not call
remove_from_zone without journaling. Since the caller reads the object
immediately beforehand, assert or otherwise require the object to exist and
remove this fallback; alternatively, record the zone-list removal through the
existing ObjectCease command path before returning.
Comment on lines
+2035
to
+2050
| if let Some(index) = command.zone_change_index { | ||
| if index >= state.zone_changes_this_turn.len() { | ||
| return Err( | ||
| ResolvedControllerOverrideReplayInvariantError::MissingZoneChangeRecord(index), | ||
| ); | ||
| } | ||
| } | ||
| if let Some(index) = command.battlefield_entry_index { | ||
| if index >= state.battlefield_entries_this_turn.len() { | ||
| return Err( | ||
| ResolvedControllerOverrideReplayInvariantError::MissingBattlefieldEntryRecord( | ||
| index, | ||
| ), | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Recorded snapshot positions are bounds-checked but not identity-checked.
If a replayed prefix produced a differently ordered zone_changes_this_turn / battlefield_entries_this_turn, an in-range index silently retags an unrelated record's controller instead of failing. The command already carries object, so compare the record's object_id before mutating and surface the existing Missing*Record error otherwise.
🛡️ Proposed precondition tightening
if let Some(index) = command.zone_change_index {
- if index >= state.zone_changes_this_turn.len() {
+ if state
+ .zone_changes_this_turn
+ .get(index)
+ .is_none_or(|record| record.object_id != object_id)
+ {
return Err(
ResolvedControllerOverrideReplayInvariantError::MissingZoneChangeRecord(index),
);
}
}
if let Some(index) = command.battlefield_entry_index {
- if index >= state.battlefield_entries_this_turn.len() {
+ if state
+ .battlefield_entries_this_turn
+ .get(index)
+ .is_none_or(|record| record.object_id != object_id)
+ {
return Err(
ResolvedControllerOverrideReplayInvariantError::MissingBattlefieldEntryRecord(
index,
),
);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Some(index) = command.zone_change_index { | |
| if index >= state.zone_changes_this_turn.len() { | |
| return Err( | |
| ResolvedControllerOverrideReplayInvariantError::MissingZoneChangeRecord(index), | |
| ); | |
| } | |
| } | |
| if let Some(index) = command.battlefield_entry_index { | |
| if index >= state.battlefield_entries_this_turn.len() { | |
| return Err( | |
| ResolvedControllerOverrideReplayInvariantError::MissingBattlefieldEntryRecord( | |
| index, | |
| ), | |
| ); | |
| } | |
| } | |
| if let Some(index) = command.zone_change_index { | |
| if state | |
| .zone_changes_this_turn | |
| .get(index) | |
| .is_none_or(|record| record.object_id != object_id) | |
| { | |
| return Err( | |
| ResolvedControllerOverrideReplayInvariantError::MissingZoneChangeRecord(index), | |
| ); | |
| } | |
| } | |
| if let Some(index) = command.battlefield_entry_index { | |
| if state | |
| .battlefield_entries_this_turn | |
| .get(index) | |
| .is_none_or(|record| record.object_id != object_id) | |
| { | |
| return Err( | |
| ResolvedControllerOverrideReplayInvariantError::MissingBattlefieldEntryRecord( | |
| index, | |
| ), | |
| ); | |
| } | |
| } |
🤖 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/zones.rs` around lines 2035 - 2050, Strengthen the
validation around command.zone_change_index and command.battlefield_entry_index
to verify the indexed record’s object_id matches command.object before any
controller mutation. Keep the existing bounds checks and return
MissingZoneChangeRecord or MissingBattlefieldEntryRecord for both out-of-range
and identity mismatches.
P1 defined RulesExecutionNodeKind::PlayerLeave and the matching node ref, but nothing in the engine ever produced one. The CR 800.4 sweep marked is_eliminated and appended to eliminated_players with raw writes, and every mutation it performed downstream -- owned-object exiles, control-effect reversions, stack removals -- was attributed to whatever proposal happened to be live when the state-based action fired. do_eliminate now opens the leave's own execution node before the sweep and restores the enclosing scope after it, so those downstream commands are attributed to the departure and a replay can recognize the sweep as one causal unit. The departure itself is one ResolvedPlayerLeaveCommand: the two writes always move together. The command carries no expected_old field -- "was still in the game" is the precondition, and a stored copy could only ever hold one value. Re-applying a departure is a typed invariant failure rather than a silent re-elimination.
CI's wasm and card-data lib builds fail with `-D dead-code` on apply_resolved_controller_override: game::zones is `pub` only under the test-support feature, so in a production build a pub fn nothing in the crate calls is dead. The sibling authorities do not have this problem because resolve_and_apply_zone_change, stamp_battlefield_entry_provenance, and cease_object all install through their own applier. Make the override do the same. Resolve and replay now share one mutation body instead of two copies that can drift, and the snapshot retag is factored into retag_battlefield_entry_snapshots so both paths install the same thing.
The fixture set life directly and relied on advance_until_stack_empty to run the CR 704.5a state-based check, which never fired with an already-empty stack. Resolving a real Lightning Bolt at the victim takes them to 0 through the production pipeline and makes the check run where it actually runs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/types/resolved_commands.rs`:
- Around line 350-357: Add verified inline CR annotations using the exact `// CR
800.4: <description>` format, after checking `docs/MagicCompRules.txt`, for the
departure command semantic boundary at
crates/engine/src/types/resolved_commands.rs:350-357, leave-node creation at
:1301-1309, recording semantics at :1602-1608, and replay validation at
:2154-2164; replace the existing `///` rule references where applicable and
ensure each annotation accurately describes the covered behavior.
In `@crates/engine/tests/integration/cr733_resolved_player_leave.rs`:
- Around line 83-86: Update the command filtering around
ResolvedRulesCommand::PlayerLeave to avoid the wildcard match: use a selective
matches! predicate for PlayerLeave commands belonging to P1, followed by
refutable let extraction, or explicitly enumerate all known enum variants.
Preserve the existing collection of matching PlayerLeave commands.
- Around line 75-133: Update the integration test around
apply_resolved_player_leave to capture a pre-elimination state snapshot and add
a player-owned object/control-effect fixture. Replay the journal entries from
that snapshot through the production dispatcher rather than directly replaying
only the departure fields, then assert the resulting CR 800.4 sweep commands
carry a RulesExecutionNodeRef::PlayerLeave cause and preserve the existing
replay/invariant checks.
🪄 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: 0cf74323-0be3-4769-bc0b-dce0a77188f3
📒 Files selected for processing (8)
crates/engine/src/game/elimination.rscrates/engine/src/game/zones.rscrates/engine/src/types/game_state.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_player_leave.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/engine/tests/integration/cr733_resolved_draw.rs
- crates/engine/tests/integration/cr733_resolved_commands_p2.rs
- crates/engine/src/game/zones.rs
Comment on lines
+350
to
+357
| /// One exact CR 800.4 player departure. | ||
| /// | ||
| /// The departure itself is two writes — the player's `is_eliminated` flag and | ||
| /// their append to `eliminated_players` — that always move together, so they are | ||
| /// one command rather than two. Everything the CR 800.4 sweep does afterwards | ||
| /// (exiling owned objects, reverting control effects, clearing the stack) | ||
| /// already journals through its own family; this command carries the departure, | ||
| /// and the surrounding `PlayerLeave` node carries the causal grouping. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use verified inline CR annotations for the new departure rules logic.
The new CR 800.4 references use ///; this does not meet the required exact // CR <number>: <description> annotation format.
crates/engine/src/types/resolved_commands.rs#L350-L357: add a verified inline CR annotation for the departure command’s semantic boundary.crates/engine/src/types/resolved_commands.rs#L1301-L1309: annotate leave-node creation with the verified CR 800.4 behavior.crates/engine/src/types/resolved_commands.rs#L1602-L1608: annotate recording semantics with the verified rule.crates/engine/src/types/resolved_commands.rs#L2154-L2164: annotate replay validation with the verified rule.
As per coding guidelines, “Verify every implementation of MTG game rules against the relevant Comprehensive Rules section”; as per path instructions, use the exact // CR XXX.Y... form after verifying it in docs/MagicCompRules.txt.
📍 Affects 1 file
crates/engine/src/types/resolved_commands.rs#L350-L357(this comment)crates/engine/src/types/resolved_commands.rs#L1301-L1309crates/engine/src/types/resolved_commands.rs#L1602-L1608crates/engine/src/types/resolved_commands.rs#L2154-L2164
🤖 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/types/resolved_commands.rs` around lines 350 - 357, Add
verified inline CR annotations using the exact `// CR 800.4: <description>`
format, after checking `docs/MagicCompRules.txt`, for the departure command
semantic boundary at crates/engine/src/types/resolved_commands.rs:350-357,
leave-node creation at :1301-1309, recording semantics at :1602-1608, and replay
validation at :2154-2164; replace the existing `///` rule references where
applicable and ensure each annotation accurately describes the covered behavior.
Sources: Coding guidelines, Path instructions
Comment on lines
+75
to
+133
| // The discriminating assertion: the departure is journaled as an exact | ||
| // resolved command. Two raw field writes record nothing here. | ||
| let leaves: Vec<_> = state | ||
| .resolved_rules_journal | ||
| .entries() | ||
| .iter() | ||
| .skip(journal_start) | ||
| .filter_map(|entry| entry.command.clone()) | ||
| .filter_map(|command| match command { | ||
| ResolvedRulesCommand::PlayerLeave(command) if command.player == P1 => Some(command), | ||
| _ => None, | ||
| }) | ||
| .collect(); | ||
| assert_eq!( | ||
| leaves.len(), | ||
| 1, | ||
| "the elimination authority must journal exactly one resolved departure" | ||
| ); | ||
|
|
||
| // The second claim: the departure runs under a PlayerLeave node, not under an | ||
| // ambient proposal. This is what lets a replay recognize the CR 800.4 sweep as | ||
| // one causal unit. | ||
| assert!( | ||
| matches!(leaves[0].cause, RulesExecutionNodeRef::PlayerLeave(_)), | ||
| "CR 800.4: the departure opens its own execution node, got {:?}", | ||
| leaves[0].cause | ||
| ); | ||
|
|
||
| // Replay-exactness: the recorded departure reinstalls both writes, and is not | ||
| // idempotent — a second application is a typed invariant failure rather than a | ||
| // silent re-elimination. | ||
| let mut replay = runner.state().clone(); | ||
| replay | ||
| .players | ||
| .iter_mut() | ||
| .find(|player| player.id == P1) | ||
| .expect("the player record survives elimination") | ||
| .is_eliminated = false; | ||
| replay.eliminated_players.retain(|player| *player != P1); | ||
|
|
||
| engine::game::elimination::apply_resolved_player_leave(&mut replay, &leaves[0]) | ||
| .expect("the recorded departure must replay"); | ||
| assert!( | ||
| replay | ||
| .players | ||
| .iter() | ||
| .find(|player| player.id == P1) | ||
| .expect("the player record survives elimination") | ||
| .is_eliminated, | ||
| "replay installs the recorded departure" | ||
| ); | ||
| assert!( | ||
| replay.eliminated_players.contains(&P1), | ||
| "replay installs the eliminated-list append" | ||
| ); | ||
| assert!( | ||
| engine::game::elimination::apply_resolved_player_leave(&mut replay, &leaves[0]).is_err(), | ||
| "a departure is not idempotent: re-applying it is a typed invariant failure" | ||
| ); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Exercise retained-prefix replay from before the departure.
This only replays the two direct fields after cloning the post-sweep state; all CR 800.4 effects are already present. Add a player-owned object/control-effect fixture, replay journal entries from a pre-elimination snapshot through the production dispatcher, and assert the sweep commands carry the PlayerLeave cause. Otherwise an incorrectly scoped or unreplayable departure sweep still passes.
🤖 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_player_leave.rs` around lines
75 - 133, Update the integration test around apply_resolved_player_leave to
capture a pre-elimination state snapshot and add a player-owned
object/control-effect fixture. Replay the journal entries from that snapshot
through the production dispatcher rather than directly replaying only the
departure fields, then assert the resulting CR 800.4 sweep commands carry a
RulesExecutionNodeRef::PlayerLeave cause and preserve the existing
replay/invariant checks.
Source: Path instructions
Comment on lines
+83
to
+86
| .filter_map(|command| match command { | ||
| ResolvedRulesCommand::PlayerLeave(command) if command.player == P1 => Some(command), | ||
| _ => None, | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Avoid the wildcard match on ResolvedRulesCommand.
ResolvedRulesCommand is a known enum; the wildcard prevents compiler assistance when variants change. Use a selective matches! filter plus a refutable let extraction, or enumerate variants explicitly.
🤖 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_player_leave.rs` around lines
83 - 86, Update the command filtering around ResolvedRulesCommand::PlayerLeave
to avoid the wildcard match: use a selective matches! predicate for PlayerLeave
commands belonging to P1, followed by refutable let extraction, or explicitly
enumerate all known enum variants. Preserve the existing collection of matching
PlayerLeave commands.
Sources: Coding guidelines, Path instructions
Parse changes introduced by this PR✓ No card-parse changes detected. |
This was referenced Jul 26, 2026
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.
Summary by CodeRabbit