fix(parser): keep the card-type gate on "from among them" casts (#6880) - #6959
Conversation
Velomachus Lorehold reads "you may cast an instant or sorcery spell with
mana value less than or equal to Velomachus Lorehold's power from among
them without paying its mana cost." The mana-value gate was enforced; the
instant-or-sorcery gate was not, so any card type could be cast.
The "from among them" branches emitted `target: TargetFilter::ExiledBySource`
unconditionally -- a bare anaphor with no card-type leg. The correct
composition already existed in a sibling branch in the same function, which
parses the type disjunction and ANDs it with the anaphor. Route all the
bare-emitting branches through one helper so the type gate survives.
Three branches emitted the bare filter, not two: "from among them", the
hand-bound sibling, and "from among those/the exiled cards". The third has
type-gated members (Eager Flameguide, Kylox), so fixing only the first two
would have left the class half-covered.
Deliberately NOT copying the sibling's `FilterProp::InZone { Zone::Exile }`
leg. Velomachus is `Dig { keep_count: 0 }` -- the looked-at cards stay in the
LIBRARY and are published via `last_revealed_ids`, so an exile-zone leg would
match nothing and turn a permissive filter into a total no-op. The type gate
is ANDed with `ExiledBySource` alone.
No engine change. `TargetFilter::references_exiled_by_source` uses `any` over
`And` legs, so the composed shape still enters the look-at fallback, and
`remap_exiled_by_source_for_looked_cards` recurses into `And`, so the library
remap survives.
Ten cards now carry a correct type gate, each verified against its real
Oracle text: Velomachus Lorehold, Jace's Mindseeker, Muse Vortex, Summons of
Saruman and Talent of the Telepath (instant-or-sorcery); Chiss-Goria
(artifact); Herald of Amity (Aura); Eager Flameguide (creature); Narset,
Enlightened Master (noncreature card); and The Omenkeel, whose "you may play
lands from among those cards" previously let any exiled card be played as a
land.
Five cards in the class stay permissive -- a pre-existing gap, not a
regression: Epic Experiment and Ral, Leyline Prodigy ("instant and sorcery
spells"), Kylox ("any number of instant and/or sorcery spells"), Collected
Conjuring ("up to two sorcery spells" -- the leading count blocks the article
parse), and Sanwell ("a Vehicle or artifact creature spell" -- Vehicle is a
subtype). Extending the disjunction helper to "and"/"and/or" would reach
three of them but changes two other call sites, so it is left for follow-up.
Two of the four tests are regression guards for the 31 untyped cards in the
class: they pass before and after, and pin that a bare "cast a spell" clause
still emits a bare anaphor rather than acquiring a spurious type leg.
This is parser-side and therefore inert until card data is regenerated and
redeployed. Verified end to end rather than at AST level: the card-data
pipeline was rerun after the fix and the census re-taken against the
regenerated file.
Does not address #3267 (Sanwell) -- its reported rest-to-bottom symptom is
untouched, and Sanwell is one of the five whose type gate still does not
parse.
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe parser now preserves card-type restrictions in cast-from-among clauses. It combines type filters with exile-source filters and applies them to hand and exile casts. Tests cover typed Velomachus permissions and unrestricted Svella permissions. ChangesCast-from-among card-type filtering
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/engine/src/parser/oracle_effect/mod.rs (2)
21999-22053: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: align the sibling
has_from_among_cards_exiled_with_selfbranch with the newparse_cast_type_gateskip-logic.
parse_cast_type_gatedeliberately returnsNonewhen the parsed type filters are onlyTypeFilter::Card/TypeFilter::Any(Line 22028-22032), so an untyped clause stays unrestricted. The pre-existing sibling branch at Line 22991-23016 (has_from_among_cards_exiled_with_self) builds its typed leg with the sameparse_cast_type_disjunction(rest).unwrap_or_else(|| parse_type_phrase(rest).0)composition but does not apply this Card/Any skip — an untyped clause there still wraps the target inTargetFilter::And{[Typed(Card), ExiledBySource]}instead of the bareExiledBySourcethe new helper would produce. This is functionally harmless (Cardmatches everything) but is duplicated composition logic that could drift.Consider reusing
parse_cast_type_gatethere too for one shared code path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/mod.rs` around lines 21999 - 22053, Update the has_from_among_cards_exiled_with_self branch to reuse exiled_cast_target_with_type_gate or parse_cast_type_gate instead of duplicating type parsing. Preserve the existing ExiledBySource behavior while ensuring Card/Any-only clauses remain unrestricted and typed clauses are AND-combined with the exile-source filter.
22900-22908: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the hand-bound branch's new type gate.
This new
if let Some(typed) = parse_cast_type_gate(rest) { hand_filter.type_filters = typed.type_filters; }block is a genuinely new code path (the hand-reveal cast permission previously always used a bareCardfilter). The paired test file (kiora_self_library_peek_cast.rs) only exercises the exile-bound branches (Velomachus, Jace's Mindseeker, Talent of the Telepath — all typed; Svella, Aetherworks Marvel, Apex of Power — all untyped). None of the new tests setctx.chain_prior_hand_reveal_targettogether with a typed clause, so this specific overwrite (replacinghand_filter.type_filtersentirely rather than appending) is not exercised by any test in this PR.Confirm whether a real card combines a hand-reveal target (Silent-Blade Oni's shape) with a card-type restriction, and if so add a parser-level assertion (mirroring
from_among_them_cast_retains_the_instant_or_sorcery_gate) that the hand-boundTargetFilter::Typedcarries the parsed type filters alongsideFilterProp::InZone { zone: Zone::Hand }and the revealed player's controller.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/mod.rs` around lines 22900 - 22908, The hand-bound typed cast-gate path lacks parser-level coverage. In the tests alongside from_among_them_cast_retains_the_instant_or_sorcery_gate, add a case using chain_prior_hand_reveal_target with a typed cast clause, asserting the resulting TargetFilter::Typed preserves the parsed type filters together with FilterProp::InZone for Zone::Hand and the revealed player’s controller; first confirm and use a real card pattern matching Silent-Blade Oni.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 21999-22053: Update the has_from_among_cards_exiled_with_self
branch to reuse exiled_cast_target_with_type_gate or parse_cast_type_gate
instead of duplicating type parsing. Preserve the existing ExiledBySource
behavior while ensuring Card/Any-only clauses remain unrestricted and typed
clauses are AND-combined with the exile-source filter.
- Around line 22900-22908: The hand-bound typed cast-gate path lacks
parser-level coverage. In the tests alongside
from_among_them_cast_retains_the_instant_or_sorcery_gate, add a case using
chain_prior_hand_reveal_target with a typed cast clause, asserting the resulting
TargetFilter::Typed preserves the parsed type filters together with
FilterProp::InZone for Zone::Hand and the revealed player’s controller; first
confirm and use a real card pattern matching Silent-Blade Oni.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 73a6eb2f-2407-4478-a3a6-53f3e1e5614d
📒 Files selected for processing (2)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/tests/integration/kiora_self_library_peek_cast.rs
|
Generated for head Parse changes introduced by this PR · 11 card(s), 7 signature(s) (baseline: main
|
… doc
The hand-bound branch got a genuinely new code path -- it previously always
used a bare `Card` filter -- and nothing in the fix commit exercised it. All
four of those tests drive the exile-bound branches.
The path is reachable by a real card. Mindclaw Shaman reads "target opponent
reveals their hand. You may cast an instant or sorcery spell from among those
cards without paying its mana cost", and against regenerated card data it now
parses to a `Typed` filter carrying `AnyOf[Instant, Sorcery]` alongside
`InZone { Hand }` and `controller: Opponent`. So it is an additional card
corrected by this PR, not just an untested branch.
Assert all three legs. A test pinning only the type filter would not catch a
regression that dropped the zone or the controller.
Also add an untyped hand-bound guard, so a bare "cast a spell" hand clause
keeps its bare filter rather than acquiring a spurious type leg -- the
hand-side counterpart of the exile-side guard already present.
Sharpen `parse_cast_type_gate`'s doc to say why non-type restrictions carried
as properties (Chandra's "red spells", Meeting of the Five's "spells with
exactly three colors", Perception Bobblehead's mana-value bound) also yield
`None`: only the type axis gates, so the helper never invents a type
restriction the Oracle text did not state.
Raised by CodeRabbit on #6959.
The untyped guard covered three cards whose clause carries no qualifier at
all next to "spell". That leaves the interesting case untested: Oracle text
that DOES carry a restrictive qualifier immediately next to "spell", but
along an axis this filter does not model.
CR 601.3 genuinely restricts those casts -- just by color or mana value, not
by card type -- so `parse_cast_type_gate` must never mistake either for a
card type and synthesize a gate the text did not state. That is precisely
what its doc claims, and nothing pinned it.
Meeting of the Five ("spells with exactly three colors") probes the color
axis. Perception Bobblehead and Kiora ("a spell with mana value N or less")
probe the property axis, which rides on `CastPermissionConstraint` instead.
All three emit a bare `ExiledBySource` in regenerated card data, so the
assertion matches measured behaviour rather than an assumption.
The new const is byte-identical to the card's real Oracle text, verified
programmatically against card-data.json rather than by eye -- a paraphrased
fixture can take a different parser branch and quietly stop testing what it
names.
The doc said "the 25-card majority". The census it came from was superseded twice while this PR was open, and the figure is now 26 -- so the comment disagreed with the description of the change it documents. A count in prose goes stale silently and nothing forces anyone to re-verify it, which is the same reason the PlayerFilter variant count was removed from analysis/resource.rs. Say "the untyped majority" instead: the property the test asserts does not depend on the cardinality.
…itted The CR 603.5 producer census pins `game/effects/mod.rs` line-exact. The phase-rs#6961 + v0.44.0 uniform +78 shift moved the three producers to :5996/:6073/:9048, and that re-pin was edited during the 851180c fold but never committed — it stayed in the working tree, so the pushed tip 0b5a2bf still asserted :5918/:5995/:8970 and CI went red on it. This commit is that edit and nothing else. Why the local gate was green while CI was red: the census test walks `src/` from disk at runtime (`env!("CARGO_MANIFEST_DIR")` + `read_to_string`), so it measures the WORKING TREE, never the commit. A dirty tree can never certify a commit — "green at the exact tip" requires HEAD == sha AND a clean `git status` on the gated paths, and that precondition is now part of the drift-log instructions. Re-verified in the `refs/pull/<n>/merge` layout rather than locally, per the drift log's own rule: `git merge-tree --write-tree HEAD upstream/main` (upstream 1738d5c) puts the producers at exactly :5996/:6073/:9048, with `scoped_library_search.rs:452` and `engine.rs:11427` unmoved — all five production producers accounted for, still no sixth. Upstream phase-rs#6959 is innocent. Assisted-by: ClaudeCode:claude-opus-5
1 participant
Fixes #6880.
The bug
Velomachus Lorehold: "…you may cast an instant or sorcery spell with mana value less than or equal to Velomachus Lorehold's power from among them without paying its mana cost."
The mana-value gate was enforced; the instant-or-sorcery gate was not, so any card type could be cast. The parsed
CastFromZonecarried the rightconstraint: ManaValue{LE, Power(Source)}but a baretarget: ExiledBySource— an anaphor with no card-type leg.The correct composition already existed in a sibling branch of the same function, which parses the type disjunction and ANDs it with the anaphor. The fix routes the bare-emitting branches through one shared helper.
Three branches, not two
The survey that scoped this named two bare-emitting branches. There are three:
"from among them", the hand-bound sibling, and"from among those/the exiled cards". The third has type-gated members (Eager Flameguide, Kylox), so fixing only the first two would have left the class half-covered.What is deliberately not copied from the sibling
The sibling composition includes
FilterProp::InZone { zone: Zone::Exile }. Copying that leg would have been worse than the bug. Velomachus isDig { keep_count: 0 }— the looked-at cards stay in the library and are published vialast_revealed_ids. An exile-zone leg matches nothing there, converting a permissive filter into a total no-op: the card would become uncastable under a commit claiming a fix. The type gate is ANDed withExiledBySourcealone, and the reason is documented at the call site.No engine change
Confirmed with the mechanism, not by assertion:
TargetFilter::references_exiled_by_sourceusesanyoverAndlegs, so the composed shape still enters the look-at fallback.remap_exiled_by_source_for_looked_cardsrecurses intoAnd, so the library remap survives.Class — 37 anchor cards, 12 type-restricted, 10 fixed
The original scoping said 8 cards. That was wrong in both directions. Re-derived across all 35,657 entries: 41 cards carry the anchor with a bare
ExiledBySource, of which 15 name a card type (10 fixed, 5 still bare) and 26 name none and are correctly bare. 10 + 5 + 26 = 41.(An earlier revision of this description said 37/12/25. That was itself an undercount, caught by the same denominator discipline: the two anchor phrasings were censused separately, and a
"cast "regex missed The Omenkeel, whose verb is "play lands". Corrected here.)The original list also omitted four cards and was partly unverifiable by the method that produced it — 5 of its 8 have
abilities: [], because theCastFromZonelives under.triggers. A census walking only.abilitiessilently undercounts.Fixed (10 of the 15) — each checked against its real Oracle text:
AnyOf[Instant, Sorcery]ArtifactSubtype(Aura)CreatureCard + Non(Creature)Land— "you may play lands from among those cards" previously let any exiled card be played as a landPlus one more card from outside this population — see the follow-up section: Mindclaw Shaman, corrected on the hand-bound branch. It is not a missing member of the 41/15 set; that set is defined by a bare
ExiledBySource, which the hand-bound branch never emits. 11 cards are corrected in total.Still permissive (5 of the 15) — pre-existing gap, not a regression, each blocked by a missing helper rather than by this change:
" or "onlyExtending
parse_cast_type_disjunctionto"and"/"and/or"would reach three of them but changes two other call sites, so it is left for follow-up rather than widened here.On Elder Brain — checked, and it is correctly bare. It reads "You may play lands and cast spells from among the exiled cards", which looks like it should carry a
Landgate. It does not, and should not: the clause reaching the cast parser is "spells from among the exiled cards", which names no card type, so the bare binding is right. Verified against parsed card data — Elder Brain emitsCastFromZonewith a bareExiledBySourceandmode: Cast, whereas The Omenkeel's single "play lands from among those cards" emitsmode: PlaywithAnd[Typed[Land], ExiledBySource]. Different clauses, correctly different results. Elder Brain is in the 26.Separately, Elder Brain does have a real defect, filed rather than fixed here. It emits only one unit — the
Casthalf — with noPlayunit and noTyped[Land]anywhere on the card. Its "play lands" permission is dropped outright, not merely under-restricted. That is a different defect in a different population and out of scope for this PR.Verification
from_among_them_cast_retains_the_instant_or_sorcery_gate,velomachus_does_not_offer_a_creature_inside_its_mana_value_ceiling). After the fix: 22,815 / 22,815.clippy,test-engine,card-dataall green. Parser combinator gate exit 0.Inertness — verified end to end
This is parser-side and therefore inert until
card-data.jsonis regenerated and redeployed; it is invisible in the app on merge alone. Verified beyond the AST level: the card-data pipeline was rerun after the fix and the census re-taken against the regenerated file.Does not close #3267
Sanwell's reported symptom there is rest-to-bottom behaviour, which is untouched — and Sanwell is one of the five whose type gate still does not parse. Please do not auto-close it.
Summary by CodeRabbit
Follow-up commit: hand-bound coverage (
07163ac)CodeRabbit flagged that the hand-bound branch gained a genuinely new code path — it previously always used a bare
Cardfilter — and that none of the four original tests exercised it; they all drive the exile-bound branches.The right first question was whether the path is reachable at all, since unreachable code should be documented or removed rather than tested. It is reachable, by a real card: Mindclaw Shaman — "target opponent reveals their hand. You may cast an instant or sorcery spell from among those cards without paying its mana cost." Against regenerated card data it now parses to a
Typedfilter carryingAnyOf[Instant, Sorcery]alongsideInZone { Hand }andcontroller: Opponent.So it is an 11th corrected card, not merely an untested branch — this PR was undercounting its own impact. Fittingly, Mindclaw Shaman is another
abilities: 0, triggers: 1card, the same shape that made the original 8-card scoping unverifiable.The new tests assert all three legs (type filter, zone, controller); a test pinning only the type filter would not catch a regression that dropped the zone or the controller. An untyped hand-bound guard is added alongside, as the hand-side counterpart of the exile-side guard.
The same commit sharpens
parse_cast_type_gate's doc to state why non-type restrictions carried as properties — Chandra's "red spells", Meeting of the Five's "spells with exactly three colors", Perception Bobblehead's mana-value bound — also yieldNone: only the type axis gates, so the helper never invents a restriction the Oracle text did not state.CodeRabbit's other nit — that a sibling branch still hand-rolls the same composition rather than calling the shared helper — is a real single-authority point but changes AST shape for cards currently parsing that way, so it needs its own card-data census. Deferred to #6960.
Why the 11th card was missed — the denominator, not the walk
Worth recording, because it is a different error from the
.abilities-vs-.triggersone corrected above.This description defines its population as "cards carrying the anchor with a bare
ExiledBySource" — 41 cards, 15 type-restricted. But the defect lives in a function with three emitting branches, and the hand-bound branch never emitsExiledBySourceat all; it emits aTypedhand filter. Mindclaw Shaman was therefore excluded by construction, not by an execution mistake. No amount of walking.triggersas well as.abilitieswould have surfaced it.The tell is that the census predicate named a concrete emitted value rather than the behaviour under test (a cast permission that should carry a type gate). Whenever a population is defined by a value one code path emits, the question to ask is how many paths reach the defect and whether each emits that value.
No other hand-bound card is newly corrected
Verified with its own denominator: 35,657 MTGJSON entries → 50 carrying
cast[^.]*from among (them|those cards). The hand-bound subset is 4: Mindclaw Shaman (typed → now fixed), and Silent-Blade Oni, Mindleech Mass, Extract Brain (all "cast a spell", untyped → unchanged, and covered by the new untyped guard). The 350-card complement carrying the anaphor outside those 50 was checked for any hand-reveal-plus-cast combination: empty.Incidental, pinned but not changed: Silent-Blade Oni and Mindleech Mass bind
controller: TriggeringPlayerrather thanDefendingPlayer. Pre-existing and untouched here.