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) - #5727
Merged
Conversation
… 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 #70/#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.
matthewevans
enabled auto-merge
July 13, 2026 07:55
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors mana-spend triggers (such as Path of Ancestry and Pyromancer's Goggles) to use the engine's standard TargetFilter instead of a bespoke ManaRestriction for their trigger event filters, aligning with CR 603.3 and avoiding conflation with CR 106.6 spend restrictions. The ManaRestriction::SharesCreatureTypeWithCommander variant has been replaced with FilterProp::SharesCreatureTypeWithCommander, which is evaluated against live game state. Additionally, a post-pass fold mechanism has been introduced to reclaim orphaned copy-retarget clauses (CR 707.10c) for mana-spend triggers that copy spells. No review comments were provided, so there is no feedback to evaluate.
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.
…bject 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.
Parse changes introduced by this PR · 7 card(s), 7 signature(s) (baseline: main
|
matthewevans
added a commit
to jaytbarimbao-collab/phase
that referenced
this pull request
Jul 13, 2026
… parity rows it exposes (CR 603.3b + CR 603.4) (phase-rs#5732) `integration_cards.json` is a cached subset of the card-data export, but it was last FULLY regenerated at b9685cc (phase-rs#5695). Nineteen parser/types PRs merged since then; every fixture touch in between was surgical (phase-rs#5672 +1, phase-rs#5679 +2, phase-rs#5727 2 entries), so the parse values silently drifted. Regenerated from the export at 8c35dc5 (oracle-gen, MTGJSON 5.3.0+20260629). Population (json deep-equality, not line counts — the file is one line): committed 2658 entries -> 2726. 70 added, 2 removed, 44 changed values. Attribution of the 44 changed (causal: exports built at phase-rs#5720 / phase-rs#5717 / phase-rs#5730 and compared, NOT shape-guessing): phase-rs#5723 (P02-U3b shared condition grammar) ...... 3 archive trap, temple of civilization, thaumaton torpedo (all gained the comparator/lhs/rhs/qty/scope condition shape) phase-rs#5721 + phase-rs#5719 (where-X quantity channel + ..... 21 restriction grammar; both merged BEFORE phase-rs#5717 — merge order != PR-number order) bellowsbreath ogre, cryptex, deadly rollick, deflecting swat, desert, dread wanderer, esquire of the king, flesh, fraying sanity, gloomlake verge, great desert hellion, gutterbones, officious interrogation, once upon a time, potioner's trove, ribald shanty, rock jockey, second little pig, shifting woodland, snuff out, starport security phase-rs#5695..phase-rs#5720 no-regen window (bloc) ........... 20 Stale already at phase-rs#5720, so attributable to the 19-PR window above the b9685cc anchor, not to any single PR: alrund god of the cosmos, animal friend, approach of the second sun, cavernous maw, fblthp the lost, from father to son, hour of revelation, increasing vengeance, jodah the unifier, mana reflection, misty salon, puca's eye, ram through, reidane god of the worthy, secrets of the key, sevinne's reclamation, temple of the dead, the dining car, unleash the flux, valgavoth terror eater The 70 added keys are new test-source card references the generator collects; phase-rs#5729 (tests-only) contributed zero parse delta, as expected. Corrected premise: 44 entries are truly stale, not 7. Six of the seven originally reported reproduce; `osteomancer adept` is NOT stale (committed == fresh). The regen turns `ordering_parity_sweep` red, so the gate's evidence rows ship ATOMICALLY with it. Both rows are population entries, not ordering regressions: the sweep skips Unimplemented-bearing triggers, so a card only enters it once its parse binds. great desert hellion -> BATCH_GENUINE_ROWS. Its LTB Draw was Unimplemented until phase-rs#5721/phase-rs#5719 bound Intensity{Source}. Each co-departing Hellion draws off its OWN intensity but discards the SHARED hand, so the second trigger discards the cards the first just drew: with intensities a != b the final hand, graveyard and library differ by order. The members are not identical functions, so commutation genuinely fails and the new prompt is the CR 603.3b choice the legacy serde walk wrongly auto-ordered (CR 603.5: each "may" is chosen on resolution). planar collapse -> DOCUMENTED_OVER_PROMPT (L8-held family). New fixture key. Upkeep ObjectCount(Creature) >= 4 intervening-if x DestroyAll + self-Sacrifice: the first copy's sweep drives the census to 0, so the sibling's CR 603.4 re-check is false and it does nothing. Monotone and self-limiting — identical siblings commute up to relabeling, so the prompt is conservative, fail-closed and rules-correct. Neither row weakens the gate: both are direction-gated over-prompts (an under-prompt is never suppressible), and both are consumed by the ledger's exact-set asserts (over_prompt_hit 18->19, batch_genuine_hit 1->2), so a misclassification still trips the STRICT PROOF-GATE. Verification: engine lib 16481/16481 pass (was 16480 + 1 red); integration 2929/2929 pass. 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
1 participant
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.
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_retargetresidual.Two gates rejected them, and the second was a type error.
parse_mana_spend_trigger's effect allowlist did not admitEffect::CopySpell.The allowlist is not arbitrary —
parse_effect_chainparses somespell-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".
CopySpellisadmissible 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_abilitybail — which is what CHECKS the reasoning per card — is retained. Jade Orb
still stays honestly gapped (pinned by test).
ManaSpellGrant::TriggerOnSpendtyped its filter as aManaRestriction. Thatis 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
ManaRestrictionpurely to serve a trigger — which isthe 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_filterDELEGATE to the sharedoracle_target::parse_type_phraseinstead 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::restrictionsfield.ManaRestriction::SharesCreatureTypeWithCommanderis deleted; its concept movesto
FilterProp::SharesCreatureTypeWithCommander, where an object predicatebelongs. It is a RELOCATION, not a new capability, and deliberately not expressed
as
SharesQuality{CreatureType, reference: <commander>}— that referenceresolution walks
state.objectsfor anis_commanderobject, but the authorityfor "your commander" in a live game is
deck_pools[player].current_commander,which is exactly why
commander_creature_typesreads the deck pool FIRST. (Theonly code that flags a command-zone object lives under
#[cfg(test)].) ASharesQualityport 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 continuationrecognizer went looking for the sentence's antecedent, the
CopySpelldid notexist 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 thecopy modeled but permanently un-retargetable — which is exactly what the first
measured pass showed, and why the tests assert
orphaned == 0rather than merelythe 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 #70/#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_commanderis theinstrument that can see it, and it passes.
The engine's copy-retarget test walker was itself blind to a
CopySpellnested inEffect::Mana's grants — it reportedretargets: []for these cards — so it istaught to descend there too. A walker that cannot reach where the thing hides is
measuring the wrong thing.
Fixture:
integration_cards.jsoncarries 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.