fix(engine): install both Room halves' static abilities and gate them by door lock (#7564) - #7573
Merged
Merged
Conversation
… by door lock (phase-rs#7564) CR 709.5: a locked half doesn't have the rules text of that half. Statics were still face-resident: the non-cast half's statics never existed for the engine (unlocking its door changed nothing), and a Room entering uncast (CR 709.5d: neither designation) wrongly applied its live face's statics while both doors were locked. - `StaticDefinition.room_door` (serde-defaulted): the half a static's printed text lives on; `install_room_door_text` now stamps and installs BOTH halves' statics into the base set (mirror of the phase-rs#7567 trigger install). - `room::door_text_functions`: shared single authority for the CR 709.5 lock gate (CR 709.5g: re-locking turns the text back off); the trigger iterator's inline gate now delegates to it. - Gate applied in `functioning_abilities::static_functions_in_zone` (covers every delegating statics gather) and in the documented layers exception (`active_continuous_effects_from_static_definitions`). Class (card-data.json): 10 Room halves carry statics — Access Maze, Dazzling Theater, Fractured Realm, Greenhouse, Lecture Hall, Porcelain Gallery, Secret Arcade, Solitary Study, Steaming Sauna, Walk-in Closet. Known remainder (phase-rs#7564): the two abilities-carrying halves are parser gaps, not lock-gating bugs — Prop Room's "Untap each creature you control during each other player's untap step" is not recognized by the Seedborn-class static parser (it accepts only "untap all ..."), and Warped Space's cost substitution is Unimplemented; door-gating for true activated abilities has no real member until such a half parses. Battlefield name merging (a locked half loses its NAME too) also stays open. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThis change adds optional Room door metadata to static definitions, stamps metadata onto both Room halves, and gates static and trigger functionality by door unlock state. Tests cover locked, unlocked, re-locked, uncast, and off-battlefield behavior. Existing fixtures initialize the new field to ChangesRoom door functionality
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change gates Room static abilities by door lock and installs both halves’ abilities; reported tests and clippy checks are clean, so no actionable merge-blocking risk remains after normal checks. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant RoomSetup
participant GameObject
participant Room
participant AbilityEvaluation
participant Layers
RoomSetup->>GameObject: install_room_door_text
GameObject->>GameObject: stamp static definitions with RoomDoor
AbilityEvaluation->>Room: door_text_functions
Room-->>AbilityEvaluation: return unlock-state result
AbilityEvaluation->>Layers: collect functioning continuous effects
Layers-->>AbilityEvaluation: exclude locked door-stamped effects
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.1)crates/engine/src/game/casting_tests.rsast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 20, 2026
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
approved these changes
Aug 21, 2026
matthewevans
left a comment
Member
There was a problem hiding this comment.
Approved: Room static text is installed at the base-definition seam and gated by the existing Room unlock designations across active statics, triggers, and continuous layers. The current-head runtime coverage exercises locked, unlocked, re-locked, uncast, and off-battlefield behavior; the parse-diff for this head reports no card-parse changes.
This was referenced Aug 21, 2026
matthewevans
pushed a commit
to JacobWoodson/phase
that referenced
this pull request
Aug 24, 2026
…rped Space) (phase-rs#7782) * feat(parser,engine): lower the origin-zone alternative-cost grant (Warped Space) "Once each turn, you may pay {0} rather than pay the mana cost for a spell you cast from exile." lowered to `Effect::Unimplemented`: `parse_spells_alternative_cost` strict-failed on the origin-zone qualifier after "spell you cast" (issue phase-rs#7575). Parser: the zone qualifier the keyword-grant subject walk already recognizes ("from exile"/"from your hand" → `FilterProp::InZone`) is now one shared authority (`parse_cast_origin_zone_qualifier`) used by both paths; the alternative-cost lowering consumes it before the mana-value gate and fills the `zone_filter` slot that `apply_spell_keyword_subject_constraints` already carries. Singular subjects peel their article, and a color-quality word routes through the shared `peel_color_quality_prefix` — `parse_type_phrase` drops it, which would have silently over-broadened Darksteel Monolith's grant to any spell. Engine: `payable_spell_alternative_cost_details` gated ALL granted alternative costs to hand casts. A grant whose own `affected` filter constrains the cast's origin zone (`InZone`/`InAnyZone`) has already matched that origin, so it now reaches the authorized non-hand cast; the gate reads the origin-zone authority (`pending_cast_origin_zone_for`) rather than the object's current zone. Zone-LESS grants (Rooftop Storm, Fist of Suns, Jodah, As Foretold) keep the hand-only reach unchanged — widening them to permission-authorized graveyard/exile casts is CR-plausible (CR 118.9) but a separate, separately measured change. Class (Regel 13, all 35,798 cards double-parsed, fix in vs. out): exactly 4 cards change — Warped Space, Dragon's Smile, Tlincalli Hunter ("from exile"), Darksteel Monolith ("a colorless spell you cast from your hand"). All four lowered to Unimplemented before. Counter-proofs (abort-guarded probes, run separately): parser half removed → the three zone unit tests and the exile integration test fail, the zone-less pins stay green; engine half removed → parser tests stay green, exactly the exile integration test fails. Tests: 4 parser unit tests (incl. an As Foretold zone-less pin and the colorless-constraint guard), 1 grant unit probe (exile-scoped filter reaches an exile card), 2 integration tests (empty-pool exile cast pays {0} via the OptionalCostChoice, second cast that turn is denied; hand cast stays out of the exile-scoped grant's reach). Not covered: - `payable_spell_alternative_cost_for_timing` keeps its hand-only gate; no timing-permission grant in the class carries a zone scope. - The choice arm still fires only for `CastingVariant::Normal` casts; a face-down or otherwise variant-elected exile cast does not receive the {0} choice (no card in the measured class needs it). - Room door gating is phase-rs#7573's merged concern; these tests host the static on a plain enchantment. Fixes phase-rs#7575 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(engine,parser): review round — branch-scoped origin authority, strict grammar Review finds (matthewevans, CodeRabbit): 1. Branch-scoped origin authority. `origin_zone_scoped: bool` was a whole-filter presence bit: `Or(hand-scoped, Creature)` matched by an exile Creature through the unscoped branch would have unlocked the non-hand grant. The bit is gone; a non-hand origin now matches only THROUGH an origin-scoped branch (`matches_via_origin_scoped_branch`, fully recursive over Or/And/TrackedSetFiltered, exhaustive match with no wildcard arm), decided inside the grant lookup itself. Regression: a_mixed_or_grant_stays_hand_only_for_the_unscoped_branch — counter-proof: with a plain match in the non-hand arm it fails. 2. Strict, case-aware grammar. Article and color-quality prefixes parse on the LOWERCASE view (end-anchored original tail for the preserved type words); the shared zone qualifier requires a word boundary, so "from exiled" no longer matches "from exile". 3. Regression evidence. The once-per-turn slot is proven to RESET on the caster's next turn (full turn cycle, fresh {0} choice); the hand-zone negative carries a positive reach guard in the same fixture and runs before the slot is spent; the PendingCast origin override is pinned with the object ON the stack (exile origin receives the grant, graveyard origin is a zone mismatch). 4. CR annotations. The resolve_top comment cites CR 117.1a — CR 302.1 is the hand-specific creature rule, and the permission replaces only the zone clause (CR 101.1); `pending_cast_origin_zone_for` loses its stray CR 603.4 (intervening-if) citation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(engine): one origin-zone chain for every spell-grant walker Review find (CodeRabbit, phase-rs#7782 round 2): the alternative-cost grant resolved the cast origin through `pending_cast_origin_zone_for` alone, while the keyword-grant walkers prefer the `cast_from_zone` stamp — which survives `finalize_cast` when the pending record is cleared. A re-ask after finalize would have seen `Zone::Stack` and dropped the grant. The triple chain (`cast_from_zone` → pending record → current zone) is now one named authority, `spell_cast_origin_zone`, used by all four walkers (both keyword-grant fns, the alternative-cost grant, and the offer gate in `payable_spell_alternative_cost_details`) — the two hand-rolled copies in the keyword walkers are replaced by it. Pin: `the_stamped_cast_from_zone_survives_finalize_for_the_grant` — a finalized hand cast keeps its zone-less (Rooftop Storm class) grant, and a finalized exile cast keeps the exile-scoped one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(engine): route the origin chain through the persisted-origin authority Review find (matthewevans, phase-rs#7782 round 2 — crossed with `548a5d41b`, which unified the chain but read only the object's `cast_from_zone` field): the persisted origin has TWO storage sites, and `spell_cast_origin` already owns that split — the stack entry's `ResolvedAbility.context.cast_from_zone` for instants/sorceries, the object field for permanent placeholders. `spell_cast_origin_zone` now resolves through that authority first, so a finalized instant/sorcery cast keeps its origin for grant evaluation exactly like a permanent does; the pending record and the current zone stay as fallbacks. The ability-context leg is `spell_cast_origin`'s own tested behavior (trigger matcher); the chain wiring is pinned by `the_stamped_cast_from_zone_survives_finalize_for_the_grant`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(engine): current cast outranks the stamp; the stamp dies on stack exit Review finds (phase-rs#7782 round 3, CodeRabbit — the stale-`cast_from_zone` window): 1. Order. `spell_cast_origin_zone` consulted the persisted stamp before the pending record. The stamp is written at finalize, so during a NEW cast it can only describe a PREVIOUS cast of this object — a graveyard recast with a leftover Hand stamp would have received a zone-less (hand-reach) grant. The in-flight pending record now outranks it. Pin: a_pending_recast_outranks_a_stale_cast_from_zone_stamp. 2. Lifetime. The battlefield already owns its two resets (`reset_for_battlefield_entry` — restored from `CastLinkSnapshot` for genuine cast resolutions — and `reset_for_battlefield_exit`), but a spell leaving the STACK for a non-battlefield zone (countered, fizzled, instant/sorcery to the graveyard) kept its stamp. The zone-transition cleanup now clears `cast_from_zone` on every entry except Stack and Battlefield (CR 400.7). Pin: the_cast_from_zone_stamp_dies_off_stack_and_battlefield. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(engine): replay installs the same cast-origin lifetime as the live path Review finds (phase-rs#7782 round 4, matthewevans + CodeRabbit): 1. Replay equivalence. `apply_resolved_zone_change` installs a replayed transition directly and never ran the live path's cast-origin clear — a replayed stamped Stack→Graveyard kept the stale stamp the live transition removed. The clear is now ONE primitive (`clear_cast_origin_off_provenance_zones`), called by the live transition cleanup and by the replay applier's non-battlefield branch, so both paths agree by construction. Pin: a_replayed_stack_exit_clears_the_stamp_like_the_live_one (live command recorded, replayed onto a pre-move clone, both stamps gone). 2. Documentation. `spell_cast_origin_zone`'s outer doc said persisted-first while the code (correctly) resolves the in-flight pending record first; the doc now states the actual precedence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
2 participants
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.
Fixes the statics rows of #7564 (the trigger rows landed in #7567).
CR 709.5: a locked half doesn't have the rules text of that half. Statics were still face-resident: the non-cast half's statics never existed for the engine (unlocking its door changed nothing), and a Room entering uncast (CR 709.5d: neither designation) wrongly applied its live face's statics while both doors were locked.
Change
StaticDefinition.room_door(serde-defaulted; mirror ofTriggerDefinition.room_doorfrom fix(engine): fire the unlocked door's own trigger, and install both Room halves' door text (#7564) #7567): stamped byinstall_room_door_text, which now installs BOTH halves' statics into the base set.room::door_text_functions: shared single authority for the lock gate (CR 709.5c; CR 709.5g — re-locking turns the text back off). The trigger iterator's inline gate now delegates to it.static_functions_in_zone(covers every delegating statics gather) and in the documented layers exception (active_continuous_effects_from_static_definitions).Class (card-data.json): 10 Room halves carry statics — Access Maze, Dazzling Theater, Fractured Realm, Greenhouse, Lecture Hall, Porcelain Gallery, Secret Arcade, Solitary Study, Steaming Sauna, Walk-in Closet.
Evidence
AdditionalLandDrop), uncast entry (CR 709.5d), layers-seam anthem on/off/on-lock, helper-levelactive_static_definitionsgate incl. off-battlefield non-gating.What the tests do NOT prove: the off-battlefield assert uses a synthetic
active_zones=[Graveyard]static — no real Room half functions off-battlefield today.game_functioning_statics/battlefield_functioning_staticsare covered via the shared predicate, not per-consumer.Known remainder (stays on #7564): battlefield NAME merging (a locked half loses its name too — display-side). The two
abilities-carrying halves (Prop Room, Warped Space) turned out to be parser gaps, not lock-gating bugs — filing separately; activated-ability door gating has no real member until such a half parses.🤖 Generated with Claude Code
Summary by CodeRabbit