fix(engine): play-from-exile permission is consultable from the graveyard for milled cards (#751) - #3168
Conversation
…yard for milled cards (#751)
There was a problem hiding this comment.
Code Review
This pull request introduces a regression test for GitHub issue #751 (Ark of Hunger / Tablet of Discovery) to verify that milled cards carrying play permissions can be correctly played or cast from the graveyard and that these permissions are properly pruned at the end of the turn. The feedback suggests refactoring the test helper mill_one_and_grant to replace the is_land: bool parameter with a typed enum, adhering to Style Guide Rule R2 to avoid boolean fields and improve type safety.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn mill_one_and_grant( | ||
| runner: &mut GameRunner, | ||
| library_card_name: &str, | ||
| is_land: bool, | ||
| mana_cost: ManaCost, | ||
| ) -> engine::types::identifiers::ObjectId { |
There was a problem hiding this comment.
The is_land: bool parameter should be replaced with a typed enum to better express the card's properties.
Why it matters: Using a bool is contrary to the spirit of Style Guide R2 ('No bool fields'), which favors typed enums for expressiveness and safety. It also leads to passing a meaningless ManaCost::NoCost for lands.
Suggested fix:
- Introduce a new enum to represent the milled card's properties:
enum MilledCardInfo {
Land,
Spell(ManaCost),
}- Update the
mill_one_and_grantfunction to accept this enum. The function body (lines 83-88) should be changed to amatchstatement on this new enum. - Update the call sites accordingly, e.g.,
MilledCardInfo::LandorMilledCardInfo::Spell(cost).
fn mill_one_and_grant(
runner: &mut GameRunner,
library_card_name: &str,
card_info: MilledCardInfo,
) -> engine::types::identifiers::ObjectId {References
- Rule R2 states that
boolfields should be avoided in favor of typed enums to better express the design space. While this is a function parameter and not a field, the principle of using typed enums for clarity and safety applies. (link)
Issue
Fixes #751. Ark of Hunger (and the class of "mill a card, then you may play it" cards — CR 701.17d) grants a
PlayFromExilepermission on a card that lands in the graveyard, but the engine's "can this object enter the cast path?" consult sites were gated toZone::Exileonly. The milled card carried a live permission yet was never castable/playable.Fix
Introduces a separate predicate
play_from_exile_object_in_cast_path(obj)=matches!(obj.zone, Zone::Exile | Zone::Graveyard) && !is_land, used at the three object-taggedPlayFromExileconsult sites plus a new graveyard cast-surface branch and a new graveyard land sweep.The original
exile_object_can_enter_cast_pathstaysZone::Exile-only, and the two static-ExileCastPermissioncallers are byte-for-byte unchanged — a card milled into a graveyard must not reach the static path (exile-only by CR 113.6b). Lands are excluded from the cast path (CR 305.1) and routed through the land-play sweep.Tests
crates/engine/tests/integration/ark_of_hunger_play_from_graveyard_751.rsdrives the real Mill→grant→cast pipeline:milled_card_is_castable_from_graveyard(discriminating — flips false→true on the fix)milled_land_is_playable_via_land_path_not_cast_path(CR 305.1 carve-out)exiled_card_with_play_permission_stays_on_exile_path(Advanced Reconstruction regression — gate is exile-OR-graveyard, never any-zone)graveyard_play_permission_is_pruned_at_end_of_turn(CR 514.2)All four PASS in local Tilt
test-engine. Independent/review-impl: APPROVED, zero findings. CR annotations (113.6b, 305.1, 514.2, 601.2a, 604.2, 701.17a, 701.17d, 715.3d) grep-verified against the Comprehensive Rules.