feat(engine): journal ordinary CR 111.1 token births as resolved commands - #6646
Conversation
…ands The token resolver drew an ObjectId from next_object_id and a CR 613.7d entry timestamp, then built the body in place with raw field writes. A retained-prefix replay had no record that the object existed at all, and re-running the resolver would draw a different id and timestamp -- handing out a colliding id and reordering the token against continuous effects in the layer system. This is the first family whose replay MATERIALIZES its subject instead of verifying and installing into an existing one, so the applier's precondition is inverted: the recorded id must be ABSENT, and re-applying fails closed rather than silently duplicating the token. The ordinary-token body block is extracted into materialize_token_spec_body, a pure function on &mut GameObject shared by the resolve path and the applier so the two cannot drift. Operating on the object rather than on GameState lets it serve both orderings -- resolve inserts first and mutates in place, replay builds a detached object and inserts it afterwards -- with no restructuring of the resolver. Counters, the attacking entry, and later status changes stay OUT of the command: they already journal through the counters, combat, and object-status families. SCOPE: ordinary TokenSpec births. Copy tokens (CR 707.2) and meld births go through the liminal-entry path, whose LiminalEntry carries no body spec (LiminalEntryKind distinguishes Token from Meld, not Spec from Copy), so wiring them needs a new field on that shared serialized struct. Documented on the command as the follow-up.
📝 WalkthroughWalkthroughToken creation now records a resolved journal command containing token identity and materialization data. Replay reconstructs the token through shared logic with invariant checks, battlefield insertion, and object-ID high-water updates. Integration dispatchers and tests cover exact replay and duplicate-application rejection. ChangesToken creation replay
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TokenEffect
participant ResolvedRulesJournal
participant ReplayDispatcher
participant GameState
TokenEffect->>ResolvedRulesJournal: record token creation
ReplayDispatcher->>GameState: apply_resolved_token_creation
GameState->>GameState: validate identity and allocation invariants
GameState->>GameState: materialize token and add to battlefield
GameState-->>ReplayDispatcher: replay result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 836-867: Update the token creation lookup around
materialize_token_spec_body to treat a missing state.objects entry as an
invariant violation: replace the optional get_mut/map flow with the same
expect("token just created") behavior used by the later lookup, then journal the
resulting ObjectIncarnationRef unconditionally through record_token_creation.
- Around line 1073-1087: Update the token replay restoration flow after
materialize_token_spec_body to advance state.next_timestamp using the replayed
command.entry_timestamp, preserving the timestamp allocator high-water mark
alongside state.next_object_id. Ensure subsequent timestamp allocation cannot
reuse the restored entry timestamp or produce reordered effects.
In `@crates/engine/tests/integration/cr733_resolved_token_creation.rs`:
- Around line 99-113: Extend the replay assertions in the test around the
resolved token to verify the recorded resulting_tapped state and
resulting_next_object_id high-water value, using the command and replay state
symbols already present. Add a second case for a tapped-entering non-vanilla
token such as Treasure so both true and false tapped outcomes are exercised,
while preserving the existing token, timestamp, P/T, and battlefield assertions.
- Around line 117-120: Update the re-applied token birth assertion in the
integration test to verify the exact ResolvedTokenCreationReplayInvariantError
variant representing duplicate/non-idempotent creation, rather than only
checking is_err(). Import ResolvedTokenCreationReplayInvariantError and match
the same specific variant used by the sibling resolved-commands test.
🪄 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: b7627240-de8a-4738-9cb5-5dc2d745a8a6
📒 Files selected for processing (6)
crates/engine/src/game/effects/token.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_token_creation.rscrates/engine/tests/integration/main.rs
| let created_reference = state.objects.get_mut(&obj_id).map(|obj| { | ||
| materialize_token_spec_body( | ||
| obj, | ||
| &spec, | ||
| token_image_ref.clone(), | ||
| turn_number, | ||
| entry_timestamp, | ||
| resulting_tapped, | ||
| ); | ||
| ObjectIncarnationRef::from_object(obj) | ||
| }); | ||
|
|
||
| // CR 733: journal the settled creation, after the body borrow ends. | ||
| // Counters, the attacking entry, and any later status change journal | ||
| // through their OWN families, so this command covers the birth only. | ||
| if let Some(object) = created_reference { | ||
| let cause = state.current_or_begin_rules_execution_node(); | ||
| let command = ResolvedTokenCreationCommand { | ||
| object, | ||
| owner, | ||
| entry_timestamp, | ||
| spec: spec.clone(), | ||
| token_image_ref: token_image_ref.clone(), | ||
| resulting_tapped, | ||
| resulting_next_object_id: state.next_object_id, | ||
| cause, | ||
| }; | ||
| state | ||
| .resolved_rules_journal | ||
| .record_token_creation(command) | ||
| .expect("resolved token creation must have a live journal cause"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Silent journal gap if the just-created object is missing — the same lookup panics at Line 984.
state.objects.get_mut(&obj_id) returning None is treated as "skip the body and skip the journal", so the token would exist on the battlefield (zones::create_object already inserted it) with no TokenCreation command recorded, and replay would silently omit it — a fail-open divergence. Line 981-985 in this same loop treats the identical lookup as an invariant with .expect("token just created"). Make this one consistent so the invariant fails loudly instead of producing an unreplayable journal.
♻️ Proposed fix
- let created_reference = state.objects.get_mut(&obj_id).map(|obj| {
- materialize_token_spec_body(
- obj,
- &spec,
- token_image_ref.clone(),
- turn_number,
- entry_timestamp,
- resulting_tapped,
- );
- ObjectIncarnationRef::from_object(obj)
- });
+ let created_reference = {
+ let obj = state
+ .objects
+ .get_mut(&obj_id)
+ .expect("token just created");
+ materialize_token_spec_body(
+ obj,
+ &spec,
+ token_image_ref.clone(),
+ turn_number,
+ entry_timestamp,
+ resulting_tapped,
+ );
+ ObjectIncarnationRef::from_object(obj)
+ };
// CR 733: journal the settled creation, after the body borrow ends.
// Counters, the attacking entry, and any later status change journal
// through their OWN families, so this command covers the birth only.
- if let Some(object) = created_reference {
- let cause = state.current_or_begin_rules_execution_node();
- let command = ResolvedTokenCreationCommand {
- object,
+ {
+ let cause = state.current_or_begin_rules_execution_node();
+ let command = ResolvedTokenCreationCommand {
+ object: created_reference,📝 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.
| let created_reference = state.objects.get_mut(&obj_id).map(|obj| { | |
| materialize_token_spec_body( | |
| obj, | |
| &spec, | |
| token_image_ref.clone(), | |
| turn_number, | |
| entry_timestamp, | |
| resulting_tapped, | |
| ); | |
| ObjectIncarnationRef::from_object(obj) | |
| }); | |
| // CR 733: journal the settled creation, after the body borrow ends. | |
| // Counters, the attacking entry, and any later status change journal | |
| // through their OWN families, so this command covers the birth only. | |
| if let Some(object) = created_reference { | |
| let cause = state.current_or_begin_rules_execution_node(); | |
| let command = ResolvedTokenCreationCommand { | |
| object, | |
| owner, | |
| entry_timestamp, | |
| spec: spec.clone(), | |
| token_image_ref: token_image_ref.clone(), | |
| resulting_tapped, | |
| resulting_next_object_id: state.next_object_id, | |
| cause, | |
| }; | |
| state | |
| .resolved_rules_journal | |
| .record_token_creation(command) | |
| .expect("resolved token creation must have a live journal cause"); | |
| } | |
| let created_reference = { | |
| let obj = state | |
| .objects | |
| .get_mut(&obj_id) | |
| .expect("token just created"); | |
| materialize_token_spec_body( | |
| obj, | |
| &spec, | |
| token_image_ref.clone(), | |
| turn_number, | |
| entry_timestamp, | |
| resulting_tapped, | |
| ); | |
| ObjectIncarnationRef::from_object(obj) | |
| }; | |
| // CR 733: journal the settled creation, after the body borrow ends. | |
| // Counters, the attacking entry, and any later status change journal | |
| // through their OWN families, so this command covers the birth only. | |
| { | |
| let cause = state.current_or_begin_rules_execution_node(); | |
| let command = ResolvedTokenCreationCommand { | |
| object: created_reference, | |
| owner, | |
| entry_timestamp, | |
| spec: spec.clone(), | |
| token_image_ref: token_image_ref.clone(), | |
| resulting_tapped, | |
| resulting_next_object_id: state.next_object_id, | |
| cause, | |
| }; | |
| state | |
| .resolved_rules_journal | |
| .record_token_creation(command) | |
| .expect("resolved token creation must have a live journal cause"); | |
| } |
🤖 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/token.rs` around lines 836 - 867, Update the
token creation lookup around materialize_token_spec_body to treat a missing
state.objects entry as an invariant violation: replace the optional get_mut/map
flow with the same expect("token just created") behavior used by the later
lookup, then journal the resulting ObjectIncarnationRef unconditionally through
record_token_creation.
| materialize_token_spec_body( | ||
| &mut object, | ||
| &command.spec, | ||
| command.token_image_ref.clone(), | ||
| state.turn_number, | ||
| command.entry_timestamp, | ||
| command.resulting_tapped, | ||
| ); | ||
|
|
||
| state.objects.insert(object_id, object); | ||
| // allow-raw-zone: replay materializes a token birth, which has no from-zone move (CR 111.1 + CR 614.12). | ||
| zones::add_to_zone(state, object_id, Zone::Battlefield, command.owner); | ||
| // CR 111.1: replay must not hand the same id out again to a later allocation. | ||
| state.next_object_id = state.next_object_id.max(command.resulting_next_object_id); | ||
| Ok(()) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C6 'fn next_timestamp' crates/engine/src/types/game_state.rs
rg -nP -C2 'next_timestamp' crates/engine/src --type=rustRepository: phase-rs/phase
Length of output: 44644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
ast-grep outline crates/engine/src/game/effects/token.rs --view expanded | sed -n '1,260p'
printf '\n--- resolved_commands ---\n'
ast-grep outline crates/engine/src/types/resolved_commands.rs --view expanded | sed -n '1,260p'
printf '\n--- token.rs around apply_resolved_token_creation ---\n'
sed -n '1000,1105p' crates/engine/src/game/effects/token.rs
printf '\n--- resolved_commands.rs token command definitions ---\n'
sed -n '1,280p' crates/engine/src/types/resolved_commands.rsRepository: phase-rs/phase
Length of output: 38865
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- search for next_timestamp high-water restoration in replay appliers ---'
rg -n "next_timestamp\s*=\s*state\.next_timestamp|max\(.*next_timestamp|next_timestamp.*max|resulting_timestamp|entry_timestamp" crates/engine/src/game crates/engine/src/types/resolved_commands.rs
printf '\n%s\n' '--- inspect zones::add_to_zone ---'
sed -n '740,840p' crates/engine/src/game/zones.rs
printf '\n%s\n' '--- inspect any replay applier that restores timestamp high-water ---'
rg -n "replay.*next_timestamp|max\(.*entry_timestamp|max\(.*resulting_timestamp|next_timestamp = .*max" crates/engine/src/game crates/engine/src/types | sed -n '1,220p'Repository: phase-rs/phase
Length of output: 8288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- transform replay applier ---'
sed -n '140,190p' crates/engine/src/game/transform.rs
printf '\n%s\n' '--- attach replay applier ---'
sed -n '1340,1385p' crates/engine/src/game/effects/attach.rs
printf '\n%s\n' '--- zone change replay applier tail ---'
sed -n '860,930p' crates/engine/src/game/zones.rs
printf '\n%s\n' '--- next_timestamp allocator resets / replay initialization ---'
rg -n "next_timestamp\s*=\s*0|next_timestamp\s*=\s*1|state\.next_timestamp|reset.*next_timestamp|replay" crates/engine/src | sed -n '1,220p'Repository: phase-rs/phase
Length of output: 32641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- materialize_token_spec_body and helpers ---'
sed -n '1090,1215p' crates/engine/src/game/effects/token.rs
printf '\n%s\n' '--- token creation resolve path around injection ---'
sed -n '760,920p' crates/engine/src/game/effects/token.rs
printf '\n%s\n' '--- ability injection helpers and replay-facing materializers ---'
sed -n '3120,3345p' crates/engine/src/game/effects/token.rsRepository: phase-rs/phase
Length of output: 21150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '800,890p' crates/engine/src/game/effects/token.rsRepository: phase-rs/phase
Length of output: 3779
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- replay state reconstruction flow ---'
sed -n '150,260p' crates/engine/src/game/replay.rs
printf '\n%s\n' '--- replay-related command application sites with next_timestamp usage ---'
rg -n "apply_resolved_.*\(|record_.*timestamp|next_timestamp" crates/engine/src/game/replay.rs crates/engine/src/types/replay.rs crates/engine/src/game/engine_replacement.rs crates/engine/src/game/engine_resolution_choices.rs | sed -n '1,240p'
printf '\n%s\n' '--- inspect GameState clone/reset behavior for next_timestamp ---'
sed -n '16770,16805p' crates/engine/src/types/game_state.rsRepository: phase-rs/phase
Length of output: 4418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- token replay test coverage around creation/replay ---'
sed -n '6030,6135p' crates/engine/src/game/effects/token.rs
printf '\n%s\n' '--- transform/zone-change replay invariants mentioning timestamps ---'
sed -n '1,240p' crates/engine/src/types/resolved_commands.rs | sed -n '130,240p'
sed -n '560,640p' crates/engine/src/types/resolved_commands.rs
printf '\n%s\n' '--- replay equality / checkpoint state includes next_timestamp ---'
sed -n '17660,17690p' crates/engine/src/types/game_state.rsRepository: phase-rs/phase
Length of output: 15348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ResolvedTokenCreationCommand|apply_resolved_token_creation\(" crates/engine/src | sed -n '1,200p'Repository: phase-rs/phase
Length of output: 836
Restore the timestamp allocator high-water. Replay reinstalls entry_timestamp and next_object_id, but leaves state.next_timestamp stale, so a resumed replay can hand out the same timestamp again and reorder later effects.
🤖 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/token.rs` around lines 1073 - 1087, Update the
token replay restoration flow after materialize_token_spec_body to advance
state.next_timestamp using the replayed command.entry_timestamp, preserving the
timestamp allocator high-water mark alongside state.next_object_id. Ensure
subsequent timestamp allocation cannot reuse the restored entry timestamp or
produce reordered effects.
| let replayed = &replay.objects[&token_id]; | ||
| assert!(replayed.is_token, "replay materializes a token"); | ||
| assert_eq!( | ||
| replayed.timestamp, birth.entry_timestamp, | ||
| "CR 613.7d: replay installs the recorded timestamp instead of re-drawing one" | ||
| ); | ||
| assert_eq!( | ||
| replayed.power, token.power, | ||
| "replay installs the same body the resolve path built" | ||
| ); | ||
| assert_eq!(replayed.toughness, token.toughness); | ||
| assert!( | ||
| replay.battlefield.contains(&token_id), | ||
| "replay adds the token to the battlefield zone list, not just the object map" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
resulting_tapped and the id high-water are recorded but never asserted.
The command carries resulting_tapped (CR 614.1) and resulting_next_object_id, and the applier acts on both — yet replay assertions cover only is_token, timestamp, and P/T. A vanilla Soldier enters untapped, so an applier that ignored resulting_tapped entirely would pass this test. Same for the next_object_id high-water restore, which is the whole point of recording the field.
💚 Proposed additional assertions
assert_eq!(replayed.toughness, token.toughness);
+ assert_eq!(
+ replayed.tapped, birth.resulting_tapped,
+ "CR 614.1: replay installs the recorded post-replacement tapped state"
+ );
+ assert_eq!(replayed.owner, birth.owner);
+ assert!(
+ replay.next_object_id >= birth.resulting_next_object_id,
+ "replay restores the id high-water so a later allocation cannot collide"
+ );
assert!(
replay.battlefield.contains(&token_id),A second case covering a tapped-entering token (and a non-vanilla token such as Treasure) would exercise the field's real range rather than its default.
📝 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.
| let replayed = &replay.objects[&token_id]; | |
| assert!(replayed.is_token, "replay materializes a token"); | |
| assert_eq!( | |
| replayed.timestamp, birth.entry_timestamp, | |
| "CR 613.7d: replay installs the recorded timestamp instead of re-drawing one" | |
| ); | |
| assert_eq!( | |
| replayed.power, token.power, | |
| "replay installs the same body the resolve path built" | |
| ); | |
| assert_eq!(replayed.toughness, token.toughness); | |
| assert!( | |
| replay.battlefield.contains(&token_id), | |
| "replay adds the token to the battlefield zone list, not just the object map" | |
| ); | |
| let replayed = &replay.objects[&token_id]; | |
| assert!(replayed.is_token, "replay materializes a token"); | |
| assert_eq!( | |
| replayed.timestamp, birth.entry_timestamp, | |
| "CR 613.7d: replay installs the recorded timestamp instead of re-drawing one" | |
| ); | |
| assert_eq!( | |
| replayed.power, token.power, | |
| "replay installs the same body the resolve path built" | |
| ); | |
| assert_eq!(replayed.toughness, token.toughness); | |
| assert_eq!( | |
| replayed.tapped, birth.resulting_tapped, | |
| "CR 614.1: replay installs the recorded post-replacement tapped state" | |
| ); | |
| assert_eq!(replayed.owner, birth.owner); | |
| assert!( | |
| replay.next_object_id >= birth.resulting_next_object_id, | |
| "replay restores the id high-water so a later allocation cannot collide" | |
| ); | |
| assert!( | |
| replay.battlefield.contains(&token_id), | |
| "replay adds the token to the battlefield zone list, not just the object map" | |
| ); |
🤖 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_token_creation.rs` around
lines 99 - 113, Extend the replay assertions in the test around the resolved
token to verify the recorded resulting_tapped state and resulting_next_object_id
high-water value, using the command and replay state symbols already present.
Add a second case for a tapped-entering non-vanilla token such as Treasure so
both true and false tapped outcomes are exercised, while preserving the existing
token, timestamp, P/T, and battlefield assertions.
Source: Path instructions
| assert!( | ||
| engine::game::effects::token::apply_resolved_token_creation(&mut replay, birth).is_err(), | ||
| "a token birth is not idempotent: re-applying it must fail closed" | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the specific invariant variant, not just is_err().
Any error satisfies this — including UnknownOwner or IdAboveHighWater from an unrelated regression — so the non-idempotence claim can pass for the wrong reason. The sibling test in cr733_resolved_commands_p2.rs (Line 196-199) pins the exact variant.
💚 Proposed fix
- assert!(
- engine::game::effects::token::apply_resolved_token_creation(&mut replay, birth).is_err(),
- "a token birth is not idempotent: re-applying it must fail closed"
- );
+ assert!(
+ matches!(
+ engine::game::effects::token::apply_resolved_token_creation(&mut replay, birth),
+ Err(ResolvedTokenCreationReplayInvariantError::ObjectAlreadyExists(id)) if id == token_id
+ ),
+ "a token birth is not idempotent: re-applying it must fail closed on the live id"
+ );Requires importing engine::types::resolved_commands::ResolvedTokenCreationReplayInvariantError.
🤖 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_token_creation.rs` around
lines 117 - 120, Update the re-applied token birth assertion in the integration
test to verify the exact ResolvedTokenCreationReplayInvariantError variant
representing duplicate/non-idempotent creation, rather than only checking
is_err(). Import ResolvedTokenCreationReplayInvariantError and match the same
specific variant used by the sibling resolved-commands test.
Parse changes introduced by this PR✓ No card-parse changes detected. |
The token resolver drew an ObjectId from next_object_id and a CR 613.7d entry
timestamp, then built the body in place with raw field writes. A retained-prefix
replay had no record that the object existed at all, and re-running the resolver
would draw a different id and timestamp -- handing out a colliding id and
reordering the token against continuous effects in the layer system.
This is the first family whose replay MATERIALIZES its subject instead of
verifying and installing into an existing one, so the applier's precondition is
inverted: the recorded id must be ABSENT, and re-applying fails closed rather
than silently duplicating the token.
The ordinary-token body block is extracted into materialize_token_spec_body, a
pure function on &mut GameObject shared by the resolve path and the applier so
the two cannot drift. Operating on the object rather than on GameState lets it
serve both orderings -- resolve inserts first and mutates in place, replay builds
a detached object and inserts it afterwards -- with no restructuring of the
resolver.
Counters, the attacking entry, and later status changes stay OUT of the command:
they already journal through the counters, combat, and object-status families.
SCOPE: ordinary TokenSpec births. Copy tokens (CR 707.2) and meld births go
through the liminal-entry path, whose LiminalEntry carries no body spec
(LiminalEntryKind distinguishes Token from Meld, not Spec from Copy), so wiring
them needs a new field on that shared serialized struct. Documented on the
command as the follow-up.
Summary by CodeRabbit
New Features
Bug Fixes
Tests