ship/cr733 p2 scalar status - #6339
Conversation
CR 119.8: gaining or losing 0 life doesn't count as a life change. The ordinary path legitimately reaches the post-replacement appliers with amount 0 (X=0 spells, fails-closed paths); the ZeroDelta-rejecting applier turned that no-op into a panic. Events and layer marks keep their pre-P2 unconditional behavior.
📝 WalkthroughWalkthroughAdds typed, journaled player-resource and object-status commands. Direct mutations for life, energy, counters, speed, tapping, untapping, and exerting now use centralized authority appliers with replay invariant checks. Integration tests cover command replay, malformed payloads, stale incarnations, precondition mismatches, and underflow. ChangesResolved command authority
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RuleEffect
participant GameState
participant ObjectState
participant ResolvedRulesJournal
RuleEffect->>GameState: resolve_and_apply_player_edit
GameState->>GameState: apply_resolved_player_edit
GameState->>ResolvedRulesJournal: record_player_edit
RuleEffect->>ObjectState: resolve_and_apply_object_edit
ObjectState->>ObjectState: apply_resolved_object_edit
ObjectState->>ResolvedRulesJournal: record_object_status
Possibly related PRs
Suggested labels: 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/engine_replacement.rs (1)
425-452: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResumed Tap/Untap arms check
.is_ok()instead of the transition bool — can emit a spurious event.
resolve_and_apply_object_editreturnsOk(false)when the object is already in the requested tapped state (a legal no-op — CR 701.26a: tapping an already-tapped permanent causes no state change)..is_ok()istruefor bothOk(true)andOk(false), so these arms will pushPermanentTapped/PermanentUntappedeven when nothing actually changed. Every sibling implementation in this PR (tap_untap.rs::process_one_tap/process_one_untap,restrictions.rs::tap_permanent_for_cost,casting_costs.rs::auto_tap_mana_sources_inner) correctly gates the event on the returned bool instead. This is reachable:process_one_tapproposesProposedEvent::Tapwithout checkingobj.tappedfirst, so a replacement-ordering pause on an already-tapped target's Tap event resumes here and fires a spuriousPermanentTapped.🐛 Proposed fix
ProposedEvent::Tap { object_id, .. } => { - if crate::game::object_state::resolve_and_apply_object_edit( + if crate::game::object_state::resolve_and_apply_object_edit( state, object_id, crate::types::resolved_commands::ResolvedObjectStatus::Tapped, true, ) - .is_ok() + .unwrap_or(false) { events.push(GameEvent::PermanentTapped { object_id, caused_by: None, }); } } // CR 701.26b: Untap accepted after replacement choice. ProposedEvent::Untap { object_id, .. } => { - if crate::game::object_state::resolve_and_apply_object_edit( + if crate::game::object_state::resolve_and_apply_object_edit( state, object_id, crate::types::resolved_commands::ResolvedObjectStatus::Tapped, false, ) - .is_ok() + .unwrap_or(false) { events.push(GameEvent::PermanentUntapped { object_id }); } }🤖 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/engine_replacement.rs` around lines 425 - 452, Update the resumed ProposedEvent::Tap and ProposedEvent::Untap arms to emit their GameEvent only when resolve_and_apply_object_edit returns Ok(true), not merely Ok(_). Preserve error handling and ensure already-tapped or already-untapped no-op transitions do not push PermanentTapped or PermanentUntapped.
🧹 Nitpick comments (2)
crates/engine/src/game/engine_debug.rs (1)
616-698: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared before/after/actual-delta derivation.
apply_player_counter_deltaandapply_energy_deltaduplicate the same checked_add(positive)/saturating_sub(negative)/i32::try_from(i64 diff)logic, differing only in the field read and theResolvedPlayerEditvariant built. A small shared helper (e.g.fn saturating_scalar_delta(before: u32, delta: i32) -> (u32, i32)) would remove the duplication and keep the two thin wrappers focused on theirResolvedPlayerEditvariant.♻️ Proposed refactor
+fn saturating_scalar_delta(before: u32, delta: i32) -> (u32, i32) { + let after = if delta.is_positive() { + before + .checked_add(delta as u32) + .expect("debug scalar addition must not overflow") + } else { + before.saturating_sub(delta.unsigned_abs()) + }; + let actual_delta = i32::try_from(i64::from(after) - i64::from(before)) + .expect("a requested i32 scalar delta must remain representable"); + (after, actual_delta) +} + fn apply_player_counter_delta(...) { let Some(before) = ... else { return; }; - let after = if delta.is_positive() { ... } else { ... }; - let actual_delta = i32::try_from(...).expect(...); + let (after, actual_delta) = saturating_scalar_delta(before, delta); + let _ = after; // retained only if needed elsewhere if actual_delta != 0 { ... } }🤖 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/engine_debug.rs` around lines 616 - 698, Extract the duplicated before/after/actual-delta calculation from apply_player_counter_delta and apply_energy_delta into a shared helper such as saturating_scalar_delta(before: u32, delta: i32) returning (u32, i32). Preserve the existing checked positive addition, saturating negative subtraction, and i32 conversion behavior, then keep each wrapper focused on reading its field and constructing its respective ResolvedPlayerEdit variant.crates/engine/src/game/casting_costs.rs (1)
6371-6384: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse the safe player lookup instead of raw index arithmetic.
state.players[player.0 as usize]assumesPlayerIdnumeric value == vector index. That invariant holds today, but it's a deviation from the pattern used for the identical operation elsewhere in this same PR —engine_resolution_choices.rs'sPayableResource::Energyarm andgame_state.rs's ownapply_resolved_player_editboth use.iter().find(|p| p.id == player). Prefer the same safe lookup here to avoid a panic vector if that invariant is ever broken (e.g. player removal/reordering) and for consistency.🛡️ Proposed fix
- let energy = state.players[player.0 as usize].energy; + let energy = state + .players + .iter() + .find(|candidate| candidate.id == player) + .map(|candidate| candidate.energy) + .unwrap_or(0);🤖 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/casting_costs.rs` around lines 6371 - 6384, Replace the raw `state.players[player.0 as usize]` access in the energy-payment logic with the established safe lookup that searches players by `p.id == player`, and read the matched player’s energy before applying the existing validation and deduction flow.
🤖 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/engine_combat.rs`:
- Around line 221-230: Remove the early return based on the exerted result in
the combat exert flow. Keep resolve_and_apply_object_edit’s boolean only for
deduplicating the journal/status update, while always dispatching the
CreatureExerted event and installing its continuous effect for every legal exert
choice, including already-exerted creatures.
In `@crates/engine/src/game/turns.rs`:
- Around line 1372-1381: Update both untap-pass branches around
resolve_and_apply_object_edit to handle a failed resolved edit without
panicking: replace the expect-based assertion with graceful error handling that
skips the object and emits no PermanentUntapped event when the target is missing
or stale. Preserve the existing event behavior when the edit succeeds.
---
Outside diff comments:
In `@crates/engine/src/game/engine_replacement.rs`:
- Around line 425-452: Update the resumed ProposedEvent::Tap and
ProposedEvent::Untap arms to emit their GameEvent only when
resolve_and_apply_object_edit returns Ok(true), not merely Ok(_). Preserve error
handling and ensure already-tapped or already-untapped no-op transitions do not
push PermanentTapped or PermanentUntapped.
---
Nitpick comments:
In `@crates/engine/src/game/casting_costs.rs`:
- Around line 6371-6384: Replace the raw `state.players[player.0 as usize]`
access in the energy-payment logic with the established safe lookup that
searches players by `p.id == player`, and read the matched player’s energy
before applying the existing validation and deduction flow.
In `@crates/engine/src/game/engine_debug.rs`:
- Around line 616-698: Extract the duplicated before/after/actual-delta
calculation from apply_player_counter_delta and apply_energy_delta into a shared
helper such as saturating_scalar_delta(before: u32, delta: i32) returning (u32,
i32). Preserve the existing checked positive addition, saturating negative
subtraction, and i32 conversion behavior, then keep each wrapper focused on
reading its field and constructing its respective ResolvedPlayerEdit variant.
🪄 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: 96bb46dd-f141-4913-bdb5-e87a53a16996
📒 Files selected for processing (24)
.agents/cr733/RUN6-REPORT.mdcrates/engine/src/game/casting_costs.rscrates/engine/src/game/combat.rscrates/engine/src/game/costs.rscrates/engine/src/game/effects/energy.rscrates/engine/src/game/effects/life.rscrates/engine/src/game/effects/player_counter.rscrates/engine/src/game/effects/rad_counters.rscrates/engine/src/game/effects/tap_untap.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_combat.rscrates/engine/src/game/engine_debug.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/mod.rscrates/engine/src/game/object_state.rscrates/engine/src/game/restrictions.rscrates/engine/src/game/speed.rscrates/engine/src/game/stickers.rscrates/engine/src/game/turns.rscrates/engine/src/types/game_state.rscrates/engine/src/types/mod.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/cr733_resolved_commands_p2.rs
| let exerted = crate::game::object_state::resolve_and_apply_object_edit( | ||
| state, | ||
| attacker, | ||
| crate::types::resolved_commands::ResolvedObjectStatus::Exerted, | ||
| true, | ||
| ) | ||
| .expect("declared attacker must remain a live exact object"); | ||
| if !exerted { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Re-exerting an already-exerted creature must still fire its "when you do" trigger — the early return drops it.
CR 701.43b: "A permanent can be exerted even if it's not tapped or has already been exerted in a turn." Exerting a second time this turn is a legal, distinct choice whose linked "when you do" ability (CR 701.43d + 607.2h) is expected to trigger again — only the redundant "won't untap" status is deduplicated, not the event. resolve_and_apply_object_edit returning Ok(false) for the already-exerted case correctly skips the journal/status write, but gating the CreatureExerted event and the continuous-effect install on that same bool means the trigger silently never fires on a repeat exert (e.g. a vigilant exert creature attacking twice via an extra combat). Tap's idempotent-no-op semantics don't transfer to exert, whose flag is a once-per-turn status but whose action/trigger fires every time it's chosen.
🐛 Proposed fix — decouple the journal no-op from the event/effect
- let exerted = crate::game::object_state::resolve_and_apply_object_edit(
- state,
- attacker,
- crate::types::resolved_commands::ResolvedObjectStatus::Exerted,
- true,
- )
- .expect("declared attacker must remain a live exact object");
- if !exerted {
- return;
- }
+ crate::game::object_state::resolve_and_apply_object_edit(
+ state,
+ attacker,
+ crate::types::resolved_commands::ResolvedObjectStatus::Exerted,
+ true,
+ )
+ .expect("declared attacker must remain a live exact object");
state.add_transient_continuous_effect(📝 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 exerted = crate::game::object_state::resolve_and_apply_object_edit( | |
| state, | |
| attacker, | |
| crate::types::resolved_commands::ResolvedObjectStatus::Exerted, | |
| true, | |
| ) | |
| .expect("declared attacker must remain a live exact object"); | |
| if !exerted { | |
| return; | |
| } | |
| crate::game::object_state::resolve_and_apply_object_edit( | |
| state, | |
| attacker, | |
| crate::types::resolved_commands::ResolvedObjectStatus::Exerted, | |
| true, | |
| ) | |
| .expect("declared attacker must remain a live exact object"); |
🤖 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/engine_combat.rs` around lines 221 - 230, Remove the
early return based on the exerted result in the combat exert flow. Keep
resolve_and_apply_object_edit’s boolean only for deduplicating the
journal/status update, while always dispatching the CreatureExerted event and
installing its continuous effect for every legal exert choice, including
already-exerted creatures.
| } else if crate::game::object_state::resolve_and_apply_object_edit( | ||
| state, | ||
| object_id, | ||
| crate::types::resolved_commands::ResolvedObjectStatus::Tapped, | ||
| false, | ||
| ) | ||
| .expect("untap-step object must remain a live exact object") | ||
| { | ||
| events.push(GameEvent::PermanentUntapped { object_id }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
.expect() on the resolved untap edit can panic if a target object leaves the battlefield mid-loop.
Both untap passes snapshot to_untap before the loop, then call replace_event per object (which can run replacement/trigger side effects) before applying the tap-status transition. If an earlier iteration's side effects remove a later target from state.objects (or change its incarnation), resolve_and_apply_object_edit returns Err(UnknownObject/StaleObject/...), and .expect("... must remain a live exact object") panics — crashing the whole engine during a core, every-turn code path.
This is inconsistent with the sibling stun-counter-removal branch in the very same loop, which already handles a possibly-missing object defensively via state.objects.get(&object_id).is_some_and(...) / if let Some(obj) = state.objects.get_mut(&object_id) rather than panicking. Tracing further: has_stun itself defaults to false for a missing object, which funnels straight into the else if branch that panics — so the "object still exists" invariant is silently assumed exactly where it's least certain to hold.
Recommend treating a resolved-edit Err the same way the stun branch treats a missing object: skip gracefully (no event) instead of asserting liveness.
🛡️ Proposed fix (apply to both occurrences)
- } else if crate::game::object_state::resolve_and_apply_object_edit(
- state,
- object_id,
- crate::types::resolved_commands::ResolvedObjectStatus::Tapped,
- false,
- )
- .expect("untap-step object must remain a live exact object")
- {
+ } else if matches!(
+ crate::game::object_state::resolve_and_apply_object_edit(
+ state,
+ object_id,
+ crate::types::resolved_commands::ResolvedObjectStatus::Tapped,
+ false,
+ ),
+ Ok(true)
+ ) {
events.push(GameEvent::PermanentUntapped { object_id });
}Also applies to: 1702-1711
🤖 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/turns.rs` around lines 1372 - 1381, Update both
untap-pass branches around resolve_and_apply_object_edit to handle a failed
resolved edit without panicking: replace the expect-based assertion with
graceful error handling that skips the object and emits no PermanentUntapped
event when the target is missing or stale. Preserve the existing event behavior
when the edit succeeds.
* cr733(p2): apply resolved scalar and status commands * cr733(p2): record scalar and status tranche * cr733(p2): skip the journal command for zero-amount life changes CR 119.8: gaining or losing 0 life doesn't count as a life change. The ordinary path legitimately reaches the post-replacement appliers with amount 0 (X=0 spells, fails-closed paths); the ZeroDelta-rejecting applier turned that no-op into a panic. Events and layer marks keep their pre-P2 unconditional behavior. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes
Tests