feat(parser): Lignify-class enchanted subtype-only type-change with base P/T (#5300)#1
Open
jsdevninja wants to merge 2430 commits into
Open
feat(parser): Lignify-class enchanted subtype-only type-change with base P/T (#5300)#1jsdevninja wants to merge 2430 commits into
jsdevninja wants to merge 2430 commits into
Conversation
…ure, an artifact, and a land (phase-rs#5026) * fix(engine): Tomb of Annihilation "Oubliette" also sacrifices a creature, an artifact, and a land Oubliette's Oracle text is "Discard a card and sacrifice a creature, an artifact, and a land." The room was implemented as "Discard a card" only, silently dropping the three mandatory sacrifices — venturing into Oubliette cost the player just a card and never any permanents. Chain the discard to three sacrifices (one creature, one artifact, one land) in printed order via sub_ability. Each is a count-of-one sacrifice with min_count 0 so it resolves to zero when the controller has no permanent of that type (CR 701.21a: a player can only sacrifice permanents they control). CR 701.9 (discard) + CR 701.21a (sacrifice). Test: - runtime (engine venture path): tomb_oubliette_discard_sacrifice_runtime ventures Trapped Entry -> Oubliette with the controller holding exactly one card and controlling one creature, one artifact, and one land (so discard and each sacrifice auto-resolve on their single legal choice), then asserts all four cards land in the graveyard and the permanents leave the battlefield. Fails on the old "discard only" room (revert-probed: only the discarded card reaches the graveyard). * fix(PR-5026): correct discard CR annotation --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…sher, not a creature sacrifice (phase-rs#5029) Veils of Fear's Oracle text is "Each player loses 2 life unless they discard a card." The room was implemented as "each player sacrifices a creature" — the wrong keyword action entirely, so venturing into it never cost life and demanded a creature instead. Build the room from its Oracle text via the shared parser (as several other rooms already do), which yields the correct punisher: base LoseLife{2}, player_scope All (each player), and unless_pay { cost: Discard 1, payer: ScopedPlayer }. CR 118.12a: "[Do something] unless [a player does something else]." Test: - runtime (engine venture path): tomb_veils_of_fear_life_loss_runtime ventures Trapped Entry -> Veils of Fear with both players holding no cards, declines each player's discard-or-lose-life prompt (PayUnlessCost { pay: false }), and asserts both players lose 2 life. Fails on the old sacrifice room, which never touched life (revert-probed: both players stay at 20).
…l forms (phase-rs#5030) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…g Frost) (phase-rs#4930) * fix(parser): generalize legendary supertype grant to all supertypes (Glittering Frost) "Enchanted [permanent-type] is [supertype]" (Glittering Frost: "Enchanted land is snow.") grants a supertype, not a card type (CR 205.4a, additive per CR 205.4b). This shape already had ONE owner: the attached-subject predicate seam (parse_enchanted_equipped_predicate -> parse_continuous_gets_has -> parse_continuous_modifications), which recognized only the narrow legendary case via parse_legendary_supertype_grant. Generalize that recognizer instead of adding a parallel whole-clause parser: - grammar.rs: parse_legendary_supertype_grant -> parse_supertype_grant, built on nom_target::parse_supertype_word so it returns the matched supertype (Legendary/Basic/Snow). Still scans at word boundaries so the grant is found mid-compound ("... is legendary, gets +1/+1, and has flying"); a peek(not(alphanumeric1)) guard keeps a supertype-prefixed longer word ("snow" in "snowman") from misfiring. - keyword_grant.rs: the call site pushes AddSupertype for the returned supertype, so Glittering Frost and In Bolas's Clutches share one seam. This reverts the separate parse_enchanted_is_supertype parser and its dispatch hookup (type_change.rs, dispatch.rs restored to origin/main) so the "enchanted subject is [supertype]" shape keeps a single owner. Tests use the production front door (parse_static_line_multi), exercising the real attached-Aura dispatch rather than a single-line bypass. Legendary grants (In Bolas's Clutches, On Serra's Wings compound) stay green; "Enchanted land is a Mountain." still routes to the basic-land-type aura and emits no AddSupertype. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(parser): cover World/Ongoing in parse_supertype_word The generalized supertype-grant path (parse_supertype_grant) delegates to the shared nom_target::parse_supertype_word building block, but that recognizer only accepted legendary/basic/snow. The engine Supertype enum and CR 205.4a both also define World and Ongoing, so the "general" supertype-grant seam silently dropped those two supertypes. Extend the shared recognizer with `world` -> Supertype::World and `ongoing` -> Supertype::Ongoing (same nom alt/tag/value arm style; no stringly dispatch). None of the five words is a prefix of another, so the alt ordering stays boundary-safe; callers keep applying their own boundary guard (grammar.rs uses peek(not(alphanumeric1)) after the word), so "world" cannot mis-match "worldly" etc. Tests: - oracle_nom/target.rs: parse_supertype_word recognizer maps world/ongoing to the right Supertype (and still legendary/basic/snow; rejects host). - oracle_static/tests.rs: end-to-end via parse_static_line_multi that "Enchanted permanent is world." -> AddSupertype{World} and "Enchanted permanent is ongoing." -> AddSupertype{Ongoing}, proving the general path now covers the full CR 205.4a set. No corpus parse changes (no real card grants World/Ongoing; this completes the shared building block). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…e-rs#4961) (phase-rs#5016) * fix(effects): no-op tap/untap choice with zero eligible targets (phase-rs#4961) Prevent unsatisfiable EffectZoneChoice wedges when a resolution-time tap selection has no eligible permanents but min_count >= 1. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(effects): make no-candidate tap/untap handling reachable (phase-rs#4961) Move the empty-pool no-op into the resolve_multi_target_bounds Err arm (the reachable no-candidate point) instead of an unreachable post-bounds guard, and add a companion regression proving a satisfiable pool still prompts. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(effects): scope no-candidate tap/untap no-op to empty legal pool (phase-rs#4961) The Err(_) arm treated any bounds-resolution failure with an empty pool as a resolved no-op, which also swallowed the earlier unresolved-quantity error (e.g. "tap X target creatures" before X is chosen). Re-check multi_target_needs_quantity_choice so only the "not enough legal targets" empty-pool case becomes a no-op; unresolved-quantity failures fall through to the normal target path. Adds a regression covering the unresolved-X branch. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: root <root@195-201-198-180.ptr> Co-authored-by: Cursor <cursoragent@cursor.com>
… a compound (phase-rs#5031) Singularity Rupture ("Destroy all creatures, then any number of target players each mill half their library, rounded down.") dropped its entire second clause: only `DestroyAll` parsed, so the mill never happened and no target prompt appeared. The `, then <clause>` boundary detector recognized imperative-verb, "you control" subject, and damage continuations, but not a player-*subject*-led clause. "any number of target players each mill …" begins with a subject, not a verb, so the tail was never peeled off — even though `parse_subject_application` lowers it to a per-player targeted Mill on its own. Add a targeted `starts_target_players_subject_clause` recognizer for the distributive "any number of target player(s) [each] …" form and gate the `Then` boundary on it. Closes phase-rs#4783 Co-authored-by: Nick M <274344962+nickmopen@users.noreply.github.com>
…sher, not a fabricated Zombie token (phase-rs#5034) Sandfall Cell's Oracle text is "Each player loses 2 life unless they sacrifice a creature, artifact, or land of their choice." The room was implemented as "you lose 2 life and create a 2/2 black Zombie creature token" — a token that appears nowhere on the card, and it only affected the controller instead of each player. Build the room from its Oracle text via the shared parser (as several other rooms already do), which yields the correct punisher: base LoseLife{2}, player_scope All (each player), and unless_pay { cost: Sacrifice one permanent matching creature/artifact/land you control, payer: ScopedPlayer }. CR 118.12a: "[Do something] unless [a player does something else]." Test: - runtime (engine venture path): tomb_sandfall_cell_life_loss_runtime ventures Veils of Fear -> Sandfall Cell with both players controlling no permanents, declines each player's sacrifice-or-lose-life prompt (PayUnlessCost { pay: false }), and asserts both players lose 2 life. Fails on the old room, which only made the controller lose life and created a bogus token (revert-probed: the non-controller player stays at 20).
GameState.rng_seed is a serialized field and the engine's randomness is a deterministic ChaCha20 stream seeded from it. Every StateUpdate and GameStarted message carries the filtered GameState to clients, but filter_state_for_viewer never redacted rng_seed — so the seed was broadcast to every player and spectator. With the seed, a client can reconstruct the RNG stream and predict every future shuffle, draw, coin flip, and random selection, including its own and the opponent's hidden library order. That defeats the library/hand redaction the same filter performs and breaks ranked integrity. Redact rng_seed (and reset the serde-skipped RNG handle) in filter_state_for_viewer. The authoritative engine and on-disk persistence operate on the unfiltered state, so server-side randomness and session restore are unaffected; only the client-facing copy loses the seed. Add a test asserting the seed is zeroed for both seats and the non-seat spectator while the source state is untouched. Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…hase-rs#5036) * test(engine): lock in rules-correct behavior for AI-reported legality cases Discord ai-suggestions reported three suspected engine rule violations that end-to-end tracing (plan + independent review) proved are already correct on main. Add discriminating regression guards, green on unmodified source, so the behavior can't silently regress: - J1: 'sacrifice another creature' excludes the ability source (Disciple of Bolas) — cast-pipeline test + helper companion + Another-vs-none sibling. Effect path (from_ability) and cost path (from_source) both set recipient_id: None, so filter.rs:3669 excludes the source identically. CR 701.21a. - J2: negative loyalty ability not offered below cost at the can_activate_ability_now legal-action seam (Ob Nixilis of the Black Oath at 1 loyalty). CR 606.6. - J3: delayed 'sacrifice it' snapshots the creation-time token, not a later one (Saheeli, Radiant Creator). CR 603.7c. Test-only; no production source changed. The two genuinely-behavioral reports route to phase-ai: J3 = legend-rule token-copy target selection (phase-rs#377), J4 (Disorder commander-zone) = tactical decline of a CR 903.9a 'may' choice. * feat(ai): veto net-negative self-cost ability activations Discord ai-suggestions repeatedly reported the AI activating abilities whose self-cost (sacrifice / pay-life / discard / exile-own-graveyard) is a net loss with no payoff: High Market and Zuran Orb sac'ing for trivial life, Enduring Tenacity sac'ing its best creature, Goblin Bombardment sac'ing its board to ping a face, Carrion Feeder sac'ing itself, discard-to-gain loops, etc. Existing gates only covered the aristocrats sac path (FreeOutletActivation) and lethal life costs (AntiSelfHarm). Add one general building block instead of N per-card fixes: - self_cost.rs: real_self_cost (prices the four self-cost axes), benefit_is_trivial (walks the ability effect chain; conservative -- dynamic/Fling damage, mana, land-search, and beneficial counters are non-trivial so ramp/burn/fixing are never suppressed), synergy_justifies_self_cost (per-axis stand-down on aristocrats death-triggers / lifegain / reanimator / cEDH). - self_cost_value.rs: SelfCostValuePolicy -- reject when benefit trivial and cost real; deprioritize when cost is below the reject floor; stand down on synergy. Reject collapses the candidate to NEG_INFINITY so Pass wins. Keyed on the AbilityCost tree, not card names -- covers the whole class. Verdicts use registry band helpers (no raw sentinels). Reconciled with SacrificeLandProtection and Spellskite policies (disjoint payoff types; both already handle their cards). cargo ai-gate: 0 FAIL, 0 W->L flips across matchups; only signal is +avg-turns from holding marginal artifact sacs. Baseline NOT refreshed. A larger-N ai-gate confirmation is recommended before shipping to main. * feat(ai): fix effect-valence target selection for grant/copy/mill Discord ai-suggestions reported the AI mis-targeting by effect valence: Undying Malice cast on an opponent's creature, and copying its own legendary (Saheeli/phase-rs#377) so the token legend-rules the original away. Root cause: wrong inputs to the already-correct valence machinery, not per-card logic. - effect_classify: classify ContinuousModification::GrantTrigger by the granted trigger's executed-effect polarity (delegates to effect_polarity), so an undying/return grant reads Beneficial (AI aims it at own creatures) and a downside grant reads Harmful. CR 603.1. - copy_value: penalize copying your own legendary even without a pre-existing duplicate (the copy itself creates the legend-rule collision), gated on a new copy_effect_strips_legendary guard (RemoveSupertype{Legendary} -> non-legendary token, no collision) and legend_rule_exempt (Mirror Gallery/Sakashima). Stays a +35 penalty so a non-legendary alternative simply outscores it. CR 704.5j/205. - mill_targeting: require graveyard-sourced retrieval (ChooseFromZone{Graveyard} / ChangeZone{origin:Graveyard}) for mill-payoff detection, so Thought Scour (a library cantrip) is no longer misread as a mill-the-opponent engine. CR 701.17a. Two reports dropped after re-diagnosis as non-bugs: Beast Within/Nature's Claim on own permanent (own permanent already scores -2 vs opponent +2; forced-target) and Curse of the Forsaken (its 'gains 1 life' beneficiary resolves to the attacker's controller, so cursing an opponent is +EV). Strength of the Tajuru verified already-correct. Self-mill hard-reject exemption deferred. cargo ai-gate: 0 FAIL, 0 flips in copy/aura mirrors (red-mirror WARN is net +1, p=0.31, and casts none of the changed effect types). Baseline NOT refreshed. * feat(ai): veto committing to X-cost casts whose only legal X is 0 Discord ai-suggestions reported the AI casting X-cost spells and abilities for X=0 (Exsanguinate turn 1, Day of Black Sun, Their Number Is Legion, Mirror Entity wiping its own board to 0/0, Helix Pinnacle). Diagnosis: the existing x_value ramp fires at the downstream ChooseXValue decision, AFTER the AI already committed to cast at Priority -- so nothing rejected committing to an {X} cast whose only affordable X is 0. - x_reference.rs: extract the nine X-reference detectors out of x_value.rs into one shared module (ramp + gate consume it). Add detection for X inside GenericEffect statics, QuantityRef::CostXPaid (Mirror Entity), and filter-CMC-X (Day of Black Sun) -- the ramp previously missed these too. - x_cast_gate.rs: XCastGatePolicy on [CastSpell, ActivateAbility]. Rejects when the cost has an {X} shard, max_x_value == 0, and the payoff is a no-op at X=0. no_op_at_x_zero is chain-aware: an effect referencing PreviousEffectAmount whose immediate predecessor is X-scaled is itself 0 at X=0 (Exsanguinate's GainLife residual), so it gates robustly even when the AI is life-critical. A fixed (non-X) beneficial payoff keeps references_any_x false -> not gated. Rejecting yields Pass; the card is held for a later turn, never lost. Keyed on the AbilityCost tree, not card names. Reuses the engine's own max_x_value affordability authority (read-only re-exported from casting_costs) and self_cost::effect_is_trivial for the non-X residual. Band-helper verdicts. cargo ai-gate: 0 FAIL, 0 W->L flips in copy/aggro/enchantress mirrors (red-mirror WARN is net +1, p=0.31, and casts no X spells). Baseline NOT refreshed. Scoped to the 5 reported cards (3 spells + 2 activated). ETB-X *creatures* (Hangarback-class) are not gated -- they resolve as permanents with no top-level Spell ability, so the CastSpell path bails before the object-level X check; left as a documented follow-up (also a CostXPaid-in-PutCounter detector gap). * perf(ai): gate x_cast_gate affordability sweep on AI search depth XCastGatePolicy::verdict() called engine::game::max_x_value — a board-wide feasible_mana_capacity affordability sweep — and PolicyRegistry::verdicts() runs every policy's verdict() per candidate at every AI search node, so on a large late-game board one decision regressed ~2.1s -> ~5.7s. cargo ai-gate (win-rate, latency-blind) and cargo ai-perf-gate (perf counters; max_x_value bumps none) are both structurally blind to it. Add a typed SearchDepth { Root, Lookahead } to PolicyContext with an at_root() accessor, mirroring the existing deadline_expired()/can_afford_projection() expensive-work self-gating precedent. gate_rejects() returns early in lookahead nodes (the resulting-state eval already reflects an X=0 no-op cast), and runs the card-local no_op_at_x_zero payoff walk before the board-wide max_x_value sweep — a commutative reorder that also spares the sweep for non-no-op payoffs at the root. Root-only gating stays rules-correct (CR 601.2b/f): the committed action is the argmax over root candidates, and Reject -> NEG_INFINITY cannot be rescued by any finite lookahead continuation, so every real X=0 no-op commitment is still suppressed. Verified by the discriminating helix_pinnacle_lookahead_returns_neutral (same fixture flips verdict on search_depth alone) and the end-to-end xcast_zero_no_op_not_committed_at_root. Group priors()'s batch-constant scoring inputs into PriorsEnv to keep the signature under clippy's argument limit after threading search_depth. * docs(ai): codify verdict() search-inner-loop perf constraints in add-ai-feature-policy PolicyRegistry::verdicts() runs every policy's verdict() per candidate at every AI search node, so a board-wide/affordability engine call there is a wall-clock landmine that both cargo ai-gate (win-rate only) and cargo ai-perf-gate (counter-based; uninstrumented calls bump nothing) are structurally blind to. Add a Performance section to the skill: order predicates cheap->expensive (card-local AST before board-wide), use activation() as the free whole-policy opt-out, gate expensive work behind at_root(), and require either counter instrumentation or a wall-clock A/B on a large late-game board (a 0-flip ai-gate is necessary, not sufficient). Uses the x_cast_gate max_x_value regression (commit 3de827350d) as the case study. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…phase-rs#5013) * fix(parser): parse Volo creature-type spell qualifier (phase-rs#4962) Recognize "that doesn't share a creature type with …" post-spell modifiers and disjunctive shared-quality references so Volo's SpellCast trigger matches and CopySpell creates a token copy. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(parser): address Volo PR review and CI (phase-rs#4962) Remove cross-PR optional-tap assertion, fix mod order and fmt, make parse_shared_quality_reference composable, strengthen Or-filter test, and add negative integration coverage for shared creature types. Co-authored-by: Cursor <cursoragent@cursor.com> * test(integration): drop Volo negative case pending runtime Or filter (phase-rs#4962) The disjunctive SharesQuality reference parses correctly in unit tests; runtime cast-trigger filtering for the Or reference needs a follow-up. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(filter): enforce Volo shared-type condition at runtime (phase-rs#4962) A cast creature spell sits on the stack and was self-satisfying its own "creature you control" shared-type reference, so Volo copied every creature spell. Exclude the stack spell from its own shared-quality reference scan and add discriminating runtime coverage for both disjunctive blockers (a creature you control and a creature card in your graveyard) plus a positive case where a controlled non-sharing creature does not block the copy. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(filter): honor CR 109.2 zone semantics for shared-quality references (phase-rs#4962) A bare "creature you control" / "creature card in your graveyard" shared-quality reference is a zoned object reference (CR 109.2): a permanent on the battlefield or a card in the named zone, never a spell on the stack. The previous fix only excluded the object under test, so a sibling creature spell already on the stack still satisfied Volo's / Menagerie Curator's "creature you control" reference and wrongly blocked the copy. Exclude stack objects from the reference scan instead. Add a filter regression proving a same-type creature spell on the stack does not satisfy the reference (must not block), while the same object moved to the battlefield does — the zone axis decides, not object identity. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: root <root@195-201-198-180.ptr> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…] [supertype]" (Leyline of Singularity, Melting) (phase-rs#5020) * feat(parser): supertype-defining statics '[subject] is/are [no longer] [supertype]' (Leyline of Singularity, Melting) Global supertype-setting statics were unimplemented — 'All nonland permanents are legendary' (Leyline of Singularity), 'All creatures are legendary' (Sixth Stage of Magic Design), 'Permanents with ice counters on them are snow' (Rimefeather Owl), and 'All lands are no longer snow' (Melting) all dropped to an Unimplemented gap, even though the color sibling ('All creatures are white') and the AddSupertype/RemoveSupertype runtime already existed. Add parse_subject_is_supertype in the test-free type_change.rs — the supertype mirror of parse_subject_is_color: reuse the shared subject grammar (parse_continuous_subject_filter, covering 'All'/'Each', 'nonland permanents', controller suffixes, counter-filtered subjects), the shared parse_supertype_word token (legendary/basic/snow), and the existing ContinuousModification::AddSupertype/RemoveSupertype runtime applied at Layer 4 in game/layers.rs. NO new variant, NO new runtime. An optional 'no longer'/'not' negation flips to RemoveSupertype (CR 205.4b / CR 707.9b). all_consuming on the supertype word keeps this to the bare form (a trailing tail or unrecognized predicate leaves the line unsupported). Dispatched right after the color path and before parse_land_type_change, so 'All lands are basic' (supertype) is not probed as a land-type line; the supertype predicate is disjoint from color and land-type words, so no branch is stolen (color/land-type siblings unchanged). 960 oracle_static tests pass + new static_subject_are_supertype_adds_and_removes regression (add, remove, basic-vs-land-type, color-unchanged, subtype-not-claimed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(parser): clippy doc-lazy-continuation + correct supertype-removal CR CI lint gate (`clippy --all-targets -D warnings`) flagged `clippy::doc_lazy_continuation` on the supertype-static test doc: the two `+`-prefixed lines read as a markdown list, making the following line an unindented lazy continuation. Reword to a comma list — no `+` bullets. Also correct the CR annotation on the "no longer"/"not" negation branch of `parse_subject_is_supertype`: it cited CR 707.9b (copy effects), which is unrelated. Supertype gain/loss is CR 205.4b ("When an object gains or loses a supertype…"). Per reviewer (gemini) request. Comment-only; no logic change. fmt / parser gate / clippy all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * review: cede attached-aura supertype form to phase-rs#4930; keep the global-static path phase-rs#4930 ("Enchanted [type] is [supertype]" aura, Glittering Frost) merged upstream and already (a) generalized the shared parse_supertype_word to the full CR 205.4a set (legendary/basic/snow/world/ongoing) and (b) owns the attached-subject Aura/ Equipment supertype-grant seam (parse_supertype_grant). So this PR's earlier parse_supertype_word extension is now redundant — dropped in the rebase, ceding that building block to phase-rs#4930. Remaining scope of this PR is the disjoint GLOBAL "[subject] is/are [no longer] [supertype]" static form (Leyline of Singularity, Sixth Stage of Magic Design, Melting, Rimefeather Owl, Rootpath Purifier), handled by parse_subject_is_supertype (type_change.rs) + its dispatch wiring — cards phase-rs#4930 does not cover. To make the overlap intentional and bounded (matthewevans' ask), parse_subject_is_ supertype now declines an attached-subject filter (FilterProp::EnchantedBy / EquippedBy): the "Enchanted/Equipped [type] is [supertype]" shape is owned solely by phase-rs#4930's predicate seam, so Glittering Frost et al. are no longer double-handled. World/Ongoing coverage still validated end-to-end here via phase-rs#4930's now-shared parse_supertype_word. Full engine suite green; fmt + clippy (--all-targets --features proptest) clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…t" (phase-rs#5043) Monomania ("Target player chooses a card in their hand and discards the rest.") previously failed to parse: the "chooses ... and discards the rest" phrasing was unrecognized, so the clause lowered to Effect::Unimplemented { name: "choose" }. Add a whole-clause nom recognizer in the "choose" verb dispatch that routes this phrasing to the EXISTING Effect::Discard (via TargetedImperativeAst::Discard) with a canonical ClampMin { Offset { HandSize, -1 }, 0 } "hand size minus one, floored at zero" count -- the discarding player keeps exactly one card of their choice. No new effect or game logic is introduced; the possessive selects the HandSize scope and inject_subject_target rewrites the discarding player to the spell target, exactly like the sibling "discard their hand" path. Anchored with eof so a bare "choose a card ..." prefix cannot hijack other choose clauses (regression test covers Balance-family and cardinality near-misses). Co-authored-by: root <root@vmi3390064.contaboserver.net> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ain and as-long-as class fixes (phase-rs#4990) * feat(engine): S07 batch 1 — "this way" tracked-set Condition_If (9 cards) Wire leading- and trailing-if conditions gated on "this way" tracked sets / zone-change refs onto existing typed variants, plus a tight swallow_check recognizer for conditional-counters-on-put-onto-battlefield. Cards (9): Oviya (Automech Artisan), Spelunking, Town Greeter, Cache Grab, Nashi (Searcher in the Dark), Arid Archway, Break the Spell, Portent of Calamity, Transcendent Archaic. - New nom combinators for you-put-into-hand / returned-to-hand / control-or-returned / you-draw / exiled-N-or-more "this way" conditions; hoist active-voice this-way gates to fire under both `if` and `when` prefixes. - Portent: trailing-if free-cast gate via QuantityRef::TrackedSetSize GE 4 (exile-resume publishes cause-less, so the unfiltered count is correct). - Oviya: swallow_check recognizes conditional_enter_with_counters on a Hand->Battlefield ChangeZone as covering the Condition_If clause (runtime gate on moved-object type verified); trample static confirmed wired. - Zero new engine variants. All 9 flip supported:true gap_count:0. - Discriminating runtime tests (positive + negative sibling) per card; Portent GE-4 both arms; Oviya artifact->2 / non-artifact->0. - coverage-regression-check vs main: REGRESSED(engine)=0; swallowed 947->937. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): S07 batch 2 — cost-object / additional-cost / gift Condition_If (5 cards) Wire conditions gated on additional-cost payment, cost-object properties, and gift-given state onto existing typed variants; add is_suspected to the last-known-information channel so "the sacrificed creature was suspected" resolves. Cards (5): Agency Coroner, Grab the Prize, Cinder Strike, Coiling Rebirth, Longstalk Brawl. (Celestial Reunion is a separate in-scope increment — needs a new interactive behold-choose-a-creature-type cost subsystem — and stays honestly unsupported until that lands.) - B3 Agency Coroner: thread is_suspected through LKISnapshot + the synthesized ZoneChangeRecord (both #[serde(default)]), captured live from the object before the sacrifice reset; read at zone_change_record_matches_property so FilterProp::Suspected evaluates on the sacrifice-cost path. CR 701.60. - CostPaidPredicate::Color -> Property(FilterProp) (both applied via typed.properties); add the suspected branch -> FilterProp::Suspected. - Grab the Prize / Cinder Strike: additional-cost object predicate ("discarded card wasn't a land") / optional blight cost -> higher effect. - Coiling Rebirth / Longstalk Brawl: gift-given (AdditionalCostPaid) gate. - Zero new engine variants. All 5 flip supported:true gap_count:0. - Discriminating tests: Coroner suspected->2/unsuspected->1; Grab nonland->2dmg /land->0; Cinder blight->4/decline->2; gift cards gift->bonus/no-gift->base. - coverage-regression vs main: REGRESSED(engine)=0; all 5 in GAINED. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): S07 batch 3 — target/identity Condition_If (5 cards) Wire conditions gated on target identity, combat presence, and populated anaphora onto existing typed variants; add a full-cast hardening test for Batch-2's Coiling Rebirth. Cards (5): Steer Clear, Fear of Immobility, Charging Hooligan, Yenna Redtooth Regent, Eliminate the Impossible. (A Killer Among Us and Malamet Battle Glyph are deferred to their own increments — A Killer needs a new interactive secret-choice subsystem; Malamet is blocked on a pre-existing multi-target propagation bug and stays honestly unsupported.) - Steer Clear: strip_instead_clause gains a multi-sentence guard so a prior sentence + "instead if" line defers to the chain parser (root-cause fix); ConditionInstead{ControllerControlledMatchingAsCast}. Structural-only, REGRESSED(engine)=0. - Fear of Immobility: stun sub gated And[HasObjectTarget, TargetMatchesFilter {Opponent}] on the tapped target's live controller (CR 109.4). - Charging Hooligan: parse_a_type_is_in_combat -> IsPresent{Rat+Attacking} gates the trample grant (CR 508.1/509.1). - Yenna: QuantityCheck{ObjectCount{LastCreated & Aura} >= 1} gates untap+scry (field-expressive: the created token is not targets[0]). - Eliminate the Impossible: resolve_populated_unsuspect_anaphors lowers PumpAll -> Unsuspect{population, All} + existential Suspected gate (CR 701.60a); reclaims the previously-swallowed clause. - Coiling Rebirth hardening: full cast->resolve pinning ObjectId survival across graveyard->battlefield so Not(Legendary) reads live supertypes; gift+nonlegendary->token / legendary->none / gift-declined->none. - Zero new engine variants. All 5 flip supported:true gap_count:0. - coverage-regression vs main: REGRESSED(engine)=0; swallowed 947->920. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): S07 Celestial Reunion — behold-chosen-type additional cost + conditional search destination Celestial Reunion: "As an additional cost to cast this spell, you may choose a creature type and behold two creatures of that type. Search your library for a creature card. If you beheld a creature this way and that card is the chosen type, put it onto the battlefield; otherwise put it into your hand." Full-gate S07 increment (Condition_If shape gated on an optional additional cost whose result-object destination depends on a resolution choice): - AbilityCost::Behold gains type_choice: Option<ChoiceType>; new WaitingFor::CostTypeChoice round-trip lets the caster choose the creature type during cost payment (CR 601.2b), writing ChosenAttribute::CreatureType onto the spell object; single-authority feasibility helper drives both B1 payability and B2 option enumeration. - TargetFilter::without_prop + FilterProp::IsChosenCreatureType express "that card is the chosen type". - Conditional destination lowered as SearchLibrary → ChangeZone{Battlefield, And[AdditionalCostPaid, TMF{IsChosenCreatureType}]} / else ChangeZone{Hand}; a post-lowering fold (fold_search_choose_type_conditional_destination) repairs the mangled chain, corpus-grep-scoped to this card only. - Deferral disjunct in effects/mod.rs gated tightly on matches!(WaitingFor::SearchChoice) (result-injecting only) so no supported EffectZoneChoice/grave-choice card regresses (byte-identical for every non-SearchChoice wait). - phase-ai: CostTypeChoice routed to DecisionKind::ActivateAbility (cost-phase bucket); N-1 AI candidate/cancel-cast seam confirmed live. - Frontend: CostTypeChoice reuses NamedChoiceModal (no new i18n strings). Card flips supported:true gap_count:0. 6 discriminating WaitingFor→GameAction round-trip tests (per-leg AdditionalCostPaid / IsChosenCreatureType discriminators, non-vacuous by revert-trace). REGRESSED(engine)=0. CR 205.3m, 601.2b, 601.2h, 701.4a, 702.73a, 608.2c (grep-verified). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): target-model infra — ParentTargetSlot indexed resolution + 3 delayed/filter sites Increment A of the S07 target-model work: behavior-neutral infrastructure that teaches four resolution sites to honor `TargetFilter::ParentTargetSlot { index }` (an anaphoric reference to a specific earlier chain target slot), where they previously either fell through to all-objects or dropped the index. Flips no card; unblocks indexed-slot consumers (S25 B3 delayed-trigger snapshot machinery + the S07 Malamet/Longstalk counter-target fix landing separately). - `game/targeting.rs`: extract the inline root-lookup/flatten logic into a single authority — `parent_chain_targets_from_root` (byte-identical to the old inline block) + `resolve_parent_slot_from_root(state, ability, index)` (adds `.nth(index)`). Refactor the existing caller to use it. - `game/effects/counters.rs`: `resolve_defined_or_targets` gains a `ParentTargetSlot { index }` arm resolving via the helper (CR 608.2c + CR 122.1). - `game/effects/delayed_trigger.rs` `concrete_parent_target_filter`, `game/effects/mod.rs` `filter_refs_parent_target`, `game/filter.rs` `normalize_contextual_filter`: each gains a `ParentTargetSlot { index }` arm (incl. the `Not(ParentTargetSlot)` case), strictly additive — existing `ParentTarget` (non-indexed) callers byte-untouched (CR 603.7c + CR 608.2c). 4 discriminating tests, each empirically revert-probed (all fail when the arm is neutered): indexed resolution picks the right slot / binds the right object / detects the ref / excludes only that slot. REGRESSED(engine)=0; engine suite 14562 passed. CR 608.2c, 122.1, 603.7c (grep-verified). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): S07 Malamet Battle Glyph — cast-path (entered-this-turn condition + reciprocal-fight class fix) Flips Malamet Battle Glyph to supported:true gap_count:0 and fixes the latent cast-time panic in the committed Longstalk Brawl / Duel for Dominance "those creatures fight each other" class (their tests only drove evaluate_condition, never a full cast). Malamet: "Choose target creature you control and target creature you don't control. If the creature you control entered this turn, put a +1/+1 counter on it. Then those creatures fight each other." - Condition detector (parser/oracle_effect/conditions.rs): new parse_target_entered_this_turn_condition — "if [anaphor | filter] entered this turn" → TargetMatchesFilter{creature + controller + FilterProp::EnteredThisTurn} (per-object, CR 400.7). Building-block: also flips Samut, Vizier of Naktamun ("if that creature entered this turn, draw a card"). Rejects control-count / existential / source forms (over-fire negatives). - Chain rewrite (parser/oracle_effect/lower.rs): rewrite_two_target_counter_chain — for a ≥2 Typed-TargetOnly chain, re-key PutCounter{ParentTarget} → ParentTargetSlot{0} and set subject_slot:Some(0) on the node's condition so the counter + its condition bind the FIRST (you-control) target, not the most-recent slot (CR 608.2c). - subject_slot: Option<usize> on AbilityCondition::TargetMatchesFilter (serde default + skip_serializing_if); None = legacy node-local first object. - Reciprocal-fight cast fix: shared parse_fight_target ("each other" → ParentTarget) wired into both fight dispatchers; resolve_fight_fighters recovers both fighters from the flattened chain root when local object targets < 2 and both root slots resolve to distinct objects (CR 701.14a). Root cause of the spurious all-players target slot: the reciprocal Fight target lowers to a context-ref (TrackedSet), which the fight slot-gen/assign arms did not skip — broadened both from SelfRef|ParentTarget to filter.is_context_ref() (fight-scoped, load-bearing: reverting it re-panics all three casts). 7 full cast().resolve() tests (Malamet both condition branches + which-creature + both-fight; Longstalk castable; Duel under coven) + synthetic fight-guard + 4 detector tests. Tail Swipe correctly stays RED (unrelated Unimplemented node). REGRESSED(engine)=0; only Malamet + Samut flip; semantic-audit 305→305. CR 109.4, 400.7, 603.2, 608.2c, 701.14a (grep-verified). Assisted-by: ClaudeCode:claude-opus-4.8 * fix(parser): create "A, a B, and a C token" 3+-item chains no longer drop the middle token split_create_token_sequence recognized only " and " as a token separator, never the intra-list comma, so "create A, a B, and a C" split into left="A, a B," / right="C" and parse_token_description kept only the first item — silently dropping every middle token, with no diagnostic (a false supported:true). Rewrite the binary splitter into an N-way split over the conjunctive-only separator set {", and ", ", ", " and "}, each guarded by peek(parse_token_noun_start) and a quote-swallowing item unit (both lifted from the sibling split_choice_list_items), keeping the existing "and "-gate so disjunctive "A, B, or C" lists stay routed to the modal choice parser. All N tokens chain via sub_ability in written order. When the gate fires with >=2 items but any item is not an Effect::Token, emit an honest Effect::unimplemented instead of the silent single-token fallback. Fixes the middle-token drop for 9 conjunctive cards (Bestial Menace, Fae Offering, Somberwald Beastmaster, Liberated Livestock, Mascot Exhibition, Triplicate Titan, Trostani's Summoner, The Companion of the Wilds, A Killer Among Us) and corrects The Companion's can't-block misattribution (now on the Rat, not the Food token). Overencumbered's subject-scoped "enchanted opponent creates ..." path (parse_search_creation_imperative) is a separate parser with the same symptom — tracked debt, not fixed here. CR 608.2c. REGRESSED(engine)=0. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): surface context-ref UnattachAll attachment in effect_parent_ref_slots effect_parent_ref_slots had Attach/Token/CopyTokenOf context-ref arms but no UnattachAll arm, so the `_ => {}` fallback silently dropped a context-ref `attachment`: a delayed "when you lose control of this, unattach it" trigger snapshotted nothing and resolved inert. Add the mirror arm guarded by `attachment.is_context_ref()` (class fix — Stolen Uniform, Ogre Geargrabber). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): S07 A Killer Among Us — secret creature-type choice + target-hoist gate Complete "A Killer Among Us" end-to-end (S07 condition-if tranche): - N1: parameterize ChoiceType::CreatureType { options } for a restricted candidate set (byte-stable unit-when-empty serde); compute_options honors it. - N2: parse "secretly choose Human, Merfolk, or Goblin" as a restricted creature-type enumeration (creature-only subtype vocab, no hardcoded names). - N3: recognize "reveal the creature type you chose" as a no-op cost so the ability resolves to a single Sacrifice(SelfRef), not a phantom double-Sacrifice. - N4: hoist the condition's "target attacking creature token" into the body's first anaphor so the ability declares slot 0, and gate the buff on TargetMatchesFilter{ IsChosenCreatureType, subject_slot: Some(0) } — the counters and deathtouch land on the attacking token, and the chosen type is read post-sacrifice from the graveyard object via LKI (CR 608.2h). 7 cast-level tests including the matching/non-matching gate pair and a multi-authority source-scoped-read fixture. A Killer flips supported:true gap_count:0. Depends on the token-splitter fix (f2648a0cb) for the 3-token ETB. CR 205.3e, 205.3m, 607.2d, 608.2c, 608.2h, 702.73a. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): S07 damage-doubler Condition_If false-positives — Trance Kuja + Rollercrusher Two Standard damage-doubling replacements were flagged supported:false by a spurious SwallowedClause(Condition_If): the ability-word "—" prefix injects a false leading " if " marker into swallow_check's detector. - Trance Kuja (Flare Star): unconditional Wizard-source damage doubler. Add a Condition_If exemption (unconditional_valmod_leading_if_is_only_if_marker) that strips the ability word, requires the body to start "if " with no while/ as-long-as/only-if residual and exactly one "if", and a value-modifier replacement with no condition. CR 614.1a + CR 120.8. - The Rollercrusher Ride: Delirium-gated noncombat doubler. Generalize parse_while_antecedent to scan forward to the " while " gate (the recipient clause sits between the "would deal " anchor and the gate), and wire the captured gate into ReplacementCondition::OnlyIfQuantity{DistinctCardTypes,GE,4}. Life-gain caller unchanged (flush empty-prefix case); fail-closed on unparseable guard. CR 614.1a. Both flip supported:true gap_count:0; coverage REGRESSED(engine)=0 (+2 exactly these cards); semantic-audit 0 findings. 7 discriminating cast/parse tests (revert->fail proven). Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): delayed-trigger ParentTargetSlot snapshots full root chain parent_target_snapshot seeded from the tail clause's per-clause ability.targets, so a multi-clause parent chain — whose tail carries only its own slot — left an inner ParentTargetSlot{index} anaphor indexing out of range, degrading valid_card to Any and resolving the delayed effect inert. Reseed from the flattened root chain (parent_chain_targets_from_root, the same root-chain flatten resolve_parent_slot_from_root uses); a non-empty root chain wins, else the TriggeringSource event-context fallback is byte-unchanged. Single-target delayed triggers (Flickerwisp, Grave Betrayal) have root chain == per-clause targets, so they are unaffected by construction. CR 603.7c + CR 608.2c. Class fix for every multi-clause delayed trigger snapshotting a parent-slot ref. New revert-proven test (multi-clause tail snapshots full root chain — panics [Object(10)] vs [Object(10),Object(11)] on revert); existing single-clause and TriggeringSource sibling tests preserved. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): Cloud — self+equipment DoubleTriggers gated on equipped; re-attach split as-long-as condition Cloud, Midgar Mercenary parsed DoubleTriggers{cause:Any} but dropped both the affected scope and the "as long as equipped" gate (two swallows: Condition_If + Condition_AsLongAs). Three coordinated fixes: - Parser scope (oracle_static/evasion.rs): SelfRef is restrictive; new parse_doubler_disjunct maps "~" -> SelfRef and "an Equipment attached to it" -> Typed(Equipment, AttachedToSource) ("it" = the doubler source, not the enchanted recipient). affected = Or[SelfRef, Typed(Equipment, AttachedToSource)]. - Parser gate (oracle_static/dispatch.rs): the inverted-"as long as" split re-dispatches the remainder but only re-wrapped it with .description(), dropping the split condition. Re-attach parse_static_condition(split.condition_text) when the recursed def has no condition. Fires only on condition.is_none() and only attaches a recognized condition — strictly corrective; fixes the whole class, not just DoubleTriggers. "~ is equipped" -> SourceIsEquipped. CR 611.3a. - Runtime self-inclusion (game/triggers.rs): apply_trigger_doubling unconditionally self-excluded the doubler. Gate it: when the affected filter references self (filter_references_self), the doubler's OWN triggers double too. Panharmonicon/Isshin (affected:None) and Wayta (non-self) stay self-excluded. CR 603.2d. Cloud flips supported:true gap_count:0 (both swallows cleared). Measured REGRESSED(engine)=0 across 35367 faces; the class fix additionally rescues 4 sibling inverted-as-long-as statics (Ethrimik, Glimpse the Cosmos, Pact Weapon, The Bird Champion); -7 swallowed clauses accounted per-handler, 0 new swallows. 5 revert-proven tests (4 behavioral + parse-shape); DoubleTriggers-family regression (Panharmonicon/Isshin/Wayta/Harmonic Prodigy/Splinter/Delney/Drivnod/ Hama Pashar) all unchanged. CR 301.5a. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): S07 Batch-5A — Sonic Shrieker + Slumbering Trudge Condition_If Two Standard cards flagged supported:false by a spurious SwallowedClause(Condition_If): - Sonic Shrieker (detector-only): "If a player is dealt damage this way, they discard a card." Already parses + resolves correctly (ETB DealDamage -> GainLife -> Discard{ParentTarget}: the damaged player chooses a discard; a creature target is a no-op). Only the swallow warning was spurious. Add a text-gated exemption (any_ability_has_parent_target_discard + "dealt damage this way") mirroring the existing "lost life this way" branch — the ParentTarget discard is the CR 608.2c back-reference, structurally represented, not a swallowed condition. The prevented- damage edge (CR 615.5) matches Screaming Nemesis's shipped fidelity ceiling. - Slumbering Trudge: "If X is 2 or less, it enters tapped." The SetTapState ETB replacement dropped its condition and tapped unconditionally. New combinator parse_enters_tapped_if_x_comparison captures the cast-X gate into ReplacementCondition::OnlyIfQuantity{CostXPaid, LE, 2}, dispatched before the unconditional enters-tapped guard. Reuses existing typed surface (no new variant). CR 107.3 / 614.1c / 614.1d. Both flip supported:true gap_count:0. Measured REGRESSED(engine)=0; GAINED = exactly these two cards (no class collateral); semantic-audit 0 findings. 7 discriminating tests (both revert-proven; the no-swallow test uses a Flying keyword to avoid the Unimplemented detector-suppressor). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): Avatar Aang — "done all four this turn" transform gate (QuantityRef::BendTypesThisTurn) Avatar Aang parsed ElementalBend -> Draw -> Transform with no condition, so it transformed on every bend (two swallows: Condition_If + Duration_ThisTurn). The per-turn bend tracking already exists (Player::bending_types_this_turn); the only gap was a QuantityRef leaf reading its distinct-type count. - New bare QuantityRef::BendTypesThisTurn (Controller-scoped), reading bending_types_this_turn.len() — the distinct-bend-type cardinality axis, mirroring the bare CrimesCommittedThisTurn/DescendedThisTurn siblings. Wired into every exhaustive QuantityRef match: the 3 quantity.rs classification groups, the triggers.rs cost-paid-object walker, and the ability_scan.rs growing-cascade scanner (=> Axes::NONE), plus coverage describe/kind. - Parser: "done all four this turn" -> QuantityComparison{BendTypesThisTurn, GE, 4} in parse_youve_this_turn; the existing "then if" intervening-if machinery attaches it to the Transform sub-ability (CR 603.4), not the Draw. Duration_ThisTurn swallow cleared by registering the new marker. BendingType has exactly 4 variants, so GE 4 == all four; four firebends give len 1 (distinct-type semantics). CR 603.4 / 608.2c / 701.65b / 701.66b / 701.67c / 702.189b. Flips supported:true gap_count:0 (both swallows cleared). Measured REGRESSED(engine)=0; GAINED = exactly [Avatar Aang]; semantic-audit 0 findings. 3 runtime tests (partial- bend discriminator revert-proven) + 1 inline parser test. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): S07 review round — CostTypeChoice cancel-rewind, viewer redaction, manabrew + stack-badge wiring Address the four review findings on the behold "choose a creature type" pre-cost (`WaitingFor::CostTypeChoice`, Celestial Reunion): - [MED] Cancel now clears the temporary `ChosenAttribute::CreatureType` from the spell object (`handle_cancel_cast`), so backing out fully rewinds the choice and a re-cast re-prompts instead of silently reusing the stale type (CR 601.2 + CR 733.1 — the entire action is reversed). - [MED] `filter_state_for_viewer` redacts `CostTypeChoice.options` to empty for viewers who cannot see the caster's private zones — the option list is derived from beholdable hand cards and leaked private info (CR 400.2). - [MED] manabrew-compat maps `CostTypeChoice` to `chooseType` with `validTypes` and `source_card_id` from `pending_cast.object_id` (was falling through to `UnsupportedPrompt`), plus a `waiting_for_type` label. - [LOW] `StackDisplay.getPendingCastObjectId` includes `CostTypeChoice` so the stack keeps its "Casting" badge during the prompt. Three discriminating tests, each revert-fail verified: cancel_after_type_choice_rewinds_and_recast_reprompts, cost_type_choice_options_redacted_for_opponent_viewer, cost_type_choice_prompt_maps_to_choose_type_with_source. Assisted-by: ClaudeCode:claude-opus-4.8 --------- Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…card leak) (phase-rs#5037) * fix: filter library-draw events before broadcasting to clients StateUpdate and GameStarted carry a parallel events array alongside the filtered GameState. That array was sent verbatim to every seat and spectator, leaking hidden library information: - GameEvent::CardDrawn exposes the specific object_id of the drawn card - GameEvent::ZoneChanged from Library embeds the full card identity in ZoneChangeRecord (name, types, mana value, etc.) The structured game log already excluded these events; opponents could still read the raw event JSON and learn every card drawn from library. Add filter_events_for_viewer and apply it at every server broadcast chokepoint (per-player StateUpdate/GameStarted and spectator updates). Library-to-hand moves and individual CardDrawn events are visible only to the drawing player; public moves (mill, cast, etc.) remain broadcast to all. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: rustfmt for event filter CI Co-authored-by: Cursor <cursoragent@cursor.com> * fix: rustfmt ai_events filter call for CI Co-authored-by: Cursor <cursoragent@cursor.com> * fix: gate library zone-change events on face-down visibility Library-to-battlefield manifest/cloak and face-down exile moves were still broadcast via ZoneChangeRecord identity. Use post-move object visibility (matching state filter) instead of a fixed destination allowlist. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: rustfmt test asserts and remove invalid exile.push in test Apply CI rustfmt for opponent/spectator assert blocks. create_object already registers exile zone entries; im::Vector has no push. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(PR-5037): honor turn control in draw event filtering --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…phase-rs#5046) PR phase-rs#5036 (Wave-1 legality vetoes + x_cast_gate search-depth perf fix) raised 6 engine simulation counters ~15-20% -- crew_eligibility_scans, legend_rule_mode _gate_scans, sba_battlefield_snapshot_builds, restriction_static_mode_gate_scans, layers_full_eval, state_clone_for_legality -- turning the non-required Decision-cost perf gate red. Verified via a frozen-card-data A/B (old 5aa1545 vs merged bb03392, both at card-data hash 1b59) that the increase is a trajectory/coverage shift, NOT a per-node cost regression: wall_clock was flat/faster (76.8s new vs 82-90s old), and the new policies do no state-clone or board-wide engine calls in verdict()/priors(). The vetoes change which actions the AI commits to, so simulated games traverse more board states that trigger these engine checks. CI new-code counters (crew 10041) match the local frozen-data run (crew 10019) within 0.2%, confirming card-data stability across environments. Rebaseline to the new counter levels so the gate reflects post-phase-rs#5036 AI behavior and can catch future real regressions instead of staying stuck red. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…-rs#5048) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: Codex <codex@openai.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: Codex <codex@openai.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: Codex <codex@openai.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix(parser): UNSUPPORTED one-off: Ace's Baseball Bat * test: annotate non-combinator test assertion in must-be-blocked diagnostic check * fix(PR-4591): address matthewevans review — spare capacity + mtgish import Resolves both CHANGES_REQUESTED items from @matthewevans: [MED] combat.rs — filtered MustBeBlocked spare-capacity correctness Previously, has_available_blocker for MustBeBlocked { by: Some(filter) } rejected any creature in assigned_blockers regardless of its ExtraBlockers capacity, mirroring a bug that MustBeBlockedByAll already handled correctly. Fix: switch the early-exit from assigned_blockers.contains(id) (all-attacker set) to blockers_per_attacker.get(&attacker_id).is_some_and(|bl| bl.contains(id)) (this-attacker set), and add attackers_per_blocker.get(id) < extra_block_limit() to allow multi-block-capable creatures to count as available — mirroring the MustBeBlockedByAll path at combat.rs:1496. Test: must_be_blocked_filtered_counts_multi_blocker_spare_capacity (Dalek with ExtraBlockers{count:Some(1)} blocking another attacker must still cover the MustBeBlocked attacker; assigned to both is legal). [MED] static_effect.rs — wire MustBeBlockedByADefender in mtgish converter P::MustBeBlockedByADefender(filter) previously fell through to UnknownVariant. Now maps to StaticMode::MustBeBlocked { by: Some(filter::convert(filter)?) }, matching the engine type this PR introduced. Covers Dalek (Ace's Baseball Bat) and Eldrazi (Slayer's Cleaver) cases in the mtgish corpus. CR 509.1c annotation added. Test: must_be_blocked_by_a_defender_lowers_to_filtered_must_be_blocked verifies Dalek and Eldrazi cases convert to MustBeBlocked{by:Some(_)}. Verification: - cargo fmt --all — clean - cargo clippy -p engine --all-targets — exit 0 - cargo clippy -p mtgish-import --all-targets — exit 0 - cargo test -p engine — 14139 passed / 0 failed - cargo test -p mtgish-import (unit tests) — 1 new test passed - check-parser-combinators.sh — no new forbidden patterns Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…tes need a hidden-commit tally (phase-rs#4685) * fix(parser): UNSUPPORTED cluster: SECRET-BALLOT vote seam — secret votes need a hidden-commit tally Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(PR-4685): widen vote candidate/ballot indices from u8 to u32 Matt's CHANGES_REQUESTED: SubmitVoteCandidate::candidate_index was u8, making candidates beyond index 255 unreachable or aliased in token-heavy games (Council's Judgment / Prime Minister's Cabinet Room). Widened to u32 across all seven affected surfaces: - GameAction::SubmitVoteCandidate { candidate_index: u32 } - WaitingFor::VoteChoice.ballots: im::Vector<(PlayerId, u32)> - GameState::last_vote_ballots: im::Vector<(PlayerId, u32)> - VoteRoundState::ballots + append_vote_ballot_and_advance(idx: u32) - QuantityRef::VoteCount { choice_index: u32 } - PlayerFilter::VotedFor { choice_index: u32 } - resolve_tally / resolve_top_votes_tally ballot signatures - All as u8 casts in candidates.rs, engine_resolution_choices.rs, oracle_vote.rs, and vote.rs tests Added regression test: object_vote_last_candidate_above_u8_max_is_selectable creates 257 opponent permanents, votes unanimously for candidate 256 (>255), and asserts it is exiled while un-voted permanents remain. Test fails with the original u8 cast due to index wrap. TieResolution::Breaker(u8) is intentionally left as u8 — it indexes named vote option lists (bounded by Oracle text, never >255 choices in practice) and is not involved in object-pool candidate dispatch. Verification: cargo clippy -p engine -p phase-ai --all-targets -D warnings clean; cargo test -p engine all pass including new regression test; server-core clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR * fix(PR-4685): update vote exile change-zone template --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
… — play historic from TOP OF LI (phase-rs#4341) * fix(parser): UNSUPPORTED cluster: The Fourth Doctor (Blast COMMANDER) — play historic from TOP OF LI * fix(PR-4341): address maintainer review — honest unsupported rider, land-play frequency, text/lower desync, context restore Maintainer @matthewevans raised four issues on PR phase-rs#4341 (The Fourth Doctor parser cluster). All four are addressed here. 1. Reflexive trigger provenance (issue #1): Replace the rules-incorrect `TriggerMode::PlayCard` trigger with a `TriggerMode::Unknown` gap marker. A global PlayCard trigger cannot distinguish which permission authorized a given play (CR 603.12), so the "When you do" rider was firing even when a different top-of-library permission authorized the play. The Unknown trigger keeps the gap visible in coverage until the casting/land-play pipeline gains permission-provenance tracking. 2. Land-play frequency consumption (issue phase-rs#2): Add `record_top_of_library_land_permission` in engine.rs and call it from all three completion arms of `handle_play_land`. A `OncePerTurn` `TopOfLibraryCastPermission { play_mode: Play }` now consumes the per-turn slot on land plays, mirroring how `finalize_cast` records this for spell casts (CR 401.5 + CR 601.2a). 3. Context mutation without restore (issue phase-rs#3 / Gemini): The reflexive rider is no longer parsed at all (Unknown trigger is emitted unconditionally), eliminating the ctx.subject/ctx.actor mutation that was leaking into subsequent lines. 4. text/lower desync on frequency-prefix strip (issue phase-rs#4 / Gemini): In `try_parse_top_of_library_cast_permission`, shadow both `text` and `lower` together when stripping the "once each turn, " / "once during each of your turns, " prefix, so downstream helpers receive aligned slices (Gemini phase-rs#3 suggestion, CR 601.2a). 5. Redundant scan_contains pre-check removed (Gemini phase-rs#2): The `scan_contains` guard before `split_once_on_lower` in oracle.rs was redundant — `try_parse_top_of_library_cast_permission` already validates the anchor. Removed for clarity. Test: `the_fourth_doctor_full_card_parse` updated to expect `TriggerMode::Unknown` instead of `TriggerMode::PlayCard`. Verification: cargo clippy -p engine --all-targets -- -D warnings: clean. cargo test -p engine: all pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR * fix(PR-4341): capture top-of-library land permission before zone change The `record_top_of_library_land_permission` helper was called in the land-play epilogue — after `zone_pipeline::deliver` had already moved the land from Library → Battlefield. At that point `top_of_library_permission_source` reads `player.library.front()`, which now points to the *next* card, so `top_id != object_id` always triggered and the slot was silently left unconsumed. A `OncePerTurn` top-of-library `Play` permission (The Fourth Doctor shape) could therefore be reused indefinitely for lands. Fix: capture `(src_id, frequency)` from `top_of_library_permission_source` *before* the replacement pipeline, alongside the existing `in_library_with_permission` eligibility check. The simplified `record_top_of_library_land_permission` now accepts the pre-captured pair instead of re-deriving it post-delivery. All three epilogue paths (Execute + `NeedsChoice` replacement prompt, `NeedsChoice` directly, and the normal path) use the captured value. Also adds `once_per_turn_library_land_play_consumes_slot_and_blocks_second_play`, a production-path regression test that plays two historic lands from the top of the library under a `OncePerTurn` `play_mode: Play` permission, asserts the first play stamps `top_of_library_cast_permissions_used`, and asserts the second play is rejected — the discriminating assertion that the pre-capture bug made impossible to satisfy. Addresses matt's round-4 review blocker (2026-06-28). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR * fix(CR-annotations): replace CR 601.2a with CR 305.1/116.2a/401.5 on land-play paths Matt's 2026-07-01 review (CHANGES_REQUESTED) identified that all six CR 601.2a annotations in the top-of-library land-play path cite the spell-casting-to-stack procedure (601.2a) instead of the rules that actually govern this path: - CR 305.1: playing a land is a special action, not a spell cast - CR 116.2a: playing a land puts the card onto the battlefield from the zone it was in (zero stack involvement) - CR 401.5: top-of-library visibility closes after the special action Fixed locations: - engine.rs: doc-comment on record_top_of_library_land_permission (×1) - engine.rs: inline comment in handle_play_land pre-capture block (×1) - engine.rs: three call-sites in the success/NeedsChoice/fallthrough branches of handle_play_land that record the permission slot (×3) - engine_tests.rs: doc-comment on the once-per-turn slot regression test (×1) All six now cite CR 305.1 + CR 116.2a + CR 401.5 with explanatory text. The two remaining mentions of CR 601.2a in the updated comments are explicit "does not apply here" notes, which is the correct documentation. Verification: cargo clippy -p engine --all-targets -- -D warnings (clean), cargo test -p engine (all pass), cargo fmt --all (no changes). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix(parser): UNSUPPORTED one-off: Last Night Together
* docs: reword fold doc comment to constructor form (combinator gate)
* fix(PR-4562): address matthewevans CHANGES_REQUESTED review comments
[HIGH] Move "each of them" plural-pronoun anaphor to central target parser.
The "each of them" → ParentTarget binding was only handled inside
`resolve_counter_placement_target` in counter.rs. Other effect parsers
(destroy, exile, bounce, tap, etc.) would still route through the "each"
+ type-phrase arm in oracle_target.rs and produce an all-matching
TargetFilter::Any, causing those effects to apply to every battlefield
permanent instead of the parent ability's chosen targets (CR 608.2c).
Fixed by adding a "each of" + parse_word_bounded("them") arm in
`parse_target_with_syntax` (oracle_target.rs), between the existing
"each of those" and "each of <N> target" arms. Resolution delegates to
`resolve_pronoun_target(ctx, "them")` which applies the same trigger-
subject vs. compound-anaphor dispatch as the bare "them" pronoun. The
special-case block is removed from counter.rs — the general path at
line 277 of resolve_counter_placement_target now handles it via the
central parser. Added two unit tests in oracle_target.rs and kept the
counter regression test (last_night_together_restricted_combat.rs) as
a consumer-level proof.
[MED] Store source ObjectId alongside attacker restriction to avoid dummy.
`passes_combat_attacker_restriction` was using ObjectId(0) as the
FilterContext source, breaking any source-relative restriction predicate
(e.g. "creatures that share a color with this card") on future cards.
Fixed by adding `attacker_restriction_source: Option<ObjectId>` to
ExtraPhase and `current_combat_attacker_restriction_source` to GameState.
additional_phase.rs sets the source to ability.source_id when scheduling
a restricted combat; turns.rs propagates it to the current field and
clears it alongside the restriction at end-of-combat. combat.rs now
reads the stored source via unwrap_or(ObjectId(0)) (the existing concrete-
set/Typed subjects are unaffected; this future-proofs source-relative
subjects per CR 611.2c).
Updated all ExtraPhase literal construction sites (15 in engine src +
6 in test files) to include attacker_restriction_source: None.
Verified: cargo clippy -p engine -p phase-ai -- -D warnings: 0 errors/warnings.
cargo test -p engine: all tests pass (including last_night_together_
restricted_combat, each_of_them_is_parent_target,
each_of_themselves_does_not_match_each_of_them_arm).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
* fix(PR-4562): clear combat attacker restriction on EndTheTurn (CR 724.1d + CR 511.3)
`end_turn_to_cleanup` cleared `extra_phases` but skipped the EndCombat
step, so `current_combat_attacker_restriction` and
`current_combat_attacker_restriction_source` were never cleared when
`Effect::EndTheTurn` (Time Stop, Obeka, Glorious End, etc.) fired during
a restricted extra combat. This left stale restriction state that could
leak into the next turn's attacker queries and AI fallback checks,
applying a previous combat's restriction outside its CR 511.3 duration.
Fix:
- `end_turn_to_cleanup`: clear both restriction fields immediately after
`extra_phases.clear()`, per CR 724.1d (turn ends during combat → combat
phase is over) + CR 511.3 (attacker restriction expires with combat).
- `start_next_turn`: add a defensive reset of both fields at turn start as
a belt-and-suspenders backstop against any future edge case.
- `end_the_turn.rs` tests: add regression
`end_the_turn_clears_combat_attacker_restriction` that seeds both
restriction fields, resolves EndTheTurn, and asserts both are None.
Addresses matthewevans' [HIGH] review comment from 2026-07-01 on commit
c61eab3: "Restricted-combat state still survives EndTheTurn."
Verified: cargo fmt --all, cargo clippy -p engine -p phase-ai
--all-targets -- -D warnings (clean), cargo test -p engine (0 failures).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
* Fix Last Night Together ability scan pattern
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…erived cost (phase-rs#5032) Generalizes the alternative-cost-keyword grant machinery from graveyard-only to a zone-parameterized grant, unlocking cast keywords granted in the HAND with a mana-value-derived cost: - Dream Devourer: "Each nonland card in your hand without foretell has foretell. Its foretell cost is equal to its mana cost reduced by {2}." (foretell, MV-2) - Aminatou, Veil Piercer: "Each enchantment card in your hand has miracle. Its miracle cost is equal to its mana cost reduced by {4}." (miracle, MV-4) Composes existing primitives rather than adding a subsystem: - New ManaCost::SelfManaCostReduced { reduction } leaf + reduced_by_generic() (CR 601.2f: reduces the generic component only, floored at {0}; colored/hybrid pips untouched). A pre-resolution placeholder concretized by the single cost authority resolve_keyword_mana_cost before any arithmetic path. - GraveyardGrantedKeywordKind -> GrantedCastKeywordKind with Foretell/Miracle variants and a grant_zone() method; target_filter_is_your_graveyard generalized to target_filter_is_your_zone(filter, zone). The grant gate declines cross-zone mismatches (foretell-in-graveyard, flashback-in-hand). - New nom combinator parse_reduced_by_generic_suffix threads the '{N}' reduction through all five per-keyword continuation-dispatch sites. reduction==0 normalizes to SelfManaCost, preserving existing graveyard grants byte-for-byte. - 'without foretell'/'miracle' route to KeywordMatch::Kind (not exact-cost Concrete) so the self-exclusion filter matches a granted instance. - Cost resolved through the single authority at all three runtime seams: the foretell special-action stamp into CastingPermission::Foretold{cost}, the miracle first-draw offer enqueue, and the miracle cast substitution. - CR 613.1f RAII self-reference guard: Dream Devourer's 'without foretell' grant is gated on a predicate over the same object; the guard resolves the nested query against base keywords only and cleans up on every exit path. Frontend/AI unaffected: the placeholder never crosses the wasm bridge unresolved (obj.mana_cost is always the printed concrete cost; MiracleReveal.cost and Foretold.cost are resolved at stamp time). Tests: 7 scenario-runner runtime tests (real miracle cast pays MV-4 from the pool; exiled Foretold{cost} carries the concrete MV-2, latched across source removal; floor-at-{0}; no-offer negatives) + 7 parser tests (grant shape, zone gate declines, reduction-suffix normalization, without-keyword Kind routing). Full cargo test -p engine green; workspace build + clippy -D warnings clean. CR 601.2f, 702.143a/d, 702.94a, 118.9/118.9c, 113.6b, 613.1f, 202.3.
…lause (phase-rs#5356) * fix(parser): "destroy/exile all X except for <type-list>" exclusion clause (phase-rs#4710) Scourglass's "Destroy all permanents except for artifacts and lands" silently dropped the exception clause and destroyed everything, because parse_type_phrase_with_ctx had no suffix parser for "except for <type-list>" (only the predicate-based "except those that <relative-clause>" was recognized). Confirmed via a live parse before the fix: Effect::DestroyAll{target: Typed{type_filters:[Permanent], ..}} with zero parse warnings. General class fix, not a one-card patch: a live Scryfall search (o:"except for" o:"destroy all") surfaces Elspeth Tirel's -5 ability ("except for lands and tokens") sharing the exact same grammar, with a heterogeneous type/property split (lands -> TypeFilter::Non, tokens -> FilterProp::NonToken). Adds parse_except_for_type_list_suffix, reusing the existing classify_negation (already used by the "nonartifact, nonland" prefix negation loop) and the existing Oxford-comma-tolerant match_mass_union_separator -- no new TargetFilter/TypeFilter/FilterProp variant, no runtime change (game/filter.rs's TypeFilter::Non evaluation already correctly treats artifact creatures as satisfying "artifact" per CR 205.2b). Guards against classify_negation's catch-all (any unrecognized word -> negated Subtype) misfiring on named/designation exceptions like Mageta the Lion ("except for Mageta") or Slash the Ranks ("except for commanders") -- those would otherwise silently produce a no-op Non(Subtype("Mageta")) exclusion that looks fixed but isn't. The new suffix declines the whole clause instead, leaving those cards' existing unhandled behavior honest. Verified with a live scratch parse before writing the fix, then 6 new tests (parser-level suffix shape, full-ability-line parse for both cards, hostile Mageta fixture, and a game/filter.rs runtime test proving Non(Artifact) excludes artifact creatures). Full engine test suite re-run clean: 15692 passed, 0 failed, 0 regressions. * fix(PR-5356): handle except-list whitespace * fix(parser): reject except-for-type-list suffix on filtered-subset exceptions CI's parse-diff coverage report on this PR caught a real regression: Flame Sweep ("Flame Sweep deals 2 damage to each creature except for creatures you control with flying") has an exception clause that is a FILTERED SUBSET ("creatures you control with flying"), not a bare type list -- but the naive suffix parser accepted "creatures" as a recognized type word and stopped at "you" (not a valid list separator), silently emitting Non(Creature) alongside the base Creature filter. That's self-contradictory and matches zero creatures, breaking the card's damage effect entirely (worse than the pre-fix silent-drop, which at least left the base filter intact). parse_except_for_type_list_suffix now requires the clause to actually terminate (end of text or a following '.') after the parsed list items; otherwise it declines the whole clause, mirroring the existing Subtype-fallback guard's philosophy of "decline rather than partially apply." Flame Sweep's flying-exception itself remains an out-of-scope, pre-existing gap (needs a filtered-subset "except for X you control with Y" mechanism, not this type-list one) -- this fix only ensures it isn't made worse. Added a regression test pinning Flame Sweep's base filter to plain Creature. Full engine test suite re-run clean: 15693 passed, 0 failed. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…esence (phase-rs#1033) (phase-rs#5350) * test(engine): pin Underworld Breach's grant to its own battlefield presence (phase-rs#1033) Investigated phase-rs#1033's second claim ("Breach recasts itself from the graveyard via its own granted escape"): already correct per CR 604.2 — a static ability's continuous effect only exists while its source remains on the battlefield. Verified locally: with Breach placed directly in the graveyard (simulating post-sacrifice), neither Breach itself nor another graveyard card shows escape availability derived from its grant. Adds permanent regression coverage alongside the existing granted_escape_requires_exile_cost_payment test, which covers the first (and only reproducing) half of the original report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(PR-5350): make Breach escape regression discriminating * fix(PR-5350): gate battlefield-only statics off-zone * fix(PR-5350): preserve command-zone emblem statics * fix(PR-5350): narrow off-zone base static grants --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…5123) * fix(parser): split repeated named spell conditions * docs(parser): fix named spell CR citation
* fix(parser): parse supertype-qualified enchant targets * fix(parser): address enchant target review cleanup
…s#5121) * fix(parser): parse repeated named control conditions * docs(parser): fix repeated named-control CR citation * docs(parser): narrow repeated named-control CR citation
* fix(parser): parse repeated named control clauses * fix(parser): address control named review cleanup * fix(parser): complete named-control list parsing
…s#5205) * fix(parser): UNSUPPORTED full-set one-off: The Doctor's Tomb * fix(engine): resume life assignment after replacement choice * test(engine): cover redistribution replacement resume
…5098) * fix(parser): bind perpetual cost grants to prior referents * fix(engine): no-op skipped perpetual parent targets * test(engine): update loyalty grant ability fixture * test(parser): keep bound-it cost grant context-specific * style(parser): move new test imports to module scope
…e-rs#5362) Famished Worldsire prints "Devour land 3" (CR 702.82c — sacrifice any number of LANDS, enters with 3 +1/+1 counters per sacrifice). The numeric-count keyword extractor ran parse_number on "land 3", which fails on the leading qualifier, so the param fell back to "land 3" and the Devour FromStr's unwrap_or(1) produced Devour(1) — the reported bug. Skip a single leading non-numeric qualifier word (via take_until) before extracting the count when parse_number fails at the head. Plain "Devour N" and other numeric-count keywords are unaffected (the skip only fires on the parse_number-fails branch). Note: Keyword::Devour(N) stores only the counter multiplier; the "lands only" sacrifice restriction (CR 702.82c quality) is a separate synthesis-layer gap and is left as follow-up. Closes phase-rs#5260
* fix: resolve unauthenticated P2P backup read exposure Require host_peer_id ownership checks on GET /p2p-draft-backup/:code so a guessed draft code alone cannot read backup snapshots. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(PR-5365): hide P2P backup owner mismatches --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…bstruction) (phase-rs#5319) * feat(engine): loyalty-ability activation cost modifiers (Eidolon of Obstruction) Loyalty abilities are activated abilities (CR 606.1), so a "cost {N} more/less to activate" static can target them the same way it targets any activated ability. Three composed pieces let Eidolon of Obstruction — "Loyalty abilities of planeswalkers your opponents control cost {1} more to activate" — parse and function end to end: - Parser: the existing "Activated abilities of [subject] cost {N} …" static (Training Grounds / Skyseer's Chariot) is parameterized on its leading noun so it also accepts "Loyalty abilities of [subject]", emitting `ReduceAbilityCost { keyword: "loyalty" }`. Only the noun axis varies, so it is one `alt` — not a new handler. - Runtime gate: `apply_static_activated_ability_cost_reduction` matches a `keyword == "loyalty"` static against an ability whose cost `is_loyalty_ability_cost` (loyalty abilities are identified by their `AbilityCost::Loyalty` cost, not an `AbilityTag`). - Cost application (CR 118.7): a loyalty ability's printed cost has no mana component, so `increase_generic_in_cost` now ADDS a generic-mana component (wrapping the cost in a `Composite`) instead of no-oping. This also fixes the latent gap where a cost-raise static silently failed to tax any activated ability whose cost is `{T}`/sacrifice-only. Tests: parser test pins the `keyword: "loyalty"` mode; a runtime test drives `apply_cost_reduction` and asserts an opponent's loyalty ability gains {1} while the controller's own loyalty ability and a non-loyalty ability are untaxed. * review: keep taxed loyalty abilities under the CR 606.3 once-per-turn gate Addresses PR review: - HIGH (matthewevans): after `increase_generic_in_cost` wraps a loyalty ability's cost in `Composite { Loyalty, Mana }`, `is_loyalty_ability_cost` no longer recognized it, so the direct `GameAction::ActivateAbility` guard in `handle_activate_ability` skipped the CR 606.3 per-permanent gate and a second taxed loyalty activation could slip through. `is_loyalty_ability_cost` now recurses into `Composite`, so a taxed loyalty ability stays classified as a loyalty ability everywhere the predicate is used. Adds a direct-activation regression that fails on a second taxed loyalty activation in the same turn. - MEDIUM (gemini): simplify the `std::mem::replace` + `if let` in `increase_generic_in_cost` to a direct assignment, and compute `ability_is_loyalty` on the already-unwrapped `cost` instead of re-checking the `Option`. * review: apply the Eidolon tax on the real loyalty-activation path Addresses matthewevans' [HIGH]: the fixed-loyalty fast path (`handle_activate_loyalty`) paid only loyalty counters and never called the cost-modifier authority, so an Eidolon-taxed loyalty ability could still be activated for free in actual play — the earlier test only exercised `apply_cost_reduction` directly. - `handle_activate_loyalty` now detects (via `loyalty_ability_gains_mana_tax`) when a cost-raise static adds a mana component to a loyalty ability and defers to the general `handle_activate_ability` flow, which applies the tax, pays the loyalty counters, records the CR 606.3 activation, and enforces the once-per-turn gate. The CR 606.6 negative-loyalty check runs first, before either path, so it is preserved for the delegated case. Untaxed loyalty abilities keep the unchanged fast path (zero behavior change). - CR 601.2g + CR 118.7: the mana-leg-first detour in `handle_activate_ability` now also fires for a loyalty cost, so a `Composite { Loyalty, Mana }` hoists its mana leg through `enter_payment_step` (paid FIRST) and defers the loyalty counter cost as the `ManaLeg` residual. Without this the composite fell through to `pay_ability_cost`, which paid the loyalty counters and then failed on the unaffordable mana — a free loyalty change. - Production regression `eidolon_forces_mana_payment_for_real_loyalty_activation` drives the real `handle_activate_loyalty` entry point: an untaxed activation needs no mana, while a taxed activation by a manaless player stops at the mana-payment step (or is refused) and never changes loyalty. Full engine suite green (15,658 passed); clippy --all-targets clean. * review: keep taxed targeted loyalty abilities target-first (CR 601.2c) Addresses matthewevans' [HIGH]: delegating an Eidolon-taxed loyalty ability to `handle_activate_ability` hoisted the added mana leg — and paid the loyalty cost / recorded the activation on resume — before target selection, so a targeted loyalty ability could spend mana/loyalty before its targets were chosen or the activation was cancelled. The untaxed loyalty path picks targets first (CR 601.2c/602.2b); the taxed path must too. - The mana-leg-first hoist now fires only for a NON-TARGETED taxed loyalty ability. A targeted one falls through to the general target-first path, which chooses targets before paying costs — matching the untaxed loyalty path. - `increase_generic_in_cost` now places the added mana component FIRST in the wrapping `Composite` (CR 601.2h — costs may be paid in any order). Any payment path that pays a `Composite` in order now settles the mana leg before the loyalty counters, so an unaffordable/cancelled mana payment never leaves a free loyalty change — including the post-target payment on the targeted path. - New regression `eidolon_taxed_targeted_loyalty_selects_targets_before_paying` drives a taxed targeted `[−1]` loyalty ability through the real `handle_activate_loyalty` entry point and asserts it stops at target selection with the loyalty counter unchanged. Full engine suite green (15,750 passed); clippy --all-targets clean.
…-card draw (CR 121.6b) (phase-rs#5360) * fix(engine): offer draw-replacement independently per unit of a multi-card draw (CR 121.6b) Multi-card draws (Effect::Draw{count: N > 1}) treated the whole count as one atomic replaceable event: accepting a substitute replacement (Dredge, Notion Thief, Hullbreacher, or any other ReplacementEvent::Draw producer) zeroed the ENTIRE proposed count, not just the unit it replaced. A count:2 draw with one dredge-eligible card in the graveyard, if dredged, delivered zero cards instead of one dredged + one normal draw -- exactly reproducing a live bug report: "drew no cards" when dredging during a two-card draw, then the unconditional discard step still fired as normal. CR 121.6b: "If an effect replaces a draw within a sequence of card draws, the replacement effect is completed before resuming the sequence." Each unit of a multi-card draw is an independent event and must be independently offered replacement. Adds resume_multi_draw (game/effects/draw.rs), looping one unit at a time through the existing draw_through_replacement/apply_draw_after_replacement primitives instead of proposing the whole count as one event. A unit that pauses on its own replacement choice stashes the new PendingMultiDraw{player, remaining, accumulated} continuation (types/game_state.rs), drained by handle_replacement_choice's Draw arm (engine_replacement.rs) using the same re-pause-composes-arbitrarily-many-times contract already proven by the zone-move batch-delivery machinery (zone_pipeline.rs's library_placement_survives_two_sequential_parks). CR 609.3: apply_draw_after_replacement now returns the per-unit actually- drawn count instead of writing state.last_effect_count itself -- resume_multi_draw accumulates the true total across every unit of the original instruction and commits it exactly once, so a chained "discard that many" sees the real total (e.g. 1, for a 2-unit draw where one unit was dredged for 0), not just the last unit's count. CR 800.4a: PendingMultiDraw is added to replacement::abandon_post_replacement_ continuation (the single authority for tearing down a departing player's in-flight continuations, per its own documented history of a prior bug caused by hand-listing fields at the call site instead) rather than elimination.rs directly -- single-player-scoped, safe to null outright, unlike the deliberately-preserved multi-player queue fields nearby. engine_debug.rs's DebugAction::DrawCards routes through the same resume_multi_draw primitive for parity with the real draw pipeline. Connive's own multi-count draw path (Connive N, N > 1) is explicitly NOT touched by this fix -- it has a documented history (connive.rs) of a previously-fixed collision between a shared generic mid-draw continuation and its own draw-then-connive ordering requirements, and reusing the new pending_multi_draw mechanism at the same resume call site risked reintroducing that exact failure mode. Tracked as a separate follow-up. Six rounds of adversarial plan review preceded implementation, each finding a real, previously-hidden issue: incomplete caller enumeration, an unaddressed count-doubling replacement class (Teferi's Ageless Insight/ Brainsurge, CR 121.2/614.11a), a hand-waved resume signature, last_effect_ count corruption across loop iterations, Connive's collision risk, and the elimination-cleanup fix's correct location. An off-by-one in the initial remaining-count bookkeeping was caught by the first test run and fixed before commit. Tests: two new production-path regression tests (dredge one-of-two units + decline-still-draws-normally) driving the real GameAction::ChooseReplacement dispatch, plus two elimination-cleanup assertions (departing player's pending_multi_draw cleared; surviving player's preserved). Full engine test suite re-run clean: 15698 passed, 0 failed, 0 regressions. Clippy clean. * fix(engine): fold the just-resumed unit's drawn count into the multi-draw total matthewevans/gemini-code-assist review on phase-rs#5360: handle_replacement_choice's Draw arm resolves the ONE paused unit directly via apply_draw_after_replacement and discarded its returned count -- that unit's contribution never reached resume_multi_draw's accumulator (which only sums units IT executes without pausing), so state.last_effect_count silently undercounted whenever a paused unit went on to draw real cards (e.g. declining a Dredge offer). Fold the just-resolved unit's actually-drawn count into the stashed PendingMultiDraw.accumulated right where it's produced, before the drain below reads it to resume the remaining units. Extends the existing decline regression to also decline unit 2's offer and assert last_effect_count == 2 (both units' real draws), the exact fixture the review suggested. Full engine suite re-run clean: 15698 passed, 0 failed, 0 regressions. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…dge works mid-draw (phase-rs#5372) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Owner
Author
|
Superseded by phase-rs#5305 (merged to main). Upstream PR phase-rs#5302 closed for the same reason. |
…ase P/T (phase-rs#5300) Auras that name only a creature subtype before the base-P/T seam (Lignify) strict-failed because parse_enchanted_is_type requires a core card type word. Add parse_enchanted_is_creature_subtype_with_base_pt composing SetCardTypes, RemoveAllSubtypes, AddSubtype, SetPower/Toughness, and RemoveAllAbilities from existing layer modifications with no new runtime. Closes phase-rs#5300. Co-authored-by: Cursor <cursoragent@cursor.com>
Trim trailing dot on lowercased base-P/T fragment before remainder extraction (Gemini). Add inline CR 205.1a / CR 613.4b annotations on modification pushes. Add trailing-dot regression test via parse_static_line. Co-authored-by: Cursor <cursoragent@cursor.com>
Add parse_pt_mod_with_remainder in grammar.rs composing signed and unsigned
nom P/T parsers; replace hand-scanned find('/') tail extraction in the
Lignify-class handler. Remove redundant inline CoreType/SubtypeSet import
(already in prelude). Add combinator-level unit test.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Setting a creature subtype replaces creature subtypes only — card types stay (Artifact/Land creatures keep Artifact/Land). Align dedicated handler with parse_enchanted_is_type and lignify_subtype_only test. Co-authored-by: Cursor <cursoragent@cursor.com>
…type Follow-up to phase-rs#5305: Lignify coverage landed on main via parse_enchanted_is_type. Remove redundant dedicated dispatch arm; add parse_pt_mod_with_remainder and wire it into parse_enchanted_is_type base-P/T and inline-P/T paths, replacing find('/') byte-slicing. Co-authored-by: Cursor <cursoragent@cursor.com>
jsdevninja
force-pushed
the
jsdevninja/lignify-subtype-type-change-parser
branch
from
f730f80 to
0b0b4db
Compare
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
20 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.
Summary
parse_enchanted_is_creature_subtype_with_base_ptinoracle_static/type_change.rsfor the Lignify class: enchanted type-change auras that name only a creature subtype before thewith base power and toughnessseam (Treefolk,Wall, …) without an explicitcreaturecore-type word.SetCardTypes,RemoveAllSubtypes,AddSubtype,SetPower/SetToughness,RemoveAllAbilities) in written order — no new engine variant, no new runtime (same structural pattern as feat(parser+engine): Imprisoned in the Moon type-swap-with-granted-ability aura (#4770) phase-rs/phase#5142 / Bug? — The Ai controls Strefan, Maurer Progenitor as commander, i used Imprisoned in the moon on it, and its abilities… phase-rs/phase#4770).parse_enchanted_becomes_type_with_ability, beforeparse_enchanted_is_type.Closes phase-rs#5300.
Test plan
RemoveAllAbilitiesloses all abilities and is a Treefolk …)parse_enchanted_is_type)cargo fmt, parser combinator gate,cargo test -p engine --lib parser::oracle_static