feat(engine): S01 standard-set completion — 11 cards / 7 mechanic groups#4688
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
lgray
force-pushed
the
ship/std-s01-completion
branch
from
476e049 to
b1f78e0
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Excess-damage fight conditions read the wrong fighter. Evidence: crates/engine/src/game/effects/mod.rs:5037 selects the first object target of the fight when computing PreviousEffectAmount excess, but the parser accepts text like "the creature the opponent controls is dealt excess damage this way" where the recipient is the second fight target. Why it matters: dual-target fight cards such as The Last Agni Kai can add mana based on excess damage to the controller's creature instead of the opponent's creature. Suggested fix: carry the parsed damage-recipient subject into the lowered condition, or make the fight excess resolver select the actual recipient named by the condition, with a dual-target runtime test.
[HIGH] Full-cost "you may cast ... from graveyard" effects lower to a delayed permission instead of a during-resolution cast. Evidence: crates/engine/src/parser/oracle_effect/mod.rs:16884 turns Quistis/Tinybones-style text into Effect::CastFromZone, and crates/engine/src/game/effects/cast_from_zone.rs:297-:308 only casts during resolution for free-cast/graveyard-free-cast paths; otherwise it grants lingering permissions. Why it matters: these triggers are one-shot casts during resolution, but the PR changes timing and can allow/deny spells based on a later priority window instead. Suggested fix: implement a paid during-resolution cast path that uses the real mana cost plus mana_spend_permission, or keep this text unsupported until that path exists.
[MED] Graveyard replacement riders can leak from an unconsumed cast permission. Evidence: crates/engine/src/game/casting_costs.rs:5832-:5841 scans any ExileWithAltCost { graveyard_replacement: Some(..) } on the object instead of the selected permission that authorized the current cast, while the adjacent enters-with-counter path uses selected-permission authority at :5851-:5856. Why it matters: a stale or foreign permission can redirect the resolving spell to exile/library/hand even when the current cast did not use that rider. Suggested fix: add a selected-permission helper analogous to selected_exile_alt_cost_permission_enters_with_counter and use it when installing apply_spell_graveyard_replacement_rider.
[MED] Legacy serialized excess-damage quantities lose their excess-only gate. Evidence: crates/engine/src/types/ability.rs:4649 replaces the former excess_only bool with a defaulted channel: DamageChannel, but there is no alias/compat deserializer mapping old excess_only: true payloads to DamageChannel::Excess. Why it matters: old card data/saves/scenarios reload as total-damage checks and can fire on any matching damage. Suggested fix: add serde compatibility for the old bool field and a legacy JSON test for excess_only: true.
[MED] Legacy serialized ExileWithAltCost permissions lose their exile-on-resolution rider. Evidence: crates/engine/src/types/ability.rs:2118 replaces exile_instead_of_graveyard_on_resolve with defaulted graveyard_replacement, but there is no alias/compat mapping from the old bool to Some(SpellStackToGraveyardReplacement::Exile). Why it matters: in-flight saved/reconnected permissions reload without the required graveyard replacement and resolve to the graveyard. Suggested fix: add serde compatibility for the old bool field and cover it with a legacy CastingPermission round-trip test.
Parse changes introduced by this PR · 35 card(s), 45 signature(s) (baseline: main
|
Completes the S01 standard-set card cluster. Groups (each landed through an
adversarial review + verification gate):
- Reflexive object-property conditions (card-type / exact-power / compound
or-if) and the current-phase condition (Dose of Dawnglow).
- Excess-damage "this way" condition (Torch the Witness, Orbital Plunge) +
convergence of the DamageDealtThisTurn excess flag onto a typed DamageChannel.
- Full Bore — target-scoped "cast for its warp cost" condition.
- ConditionInstead not-swap trailing tail (CR 608.2c) — Throw from the Saddle,
Take the Fall, That's Rough Buddy, Evil's Thrall.
- Kylox's Voltstrider (MKM) — collect-evidence activated ability with the cost
charged through the single-authority interactive-cost resolver (fixes a
CR 701.59 free-animate), plus the cast-from-exile library-bottom rider.
- Quistis Trepe + Tinybones the Pickpocket — cast-from-graveyard with any-type
mana (CR 609.4b), multiplayer-correct target (Owned{TriggeringPlayer}).
- Summoner's Grimoire (MKM) — conditional enters-tapped-and-attacking on a
granted put-from-hand, a zero-new-variant leaf parameterization of the
ChangeZone enters-modifier cluster (enters_modified_if, CR 614.12).
Assisted-by: ClaudeCode:claude-opus-4.8
lgray
force-pushed
the
ship/std-s01-completion
branch
from
b1f78e0 to
e21efa8
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up. I re-reviewed the current head (e21efa8890aa0c36ed57d586d84adf0b69c889c8) and the main blockers from the previous review are still present.
[HIGH] The fight excess-damage condition still reads the wrong fighter. previous_effect_excess_amount_from_events still selects the first object target on the Fight ability (crates/engine/src/game/effects/mod.rs:5037-5040) and then sums excess damage dealt to that object (:5041-5056). For templates like The Last Agni Kai, the first target is the source creature, but the condition is about excess damage dealt to the opponent's creature, so the condition can pass/fail on the wrong damage event.
[HIGH] Full-cost resolution-time graveyard casts still become lingering permissions. The parser now forwards ManaSpendPermission::AnyTypeOrColor, but handle_cast_from_zone only performs an immediate graveyard cast when without_paying_mana_cost is true (crates/engine/src/game/effects/cast_from_zone.rs:288-297). Quistis/Tinybones-style text says to cast the targeted graveyard card during resolution while paying its normal cost, so the non-free path still falls through to grant_lingering_permissions at :308 instead of creating the required during-resolution paid-cast choice.
[MED] The graveyard replacement rider is still read from any permission on the object, not the permission consumed by this cast. finalize_cast_with_phyrexian_choices still scans obj.casting_permissions.iter().find_map(...) for the first ExileWithAltCost { graveyard_replacement: Some(...) } (crates/engine/src/game/casting_costs.rs:5832-5840). That can apply another grant's replacement rider to this cast; it needs the same selected-permission authority used by selected_exile_alt_cost_permission_enters_with_counter.
[MED] The serialized-state migration gaps are still present. DamageDealtThisTurn now defaults channel to total (crates/engine/src/types/ability.rs:4644-4650), and ExileWithAltCost now defaults graveyard_replacement to None (:2118-2119), but there is no compatibility path for legacy excess_only: true or exile_instead_of_graveyard_on_resolve: true. Existing saved/generated JSON using those fields will deserialize as total damage / no replacement rider.
…subject
The Last Agni Kai dual-target fight ("target creature you control fights
target creature an opponent controls. If the creature the opponent controls
is dealt excess damage this way, add that much R") put both fighters in
ability.targets as [subject, fought]. Both previous_effect_amount_from_events
and previous_effect_excess_amount_from_events blindly find_map the FIRST
object target (the subject), summing excess dealt to the wrong creature.
Extract resolve_fight_fighters as the single authority for the (subject,
fought-creature) split (also de-duplicating two identical branches in
fight::resolve) and read the recipient from it in both amount functions.
CR 120.10 + CR 701.14a. Adds a discriminating dual-target runtime test.
# Conflicts: # crates/engine/src/game/casting_tests.rs
… compat for legacy bools MED (graveyard rider): finalize_cast_with_phyrexian_choices read the graveyard-redirect rider from the FIRST ExileWithAltCost permission carrying one, so a non-consumed sibling permission's rider could leak onto this cast. Add selected_exile_alt_cost_permission_graveyard_replacement (mirrors the enters-with-counter selected-permission authority) and use it. CR 608.2c. MED (serde compat): the channel: DamageChannel field (replacing excess_only: bool) and graveyard_replacement: Option<...> (replacing exile_instead_of_graveyard_on_resolve: bool) had no compatibility path for pre-migration serialized state. Add serde alias + compat deserializers so legacy excess_only:true -> Excess and exile_instead_of_graveyard_on_resolve:true -> Some(Exile), with legacy-JSON round-trip tests. CR 120.10 / CR 608.2n.
…09.4b) "You may cast target X card from a graveyard, and mana of any type can be spent to cast that spell" (Quistis Trepe, Tinybones the Pickpocket) was lowering to a lingering ExileWithAltCost permission — wrong CR 608.2g timing, and inert for Tinybones' opponent-graveyard target (phase-rs#2884). Make it a during-resolution one-shot PAID cast, offered accept/decline as the trigger resolves. - Parameterize the during-resolution cast authority with a typed ResolutionCastCost { Free, FullCost { mana_spend_permission } } instead of hardcoded zero/None/Auto. The 5 existing free-cast callers (Cascade, Discover, Ripple, FreeCastWindow, the shared during-resolution caller) pass Free and are byte-identical. - New CastOfferKind::GraveyardPaidCast + GameAction::GraveyardPaidCastChoice. A router paid-gate in cast_from_zone::resolve opens the offer; accept casts at the card's real printed cost (SelfManaCost) with the CR 609.4b any-type-mana concession; decline leaves the card in the graveyard. - Parser sets driver = DuringResolution in try_parse_cast_target_from_graveyard_any_mana so the router routes to the offer rather than the lingering path. - Re-home the concession-only ExileWithAltCost past finalize_cast's cascade-constraint gate so the CR 609.4b any-type concession survives to the mana-payment step (off-color payment would otherwise fail). - Cross-crate exhaustiveness arms (phase-ai forced_action -> Decline, engine source_object -> None, server-core payload guard no-op); AI accept/decline candidates; scenario label; frontend paid-cast offer modal + i18n (7 locales). 6 new discriminating runtime tests drive the real apply() pipeline (router opens offer not lingering permission; accept opens manual payment then stack; off-color pay via any-type concession; decline; free-cast still Auto; without_paying bypass) plus reconciled Quistis/Tinybones/swallow_check assertions.
…ars it Kylox's Voltstrider casts a card from exile with a "put it on the bottom of its owner's library instead" rider, but finalize_cast read the rider AFTER the Exile->Stack move — and apply_zone_exit_cleanup drops every ExileWithAltCost permission when a card leaves exile (CR 400.7 / CR 113.6e), so the rider was gone by the read and the spell wrongly went to the graveyard. Capture the graveyard_replacement destination into a local BEFORE the casting_to_stack move (mirroring the sibling exile-scoped captures already taken pre-move for the same reason), and apply it after the move. Behavior-neutral for graveyard/hand in-place casts (Quistis, Emry, Electrodominance — their permission survives the move). No double-install with the during-resolution paid path: evaluate_cascade_constraint_with_resulting_mv strips the resolution_cleanup permission before the finalize read, so the two rider-application points (initiate_cast_during_resolution vs finalize) are mutually exclusive per cast. CR 614.1a / CR 608.2n. Adds an exile-origin end-to-end test (bottoms the resolved spell instead of graveyarding it) and a during-resolution guard asserting exactly one redirect installs.
…ss and Orbital Plunge The excess-damage "this way" channel class was covered, but neither card had a test driving its own Oracle text through a real cast. Add per-card overkill vs exact-lethal runtime tests: the excess-gated follow-up (Investigate clue / Lander token) fires only when excess > 0, discriminating the Excess channel from Total (a Total-summing resolver would wrongly fire on the exact-lethal case). CR 120.10.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review: passes the authorized review bar at head 10be31afa74eb17a1345adbf420c348ec7c676db.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 participants
🤖 AI text below 🤖
Completes the S01 standard-set card cluster — 11 cards across 7 mechanic groups, each landed through an adversarial review + verification gate.
Groups
DamageDealtThisTurnexcess flag onto a typedDamageChannel.CastVariantPaid { subject: ObjectScope::Target }, CR 608.2c / CR 702.185a).Owned { TriggeringPlayer }).enters_modified_if, CR 614.12).Verification (rebased on
upstream/main@2222dbeed)cargo fmt --all --check: cleancargo clippy --workspace --all-targets --features engine/proptest -D warnings: 0 warningscargo test -p engine --features proptest: 16531 passed, 0 failed (15 ignored). All 7 load-bearing S01 tests pass.2222dbeedbaseline against the same MTGJSON + identical committedknown-tokens.toml, so the diff is purely parser-source):false → trueplus 9 class-level bonus unlocks; Tinybones + Evil's Thrall were already supported and were improved without a coverage flip.