Fix(card) skyclave apparition - #70
Merged
matthewevans merged 15 commits intoApr 22, 2026
Merged
Conversation
matthewevans
left a comment
Member
There was a problem hiding this comment.
Thanks for the contribution — this is a nice step toward handling linked-exile follow-ups like Skyclave Apparition. I left two small inline notes where I think the runtime and parser shape still need attention.
Member
|
Thanks for this contribution, @BartlomiejSzczotka! 🙏 I pushed two follow-up commits directly to your branch to get Skyclave across the finish line:
A couple of known follow-ups (not blocking this PR):
Thanks again — this was a great contribution and a good forcing function to build out the linked-exile snapshot infrastructure. 🎉 |
matthewevans
enabled auto-merge (squash)
April 22, 2026 20:56
real-venus
pushed a commit
to real-venus/phase
that referenced
this pull request
Jul 13, 2026
… and let its copy be retargeted (CR 603.3 + CR 707.10c) (phase-rs#5727) * fix(engine): type the mana-spend trigger's filter as an EVENT filter, and let its copy be retargeted (CR 603.3 + CR 707.10c) Pyromancer's Goggles and Primal Wellspring make a copy of the spell their mana was spent on. We modeled no copy at all, and their retarget sentence died as an `orphaned_copy_retarget` residual. Two gates rejected them, and the second was a type error. 1. `parse_mana_spend_trigger`'s effect allowlist did not admit `Effect::CopySpell`. The allowlist is not arbitrary — `parse_effect_chain` parses some spell-referencing effects only PARTIALLY and silently swallows the remainder (Jade Orb's "…and gains hexproof" is the cited case), so the gate keeps a half-parsed clause from flipping a card to "supported". `CopySpell` is admissible because the retarget continuation is absorbed INTO the CopySpell as its CR 707.10c permission, leaving nothing dangling. That reasoning is now written where the next person will extend it, and the adjacent `sub_ability` bail — which is what CHECKS the reasoning per card — is retained. Jade Orb still stays honestly gapped (pinned by test). 2. `ManaSpellGrant::TriggerOnSpend` typed its filter as a `ManaRestriction`. That is CR 106.6 SPEND legality — "what may this mana pay for" — but it was being used as the CR 603.3 TRIGGER EVENT filter, "which spell makes this fire". Not one of these cards restricts its mana: Goggles' {R} may be spent on anything; it merely triggers on a red instant or sorcery. The wrong type forced the filter vocabulary to grow along the wrong axis — a color predicate would have had to be bolted onto `ManaRestriction` purely to serve a trigger — which is the cross-rule-section conflation the categorical-boundary rule forbids. So retype it to `TargetFilter`, the engine's existing "which object matches" vocabulary, and let `parse_spend_trigger_filter` DELEGATE to the shared `oracle_target::parse_type_phrase` instead of its bespoke three-shape matcher. One call now covers the whole type x color class — Goggles' "a red instant or sorcery spell" lowers to `Or[Typed{Instant, HasColor(Red)}, Typed{Sorcery, HasColor(Red)}]` with no new variant — rather than one filter shape at a time. The three existing shapes migrate whole; all three were mislabeled event filters (verified against every card's Oracle text: none says "spend this mana ONLY to cast X"). Genuine CR 106.6 spend restrictions are untouched — they live on the separate `ManaUnit::restrictions` field. `ManaRestriction::SharesCreatureTypeWithCommander` is deleted; its concept moves to `FilterProp::SharesCreatureTypeWithCommander`, where an object predicate belongs. It is a RELOCATION, not a new capability, and deliberately not expressed as `SharesQuality{CreatureType, reference: <commander>}` — that reference resolution walks `state.objects` for an `is_commander` object, but the authority for "your commander" in a live game is `deck_pools[player].current_commander`, which is exactly why `commander_creature_types` reads the deck pool FIRST. (The only code that flags a command-zone object lives under `#[cfg(test)]`.) A `SharesQuality` port would have consulted the fallback and never the authority, so a registered-but-uninstantiated commander would be invisible and Path of Ancestry would have silently stopped triggering. The new prop calls the SAME helper the spend site called before the retype, so behavior is preserved BY CONSTRUCTION. Finally, the retarget sentence had to be able to REACH the copy. It cannot bind on the ordinary clause-streaming path, and not by accident: the spend-trigger fold (`extract_mana_spend_trigger_from_chain`) is a POST-pass, so when the continuation recognizer went looking for the sentence's antecedent, the `CopySpell` did not exist yet. It is therefore reclaimed from its honest residual by the fold itself, via `sequence::absorb_orphaned_copy_retarget`. Without that, a half-fix leaves the copy modeled but permanently un-retargetable — which is exactly what the first measured pass showed, and why the tests assert `orphaned == 0` rather than merely the presence of a copy. EVIDENCE — two instruments, because one is structurally blind here. (i) Full-pool whole-face diff (35,396 faces, every face's ENTIRE tree compared): exactly 7 faces change, zero others. primal wellspring [] orph=1 -> [MayChooseNewTargets] orph=0 (target) pyromancer's goggles [] orph=1 -> [MayChooseNewTargets] orph=0 (target) path of ancestry / gilanra / lapis orb / a-lapis orb / scaled nurturer filter shape migration only, semantics identical Pool-wide `orphaned_copy_retarget`: 5 -> 3 (the 3 left are phase-rs#70/phase-rs#71's faces). (ii) A RUNTIME test, because (i) CANNOT see this change. The retype moves an evaluation from a bespoke call-site check into the generic filter layer: a runtime semantics change with an IDENTICAL parse shape. The full-pool diff would come back 100% clean even if Path of Ancestry silently stopped triggering. `mana_spend_trigger_shares_creature_type_with_commander` is the instrument that can see it, and it passes. The engine's copy-retarget test walker was itself blind to a `CopySpell` nested in `Effect::Mana`'s grants — it reported `retargets: []` for these cards — so it is taught to descend there too. A walker that cannot reach where the thing hides is measuring the wrong thing. Fixture: `integration_cards.json` carries the changed shape for 2 of its cards (lapis orb, path of ancestry) and both are updated from the fresh export. NOT a regen: a regen would have bundled 7 unrelated cards whose committed entries are already stale against this base (alrund, mana reflection, misty salon, osteomancer adept, ram through, the dining car, unleash the flux — reported separately). The diff is verified to touch exactly those 2 entries and no others. It rides in this commit rather than its own because the fixture shape is coupled to the type: split either way, one commit would be red. gates: fmt clean; clippy -D warnings 0 diagnostics; nextest 19,211 passed / 0 failed; workspace check clean. * fix(engine): classify SharesCreatureTypeWithCommander in the event-subject support gate (CR 903.3) Rebase fallout: main gained classify_prop (types/events.rs) after the unit's base, and its exhaustive match over FilterProp (deliberately wildcard-free so new variants are a compile error, not a silent Supported) did not cover the new variant. Classified Unsupported: the prop needs the live deck-pool commander registry, and it is only parsed inside mana-spend spell filters (CR 106.6 / CR 603.7a) evaluated live at the casting site — reaching it from the event-subject grammar should fail the gate loudly, per the bucket's design. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.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
3 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.
mapping oracle for Skyclave Apparition card -> all test passed -> game run and I check if there is no error on card