Skip to content

Commit 82d5ebf

Browse files
committed
fix(engine): make unresolved ParentObjectTargetController restriction fail-closed, not panic
Address maintainer review: the enforcement arm in casting::restriction_scope_matches_player did debug_assert!(false) for RestrictionPlayerScope::ParentObjectTargetController, but add_restriction's fill_runtime_fields can legitimately leave that scope unresolved when there is no object referent (proven by parent_object_target_controller_unresolved_without_object_target). That made the documented fail-closed path unsafe: a castability query against that stored state would panic in debug/test builds instead of restricting no one. Remove the debug assertion for this arm so it genuinely returns false (fail-closed). Unlike the always-resolved sibling scopes (TargetedPlayer, ScopedPlayer), this scope can reachably remain unresolved, so an unresolved value is a valid state, not a bug. Add an integration test that drives the public can_cast_object_now against the stored unresolved restriction and asserts it restricts no one without panicking — proving the fail-closed behavior, not just the stored enum shape. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01WQcP6woAkU5iEBTkA9oQ6J
1 parent 3b23999 commit 82d5ebf

2 files changed

Lines changed: 66 additions & 8 deletions

File tree

crates/engine/src/game/casting.rs

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -510,14 +510,16 @@ fn restriction_scope_matches_player(
510510
false
511511
}
512512
RestrictionPlayerScope::ParentObjectTargetController => {
513-
// CR 109.4: resolved to `SpecificPlayer` by `add_restriction` when the
514-
// restriction is created (via `parent_target_controller`), so an
515-
// unresolved scope here means the object referent could not be found —
516-
// fail-closed, restrict no one.
517-
debug_assert!(
518-
false,
519-
"ParentObjectTargetController should be resolved by add_restriction"
520-
);
513+
// CR 109.4: normally resolved to `SpecificPlayer` by `add_restriction`
514+
// (via `parent_target_controller`) when the restriction is created.
515+
// Unlike the always-resolved sibling scopes (`TargetedPlayer`,
516+
// `ScopedPlayer`), this one can legitimately remain unresolved when
517+
// there is no object referent — a malformed or hostile state, proven
518+
// reachable by `add_restriction`'s
519+
// `parent_object_target_controller_unresolved_without_object_target`.
520+
// That is a genuine fail-closed outcome (restrict no one), NOT a bug,
521+
// so this arm must return `false` rather than `debug_assert!(false)` —
522+
// a debug/test panic here would break the documented fail-closed path.
521523
false
522524
}
523525
RestrictionPlayerScope::OpponentsOfSourceController => {

crates/engine/tests/integration/render_silent_cant_cast.rs

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
1717
use engine::game::casting::can_cast_object_now;
1818
use engine::game::scenario::{GameRunner, GameScenario, P0, P1};
19+
use engine::types::ability::{
20+
GameRestriction, ProhibitedActivity, RestrictionExpiry, RestrictionPlayerScope,
21+
};
1922
use engine::types::card_type::CoreType;
2023
use engine::types::game_state::{CastingVariant, StackEntry, StackEntryKind};
2124
use engine::types::identifiers::{CardId, ObjectId};
@@ -53,6 +56,59 @@ fn put_spell_on_stack(runner: &mut GameRunner, controller: PlayerId) -> ObjectId
5356
spell
5457
}
5558

59+
/// CR 109.4 fail-closed enforcement: if an "its controller can't cast spells"
60+
/// restriction is ever stored with an UNRESOLVED `ParentObjectTargetController`
61+
/// scope (no object referent — the malformed/hostile state that
62+
/// `add_restriction`'s `parent_object_target_controller_unresolved_without_object_target`
63+
/// proves `fill_runtime_fields` can leave), a later castability query must return
64+
/// "unrestricted" and MUST NOT panic. This drives the public `can_cast_object_now`
65+
/// against exactly that stored state — it panicked before the enforcement arm's
66+
/// `debug_assert!(false)` was removed, and now returns fail-closed (restrict no
67+
/// one). Guards `casting::restriction_scope_matches_player`.
68+
#[test]
69+
fn unresolved_parent_object_target_controller_restriction_is_fail_closed() {
70+
let mut scenario = GameScenario::new();
71+
scenario.at_phase(Phase::PreCombatMain);
72+
73+
let mut p0_spell = scenario.add_spell_to_hand_from_oracle(P0, "P0 Spell", true, "Draw a card.");
74+
p0_spell.with_mana_cost(ManaCost::Cost {
75+
generic: 0,
76+
shards: vec![ManaCostShard::Blue],
77+
});
78+
let p0_spell = p0_spell.id();
79+
scenario.add_basic_land(P0, ManaColor::Blue);
80+
81+
let mut runner = scenario.build();
82+
83+
// Baseline: castable before any restriction exists.
84+
assert!(
85+
can_cast_object_now(runner.state(), P0, p0_spell),
86+
"sanity: P0's spell must be castable before the restriction is stored"
87+
);
88+
89+
// Store the hostile UNRESOLVED restriction directly — the exact state the
90+
// sibling unit test proves `fill_runtime_fields` leaves when there is no
91+
// object referent (scope stays `ParentObjectTargetController`, never lowered
92+
// to `SpecificPlayer`).
93+
runner
94+
.state_mut()
95+
.restrictions
96+
.push(GameRestriction::ProhibitActivity {
97+
source: ObjectId(9999),
98+
affected_players: RestrictionPlayerScope::ParentObjectTargetController,
99+
expiry: RestrictionExpiry::EndOfTurn,
100+
activity: ProhibitedActivity::CastSpells { spell_filter: None },
101+
});
102+
103+
// Fail-closed: the unresolved scope restricts NO ONE and must not panic. This
104+
// call routes through `restriction_scope_matches_player`'s
105+
// `ParentObjectTargetController` arm.
106+
assert!(
107+
can_cast_object_now(runner.state(), P0, p0_spell),
108+
"an unresolved ParentObjectTargetController restriction must restrict no one (fail-closed)"
109+
);
110+
}
111+
56112
/// Put a spell on the stack whose OWNER and CONTROLLER differ: `owner` owns the
57113
/// card (so a counter sends it to `owner`'s graveyard, CR 701.6a) while
58114
/// `controller` controls it on the stack (e.g. a spell cast from another

0 commit comments

Comments
 (0)