chore: update coverage stats and badges - #4
Merged
Conversation
matthewevans
enabled auto-merge (squash)
April 8, 2026 13:17
matthewevans
added a commit
to AlexD-Richardson/phase
that referenced
this pull request
Apr 27, 2026
…e verbs The previous orthographic rule stripped `-es` whenever the resulting stem ended in `ch`/`sh`/`ss`/`x`/`z`. This was correct for `searches → search` but over-applied to the `-eze`/`-eeze` family, producing invented stems like `freezes → freez`, `breezes → breez`, `sneezes → sneez` — none of which match any downstream lookup, but they violate the project's "parser must not swallow" rule by silently fabricating non-existent words for unknown inputs. Narrow the rule to only strip `-es` when the resulting stem is a registered `PREDICATE_VERBS` member. Unknown verbs now pass through unchanged, and the only verbs that take the strip are ones the parser explicitly knows about. Adds a regression test that asserts neither `freezes` nor `breezes` nor `sneezes` produces an invented stem, while the registered `searches → search` case still works. PR phase-rs#124 review feedback (findings phase-rs#4, phase-rs#5 third test).
matthewevans
added a commit
that referenced
this pull request
Apr 27, 2026
* Add Winds of Abandon * fix(overload): spell out ChangeZone field drops explicitly (no `..` rest) The overload `Effect::ChangeZone → ChangeZoneAll` arm previously used `..` to absorb every field other than `origin`/`destination`/`target`. Today's dropped fields are semantically inert for hidden-zone exile, but `..` would silently absorb any newly-added `ChangeZone` field too, meaning a future field addition could go missing in the overloaded form without compiler help. Bind every field by name and annotate the rationale for each drop. Adding a new `ChangeZone` field will now fail to compile here, forcing a deliberate decision about overload semantics. PR #124 review feedback (finding #3). * fix(engine): preserve sub_ability on resumed repeat_for iterations + handle synchronous-continuation drain Two interlocking bugs in the `repeat_for` resume path surfaced during PR #124 review of overloaded Winds of Abandon: 1. Iteration-resume path dropped sub_ability for iterations 2+. The stash site cleared `resume_ability.sub_ability = None` and the drain called `resolve_effect` directly, bypassing the chain-level wiring at line 1461 that stashes the SearchChoice continuation. For iteration 0, the put-onto-battlefield + shuffle continuation runs correctly; for iterations 1+, it never ran at all — opponents 2+ would be prompted to pick a card but their chosen card stayed in their library. Fix: keep `sub_ability` on the resumed copy and clear `repeat_for` instead (so the resume call doesn't re-enter the outer iteration loop). The drain now calls `resolve_ability_chain` (depth=1, to preserve chain-local tracked-set state) so each resumed iteration goes through the same line-1660 SearchChoice continuation wiring as iteration 0. Per-iteration parent-target rebinding still propagates correctly because `rebind_first_object_target` updates `iter_ability.targets`, which the line-1651 sub_ability propagation copies onto the continuation chain. 2. Synchronous-continuation case was unhandled. If an iteration set `pending_continuation` without changing `waiting_for`, the inner loop would increment and run the next iteration — clobbering the continuation — or, on the trailing iteration, fall through and break without re-stashing remaining iterations. Fix: detect a None→Some `pending_continuation` transition and re-stash with `next_iteration = iteration + 1` before breaking, so the outer drain runs the continuation and then re-enters this drain for the next iteration. Tests: - `repeat_for_resumed_iteration_runs_full_sub_ability_chain`: end-to-end across two distinct opponents, asserts both chosen lands land on the battlefield AND both Shuffle resolutions emit EffectResolved events. Would have caught finding #1 directly. - `drain_pending_repeat_iteration_restashes_on_synchronous_continuation`: exercises the synchronous-continuation case with a multi-iteration resume that completes without entering any choice state. PR #124 review feedback (findings #1, #2, #5 first/second tests). * fix(parser): narrow normalize_verb_token's -es rule to known predicate verbs The previous orthographic rule stripped `-es` whenever the resulting stem ended in `ch`/`sh`/`ss`/`x`/`z`. This was correct for `searches → search` but over-applied to the `-eze`/`-eeze` family, producing invented stems like `freezes → freez`, `breezes → breez`, `sneezes → sneez` — none of which match any downstream lookup, but they violate the project's "parser must not swallow" rule by silently fabricating non-existent words for unknown inputs. Narrow the rule to only strip `-es` when the resulting stem is a registered `PREDICATE_VERBS` member. Unknown verbs now pass through unchanged, and the only verbs that take the strip are ones the parser explicitly knows about. Adds a regression test that asserts neither `freezes` nor `breezes` nor `sneezes` produces an invented stem, while the registered `searches → search` case still works. PR #124 review feedback (findings #4, #5 third test). * test(engine): genuinely exercise synchronous-continuation re-stash predicate The previous version of `drain_pending_repeat_iteration_restashes_on_synchronous_continuation` constructed a Draw + sub-Draw setup that completed each iteration cleanly without ever installing a `pending_continuation` synchronously — meaning the `installed_continuation` predicate was never evaluated true. The test passed regardless of whether the predicate existed. Rewrite the test to use `ConditionInstead` with an `else_ability` and a non-Priority pre-set `waiting_for`, which exercises the line-1486 path that synchronously stashes the else branch into `pending_continuation` without changing `waiting_for`. Verified by temporarily disabling the `installed_continuation` predicate: the test now FAILS with the exact "pending_continuation overwritten before consumption" debug_assert that finding #2 describes. Asserts after one resumed iteration: - `pending_continuation` is Some (else_ability was stashed synchronously) - `pending_repeat_iteration` is re-stashed with `next_iteration = 2` - only iteration 1's parent Draw fired (1 card), proving the drain broke immediately on the synchronous-continuation transition PR #124 review feedback (finding #2 — proper test coverage). --------- Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
matthewevans
added a commit
that referenced
this pull request
May 11, 2026
… grantee (#326) Five interlocking subbugs prevented Jeleva, Nephalia's Scourge from behaving anywhere close to its Oracle text: 1. ETB exiled exactly one card per player instead of X — the `each player exiles the top X cards of their library, where X is the amount of mana spent to cast Jeleva` body parsed to a `ChangeZone` with an unbounded library-card filter and no count. `parse_exile_ast` now recognises `their library` (and `each player's library`) under the `each player` outer scope so it binds to `TargetFilter::ScopedPlayer`, parses `x` as `Variable { "X" }`, and substitutes the count with the dynamic quantity expression from the `", where X is …"` suffix via the new `resolve_exile_top_where_x_binding` helper (mirroring `try_parse_token_enters_with_counters`'s X-binding rule). Composes with the typed `QuantityRef::ManaSpentToCast { SelfObject, Total }` ref shipped in #307 for Painful Truths Converge. 2. ETB prompted the user to pick which library card to exile — falls out of the ExileTop fix above; ExileTop is a top-N batch move with no target selection. 3. Attack trigger let the player cast any card — `try_parse_cast_effect` already recognised the per-resolution `from among them / those exiled cards` anaphor (commit d04aab782) but not the persistent `from among cards exiled with ~` form that references the source permanent's own tracked exile set across resolutions. Adds a new branch that detects the `cards exiled with [~|this creature|this spell|it]` anchor and emits `CastFromZone { target: And[Typed{Instant, Sorcery, InZone(Exile)}, ExiledBySource] }`. Includes a `parse_cast_type_disjunction` helper for the `an instant or sorcery spell` / `an instant or sorcery card` disjunctive type phrase that `parse_type_phrase` doesn't currently handle (it only does adjacent-conjunction, not ` or `-separated core types). 4. Cast spell resolved under the wrong player's control — `ExileWithAltCost` and `ExileWithAltAbilityCost` had no grantee field, so `has_exile_cast_permission` fell back to `obj.owner == player`, letting any player who owned an exiled card cast it (each opponent whose library was raided by Jeleva's ETB). Adds an optional `granted_to: Option<PlayerId>` field to both variants (mirrors `PlayFromExile.granted_to`) and threads it through every grant site: - `cast_from_zone::resolve` records `ability.controller` at grant time (the Jeleva attack-trigger surface). - Discover and Cascade choice handlers record the discovering / cascading player from the WaitingFor continuation. - `grant_permission::resolve` fills None placeholders from `PermissionGrantee::{AbilityController, ObjectOwner, ParentTargetController}` so Airbending and the wider parser-emitted class continue to bind correctly without per-call-site changes. - `has_exile_cast_permission`, `spell_objects_available_to_cast`, `prepare_spell_cast_with_variant_override`, and the alt-cost cost readers (`alt_cost_from_exile`, the `ExileWithAltAbilityCost` reader in `casting_costs::check_additional_cost_or_pay`) gate on the granted_to binding when set, falling back to the legacy owner-based check only when None (test scaffolding / pre-binding code paths). 5. Same root cause as #4 — once granted_to gates the cast, the spell resolves under the granted player's control per CR 601.2a (the player who proposed the cast becomes the spell's controller). Annotations bound to CR 611.2a (continuous-effect duration binding), CR 118.9 (alternative costs paid by the spell's controller), CR 601.2a (cast initiation), CR 406.6 / CR 603.10a (exile linkage across resolutions), CR 107.3i (X-binding suffix), CR 202.3 + CR 601.2h (mana-spent-to-cast quantity ref, reusing #307 infrastructure). Cards positively affected: - Jeleva, Nephalia's Scourge — full ETB + attack-trigger flow. - Etali (Primal Storm / Primal Conqueror), Apex of Power, and the wider Branch 1.5 class — cast permissions now correctly scoped to the resolving ability's controller rather than any owner (was a latent multiplayer bug, surfaced by Jeleva). - Silent-Blade Oni and the cross-hand-cast class — same scope fix. Two new parser regression tests: - `jeleva_etb_each_player_exiles_top_x_resolves_to_mana_spent_to_cast` - `cast_from_among_cards_exiled_with_self_binds_to_exiled_by_source_and_type` 6733 unit tests pass, 264 integration tests pass, clippy clean.
matthewevans
pushed a commit
that referenced
this pull request
May 24, 2026
Three reviewer findings on the prior PerTurnCastLimit work for Ethersworn Canonist: FINDING #1 (MEDIUM): the conditional-subject combinator inlined `tag("each player who has cast ")` and hard-coded `ProhibitionScope::AllPlayers`, bypassing the shared `strip_casting_prohibition_subject` building block. A future card phrased as "Each opponent who has cast ..." would have reduced to zero coverage. Restructured to strip the subject prefix via the shared helper first, then nom-match `who has cast (a|an) <SUBJ> spell this turn can't cast additional <OBJ> spells`. Two class tests (`each_opponent_scope`, `you_scope`) lock in the subject axis. FINDING #2 (MEDIUM): CR citations referenced 101.2 + 604.1 ("can't" beats "can" + static enforcement), but the *authorizing rule for the casting prohibition itself* (CR 601.2 + CR 601.3a) was missing. Verified both rule numbers against docs/MagicCompRules.txt before adding. Updated citations on the combinator docstring, the caller's inline comment, and the swallow_check marker comment. LOW #4 (carry-over): swallow marker was a bare `"PerTurnCastLimit"` substring — could false-positive on the literal string appearing in a description or unrelated location. Tightened to the serde external-tag shape `"\"PerTurnCastLimit\":{"` (and same for PerTurnDrawLimit), matching the precision class of the existing `"condition":{"type":"Unrecognized"` marker on the line above. Verification: - cargo fmt --all: clean - cargo clippy --all-targets -- -D warnings: clean - cargo test -p engine: 8187 lib + 422 integration + 7 doctest, all pass - oracle-gen for "ethersworn canonist": no Unimplemented, no parse_warnings, AST unchanged from baseline Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mike-theDude
pushed a commit
to mike-theDude/phase
that referenced
this pull request
May 25, 2026
…ct count - engine.rs: populate current_trigger_match_count from pending_optional_trigger_match_count before drain_pending_continuation so EventContextAmount resolves the correct "that many" in the resumed continuation (Gemini comment #1 — logic error in state restoration) - trigger_matchers.rs: add CR 603.2c doc annotation to trigger_event_subject_ids (comment phase-rs#2); extend match to return ObjectId arms for TokenCreated, CreatureDestroyed, PermanentSacrificed, PermanentTapped, PermanentUntapped, and DamageDealt (object target only, per CR 120.3) — previously these all fell through to std::iter::empty() (comment phase-rs#3) - quantity.rs: replace .map(|n| n as i32) with .map(u32_to_i32_saturating) for consistency with the rest of the file (comment phase-rs#4) Verified: cargo clippy --all-targets -- -D warnings (clean), cargo test -p engine (8956 passed) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced May 25, 2026
matthewevans
pushed a commit
to mike-theDude/phase
that referenced
this pull request
May 29, 2026
…e (refs phase-rs#594) (phase-rs#1401) * Fix phase-rs#594 (partial): parser drops count+controller for "target opponent's library" exile Extend parse_exile_ast to handle "target opponent's library" / "target player's library" patterns. Restores count and controller filter for the ExileTop effect emitted by Maralen, Fae Ascendant and Court of Locthwain ETB triggers. Variant choices (canonical leaves already produced by parse_target): - "target opponent" → TargetFilter::Typed(TypedFilter::default().controller(Opponent)) - "target player" → TargetFilter::Player Also routes "once each turn, you may cast" lines through the static classifier in preparation for the follow-up cast-from-exile-permission work (Step 10-14 of the investigator plan, tracked as a separate PR). Today this changes the Unimplemented "name" marker on Maralen's second ability from "effect_structure" to "static_structure" — both stay Unimplemented; the routing change only sets up the static handler that the follow-up PR will implement. Defect phase-rs#3 in the issue (subject expansion dropped) was investigated and confirmed already correct in code — no change needed. Defect phase-rs#4+phase-rs#5 (this-turn exile-link scoping + ExileCastPermission static) are deferred to a follow-up PR per investigator's recommended sequencing. Tests: - exile_top_target_opponents_library (parser-level): ExileTop count=2, target-opponent - exile_top_target_players_library_singular (parser-level): ExileTop count=1, Player - trigger_maralen_etb_exile_top_two_of_target_opponents_library (trigger integration): full Oracle text round-trips to ChangesZone trigger with ExileTop(Opponent, 2) Refs phase-rs#594 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * parser: correct CR citation on "once each turn, you may cast" classifier entry CR 601.3e specifically governs static abilities that allow casting spells from non-hand zones (Garruk's Horde, Melek, Maralen). The earlier citation (CR 113.6 + CR 117.1) was adjacent rather than targeted. Per PR phase-rs#1401 review. --------- Co-authored-by: Michael Briningstool <mbriningstool@gmail.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
YB0y
pushed a commit
to YB0y/phase
that referenced
this pull request
May 29, 2026
…rom-exile (phase-rs#1404) * feat(engine): ExileCastPermission static for Maralen-class cast-from-exile Closes the remaining gap on phase-rs#594 left after phase-rs#1401 (count + controller fix on the trigger half). Adds the engine support for "Once each turn, you may cast a spell ... from among cards exiled with ~ this turn without paying its mana cost." — defects phase-rs#4 (per-turn exile-link scoping) and phase-rs#5 (cast-from-exile permission static) in the issue. New `StaticMode::ExileCastPermission { frequency, play_mode, without_paying_mana_cost }` mirrors `GraveyardCastPermission` for the exile-pool sibling. Runtime additions: - `GameState::cards_exiled_with_source_this_turn` — per-source per-turn rolling list, populated alongside `exile_links::push_tracked_by_source` and cleared at turn cleanup. Keeps the persistent `exile_links` pool untouched (still backs the open-ended `ExiledBySource` filter). - `GameState::exile_cast_permissions_used` — per-source `OncePerTurn` slot, mirroring `graveyard_cast_permissions_used`. - `CastingVariant::ExilePermission { source, frequency }` — casting context that finalize-cast consults to stamp the slot. - `casting.rs::exile_objects_castable_by_permission` + `exile_cast_permission_source` extend `spell_objects_available_to_cast` and `has_exile_cast_permission`. - `casting.rs::is_exile_permission_free_cast` zeroes the mana cost when the static carries `without_paying_mana_cost: true`. - `exile_links::LINKED_EXILE_CONSUMER_TAGS` gains `"ExileCastPermission"` so a Maralen source is auto-detected as a tracked-exile consumer; her ETB trigger then populates exile_links + the per-turn map for free. - Parser handler in `oracle_static.rs::try_parse_exile_cast_permission` uses the shared nom combinator chain — `parse_type_phrase` already composes the dynamic "with mana value …" suffix through `parse_mana_value_suffix`, so Maralen's filter reaches `Cmc(LE, ObjectCount{Elf|Faerie, You})` through one call. CR annotations: 601.2a (cast permission grant), 113.6b (zone-restricted functioning), 118.9 (alternative cost), 400.7 (zone-change resets the source ObjectId / per-turn slot), 305.1 (play vs cast). All verified against docs/MagicCompRules.txt. Tests: - Parser: Maralen full line, longer "once during each of your turns" synonym, rejects missing "this turn" suffix, regression-guarded against the graveyard branch intercepting. - Engine: surface the per-turn pool, OncePerTurn slot gates and resets, cards outside the per-turn map (stale exile from prior turn) are pruned. Branch: bugfix/594-maralen-cast-from-exile Refs phase-rs#594. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(engine): typed ExileCastCost enum replaces bool field (R2) Replaces the `without_paying_mana_cost: bool` field on StaticMode::ExileCastPermission with a typed `ExileCastCost` enum (PayNormalCost | WithoutPayingManaCost), per CLAUDE.md rule R2 (no raw bool fields — use typed enums that express the design space). Addresses Gemini review comment on PR phase-rs#1404. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * perf(PR-1404): empty-pool fast-exit for exile-cast permission scans exile_permission_sources scans the whole battlefield and allocates an active_static_definitions iterator per controlled permanent. Both exile_objects_castable_by_permission (once per legal-actions / AI-search node) and exile_cast_permission_source (per-object castability predicate, casting.rs:829) call it on the hot path. Guard both with a single cards_exiled_with_source_this_turn.is_empty() check: a card is castable-via-permission only if it was exiled-with-a- source this turn (it must live in that pool), so an empty pool provably yields no offers. Skips the scan in the ~100% of board states with no Maralen-class permanent active; output is identical. Add start_next_turn_resets_exile_cast_permission_tracking: a discriminating regression test that drives start_next_turn and fails if either per-turn reset line (exile_cast_permissions_used / cards_exiled_with_source_this_turn) is dropped. --------- Co-authored-by: Michael Briningstool <mbriningstool@gmail.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
2 tasks
matthewevans
added a commit
that referenced
this pull request
Jun 10, 2026
* refactor(engine): route single-authority bypasses through canonical entries
- explore.rs: replace hand-rolled library->hand zone mutation with
zones::move_to_zone (emits ZoneChanged + runs zone-exit cleanup)
- layers: drop the unused evaluate_layers re-export; migrate the two
production direct callers (become_copy, engine_replacement) to
flush_layers / mark_layers_full + flush_layers; doc-reserve direct
evaluate_layers calls for tests
- ability_utils: funnel validate_selected_targets{,_for_ability} count
validation through one shared inner body (targeting.rs pattern)
- engine_resolution_choices: route the two synthetic-ability resolver
bypasses through resolve_ability_chain; doc resolve_effect's contract
- keywords: document the object-scoped vs state-scoped query tiers
* feat(parser): single authority for expressing unparsed text
Effect::unimplemented(name, fragment) is the one way to construct the
Unimplemented effect; name is a stable pattern-class key consumed by
coverage gap categorization. Delete the dead parse_or_unimplemented /
option_to_nom boundary fns (zero callers) and fix the stale CLAUDE.md
oracle_nom/error.rs description.
* ci: extend parser gate + add engine-authority and skill-doc gates
- check-parser-combinators.sh: close .rfind/.split/.splitn blind spots;
new family (E) verbatim-sentence equality (== "25+ chars"); new
family (F) hand-constructed Effect::Unimplemented literals
- check-engine-authorities.sh (new): diff gate forbidding raw
.keywords.contains/.iter queries outside the keyword authorities
- check-skill-doc.sh (new): asserts oracle-parser SKILL.md matches the
parser tree (paths, symbols, priority-table row parity); SKILL.md
regenerated against the code (38-slot priority table, IR layer,
clause_shell, oracle_static/ split, single-authority doctrine)
* refactor(parser): make condition bridges exhaustive over their source enums
static_condition_to_trigger_condition, static_condition_to_ability_condition,
and ability_condition_to_static_condition no longer hide unbridged variants
behind wildcards: adding a StaticCondition/AbilityCondition variant is now a
compile error at each bridge, forcing a conscious bridging decision. All
expanded arms map to None exactly as the wildcards did (no behavior change).
* refactor(engine): cost-payment Phase 0 — sorcery-speed delegation + pay_from_pool rename
Phase 0 of the cost-payment unification plan (.planning/cost-payment-unification):
- casting.rs: CastingProhibitionCondition::NotSorcerySpeed now delegates to
restrictions::is_sorcery_speed_window instead of re-deriving the CR 307.1
timing predicate inline — restrictions.rs is the single timing authority.
- mana_payment.rs: pay_cost → pay_from_pool (pool-level arithmetic only),
freeing the pay_cost name for the future game/costs.rs ability-cost
payment authority. Callers + re-export updated.
* refactor(parser): consolidate three parallel duration grammars into oracle_nom/duration.rs
Single authority for Oracle duration phrases (CR 611.2 / CR 611.2b /
CR 514.2): the prefix-nested nom grammar in oracle_nom/duration.rs now owns
every phrase→Duration mapping. strip_leading_duration /
strip_trailing_duration in oracle_effect/lower.rs are positional-only
wrappers (word-boundary scan + quantity-clause guard; the two phrase tables
are deleted), and the three inline re-encodings in oracle_effect/subject.rs
delegate to the grammar.
Conflict between the old grammars resolved in the strip table's (test-pinned)
favor: 'for as long as you control ~' maps to UntilHostLeavesPlay. That is a
pre-existing imprecision vs CR 611.2b (control loss without leaving the
battlefield should end it) — now visible in exactly one place.
* refactor(parser): replace verbatim sentence clusters with nom single authorities
- 'Your speed can increase beyond 4' (CR 702.179d-e) was string-equality
encoded at three sites (two oracle.rs routing predicates + the semantic
parse in oracle_static/dispatch.rs); now one is_speed_unlock_sentence
combinator in dispatch.rs, called by all three.
- The ChooseFromZone dig continuation 'put the rest on the bottom of your
library [in a random order | in any order]' (CR 401.4) was three full-line
equalities in sequence.rs; now one all_consuming combinator with the order
suffix as an opt(alt(...)) axis.
* refactor(engine): zone-change pipeline Phase A — carve out game/zone_pipeline.rs
Zero-behavior-change carve-out per .planning/zone-change-pipeline/PLAN.md:
execute_zone_move / deliver_replaced_zone_change / apply_zone_delivery_tail
and friends move verbatim from effects/change_zone.rs into the new
game/zone_pipeline.rs module (one mechanical super::→crate:: path fix); a
pub(crate) use shim keeps every existing caller compiling unchanged.
Seeds the Phase B+ vocabulary (not yet consumed, scoped #[allow(dead_code)]):
ZoneMoveRequest + builders, ZoneChangeCause (CR-annotated exemption split),
EntryMods (EtbTapState, not bool), ExileLinkSpec, DeliveryCtx, and the
ApprovedZoneChange proof token (no Serialize/Deserialize/Clone/Default;
private event field; pub(crate) approve_post_replacement mint path).
Independently reviewed: APPROVED. Phase B watch-item: move_object must seed
EtbTapState onto the ProposedEvent directly instead of round-tripping
through execute_zone_move's legacy bool boundary.
* refactor(types): unify Effect::AddCounter into Effect::PutCounter (CR 122.1)
Effect::AddCounter and Effect::PutCounter expressed the same operation
(place counters on an object) through two variants, forcing every match
site to handle both — and several AI policy sites silently handled only
one (cast_facts, redundancy_avoidance had AddCounter-only legs with
PutCounter receiving different treatment). One variant, one meaning:
PutCounter is the single authority, with #[serde(alias = "AddCounter")]
accepting legacy serialized data.
All ~70 match/construction sites across engine, phase-ai, and
mtgish-import migrated; duplicate arms produced by the rename were
collapsed (compiler-verified via -D unreachable-patterns). Engine
inventory regenerated; the two remaining AddCounter entries are
unrelated enums (ReplacementEvent / game event).
* refactor(parser): rewrite parse_restriction_modes as negation x verb-phrase list grammar
Replace ~14 verbatim equality clusters ('can't attack or block',
'can't block or be blocked', ...) with a composed nom grammar:
alt(("can't", "cannot")) + separated_list1 over restriction atoms
(attack / block / be blocked / be sacrificed / be enchanted / be
equipped / be countered / transform / crew). Any combination of atoms
now parses — including orders no current card uses — instead of only
the enumerated permutations. Elided-'be' English ellipsis ('can't be
equipped or enchanted') handled via a bare-atom fallback inside the
list parser.
Also freezes oracle_quantity.rs for new grammar: new quantity
recognition belongs in oracle_nom/quantity.rs (module doc).
* refactor(parser): delete zero-caller parse_target_phrase wrapper
oracle_target::parse_target is the single authority for 'target <type
phrase>' extraction; the unused nom wrapper was a second grammar waiting
to drift. Its tests retargeted to parse_type_phrase, the building block
production actually calls.
* ci: make coverage ratchet informational on push-to-main
On push-to-main the regression check can only do harm: the merge has
already happened, and a failing check blocks the R2 baseline republish
that runs later in the same job — wedging main red until someone
manually uploads a baseline (the recurring PR #2339 / PR #2802
swallowed-clause ratchet deadlock). continue-on-error on main pushes
lets the baseline self-heal after an intentional ratchet absorption,
while PR and merge_group runs remain fully blocking.
* refactor(types): replace sorcery_speed bool with ActivationRestriction::AsSorcery single authority
CR 602.5d: 'Activate only as a sorcery' timing is now represented solely by
ActivationRestriction::AsSorcery in AbilityDefinition.activation_restrictions.
The parallel sorcery_speed display bool is deleted; is_sorcery_speed() queries
the restriction. A hand-written Deserialize (AbilityDefinitionDe mirror)
tolerates the legacy serialized field and migrates sorcery_speed:true into
AsSorcery (dedup). Serialized exports no longer emit the key; snapshots and
the integration card fixture follow.
* refactor(types): fold ActivationCadence into ActivationRestriction (CR 602.5b)
CR 602.5b: once-each-turn activation cadence on keyword actions (Crew) is now
expressed with the existing ActivationRestriction::OnlyOnceEachTurn instead of
the parallel ActivationCadence enum, which is deleted. Keyword::Crew's
once_per_turn becomes Option<Box<ActivationRestriction>> (None = unrestricted;
boxed to break the Keyword -> ActivationRestriction -> ParsedCondition ->
Keyword size cycle). The custom Crew deserializer accepts both the legacy
ActivationCadence tagged shape ({"type":"Unlimited"}/{"type":"OncePerTurn"})
and the new Option<ActivationRestriction> shape. Regenerated integration card
fixture carries the new export shape (and drops the legacy sorcery_speed key).
* docs(engine): fix stale sorcery_speed display-flag comment (review follow-up)
* refactor(engine): seed three-state EtbTapState directly onto ZoneChange proposal (Phase B watch-item)
execute_zone_move took `effect_enter_tapped: bool` and could only ever set
EtbTapState::Tapped, collapsing the Unspecified-vs-Untapped distinction the
pipeline carrier (ProposedEvent::ZoneChange.enter_tapped: EtbTapState) exists
to preserve. move_object further round-tripped its req.mods.enter_tapped
(EtbTapState) through .is_tapped() before passing it in.
Thread EtbTapState end-to-end: execute_zone_move now accepts EtbTapState and
seeds it onto the proposal whenever it is not Unspecified (CR 614.1). All
callers that already hold an EtbTapState (change_zone resolver paths, seek)
pass it through unchanged; bool-literal callers pass EtbTapState::Unspecified.
move_object passes req.mods.enter_tapped directly. No bool round-trip remains.
This is the recorded Phase A review condition for proceeding with Phase B.
* refactor(engine): route destroy.rs delivery through zone pipeline proof token (Phase B)
apply_destroy_after_replacement delivered its inner (post-replacement)
ZoneChange with a bare zones::move_to_zone in both arms (the post-Destroy inner
move and the outer-replacement-redirected-to-ZoneChange move). A destruction
redirected to the battlefield (CR 614.6) therefore dropped the entire delivery
tail: CR 614.1c enters-with-additional-counter statics, enter_tapped /
enter_with_counters, exile-link tracking, and the post-replacement-continuation
drain.
Seal each already-replaced ZoneChange via ApprovedZoneChange::approve_post_replacement
(preserving its `applied` set and re-validating it is a ZoneChange) and deliver
through zone_pipeline::deliver — the single delivery tail. A NeedsChoice from
the tail propagates as `false` (state.waiting_for already set) so the caller
does not advance.
Adds a discriminating integration test
(destroy_redirect_to_battlefield_delivery_tail) that casts "Destroy target
creature" at a victim with a Moved->Battlefield redirect under a CR 614.1c
additional-+1/+1-counter static, and asserts the redirected creature receives
the counter — fails on the old raw move (0 counters), passes through the tail.
CR 701.8a (line 3315), CR 614 / 614.6 (line 3072), CR 614.1c (line 3056).
* refactor(engine): route sacrifice.rs delivery through zone pipeline proof token (Phase B)
apply_sacrifice_after_replacement delivered its inner (post-replacement)
graveyard ZoneChange — and the outer-redirect ZoneChange — with a bare
zones::move_to_zone. A sacrifice redirected to the battlefield (CR 614.6)
therefore dropped the delivery tail (CR 614.1c enters-with-additional-counter
statics, enter_tapped / enter_with_counters, post-replacement-continuation).
Seal each already-replaced ZoneChange via ApprovedZoneChange::approve_post_replacement
and deliver through zone_pipeline::deliver. The Sacrifice proposal carries no
source, so no exile-link context is attributed. A NeedsChoice from the tail
returns SacrificeApply::NeedsChoice (state.waiting_for already set) so the
caller pauses.
Adds a discriminating test (sacrifice_redirected_to_battlefield_applies_enters_with_counters_tail)
driving the real sacrifice_permanent pipeline with a Moved->Battlefield redirect
under a CR 614.1c additional-+1/+1-counter static — fails on the old raw move
(0 counters), passes through the tail.
CR 701.21a (line 3445), CR 614 / 614.6 (line 3072), CR 614.1c (line 3056).
* refactor(engine): route sba.rs lethal-damage delivery through zone pipeline proof token (Phase B)
The lethal-damage state-based-action destruction loop (check_lethal_damage)
delivered its inner (post-replacement) ZoneChange with a bare
zones::move_to_zone. A lethal-damage death redirected to the battlefield
(CR 614.6, Rest in Peace / "would die -> return" class) therefore dropped the
delivery tail (CR 614.1c enters-with-additional-counter statics, enter_tapped /
enter_with_counters, post-replacement-continuation).
Seal the already-replaced ZoneChange via ApprovedZoneChange::approve_post_replacement
(source carried from the Destroy event) and deliver through zone_pipeline::deliver.
Per CR 704.3, completing all SBAs may require a replacement choice surfaced by
the delivery tail (CR 614.12a Devour as-enters); the NeedsChoice arm pauses
exactly as the existing regeneration NeedsChoice arm does (state.waiting_for
already set by the tail).
Adds a discriminating test (sba_lethal_damage_redirected_to_battlefield_applies_enters_with_counters_tail)
driving check_lethal_damage directly with a Moved->Battlefield redirect under a
CR 614.1c additional-+1/+1-counter static — fails on the old raw move
(0 counters), passes through the tail (exactly 1).
CR 701.19b (line 3426), CR 704.3 (line 5457), CR 704.5g (line 5476),
CR 614 / 614.6 (line 3072), CR 614.1c (line 3056), CR 614.12a (line 3099).
* docs(engine): correct SBA delivery CR annotation to 704.5g + 614.6 (review follow-up)
701.19b is regeneration-via-static; the delivered event here is a
lethal-damage destruction (CR 704.5g) whose redirect is a replacement
(CR 614.6).
* refactor(engine): hoist token + B->B no-op guards into move_object preflight (Phase C0a, Risk #11)
The unified pipeline runs replace_event ahead of zones::move_to_zone's
delivery-time guards. A one-shot replacement could therefore be consumed
(last_effect_count, CR 616.1 choices) on a move the primitive then rejects as
a no-op. Hoist the two cheap, side-effect-free object-level guards
(CR 111.8 token-outside-battlefield cease-to-exist, CR 603.2g + CR 603.6a
Battlefield->Battlefield no-op) into move_object's preflight, before the
execute_zone_move replacement consult.
Behavior-neutral in Phase C0a (move_object has no production callers yet);
the guards take effect as the bucket-B effect sites migrate in C1+.
CR 111.8 (line 663), CR 603.2g (line 2573), CR 603.6a (line 2595).
* fix(engine): clear damage on lethal-damage death redirected to battlefield (Phase C0b, CR 614.5)
A lethal-damage SBA destruction redirected by a Moved replacement back to the
battlefield delivers a Battlefield->Battlefield ZoneChange. zones::move_to_zone's
CR 603.2g no-op guard returns before reset_for_battlefield_entry, so the
creature keeps its marked damage. The SBA fixpoint then re-derives lethal damage
and re-fires the destruction-replacement on every iteration (counter / event
stacking, capped at MAX_SBA_ITERATIONS=9) — a pre-existing CR 614.5 violation
(a replacement gets only one opportunity), surfaced visibly by the Phase B
delivery-tail routing because the enters-with-additional-counter static now
applies on each re-entry.
Root cause: the no-op guard is correct for a spurious self-loop (Coiling Oracle)
but the redirected death is a genuine leave-and-re-enter — a new object per
CR 400.7 whose damage is gone. After delivering the redirect, clear the marked
damage the no-op delivery left behind so the fixpoint sees no lethal damage and
the one-shot replacement is not re-applied.
Adds a discriminating test (sba_lethal_damage_redirect_to_battlefield_applies_counter_exactly_once)
driving check_state_based_actions with a Moved->Battlefield redirect under a
CR 614.1c additional-+1/+1-counter static: pre-fix the creature ends with
Some(9) counters (9 re-fires); post-fix exactly Some(1).
CR 614.5 (line 3069), CR 400.7 (line 1948), CR 603.2g (line 2573), CR 704.5g.
* fix(engine): route mill delivery through zone pipeline so Moved redirects fire (Phase C1)
mill::apply_mill_after_replacement delivered each milled card with a bare
zones::move_to_zone, which never proposed a per-card ZoneChange. Moved-level
redirects ("if a card would be put into a graveyard from anywhere, exile it
instead" — Rest in Peace / Leyline of the Void class) were therefore silently
dropped for milled cards: a milled card reached the graveyard even with Rest in
Peace in play (PLAN §8 Risk #1; confirmed bug).
Route each milled card through zone_pipeline::move_object, which proposes the
inner ZoneChange and consults the Moved replacements before delivery. The milled
card itself anchors the Effect cause (mill to a graveyard creates no exile-link
and a Moved replacement's valid_card is evaluated against the moved card, so
this matches the pre-pipeline raw attribution while enabling the consult).
A per-card NeedsChoice (CR 616.1 multi-replacement race on one milled card) is
parked (state.waiting_for already set) and stops the batch; no real card
produces this on a library->graveyard mill, so the resumable batch-continuation
(PLAN §5a / Risk #10) is deferred. Discriminating test asserts non-interactivity
via the mandatory RIP redirect.
Adds mill_honors_rest_in_peace_graveyard_to_exile_redirect: drives the real Mill
pipeline with a global graveyard->exile Moved replacement and asserts milled
cards land in EXILE (graveyard empty) — fails on the old raw move (graveyard).
Also removes the now-live #[allow(dead_code)] on zone_pipeline::move_object.
CR 701.17a (line 3402), CR 614.6 (line 3072), CR 616.1 (line 3169).
* fix(engine): resume mill batch across per-card replacement choices instead of stranding (Phase C1 review fix)
The C1 mill migration bailed with 'return Ok(())' when a per-card Moved
replacement surfaced a choice, on the false claim that no real card produces
one: the CR 616.1 materiality classifier treats ANY destination-redirecting
Effect::ChangeZone as Unconditional-material, so two simultaneously-applicable
graveyard->exile redirects (Rest in Peace + Leyline of the Void — a real,
common combination) prompt for ordering on EVERY milled card. Pre-fix the
first card's prompt never even surfaced (the pause set pending_replacement but
no caller parked waiting_for) and cards 2..N stranded in the library.
Implement the PLAN §5a batch continuation (option a):
- mill's per-card delivery loop now parks the prompt via
replacement::park_waiting_for and stashes the undelivered tail in the new
state.pending_mill_deliveries (PendingMillDeliveries { remaining,
destination }).
- handle_replacement_choice drains the parked tail after the chosen event
delivers (Execute arm, following the established pending-queue drain
pattern; Prevented arm analogous) via
mill::drain_pending_mill_deliveries, re-parking when the next card surfaces
its own prompt.
Discriminating test mill_under_two_graveyard_redirects_delivers_every_card_through_ordering_choices:
mills 3 cards under RIP + Leyline, answers each CR 616.1 ordering prompt, and
asserts ALL milled cards leave the library and end in exile with the tail
fully drained. Pre-fix it fails (no prompt surfaced; cards stranded).
CR 701.17a (line 3402), CR 616.1 (line 3169), CR 614.6 (line 3072).
* docs(engine): scope C0b damage scrub as degenerate self-redirect guard, not CR 400.7 re-entry (review fix)
The C0b comment justified clearing damage_marked via the CR 400.7 new-object
rule, contradicting the chosen delivery: the Battlefield->Battlefield no-op
means reset_for_battlefield_entry never ran, so incarnation epoch, summoning
sickness, counters, and entered_battlefield_turn are all stale — claiming a
new object while delivering a stale one is inconsistent.
Reword to the narrow reading: a 'remains on the battlefield instead of dying'
replacement is regeneration-shaped — CR 701.19a/b replaces destruction with
'remove all damage marked on it' while the permanent STAYS the same object —
so the two-field damage scrub matches that semantics without claiming a
new-object re-entry. Add a TODO at the site recording the staleness (and the
delivery tail's CR 614.1c counter re-application) as the open question for
when a real would-die->battlefield redirect card class appears: no card
currently parses to one (parser builds die->exile / shuffle-back;
Persist/Undying are dies-triggers), so the semantics decision is deliberately
not baked in on zero real cards. Test doc header updated to match.
Comment-only change; behavior unchanged (test still passes: exactly one
CR 614.1c counter per SBA fixpoint).
CR 701.19a/b (lines 3424/3426), CR 603.2g (line 2573).
* docs(engine): correct mill NeedsAuraAttachmentChoice stash comment (round-2 review P2)
The stash comment claimed reaching the aura-attachment-choice arm 'fails
loudly (undrained tail)'. It does not: an aura-host choice surfaces as
WaitingFor::ReturnAsAuraTarget, not the replacement-choice path, so
drain_pending_mill_deliveries (only invoked from handle_replacement_choice)
never fires for it. A stale pending_mill_deliveries would instead be silently
drained by the NEXT unrelated replacement-choice resume. The arm is dead code
for every parsed Mill today (Battlefield is not a Mill destination); document
the real behavior so a future battlefield-mill variant is understood as a bug
to surface rather than a self-healing path.
CR 303.4f (line 822).
* fix(engine): propagate mill per-card pause through apply_mill_after_replacement (round-2 review P1)
The nested Mill-event resume path clobbered a per-card mill park. When a
Mill-event replacement (e.g. a mill-doubler) resolves through a CR 616.1
ordering choice, handle_replacement_choice's Mill arm applied the accepted
event with 'let _ = apply_mill_after_replacement(...)', discarding the helper's
pause signal, then unconditionally reset state.waiting_for to Priority (~:392).
If deliver_mill_cards parked a per-card prompt inside that arm (two
simultaneously-applicable graveyard->exile redirects make every milled card
prompt for CR 616.1 ordering), the reset stranded the first paused card.
Plumb deliver_mill_cards's existing pause bool out through
apply_mill_after_replacement (now returns Result<bool, EffectError>; true =
fully delivered, false = parked) and early-return from the Mill arm on a pause,
mirroring the apply_etb_counters early-return precedent. EffectError has no
EngineError conversion at that arm, so the error is mapped to 'delivered'
(preserving the prior let _ swallow) and only the pause is acted on. mill::resolve
also bails before EffectResolved on a per-card pause so it doesn't emit a
resolution event over a parked prompt.
Reachable only with two simultaneous Mill-event replacements that both prompt,
which no parsed card produces, so a full runtime repro is impossible. The unit
test apply_mill_after_replacement_reports_per_card_pause_to_caller drives the
shared seam directly: under two graveyard->exile redirects the first milled
card surfaces a CR 616.1 prompt, asserting the helper returns false, leaves
waiting_for set to that prompt, and parks the tail — the exact contract the
Mill arm's early-return now depends on.
CR 701.17a (line 3402), CR 616.1 (line 3169), CR 614.6 (line 3072).
* docs(engine): confirm rad-counter mill read is park-safe (round-2 review P3)
rad_counters assesses CR 728.1 life loss from library_before (a pre-mill
snapshot) immediately after apply_mill_after_replacement. Document that this
read remains correct even if the mill parks mid-batch on a per-card CR 616.1
ordering choice: milled_ids is fixed by the post-replacement count against the
snapshot (identifying the correct top-N cards regardless of delivery routing),
and the per-card life-loss loop reads card type from state.objects, which is
zone-independent. The parked tail is delivered by the resume path; the snapshot
predates any delivery.
CR 728.1 (line 6239).
* fix(engine): route countered spell to graveyard/exile through zone pipeline (Phase C3)
counter::resolve and resolve_all moved a countered spell off the stack with a
raw zones::move_to_zone, which never proposed a per-card ZoneChange. Moved-level
graveyard redirects (Rest in Peace / Leyline of the Void: 'if a card would be
put into a graveyard from anywhere, exile it instead') were therefore silently
dropped for countered spells: a countered spell reached the graveyard even with
Rest in Peace in play (PLAN §8 Risk #3 — graveyard-redirect class, confirmed
bug).
Route the stack -> graveyard/exile move through zone_pipeline::move_object so
the inner ZoneChange is proposed and Moved replacements are consulted before
delivery. The exile-on-counter destination (CR 702.34a/127a/180a Flashback /
Aftermath / Harmonize) is a static destination rule, not a replacement, so it
is still selected before the consult. SpellCountered now fires immediately on
stack removal (the counter itself) rather than after the consequent move, so a
CR 616.1 ordering pause during delivery does not drop it. A single applicable
redirect never prompts; only two simultaneous redirects produce a CR 616.1
choice — that mass multi-card continuation is the Phase C4 class (bail on pause;
no parsed card combines mass counter with a double graveyard redirect today).
Discriminating test countered_spell_honors_rest_in_peace_graveyard_to_exile_redirect:
counters a spell with a global graveyard->exile Moved redirect on the
battlefield and asserts the spell ends in EXILE (graveyard empty). FAILS on the
pre-C3 raw move (spell reaches the graveyard, redirect dropped).
CR 608.2b (line 2807), CR 614.6 (line 3072), CR 616.1 (line 3169), CR 701.6a (line 3309).
* fix(engine): route mass bounce through zone pipeline + generalize batch continuation (Phase C4)
bounce::resolve_all delivered each mass-bounced permanent with a raw
zones::move_to_zone under a comment claiming 'no replacement-pipeline detour is
needed because mass-bounce events are not destruction events (CR 614.6 doesn't
apply here)'. That justification was wrong by citation: CR 614.6 governs
replacement semantics generally, and CR 614.1 replacements watch zone-change
*events*, not only destruction. The raw move never proposed a per-object
ZoneChange, so Moved redirects watching the bounce destination ('if a permanent
would be returned to a hand, exile it instead' class) silently never fired
(PLAN §8 Risk #4). The single-target bounce raw moves (battlefield/graveyard/
stack -> destination) had the same gap.
Route every bounce move through the pipeline. mass bounce uses a new shared
batch entry zone_pipeline::move_objects_simultaneously; the single-target and
non-targeted-single paths use move_object. The stale 614.6 comment is deleted.
Generalize the Phase C1 mill continuation rather than add a second parallel
struct (CLAUDE.md parameterize-don't-proliferate applies to state types too):
- PendingMillDeliveries -> PendingBatchDeliveries (identical { remaining,
destination } shape; serialized as a plain struct so the type rename is
wire-transparent; the GameState field pending_mill_deliveries ->
pending_batch_deliveries carries a serde field-name alias for save compat).
- New zone_pipeline::move_objects_simultaneously runs each request through
move_object, parks + stashes the undelivered tail on a CR 616.1 pause, and
stamps CR 603.10a co-departure over the departed subset on completion (a no-op
for non-battlefield origins, so mill reuses it unchanged).
- zone_pipeline::drain_pending_batch_deliveries replaces
mill::drain_pending_mill_deliveries; mill::apply_mill_after_replacement now
delegates to the shared batch entry, deleting its bespoke deliver_mill_cards.
- engine_replacement.rs drains pending_batch_deliveries from both the Execute
and Prevented resume arms.
A single applicable redirect never prompts (the realistic path), so the common
mass bounce never pauses. Only two simultaneous redirects on one object split a
batch; the co-departure stamp is then per delivered segment rather than threaded
across the pause boundary (no parsed card hits this — documented).
Two discriminating tests (bounce_destination_redirect.rs):
- mass_bounce_honors_to_hand_redirect: a single to-hand -> exile redirect sends
every mass-bounced creature to EXILE, not the hand. FAILS on the old raw move.
- mass_bounce_under_two_redirects_delivers_every_permanent_through_choices: two
simultaneous redirects make every bounced creature prompt a CR 616.1 ordering
choice; answering each delivers ALL of them (none stranded) and fully drains
the parked batch tail.
CR 614.6 (line 3072), CR 616.1 (line 3169), CR 603.10a (line 2634), CR 400.7 (line 1948).
* fix(engine): centralize replacement-choice park in move_object so paused single moves can't ghost (round-3 review Fix 1+2)
zone_pipeline::move_object did not park state.waiting_for on NeedsChoice:
replace_event sets only pending_replacement, and the wait-state was each
caller's to set. The C3/C4 single-move migrations (counter.rs x2, bounce.rs x3)
bailed with 'return Ok(())' under comments falsely claiming the pipeline parks.
Under two simultaneously-applicable graveyard->exile redirects (Rest in Peace +
Leyline of the Void, or two RIP copies — RIP is not legendary), a countered
spell was: removed from the stack, SpellCountered emitted, its move parked in
pending_replacement, and the prompt NEVER surfaced (the engine gates
ChooseReplacement on the wait state) — permanently ghosting the spell (off
state.stack but obj.zone == Stack).
Centralize the park at the single unparked origin: execute_zone_move's
replace_event NeedsChoice arm now calls replacement::park_waiting_for before
returning, making counter.rs, bounce.rs, seek.rs (latent same-class ghost — it
discards the result entirely), and every future C5-C9 single-move migration
safe by construction. Idempotence verified:
- park_waiting_for keeps the CR 614.12a devour guard (never clobbers an
already-surfaced EffectZoneChoice) and recomputes the identical
ReplacementChoice from the same pending_replacement, so callers that still
self-park (change_zone's park_waiting_for arms; end_phase /
exile_from_top_until's replacement_choice_waiting_for) double-set the same
value.
- sba.rs's self-parks are on its OWN replace_event calls (Destroy / inner
ZoneChange proposals), not downstream of move_object — unaffected today;
redundant-but-idempotent when C7 migrates it.
- deliver_batch's explicit park is removed (now redundant); the delivery-tail
NeedsChoice path is deliberately NOT parked here — its wait state is already
set by the counter-pause/devour machinery.
The false 'parks via the pipeline' comments are now true and updated to credit
the centralized park; two stale CR 608.2b citations corrected to CR 701.6a.
Discriminating test countered_spell_under_two_redirects_surfaces_prompt_and_exiles
(fail-first verified): counters a spell under RIP + Leyline, asserts the
CR 616.1 ordering prompt SURFACES, answers it via a real
GameAction::ChooseReplacement dispatch, and asserts the spell ends in EXILE with
no ghost (zone == Exile, exile container holds it, graveyard empty,
pending_replacement clear). Pre-fix run: FAIL with 'waiting_for = Priority {
player: PlayerId(0) }, spell zone = Stack' — the exact unparked-pause ghost.
CR 616.1 (line 3169), CR 614.6 (line 3072), CR 701.6a (line 3301).
* fix(engine): batch-continuation comment accuracy + rad-counter park bail + discard delivery re-bucket (round-3 review Fix 3)
(a) deliver_batch's aura-arm comment claimed a stale tail 'surfaces a bug'
(undrained). It does not: the engine_replacement drain gate keys on
pending_batch_deliveries.is_some(), not provenance, so a stale tail would be
silently drained by the NEXT unrelated replacement-choice resume — the same
mischaracterization the P2 review fix corrected in mill.rs. State the real
behavior.
(b) The mark_simultaneous_departures no-op-for-mill reasoning was wrong:
departed_subset (zones.rs:609) DOES include milled cards — it filters on
current zone != Battlefield, and a card now in a graveyard passes. The actual
no-op comes from the EVENT gate: mark_simultaneous_departures (zones.rs:580)
only stamps ZoneChanged events with from: Some(Zone::Battlefield), and a
library-origin move emits none. Both comments corrected.
(c) rad_counters now bails on a parked mill (apply_mill_after_replacement ->
false) like mill::resolve does: continuing into the CR 728.1 life-loss loop
would propose LifeLoss replacement events while the parked CR 616.1 choice is
pending and could overwrite pending_replacement, ghosting the paused milled
card. The life-loss/rad-removal tail is dropped on that parked path (reachable
only with two simultaneous graveyard redirects active while rad counters
trigger) — accepted and documented rather than threaded through the batch
resume.
(d) discard.rs complete_discard_to_graveyard re-bucketed with a comment: it is
a Bucket A post-replacement DELIVERY site (eventual Phase E shape:
approve_post_replacement + deliver on the lowered ZoneChange carrying applied),
not a move_object site — re-proposing would double-apply Discard-level
definitions. Deferred from this round because deliver's tail drains
post_replacement_continuation at delivery time with Moved context, and timing
equivalence for the Discard-execute chain class (madness / Abundance) needs its
own discriminating tests. Audit correction recorded at the site: the
discard_applier Discard->ZoneChange lowering runs only when a Discard-level
definition (madness class) applies — moved_matcher accepts only ZoneChange
proposals — so a PLAIN discard never consults Moved redirects and Rest in Peace
does not exile it today. That graveyard-redirect gap (same class as mill C1 /
counter C3 / bounce C4) needs the inner-ZoneChange-with-applied lowering plus a
discriminating RIP-on-discard test as its own commit.
CR 616.1 (line 3169), CR 728.1 (line 6239), CR 603.10a (line 2634), CR 701.9a (line 3334 area — see docs), CR 303.4f (line 1652).
* fix(engine): plain discard consults Moved redirects (CR 614.6 / 701.9a)
A plain discard moved hand -> graveyard via raw `move_to_zone`, never
consulting `Moved` replacements, so Rest in Peace / Leyline of the Void
did not exile a discarded card. Lower the accepted Discard into an inner
hand -> graveyard `ZoneChange` carrying the outer pass's `applied` set and
run it through the replacement pipeline, mirroring `discard_applier`'s
lowering. The `applied` set guards against re-running Discard-level
(madness) definitions; the lowered ZoneChange already re-loops through the
pipeline (CR 616.1f) for the madness path, so only the unmodified
Execute(Discard) arm needed the fix.
- complete_discard_to_graveyard now takes (source_id, applied), re-proposes
through replace_event, and returns DiscardOutcome so a Moved-redirect
CR 616.1 choice can propagate.
- Mayhem marker (CR 702.187b, record_card_discarded) stamped only when the
card actually reaches the graveyard (CR 701.9c) — redirects leave it
elsewhere, matching the Madness -> exile path.
- All four callers (resolve specific/non-specific, discard_as_cost,
handle_replacement_choice resume, ward-discard) updated to surface the
pause.
CR grep (docs/MagicCompRules.txt):
701.9a To discard a card, move it from its owner's hand to that player's graveyard.
701.9c If a card is discarded, but an effect causes it to be put into a hidden zone instead ...
614.6. If an event is replaced, it never happens. A modified event occurs instead ...
Discriminating tests (effects/discard.rs):
plain_discard_consults_rest_in_peace_and_exiles — RIP on board, plain
discard ends in exile not graveyard (failed on the old raw-move path).
madness_class_discard_still_works_without_double_consult — madness
redirect to exile fires exactly once (no double-consult).
* fix(engine): seek bail-on-pause + route non-battlefield move through pipeline (CR 616.1 / 614.6)
Seek ignored `execute_zone_move`'s result and always pushed
`EffectResolved`, so a per-card replacement pause was clobbered, and a
multi-card seek with consecutive pausing replacements overwrote
`pending_replacement` — ghosting earlier picks. It also moved
non-battlefield destinations via raw `zones::move_to_zone`, never proposing
a per-card ZoneChange (C9 entry), so `Moved` redirects never fired.
Replace the per-card loop with the shared batch entry
`zone_pipeline::move_objects_simultaneously` (mill::resolve pattern): both
destinations now route through the pipeline, attribution stays
`ability.source_id` (so battlefield entries record
`entered_via_ability_source` and exile-link tracking keys off the seek
source), and a mid-batch CR 616.1 pause parks `state.waiting_for` + stashes
the tail in `pending_batch_deliveries`. Bail before `EffectResolved` on
NeedsChoice so the prompt is not clobbered.
CR grep (docs/MagicCompRules.txt):
614.6. If an event is replaced, it never happens. A modified event occurs instead ...
(616.1 ordering among applicable replacements — pipeline_loop authority)
Discriminating test (effects/seek.rs):
seek_parks_on_per_card_replacement_choice_and_stashes_tail — two
simultaneous graveyard->exile redirects make the first seeked-to-graveyard
card prompt for CR 616.1 ordering; the batch parks, stashes the tail, and
EffectResolved is NOT emitted (old loop ran to completion over the parked
prompt).
* fix(engine): reveal_until battlefield entry routes through zone pipeline (CR 614.1c / 306.5b)
The kept-card battlefield entry used a raw `zones::move_to_zone`, skipping
the CR 614.1c delivery tail — so a revealed planeswalker/battle entered
with 0 loyalty/defense counters and was immediately put into the graveyard
by CR 704.5i. Route the entry through `zone_pipeline::move_object`: the
delivery tail seeds intrinsic enters-with counters, enters-with-counters
statics, and applies the CR 614.1 tap-state from the seeded EntryMods. The
previous manual `obj.tapped = true` is dropped (the tail does it — double-
application check). The `enters_attacking` combat placement (CR 508.4)
stays after delivery (not part of the zone tail). Bail on NeedsChoice /
NeedsAuraAttachmentChoice (centralized park in move_object).
The `move_to_library_position` rest-pile site (shuffle_to_bottom) is a
library-placement SIBLING raw mover, DEFERRED to Phase D with a comment:
move_object's placement arm is still a Phase-A stub that skips the
replacement consult, and a library→bottom reposition has no Moved-redirect
class to consult, so routing through it gains nothing now.
CR grep (docs/MagicCompRules.txt):
306.5b A planeswalker has the intrinsic ability "This permanent enters with a ..."
310.4b A battle has the intrinsic ability "This permanent enters with a number ..."
614.1c Effects that read "[This permanent] enters with . . . ," ...
704.5i If a planeswalker has loyalty 0, it's put into its owner's graveyard.
508.4. If a creature is put onto the battlefield attacking, its controller ch...
303.4f If an Aura is entering the battlefield under a player's control by any...
616.1. If two or more replacement and/or prevention effects are attempting to...
Discriminating test (effects/reveal_until.rs):
reveal_until_planeswalker_enters_with_intrinsic_loyalty — a loyalty-4
planeswalker revealed to the battlefield enters with 4 loyalty counters
(old raw path: 0 counters, dead by CR 704.5i).
* fix(engine): resolution-choice battlefield entries route through zone pipeline (CR 614.1c)
The RevealUntilKeptChoice-accept and DigChoice-kept handlers moved
battlefield-entry cards via raw `zones::move_to_zone` + a manual
`obj.tapped`, skipping the CR 614.1c delivery tail — so a kept/dug
planeswalker or battle entered with 0 loyalty/defense and died to
CR 704.5i. Route the battlefield branches through
`zone_pipeline::move_object` (consistent with the synchronous
reveal_until path migrated in C5): the tail seeds intrinsic enters-with
counters and applies the CR 614.1 tap-state from the seeded EntryMods, so
the manual tap is dropped. Bail on NeedsChoice / NeedsAuraAttachmentChoice
(centralized park; realistically unreachable for the dig classes).
`enters_attacking` (CR 508.4) combat placement stays post-delivery.
DigChoice now binds `source_id` for CR 400.7 attribution (falls back to the
moved object when None, matching the pre-pipeline raw move).
Non-battlefield resolution-choice moves (manifest/surveil/dig rest to
graveyard, route_rest_partition) are left raw this round — they sit inside
multi-card loops with post-loop cleanup (revealed-marker clearing,
continuation drain) where a per-card Moved-redirect pause cannot simply
`return` without stranding the rest; migrating them needs the batch-entry +
continuation restructure and is deferred. Library-placement sibling sites
(move_to_library_position / move_to_library_at_index) stay deferred to
Phase D.
CR grep (docs/MagicCompRules.txt):
306.5b A planeswalker has the intrinsic ability "This permanent enters with a ..."
310.4b A battle has the intrinsic ability "This permanent enters with a number ..."
614.1c Effects that read "[This permanent] enters with . . . ," ...
704.5i If a planeswalker has loyalty 0, it's put into its owner's graveyard.
508.4. If a creature is put onto the battlefield attacking, its controller ch...
400.7. An object that moves from one zone to another becomes a new object wit...
616.1. If two or more replacement and/or prevention effects are attempting to...
303.4f If an Aura is entering the battlefield under a player's control by any...
Discriminating test (effects/reveal_until.rs):
reveal_until_kept_choice_planeswalker_enters_with_loyalty — drives the
RevealUntilKeptChoice accept handler; a loyalty-5 planeswalker enters with
5 loyalty counters (old raw handler: 0, dead by CR 704.5i).
* fix(engine): non-destroy SBA deaths consult Moved redirects (CR 704.5 / 614.6)
The non-destroy state-based-action graveyard moves (zero toughness, zero
loyalty, zero defense, legend-rule loser, unattached aura, battle without a
protector, final-chapter Saga sacrifice) used a bare `zones::move_to_zone`,
skipping the CR 614.6 replacement consult. These are "leaves the
battlefield" / "dies" events (CR 603.6c + CR 700.4), so a `Moved`
graveyard->exile redirect (Rest in Peace / Leyline of the Void) must apply
— it did not.
Add a shared `move_to_graveyard_via_pipeline` helper that routes each
SBA-departing permanent through `zone_pipeline::move_object` with
`ZoneChangeCause::StateBasedAction`. It returns `true` (and the caller
bails) on a CR 616.1 ordering pause, mirroring the established
`check_lethal_damage` regeneration-pause arm; the CR 704.3 fixpoint re-runs
after the choice resolves and re-derives any undelivered SBA deaths, so
bailing strands nothing. The self-parks at the destroy loop remain
redundant-but-idempotent with the centralized park in move_object.
CR grep (docs/MagicCompRules.txt):
704.5f If a creature has toughness 0 or less, it's put into its owner's graveyard.
704.5i If a planeswalker has loyalty 0, it's put into its owner's graveyard.
704.5j If two or more legendary permanents with the same name are controlled ...
704.5m If an Aura is attached to an illegal object or player, or is not attac...
704.5s If the number of lore counters on a Saga permanent ...
704.5v If a battle has defense 0 ...
704.5w If a battle has no player in the game designated as its protector ...
603.6c Leaves-the-battlefield abilities trigger when a permanent moves from t...
700.4. The term dies means "is put into a graveyard from the battlefield."
614.6. If an event is replaced, it never happens. A modified event occurs instead ...
616.1. If two or more replacement and/or prevention effects are attempting to...
Discriminating test (sba.rs):
sba_zero_toughness_death_consults_rest_in_peace_and_exiles — a
zero-toughness creature with RIP on the battlefield is exiled, not put into
the graveyard (old bare-move path: graveyard).
* docs(engine): document C8/C9 zone-pipeline deferral — no Moved class targets Hand/Exile/Stack (PLAN Risk #5/#8)
C8 (casting_costs/engine.rs cost sites) and C9 (Hand/Exile 1-site tail) are
analyzed and deferred: the parser and synthesis only ever emit `Moved`
redirects with `destination_zone` Graveyard (Rest in Peace / Leyline class)
or Battlefield (ETB modifiers) — verified by grep of
oracle_replacement.rs `destination_zone(Zone::...)`. There is NO `Moved`
redirect class targeting a Hand, Exile, or Stack destination, so routing
those moves through `move_object` would consult nothing and only add an
unresumed-pause path to cost-payment / draw / dig flows that do not model a
mid-flow replacement-choice resume. Draw-level replacements already apply at
`ReplacementEvent::Draw` upstream.
Records the finding inline at draw.rs:209 (the PLAN Risk #5 audit anchor),
covering the whole Hand/Exile C9 tail (gift_delivery, connive, explore,
turns return-to-hand; haunt, discover, exile_top, cascade, collect_evidence,
ripple). seek.rs:97 and mill.rs:108 were migrated in earlier rounds (D2 / C1);
discard.rs:38 stays Bucket A (Phase E).
CR grep (docs/MagicCompRules.txt):
614.6. If an event is replaced, it never happens. A modified event occurs instead ...
No behavioral change; comment-only. No discriminating test (the deferral is
the absence of a behavioral change to test).
* fix(engine): scope self-ETB Moved replacements to battlefield entry (CR 614.1c)
Self-scoped as-enters replacements ("~ enters with N counters", enters
tapped/prepared, as-enters choices, enter-as-copy, Karoo/shock/reveal lands;
plus the Fading/Vanishing, Modular, Sunburst, Graft, Bloodthirst, Devour,
Amplify keyword synthesizers) parsed/synthesized as
ReplacementEvent::Moved with valid_card(SelfRef) and NO destination_zone.
moved_matcher skips the destination gate when destination_zone is None, and
a battlefield permanent is in-scan for its OWN departure — so the def
matched the permanent's own battlefield EXIT: phantom counters +
CounterAdded events on corpses (SBA deaths, bounce, destroy/sacrifice), a
spurious CR 616.1 ordering prompt under a single Rest in Peace, and
Optional clone defs forcing an "enter as a copy?" prompt on death.
Fix is data-shape, not matcher special-casing: stamp
.destination_zone(Zone::Battlefield) at construction on every self-ETB
Moved def — CR 614.1c defs ("[This permanent] enters with...") are
definitionally battlefield-entry-scoped. Parser sites: enters-tapped
(unconditional/unless/if-controls), Karoo pay-cost, enters-prepared,
reveal-land, shock land, as-enters-choose, clone, counter-choice, and the
enters-with-counters builder (previously stamped only the is_external
ChangeZone branch). Synthesis sites: Fading/Vanishing, Modular, Sunburst,
Graft, Bloodthirst, Devour, Amplify (Riot/Unleash/Siege/Tribute/Saga were
already stamped). The unearth / "would leave the battlefield" departure
watchers stay destination-agnostic by design.
CR grep (docs/MagicCompRules.txt):
614.1c Effects that read "[This permanent] enters with . . . ," "As [this per...
614.6. If an event is replaced, it never happens. A modified event occurs ins...
616.1. If two or more replacement and/or prevention effects are attempting to...
Discriminating tests (fail-first evidence captured pre-fix):
sba.rs::sba_death_does_not_apply_own_enters_with_counter_replacement
— FAILED pre-fix: phantom counter on corpse (left: 1, right: 0)
sba.rs::sba_death_under_single_rip_exiles_directly_no_prompt_no_counters
— FAILED pre-fix: spurious CR 616.1 ordering prompt
bounce.rs::bounce_does_not_apply_own_enters_with_counter_replacement
— FAILED pre-fix: phantom counter on bounced card (left: 1, right: 0)
All three drive parse_replacement_line (real parser output), then the SBA /
bounce pipeline.
* docs(engine): correct C8/C9 deferral rationale at draw.rs (destination-None Moved class)
The recorded rationale ("no Moved class targets Hand/Exile") was incomplete:
self-ETB Moved constructors carried NO destination_zone, and None matches
every destination — so pre-Fix-1 such defs DID match Hand/Exile deliveries.
The deferral conclusion stands (and was reinforced: migrating C8/C9 before
the destination stamps landed would have widened the phantom-application
defect). Post-stamp, explicit destinations are Graveyard/Battlefield only
and the remaining destination-None defs are deliberate battlefield-departure
watchers (unearth class), so the original claim is now true. Comment-only.
* fix(engine): prevented discard records nothing; document paused-path bookkeeping gap (CR 614.6 / 701.9a)
Prevented arm: a prevented inner ZoneChange means the card never left the
hand — per CR 701.9a (to discard = move hand -> graveyard) NO discard
occurred, so skip record_discard / the CR 702.187b Mayhem stamp / the
Discarded event. Previously the arm fell through and recorded+emitted a
discard that never happened, incoherent with the NeedsChoice early-return.
Distinct from a REDIRECTED discard (CR 701.9c: still discarded — the
Execute and madness arms keep recording+emitting).
NeedsChoice arm: documented gap (counter.rs resolve_all style) instead of a
resume-side continuation — a CR 616.1 ordering pause on the inner move (TWO
materially-different Moved redirects on one discard, e.g. RIP + Wheel of
Sun and Moon) delivers the card via the generic ZoneChange resume with no
discard context, skipping "whenever you discard" bookkeeping for that card
and abandoning a multi-card remainder. A proper fix needs a new serialized
state slot + resume wiring; not built for a board no parsed deck assembles.
Single-redirect boards (RIP alone) never prompt and are fully correct.
CR grep (docs/MagicCompRules.txt):
614.6. If an event is replaced, it never happens. A modified event occurs ins...
701.9a To discard a card, move it from its owner's hand to that player's grav...
701.9c If a card is discarded, but an effect causes it to be put into a hidde...
* fix(engine): batch-tail re-stash preserves request mods + attribution (CR 400.7 / 614.1c)
PendingBatchDeliveries rebuilt paused-batch tails as
ZoneMoveRequest::effect(obj, dest, obj), dropping the seek flow's
enter_tapped mod and ability-source attribution across the pause boundary
(seek is the first batch caller passing non-default EntryMods + a shared
source). Extend the stash with serde-default fields (source_id,
enter_tapped, exile_tracking) carrying the batch-uniform request context:
captured from the first tail request in the new stash_batch_tail helper
(source equal to the request's own object_id is the mill self-anchor idiom
and stashes None), re-applied per request in drain_pending_batch_deliveries.
Batch-uniform rather than per-request, mirroring the single-destination
batch design; per-card heterogeneity remains a flagged design extension.
Test: seek_parks_on_per_card_replacement_choice_and_stashes_tail extended to
assert the stashed tail preserves the seek's ability-source attribution.
* feat(engine): scan stack-resident object's own Moved redirect on stack exit (CR 608.2n / 614.12)
find_applicable_replacements scanned only [Battlefield, Command] plus the
entering-object (to:Battlefield) and discard exceptions, so a spell's own
self-scoped `Moved` replacement was never discovered for its stack ->
graveyard move. Add a stack-self-move exception mirroring the entering-
object exception: when the proposed event is a ZoneChange whose `from` is
Stack, include the moving object itself as a candidate source so its own
SelfRef-scoped Moved def can fire as it leaves the stack.
Hot-path cost: a single per-event Option match on the event's `object_id`
when `from == Stack` (no extra zone sweep); the loop iterates the same
`active_replacements` set as before, this only lets that one object pass
the zone gate, and the `is_stack_self_move && !in_scanned_zone` SelfRef
guard keeps it scoped to that object's own definitions.
Inert until a stack -> graveyard self-redirect def is installed (next
commit wires the Invoke Calamity rider through it): no parsed self-scoped
Moved def today carries destination_zone: Graveyard, and the existing
enters-with-counters self-defs are scoped to destination_zone: Battlefield.
CR grep (docs/MagicCompRules.txt):
608.2n As the final part of an instant or sorcery spell's resolution, the spell is put into its owner's graveyard.
614.12. Some replacement effects modify how a permanent enters the battlefield.
* fix(engine): route stack resolution defaults through pipeline; replace exile-rider flag with synthetic Moved def (CR 608.2n / 614.6)
The stack resolution-default moves (resolved instant/sorcery → graveyard,
fizzled/countered-on-resolution spell, prevented permanent → graveyard)
delivered via raw `move_to_zone`, never proposing the inner ZoneChange, so
board-wide `Moved` graveyard→exile redirects (Rest in Peace / Leyline of
the Void) silently dropped on resolved/countered/prevented spells (PLAN §8
Risk #2 — confirmed bug). Route all three through `zone_pipeline::move_object`
(new `ZoneMoveRequest::spell_resolution_default`, Cause::SpellResolutionDefault).
The Invoke Calamity free-cast "if this spell would be put into your
graveyard, exile it instead" rider was a bespoke per-object boolean
(`exile_from_stack_instead_of_graveyard`) read by hand at the two dest
computations. Replace it with a synthetic self-scoped `Moved`
replacement (valid_card: SelfRef, destination_zone: Graveyard, execute:
ChangeZone → Exile) installed at the same attach point — the same class as
RIP/Leyline, just scoped to one spell — so the pipeline applies it like
any other Moved redirect. Delete the flag, its game_object field, and the
`object_exiles_instead_of_graveyard` reader. The flag may appear in saved
games; GameObject has no deny_unknown_fields, so serde ignores the legacy
field on deserialize (an old mid-cast save lacks the synthetic def — the
def travels with the object via replacement_definitions; acceptable).
Flashback/aftermath/harmonize exile-on-stack-exit stays a STATIC
destination rule selected pre-pipeline (dest = Exile), so its proposed
move is Stack→Exile; the Graveyard-scoped redirect class never matches it
— no double-apply. CR 616.1 ordering pauses (two simultaneous redirects)
are parked by move_object; each site emits the standard StackResolved +
trigger-context-clear epilogue and bails for the replacement-choice resume.
CR grep (docs/MagicCompRules.txt):
608.2n As the final part of an instant or sorcery spell's resolution, the spell is put into its owner's graveyard.
608.3e If a permanent spell resolves but its controller can't put it onto the battlefield, that player puts it into its owner's graveyard.
614.1a Effects that use the word "instead" are replacement effects.
614.6. If an event is replaced, it never happens. A modified event occurs instead.
Discriminating tests:
stack.rs::rest_in_peace_exiles_resolved_instant — RIP redirects a
resolved instant's graveyard move to exile (fails on old raw delivery).
stack.rs::flashback_spell_exiles_once_with_rest_in_peace_present —
flashback exiles exactly once via its static rule; RIP (graveyard-scoped)
does not double-apply on the stack→exile move.
casting_costs.rs::invoke_calamity_rider_exiles_free_cast_spell_on_resolution —
the rider's synthetic Moved def redirects a free-cast spell to exile on
resolution through the deleted-flag's replacement path.
* fix(engine): paused fizzle runs the resolution epilogue before bailing (CR 608.2b / 616.1)
The fizzle/countered-on-resolution arm bailed on a replacement-ordering
pause with a bare `return`, skipping the epilogue directly below it
(StackResolved emission + current_trigger_event / current_trigger_events /
current_trigger_match_count / die_result_this_resolution clears) — leaking
stale cross-resolution context across the pause and never emitting
StackResolved. The other two C2 sites (instant→graveyard else-branch,
prevented-permanent arm) inline that epilogue before their pause-returns;
the fizzle arm now falls through to its shared epilogue instead (the
pause needs nothing else from the fizzle path — `move_object` parked the
prompt and the move, and the resume path delivers it).
Reachable: an Invoke Calamity free-cast spell fizzling under a single
Rest in Peace = rider + RIP = two simultaneous graveyard→exile redirect
candidates = CR 616.1 ordering prompt = this arm.
Also documents the rider's known scope gap at the install site: the
synthetic def carries no "this turn" duration (ReplacementDefinition has
no duration field; revert_layered_characteristics_to_base only runs for
battlefield exits), behavior-preserving vs the deleted flag — a Duration
field on ReplacementDefinition is the eventual fix.
CR grep (docs/MagicCompRules.txt):
608.2b If the spell or ability specifies targets, it checks whether the targets are still legal ...
616.1. If two or more replacement and/or prevention effects are attempting to modify the way an event affects an object or player ...
Discriminating test (fail-first verified):
casting_costs.rs::invoke_calamity_rider_fizzle_under_rip_parks_choice_with_clean_epilogue —
free-cast spell with the rider fizzles under one RIP → CR 616.1 prompt
parks → StackResolved emitted + all four context fields cleared on the
paused path → GameAction::ChooseReplacement resumes → spell exiled.
Pre-fix failure: panicked at "paused fizzle must still emit
StackResolved" (prompt assertion passed, proving reachability).
* fix(engine): route surveil + manifest-dread rest piles through zone pipeline (Phase C6)
The non-battlefield rest-pile loops in engine_resolution_choices.rs delivered
every unkept card with a bare zones::move_to_zone(.., Zone::Graveyard, ..),
proposing no per-card ZoneChange — so each card's own Moved redirects (Rest in
Peace / Leyline of the Void: 'would be put into a graveyard from anywhere ->
exile instead') silently never fired (PLAN Risk #1 class). The post-loop
cleanup (surveil kept-on-top reorder; manifest-dread reveal-marker removal) ran
inline at the end of the loop, which is why C6 was deferred twice: a per-card
CR 616.1 pause mid-pile would run that cleanup before the paused tail finished,
then never again.
Route both piles through a new zone_pipeline::move_objects_simultaneously_then,
which carries a typed BatchCompletion (NOT a closure) describing the post-loop
work. The batch runs the cleanup exactly once on true completion: inline on the
synchronous (single-redirect) path, and via drain_pending_batch_deliveries when
a CR 616.1 ordering choice pauses the pile. BatchCompletion rides on the parked
PendingBatchDeliveries tail (mirroring PendingCounterPostAction), and the drain
re-attaches it across each re-park so it can never run early or twice. The
paused-on-last-card empty-tail case is handled by ensure_batch_record so the
completion still fires after the final card's redirect resolves.
CR grep (docs/MagicCompRules.txt):
701.25a To 'surveil N' means to look at the top N cards of your library, then put any number of them into your graveyard and the rest on top of your library in any order.
614.6. If an event is replaced, it never happens. A modified event occurs instead ...
616.1. If two or more replacement and/or prevention effects are attempting to modify ...
603.10a Some zone-change triggers look back in time. These are leaves-the-battlefield abilities ...
Discriminating tests (surveil_rest_pile_redirect_continuation.rs):
surveil_rest_pile_honors_graveyard_exile_redirect — single redirect: unkept
cards land in EXILE (not graveyard), kept card stays on top once. FAILS on
the old raw move (cards reached the graveyard).
surveil_rest_pile_under_two_redirects_runs_keep_on_top_cleanup_once — two
simultaneous redirects pause every unkept card on a CR 616.1 prompt;
answering each delivers ALL to exile (none stranded), drains the parked
tail, and runs the kept-on-top reorder EXACTLY once (kept card on top,
present once — not duplicated, not missing).
* refactor(engine): route exempt-cause moves through zone pipeline (Phase D)
Migrate the four CR-exempt zone-change classes off raw zones::move_to_zone onto
the single pipeline entry under explicit exempt ZoneChangeCause variants, so
Phase E can flip enforcement knowing every production caller goes through the
pipeline. The exempt path seals the proposed event directly and delivers it
WITHOUT the replace_event consult (the 'would'-semantics layer); the
unconditional primitive guards (CR 111.8 token, CR 614.1d ETB block, CR 400.7
cleanup) still run in zones.rs delivery for every cause (PLAN section 2/3).
Sites:
- mulligan.rs (CR 103.5 PregameProcedure): Serum Powder hand->exile, mulligan
hand->library shuffle, pregame draw, and the two opening-hand/mulligan
bottoming loops (via the library-placement arm).
- elimination.rs (CR 800.4a PlayerLeftGame): owner's objects -> exile. 'This is
not a state-based action' and no replacement applies to a player leaving.
- casting_costs.rs::finalize_cast (CR 601.2a CastingToStack): the committed
Hand/GY/Exile/Command -> Stack transition. Part of the casting process, not a
replaceable event.
- engine_debug.rs (DebugCommand): MoveToZone (incl. library top/bottom/Nth via
placement), Mill, and the no-ETB battlefield staging path. Operator intent is
'force the state'; no intrinsic enters-with-counter seeding, matching the
prior raw placement.
ZoneChangeCause::is_exempt() is the single authority for the consult skip; each
exempt arm carries its CR citation. New ZoneMoveRequest constructors
(casting_to_stack / pregame / player_left_game / debug) keep call sites short.
Removed now-unused 'use super::zones' in mulligan.rs.
CR grep (docs/MagicCompRules.txt):
103.5. Each player draws a number of cards equal to their starting hand size ... A player who is dissatisfied with their initial hand may take a mulligan ... shuffles the cards in their hand back into their library ... puts a number of those cards ... on the bottom of their library
601.2a To propose the casting of a spell, a player first moves that card (or that copy of a card) from where it is to the stack.
800.4a When a player leaves the game, all objects (see rule 109) owned by that player leave the game ... This is not a state-based action.
No behavior change: exempt causes already bypassed the consult via raw moves;
this routes them through the pipeline with identical delivery. Existing mulligan
/ elimination / debug / casting tests stay green (test-engine ok).
* docs(engine): document W3 library-placement consult deferral with evidence (Phase D)
The placement-aware pipeline arm exists as a stub (move_object delivers a
Some(placement) request directly via move_to_library_at_index, skipping the
replacement consult). Phase D's exempt pregame/debug library callers already
route through it. Completing the stub so it RUNS the consult on a library
placement (PLAN section 3.5 'the consult should still run for future-proofing')
is DEFERRED with evidence:
- Zero value today: no Moved replacement in the card pool targets
destination_zone(Library). Verified destination distribution:
25 destination_zone(Zone::Battlefield)
17 destination_zone(Zone::Graveyard)
2 destination_zone(Zone::Exile)
0 Library
so the consult is a guaranteed no-op on every library placement.
- Real risk for that no value: a correct completion must gate the CR 701.24a
delivery-tail auto-shuffle on placement-absence across the shared deliver /
deliver_replaced_zone_change / DeliveryCtx signatures (a library *placement*
must NOT shuffle; a plain library-destination ZoneChange MUST). That is a
cross-cutting change to every bucket-A/B/C delivery caller with a
silent-randomization landmine (a wrong gate randomizes put-on-top / scry /
cascade placements with no crash or test failure).
The raw move_to_library_position / _at_index sibling production callers
(put_on_top, cascade, discover, reveal_until, drawn_this_turn_choice,
engine_resolution_choices) stay on the raw movers until that completion: a
library reposition is not 'put into a graveyard/exile/hand', so nothing is
skipped by staying raw. Migrating them onto the stub arm ahead of the consult
completion is value-neutral churn that does not advance Phase E (which can only
demote the library movers once ALL callers AND the consult are in place) and
carries per-site top-order-reversal preservat…
ntindle
added a commit
to ntindle/phase
that referenced
this pull request
Jun 27, 2026
…and-play frequency, text/lower desync, context restore Maintainer @matthewevans raised four issues on PR phase-rs#4341 (The Fourth Doctor parser cluster). All four are addressed here. 1. Reflexive trigger provenance (issue #1): Replace the rules-incorrect `TriggerMode::PlayCard` trigger with a `TriggerMode::Unknown` gap marker. A global PlayCard trigger cannot distinguish which permission authorized a given play (CR 603.12), so the "When you do" rider was firing even when a different top-of-library permission authorized the play. The Unknown trigger keeps the gap visible in coverage until the casting/land-play pipeline gains permission-provenance tracking. 2. Land-play frequency consumption (issue phase-rs#2): Add `record_top_of_library_land_permission` in engine.rs and call it from all three completion arms of `handle_play_land`. A `OncePerTurn` `TopOfLibraryCastPermission { play_mode: Play }` now consumes the per-turn slot on land plays, mirroring how `finalize_cast` records this for spell casts (CR 401.5 + CR 601.2a). 3. Context mutation without restore (issue phase-rs#3 / Gemini): The reflexive rider is no longer parsed at all (Unknown trigger is emitted unconditionally), eliminating the ctx.subject/ctx.actor mutation that was leaking into subsequent lines. 4. text/lower desync on frequency-prefix strip (issue phase-rs#4 / Gemini): In `try_parse_top_of_library_cast_permission`, shadow both `text` and `lower` together when stripping the "once each turn, " / "once during each of your turns, " prefix, so downstream helpers receive aligned slices (Gemini phase-rs#3 suggestion, CR 601.2a). 5. Redundant scan_contains pre-check removed (Gemini phase-rs#2): The `scan_contains` guard before `split_once_on_lower` in oracle.rs was redundant — `try_parse_top_of_library_cast_permission` already validates the anchor. Removed for clarity. Test: `the_fourth_doctor_full_card_parse` updated to expect `TriggerMode::Unknown` instead of `TriggerMode::PlayCard`. Verification: cargo clippy -p engine --all-targets -- -D warnings: clean. cargo test -p engine: all pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
matthewevans
added a commit
to ntindle/phase
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 phase-rs#2): Add `record_top_of_library_land_permission` in engine.rs and call it from all three completion arms of `handle_play_land`. A `OncePerTurn` `TopOfLibraryCastPermission { play_mode: Play }` now consumes the per-turn slot on land plays, mirroring how `finalize_cast` records this for spell casts (CR 401.5 + CR 601.2a). 3. Context mutation without restore (issue phase-rs#3 / Gemini): The reflexive rider is no longer parsed at all (Unknown trigger is emitted unconditionally), eliminating the ctx.subject/ctx.actor mutation that was leaking into subsequent lines. 4. text/lower desync on frequency-prefix strip (issue phase-rs#4 / Gemini): In `try_parse_top_of_library_cast_permission`, shadow both `text` and `lower` together when stripping the "once each turn, " / "once during each of your turns, " prefix, so downstream helpers receive aligned slices (Gemini phase-rs#3 suggestion, CR 601.2a). 5. Redundant scan_contains pre-check removed (Gemini phase-rs#2): The `scan_contains` guard before `split_once_on_lower` in oracle.rs was redundant — `try_parse_top_of_library_cast_permission` already validates the anchor. Removed for clarity. Test: `the_fourth_doctor_full_card_parse` updated to expect `TriggerMode::Unknown` instead of `TriggerMode::PlayCard`. Verification: cargo clippy -p engine --all-targets -- -D warnings: clean. cargo test -p engine: all pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR * fix(PR-4341): capture top-of-library land permission before zone change The `record_top_of_library_land_permission` helper was called in the land-play epilogue — after `zone_pipeline::deliver` had already moved the land from Library → Battlefield. At that point `top_of_library_permission_source` reads `player.library.front()`, which now points to the *next* card, so `top_id != object_id` always triggered and the slot was silently left unconsumed. A `OncePerTurn` top-of-library `Play` permission (The Fourth Doctor shape) could therefore be reused indefinitely for lands. Fix: capture `(src_id, frequency)` from `top_of_library_permission_source` *before* the replacement pipeline, alongside the existing `in_library_with_permission` eligibility check. The simplified `record_top_of_library_land_permission` now accepts the pre-captured pair instead of re-deriving it post-delivery. All three epilogue paths (Execute + `NeedsChoice` replacement prompt, `NeedsChoice` directly, and the normal path) use the captured value. Also adds `once_per_turn_library_land_play_consumes_slot_and_blocks_second_play`, a production-path regression test that plays two historic lands from the top of the library under a `OncePerTurn` `play_mode: Play` permission, asserts the first play stamps `top_of_library_cast_permissions_used`, and asserts the second play is rejected — the discriminating assertion that the pre-capture bug made impossible to satisfy. Addresses matt's round-4 review blocker (2026-06-28). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR * fix(CR-annotations): replace CR 601.2a with CR 305.1/116.2a/401.5 on land-play paths Matt's 2026-07-01 review (CHANGES_REQUESTED) identified that all six CR 601.2a annotations in the top-of-library land-play path cite the spell-casting-to-stack procedure (601.2a) instead of the rules that actually govern this path: - CR 305.1: playing a land is a special action, not a spell cast - CR 116.2a: playing a land puts the card onto the battlefield from the zone it was in (zero stack involvement) - CR 401.5: top-of-library visibility closes after the special action Fixed locations: - engine.rs: doc-comment on record_top_of_library_land_permission (×1) - engine.rs: inline comment in handle_play_land pre-capture block (×1) - engine.rs: three call-sites in the success/NeedsChoice/fallthrough branches of handle_play_land that record the permission slot (×3) - engine_tests.rs: doc-comment on the once-per-turn slot regression test (×1) All six now cite CR 305.1 + CR 116.2a + CR 401.5 with explanatory text. The two remaining mentions of CR 601.2a in the updated comments are explicit "does not apply here" notes, which is the correct documentation. Verification: cargo clippy -p engine --all-targets -- -D warnings (clean), cargo test -p engine (all pass), cargo fmt --all (no changes). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
RenzoMXD
added a commit
to RenzoMXD/phase
that referenced
this pull request
Jul 4, 2026
…-rs#4886) Address [MED] review finding phase-rs#4 on phase-rs#4938: the seed could still be cleared before a paused repeat-this-process continuation re-entered and emitted later token proposals. Root cause: drain_pending_continuation cleared post_replacement_token_choice_applied BEFORE calling drain_pending_repeat_until; that drain re-enters resolve_ability_chain (effects/mod.rs:721 / :744) and can propose further tokens, which then lost the inherited replacement id and re-prompted the same Jinnie replacement — the loop, reached via a repeat-until pause. Fix: treat pending_repeat_until as part of the originating continuation frame. Drain it first, then clear the seed only at true full-drain — Priority + no pending_continuation + no pending_repeat_iteration + no pending_repeat_until. The seed now outlives every drain in the function. cargo fmt, cargo clippy -p engine --all-targets -- -D warnings, and the full engine lib suite (14873 tests) pass. New regression pins the invariant: a pre-seeded applied set survives a pending_repeat_until drain that re-prompts the controller.
davidomil
pushed a commit
to davidomil/phase
that referenced
this pull request
Jul 5, 2026
…4886) (phase-rs#4938) * fix(engine): stop Jinnie Fay token replacements from looping (phase-rs#4886) * Fix nested token replacement continuation * fix(engine): scope token-choice applied seed to its originating ChooseOneOf (phase-rs#4886) Address [HIGH] review finding on phase-rs#4938: the inherited replacement-applied seed could still be dropped across an intervening nested replacement choice inside the paused branch chain. Root cause: the seed lived as a single transient GameState field cleared at TWO sites that cannot distinguish an outer (originating) token-choice drain from a nested replacement's drain: - continue_replacement_impl's `_ => None` arm wiped it for every non-token-choice nested replacement (replacement.rs) - apply_pending_post_replacement_effect cleared it whenever waiting_for was not ChooseOneOfBranch (engine_replacement.rs) So a Jinnie branch that created a token, paused on another optional ReplacementChoice, then resumed a later token sub-ability lost the originating replacement id and re-prompted the same Jinnie replacement — the exact loop this is trying to prevent. Fix: make the seed OWNED by the originating token-choice ChooseOneOf continuation. Seed it only when a CreateToken event is replaced by a token-choice continuation; preserve it across all other replacement activity; clear it only when that originating ChooseOneOf fully drains back to priority (effects/choose_one_of.rs). Remove both clobbering sites. Document the ownership on the field. cargo fmt, cargo clippy -p engine --all-targets -- -D warnings, and the full engine lib suite (14679 tests) pass. New regression pins the invariant: a pre-seeded applied set survives an intervening non-token-choice continuation drain. * fix(engine): clear token-choice seed at full-drain, not ChooseOneOf completion (phase-rs#4886) Address [HIGH] review finding phase-rs#3 on phase-rs#4938: the seed could still be cleared by an intervening nested choice before a later token sub-ability drained. Root cause: choose_one_of.rs cleared the global seed the moment its branch resolved back to priority. But a branch shaped `ChooseOneOf(non-token) -> sub_ability Token` stashes the token sub-ability into pending_continuation (effects/mod.rs), which the ChooseBranch handler drains only afterward (engine_resolution_choices.rs::drain_pending_continuation). Clearing at ChooseOneOf completion wiped the seed in the gap between the nested choice resolving and the stashed token sub-ability proposing, so the later token lost the inherited replacement id and re-prompted the same Jinnie replacement — the loop, reached via a non-token ChooseOneOf instead of a ReplacementChoice. Fix (per review's 'or otherwise clear only after that frame and its stashed continuation have drained'): remove the choose_one_of.rs clear entirely; clear the seed at the true full-drain point in drain_pending_continuation — Priority + no pending_continuation + no pending_repeat_iteration. By then the originating token-choice frame and every stashed sub-ability have completed, so no token proposal can still need the seed. cargo fmt, cargo clippy -p engine --all-targets -- -D warnings, and the full engine lib suite (14828 tests) pass. New regression pins the exact shape: a Jinnie branch that parks on a non-token ChooseOneOf before a token sub-ability. * fix(engine): clear token-choice seed after repeat-until drains (phase-rs#4886) Address [MED] review finding phase-rs#4 on phase-rs#4938: the seed could still be cleared before a paused repeat-this-process continuation re-entered and emitted later token proposals. Root cause: drain_pending_continuation cleared post_replacement_token_choice_applied BEFORE calling drain_pending_repeat_until; that drain re-enters resolve_ability_chain (effects/mod.rs:721 / :744) and can propose further tokens, which then lost the inherited replacement id and re-prompted the same Jinnie replacement — the loop, reached via a repeat-until pause. Fix: treat pending_repeat_until as part of the originating continuation frame. Drain it first, then clear the seed only at true full-drain — Priority + no pending_continuation + no pending_repeat_iteration + no pending_repeat_until. The seed now outlives every drain in the function. cargo fmt, cargo clippy -p engine --all-targets -- -D warnings, and the full engine lib suite (14873 tests) pass. New regression pins the invariant: a pre-seeded applied set survives a pending_repeat_until drain that re-prompts the controller. * fix(engine): close two more token-choice replacement seed gaps (phase-rs#4886) Addresses review phase-rs#6 on PR phase-rs#4938: - is_token_replacement_choice only scanned a ChooseOneOf's branches for token creation, missing a token created by a sub_ability tail chained after the whole choice resolves. Reuse the existing recursive ability_tree_creates_tokens classifier (which already walks sub_ability/else_ability) instead of a narrower branches-only check. - The elimination teardown path abandoned a live post-replacement continuation without clearing post_replacement_token_choice_applied, the one field missing from its established sibling-clearing bundle. Extract that bundle into a single abandon_post_replacement_continuation helper so a future field can't be missed the same way again. Both fixes are pinned by discriminating regression tests (verified to fail without their corresponding fix, pass with it). --------- Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
lgray
added a commit
to lgray/phase
that referenced
this pull request
Jul 14, 2026
…f; two false claims struck
Adversarial review found §5.5 (target architecture) was LESS SOUND than the code it
proposed to replace. All findings independently re-measured and CONFIRMED. §1-§4 (the RCA)
survived review intact.
TWO CLAIMS IN THIS DOC WERE MEASURABLY FALSE — struck:
B2 ("no counter-growth cover exists"): FALSE. loop_states_cover_modulo_counter_growth
(resource.rs:1326) already exists, its doc names "the proliferate/charge (Pentad Prism)
and burden (The One Ring) omega-cover shape", it is wired into BOTH detect_loop
(loop_check.rs:230) and interactive_loop_bridge (engine.rs:632), and it has 4
discriminating tests. Combo B's blockers are B1 + B3 + B4, NOT B1 + B2 — the cover is
never CONSULTED because B1 clears the ring first. §6.5b is not "subsumed"; it is ALREADY
SHIPPED. Build nothing.
"ResourceVector already computes exactly these deltas": FALSE. It has NO untapped-permanent
axis and no tap state at all; `mana` is floating-pool mana SUMMED ACROSS ALL PLAYERS
(unusable for a per-player LP on a 4-player board); tokens_created/cards_drawn are
event-fed and left ZERO by snapshot. And ResourceVector::delta diffs two SNAPSHOTS — it is
a measurement, not a symbolic effect vector. No symbolic Delta extractor exists. §5.5 is a
NEW SUBSYSTEM, not a reuse.
IS COMPETITIVE MAGIC PROVABLY AN LP? NO — and this is the central correction (§5.5.7-A).
Delta is not symbolically derivable from the AST, for three reasons, each with a real card:
1. Replacements rewrite Delta at resolution. Solemnity: proliferate's AST-Delta is
+1 counter; its TRUE Delta is 0.
2. Delta can depend on the marking (breaks VAS linearity). Freed from the Real:
"{U}: Untap enchanted creature" is +1 only if that creature is tapped, else 0.
3. Legality is not a resource: activation limits, summoning sickness, legend rule,
loyalty (CR 606.3), max hand size — none are axes, none are AbilityConditions.
=> LP PROPOSES (cheap, fungible, payment-independent — dissolves A7 in the proposal);
the existing DRIVE + per-object cover VERIFIES (sound; the only thing that sees
replacements, sickness, marking-dependent Delta). Certify only if BOTH agree.
NEVER certify on the LP alone.
TWO GAME-ENDING HOLES (real cards, both parse today):
H1 Manaforge Cinder — "{1}: Add {B} or {R}. Activate no more than three times each turn."
Parses as activation_restrictions:[MaxTimesEachTurn{3}] AND is_mana_ability:true, so it
IS a §5.5.1 transition. LP certifies; the drive is EXACTLY 2 iterations
(engine.rs:1688) so it passes; the loop dies on activation 4. Fix is LP-native: model a
per-turn allowance as a CONSUMABLE axis (a gated ability draws from a pool the cycle
never refills => net < 0 => no T-invariant => rejected, no special case).
H2 Cryptolith Rite vs Earthcraft — IDENTICAL LP Delta ("consume 1 untapped creature =>
+1 mana"), OPPOSITE truth. Earthcraft's cost is TapCreatures{count:1} (no {T} on the
creature) => sickness-immune => a fresh token can pay => genuinely infinite with
Squirrel Nest. Cryptolith Rite GRANTS "{T}: Add one mana" => CR 302.6 => a fresh token
CANNOT pay. The discriminator is the SHAPE OF THE COST, not any resource level.
DEEPEST FINDING (§5.5.7-F): the fragment guard scans the WRONG AST SURFACE. The reviewer
could NOT break the Comparator-expressed Threshold/ZeroTest arm — that class is genuinely
handled. Every hole routes AROUND AbilityCondition/StaticCondition: the cliffs live in COSTS
(Cryptolith Rite), ACTIVATION RESTRICTIONS (Manaforge Cinder), STATIC COST MODIFIERS
(Damping Sphere, caught today by the `projected` axis §5.5 never mentioned), REPLACEMENTS
(Solemnity), and PER-OBJECT ATTRIBUTES (summoning sickness).
MUST-PRESERVE — three in-tree safety properties §5.5 silently discarded (§5.5.7-B):
(a) repetition-blocking legality gates, which project_out_resources PRESERVES ON PURPOSE
("...would erase the gate that makes a once-per-turn ability NON-repeatable, falsely
certifying it as infinite");
(b) the `projected` axis + its firewall (fire_time_conditions_read_projected_resource),
guarded by the in-tree R-e2 test (resource.rs:5052);
(c) the strict per-object board cover (it is the verifier).
ALSO CORRECTED:
- §6.4 RE-SCOPED to the verifier, not dropped: the LP dissolves the payment choice in the
PROPOSER, but the verifier still calls select_convoke_taps and still compares frames.
- Place granularity (H5, root cause of H2): §5.5.2 needed FUNGIBLE class places while
Freed-from-the-Real/sickness need PER-OBJECT places; the plan never chose. Resolution:
places are fungible within an equivalence class defined by every predicate any COST or
FILTER on the board can test (sickness, legendary, color, type, tapped, token). This also
fixes Relic of Legends ("tap an untapped LEGENDARY creature" — a Saproling is not a
substitute for Kilo).
- CR 113.6 runs a-k, not a-d. 113.6k: a trigger condition that can't trigger from the
battlefield functions in all zones it can trigger from. Battlefield-only scoping would
drop legitimate PUBLIC-zone observers. CR 400.2 is about HIDDEN zones, not
non-battlefield zones — do not conflate.
- FragmentClass: ZeroTest is a leaf parameterization of Threshold => sibling-cluster smell.
Use Threshold{comparator, bound}. Run the mandatory add-engine-variant gate.
- Monotonicity lemma does not COMPOSE: affinity (monotone-DOWN) + Damping Sphere
(monotone-UP) are each "monotone"; the composite is not.
- §7 acceptance criteria: phase-rs#3/phase-rs#4 were VACUOUS (assert_eq passes as false==false when both
arms decline — a trap already hit for real); phase-rs#8 quoted INVENTED oracle text with no card
named (CLAUDE.md: "verify the card, not just the rule") — replaced with real cards
(Manaforge Cinder, Cryptolith Rite vs Earthcraft, Damping Sphere, Solemnity, Rule of Law,
Four Horsemen); phase-rs#10 was a property not a test, vacuous on a corpus certifying nothing.
Added a multiplayer criterion — the driving fixture is a 4-player board and no criterion
mentioned player count.
- Still live and untouched by §5.5: gates (5)/(5b)/(6) — gate (6) rejects on ANY non-empty
delayed_triggers, which fires on any board with a Kiki-Jiki token — and
cost_surface_references_growing_class, which does not dissolve, it MOVES (the LP needs
cost to compute Delta).
Assisted-by: ClaudeCode:claude-opus-4.8
lgray
added a commit
to lgray/phase
that referenced
this pull request
Jul 14, 2026
… (CR 732.2a) Replaces the S5.5.1-S5.5.11 palimpsest with one architecture, derived by READING the rules rather than citing them. Substantive changes: - RC-2 (new, and it is a second independent root cause): the cover requires board recurrence on BOTH driven pairs, but the first driven iteration convokes Witherbloom itself (select_convoke_taps sorts by ObjectId). That is a BOUNDED START-UP TRANSIENT, which CR 732.2a explicitly permits -- a shortcut may be 'a non-repetitive series of choices' followed by 'a loop that repeats a specified number of times'. Fixing RC-1 alone leaves the loop undetected. - Strikes false claim phase-rs#4 ('convoking Witherbloom is illegal at iteration 2, so the payment choice is decisive'): select_convoke_taps re-runs per iteration, the place (untapped green creatures) is non-depleting with delta 0, and the loop certifies whichever creature is convoked. No proposer search is needed. - Combo B: the Kilo cycle is ONE activation (CR 605.3a -- Relic's mana ability is activated inside Freed's cost payment), so arm on activation like we arm on cast. Do NOT touch the ring or the DoS cap. - CR 302.6 discriminator VERIFIED from card-data: Earthcraft's cost has no tap symbol (sick fodder legal, ACCEPT) vs Cryptolith Rite's granted '{T}:' on the creature itself (sick fodder illegal, REJECT). Same idiom, opposite verdicts. - CR 601.2b-h + 602.2b enumerate the choice vector; template.decisions == [ConvokeTaps] is measurably incomplete against it. - CR 113.6 (not CR 400.2) is the primary authority for the zone scope, and battlefield-only is WRONG -- 113.6b/c/d/e/f/j/k are live exceptions. - CR 106.4/500.5: mana empties each step, so counters -- not mana -- are the durable omega-axis. CR 104.4b optional-loop gate ALREADY EXISTS. - CR 732.2a's own worked example (Presence of Gond + Intruder Alarm) is now an acceptance fixture. Corrects CR 502.2 -> CR 502.3. Assisted-by: ClaudeCode:claude-opus-4.8
This was referenced Jul 23, 2026
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
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.
Automated update of README coverage badges from latest card data.