parser: compose leading action with as-enters persisted choice (Anointed Peacekeeper ETB) - #1
Closed
keloide wants to merge 1 commit into
Closed
parser: compose leading action with as-enters persisted choice (Anointed Peacekeeper ETB)#1keloide wants to merge 1 commit into
keloide wants to merge 1 commit into
Conversation
…ted Peacekeeper ETB)
Implement Anointed Peacekeeper's previously-Unimplemented ETB line
("As ~ enters the battlefield, look at an opponent's hand, then choose any
card name.") by composing existing engine primitives into four general
building blocks — no new engine enum variant:
1. try_parse_named_choice: accept "any card name" alongside the existing
a/nonland/creature/land card-name forms (CardName choice).
2. parse_hand_possessive_target: accept non-targeted "an opponent's hand"
(controller-scoped Opponent), routing "look at an opponent's hand" to a
private RevealHand { reveal: false }.
3. reveal_hand::resolve: additive player-from-filter fallback via
collect_player_targets when no explicit player target slot exists
(replacement/resolution context). Explicit TargetRef::Player fast path
and Any-target MissingParam behavior unchanged.
4. parse_as_enters_choose: compose an optional leading action before the
persisted Choose, anchoring on the last enters-frame; honest-coverage
gate (returns None, leaving the line Unimplemented) if the leading
clause does not parse, so no semantics are silently dropped.
Lines 3 & 4 (cost taxes keyed to the chosen name) already consumed a
persisted ChosenAttribute::CardName; this ETB now establishes it plus the
private hand-look.
CR annotations reused from existing in-repo citations (614.1c/d, 701.20a/e,
800.4a, 201.3, 607.2d, 613.1).
Tests: parser unit tests (any-card-name accept + negatives; an-opponent's-hand
private look + siblings), resolver tests (opponent-hand fallback with
multi-authority hostile fixture, explicit-target fast path, Any->MissingParam),
as-enters composition + honest-gate tests, and an end-to-end cast pipeline test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019mjar7ovGZua3ANz9r76fk
matthewevans
added a commit
that referenced
this pull request
Jul 4, 2026
… — 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 #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 #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 #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 #3 suggestion, CR 601.2a). 5. Redundant scan_contains pre-check removed (Gemini #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>
matthewevans
added a commit
that referenced
this pull request
Jul 9, 2026
…phase-rs#5155) * feat(engine): dynamic keep count on the dig pipeline (Stargaze) Parameterize PutCount::Up/Exactly payload u32 -> QuantityExpr and add an additive Effect::Dig.keep_count_expr so "put X cards from among them" (dynamic keep) both lowers and resolves. Unlocks Stargaze and the whole "look at N, put <dynamic> into hand, rest into Y" class. The look count (twice X) already resolved; the dynamic keep was the sole blocker. Reusable building block: single-authority PutCount::to_dig_keep mapping; keep resolved against game state before WaitingFor::DigChoice; additive serde-default field keeps existing fixed-count Dig snapshots byte-identical. CR 701.20e (look), CR 608.2c (follow instructions), CR 107.1b (negative -> 0). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): opponent-constrained target-player slot (Quick Draw) Add ControllerRef::TargetOpponent -- a pure routing tag whose runtime read is identical to TargetPlayer but whose companion target-player slot offers only opponents (self excluded; any one opponent in >2p), reusing the existing TargetFilter::Typed{controller:Opponent} + find_legal_targets legality path. Lowers "creatures target opponent controls lose <kw>..." (Quick Draw) and unlocks the whole "target opponent controls" class. Shared walker effect_bound_filter_matches feeds both target-player and target-opponent detection; relative_controller_kind normalizes TargetOpponent -> TargetPlayer so the fanout/rewrite subsystem reuses unchanged; the silent spell-filter wildcard is closed to fail-closed. CR 109.4, CR 102.2 / CR 102.3 (opponent), CR 611.2c (EOT set fixed at start), CR 702.7a (first strike), CR 702.4a (double strike). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): reflexive "this way" delayed-trigger building block (Prishe, Rhino) Add DelayedTriggerLifetime::Reflexive (CR 603.12) — reflexive triggered abilities are checked immediately after being created, firing on whether the trigger event occurred earlier during the same resolution. Generalizes the former coin-flip-only discard special case: reflexive_coin_flip_resolved_without_match is removed and replaced by lifetime-keyed is_reflexive_lifetime, and build_reflexive_coin_flip_trigger now emits Reflexive so coin flips route through the same rule. Parser gains a nom detector on the disjoint " this way, " delimiter (try_parse_reflexive_this_way_trigger) plus a damage-first recognizer (parse_reflexive_excess_damage_trigger, CR 120.10 excess-damage). Zone-change "this way" reflexives stay deferred via the strip_if_you_do_conditional guard; unknown conditions remain honestly Unimplemented. Cards: Prishe's Wanderings (search-library reflexive -> +1/+1 counter), Rhino's Rampage (excess-damage-first reflexive -> destroy up-to-1, scoped to And[ParentTarget, opponent-controlled]). Tests: 4 runtime pipeline tests (2 positive, 2 discard revert-failing on Reflexive->ThisTurn) + 2 parser round-trips; migrated the breeches coin-flip integration test to the Reflexive lifetime. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): scope "lose control" trigger + emit control-loss at cleanup (CR 514.3a) match_changes_controller fired on ANY ControllerChanged/EffectResolved{GainControl}, ignoring valid_card and direction — a latent over-fire (Portent trap) for the three supported "When you lose control of ~" cards (Khârn the Betrayer, Duplicity, Gustha's Scepter). Scope it to ControllerChanged, gated by valid_card, and resolve direction by source identity: - self-ref ("~"): the source IS the changing object, whose controller has already flushed to new_controller by trigger-scan time (flush_layers runs at the top of collect_pending_triggers). Rely on CR 603.10d look-back — a loses-control ability is intrinsically the pre-change controller's, and old != new already guarantees exactly one loser. Fire for it. - delayed/SpecificObject (Stolen Uniform): the source is the graveyard spell whose controller stays constant (the temp holder), so old_controller == source.controller fires on the loss (old == caster) and not the initial gain (old == owner). CR 603.2. Deliberate rules-correctness flip for the three cards: they no longer over-fire on unrelated control changes or gains; the correct "that permanent leaves your control" case still fires. Targeted GainControl::resolve now emits ControllerChanged (mirroring GainControlAll and GiveControl) so dropping the matcher's redundant EffectResolved arm cannot regress a loss. execute_cleanup emits ControllerChanged when an until-end-of-turn control effect ends (CR 514.2), fires delayed triggers on that loss before the this-turn prune, and hands back priority; priority.rs re-enters cleanup once the stack empties (CR 514.3a "another cleanup step begins"). This is the runtime half that lets a future "when you lose control of that <permanent> this turn" delayed trigger (Stolen Uniform) fire; the card's parser front half is not yet supported and stays honestly Unimplemented. Tests: 4 runtime tests driving the real cleanup/priority/dispatch pipeline (3 delayed SpecificObject + 1 self-ref), each revert-probed RED against the exact gate it covers. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): look-at + play face-down exile building block (Outrageous Robbery) Outrageous Robbery: "Target opponent exiles the top X cards of their library face down. You may look at and play those cards for as long as they remain exiled. If you cast a spell this way, you may spend mana as though it were mana of any type to cast it." Adds casting::player_may_look_at_facedown_exile — the single authority for "may this player look at this face-down exiled card?", delegating to play_from_exile_permission_source so look- and play-permission cannot diverge (CR 406.3b: a face-down exiled spell may be cast only if the player is allowed to look at it). It inherits the source's card_filter / single_use / per-turn gating. visibility.rs face-down-exile redaction consumes it as a third look-permission class alongside foretell and hideaway. The play grant carries mana_spend_permission: Some(AnyTypeOrColor) (CR 609.4b), read by the existing play-from-exile payment path; the reveal turns the card face up on cast (CR 406.3a). Parser: the subject-voice "<player> exiles the top N ... face down" arm now (a) resolves the cost's X to Variable("X") (was Fixed(1)), (b) honors a trailing "face down", and (c) scans "as though it were mana of any type" (was color-only) so the rider folds onto the play grant. Corrects a pre-existing wrong CR annotation on that arm (701.10a Doubling -> 701.13a Exile). swallow_check recognizes the folded PlayFromExile{mana_spend_permission: Some(_)} as the structural form of the "if you cast a spell this way" rider, suppressing a Condition_If false positive (also covers Brainstealer Dragon). Tests: 3 runtime tests (grant lands face-down + any-type on real cast pipeline; look-permission is grant-scoped — grantee sees the cards, owner and other-source face-down exiles stay hidden; permission persists across the turn) + 1 parser shape test. Off-color cast-from-exile payment proven by the shared PlayFromExile consumer-arm test. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): parse "the spell's mana value <= X" gate for impulse-cast (Bre of Clan Stoutarm) Bre of Clan Stoutarm's end-step ability: "if you gained life this turn, exile cards from the top of your library until you exile a nonland card. You may cast that card without paying its mana cost if the spell's mana value is less than or equal to the amount of life you gained this turn. Otherwise, put it into your hand." The only unsupported piece was the trailing mana-value gate: it was silently dropped, which cascaded the "Otherwise" clause into Unimplemented. Adds the nom combinator parse_offered_card_mana_value_comparison — "[the|that] spell's/card's mana value is {less/greater than [or equal to]} <quantity>" -> StaticCondition::QuantityComparison { ObjectManaValue{Target} <cmp> <quantity> } (CR 202.3 mana value, CR 115.1 target, CR 608.2c). It is hard-anchored on the demonstrative prefix so it does not overlap the reflexive "its mana value is N" (ObjectManaValue{Recipient}) or the "with mana value N" filter forms. Once the gate re-homes onto the cast clause, the pre-existing else_ability machinery routes "Otherwise" to hand automatically — no new Effect, no runtime change; the ExileFromTopUntil / CastFromZone{without_paying_mana_cost} / LifeGainedThisTurn building blocks were already wired. Bre's activated ability already parsed. Tests: 4 runtime tests on the real trigger->resolution pipeline — free-cast when MV <= life (revert-failing on the combinator), to-hand when MV > life, no-op when no life gained (intervening-if), and a decline-while-eligible -> hand CHARACTERIZATION test. The decline case documents a known interpretive edge: the engine's else_ability convention routes both condition-false and optional-decline to hand, whereas a strict reading of "Otherwise" (= MV>life only) would leave a declined-but-eligible card exiled; no published Bre ruling either way as of 2026-07-02, tracked as class-wide engine debt (Bre/Wick/Chandra). Assisted-by: ClaudeCode:claude-opus-4.8 * fix: thread keep_count_expr + TargetOpponent through non-engine consumers (mtgish-import, phase-ai tests) Commits 1b67f0248 (Stargaze) and 6f3a35d11 (Quick Draw) added the `keep_count_expr` field to `Effect::Dig` and the `ControllerRef::TargetOpponent` variant to the engine but did not update the non-engine consumer crates, leaving CI's exact lint surface (`cargo clippy --workspace --exclude phase-tauri --all-targets`) red. `cargo check --workspace` and `cargo test -p engine` both miss this: nextest excludes mtgish-import, and a plain `check` skips test targets. - mtgish-import action.rs: add `keep_count_expr: None` to all 11 `Effect::Dig` initializers (fixed keep-count; the dynamic-keep override is populated only by Stargaze-class digs this crate does not convert). - mtgish-import player_effect.rs: add the `ControllerRef::TargetOpponent` arm to controller_to_scope, strict-failing with EnginePrerequisiteMissing (a single targeted opponent has no broadcast ProhibitionScope; mapping it would over-broaden the prohibition). - phase-ai control.rs + spellslinger_prowess.rs: add `keep_count_expr: None` to the 4 `Effect::Dig` initializers in `#[cfg(test)]` fixtures (only compiled by clippy --all-targets). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): Memory Vessel — play-from-exile grant + can't-play-from-zone prohibition Memory Vessel ({T}, exile it: each player exiles the top 7, may play them until the activator's next turn, and can't play from their hand) now lowers fully — the previously-collapsed "players may play cards they exiled this way, and they can't play cards from their hand" clause resolves into a per-owner play-from-exile grant plus a play-from-zone prohibition. Engine (building blocks, reused across the class): - ProhibitPlayFromZone { zone } on ProhibitedActivity — a DENY axis covering both casting and land plays (CR 116.2a/305.1/601.2a), distinct from the CastOnlyFromZones allow-list which is cast-only. Enforced at the cast gate, the play-land gate, and the castable-surface filter; the multiplayer HUD filter handles the new variant. - The untap-step prune keys "until your next turn" expiry on the granting ability's controller via the existing exiled_by_ability_controller field (CR 514.2/611.2a), so a per-owner grant expires at the ACTIVATOR's next turn, not each grantee's — mirroring the end-step prune already used by Rocco, Street Chef. No new field. Parser (nom combinators, build-for-the-class): - parse_per_owner_exiled_this_way generalizes the ObjectOwner grant arm to "[each player|players] may [play|cast] [the] card[s] they exiled this way" (covers Rocco + Memory Vessel). - try_parse_cant_play_from_zone: "[scope] can't play [cards|lands...] from [zone]" -> ProhibitPlayFromZone (also flips Shaman's Trance's graveyard prohibition — a class win). - try_parse_exile_play_grant_with_play_prohibition composes the two under a shared leading duration. Tests: card-level parse assertion (0 Unimplemented) + 3 multiplayer runtime tests (per-owner scoping, activator-keyed cross-player expiry, can't-play-from-hand blocks both cast and land while exile plays stay legal). Empirical corpus coverage diff (2555-card superset): net Unimplemented -2 (Memory Vessel + Shaman's Trance flip supported), zero regressions. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): dual-target slot registry + anaphoric slot binding (Stolen Uniform front-half) Add a declared-target-slot registry on ParseContext so "Choose target X and target Y" chains bind later anaphors ("that Equipment", "the chosen creature", "the artifact card") to a precise ParentTargetSlot{index} instead of the ambiguous grab-all ParentTarget. Generalizes the Goblin-Welder hardcoded artifact-slot resolver into a type-driven registry lookup (hardcoded arms deleted; Goblin Welder reproduced via the general path). GainControl and Attach now bind slot-precisely (Slot1 / {attachment:Slot1, target:Slot0}). Also wires the ParentTargetSlot arm into Attach's resolve_object_filter and GainControl's gain_control_object_targets — their bespoke object-resolution paths lacked it — reusing targeting::resolve_parent_slot_from_root (root-chain nth), which incidentally fixes timmerian fiends (its "the artifact card" was wrongly bound to a non-existent slot). Front-half only: Stolen Uniform's last sentence ("When you lose control of that Equipment this turn ... unattach it") stays Effect::unimplemented pending the block-D delayed-trigger container; card is not yet supported. The determiner anaphor path uses nom combinators; the pre-commit parser gate's flags are stale-base (ae663ee8c) false positives on pre-existing mod.rs / untouched oracle_trigger.rs lines — none in this commit's additions (verified). CR 601.2c (target chosen once per instance of "target") + CR 608.2c (later instructions reference earlier objects via whole-chain accumulation). Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): classify new engine surfaces in the #4904 fail-closed walker Rebasing the S25 tranche onto main (#4904 growing-cascade detector + fail-closed ability-scan walker) surfaces two exhaustive-match classification points for engine surfaces this branch added before the walker existed: - ControllerRef::TargetOpponent (Quick Draw) — Axes::NONE in the C0 axis classifier, mirroring TargetPlayer. The two are runtime-read-identical; the opponent-only legality is enforced at target selection, not a walker axis (CR 109.4). - Effect::Dig.keep_count_expr (Stargaze) — scanned via scan_quantity_expr in the C0 axis classifier. A dynamic keep-count is a projected-resource read (axis 3), scaling with game state exactly like the dig `count`, so it must feed the growing-cascade detector identically rather than being ignored. keep_count_expr also passes through effect_resolution_choice_freedom's Dig arm, which classifies Dig as MayPrompt (fail-closed) — a keep-count adds no priority WaitingFor, so the {..} pass-through is sound and needs no per-field action. ProhibitedActivity::ProhibitPlayFromZone (Memory Vessel) and DelayedTriggerLifetime::Reflexive (Prishe/Rhino) are not walker-traversed; their own commits already handle their exhaustive match sites. Only ability_scan.rs (game/) changed; the pre-commit parser gate's flags are stale-base (ae663ee8c) false positives on pre-existing parser lines not in this commit. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): pay turn-face-up cost as a special action (Overgrown Zealot, Tin Street Gossip) Turning a face-down permanent face up is a special action (CR 116.2b) that must pay the morph/megamorph/disguise cost (CR 702.37e / 702.168d) or the manifested creature's mana cost (CR 701.40b). The engine previously performed the flip for free, so mana abilities whose only live branch is "produce mana usable only to turn a permanent face up" (CR 106.6 restricted-purpose mana) were left as honest-red Unimplemented gaps. - morph.rs: split the guards + cost extraction out of `turn_face_up` into `turn_face_up_prepare` (no signature change to turn_face_up; its ~8 free callers stay free — the payment lives in the handler, not the primitive). - engine.rs: the GameAction::TurnFaceUp handler now reduces + pays the cost via `pay_special_action_mana_cost(..., SpecialAction::TurnFaceUp, ...)`, mirroring the UnlockDoor special-action sibling. Sole production paid entry. - types/ability.rs: `has_payable_branch(TurnPermanentFaceUp)` flips dead→live so the sequence-absorption seam now absorbs the restriction into a real Effect::Mana (FaceDownSpell stays dead — CR 702.37c face-down casting is still unimplemented). Monotonic MORE→true: only the 3 TurnPermanentFaceUp cards are affected (Overgrown Zealot + Tin Street Gossip gain support; Creeping Peeper already supported via SpellType/UnlockDoor, unchanged). Discrimination proven empirically: reverting the handler payment flips R1/R2/R3 red; R2 (empty pool → Err, permanent stays face_down) is the load-bearing charge proof. No new enum/field (existing ManaSpendRestriction / SpecialAction::TurnFaceUp / PaymentContext::SpecialAction) → zero consumer-crate churn. s07-frozen files untouched. Parser dispatch unchanged (oracle_tests.rs changes are test assertions only; the gate's flags are stale-base ae663ee8c false positives, not this commit). CR 106.6 + CR 116.2b + CR 702.37e + CR 702.168d + CR 701.40b + CR 702.37c. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): grant abilities beyond activated-only via GrantedAbilityScope (Symbiote Spider-Man, Choreographed Sparks, Nalfeshnee) Parameterize Effect::GainActivatedAbilitiesOfTarget with a typed GrantedAbilityScope { ActivatedOnly (default) | AllOther } field (serde-default, zero consumer-crate churn) instead of adding a sibling variant — the granted ability set is fixed at effect start (CR 611.2c), within a single rule section. - AllOther snapshots BOTH the object's activated abilities AND its separate trigger_definitions store (CR 603.1 — triggered abilities are a distinct ability class the prior activated-only loop never read), granting each and excluding the granting ability itself; the grant is permanent (CR 611.2a). - The resolver branches on the donor filter: Symbiote Spider-Man inverts the axes (donor = this card via SelfRef, recipient = the +1/+1 target via ParentTarget) vs the existing mirror. - Choreographed Sparks / Nalfeshnee: apply_spell_copy_modifications now stamps AddKeyword + GrantTrigger onto both the base and live stores (they were silently dropped), so "the copy gains haste and a sacrifice trigger" persists across the copy→token boundary. The parser fold lands at lower_effect_chain_ir (the chain chokepoint shared by ability and trigger-execute chains), so Nalfeshnee — whose grant lives in a triggered ability — flips too. Walker: the new scope field is a static ability-kind selector (no game-state read) → Axes::NONE in the fail-closed ability-scan classifier. Discrimination proven empirically: reverting the AllOther trigger snapshot flips S1/S2/S3 red; disabling the copy-modification fold flips Choreographed + Nalfeshnee red. Coverage +3, zero regressions across all 35397 faces. No new Effect variant; s07-frozen files untouched. CR 611.2a + CR 611.2c + CR 603.1 + CR 701.21a (delayed sacrifice) + copy/haste rules. Assisted-by: ClaudeCode:claude-opus-4.8 * test(engine): flip Choreographed Sparks deferred-pin to supported P2f (4b2566eb6) implemented Choreographed Sparks' copy-grant (haste + delayed-sac fold via apply_spell_copy_modifications), invalidating the deferred-honesty guard `choreographed_sparks_copy_grant_is_deferred`, which asserted the card still lowered to Unimplemented. Flip it to a supported regression guard (`_is_supported`, asserts NOT Unimplemented). This integration test lives in tests/ and was missed by P2f's `cargo test -p engine --lib` run; the full `cargo test -p engine` gate catches it. Fixup for 4b2566eb6 — fold at ship-time autosquash. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): Stolen Uniform lose-control container — runtime-correct unattach delayed trigger (#4380 block-D) Parser recognizer for Stolen Uniform's last sentence ("When you lose control of that Equipment this turn, if it's attached to a creature you control, unattach it") plus the two engine gaps it exposed, so the delayed trigger actually unattaches the right Equipment at cleanup — not a hollow parse flip. Parser (oracle_effect/mod.rs, oracle_target.rs): new nom-only try_parse_lose_control_delayed_trigger emits CreateDelayedTrigger{ ChangesController, ThisTurn, valid_card: ParentTargetSlot{1} } with effect UnattachAll{ attachment: ParentTargetSlot{1}, target: Typed{Creature, You} } (intervening-if folded into the host scope). Fires only on "when you lose control of " + a resolvable dual-target-registry anaphor, so no lose-control sibling regresses (only Stolen is dual-target). CR 603.7 / 603.4 / 603.2 / 701.3d. Gap #1 — trigger stall (triggers.rs): UnattachAll is a non-targeted mass effect, but extract_target_filter_from_effect surfaced its host filter as a required target slot, so the delayed trigger paused on an unresolvable pick and never resolved. Carve it out like Sacrifice / at-resolution Bounce; matches the None its mass siblings (DestroyAll/BounceAll) return from Effect::target_filter. CR 701.3d + CR 115.1. Gap #2 — attachment anaphor (attach.rs): resolve_unattach_all passed the raw ParentTargetSlot{1} to matches_target_filter, which returns false for positive parent-refs by design (resolve at resolution time). Resolve the context-ref attachment against the ability's target snapshot via effect_object_targets, mirroring resolve_attach. Closes the divergence for the whole ParentTarget / ParentTargetSlot "attach/unattach it" class. CR 608.2c + CR 701.3d. Depends on the s07 delayed-trigger root-chain snapshot fix (a410d2d74, picked as ccbfc4b4b) so ParentTargetSlot{1} snapshots [C, E]. Tests: stolen_uniform_lose_control_unattaches_only_that_equipment (E unattached, hostile F stays — slot-specific) + _fold_leaves_opponent_hosted_equipment (host-scope discriminator, now non-vacuous) un-ignored and green; extract_target_skips_unattach_all building-block unit test (revert-fails if the gap#1 carve-out drops). Full test-engine: 14707 lib + all integration green; CI clippy 0 warnings. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): become-a-typed-token copula on reanimated objects (Vraska the Silencer, Brilliance Unleashed) P2e — "It's a <typed thing> …" applied to a returned/reanimated non-copy object, as an indefinite continuous effect bound to that object. Reuses the copy path's SetCardTypes + subtype + granted-ability builder; zero new engine variants, zero resolver changes, zero frozen-file edits. Vraska, the Silencer: "return that card … tapped under your control. It's a Treasure artifact with '{T}, Sacrifice this artifact: Add one mana of any color,' and it loses all other card types." The copula routes to the shared parse_its_a_type_loses_others builder (now pub(super)) via a new arm in subject.rs, gated on ParentTarget | TriggeringSource (declines SelfRef so a source-permanent misbind honest-defers). The dies-trigger return binds TriggeringSource, which the existing register_transient_effect arm resolves to the returned object — no publish-as-ParentTarget needed. CR 205.1a / 205.1b / 613.1d / 611.2a / 400.7 / 603.6 (NOT 707.9d — copy-effect-scoped). Block 1a (sequence.rs): the bare " and " sequence splitter bisected the copula before "…and it loses all other card types", hiding the CR 205.1a replacement signal from the builder. Suppress the split when the remainder is exactly "(it )?loses all other card types" — class-scoped to the replacement copula (all 16 such cards unchanged; coverage REGRESSED(engine)=0). Brilliance Unleashed (mode 2 Otherwise): "Otherwise, return it to the battlefield and it's a 3/3 Robot artifact creature with flying." The Otherwise else is a fresh recursive effect chain whose clause list starts empty, so the typed referent from "Choose target artifact card" was lost and the animation copula declined to Unimplemented. Seed ctx.parent_target_available across the else recursion (behind a skip_first_conditional param; both pre-existing callers pass false so the second consumer is byte-unchanged) and scope-rebind the reanimate-else animation duration to UntilHostLeavesPlay. Additive by construction — can only turn a declining anaphor into ParentTarget, never remove a binding. Bre of Clan Stoutarm (C12) reuses the same else-seed. C3: both copulas install Duration::UntilHostLeavesPlay (CR 400.7 — a returned object is a new object; mirrors install_aura_continuous_effect). Tests (std_longtail_e.rs, +4, deferred note flipped): parser round-trip + runtime for each card. The Vraska runtime test asserts the TCE binds SpecificObject{returned_id} (not Vraska via a use_self misbind, not inert), returned obj is Artifact+Treasure, and the granted ability sacrifices SelfRef — all revert-to-red proven. gen-card-data: both flip supported, gap_count=0. Full test-engine 16924/0; CI clippy 0 warnings; coverage REGRESSED(engine)=0; semantic-audit clean. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): cloak from a chosen non-library source (Vannifar, Evolved Enigma) Vannifar's "Cloak a card from your hand" — the controller cloaks a card they CHOOSE from hand, not the top of the library. Adds a source axis to Cloak and threads the chosen object through the resolver so it cloaks the right card, not a hollow library-top flip. Parameterize (not proliferate): add `object_source: Option<TargetFilter>` FIELD to Effect::Cloak (`#[serde(default, skip_serializing_if="Option::is_none")]`). None = CR 701.58e library-top source (Cryptic Coat, Ransom Note — byte-identical serialization, back-compat proven). Some(filter) = explicit objects chosen upstream. Zero new variants. Resolver (cloak.rs): the Some branch resolves the object ids from the resolving ability's already-populated `targets` via effect_object_targets(&filter, &ability.targets) — the objects a preceding Effect::ChooseFromZone chose and forwarded (CR 608.2c) — then manifest_card(…, cloaked_2_2()) per object. It does NOT read a TrackedSet (never published for a Cloak continuation) and does NOT touch the frozen effects/mod.rs. The None branch is the unchanged library-top loop. Parser: "cloak a card from your hand" lowers (at the intercept level, mirroring the SearchLibrary sub_ability precedent — a bare Effect can't express a chain) to a composite ChooseFromZone{zone:Hand,count:1} parent + Cloak{object_source: Some(ParentTarget)} sub_ability, reusing the fully-wired ChooseFromZoneChoice interactive stack (no new WaitingFor/AI/frontend). A `from_zone: Option<Zone>` discriminant on ImperativeFamilyAst::Cloak distinguishes hand-source from library-top. Pure nom (tag/alt/all_consuming). Walker (#4904 ability_scan.rs): the compile-forced Cloak arm gains a guarded `if let Some(f) = object_source { acc = acc.or(scan_target_filter(f)); }`. Anti-hollow-win test (tests/vannifar_cloak_from_hand.rs): drives the real resolve_ability_chain → ChooseFromZoneChoice → apply(SelectCards[A]) → cloak path; asserts the chosen hand card A is cloaked (face_down 2/2 ward{2}, leaves hand) while the distinguishable library-top card B is UNTOUCHED. Revert-to-red proven twice — point object_source at library-top → B cloaked / A stays (RED at "A must be cloaked"); revert the intercept → Unimplemented. Plus back-compat (library-top unchanged, object_source:None asserted explicitly) and negative sibling. Expose the Culprit remains a separate later gate (reuses this field). Full test-engine exit 0; CI clippy 0 warnings; coverage flip Vannifar supported gap_count 1→0, REGRESSED(engine)=0 GAINED=1; semantic-audit clean. CR 701.58a / 701.58e / 608.2c (grep-verified). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): cloak an exiled face-down pile — Expose the Culprit mode 2 Expose the Culprit's mode 2 ("Exile any number of face-up creatures you control with disguise in a face-down pile, shuffle that pile, then cloak them") lowers to ChooseObjectsIntoTrackedSet{Creature, You, HasKeywordKind{Disguise}} -> Shuffle{TrackedSet} -> Cloak{object_source: Some(TrackedSet)}. - KeywordKind::Disguise (CR 702.168): a discriminant-level keyword kind (like Morph/Megamorph) so HasKeywordKind{Disguise} names the class regardless of the Disguise(ManaCost) payload. The "with disguise" filter selects only face-up disguise creatures — a face-down permanent has no keywords (CR 708.2a). - Shuffle gains a TrackedSet/pile branch (CR 701.24a): randomizes the chain's tracked object set via the RNG and emits no ShuffledLibrary action, so a pile shuffle does not fire library-shuffle triggers. - Cloak gains a TrackedSet source: manifest_card on a battlefield permanent is a no-op (the Battlefield->Battlefield zone guard), and the card literally exiles then cloaks, so each chosen creature is EXILED (a real Battlefield->Exile move — CR 122.2 counters cease, CR 603.6c leaves-the-battlefield triggers fire, CR 704.5m/704.5n Auras and Equipment fall off) then manifested back from exile as a fresh face-down 2/2 with ward {2} (CR 400.7 new object, CR 701.58a/e). The cloak reads the shuffled tracked set directly so pile order is observable. Runtime tests drive the full cast chain and prove: chosen creatures become face-down 2/2s in the shuffled (non-selection) order; no ShuffledLibrary event; a +1/+1 counter is cleared and an attached Aura falls to the graveyard after the object reset; and an empty selection is inert. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): interactive "any number" multi-zone name-matched search-and-exile Route "search <its owner's/its controller's> graveyard, hand, and library for any number of cards with <same-name-ref> and exile them" to the interactive Effect::SearchLibrary path (CR 701.23b — a search for a stated quality lets the player fail to find), covering Deadly Cover-Up, The End, Crumble to Dust, Surgical Extraction, Test of Talents, and Deicide. Zero new engine enum variants. Generalizes the existing multi-zone same-name recognizer's quantifier axis (all cards | any number | up to N) and branches the lowering: "all cards" keeps the mandatory ChangeZoneAll; the interactive quantifiers lower to SearchLibrary{SameNameAsParentTarget}. An object-relative possessive guard (its owner's / its controller's only) keeps the chosen-name class (Unmoored Ego, Memoricide, ... — "choose a card name ... with that name") on the HasChosenName path (CR 201.2). - W4: resolve_library_owner resolves a Typed(ParentTargetOwner/Controller) searched-player via controller_ref_player; the caster remains the searcher (CR 701.23a asymmetric); bare ParentTargetController (Assassin's Trophy) untouched. - W5: a found-set hand-exile counter at SearchChoice completion feeds the "draws a card for each card exiled from their hand this way" rider (CR 121.1). - W6: apply_anchor_subject gains a Draw arm so the rider draws to the searched player, not the caster. - target_filter(): a bare Typed(ParentTargetOwner/Controller) searched-player is a resolution-time context-ref (like RevealUntil), not a cast-time target slot. Tests: 6 discriminating parser tests + 3 runtime cast tests (The End controller axis; Deadly Cover-Up owner axis, Case A/B with evidence gating; Test of Talents countered-spell seed), each with revert-to-red evidence. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): bind granted-ability self-references to the granting object (CR 201.5a) When an ability grants another ability that refers to the granting object by name (Deconstruction Hammer "Sacrifice Deconstruction Hammer", The Dominion Bracelet "{15}, Exile The Dominion Bracelet", Trusty Boomerang "Return Trusty Boomerang"), CR 201.5a says the name refers only to the granting object, never the host it was granted to. Previously these self-references collapsed to the host (~/SelfRef), so an equipment's granted sacrifice/exile/return acted on the equipped creature instead of the equipment. New parse-time TargetFilter::GrantingObject, emitted only in self-reference verb-object positions (sacrifice/exile/return/put-counter-on <self> inside a quoted granted body), concretized to SpecificObject{granting object} at grant-clone time (layers.rs GrantAbility/GrantTrigger, via effect.source_id). Three channels stay separate: granter-referential -> GrantingObject; host- referential "this permanent" -> SelfRef (host); host power read -> QuantityRef (host). The name masker is allowlist-gated to verb-object positions so out-of-scope in-quote self-name references (QuantityRef "counters on ~", exclusion "other than ~", damage-source "by ~") stay byte-identical to the pre-change baseline (coverage-regression: 0 regressed / 0 gained). A single post-parse sweep degrades any residual placeholder to ~ in description strings. Fixes the cost + effect-target channels: The Dominion Bracelet, Deconstruction Hammer, Trusty Boomerang, Razor Boomerang, Fishing Pole, Hankyu, Spare Dagger, Sakashima. The QuantityRef/condition/damage-source/exclusion granter-name channel remains host-bound (pre-existing; flagged in-code as a CR 201.5a follow-up). The Dominion Bracelet's {X}-less cost reduction folds into cost_reduction (host-referential power, CR 601.2f). Grant-time concretization snapshots the granter id and is scoped in-code to "no intra-resolution zone move of the granter" (CR 201.5a second sentence + CR 400.7). Zero new engine enum variants beyond TargetFilter::GrantingObject. Tests: 10 discriminating tests (Hammer full activate/resolve zone check; Bracelet exile + host-power reduction; Trusty bounce; Sliver "this permanent" host-ref preserved; Food Fight "named" filter preserved; Archery Training / Animal Friend / Torrent of Lava out-of-scope channels unmasked; description no-leak; Fishing Pole + Hankyu counter-target), each with revert-to-red. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): phase-scoped player control for Secret of Bloodbending (CR 723.2) Secret of Bloodbending: "You control target opponent during their next combat phase. If this spell's additional cost was paid [waterbend {10}], you control that player during their next turn instead." Previously the base "next combat phase" leaf was Unimplemented — CR 723.2 limited-duration control had no representation and the runtime could only release control at a turn boundary. Parameterize Effect::ControlNextTurn with window: ControlWindow { NextTurn, NextCombatPhase } (serde-default NextTurn — all existing fixtures/card-data load unchanged). CR 723.1 full-turn control (Mindslaver, Worst Fears, Sorin, Construct a Cosmic Cube) is untouched; only the phase-scoped window is new. Runtime EXTENDS the single control machinery — no parallel schedule or controller field. A new phase-boundary activate/release hook in finish_enter_phase (turns.rs) binds control at the affected player's next BeginCombat and releases at the following PostCombatMain/Cleanup, with a release-before-activate ordering that makes "next combat phase" the first only (CR 506.7d by analogy). One release authority, turn_control::release_control_at, serves all three release sites: turn boundary (start_next_turn), combat-phase boundary (finish_enter_phase), and leave-game (do_eliminate) — the last closing a pre-existing CR 800.4a/b gap that also affected Mindslaver's full-turn control. Parser: a window alt() axis on the control suffix combinator; a subject.rs deferral guard so the full pipeline routes "control ... during their next combat phase" to the imperative ControlNextTurn parser (it was mis-parsing "combat phase" to Unimplemented via the subject-predicate path); the waterbend-paid branch swaps to the NextTurn window via AdditionalCostPaidInstead; self-exile. Edge cases designed (CR 723.1b + Scryfall ruling 2025-10-02): a skipped combat phase carries; multiple combat phases bind the first only; the controlling or controlled player leaving ends control (CR 800.4a/b); 3+ players route only the controlled seat. Coverage-regression on fresh card-data: 0 regressed, +1 (Secret). Tests: 8 groups incl. runtime cast-pilot (unpaid -> NextCombatPhase, paid -> NextTurn + exile), control-active-exactly-within-combat, first-only latch, carry, controller-leaves, 3+ player seat scoping — each with revert-to-red. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(parser): repeat a whole process a fixed/variable count of times (CR 608.2c) "Then repeat this process X more times." (Another Round) previously left an Unimplemented{repeat} node. Recognize the unconditional count form "<q> more time[s]" in try_parse_repeat_process_directive and stamp the process root's repeat_for = Offset{ <q>, +1 } (= "once + q more"), reusing the existing ungated whole-chain repeat_for driver (repeated_full_chain, effects/mod.rs) — zero engine, zero new variant. A prior /review-engine-plan rejected a proposed new RepeatContinuation variant: RepeatContinuation is the non-count companion to repeat_for, and the count belongs in repeat_for. The recognizer uses the existing parse_quantity_expr_number combinator, so it covers the whole class: "X more times" -> Offset{Variable(X),+1}, "six more times" -> Offset{Fixed(6),+1}. Build-for-the-class, not one card — coverage: Another Round and Professor Onyx flip to supported; Development improves. eof- guarded so the conditional/stop/"once"/bare forms fall through unchanged (coverage-regression on fresh card-data: 0 regressed, +2 gained). CR 608.2c: the controller repeats the same instructions in order. CR 107.3a / 601.2b: X is announced at cast and fixed once. CR 400.7: each returned creature is a new object (blink) — verified via summoning-sick re-entry per cycle. Known follow-up (pre-existing, documented, not introduced here): "exile any number of creatures you control" lowers to a cast-time target set, so each repeat iteration re-blinks the same chosen creatures rather than re-choosing a fresh "any number" per process (strict CR 608.2c). This is the shared cast-time-vs- resolution-time selection quirk, out of scope for this card; the repeat count and X+1 cycles are correct. Tests: runtime cast (X=N,M creatures -> N+1 exile->return cycles; X=0 -> exactly 1) with live revert-to-red, plus a parser-shape guard. Parser-only. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(parser): reanimate self and a targeted graveyard card to the battlefield (CR 400.7) "Return this card and target land card from your graveyard to the battlefield tapped." (Sandman, Shifting Scoundrel) previously routed through the generic shared-destination splitter, which wrapped SelfRef into a non-resolvable `And` primary and left the verbless second conjunct Unimplemented — the whole ability was inert (not even offered from the graveyard, because the `And` primary is not a bare self-move so no activation zone was stamped). Add a leaf recognizer `try_parse_reanimate_self_and_target` in `lower_imperative_clause`, run before `try_split_targeted_compound`. It mirrors the shipped Coastal Wizard / Lady Sun two-chained self-and-target idiom, adapted to a battlefield destination: a BARE `ChangeZone { SelfRef }` primary (so `activation_zone_from_self_effect` stamps the graveyard, Bloodsoaked Champion precedent) plus a targeted `ChangeZone` sub_ability that reuses the full return-to-battlefield lowering (origin inference, enter-tapped riders) from the shared path rather than re-deriving them. The gate is self-validating — it fires only when the second conjunct genuinely lowers to a non-battlefield-origin → battlefield ChangeZone — so non-reanimation "return A and B" cards (e.g. Coastal Wizard's return-to-hand) fall through unchanged. Build-for-the-class: `strip_optional_target_prefix` recovers the "up to one other target creature card" cardinality that the return-to-battlefield lowering drops, so Slimefoot and Squee flips to supported too (multi_target = up_to(1), optional, untapped). Coverage-regression on fresh card-data: 0 regressed, +2 gained (Sandman + Slimefoot exactly); no sibling "return A and B" card moved. CR 400.7: each returned object is a new object; SelfRef names only the source incarnation. CR 608.2c: the two chained moves resolve in written order. CR 601.2c + CR 115.1: the graveyard card is a chosen target; SelfRef is not. CR 113.6m: the bare self-move is what makes the ability function from (and be offered in) the graveyard. CR 614.1: "to the battlefield tapped" enters both objects tapped. Tests: runtime activation from the graveyard (both objects return, both tapped; the Land filter excludes a nonland and the unchosen land stays put — not a sweep) with live revert-to-red (revert → null activation_zone → activation illegal; sub stays Unimplemented → nothing moves), plus two parser-shape guards and a coverage-honesty flip. Parser-only; no engine, no new variant. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(parser): disjunctive "first-of-type spell this turn" intervening-if (CR 603.4) Alania, Divergent Storm's trigger — "Whenever you cast a spell, if it's the first instant spell, the first sorcery spell, or the first Otter spell other than Alania you've cast this turn, you may have target opponent draw a card." — left the whole intervening-if plus the draw as an Unimplemented node. The already-built CopySpell "if you do" sub was correct and is preserved untouched. Recognize the three-way "first-of-type this turn" intervening-if by COMPOSITION, not a bundled ordinal variant. Add one anchor-only leaf `TriggerCondition::TriggeringSpellMatchesFilter { filter }` — the cast-spell "what it IS" sibling of the existing match-event-subject cluster (TriggeringSpellTargetsFilter / SourceMatchesFilter / ZoneChangeObjectMatchesFilter / EventDamageSourceMatchesFilter) — and compose each disjunct as And(TriggeringSpellMatchesFilter(T), QuantityComparison(SpellsCastThisTurn{You,T} == 1)), collected into TriggerCondition::Or. A prior /review-engine-plan rejected a bundled TriggeringSpellIsNthCast{n,filter} as a layer-conflation (match axis + count axis in one leaf) that also duplicates the existing SpellsCastThisTurn count; the composition separates the layers and writes zero new count code, and is behavior-identical at the CR 603.4 re-check. The parser leaf recognizer (nom combinators, >=2-disjunct guarded so single-disjunct cards stay on the untouched NthSpellThisTurn constraint path) emits that shape; the "other than ~" self-exclusion lowers to Not(Named{card-name}). Build-for-the-class: the anchor variant also unlocks the plain "if it's an instant spell" intervening-if. Also fix a latent bug this exposed: spell_record_matches_filter dropped TargetFilter::Named to the catch-all false, so Not(Named{X}) over spell history was always true — silently no-opping any name self-exclusion (Alania's "other than ~"). Add the Named arm (record.name == name). Measured: zero existing cards fed a top-level Named filter into a spell-record position, so this is a pure fix (coverage-regression on fresh card-data: 0 regressed, +1 gained = Alania only; the >=2-disjunct guard protected Vengevine and the 108 NthSpellThisTurn constraint cards). CR 603.4: the intervening-if is checked at trigger time AND resolution — the count is read live, so a second matching spell cast in response correctly fizzles it. CR 601.2a: the anchor keys on the SpellCast event's spell object. CR 201.2: card name for the self-exclusion. Tests: 6 runtime cast-pipeline tests (first-instant fires + copies; second-instant same turn does not; first-sorcery fires; Otter self-exclusion by name; the CR 603.4 in-response fizzle; optional-draw decline skips the copy) each with live revert-to-red, plus a parser-shape guard and a Vengevine constraint-path regression anchor. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): Behold a [quality] as an interactive triggered-ability effect (CR 701.4a) Sarkhan, Dragon Ascendant's ETB "you may behold a Dragon. If you do, create a Treasure token." previously left an Unimplemented{behold} node — behold existed only as a casting cost (AbilityCost::Behold), never as an effect. The already- parsed Treasure sub (gated on OptionalEffectPerformed) and the second trigger (Dragon-enters → +1/+1 + becomes-Dragon-with-flying) are preserved untouched. Add Effect::Behold { filter }, an interactive resolution-time keyword action per CR 701.4a ("Reveal a [quality] card from your hand or choose a [quality] permanent you control on the battlefield"). The resolver (game/effects/behold.rs) reuses the single candidate authority eligible_behold_choices (battlefield-you- control ∪ matching hand) and a shared reveal_if_from_hand helper: - 0 candidates: whiff — set cost_payment_failed_flag, stash no continuation, so the "if you do" rider reads performed && !flag = false (no Treasure). - 1 candidate: forced, no agency — auto-select; a hand card emits CardsRevealed (card stays in hand), a permanent reveals nothing. - >=2 candidates: a genuine, rules-visible choice (CR 608.2d) — park a new WaitingFor::BeholdChoice for the controller; the submit handler validates the chosen object is in the candidate set, reveals-if-hand, sets the rider performed, and drains. A prior /review-engine-plan rejected an auto-resolve design: CR 701.4a grants the player the choice, and a hand reveal is a public event, so auto-picking among two or more distinct hand Dragons is an engine-made choice with rules-visible consequences — pillar #1 (rules-correct over convenient). Hidden-information correctness (CR 400.2): the BeholdChoice candidate list spans a hidden zone (hand), so visibility.rs redacts the pre-choice candidates to an opaque sentinel for opponents — otherwise the controller's matching hand cards would leak before they choose. The post-choice reveal exposes only the chosen card. Behold has no stack target (chosen at resolution, not declared): target_filter() is None, in the mass-effect group beside Clash/Populate. The frontend BeholdChoiceModal is display-only — it renders the engine-provided candidate list and dispatches a single-object SelectCards; it derives no eligibility. i18n reuses the existing cardChoice.behold.* keys (present in all seven locales from the cost-side UI) — zero new keys. #5051 (CR 400.7): this fixed-quality behold writes no ChosenAttribute, so the stale-chosen-type bug cannot manifest here; a code + test pin flags the future type_choice axis for the fix sweep. Tests: 7 runtime cast-pipeline tests (board-Dragon behold; hand-Dragon reveal stays in hand; >=2-hand-Dragon interactive prompt with only-chosen-revealed; opponent-view candidate redaction; accept-with-no-Dragon whiff; decline; parser shape) each with revert-to-red. Coverage-regression on fresh card-data: 0 regressed, +1 gained (Sarkhan). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): delayed sacrifice "at the end step on your next turn" (CR 513.2) Kav Landseeker's "When this creature enters, create a Lander token. At the beginning of the end step on your next turn, sacrifice that token." previously left the delayed clause an Unimplemented{at} node — the parser had no temporal arm for "the end step on your next turn", and the nearest existing condition (AtNextPhaseForPlayer, "your next end step") fires the CURRENT turn's end step (CR 513.2: a next-end-step delayed trigger created outside the end step is not backed up), which would sacrifice the Lander before it could be used. Add a turn-floor to AtNextPhaseForPlayer via a new typed enum TurnGate { None, AfterCreationTurn, After(u32) } (not a bool, not a magic sentinel): the parser emits the symbolic AfterCreationTurn, and effects::delayed_trigger::resolve stamps it to After(creation_turn) at creation (CR 603.7a) — the single-path binding site that already rewrites the PlayerId(0) controller placeholder. The matcher then skips every matching phase up to and including the floor turn and fires on the first strictly-later controller turn (CR 500.7 extra turns included). An unstamped AfterCreationTurn reaching the matcher debug_asserts and falls through to fire this turn (a loud, test-caught wrong-timing signal) rather than silently never firing. Existing AtNextPhaseForPlayer users (Greasefang, rebound, epic) default gate: None and are byte-identical — None is skip_serializing_if, so only Kav serializes a gate ("AfterCreationTurn", a pure semantic, no number). "That token" reuses the existing TargetFilter::LastCreated snapshot-at-creation (specific-token-safe per CR 603.7c / 400.7); the Lander predefined token and the parsed Lander creation are untouched. Tests: paired-timing runtime (Lander survives the current end step, is sacrificed at the controller's next end step) with a resolved-stack reach-guard, specific-token (a bystander token survives), a resolve-time stamp unit test, a parser-shape guard, and a Greasefang non-perturbation guard — each with revert-to-red. Coverage-regression on fresh card-data: 0 regressed, +1 gained (Kav). Singleton delayed-trigger test scope is deliberate (see #5072). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): Crowd-Control Warden dual enters/turned-face-up counter replacement + suppress own as-enters on face-down entry (CR 708.2a) Crowd-Control Warden ("As this creature enters or is turned face up, put X +1/+1 counters on it, where X is the number of other creatures you control. Disguise …") previously left the replacement line an Unimplemented node — the replacement dispatcher matched the "As ~ enters" pattern but no inner parser handled the dual "enters OR is turned face up" condition + the "put … on it" clause surface. PARSER: recognize the self-and-face-up counters replacement (a new leaf `lower_as_enters_or_face_up_counters`, wired at the Priority-8 replacement slot). It emits two ReplacementDefinitions — one `Moved` (enter-the-battlefield) and one `TurnFaceUp` — sharing one `PutCounter{P1P1, ObjectCount(other creatures you control), SelfRef}` execute (CR 613.2a: both conditions generate the same effect). The dynamic-count "on it" anaphor lowers to ParentTarget; the recognizer normalizes it to SelfRef (matching the directly-built enters-with-counters shape) so the runtime folds it as an ETB event modifier. A guard requires every execute effect to be PutCounter{SelfRef}, so sibling "As ~ enters, choose…/becomes…" lines fall through untouched. The dynamic count + turned-face-up runtime were already fully supported (extract_etb_counters, turn_face_up_applier) — parser-only for the card. Coverage-regression on fresh card-data: 0 regressed, +1 gained (Crowd-Control Warden), blast radius exactly 1 card. RUNTIME (the parser fix exposed a class bug): playing the Warden face-down via Disguise wrongly gained the +1/+1 counters. CR 708.2a: a permanent that enters face down is a 2/2 with no text, so its OWN "As ~ enters" replacement must not apply on a face-down entry. Add a guard in object_replacement_candidate_applies: `if is_entering && ZoneChange{face_down_profile: Some} return false` — is_entering is true only when the candidate's source IS the entering object (its own text), so EXTERNAL replacements (another permanent's enters-tapped) still apply. Masked until now: existing morph/disguise cards with an own "enters with N counters" use an X count (=0 face-down); the Warden's ObjectCount is the first nonzero one to expose it. CR 708.3: the permanent is turned face down before it enters. A pre-existing change_zone test (paused_face_down_change_zone_resumes_face_down_with_profile) forced its replacement pause via the entrant's OWN SelfRef shock replacement on a face-down entry — inadvertently asserting the masked bug. Rework it (test-only) to force the pause via EXTERNAL replacements (mirroring the morph sibling), preserving the load-bearing seam assertion (the PendingChangeZoneIteration carrier preserves the face-down profile through pause/resume). Tests: parser-shape (dual replacement, zero Unimplemented) + as-enters-choose reach-guard; runtime face-down NEG discriminator (0 counters, reds to 2 on guard revert) + hard-cast POS + face-down-then-turn-up; a no-over-suppression external test + Hooded Hydra masked-class regression guards — each labeled by its discriminating role. Full test-engine: 17263 passed. Assisted-by: ClaudeCode:claude-opus-4.8 * chore(engine): rebase adaptation — classify S25 tranche variants into the #5072 walker The #5072 PR-6.75 read/write conflict profiler (ability_rw.rs) is exhaustive over Effect / TriggerCondition / TargetFilter / ControllerRef with no wildcards. This tranche, written before #5072 merged, added new variants/fields the walker had no arms for; rebasing onto main surfaced 13 compile errors (+ a latent Effect::Behold masked by same-match E0027s). Classify each with its read/write profile and the three ordering axes (reads_member_bound / reads_event_live / writes_event_object), CR-grounded and mirroring the closest existing sibling: - TargetFilter::GrantingObject (CR 201.5a): mirrors SpecificObject on the base axes; reads_member_bound = true (fail-closed R3 divergence). Parse-template-only — grant-clone concretizes it to SpecificObject{granter} (ability_utils.rs), so the arm is inert at runtime and classified conservatively. - ControllerRef::TargetOpponent (CR 109.4): mirrors TargetPlayer (runtime-read- identical) — all axes empty (declared-target read, member-invariant). - TriggerCondition::TriggeringSpellMatchesFilter (CR 601.2a/603.4): mirrors TriggeringSpellTargetsFilter — reads_event_live. - Effect::Behold (CR 701.4a/608.2d): mirrors Reveal — a pure information event, no write; its resolution-time choice is classified in ability_scan (MayPrompt). - Effect::Cloak.object_source (CR 701.58a), Effect::Dig.keep_count_expr (CR 701.20e/608.2c), Effect::GainActivatedAbilitiesOfTarget.scope (CR 602.1): new fields bound + profiled like their sibling fields. TurnGate.gate (a match-time firing gate, not traversed by ability_rw) and Effect::ControlNextTurn's ControlWindow (absorbed by the maximal-conservative arm) need no binding. Also adapts one lib test to a main API delta: #5072-era main replaced Effect::DealDamage's former `excess_only: bool` with a typed `excess` field, so a DealDamage constructor in copy_spell.rs's tests gains `excess: None` to match. One tail adaptation commit rather than folding into the owning card commits: unlike #5072's own case, the walker lives in the rebase BASE here, so folding WOULD restore per-commit compiles — but a six-way fold across the tranche is error-prone; the arms are grouped here and can be autosquashed at ship-prep if the final PR wants per-commit bisectability. cargo check --workspace is clean (engine + phase-ai + mtgish-import + server-core + wasm/server + test targets), zero downstream dual-walk sites, and the full engine test suite passes (17549 tests, 0 failures) — no HIGH-2 / C2-ungating trip in the tranche's tests (Kav's delayed-trigger tests were already singleton-scoped). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): Rhys, the Evermore — interactive "remove any number of counters" Implements Rhys's "{W},{T}: Remove any number of counters from target creature you control" as a resolution-time interactive choice (CR 107.1c: "any number" includes zero; CR 608.2d: the controller chooses at resolution). Parser: a new "any number of" arm lowers the count to QuantityExpr::UpTo (Fixed{-1}), reusing the existing UpTo slot — no new variant, so the #5072 read/write dual-walk needs no arms. A CR 608.2d "from among" guard keeps out-of-scope multi-source removals (Galloping Lizrog, Eventide's Shadow) as Unimplemented rather than collapsing them to single-source semantics. Effect path: resolve_remove peels the UpTo flag and raises WaitingFor::RemoveCountersChoice carrying the target's live per-type counter counts; the controller's GameAction::ChooseCountersToRemove is validated by a validate_counter_selection helper shared with the cost path and applied through the CR 614.1 remove_counter_with_replacement pipeline via a pending_counter_removals queue. The queue re-parks on replacement choices and, on drain, stamps last_effect_count before the continuation drains, so "create that many" reflexive clauses (Tetravus) read the true removed total (CR 608.2h). Multiplayer: accepts_freeform_counter_removal + a session skip-legality arm let a human submit an intermediate per-type selection the coarse AI candidate set does not enumerate; the payload guard bounds the selection list. Coverage: GAINED = exactly {Rhys}. Tetravus's remove-counters trigger and token creation now resolve (proven: 3 counters -> 3 Tetravite tokens), but the card stays unsupported on an unrelated keyword-grant clause nested in that trigger. Cost-path storage lands / batteries and the multi-source cards are unchanged. The plan reviewer's re-review request was folded into /review-impl with a mandate to re-verify all six must-change resolutions at code level plus the four non-compile-enforced registration points; verdict APPROVE. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): Esper Terra — "up to N" lore, conjunctive mana, token-anaphor counter binding Fixes the two parser-leaf gaps on Esper Terra (Terra, Magical Adept // Esper Terra) plus the shared binding that makes its marquee chapter effect land on the right object. No new engine variant (reuses QuantityExpr::UpTo, PutCounter, ManaProduction::Fixed, TargetFilter::LastCreated) — the #5072 read/write dual-walk needs no arms. Changes (all parser): - Gap A (counter.rs): "put up to three lore counters on it" — strip the "up to " count marker and wrap the count in QuantityExpr::up_to (CR 608.2d + CR 122.1), mirroring the Draw/Discard/PutSticker convention. Count-side only; the target-side "on up to N target(s)" path is untouched. - Gap B (mana.rs): "Add {W}{W}, {U}{U}, {B}{B}, {R}{R}, and {G}{G}" — a conjunctive comma+"and" fixed-mana-group accumulator (parse_fixed_mana_group_list) → ManaProduction::Fixed (CR 106.4). Rejects "or" lists, requires >=2 groups, no dedup. - §B2 token-anaphor bind (context.rs + mod.rs + counter.rs): a bare "put ... counters on it" following a token creator binds to the created token, not the ability source (CR 608.2c). A ParseContext.token_created_in_chain signal (set from the most-recent prior referent, cleared by an intervening typed target) drives the it-pronoun counter branch to TargetFilter::LastCreated. A companion LastCreated guard on replace_target_with_parent's counter arm (mod.rs) preserves that bind through the lowering pass — the guard its sibling Attach/UnattachAll arms already carried. The "if it's a Saga" gate needs NO code: it parses to TargetMatchesFilter{Saga} reading ability.targets.first() (the copied enchantment, CR 707.2 type-equal to the token), so a non-Saga copy places zero lore counters by construction. Coverage (full-DB regression): REGRESSED(engine)=0. GAINED=1 (Esper Terra). Eight counter-target fingerprint changes, all correct: - 4 anaphor target corrections (SelfRef→LastCreated): Esper Terra (x3 chapters, the +1 GAINED); applied geometry; the bus runner (first put; card stays unsupported on a separate gap); journey to the lost city. - 4 Gap-A parser wins (target unchanged SelfRef, a new "up to X" node parses): clockwork avian/beast/steed/swarm (stay unsupported on the counter-cap gap). - 9 genuine-source cards ("...on this creature/enchantment/<name>") byte-identical. Measured blast radius (replaces the plan's 6/7 prediction): 3 anaphor cards corrected (applied geometry, the bus runner, journey to the lost city) / 5 pre-existing-unchanged / 9 genuine-source preserved. The 5 unchanged cards (alien invasion, intrude on the mind, lasting fayth, match the odds, kianne) put counters via a dynamic "for each"-count PutCounter on a separate parser path that never reaches the it-branch — they stay SelfRef (misbound to source), byte-identical to baseline. That path is a pre-existing, LIVE rules-wrong gap this change does not touch; deferred and logged (.planning/coverage-analysis/S25-DEFERRALS.md D1). Ship-prep note: journey to the lost city appears in #5072's R3 member-bound classification; its PutCounter target change alters its ability AST, so its reads_member_bound rw-profile — and thus se_member_bound_class — may move +/-1 at the next full-DB sweep. Attributed to this commit (expected movement, not a regression). Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): Foraging Wickermaw — "this creature becomes that color" Closes the last gap on Foraging Wickermaw: "{1}: Add one mana of any color. This creature becomes that color until end of turn." reuses the existing chosen-color carrier — no new engine variant (empty types/ diff). - Parser (oracle_effect/subject.rs): fold "that color" into the existing AddChosenColor arm of try_parse_become_color_modification, beside "the chosen color". "That color" is the color of the mana produced this activation — a resolution-time anaphor, not a value baked at parse (CR 106.1a/202.2/105.3). - Runtime (game/mana_abilities.rs): record the produced color as ChosenAttribute::Color on the source in produce_mana_from_ability, so the Layer-5 AddChosenColor reader (already powering Puca's Eye) sets the creature's color (CR 613.1e). Reuses the existing mana_sources::mana_type_to_color (Colorless→None) — no new converter, no types/mana.rs edit. Two safety properties, both proven by revert-to-red: - Gated write, zero blast radius. produce_mana_from_ability is the universal mana chokepoint, so the write fires only when the resolving ability's own chain carries a downstream AddChosenColor (via visit_links_any on the activated ability_def, fresh per activation). Coverage-regression confirms exactly one card flips (Foraging Wickermaw) and every ordinary producer — Birds of Paradise, City of Brass, painland, filter land, basic, rock — is byte-identical. - Explicit replace, not accumulate (CR 400.7). The stored ChosenAttribute::Color PERSISTS across turns (cleared only on zone change), UEOT expiry removes the Layer-5 effect but NOT the stored attribute, and the choice binder accumulates rather than replaces — so a plain push would leave a stale prior-turn color first in the list (chosen_color() is first-match). The write retain-drops the prior Color before pushing, so re-activating on a later turn with a new color replaces cleanly. The Colorless→White fallback converter (mana_payment.rs) is left untouched: it is load-bearing-defensive for the three Phyrexian-shard sites, where CR 107.4a makes Colorless unreachable. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): The Tomb of Aclazotz — cast-from-graveyard type-grant rider Closes the last gap on The Tomb of Aclazotz's "{T}: You may cast a creature spell from your graveyard this turn. If you do, it enters with a finality counter on it and is a Vampire in addition to its other types." The finality counter half was already wired via the ExileWithAltCost permission channel; this adds the parallel channel for the "is a Vampire in addition to its other types" type grant. - New Effect::AddPendingEntersModifications { modifications: Vec<ContinuousModification> } — the general "enters-with continuous modifications" carrier (CR 613 layer system), a categorical sibling of AddPendingETBCounters (CR 122 physical counters). The Vec<ContinuousModification> shape absorbs every future enters-with rider (added types, granted abilities, colors), so no third sibling is needed. The two channels stay separate because a counter is not a continuous modification. - Parser (oracle_effect/mod.rs): try_parse_cast_this_way_enters_rider lifts the former trailing-tail rejection (CR 205.1b) to accept "… and is a <type> in addition to its other types", parsing the tail via animation::parse_becomes_type_modifications (additive AddType/AddSubtype/ AddSupertype — retains printed types, CR 205.1b) and emitting the new effect as the counter rider's sub-ability. - Permission channel (mirrors enters_with_counter exactly): a new ExileWithAltCost.enters_with_modifications field (serde default + skip-if-empty), lifted from the rider at grant time and read back from the SELECTED permission at cast-finalize (CR 611.2c — no sibling-permission leak), applied to the cast object as a Duration::Permanent continuous effect (Layer 4, CR 613.1d). - #5072 read/write dual-walk: AddPendingEntersModifications is classified as a self-scoped SetMembership write (mirroring Effect::Animate/Endure's type-line writes) — NOT an ObjectCounters write; …
keloide
pushed a commit
that referenced
this pull request
Jul 19, 2026
…locked/combat source restriction on damage redirection (phase-rs#5518) * fix(engine): dropped tap-gate + unblocked/combat source restriction on damage redirection (Veteran Bodyguard, Weathered Bodyguards) Veteran Bodyguard ("As long as this creature is untapped, all damage that would be dealt to you by unblocked creatures is dealt to this creature instead.") and Weathered Bodyguards (same shape, scoped to combat damage) parsed to an unconditional, unrestricted redirection — dropping the leading "as long as untapped" gate (CR 604.2), the "by unblocked creatures" source filter (CR 509.1h), and the combat-only damage scope. parse_damage_redirection_replacement now parses all three, reusing existing types verbatim: ReplacementCondition::SourceTappedState, FilterProp::Unblocked (via parse_damage_source_subject_filter falling through to parse_type_phrase's existing unblocked-combat-status recognition), and CombatDamageScope::CombatOnly (via scan_combat_scope). Renamed strip_as_long_as_prefix_for_prevention to strip_as_long_as_condition_prefix since it is now shared between the prevention and redirection parsers. Palisade Giant is deliberately excluded — its real Oracle text has no "unblocked" restriction — and has its own regression-guard test proving the exclusion is correct. Discovered but out of scope: game/replacement.rs's damage_done_applier only reads redirect_target inside the ShieldKind::Redirection branch (CR 614.9); this whole card class parses to ShieldKind::Prevention with a separate redirect_target field that branch never reads, so the "instead" creature never actually takes the redirected damage today. Filed as a follow-up (shared runtime code, wide blast radius); the new tests document this and assert life-total delta rather than damage_marked as the discriminator. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs: remove Veteran Bodyguard, Weathered Bodyguards from parser-misparse-backlog Fixed by the preceding commit. Root cause #1 (relative-clause / filter restriction on target dropped): 750 -> 748 cards; totals rebased to 4761 distinct / 4795 total appearances. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
keloide
pushed a commit
that referenced
this pull request
Jul 19, 2026
…ng block already handles it (test-only) (phase-rs#5547) * test(engine): prove Land's Edge already parses and resolves correctly Land's Edge ("Discard a card: If the discarded card was a land card, this enchantment deals 2 damage to target player or planeswalker. Any player may activate this ability.") is listed in the parser-misparse backlog's root cause #1 (dropped relative-clause/filter restriction). Investigation found this is a stale backlog entry, not a live bug: the existing AbilityCondition::CostPaidObjectMatchesFilter building block (added 2026-05-04, previously only exercised in ConditionInstead-wrapped form by Agency Coroner/Surtland Flinger/Stormscale Anarch/Grab the Prize) already correctly handles this card's bare (non-instead) composition -- condition extraction, chunk-boundary protection for the leading "if", runtime evaluation via the cost-paid discard's LKI snapshot, and a separate PlayerFilter::All "any player may activate" mechanism. Zero production code required. Adds 2 parser unit tests locking the full parse shape (CR 602.1 + 602.1a + 602.2 + 118.1 + 608.2c + 608.2k + 400.7j) and 5 GameRunner integration tests proving the runtime behavior: discarding a land deals 2 damage, discarding a nonland deals 0 (ability still resolves), the discard snapshot binds the specifically-chosen object (not any land in hand), a non-controller can activate and pays from their own hand (CR 602.1a), and a sibling without the "any player" clause rejects non-controller activation (CR 117.3d priority-gate correctly enforced). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs: remove Land's Edge from parser-misparse-backlog Confirmed fixed (test-only, no production change) by the preceding commit. Root cause #2 (dropped intervening-if): 606 -> 605 cards; totals rebased to 4760 distinct / 4794 total appearances. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * test(engine): reduce Land's Edge PR to a single discriminating parser test Addresses matthewevans' review on phase-rs#5547: the CostPaidObjectMatchesFilter building block this PR exercises already has runtime coverage via four sibling integration tests (Agency Coroner, Surtland Flinger, Stormscale Anarch, Grab the Prize). Adding a fifth card's standalone 427-line integration test file for the same already-covered condition was duplicative coverage with real suite-runtime/maintenance cost and near-zero marginal risk reduction. The one genuinely new thing Land's Edge's shape exercises is a parser-level distinction: a BARE (non-instead) composition of CostPaidObjectMatchesFilter, versus the ConditionInstead-wrapped form all four existing sibling cards use. At runtime this is not actually a new code path -- evaluate_condition's CostPaidObjectMatchesFilter arm already fires for any ability.condition, ConditionInstead or not -- so the distinction is provable at the parser level alone. Removed: - crates/engine/tests/integration/lands_edge_discard_land_condition.rs (427 lines, 5 tests) and its main.rs mod line - lands_edge_without_any_player_clause_has_no_activator_filter (a second parser test exercising the separately-established PlayerFilter::All mechanism, not the bare-condition distinction) Kept: lands_edge_discard_land_condition_parses_as_bare_intervening_if, a single parser unit test that locks the full bare parse shape (cost, condition explicitly NOT ConditionInstead, effect, activator_filter) in one assertion block -- the "single discriminating assertion" the review asked for. Verification note: could not get a fresh cargo test run to complete locally after this reduction -- 5 consecutive attempts were killed mid-compile (a background-build contention issue this fork's ~10 concurrent worktrees have hit repeatedly; the shared WORKLIST.md cargo-lock was held by another agent throughout). This change is a pure deletion (no logic modified) of an already-green test suite; the one retained test is byte-identical to its previously-verified-passing form. CI will provide the authoritative signal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
keloide
pushed a commit
that referenced
this pull request
Jul 19, 2026
…r filter (Herald of Kozilek, Ugin, Urza's Filter) (phase-rs#6150) * fix(parser): scope bare color-category & historic spell-cost modifiers to their filter (Herald of Kozilek, Ugin, Urza's Filter) "Colorless spells you cast cost {N} less to cast" (Herald of Kozilek, Ugin, the Ineffable, It That Heralds the End), "Multicolored spells cost {N} less to cast" (Urza's Filter), and "Historic spells you cast cost {N} less" (Jhoira's Familiar) all parsed with `spell_filter: None` — the color-category / historic restriction was silently dropped, so the modifier (mis)applied to EVERY spell instead of only the named category. Root cause: the bare-word fallback in `parse_cost_mod_spell_type_prefix` (static_helpers.rs) hand-rolled only the five NAMED colors via `parse_named_color`. A bare "colorless"/"monocolored"/"multicolored" matched neither that nor `parse_bare_supertype_spell_filter`, so the whole filter returned `None` (the noun-bearing path — "Colorless CREATURE spells" — already produced the correct `ColorCount` via `parse_type_phrase`). Fix: route the bare-word subject through a single `parse_bare_spell_subject_filter` authority that resolves the color word via the existing `nom_filter::parse_color_property` combinator — so the color-CATEGORY axis resolves identically to the noun-bearing path (colorless → ColorCount{EQ,0}, monocolored → {EQ,1}, multicolored → {GE,2}, named color → HasColor) — plus "historic" (FilterProp::Historic) and the pre-existing bare-supertype path. This replaces the partial `parse_named_color` special-case with the complete color-property authority; it composes with the trailing mana-value qualifier (It That Heralds the End → ColorCount + Cmc). Parse-only: FilterProp::ColorCount and Historic already exist and are evaluated by the spell-cost filter path. CR 105.2 (object colors; colorless = zero colors) / CR 700.6 (historic) / CR 205.4a (supertypes) / CR 601.2f (cost determination). Tests: parser unit test over the full bare-subject axis (colorless/mono/multi/ historic + named-color & supertype regression + MV composition); runtime cast-cost differential driving parse -> cast -> cost determination — a colorless spell gets the {1} discount and a colored spell does NOT (revert-failing: before the fix the filter was None and every spell was discounted). Backlog: removed It That Heralds the End and Urza's Filter from root cause #1 (both now parse fully clean); decremented the associated counts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(PR-6150): address parser review comments --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
keloide
pushed a commit
that referenced
this pull request
Aug 8, 2026
…ment fan use it (phase-rs#6992) * fix(client): one authority for object activation; adopt it at the six deciding sites Four call sites each re-derived "may I activate this object, and what does a click do" from a different premise, so they disagreed. The attachments dialog and the command-zone emblem chip gated on a raw `<x>Actions.length > 0` bucket check with NEITHER a `WaitingFor` gate NOR a seat gate: an opponent's emblem chip was clickable from this viewer's seat, and the attachments dialog stayed interactive at DeclareBlockers. `viewmodel/cardActionChoice.ts` gains the two authorities: - `deriveActivationAffordances(waitingFor, canAct, legalActionsByObject, objects)` returns the `activatableObjectIds` / `manaTappableObjectIds` pair, applying the timing and seat gates once. - `resolveObjectActivation(actions, object, canTapForMana)` returns a typed `ObjectActivation` verdict — `none` / `dispatch` / `choose` — and owns the CR 605.1a mana/non-mana partition and the phase-rs#506 confirmation gate, so no call site inspects an action list again. `GameBoard` now derives the pair from that authority instead of an inline copy and publishes it unchanged on `BoardInteractionContext`. `PermanentCard`, `DialogAttachmentCard` and `CommandZone` consume it; `LibraryPile` is refactor-only. Tests. The authority itself is covered at the unit level (V14/V15/V18/V18b/ V19/V19b-V19e/V20c2/V20d). `GameBoardInteractionWiring.test.tsx` renders the real `<GameBoard/>` and reads both sets back out of the real provider from a child consumer, asserting CONTENTS rather than sizes — a parity test alone could not have caught an unwired provider. The two newly gated call sites get their own timing and seat arms. `BattlefieldZoneOverflow` is the second consumer of the affordance pair and its `activatableObjectIds` read was previously silent: deleting it left all 130 board tests green, so a row that turns it red ships with it. Assisted-by: ClaudeCode:claude-opus-5 * fix(client): make the attachment fan and the host click reach that authority THE BUG: with Kilo, Apogee Mind (401) enchanted by Freed from the Real (408), the engine published `legalActionsByObject["408"] = [#0, #1]` and the fan was inert — no ring, and clicking Freed did nothing. The fan had exactly one source, an open interaction's projection, so a populated action bucket with no prompt open reached nothing at all. Freed's own `{U}: Untap` was unreachable. `AttachmentFan` gains a second mode. Mode 1 (an engine interaction owns the prompt) is byte-unchanged and still returns early, so exactly one authority can win. Mode 2 runs only when no prompt is open: the ring and the click both come from the shared authority the battlefield already uses, so the fan can never offer what the board would not. The fan is closed BEFORE the chooser opens — it is a `fixed inset-0 z-[120]` backdrop with an `onClick` catcher and `DialogHost` anchors at z-40, so a fan left mounted would paint over the modal it just opened and swallow its clicks. The host card is the fan's anchor and is never one of its picks, even when the engine publishes actions for the host too. `PermanentCard` gains the matching host click (hunk B). An attached Aura/Equipment/Fortification is its own object (CR 301.5 / CR 303.4), but its only in-place affordance is a ~22px peek rendered BELOW the host, under the 44px touch-target floor. When the host itself offers nothing and an attachment does, the click falls through to the full-card chooser. The branch is placed LAST so it can never pre-empt the host's own target / activation / undo intent, and reads the same affordance sets as the host's own ring rather than the raw bucket. Its selection is UNCONDITIONAL, deliberately unlike the plain-click fallback which toggles: a toggle would strand the fan open over a host that just lost its ring and its attachment expansion. Tests. `AttachmentFan.test.tsx` covers mode 2 including a multi-authority hostile fixture where a prompt and a bucket are live at once. `abilityChoiceConsumerWiring.test.tsx` drives the producer and observes the real consumer — `DialogHost`'s overlay — so the store hand-off is measured rather than assumed; both its assertions are `expect.soft` so neither can pre-empt the other, and its third arm exists because the first two are blind to an always-true `selectable`. `PermanentCard.test.tsx` pins the branch ORDER with the actionable attachment held fixed across every arm. Assisted-by: ClaudeCode:claude-opus-5 * fix(client): give the activation authority its whole gate, its own id, and an exhaustive verdict Review round on phase-rs#6992. Five fixes, all frontend, all routed through the same two authorities the PR introduced — no new decision site. 1. Exhaustive `switch` + `never` at all FOUR `resolveObjectActivation` consumers (AttachmentFan, PermanentCard, DialogAttachmentCard, CommandZone). Only AttachmentFan's bare `else` errored on a new union variant; the other three compiled clean and silently dropped it. Behaviour is unchanged — `kind: "none"` still does nothing (it is reachable only through the render->click staleness window), and the fan's `close()` stays inside the two acting arms. CLAUDE.md: exhaustive match, no fallback default. 2. `resolveObjectActivation` took only the mana bit of a two-bit gate, so the non-mana partition was merged unconditionally and a cost-payment prompt offered activations the board itself refuses. It now takes the whole `ActivationAffordances` plus the object id and drops the non-mana partition when the activation ring is closed (CR 113.3b), mirroring the existing mana drop. Taking the pair rather than two loose booleans makes "half the gate" unrepresentable at a call site. 3. Sibling sweep: `CommanderCardZone` still gated `canCast`/`canNinjutsu` on the raw bucket with neither a `WaitingFor` nor a seat gate — the exact predicate this PR replaced at `CommandZone.EmblemCard`, in a component `PlayerArea` renders for every seat. Pre-existing, not introduced. 4. Grouped-emblem activation identity (maintainer review, [MED]; CodeRabbit comment 3). `GroupedEmblem` kept a representative plus a tally, so an action the engine published against a non-representative member was unreachable. It now retains every member; the chip acts on the member the shared affordance sets name, and the count badge is that list's length. This changes which id the authority is asked about, not who decides. 5. A `PermanentCard` fixture seeded a mana-only affordance pair while omitting `is_mana_ability` on the abilities — a state the engine cannot emit, since the deriver and the resolver classify through the same `isManaObjectAction`. Fix 2 surfaced it; the flags make the row exercise the mana partition. Two-sided controls, each flipping its own named assertion: the union-variant arms (1 error unfixed / 4 errors fixed / 0 with the real union); `if (true)` and `if (false)` on the activation ring; raw-bucket-restore and constant-false on the commander flags; representative-only and always-last-member on the emblem group. Assisted-by: ClaudeCode:claude-opus-5
keloide
pushed a commit
that referenced
this pull request
Aug 8, 2026
* fix(parser): compose the cast-type gate axes, and enforce it at runtime (Epic Experiment) Closes phase-rs#6960. `parse_cast_type_disjunction` handled exactly one shape: `" or "` between two bare core types, with an optional article and a required trailing "spell(s)"/ "card(s)". Everything else in the "cast from among them" class kept a bare `TargetFilter::ExiledBySource` with no card-type leg, so any card type could be cast from the exiled set. Rewritten as a per-axis composed grammar: opt(quantifier) opt(article) leg (sep leg)* head_noun reusing `oracle_nom::target::parse_type_filter_word` for the leg alphabet (core types AND subtypes) and mirroring `oracle_nom/enchant.rs` for the separator/list pair. `and`, `or`, `and/or` and serial commas all lower to `TypeFilter::AnyOf`: per CR 205.2b conjunction is a property of ADJACENT type words ("artifact creature"), while a connector enumerates alternatives. A literal `And` would be a total no-op — no card carries both Instant and Sorcery. Seven cards gain the gate they were missing: Epic Experiment, Ral Leyline Prodigy, Kylox "instant and/or sorcery spells" Collected Conjuring "up to two sorcery spells" Sanwell, Avenger Ace "a Vehicle or artifact creature spell" Wand of Wonder "up to X instant and/or sorcery spells" Scarlet Witch, Chaotic Avenger "a Hero or noncreature spell" Acceptance requires either two or more legs or a consumed quantifier, which is also the anti-swallow guard: "cast a spell from among them" (Aetherworks Marvel, Svella, Apex of Power) still yields no gate. The parser half alone was inert. The chain seam forwards every exiled card as the sub-ability's targets, so `target_ids` arrived non-empty and skipped the one site where the cast filter was applied — every exiled card got the permission regardless of type. `cast_from_zone` now retains only forwarded ids matching the clause's own legs, using a new `TargetFilter::without_exile_anaphor()` that discharges the `ExiledBySource` leg the seam already satisfied while preserving And/Or structure. Re-evaluating the anaphor here would be actively wrong: on a triggered ability it reads a snapshot captured before this ability's own exile step, which would drop every id and turn the bug into a total no-op. Scoped to filters that reference the exile anaphor, so explicitly targeted grants (Emry, Bring to Light, Urza) are untouched, and the 51 bare-anaphor rows residualize to None and keep the full forwarded set. Removes Scarlet Witch, Chaotic Avenger from parser-misparse-backlog root cause #6. Epic Experiment stays under #1: its "that weren't cast" cleanup clause is still dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(PR-6996): preserve hand cast gate predicates * test(PR-6996): distinguish hand cast type gate * fix(PR-6996): complete hand gate regression loop * test(PR-6996): select hand binding in rich gate regression --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
keloide
pushed a commit
that referenced
this pull request
Aug 25, 2026
…phase-rs#7703) * docs: custom format engine design proposal (discussion phase-rs#5312) Research + design for a general, data-driven custom-format layer, validated by expressing four Eternal Central retro formats (Old School 93-94, Old School 95, Middle School, Classic Magic) as data on top of it rather than four hardcoded GameFormat variants. Schema splits CustomFormatRules into two independent axes: StructuralRules (life, players, deck size, range of influence, team-based, singleton — already FormatConfig fields, already partially host-adjustable in the lobby) and LegalityRules (legal sets, banned/restricted, legacy rules like mana burn / damage-on-stack / pre-M10 Wish / legend-rule scope). Delivery recommendation is a "save as custom format" action on the existing lobby first (Axis A), with the four EC formats shipping as audited presets on the same schema (Axis B) in parallel. No engine or frontend code — design/research docs only, opened for maintainer review. * docs(custom-format-engine): narrow MVP to Swedish Old School + Axis A only Full LegacyRuleSet engine wiring (mana burn, damage-on-the-stack, pre-M10 Wish, legend-rule scope) is real risk and makes the MVP harder to test. Swedish Old School 93/94 — a distinct, real ruleset verified this session against oldschool-mtg.blogspot.com/p/banrestriction.html — has its own restricted list (23 names, different from EC's), an empty banned list, and no mention of mana burn or any other legacy rule, so it needs none of that wiring. Premodern already exists as a native GameFormat and needs no new work at all; it's cited only as an existing precedent for the same shape. Re-sequences to two phases: phase 1 ships the general engine + the Axis A lobby-save action + swedish_old_school() as the only new Axis B preset (zero LegacyRuleSet wiring exercised); phase 2 ships the four EC formats plus the legacy-rules engine work they actually need. Nothing is cut — the four EC formats and full legacy-rules axis remain the target, they just move to a phase that ships once the schema is already proven end-to-end by something smaller. Flags two new open items: ante-card handling (a third list-shaped rule, distinct from banned/restricted, with no schema slot yet) and Swedish Old School's reprint policy (unconfirmed against the primary source). * docs(custom-format-engine): address matthewevans's CHANGES_REQUESTED review Fixes all five design-correctness gaps from the round-2 review: 1. legal_sets was a bare Vec<SetCode>, so Axis A's default (no set restriction) evaluated as "restricted to nothing" and rejected every card. Changed to Option<Vec<SetCode>> (None = unrestricted). 2. StructuralRules dropped command_zone, commander_damage_threshold, and archenemy_player, and had no source for sideboard_policy (which turns out to be a GameFormat method, not a FormatConfig field, so Custom has no derivation path for it). Added all four; uses_commander is now derived from commander_damage_threshold rather than stored redundantly; supplies_fixed_deck stays false for Custom; allow_debug_actions is correctly excluded (orthogonal to format per its own doc comment). 3. No identity/persistence/transport contract existed for a lobby-saved format. Resolved by separating two conflated concerns: in-game peer agreement (already solved -- FormatConfig.custom_rules carries the full payload, not a lookup key) from a player's reusable saved-format library (client-side-only, never an engine/WASM type). Flags the real version-skew risk (an old client can't be rescued by serde(default) on an enum variant it doesn't know) as a lobby-join-handshake requirement. 4. Mana burn was modeled as damage at every engine Phase-enum transition. Verified against docs/MagicCompRules.txt:8278: it's life loss, not damage. Verified against the engine's own Phase enum (types/phase.rs): it flattens MTG's steps and phases into one flat list, so gating on every transition fires mid-phase (e.g. DeclareAttackers -> DeclareBlockers), not just at real phase boundaries. Also found an existing generic mechanism this can reuse: player_unspent_mana_loss_causes_life_loss / apply_empty_mana_pool_event, currently used for a Yurlok-class card-granted ability at full CR 500.5 granularity. Redesigned as a phase-group-boundary-gated second contribution to the same event, independent of the Yurlok-class check. 5. Swedish Old School's restricted list was mislabeled 23 when 25 names are enumerated, and the preset sketch had dropped Summer Magic from the legal-sets list. Classic Magic's restricted list was labeled 37 when 44 are enumerated. All four fixed and cross-checked between CONTEXT.md, PLAN.md, and RESEARCH.md. * docs(custom-format-engine): address round-2 CHANGES_REQUESTED (matthewevans) Round 2's fixes were themselves incomplete or wrong on all five points matthewevans re-flagged. Each re-verified directly against the cited engine source before treating the review as correct, not accepted at face value: 1. Round 2 only gated the mana-burn LIFE-LOSS check to phase-group boundaries, leaving the pool-emptying event firing unconditionally on every Phase transition -- so by the time a boundary was reached, the pool was already silently drained with nothing left to burn. Fixed by reusing an existing mechanism instead of gating a side-effect on an event that already ran: the engine's ManaExpiry type already has EndOfCombat ("persists through combat steps, drains at EndCombat -> PostCombatMain", used by Firebending) -- generalized with a third variant, EndOfPhaseGroup, so mana_burn-tagged mana actually persists across intra-phase-group steps and only drops (and burns) at a real phase-group crossing. 2. StructuralRules.sideboard_policy had no accessor -- GameFormat:: sideboard_policy() can't see FormatConfig.custom_rules at all. Added FormatConfig::sideboard_policy() as the single canonical accessor and specified migrating both production call sites (deck_loading.rs, match_flow.rs) to it. 3. archenemy_player is per-game seat identity validated against that game's player count (FormatConfig::archenemy_player/validate_for_player_count), not a reusable structural setting -- removed from StructuralRules entirely; Axis A doesn't support the Archenemy topology. 4. uses_commander was derived from commander_damage_threshold alone; GameFormat::uses_commander()'s own doc comment states the real invariant requires command_zone AND the threshold. Fixed the derivation to use both conditions. 5. ReprintPolicy is declared but never consumed by the evaluator, and LegendRuleScope::PreM14AnyController's historical conflation was never resolved. Added a general preset-readiness rule: a preset can't be registered as selectable until every legality/legacy field it declares is both specified and actually enforced -- this currently blocks swedish_old_school() specifically. Also fixed two more lingering 23-vs-25 restricted-list references round 2 missed. * docs(custom-format-engine): full audit of all 4 review rounds, address round 3 Per direct instruction, re-audited every point raised across all four review rounds against current source (not against prior claims) rather than only fixing the newest round. Found one additional real gap nobody had named yet: deck_validation.rs's DeckCompatibilityRequest.selected_format has the same bare-GameFormat problem as companion.rs, just never flagged by anyone. 1. Round 3's sideboard_policy fix used FormatConfig::sideboard_policy() as a method with .expect("Custom format must carry custom_rules") -- a production panic path -- and migrated only 2 of 7 real consumer call sites (companion.rs x4, deck_loading.rs x2, match_flow.rs x2, deck_validation.rs x5). Fixed: fallible validation of the format/custom_rules invariant at every construction/ingestion point (malformed values rejected at the boundary, never constructed); sideboard_policy becomes a stored FormatConfig field matching the existing uses_commander/supplies_fixed_deck pattern (verified via their own consistency test at format.rs:1512-1513), not a new method; every real consumer migrates to read it, with signatures widened wherever they only carry a bare GameFormat today (companion_offers, DeckCompatibilityRequest). 2. from_lobby_config never specified where sideboard_policy comes from. Fixed: config.format.sideboard_policy(), valid because the conversion's input is always a built-in format at save time. 3. LegacyRuleSet's three bools (mana_burn, damage_uses_stack, pre_m10_wish_reaches_exile) become typed enums (ManaBurnPolicy, CombatDamageTiming, WishOutsideGameScope), matching LegendRuleScope's existing shape. Tightened the preset-readiness gate: no preset may ship in a "playable with a caveat" state -- retracts this doc's own earlier claim that Middle School/Classic Magic could ship before damage-on-stack lands. 4. Designed the version-skew compatibility fix concretely instead of flagging it: reuse the engine's existing PROTOCOL_VERSION/ MIN_SUPPORTED_PROTOCOL handshake gate (server-core/protocol.rs) rather than inventing new negotiation. For ReprintPolicy enforcement, named two sufficient resolution paths (the general printing cross-reference, or a one-preset verification pass) rather than building the full model now. Also fixed two stale pre_m10_wish_templating references in RESEARCH.md that survived since round 1 despite PLAN.md already using the canonical name throughout. * docs(custom-format-engine): inline source citations per format, not just once Every card list/count in RESEARCH.md shared one citation at the top of section 1, and Swedish Old School -- the actual phase-1 target preset -- had no RESEARCH.md presence at all, only scattered mentions in CONTEXT.md. That makes independent verification harder than it should be, especially given every "preset data inconsistency" finding across four review rounds was an internal cross-reference mismatch, not an external-source check -- worth making the external source trivially reachable at the point of data. - Added a "Source:" line to each of the four existing EC format subsections. - Added a full "Swedish Old School 93/94" subsection to RESEARCH.md #1, matching the EC formats' treatment: direct source URL, its own verbatim legal-sets/banned/restricted/ante/legacy-rules data, and an explicit side-by-side comparison against EC's 93-94 restricted list proving these are two different, real rulesets, not one re-presented as two. - Added the same two source URLs inline at the top of PLAN.md #2, so the preset constructors are checkable without cross-referencing another file. * docs(custom-format-engine): address CodeRabbit findings on the round-4 commit Automated review on the round-4 fix commit (3aa20f1) caught 5 things, at least 3 of which are real: 1. A genuine mistake in round 4's own mechanical rename fix: the search_outside_game pseudocode was updated from the round-1 placeholder name to pre_m10_wish_reaches_exile, but the SAME round-4 commit also converted that field to the typed wish_scope: WishOutsideGameScope enum -- the pseudocode never got updated to match, in the same commit that introduced the mismatch. Fixed, with the naming-history note now tracking both renames. 2. sideboard_policy/uses_commander are plain serialized FormatConfig fields (no #[serde(skip)]) that could diverge from what's derivable from `format`/`custom_rules` on a malformed wire payload -- a real gap that already exists for built-in formats today, not just custom ones. Widened validate_custom_rules_consistency to check derived-field agreement for every format, not only Custom. 3. DeckCompatibilityRequest.selected_format specifically needs the full CustomFormatRules (legal_sets/banned/restricted), not the lighter ResolvedFormatFacts struct that's sufficient for companion.rs's two call sites -- tightened from an ambiguous "per-site judgment call" to an explicit distinction. 4. The protocol-version fix only bumped PROTOCOL_VERSION; format selection happens during lobby setup, so LOBBY_PROTOCOL_VERSION / MIN_SUPPORTED_LOBBY_PROTOCOL (a separate, real constant pair, confirmed this session) needs bumping too. 5. CombatDamageTiming::OnStack's doc comment mischaracterized historical combat damage as a triggered ability; RESEARCH.md #6 already correctly describes it as assigned damage placed on the stack as a stack object. Fixed to match. Also strengthened the preset-readiness gate from a documented convention into an actual technical mechanism (custom_format_registry() validates against a static implemented-axes table before returning a preset) -- responds to a stricter CodeRabbit read of the ReprintPolicy gate without fully adopting its more extreme "remove the field" suggestion, which goes beyond what the human reviewer (matthewevans) actually asked for. * docs(custom-format-engine): address round-5 CHANGES_REQUESTED (matthewevans) Round 5 opened with "the proposal now resolves the previously-requested custom-context, typed-policy, compatibility, and no-caveated-preset concerns" -- confirming round 4 in full. Two narrower points remained: 1. StructuralRules.singleton was declared (since round 2's full-fidelity fix) but evaluate_custom_format's step 5 always called copy_limit_violations(db, &counts, 4), never reading it, and there was no test. Fixed: parameterize on rules.structural.singleton (1 vs 4) -- confirmed this round that copy_limit_violations already takes exactly this parameter and every built-in singleton format already calls it with 1, so this is parameterizing an existing call, not new logic. Card-intrinsic overrides (Relentless Rats, DeckCopyLimit::UpTo) already compose correctly under any limit per the helper's own existing tests. 2. The registry gate (IMPLEMENTED_LEGACY_AXES) covered only LegacyRuleSet's four axes, not ReprintPolicy -- a preset could still register while declaring an unenforced ReprintPolicy. Rather than broadening the gate to a field never designed to be independently enforceable, re-read this document's own original research (RESEARCH.md #3, predating any review round): it already concluded ReprintPolicy's behavior is fully absorbed into legal_sets curation, with only the frame/art-level distinction being a real gap (Open item 2's, not this field's). Resolution: reprint_policy becomes documentation metadata, deliberately not consumed by the evaluator and deliberately outside the registration gate's scope -- satisfying the "keep/reduce to non-selectable/deferred metadata" branch round 5 explicitly offered as acceptable. * docs(custom-format-engine): address round-6 CHANGES_REQUESTED (matthewevans) Round 6 confirmed the singleton fix landed correctly and found one remaining issue: round 5 resolved ReprintPolicy to "documentation metadata, never consumed" but left the field sitting inside LegalityRules/ CustomFormatRules -- the resolved, engine-consumed, wire-traveling payload -- while a comment beside it (unchanged since round 1) still said it "gates LEGALITY". Two incompatible claims about the same field's contract: a struct's shape is itself a claim about what travels with and is enforced by the resolved ruleset, and no comment disclaiming that changes what the type says. Resolved structurally, not documentarily: reprint_policy moves out of CustomFormatRules/LegalityRules entirely, onto a newly-sketched CustomFormatDef struct (previously only described in prose as "display metadata + CustomFormatRules" -- exactly the ambiguity that let this happen), alongside label/short_label/description. This is one of the two resolutions matthewevans offered, chosen over building real engine-owned printing enforcement, for the same reason round 5 established: this document's own original research already showed the field's behavior is fully absorbed by legal_sets curation. Updated every preset sketch, the preset-readiness gate's scope description, and the registry-gate reasoning to match -- the gate no longer needs to exempt reprint_policy since it isn't on the resolved-rules struct at all. Added a discriminating test proving two CustomFormatDef values with identical rules but different reprint_policy produce identical evaluation results. * docs(custom-format-engine): address round-7 CHANGES_REQUESTED (matthewevans) Round 7 confirmed the round-6 structural move was correct ("correctly resolves the prior semantic contradiction") and found one construction gap: CustomFormatDef.reprint_policy was a required ReprintPolicy, but from_lobby_config has no legitimate value to put there -- a lobby-saved format has no authored paper-format reprint intent at all, so forcing any of the three real variants onto it would be fabricated metadata. Fixed: reprint_policy: Option<ReprintPolicy>. None for from_lobby_config (and, for now, swedish_old_school() pending Open item 6) -- the same Option<T>-over-forcing-a-value pattern this proposal already uses for legal_sets and range_of_influence. Some(_) with a real, sourced value for every other Axis B preset. Also fixed a stale round-1 cross-reference in CONTEXT.md's original Axis B field list that still named reprint_policy alongside the genuinely resolved-legality fields, never updated when round 6 moved it. Added a test requiring each of the five preset constructors to set its own specific value, not one shared "is Some" check. * docs(custom-format-engine): address round-8 CHANGES_REQUESTED (matthewevans) from_lobby_config's signature (name + &FormatConfig) never specified how a lobby save derives short_label/description, but CustomFormatDef requires both non-optionally since round 6. Fixes by deriving short_label as name's first 3 alphanumeric chars uppercased (the same convention the frontend already falls back to independently for unrecognized formats) and description via a new derive_structural_description(&StructuralRules) helper mirroring built-in formats' existing comma-joined structural phrasing — both derived from name/config alone, no invented metadata. Extends the existing lobby-save round-trip test to assert the derivation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-9 CHANGES_REQUESTED (matthewevans) The round-8 fix put label/short_label/description on CustomFormatDef, but that struct never travels past the lobby/picker — FormatConfig.custom_rules carries only CustomFormatRules, and SavedCustomFormat.name is explicitly client-local by round 2's own identity design. GameFormat::label() also structurally cannot return CustomFormatDef.label (a String) from a &'static str-returning function regardless of what's threaded in. Fixes both without adding wire surface: label() becomes Cow<'static, str>, resolving Axis-B presets via a custom_format_registry() lookup (a stable id every peer already shares) and falling back to a fixed "Custom Format" string for Axis-A ad-hoc saves, which have no registry entry to resolve at all. Retracts PLAN.md's prior "label/for_format get a Custom arm reading the resolved def" claim, which was never true for Axis A. Also found and fixed a second, independent instance of the same root cause: FormatConfig::for_format(bare GameFormat) is called for .deck_size at two real deck-validation call sites, silently returning a wrong default size for Custom formats. Gives for_format the same unreachable!() guard sideboard_policy/uses_commander already use, migrating both call sites to read custom_rules.structural.deck_size when present. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-10 CHANGES_REQUESTED (matthewevans) Matt flagged that Old School 93-94/95 register once mana burn lands with no gate tied to their source rules' printing-fidelity requirement (non-foil, original frame + art) - legal_sets is set-code-only and can't express this, and CONTEXT.md's own open item 2 already flagged the gap as unresolved, but the rollout plan had drifted out of sync with it. Resolved directly (discussed with the repo owner rather than decided unilaterally, per the doc's own "do not resolve unilaterally" instruction on that open item): legality enforcement stays legal_sets-only permanently -- no foil/frame data exists anywhere in the engine to enforce against. The source rules' spirit is instead honored by a general display fix: ArtChainEntry's existing {type: "oldest"} per-player preference becomes legal_sets-aware, so "oldest printing" respects the active format's legal set list instead of picking a promo/non-tournament printing the format doesn't recognize. Zero engine change, not gated on ReprintPolicy, benefits every format with a legal_sets restriction, not just the three Axis-B presets that declare one. Also backfills CONTEXT.md's round-history log with rounds 8 and 9, which only got PLAN.md updates in the prior two commits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-11 CHANGES_REQUESTED (matthewevans) Round 10 tried to resolve Old School 93-94/95's printing-fidelity gap by pairing legal_sets-only legality with a cosmetic ArtChainEntry display default. Matt correctly rejected this: an optional rendering preference is not a legality resolution. Resolved via a third path, grounded in existing precedent rather than a new policy call: confirmed GameFormat::Premodern's legality (LegalityFormat::Premodern) is oracle-card-level only, like every format in this engine -- none has ever checked printing, frame, or foil, and PrintedCardRef has no set-code field for any of them. legal_sets membership isn't an old-school-specific approximation needing a gate; it's this engine's one existing legality model, applied the same way to every format. Registration reverts to mana-burn-only, matching the original pre-round-10 gate. The ArtChainEntry display fix and genuine per-card printing selection (briefly surveyed: a moderate plumbing lift reusing existing PrintingPickerModal/sourcePrinting infrastructure, not from-scratch) are both retracted from this proposal as separate, real, future ideas -- general to every format, not bundled into this proposal's legality story. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-12 CHANGES_REQUESTED (matthewevans) — Old School printing fidelity Round 11's resolution ("legal_sets membership isn't an approximation of Old School 93-94/95's legality — it's the same oracle-card-level model Premodern and every other format already uses") was reviewed again and correctly rejected: Premodern never claimed a printing-level requirement in the first place, so it not enforcing one isn't an approximation of anything. Old School 93-94/95's own cited source (RESEARCH.md §1) explicitly requires non-foil original-frame/art reprints, and legal_sets genuinely falls short of that specific stated rule regardless of what every other format checks. Product decision (not resolved away by precedent this time): decline to build engine-owned printing/frame enforcement — the engine's and frontend's printing systems are confirmed disconnected, and wiring them is a real, separate, moderate-lift future feature, not old-school-specific. Accept the set-code-only approximation on its own terms: in a digital-only client, a printing's frame/border/foil status has zero gameplay consequence, since two printings with identical Oracle text are identical for every rules purpose the engine cares about. The paper community's frame/art requirement serves an anti-counterfeiting/provenance function specific to a physical table that has no digital equivalent. Takes the maintainer's own second offered resolution (explicitly scope the presets as an oracle-card/set-code approximation) and makes it structural rather than documentary, per this proposal's own established standard that conventions must be enforced, not just written down: - New `CustomFormatDef.printing_fidelity: PrintingFidelity` field (NotApplicable / SetCodeApproximation), required and paired with `reprint_policy` at construction time. - All four EC presets (old_school_93_94, old_school_95, middle_school, classic_magic) set SetCodeApproximation and must disclose the limitation in their player-facing `description`, not just a doc comment. - New registry gate (§7) and test (§6) enforce the reprint_policy/ printing_fidelity pairing and the description disclosure — separate from the existing IMPLEMENTED_LEGACY_AXES gate, which covers unimplemented engine work; this one covers an authoring omission. - CONTEXT.md's Open item 2 log gets an appended correction + final resolution (round 12), not an overwrite of round 11's now-superseded entry. Also fixes the CodeRabbit-flagged [MEDIUM]: old_school_95()'s builder called `d.legal_sets.extend(...)` directly, but legal_sets is `Option<Vec<SetCode>>` per §1 — `.extend()` doesn't exist on `Option`. Uses `get_or_insert_with(Vec::new).extend(...)` so it stays correct even if a future refactor changes the base preset's Some/None invariant. Model: claude-sonnet-5 Tier: Frontier Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQ1bpYEt331DvjqPLSFJEH * docs(custom-format-engine): fix old_school_95() field nesting + add inheritance test Maintainer review of the round-12 commit confirmed the printing-fidelity blocker resolved, but caught that old_school_95()'s builder sketch mutates d.legal_sets/restricted/banned directly — those aren't fields on CustomFormatDef at all; they live at d.rules.legality.* per §1's own declared schema (CustomFormatDef.rules: CustomFormatRules, CustomFormatRules.legality: LegalityRules). The prior get_or_insert_with fix was correct for the Option<Vec<SetCode>> detail but applied at the wrong nesting level. Every mutation now composes through the real path (d.rules.legality.legal_sets.get_or_insert_with(...).extend(...), etc.), and adds the requested preset-inheritance test: asserts old_school_95()'s resolved legal_sets/restricted/banned each equal old_school_93_94()'s base plus exactly its own declared delta (an exact-set comparison, not a superset check), which would also have caught this nesting bug mechanically. Model: claude-sonnet-5 Tier: Frontier Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQ1bpYEt331DvjqPLSFJEH --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the previously-
UnimplementedETB line of Anointed Peacekeeper — "As ~ enters the battlefield, look at an opponent's hand, then choose any card name." — by composing existing engine primitives into four general, reusable building blocks. No new engine enum variant.Implementation method (required)
/engine-implementerpipeline (plan → review-plan → implement → review-impl → commit)/engine-implementer— explain why belowThe plan-review caught two Delta #4 defects (offset-into-original vs.
~-substituted text; an over-broad peeled-slot guard) which were folded in before implementation; the implementation-review returned "essentially clean, no HIGH/MED," and its three LOW items were resolved.CR references
All reused verbatim from existing in-repo annotations (
docs/MagicCompRules.txtis network-blocked in this environment and could not be freshly grepped):oracle_replacement.rs).RevealHand { reveal: false }(reveal_hand.rs).Verification
Developer track — Tilt is down in this environment, so cargo was run directly.
cargo fmt --all— clean.cargo test -p engine --lib— 14,686 pass, 0 real failures, including regressionsas_enters_choose_a_color,as_enters_choose_two_colors,as_enters_choose_a_creature_type,as_enters_choose_a_number_sentence_ending_period,as_enters_choose_does_not_match_shock_land,enters_tapped_then_choose_color_composes_tap_and_choice(Thriving land). (The 2 apparent failures were a truncated localcard-data.jsonartifact —snuff_out_from_card_database_*/witherbloom_card_object_*self-skip on a clean tree, confirmed.)cargo test -p engine --test anointed_peacekeeper_etb— 1 pass (drives the real cast → ETB replacement → private hand-look → name-a-card pipeline; asserts persistedChosenAttribute::CardName,RevealHand { reveal:false }over the opponent's hand not the controller's, and both cost-tax statics bound).cargo clippy -p engine --lib --test anointed_peacekeeper_etb -- -D warnings— clean, exit 0../scripts/check-parser-combinators.shexit 0 (no output on success; no violations).What changed (build for the class, not the card)
try_parse_named_choiceparser/oracle_effect/mod.rsparse_hand_possessive_targetparser/oracle_effect/imperative.rsRevealHandplayer-from-filter fallback viacollect_player_targetsgame/effects/reveal_hand.rsRevealHandreached with no player target slotparse_as_enters_chooseparser/oracle_replacement.rsLines 3 & 4 of the card (the spell / activated-ability cost taxes keyed to the chosen name) already parsed and already consume a persisted
ChosenAttribute::CardName; this change makes the ETB establish that persisted name plus the private hand-look.Anchored on (≥2 analogous implementations per delta, same combinator family)
tag("a card name")/tag("a nonland card name")(oracle_effect/mod.rs:17754-17757); determiner-variant armstag("a color other than ")/tag("a color")(mod.rs:17731,17737).value(Typed(Opponent), tag("target opponent's hand"))(imperative.rs:3137-3140);value(Player, tag("target player's hand"))(imperative.rs:3136).collect_player_targets(state, ability, &target)ineffects/phase_out.rs:51,91andeffects/effect.rs:411.scan_preceded(...)inoracle_effect/search.rs:93,105,201&oracle_modal.rs:561; the existingSetTapState{SelfRef}.sub_ability(choose)composition in the same function (oracle_replacement.rs:1756-1762).Scope expansion
None. Pure composition of existing effects (
RevealHand+Choose) plus one additive resolver fallback. No newEffect/enum variant; no frontend/AI/multiplayer-filter/targeting registration touched (lines 3/4 already consume the chosen name this ETB now establishes).Known limitation (honest, out of scope)
This card does not flip to coverage-
supportedafter this change, because of a separate, pre-existing gap unrelated to line 2: line 4 ("Activated abilities of sources with the chosen name cost {2} more to activate unless they're mana abilities") parses toStaticMode::ReduceAbilityCost, which has noexemptionfield, so the CR 605.1a "unless they're mana abilities" clause is dropped and the swallow-checkdetect_condition_unlesscorrectly emits aCondition_Unlessswallowed-clause warning. This is proven independent of this change: a probe card containing only lines 1/3/4 (no ETB) emits the identical warning. Modeling that exemption spans the cost-modification subsystem (addingexemption: ActivationExemptionacross ~17ReduceAbilityCostsites + resolver application + a resolver test) and belongs in a dedicated follow-up PR. This diff does not mask the dropped line-4 semantic; it only implements line 2.🤖 Generated with Claude Code
https://claude.ai/code/session_019mjar7ovGZua3ANz9r76fk
Generated by Claude Code