feat(parser): L02 condition lane — typed conditions on ~30 Standard cards (BB1–BB8 + BB-FU1–5)#5677
Merged
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements comprehensive rules support and parser updates for several MTG mechanics, focusing on cast-origin conditions, enters-with-counter riders on static cast permissions, and intervening-if conditions. Key changes include renaming AbilityCondition::CastFromZone to WasCast to support copy-correct presuppositions, adding an enters_with_counter field to graveyard and exile cast permissions, and implementing a tracking ledger for counter-placement occurrences to support "first time counters have been put on it this turn" triggers. Additionally, the parser has been updated to handle various complex conditions, such as "During your turn, as long as..." prefixes, gendered/neutral pronouns, and additive count thresholds, while corresponding swallow-check exemptions have been added to prevent false-positive warnings. Since no review comments were provided, I have no feedback to offer on the review itself, but the code changes are highly detailed, rules-compliant, and well-tested.
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.
Parse changes introduced by this PR · 102 card(s), 68 signature(s) (baseline: main
|
matthewevans
requested changes
Jul 12, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Verdict: changes requested
Substantial, disciplined work — the condition-lane rewrite (in battlefield you control X entered this turn ≥ 1 → battlefield entries this turn (X, you) ≥ 1) is a clean typed re-representation, all 14 CR citations I spot-checked grep-verify against docs/MagicCompRules.txt, no new enum variants (no sibling-cluster debt), and the allow-noncombinator annotations are in-convention (497 pre-existing uses; the gate honors the documented form). CI is fully green — 12 checks including card-data, the parser gate, and both test shards. Rubblebelt Braggart's ∅ → not (source is suspected creature) is exactly right.
Three things block it.
1. The population is understated by more than 2x
The body claims "~30 Standard cards" and "+32 supported". The head-current parse-diff sticky (baseline fa199f8021a0) measures 69 cards / 66 signatures.
A wide diff is not automatically scope creep — "build for the class, not the card" produces the same signature, and most of these look like correct collateral repairs. But a reviewer cannot tell the two apart without reading the extras, and the body doesn't list them. Please state the real population card-by-card so the ~39 cards beyond your claim can be checked.
2. Two unexplained capability removals
replacement/Moved removed from Intrepid Paleontologist and Leonardo, Sewer Samurai. Both cards carry finality-counter reminder text — "(If a creature with a finality counter on it would die, exile it instead.)" — and both simultaneously gain a correct cast-permission static, which suggests a deliberate re-seam.
But I can find no generic finality-counter die→exile replacement anywhere in crates/engine/src/game/. If that per-card replacement/Moved was the only thing implementing the exile-instead-of-die behavior, this PR silently deletes finality-counter semantics on both cards, and no test would catch it.
Please either (a) point at where that behavior now lives, or (b) confirm it's a genuine loss and restore it. Same question for the unpaired ability/PayCost removed on one card.
3. A condition is being attached twice
trigger/YouAttack · field condition: ∅ → attackers declared ≥ 1 and attackers declared ≥ 1
The same condition conjoined with itself. It's idempotent, so behavior is likely unaffected — but it means the new lane attaches the condition twice, and the next condition where X ∧ X ≠ X will not be so forgiving. Worth fixing at the attach site rather than the symptom.
Non-blocking
ability/there · added: there (conditional=# of colors among you control permanent = 5)— attaching a typed condition to an ability namedthere(a sentence-fragment misparse the lane didn't create, but is now decorating rather than fixing). Worth a follow-up issue.- Size. 8,695 additions / 49 files / 16 commits bundling 13 units (BB1–BB8 + BB-FU1–5) across shared parser infrastructure. Individually the units look sound and the commit boundaries are clean, but this is at the edge of what can be reviewed as one artifact. Future lanes would land faster split at the BB boundary.
Happy to re-review promptly once the population is stated and the removals are explained.
…d cards)
Extend `parse_inner_condition` (the single StaticCondition recognition authority)
so 7 "Activate only if <state>" activated-ability restrictions parse and attach
the rules-correct condition to the `RequiresCondition` gate, clearing the
`SwallowedClause{Condition_If}` warnings (and `DynamicQty` on Cavernous Maw) that
demoted these cards to unsupported. Deliverable is runtime-correct activation
gating, not just the coverage flip.
Cards: Bonecache Overseer, Temple of the Dead, Puca's Eye, Master's Manufactory,
Temple of Cyclical Time, Temple of Power, Cavernous Maw. Also flips Skarrgan
Hellkite (rides the new HasCounters bridge arm — class-generality).
Class-level combinators (no new engine variants; one bridge arm):
- disjunction " or if " connector (CR 602.5 / 603.4)
- N-or-more graveyard-departure threshold (singular "a card" n=1 preserved)
- existential "a player has <hand predicate>" via PlayerAttribute + All (CR 402.1)
- bare "colors among <filter>" quantity ref (CR 105.1/105.2)
- self-inclusive "~ or another <type> entered ... this turn" → the CR 608.2h
BattlefieldEntriesThisTurn snapshot look-back, Controller-scoped (counts a
permanent that entered and later left this turn)
- StaticCondition::HasCounters → ParsedCondition bridge arm (source counter gates,
CR 122.1)
- filtered/plural/[non]combat damage-history threshold (CR 120.2a/b, 120.9, 608.2i)
- additive two-term subtype/zone count threshold (CR 205.3i / 404.1)
Verification: 31 integration tests (per-shape parse-fidelity + discriminating
runtime cond-FALSE/cond-TRUE with hostile discriminators, revert-to-red measured);
coverage +8 GAINED, REGRESSED=0 corpus-wide.
Follow-up (tracked, out of scope): migrate the singular `parse_entered_this_turn_
subject` live-board `EnteredThisTurn` to the same snapshot authority, and add
CR 608.2i to the shared `BattlefieldEntriesThisTurn` annotations for citation
precision.
Assisted-by: ClaudeCode:claude-opus-4.8
…dard cards) Attach rules-correct intervening-if conditions (CR 603.4 dual-checkpoint: checked at fire AND at resolution) to five triggered abilities whose condition clause was recognized by the swallow detector but dropped by the real parser, demoting the cards to unsupported. Deliverable is runtime-correct trigger gating, not just the coverage flip. Cards: Sharp-Eyed Rookie, Massacre Girl (Known Killer), Fearless Swashbuckler, Rubblebelt Braggart, Stalwart Successor. Class-level parser combinators (delegate to parse_inner_condition, the single StaticCondition recognition authority; bridge via static_condition_to_trigger_condition): - S1 Sharp-Eyed: possessive entering-object P/T-vs-source comparison; the source is read LIVE at resolution (CR 603.4 / 603.6a / 208.1), not latched at fire time - S2 Massacre: dying-object P/T threshold on the event victim, with a first-refusal deferral gate — self-death "if its power was N" yields to the existing parse_inner_condition authority (deathknell byte-identical), other-death proceeds to the event-object filter (CR 603.6a self vs event subject) - S3 Fearless: typed-attacker conjunction (And[Pirate>=1, Vehicle>=1] declared this combat) - S4 Rubblebelt: source-suspected state condition - S5 Stalwart: first-time-this-turn object-counter-added recognizer New engine variant (add-variant ratified distinct — Nth* are a different enum TriggerConstraint; CounterAddedThisTurn is per-controller existence): - TriggerCondition::FirstTimeObjectCountersAddedThisTurn (fieldless; per-object first-occurrence over the ObjectCounters ledger, CR 122.1 / 603.4). Walkers (ability_rw / ability_scan / coverage) mirror CounterAddedThisTurn's ObjectCounters read — NOT Axes::NONE — so CR 603.3b same-event ordering is classified read-not-none (FORGE ordering_parity_sweep delta=0). - swallow_check Duration_ThisTurn allowlist entry "FirstTimeObjectCountersAddedThisTurn" (Stalwart's until-end-of-turn duration marker), CR 122.1 / 603.4. Verification: 22 integration tests (per-shape parse-fidelity + discriminating runtime cond-FALSE/cond-TRUE, revert-to-red measured). Sharp-Eyed live-source-read non-vacuity: 2/2 fire-time latch grants vs 4/4 live-read declines. Massacre binds dying victim: tough 0 -> draw, tough 2 -> no-draw. Coverage +5 GAINED, REGRESSED=0 corpus-wide. Follow-up (tracked, in-branch before PR): BB-FU3 thread trigger dying-subject structurally (valid_card == SelfRef signal) to replace S2's fork-via-proxy routing (CR 603.6a) — a signal-cleanliness refinement, not a correctness gap. Assisted-by: ClaudeCode:claude-opus-4.8
…ers (4 Standard cards)
Extend the parser to recognize+attach the rules-correct cast-origin condition on 4
"was cast [from <zone>]" cards whose condition clause was recognized by the swallow
detector but dropped by the real parser — clearing `SwallowedClause{Condition_If}`
and gating the runtime correctly. ZERO new engine variants: parser-only, reusing the
existing WasCast/CastFromZone primitives across two channels.
Cards: Antiquities on the Loose, Otterball Antics, Anti-Venom (Horrifying Healer),
Ran and Shaw.
Two cast-origin channels (CR 601.2a cast-origin zone, CR 400.7 new-object memory,
CR 603.4 intervening-if):
- Channel A — resolving-spell self-reference (Antiquities, Otterball): passive
"if this spell was cast from anywhere other than <zone>" -> AbilityCondition::Not(
CastFromZone{zone}) on the SequentialSibling sub-ability, extending
strip_cast_from_zone_conditional with the passive-voice arm (positive + negated x
hand/graveyard/exile). Reads SpellContext.cast_from_zone.
- Channel B — entering-permanent provenance (Anti-Venom, Ran and Shaw):
* pronoun-subject widening (he/she/they was/were cast) in parse_was_cast_condition
-> TriggerCondition::WasCast{None,None,None} via the existing bridge.
* new native And-fold parse_cast_them_and_graveyard_count_intervening_if:
"if you cast them and there are three or more Dragon and/or Lesson cards in your
graveyard" -> And[WasCast, QuantityComparison{ZoneCardCount{Graveyard,
[Dragon,Lesson],Controller}, GE, 3}] (CR 404.1 owner graveyard, CR 205.3
subtypes), registered before the bare "if you cast it" handler so the conjunction
is not truncated.
Verification: 16 integration tests (per-shape parse-fidelity + discriminating runtime
cond-FALSE/cond-TRUE with hostile discriminators + measured revert-probes; Ran-Shaw
covers the multi-subtype OR, the Controller scope, and both conjuncts; Anti-Venom
cast-vs-reanimate). Non-regression asserts for the 2 benign collisions (Gideon
reminder-stripped, Spiders-Man web-slinging). Coverage +4 GAINED (13->17),
REGRESSED=0 corpus-wide.
Ceiling (tracked): the negated Channel-A form Not(CastFromZone{Hand}) over-fires on a
spell COPY (cast_from_zone=None, CR 707.10 "a copy isn't cast") — documented in a
ponytail comment + scheduled as BB-FU4 (add AbilityCondition::WasCast, wrap the whole
negated-cast-zone class as And[WasCast, Not(CastFromZone{..})]).
Assisted-by: ClaudeCode:claude-opus-4.8
…rds (Braided Net, Cloud, Tishana's Tidebinder)
Make the parser recognize+attach the rules-correct "as long as" condition on 3
cards whose condition clause was recognized by the swallow detector but dropped —
clearing SwallowedClause{Condition_AsLongAs} and gating the runtime correctly.
ZERO new engine enum variants; ONE field-add (duration) on the existing
CounterSourceRider::LosesAbilities.
Three as-long-as shapes (all reuse pre-existing Duration/StaticCondition types):
- Braided Net (CR 611.2b + CR 110.5 + CR 602.5): "activated abilities can't be
activated for as long as it remains tapped". The tapped suffix is peeled upstream
by strip_trailing_duration (-> SourceIsTapped, the wrong scope); a class-scoped
guard (cant_be_activated_clause_owns_tapped_suffix) keeps the suffix with the
clause so the CantBeActivated arm emits Duration::ForAsLongAs{IsTapped{scope:Target}}
bound to the tap target, not the artifact. The guard keys on the "can't be
activated" + "for as long as <anaphor> remains tapped" pattern (nom combinator, no
card-name/verbatim match); Braided Net is the sole corpus co-occurrence today, but
the guard is pattern-scoped for the class.
- Cloud, Planet's Champion (CR 611.3a + CR 109.5 + CR 301.5a): "During your turn, as
long as ~ is equipped, it has double strike and indestructible". A leading-condition
peel at the top of parse_subject_continuous_static (both "during your turn, " AND
"as long as " required — strictly narrower than a during-optional gate, so it never
intercepts bare as-long-as statics owned by parse_conditional_static) attaches
condition: And{[DuringYourTurn, SourceIsEquipped]} to the granted static.
- Tishana's Tidebinder (CR 611.2a): "loses all abilities for as long as this creature
remains on the battlefield". Surface the already-enforced UntilHostLeavesPlay
duration into the AST via a new duration field on CounterSourceRider::LosesAbilities
(Box<Duration>, serde-default back-compat); apply_source_static now reads the field
instead of a hard-coded literal (behavior identical).
Runtime enforcement for all three already existed — these are parser-attach plus one
field-surfacing change.
Verification: 5 integration + 5 unit tests (parse-fidelity + discriminating runtime
ON->OFF with named revert-probes; Tishana field-threading proven by a non-default
duration sentinel driven through the real resolve()). F1 non-vacuity: the
both-prefixes peel does not shadow plain during-your-turn statics (39-card is_none()
guard) and counter-animation cards (Kaito) fall through unchanged. Corpus coverage:
+3 GAINED (braided net, cloud, tishana), REGRESSED=0 corpus-wide; the shared
strip_trailing_duration guard touches only Braided Net (jq: sole "remains tapped" +
"can't be activated" co-occurrence of 42 remains-tapped cards), and the other two
"during your turn, as long as" cards (elvish refueler, evendo brushrazer) are
byte-identical before/after.
Assisted-by: ClaudeCode:claude-opus-4.8
…falling back to Normal (CR 601.2a)
When a static ExileCastPermission grants a cast and the exiled card is the ONLY
cast candidate, handle_cast_spell_with_payment_mode skipped variant election (the
had_multiple_candidates guard) and continued as a Normal cast — dropping the
ExilePermission context: WithoutPayingManaCost mana-zeroing, the OncePerTurn slot
stamp, and (once wired) the finality enters-with rider. The sole exiled card then
failed prepare_spell_cast's castable-zone check ("Card is not in a castable zone"),
since owned-exile-under-a-permission is not a castable_zone branch.
Elect the single option when it IS an ExilePermission, so the permission's casting
variant (mana permission, slot, source) is honored. Structurally scoped: an
ExilePermission candidate is only ever pushed for a card in Zone::Exile, so a
Normal/GraveyardPermission single candidate — every hand cast (Warp/Evoke/Dash) and
graveyard cast (which already has a castable_zone branch) — is untouched and falls
through byte-identically. Scoped to ExilePermission with a ponytail comment naming
the general predicate + upgrade path (the general form would preempt single-candidate
hand casts that route through their own cost-choice handlers).
Latent-bug fix for Maralen, Fae Ascendant ("Once each turn, you may cast a spell …
from among cards exiled with Maralen this turn without paying its mana cost",
CR 118.9a): pre-fix its single-candidate cast charged full mana and skipped the
once-per-turn slot. Also the runtime prerequisite for the L02 BB3 finality-on-
exile-cast placement (Intrepid Paleontologist).
Tests: single_candidate_exile_permission_elects_permission_and_zeros_mana (mana
zeroed + slot stamped; reverting the election -> castable-zone error, both asserts
flip) and single_candidate_normal_hand_cast_unchanged_by_exile_election (a Normal
single-candidate hand cast still elects Normal, no slot leak — the gated-scope guard).
Assisted-by: ClaudeCode:claude-opus-4.8
…(Noctis, Intrepid, Leonardo)
Make the parser recognize+attach the rules-correct cast-permission and its linked
"if you cast a spell this way, that permanent enters with a finality counter"
clause on 3 Standard cards whose clauses were recognized by the swallow detector
but dropped or misparsed — clearing SwallowedClause{Condition_If} (all three) and
SwallowedClause{Optional_YouMay} (Intrepid, Leonardo). ZERO new engine enum
variants; ONE new field enters_with_counter: Option<CounterType> on the two sibling
static modes GraveyardCastPermission + ExileCastPermission, mirroring the existing
Effect::CastFromZone.enters_with_counter.
Root cause was the CLASSIFIER, not the leaf combinators: Leonardo's leading "During
your turn," qualifier and Intrepid's "cards you own exiled with" infix were not
routed to the Priority-7 static dispatch, so they fell to the Priority-8 replacement
gate and misparsed into a garbage PutCounter replacement. The classifier gains a
"during your turn, " leading-qualifier arm + a "from among cards you own exiled with"
zone-anchor tolerance (both narrow widenings of existing anchors).
- Noctis, Prince of Lucis (FIN 235, CR 601.2f + CR 122.1 + CR 614.1c): captures the
"by paying 3 life in addition to paying their other costs" extra_cost (an
opt("paying ") fix on the additional-cost rider) and the finality rider.
- Intrepid Paleontologist (LCI 193, CR 108.3 + CR 109.5 + CR 113.6b): ExileCastPermission
(Persistent pool, owner=You, Dinosaur creature) + finality. Its exile pool
auto-populates: the {2} exile ability is source-scanned via LINKED_EXILE_CONSUMER_TAGS
(which lists "ExileCastPermission") -> TrackedBySource -> exile_links, zero extra code.
- Leonardo, Sewer Samurai (TMT 17, CR 109.5): GraveyardCastPermission gated to the
controller's turn (StaticCondition::DuringYourTurn, power-or-toughness <=1 creatures)
+ finality.
A shared pub(crate) parse_cast_this_way_enters_with_counter recognizer (extracted from
the existing effect-path try_parse_cast_this_way_enters_rider + a new "that artifact"
subject arm; effect path byte-identical) sets the field on the static constructors;
a new finalize_cast reader (selected_static_permission_enters_with_counter, keyed on
the zone-stable casting_variant.source) places the counter via pending_etb_counters
at the seam. CR 601.1/607.1 (linked "this way"). Requires the election-fix engine
commit that precedes this so a single-candidate exile cast is elected as
ExilePermission rather than Normal.
Scope: finality counter PLACEMENT only. CR 122.1h death->exile enforcement does not
exist engine-wide and is a known orthogonal follow-up (mirror shield counters CR
122.1c) — this change does not claim it.
The class-level building blocks correctly generalize beyond the 3 targets (verified
by corpus warndiff): Hedonist's Trove and Rona, Disciple of Gix now parse their
ExileCastPermission (were silently dropping the clause), and Advanced Floral
Invocations now captures its finality rider. Dawnhand Dissident (whose "by removing
three counters" cost is unmodeled) is correctly DECLINED by the builder and stays a
clean Unimplemented gap — no misparse.
Verification: 13 integration tests (per-card parse-fidelity + discriminating runtime
ON/OFF with named revert-probes; Intrepid drives the real {2}-exile -> pool -> cast ->
finality path; owner=You + P/T + timing gates each discriminated). Corpus coverage:
+4 GAINED (Noctis, Intrepid, Leonardo, Hedonist's Trove), REGRESSED=0 corpus-wide;
per-card diff of every status-changed card confirms all changes are correct gains or
a clean decline (Dawnhand).
Assisted-by: ClaudeCode:claude-opus-4.8
…ter, Freestrider Commando)
Make the parser recognize+attach the rules-correct condition on 2 Standard cards
whose "enters-with-counter gated by a condition" clause was recognized by the swallow
detector but dropped — clearing SwallowedClause{Condition_If} and gating the runtime
correctly. ZERO new engine enum variants, fields, or types: both fixes attach
pre-existing typed conditions onto structures the parser already builds, plus one
additive engine reader change.
- Freestrider Commando (OTJ, CR 601.2h + CR 608.2c): "if it wasn't cast or no mana was
spent to cast it" -> a single rules-correct leaf StaticCondition::QuantityComparison{
Ref(ManaSpentToCast{SelfObject, Total}), EQ, Fixed{0} } (both disjuncts <=> 0 mana
spent; Plot is a cast per CR 702.170d, so the mana disjunct is load-bearing). The
whole-phrase recognizer is hoisted to ARM 1 of parse_single_inner_condition so the
phrase collapses to one leaf (not Or[Not(WasCast), ...], which cannot map to a
ReplacementCondition); it flows through the existing extract_enters_with_only_if_suffix
-> replacement_condition_from_static (QuantityComparison -> OnlyIfQuantity) pipeline.
Blacksnag Buzzard is the structural precedent.
- Undead Sprinter (DSK): (a) the game-state gate "if a non-Zombie creature died this
turn" -> StaticDefinition.condition = QuantityComparison{ Ref(ZoneChangeCountThisTurn{
Battlefield, Graveyard, Typed(Creature + Non(Subtype("Zombie"))) }), GE, Fixed{1} }
(a filtered arm of parse_creature_died_this_turn_conditions, placed after the bare
literal so Morbid keeps the unfiltered ref); (b) the linked "If you do, this creature
enters with a +1/+1 counter" rider -> the BB3 enters_with_counter field =
Some(CounterType::Plus1Plus1), via a new "if you do" split marker + a "~ enters with "
subject arm ("this creature" normalizes to ~ before the graveyard-permission parser)
+ a rest.is_empty() -> rest.trim().is_empty() fix for the two-sentence residual.
The one engine change (CR 607.1 + CR 601.3 + CR 113.6b): a self-granting fallback in the
BB3 finalize_cast reader selected_static_permission_enters_with_counter. Undead Sprinter
grants ITSELF the graveyard-cast permission, so at the finalize seam the source has moved
graveyard->Stack and its Graveyard-scoped permission no longer "functions in zone"
(CR 113.6b) -> the primary active_static_definitions path yields None -> the +1/+1 would
never be placed. The fallback reads the enters-with rider directly from the printed def
(the permission that authorized this cast is committed in casting_variant; the rider is a
linked ability to the directly-printed permission per CR 607.1). It fires ONLY when the
primary path is None via .or_else(), so BB3's separate-battlefield-source cards
(Noctis/Leonardo/Intrepid, whose permission still functions in zone) are byte-identical.
It is a class fix for any self-granting-permission-with-rider card.
The class-level building blocks correctly generalize beyond the 2 targets (verified by
corpus warndiff): Hundred-Battle Veteran (a self-granting graveyard-cast card with an
"if you do -> finality counter" rider) now captures + places its finality rider (was
silently dropped). Ebondeath, Dracolich's name-negation "a creature not named Ebondeath
died this turn" is correctly NOT claimed by the filtered died arm (name-negation leftover
-> arm rejects) -> it stays a clean gap (a separate follow-up). Primeval Spawn is
byte-identical.
Scope: the finality/+1/+1 counter is PLACED. The Freestrider stale-field edge
(mana_spent_to_cast_amount is not scrubbed in reset_for_battlefield_entry, so a
countered-then-reanimated-same-object Freestrider mis-reads a stale value) is a
pre-existing codebase-wide field-hygiene bug (shared by Lavinia/Satoru mana-spent
readers) that the single-leaf inherits; it is correct on every realistic cast path
(hard-cast 0 / Plot 2 / reanimate-fresh 2). The rules-correct fix needs a per-incarnation
was-cast disjunction (ReplacementCondition::Or, absent) and is filed as a follow-up with
the reset-hygiene fix as one coherent cluster.
Verification: 11 integration tests (per-card parse-fidelity + discriminating runtime
ON/OFF with named revert-probes; the reader-fallback discriminator flips counter 1->0
when disabled; the Freestrider gate flips 0->2 counters on a hard-cast when the condition
is dropped). Corpus coverage: +2 GAINED (Undead Sprinter, Freestrider Commando),
REGRESSED=0 corpus-wide; the whole-corpus per-card diff confirms the only other change is
the correct Hundred-Battle Veteran generalization.
Assisted-by: ClaudeCode:claude-opus-4.8
…es (Molten Exhale, Blasphemous Edict, Sandman's Quicksand)
Make the parser recognize+attach the rules-correct condition on 3 Standard cards
whose cast-context / alternative-cost clause was recognized by the swallow detector
but dropped — clearing SwallowedClause{Condition_If} and gating the runtime
correctly. ONE new engine leaf (CastVariantPaid::Mayhem); no new fields or types.
- Molten Exhale (TDM, CR 601.2f + CR 702.8): swallow-only false-positive. The
"you may cast this as though it had flash if you behold a Dragon as an additional
cost" clause is ALREADY represented + runtime-enforced (casting_options[0] =
AsThoughHadFlash + Behold(Dragon); flash_timing_cost grants the flash only if the
behold is payable, and the behold is collected as AdditionalCost::Required). The
swallow fired only because has_pay_phrase (swallow_check.rs) recognized "if you
pay" but not "as an additional cost". Broaden it; the structural AND-half
(casting_options[].cost.is_some()) keeps genuinely-unrepresented "if" clauses
unexempt. Zero parser/engine change.
- Blasphemous Edict (DFT, CR 118.9): the alternative cost "you may pay {B} rather
than pay this spell's mana cost if there are thirteen or more creatures on the
battlefield" is gated by a game-state count. New parse_type_count_on_battlefield
arm in parse_quantity_ref -> QuantityRef::ObjectCount { filter }. Routed through
ObjectCount (which counts battlefield objects for both controllers), NOT the
battlefield-blind ZoneCoreTypeCardCountAtLeast (whose player_zone_ids returns
empty for Battlefield -> would brick the alt-cost at 0 >= 13 forever). The
casting-option gate (can_cast_object_now -> payable_spell_alternative_cost) was
already wired.
- Sandman's Quicksand (EOE, CR 702.187b + CR 608.2c): "all creatures get -2/-2;
if this spell's mayhem cost was paid, creatures your opponents control get -2/-2
instead" -> a modal effect gated on the alt-cost-paid cast context. New
CastVariantPaid::Mayhem (a unit leaf, sibling of Warp in the established
per-keyword family) + a ("mayhem cost was paid", Mayhem) row in
CAST_VARIANT_COST_PAID_PHRASES + widening the Emerge-only membership filter to
matches!(v, Emerge | Mayhem) + a cast-time stamp in
finalize_cast_with_phyrexian_choices (mirrors the adjacent Foretell/Impending
arms; fires only for CastingVariant::Mayhem, so existing Mayhem cards gain an
inert marker). The resulting ConditionInstead{CastVariantPaid{Mayhem, Source}}
AST is byte-identical to shipping Adipose Offspring modulo the variant string.
cast_variant_paid is reset in reset_for_battlefield_entry (CR 400.7) and read the
same resolution -> no staleness window.
parse_quantity_ref arm placement: the arm is last-priority within the combinator
with an end-anchor guard, so it cannot shadow parse_greatest_commander_mana_value_ref
("...commander you own on the battlefield or in the command zone") and only claims
previously-failing bare "<type> on the battlefield" forms. It feeds the full phrase
to parse_type_phrase, preserving the InZone(Battlefield) filter property.
Class-generalization (build-for-the-class, verified by whole-corpus warndiff):
the parse_quantity_ref arm correctly generalizes to 7 previously-mishandled cards,
all gains, REGRESSED=0:
- portcullis: SwallowedClause cleared; condition attached as
ObjectCount{Creature + Another + InZone(Battlefield)} GE 2 ("two or more OTHER
creatures on the battlefield" — the Another prop is captured, no over-count).
- planar collapse / impending disaster: fix a latent CR 603.4 rules bug — the
intervening-if trigger condition was None, so they fired UNCONDITIONALLY every
upkeep (destroy all creatures/lands regardless of the 4+/7+ gate). Now correctly
typed QuantityCheck GE 4/7 and enforced at resolution.
- neck tangle: condition attached (Heads GE 5); the "Otherwise" branch is promoted
from a buried Unimplemented to a structured else_ability (Unimplemented 2 -> 1);
the main tap-Heads effect is intact.
- glacial crasher / harbor serpent / hour of revelation: gain an explicit
InZone(Battlefield) on a filter that already defaulted to Battlefield at runtime
(runtime-equivalent, more Oracle-faithful).
Verification: 9 integration tests (per-card parse-fidelity + discriminating runtime).
Blasphemous: alt-cost AVAILABLE at 13 creatures / UNAVAILABLE at 12 on identical
boards via can_cast_object_now. Sandman: mayhem cast shrinks opponents only; the
cast-time stamp is load-bearing (a stamp-revert-probe flips the controller's own
creature from unaffected to -2/-2). Planar Collapse: fires at 4 creatures, fizzles
at 3 (the runtime lock on the latent-bug fix, via the production upkeep -> resolve
path). issue_2354 (Norman Osborn Mayhem) stays green. Corpus: GAINED = 4 (3 targets
+ portcullis), REGRESSED = 0 corpus-wide; the whole-corpus per-card diff confirms
the only other changes are the 6 correct class-generalizations above.
Assisted-by: ClaudeCode:claude-opus-4.8
…y alt-cost & goad "if able" (Valgavoth, Maximum Carnage)
Two Standard cards fired SwallowedClause{Condition_If} as swallow-detector
FALSE-POSITIVES: their representations already exist AND are runtime-enforced, so
the detector was spuriously demoting supported cards. Clear the false-positives —
swallow_check.rs only, zero parser/engine/variant change.
- Valgavoth, Terror Eater (DSK, CR 118.9 + CR 607.1 + CR 608.2c): "if you cast a
spell this way, pay life equal to its mana value rather than pay its mana cost"
is ALREADY represented as `ExileCastPermission.extra_cost = {PayLife(SelfManaValue),
mode: Alternative}` (built by the shipped `try_parse_alt_cost_rider`), and the
"would be put into an opponent's graveyard … exile it instead" replacement is
already represented + stripped. The alt-cost rider is the only unmatched "if". New
suppression helper `cast_this_way_alt_cost_is_only_if_marker` (sibling of
`enters_with_finality_this_way_is_only_if_marker`): fires when an Alternative-mode
exile/graveyard cast-permission rider is present and, after stripping that rider
via `try_parse_alt_cost_rider`, no OTHER bare " if " survives. Keys on
`CastCostMode::Alternative` ONLY, so `Additional`-mode riders (Festival of Embers)
are untouched. Class: every exile/graveyard cast-this-way permission with a
CR 118.9 alternative-cost rider.
- Maximum Carnage (BLB, CR 508.1d + CR 701.15b): chapter I "each creature attacks
each combat if able and attacks a player other than you if able" already lowers to
`Effect::GoadAll`, which runtime-enforces the must-attack-if-able requirement
(shipped `goad.rs` resolver test). The `cond_markers` "if able" cluster omitted
`GoadAll`; add the `"type":"GoadAll"` marker.
Class-generalization (verified by whole-corpus warndiff): the GoadAll marker also
legitimately clears Kardur, Doomscourge ("creatures your opponents control attack
each combat if able and attack a player other than you if able" → `GoadAll{Opponent}`)
— a 2-card class proof, not a one-off.
Follow-up (out of scope): `GoadAll` also marks `goaded_by`, but these cards do not
goad (Maximum Carnage's ruling: the ability "doesn't cause any creatures to become
goaded"). That is a pre-existing over-lowering of non-goad "attacks if able" to
GoadAll, tracked separately; the swallow fix here is honest about the "if able"
clause specifically (GoadAll's must-attack DOES enforce it), independent of the
goaded-status over-representation.
The third BB8 card, The Tale of Tamiyo, is a genuine gap ("if two cards that share a
card type were milled this way, draw a card and repeat this process" — a shared
class with Grindstone). Its fix requires a new AbilityCondition leaf plus a shared
chain-body change so the milled set survives the intervening draw; that change has a
codebase-wide verification surface, so it is deferred to a focused standalone engine
PR and left as honest `Unimplemented` here — not silently half-fixed.
Verification: 7 swallow-detector unit tests (revert-probes discriminating and
non-vacuous — parsed with real card context so no Unimplemented early-return
short-circuits the audit) + 1 runtime test pinning the Alternative-vs-Additional
axis (Alternative zeroes the mana cost, Additional keeps it due). Corpus: exactly 3
cards clear {Valgavoth, Maximum Carnage, Kardur}, each with real backing
(`extra_cost:Alternative` / `GoadAll`), REGRESSED=0 corpus-wide, no masked drops;
The Tale of Tamiyo still carries its Condition_If (honest deferral).
Assisted-by: ClaudeCode:claude-opus-4.8
…terball / Match-the-Odds class)
A counter anaphor ("it" / "that creature" / "that token" / "the token") placed
after a token-creating clause in the same resolution chain was misbinding to the
source (`SelfRef`) or an unbound `ParentTarget` instead of the created token
(`TargetFilter::LastCreated`) — so the counter was a no-op or landed on the wrong
object. This is a LIVE rules bug: Otterball Antics, flipped to "supported" by the
L02 cast-origin condition (BB2, ce4b00ddc), silently placed 0 counters on its Otter
token on every graveyard/flashback cast. The same root affects a pre-existing
for-each cluster (Match the Odds, Lasting Fayth, …).
Two parser dispatch paths produced the misbind; both now converge on one shared
building block — no new enum variant or field, parse-time AST only (`LastCreated`
already resolves at runtime):
- New helper `counter_anaphor_created_token_binding(anaphor, ctx) -> Option<TargetFilter>`
(oracle_effect/counter.rs), keyed on the existing `ctx.token_created_in_chain`.
Recognizes the anaphor CLASS via a nom `alt` — `is_it_pronoun` ∪ `{that creature,
that token, that permanent, the token, the permanent}` — returning
`Some(LastCreated)` iff `token_created_in_chain && (is_it_pronoun(a) || ctx.subject
∈ {None, SelfRef, Any})`. The `is_it_pronoun` path is ungated (identical to the
prior inline it-branch — behavior-preserving); the demonstratives are subject-gated
so Pip-Boy's non-self "that creature → TriggeringSource" (resolved earlier by
`resolve_that_creature_in_trigger`) is preserved. "the creature" is excluded (it is
ambiguous — Longstalk Brawl legitimately binds it to a chosen target slot).
- Call site A — `resolve_counter_placement_target` (counter.rs): the it-branch is
refactored to `helper(...).unwrap_or_else(resolve_it_pronoun)` (byte-identical for
all it inputs), and a new demonstrative arm is added AFTER the
`resolve_that_creature_in_trigger` check.
- Call site B — `try_parse_for_each_effect` counter arm (mod.rs): now consults the
same helper with the REAL seeded `ctx` before the
`parse_subject_application(ParseContext::default())` fallback (which had discarded
the seeded ctx → `token_created_in_chain=false`). The for-each dispatch intercepts
before the imperative counter path, so the two call sites are disjoint per clause.
Class fix, verified by whole-corpus per-card diff: exactly 11 cards flip a single
PutCounter target `SelfRef`/`ParentTarget → LastCreated` — animal friend, elemental
my dear, furnace-blessed conqueror, grist the plague swarm, intrude on the mind,
kianne, lasting fayth, match the odds, otterball antics, outlaw stitcher, retrieve
the esper — REGRESSED=0, each rules-correct against Oracle text. Guards unchanged:
the 5 existing `LastCreated` "it" cards, North Pole Research Base ("it" = a tapped
real target → `ParentTarget`), Immaculate Magistrate (for-each "target creature" →
`Typed`), the 9 legitimate self-ref counters, and the out-of-scope cards.
Scope: the reflexive "When you do" trigger-body case (Iroh, Tea Master) is NOT fixed
here — its blocker is a distinct `token_created_in_chain`-seeding gap in the
reflexive body parse, not this parse-time anaphor helper; it stays `ParentTarget`
before and after (no regression) and is a standalone follow-up. The
"the token" ≠ always-the-created-token counterexamples (Elder Arthur Maxson /
Phyrexian Cytoshaper, where "that token" is a triggering/granted token) confirm the
`token_created_in_chain` guard is load-bearing — a flag bypass would be unsound.
Verification: 4 discriminating runtime tests measuring RESOLVED counter deltas via
the production parse+resolve path, with revert-probes: Match the Odds (call site B,
3 counters on the token → 0 on revert), grist the plague swarm (call site A
demonstrative + "the token", 1 → 0 on revert, reach-guarded so a condition-fizzle
can't pass vacuously), Applied Geometry (it-branch, exactly 6 counters — a
double-seed would be 12), and Longstalk Brawl ("the creature" stays a chosen-target
binding, not `LastCreated`).
CR 608.2c (read the whole text; a later same-effect anaphor binds the just-created
object) + CR 111.10 (predefined token creation) + CR 122.1 (a counter is a marker on
an object).
Assisted-by: ClaudeCode:claude-opus-4.8
…Permission land-play construction (BB3 gap) BB3 (d09e2bb7a) added the required field `enters_with_counter: Option<CounterType>` to `StaticMode::GraveyardCastPermission` but never threaded it into the sole cross-crate construction site in mtgish-import (`MayPlayLandsFromGraveyard` → `GraveyardCastPermission`). BB3 was verified only with `cargo check -p engine`, which never compiles the sibling mtgish-import crate, so the workspace build shipped red. `None` is rules-correct: playing a land from the graveyard is a special action (CR 116.2a), never a spell (CR 305.1), so it is never cast and never enters with a finality counter (CR 122). Assisted-by: ClaudeCode:claude-opus-4.8
… cast-origin (CR 707.10)
A resolving-spell cast-origin condition ("if this spell was cast from anywhere
other than your hand"), lowered by BB2 to `AbilityCondition::Not(CastFromZone{Hand})`,
over-fired on a spell COPY: a copy has `cast_from_zone = None` (CR 707.10 — a copy
of a spell isn't cast; CR 400.7 — a new object with no memory of prior existence),
so `Not(None == Some(Hand))` = `Not(false)` = TRUE, wrongly granting the copy's
effect (Antiquities on the Loose's `PutCounterAll`) even though a copy was never
cast. Correct evaluation is FALSE.
Two-part fix, both semantics-preserving for existing producers:
- Option-B rename `AbilityCondition::CastFromZone{zone:Zone}` →
`WasCast{zone:Option<Zone>}`, mirroring the pre-existing `StaticCondition::WasCast`
and `TriggerCondition::WasCast` (parameterize an existing axis rather than
proliferate a sibling variant). Eval:
`cast_from_zone.is_some_and(|cz| zone.is_none_or(|z| cz == z))` — a copy's `None`
yields false; `zone: None` means "cast from any origin". Serde
`#[serde(alias = "CastFromZone")]` + `skip_serializing_if` keep old payloads
deserializable and `Some(X)` bodies byte-stable.
- Restore the dropped ∃cast conjunct ONLY on the "anywhere other than X" producer:
`And[WasCast{None}, Not(WasCast{Some(Hand)})]`. The sibling "you didn't cast it
from X" producer is DELIBERATELY left bare `Not(WasCast{Some(X)})` — its opposite
semantics (¬(cast ∧ origin=X) → a reanimated/copied Phage-style card still
triggers "you lose") is canonical and must not gain the ∃cast guard.
Whole-corpus coverage: REGRESSED=0; the ~25 cards that change serialization shape
all do so by exactly the rename or the And-wrap (structural per-node diff, zero
unexpected residual). The rename is coverage-neutral (the coverage report key
stays "CastFromZone"; And / WasCast are all Handled). `cargo check --workspace`
is green (the rename also lands in mtgish-import/src/convert/condition.rs).
Verification: `l02_bbfu4_copy_cast_origin.rs` casts Antiquities from hand, resolves
a COPY off the stack via the production pipeline, and asserts the copy grants 0
counters — MEASURED discriminating: reverting the And-wrap flips it to counters
[0,1,0,1] (sum 2). A parse-level fence
(`bbfu4_only_anywhere_other_than_gains_existential_cast_conjunct`) proves the ∃cast
wrap lands on the "anywhere other than" arm only, leaving "didn't cast it" bare.
Phage's "didn't cast it ⇒ lose" reaches the engine via `TriggerCondition::WasCast`
(untouched), asserted by a parse-attribution fence.
CR 707.10 / CR 601.2a / CR 400.7 / CR 603.4.
Assisted-by: ClaudeCode:claude-opus-4.8
… loss (untouched TriggerCondition::WasCast path)
Adds a runtime regression fence for the "if you didn't cast it from your hand,
you lose the game" trigger family (Phage the Untouchable), which had zero
runtime loss-outcome coverage. This path is UNTOUCHED by the WasCast copy-
correctness change — Phage lowers to `TriggerCondition::Not(WasCast{Hand})`,
whereas that change renamed only `AbilityCondition::CastFromZone`. The fence
therefore cannot flip on reverting the rename; it is a standing regression
fence, not a delta discriminator (labeled as such in-code).
Non-vacuity is the positive+negative pair on the same card with opposite cast
origin: a reanimated Phage (put onto the battlefield, cast_from_zone=None per
CR 400.7 → "didn't cast it from your hand" TRUE) eliminates its controller
(CR 104.3e), while a hand-cast Phage (WasCast{Hand} TRUE → Not(..) FALSE) does
not. Reuses this file's existing {0}-Reanimate pattern — no new scaffolding.
CR 104.3e / CR 400.7 / CR 800.4a.
Assisted-by: ClaudeCode:claude-opus-4.8
…Turn ledger, not the live board (CR 608.2i)
The bare-subject "entered this turn" count/existence gate (Gargoyle Flock's
"if a creature entered the battlefield under your control this turn…", Oath of
Chandra/Liliana, Akal Pakal, …) parsed to `QuantityRef::EnteredThisTurn`, whose
runtime read counts only objects still on the battlefield (`zone == Battlefield`).
A creature that entered under your control this turn and then died, was bounced,
or was sacrificed before the gate evaluates was excluded → the look-back
under-counted. CR 608.2i: an effect that looks back in time for objects it takes
no action on does not require them to still be in the zone they were in — an
explicit exception to CR 608.2h. This is a pre-existing (inherited) upstream
inconsistency: the sibling forms ("N or more … entered", the "~ or another"
disjunct) already read the departure-surviving `BattlefieldEntriesThisTurn`
ledger; only this bare-subject branch was left on the live board.
Fix (surgical mirror of the migrated siblings): drop `inject_controller_you` and
emit `QuantityRef::BattlefieldEntriesThisTurn { player: PlayerScope::Controller,
filter }` — byte-identical in shape to `parse_or_more_entered_count`. No new
variant; the ledger read and its matcher (controller / type / `FilterProp::Another`)
already exist. "under your control" moves from a filter-baked `ControllerRef::You`
to `PlayerScope::Controller`.
Sub-form boundary held: the same-named TargetFilter form ("if the creature you
control entered this turn, put a +1/+1 counter on it", Malamet Battle Glyph) is a
legal-target form and stays on the live board — a departed object is not a legal
target (CR 608.2h). It is deliberately untouched.
This is a parser change: the `parse_details` serialization of the ~32 bare-subject
"entered this turn" count cards flips `EnteredThisTurn → BattlefieldEntriesThisTurn`
(controller off the filter, `PlayerScope::Controller` on the variant). Both variants
are `Handled` in coverage → whole-corpus REGRESSED=0 (a representation-only change,
the intended ledger-consistency migration). The coverage-parse-diff bot will flag
these cards; that flag is expected here, not a regression.
Verification: a runtime look-back test on Gargoyle Flock resolving through the
production `BattlefieldEntriesThisTurn` arm — a creature that entered this turn then
departed still satisfies the gate; reverting to the old `EnteredThisTurn` emit makes
the departed creature fail the `zone == Battlefield` check → count 0 → gate FALSE →
the assertion flips red — paired with an entered-and-stayed no-regression control, an
opponent-entrant control proving `PlayerScope::Controller`, and a parse-boundary lock
on Malamet proving the target form stays `TargetMatchesFilter { use_lki: false }`
(flips if the act-on form is ever over-migrated). The 3 existing parse tests are
migrated to the new authority.
A sibling producer (`parse_entered_this_turn_ref`, the "[type] that entered this
turn" surface) has the same live-board bug but an optional any-player controller that
`BattlefieldEntriesThisTurn`'s `PlayerScope` cannot yet express; tracked as a separate
follow-up.
CR 608.2i / CR 608.2h.
Assisted-by: ClaudeCode:claude-opus-4.8
… Stalwart's first-time gate (CR 603.2c)
Stalwart Successor ("Whenever one or more counters are put on a creature you control, if
it's the first time counters have been put on that creature this turn, put a +1/+1 counter
on that creature") missed its trigger whenever the turn's first counter placement on a
creature added two or more DIFFERENT counter types at once. BB4 (this branch) introduced the
`FirstTimeObjectCountersAddedThisTurn` condition with a per-record `count(object_id) == 1`
model and documented this as the R7 fail-safe ceiling; this is that fix.
Tracing showed the ceiling masked a second bug: the "one or more counters" CounterAdded
trigger was `batched:false`, so a multi-type placement (two `CounterAdded` events) was
checked per-event — `count() == 1` was doing double duty as both the first-time gate AND the
only guard against a double-fire. A condition-only fix would over-fire. The correct fix is
two-layered:
- Batch the class (R1, CR 603.2c). Set `batched=true` for the "one or more counters"
CounterAdded form, gated at the effect-known lowering site behind
`!matches!(effect, Effect::Unimplemented)` so the three "same number and kind" reproduction
cards (Bold Plagiarist, Aragorn Company Leader, Captain Marvel Apex Avenger) — whose
per-kind reproduction isn't a typed effect yet — stay `batched:false`. This batches 7
CounterAdded triggers (Stalwart, All Will Be One, Generous Patron, Kros Defense Contractor,
Putrid Hexhag, Rikku Resourceful Guardian, and Danny Pink's granted trigger), collapsing a
multi-type placement to one firing per placement occurrence.
- Aggregate the batched magnitude (CR 608.2). Move `CounterAdded` out of the zero-arm of
`count_matching_trigger_event_subjects` into a dedicated arm returning the counter magnitude,
so a batched "deals that much damage" trigger (All Will Be One) reads the total counters
across the batch rather than `Some(0)`. Reachable only via the batched path; non-batched
triggers are unaffected (they read `extract_amount_from_event`).
- Occurrence-count the first-time gate (R2, CR 122.1 + CR 603.4). Add
`object_counter_placement_count_this_turn`, an occurrence ledger bumped once per object per
batch (mirroring the already-correct `object_tap_count_this_turn` /
`FirstTimeObjectTappedThisTurn` sibling), and read `FirstTimeObjectCountersAddedThisTurn`
from it (`== Some(1)`) instead of scanning per-type records. The new map is EXCLUDED from
`GameState`'s `PartialEq` (mirroring the tap map) so this per-turn observational counter does
not perturb loop-state board-equality.
The three excluded per-kind cards are deferred to a standalone follow-up (batched
counter-trigger per-kind reproduction aggregation); the scope predicate keys on the
`Effect::Unimplemented` variant and must be re-gated when that reproduction becomes a typed
effect.
Verification: three revert-probes, each measured to flip — reverting R1 makes All Will Be One
deal `[1,1]` (two per-event fires) not `[2]`; reverting the magnitude arm makes it deal `[]`
(0); reverting R2 makes Stalwart's multi-type-first fire zero times. Whole-corpus coverage
REGRESSED=0, FORGE `ordering_parity_sweep` delta=0, and the card-data snapshot delta is the
`batched` flag on exactly those 7 cards (the three per-kind cards byte-identical).
CR 603.2c / CR 608.2 / CR 122.1 / CR 603.4.
Assisted-by: ClaudeCode:claude-opus-4.8
…on dispatch — structural self-vs-other "its" binding (CR 603.6c)
The dying-object P/T intervening-if gate ("When ... dies, if its <stat> was N",
`parse_dying_object_pt_comparison_condition`) disambiguated self-death from
other-death by a PROXY: "did `parse_inner_condition` already hoist the clause?"
That proxy is correct today only by a coverage asymmetry
(`parse_source_power_toughness_condition` is postfix-only), and would mis-bind an
other-death POSTFIX card ("a creature an opponent controls dies, if its power was
3 or greater") to the SOURCE instead of the dying event object. No such card
exists in the DB yet — this is a latent hardening of BB4's own gate, retiring a
fragile proxy and its `ponytail:` IOU.
Replace the proxy with a structural discriminator: thread the trigger's dying
subject (`Option<&TargetFilter>`) down the single-call-site extract chain
(`extract_if_condition_with_card_name` → `try_extract_zone_change_object_filter_condition`
→ `parse_zone_change_object_filter_condition` → the gate). The production caller
reuses the existing `extract_trigger_subject_for_context` result (hoisted above
the extract call; the intervening window is ctx-inert and the call is
diagnostics-neutral, so the hoist is behavior-preserving). The gate becomes a
3-arm match:
- `Some(SelfRef)` → defer to the `parse_inner_condition` Source-binding path (CR 201.5);
- `Some(_other)` → bind the dying event object (CR 603.6c leaves-battlefield + CR 603.10a look-back);
- `None` (the `#[cfg(test)]` wrapper only) → keep the retired proxy as a behavior-identical fallback.
Also corrects a CR miscite this gate has carried since BB4: the dying gate cited
CR 603.6a (enters-the-battlefield) — replaced with CR 603.6c (leaves-the-
battlefield) + CR 201.5 (self-reference binds to source) + CR 603.10a (zone-change
look-back for the past-tense "was" snapshot) + CR 603.4 (intervening-if). The
ENTERING sibling's 603.6a is correct and left untouched.
Verification: whole-corpus card-data BYTE-IDENTICAL (MD5 match, 0 of 35396 cards
differ) — a representation-stable refactor; all three real dying-PT cards
(Deathknell Berserker / Massacre Girl / Jason Bright) unchanged; GAINED=0,
REGRESSED=0. The new synthetic other-death-POSTFIX test drives `parse_oracle_text`
end-to-end (the production `Some` path, not the `None` wrapper) and is
revert-probe discriminating: reverting the threading flips it from
`ZoneChangeObjectMatchesFilter{power>=3}` (event object) to
`QuantityComparison{Power(Source)}` — measured red pre-fix, green post-fix.
Massacre runtime + snapshot tests green; `cargo check --workspace` green.
CR 603.6c / CR 201.5 / CR 603.10a / CR 603.4.
Assisted-by: ClaudeCode:claude-opus-4.8
…it check_swallowed_clauses phase-rs#5668 ("per unit swallow audit") changed `check_swallowed_clauses` from the 3-arg `(oracle_text, parsed, diagnostics)` form to a 5-arg `pub(crate)` form taking lowering-internal `items`/`tracks` that a mutated `ParsedAbilities` cannot supply. The rebase is otherwise clean — every L02 exemption lands in detector-body regions phase-rs#5668 left intact — so only BB8's three direct-call tests needed adaptation: - `valgavoth_alt_cost_marker_is_load_bearing` + `maximum_carnage_goadall_marker_is_load_bearing`: retarget to the unchanged private `detect_condition_if` (the detector the markers live in), preserving the mutate->re-fire non-vacuity proof in BOTH directions — with the marker mutation the Condition_If swallow re-fires; on the un-mutated baseline it is suppressed. Both cards are coverage-supported (zero `Effect::Unimplemented`), so phase-rs#5668's per-unit unimplemented gate is a genuine no-op and the detector-level assertion equals production behavior. - `valgavoth_alt_cost_does_not_hide_unrelated_if`: re-expressed via phase-rs#5668's own `conditional_enter_counters_does_not_hide_unrelated_if` template — through-parse the card plus an unrelated, genuinely-unrepresented "if" clause and assert it still swallow-flags, proving the alt-cost marker doesn't over-suppress. No exemption logic changed; phase-rs#5668's audit rewrite (swallow_check.rs + oracle_ir/feature.rs) is fully preserved. Assisted-by: ClaudeCode:claude-opus-4.8
lgray
force-pushed
the
feat/std-l02-condition
branch
from
July 12, 2026 16:36
348a059 to
411e6d7
Compare
Contributor
Author
|
🤖 AI text below 🤖 Thanks for the detailed review — every point measured and addressed. Rebased onto #5668 ( B1 — population (69 cards / 66 signatures)Fully accounted: ~30 named targets + 39 legitimate class-collateral, dominated by two deliberate class-wide rules migrations — 28 "entered this turn" counts moved to the departure-surviving B2 — the two capability removals (measured against the true pre-BB3 parent
|
matthewevans
requested changes
Jul 12, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Verdict: changes requested — the finality-counter lane is rules-incomplete and coverage-unsafe.
🔴 Blocker
-
Finality is serialized as an inert generic counter, not enforced as CR 122.1h's replacement effect. Evidence:
crates/engine/src/types/counter.rs:64represents it asGeneric(String);crates/engine/src/game/casting_costs.rs:6901only adds the pending ETB counter; andcrates/engine/src/game/replacement.rs:5286synthesizes intrinsic counter replacements only for shield counters, with no battlefield-to-graveyard finality candidate. The new integration suite explicitly limits itself to placement, “never death→exile,” atcrates/engine/tests/integration/l02_bb3_cast_permission_finality.rs:16. Intrepid Paleontologist's Oracle text says: “If a creature with a finality counter on it would die, exile it instead.” CR 122.1h defines the replacement as: “If this permanent would be put into a graveyard from the battlefield, exile it instead.” A finality-countered creature therefore still reaches the graveyard after destruction, sacrifice, or an SBA in this branch.Recommendation: route this through the full engine cycle before merging. Add the typed finality-counter/replacement building block at the shared zone-change replacement seam (covering destroy, sacrifice, and SBA), with discriminating runtime tests for each; otherwise preserve strict unsupported coverage for these cards instead of clearing their condition warnings.
🟡 Non-blocking
- No additional current-head design finding. The maintainer merge advanced the head to
c49e6a8; its CI and parse-diff are correctly pending and must be fresh before the next review.
✅ Clean
- The earlier apparent duplicate Fearless Swashbuckler condition is a formatter loss, not a duplicated parse:
crates/engine/src/parser/oracle_trigger.rs:4089retains distinct Pirate and Vehicle filters, whilecrates/engine/src/game/coverage.rs:3907deliberately formats only comparator/count. - The prior
MovedandPayCostobservations were rechecked against the current implementation: the cast-permission/counter plumbing is at the correct selected-permission seam, and Blasphemous Edict's printed “pay {B} rather than pay this spell's mana cost” is an alternative-cost shape. The remaining defect is the missing generic finality rule, not those representation changes.
Recommendation: keep this PR in requested-changes state; regenerate the parse-diff and rerun current-head CI after the rules-complete finality implementation.
…iling Adds finality_counter_death_to_exile.rs: one test per distinct death path (destroy, sacrifice, SBA lethal damage, SBA zero-toughness), each driving the real pipeline (.resolve() / check_state_based_actions) and asserting the finality permanent lands in exile, not the graveyard. Each carries a negative twin (no finality counter -> graveyard) as the reach-guard proving the driver actually kills, plus a dies-trigger differential on the cast + SBA delivery classes (CR 700.4 -- the redirect is a true "instead", not graveyard-then-exile). Revert-probe: neutralizing the injection arm flips all four positives Exile->Graveyard. Flips the l02_bb3 finality suite's placement-only ceiling (the "never death->exile" comment) now that CR 122.1h is enforced. CR 122.1h + CR 700.4 + CR 400.7. Assisted-by: ClaudeCode:claude-opus-4.8
Contributor
Author
|
🤖 AI text below 🤖 Implemented the full CR 122.1h finality death→exile replacement (Option A) — finality counters now redirect a dying permanent to exile instead of the graveyard, not just get placed. Pushed as 3 commits on top of your merge Architecture. It mirrors the shield-counter intrinsic replacement, but on the correct seam: shield replaces Design calls. Promoted the counter to a first-class typed Tests. Four discriminating integration tests, one per distinct death path (destroy / sacrifice / SBA-lethal-damage / SBA-zero-toughness), each driving the real pipeline and asserting exile-not-graveyard, each with a negative twin (no counter → graveyard, the reach-guard proving the driver actually kills) and a dies-trigger differential proving true "instead" (CR 700.4 — the exiled permanent never reaches the graveyard, so CI is green (clippy |
Port typed swallow evidence with cast-permission rider coverage. Co-authored-by: Lindsey Gray <lindsey.gray@gmail.com>
# Conflicts: # crates/engine/src/parser/swallow_check.rs
matthewevans
approved these changes
Jul 13, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Approved after resolving the remaining finality runtime, visibility, and parser-audit feedback; current-main CI is green.
matthewevans
added a commit
that referenced
this pull request
Jul 13, 2026
Two PlayerFilter imports lost their last user when #5677's HasCounters arm replaced the bespoke lowering, and the describe() test helper lost its callers when SharedRestrictionParse::Unsupported dropped its payload. Local 'cargo test' passes with warnings; CI's -D warnings does not.
jsdevninja
pushed a commit
to jsdevninja/phase
that referenced
this pull request
Jul 13, 2026
* refactor(parser): make parse_inner_condition the primary restriction grammar
CR 601.3 / CR 602.5: a casting/activation restriction is an ordinary game-state
condition, so it must be recognized by the same combinators that recognize the
identical phrase in an "as long as" static. Invert the ordering:
parse_restriction_condition now runs all_consuming(parse_inner_condition) FIRST
and converts, and the bespoke restriction grammar is a narrow fallback.
- Tri-state SharedRestrictionParse {NoMatch, Converted, Unsupported}. A phrase the
shared grammar UNDERSTOOD but ParsedCondition cannot hold exactly fails the parse
outright; it is never re-read by the fallback, which would produce a weaker
reading of the same words.
- static_condition_to_restriction_condition is exhaustive over all 57 StaticCondition
variants with no wildcard arm, so a new variant is a compile error rather than a
silently permissive restriction. Counter bands are rejected, never widened into an
"at least" that drops the maximum (CR 122.1 / CR 711.2a).
- Compound conditions move into parse_inner_condition as one parameterized connective
combinator; the legacy " and "/" or " raw-string splitting is deleted. A string split
cannot see a connector sitting INSIDE an atomic leaf; requiring both sides to parse
as complete conditions is what makes the decomposition safe. n-ary chains nest
instead of leaving an unconsumed — and silently swallowed — tail.
- CR 903.3d: "you control a commander" converts to an ObjectCount over the IsCommander
filter. It previously became subtype "commander" — a subtype no permanent has — so
Deflecting Swat's free-cast condition could never be satisfied.
- An unparsed condition must never be stored as RequiresCondition { condition: None }.
That evaluates permissively (Option::is_none_or -> true) AFTER consuming the source
clause, so the ability reported as fully supported while being activatable in exactly
the situations the card forbids. All six activation sites now route through one
atomic commit_requires_condition authority that mutates nothing on failure, leaving
the clause for the ordinary Effect::Unimplemented fallback.
Full-pool dual export (population 35,396 faces): RequiresCondition{condition: null}
goes 50 occurrences / 48 faces -> 0. 42 faces become honestly unsupported: 35 were
that permissive lie (no restriction enforced at all), 3 were confidently-wrong parses
(a bare-subtype dump that could never be satisfied, a QuantityVsEachOpponent comparing
a value to itself, an "attacking or blocking" narrowed to "attacking"), and 4 are true
regressions in two named classes tracked as follow-ups. 1 gain, 15 dead parsers
deleted (-611 lines net).
* test(parser): pin the restriction-only fallback and prove the commit is atomic
Enforcement for the P02-U3 ruling. Both gates were watched failing before being
accepted; reverting the commit order reproduces the original defect verbatim as
[RequiresCondition { condition: None }, OnlyOnceEachTurn].
- restriction_only_fallback_dispatches_exactly_the_pinned_families: a STRUCTURAL pin,
not prose. The fallback's doc comment tells contributors not to add a phrase there;
a comment stops nobody. This reads the module's own source, extracts the dispatcher's
body, and rejects any parse_*(text) call outside the pinned six — each tagged with WHY
it is retained (restriction-context referent / vocabulary gap / phrasing gap pending
P02-U3b). Adding a seventh arm turns it red with the question the author must answer:
is the new phrase referentially special, genuinely inexpressible, or merely spelled
differently? Only the first two belong there.
- failed_activation_condition_commits_no_cadence_and_leaves_the_clause_unimplemented:
the witness for commit_requires_condition. An activation gate whose condition fails to
parse must commit NOTHING — not the cadence restriction sharing the sentence, not a
permissive RequiresCondition { condition: None }, and not the source text, which must
survive as Effect::Unimplemented.
- parsed_activation_condition_commits_both_cadence_and_condition: the positive
reach-guard, so the atomicity test cannot pass merely because the peeling branch never
ran. It stays green under the revert while the atomicity test goes red.
The source-scanning `.find` calls carry allow-noncombinator annotations: they parse this
module's own bytes, not Oracle text.
Test-only; parse output unchanged.
* fix(parser): drop dead-code fallout from the rebase (CI -D warnings)
Two PlayerFilter imports lost their last user when phase-rs#5677's HasCounters
arm replaced the bespoke lowering, and the describe() test helper lost
its callers when SharedRestrictionParse::Unsupported dropped its payload.
Local 'cargo test' passes with warnings; CI's -D warnings does not.
---------
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
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.
🤖 AI text below 🤖
Summary
L02 "condition lane": clears the
SwallowedClause{Condition_*}parse warnings on ~30 Standard target cards by attaching their typed conditions to the correct ability/trigger/replacement layer, built as 8 reusable building blocks (BB1–BB8) plus 5 in-branch follow-ups (BB-FU1–5) that harden the shared condition/quantity infrastructure. Net +34 newly-supported cards vs the #5668 base with zero engine regressions (coverage-regression gate: engine/oracle-changed 0); swallowed-clause diagnostics drop 1025→986. This revision additionally enforces the CR 122.1h finality death→exile replacement — 3 commits (typedCounterType::Finality→ theZoneChange{Battlefield→Graveyard}→Exileintrinsic replacement → a 4-path discriminating test suite) that make BB3's previously placement-only finality counters actually redirect a dying permanent to exile, resolving the maintainer's remaining blocker.Parse-change population (69 cards / 66 signatures): because the building blocks operate on card classes, the head-current parse-diff touches more than the ~30 named targets. The full population is enumerated + classified below — all are intended targets or correct same-signature class-collateral; every
*/REMOVEDpairs with an upgraded typed replacement (no capability loss).Parse-change population appendix (categorized, vs base
fa7f157fa)BattlefieldEntriesThisTurnledger (BB-FU1, CR 608.2i — departure-surviving look-back); 9 "make a token, put a counter on it" anaphors now binding to the token (BB-FU5); + Wilt in the Heat. Each an Oracle-verified same-typed-signature repair.ObjectCountzone made explicit (# of X→# of in-battlefield X); no behavior change.Capability-removal clarifications (reviewer-flagged, measured): Intrepid Paleontologist + Leonardo lost a garbage self-ETB
Movedmisparse (measured inert: counter on the wrong object, die→graveyard not exile) → replaced by correctExile/GraveyardCastPermission{finality}. Blasphemous Edict'sability/PayCost(a misparsed "pay {B} rather than mana cost") → correctAlternativeCostcasting option (BB7, tested). Finality die→exile enforcement (CR 122.1h) is now implemented in this branch (the 3 finality commits below), superseding BB3's original placement-only ceiling.Verify-flagged, measured correct/deferred: Lupine Prototype + Naktamun Lorespinner ("a player has ≤N cards") — measured proper existential-over-all-players (the
HandSizeplayer field is an inert placeholder read per-candidate). Happily Ever After — a pre-existingtheresentence-fragment misparse (coverage-unsupported, excluded from real play); tracked as a held follow-up, not introduced by this lane.Every unit went through
/engine-implementer(trace → plan →/review-engine-plan→ executor →/review-impl), was verified with a discriminating revert-probe, and was ratified at its commit boundary.Deferred follow-ups (surfaced by this series, tracked, gated on the user)
These were discovered during L02 and intentionally scoped OUT as standalone follow-ups (novel-vs-inherited disposition; none block the L02 deliverable):
execute-condition-path triggers lack a CR 603.4 fire-time intervening-if re-check (codebase-wide).Effect::GoadAll(spuriousgoaded_by).token_created_in_chain(Iroh class).parse_entered_this_turn_reflive-board→snapshot for the optional-controller any-player class (sibling of BB-FU1).batched:false).ReplacementCondition::Or(Freestrider / Lavinia / Satoru).110.5b→110.5+117.1ccite imprecision.(Formerly listed here: finality-counter death→exile CR 122.1h enforcement — now implemented in this branch, see the finality commits.)
Known ceilings (intentional, documented)
110.5b,117.1c) is deferred to the standalone CR-annotation sweep rather than touched piecemeal here.batched:false, gated onEffect::Unimplemented, pending the typed reproduction effect (BB-FU11).Implementation method (required)
Method: /engine-implementer
CR references
Primary rules implemented/annotated across the branch:
docs/MagicCompRules.txt). Notable correction: BB-FU3 fixes a BB4-introduced miscite on the dying gate (CR 603.6a[ETB] → CR 603.6c + 201.5 + 603.10a + 603.4).Verification
Full CI-equiv (worktree cargo, exact CI commands) — all GREEN at committed tip 5f7acb9 (the 3 finality commits on top of the maintainer's merge c49e6a8; base
b2a827081, upstream/main5e7b786):cargo fmt --all -- --check— cleancargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings— clean (0 warnings)./scripts/check-parser-combinators.sh <base>— Gate A PASS (finality adds zero production parser lines)./scripts/check-skill-doc.sh— ✓ ·./scripts/check-engine-authorities.sh <base>— Gate B PASS (82 zone hits / 62 rows; draw-producers 7 rows) ·./scripts/check-test-card-data-load.sh <base>— ✓card-data-validate -- data/card-data.json— OK (35396 parsed); oracle-gen 92.4% implemented — identical to the pre-finality run (typedCounterType::Finalityserializes byte-identically to"finality"→ parse-neutral, no card-data migration)python3 scripts/draw_replacement_census.py --corpus --check— PASS (51 rows)coverage-report -- data/ --all+coverage-regression-check.sh <R2> --fail-on-engine— REGRESSED(engine)=0, ORACLE CHANGED=0; swallowed-clause 1126→1069. The 3REGRESSED(honesty)cards (Avatar Destiny / Machinist's Arsenal[Swallow:DynamicQty], The Doctor's Childhood Barn[Effect:when]) are stale-base false positives — the branch is 6 commits behind upstream/main incl.#5689(parser "for each" scaling fix, supports the two DynamicQty cards) and#5688; the finality diff has zero production parser edits, so it cannot affect them. They clear on merge-forward.cargo nextest run --profile ci --workspace --exclude phase-tauri --exclude mtgish-import --features engine/proptest(PROPTEST_CASES=32) — 21051 passed / 21 skipped / 0 failed, incl. the 4 new finality death→exile tests (destroy / sacrifice / SBA-lethal / SBA-0-toughness)(The L02 parse-population figures above — 69/66, +34, 1025→986 — were measured vs the #5668 base
fa7f157fabefore the maintainer merged main forward; the finality work is parse-neutral so the population is unchanged, but the absolute coverage counts shifted with main's own swallow-audit evolutionfa7f157fa→b2a827081. Population re-measure vs the moved base is a reconciliation item.)Gate A
Gate A PASS head=5f7acb903af86b96045dd2792d9cf6c33edee6d5 base=5e7b786704d1d8413d33a6f6ed32f2873824e316
Anchored on
parse_inner_condition, the single authority for game-state condition recognition that every BB delegates to (no bespoke string matching)extract_if_condition_with_card_name→ zone-change filter dispatch) that BB4/BB-FU3 extend structurallyFinal review-impl
Final review-impl PASS head=5f7acb903af86b96045dd2792d9cf6c33edee6d5
Claimed parse impact
L02 target cards (SwallowedClause cleared): the 30 Standard condition-lane cards across BB1–BB8 (activate-only-if, cast-origin, as-long-as, finality/exile cast-permission, conditional enters-with-counter, cast-context cost gates, cast-this-way/goad). Follow-up-touched: Gargoyle Flock (BB-FU1), Stalwart Successor / All Will Be One / Generous Patron / Kros / Putrid Hexhag / Rikku / Danny Pink (BB-FU2, batched-counter flag), Phage the Untouchable (BB-FU4 fence), Otterball / Match-the-Odds class (BB-FU5). BB-FU3 is representation-stable (byte-identical card-data; the fix is structural disambiguation proven by a synthetic production-path test).