fix(engine): keep Tin Street coverage honest - #5165
Merged
Merged
Conversation
matthewevans
enabled auto-merge
July 6, 2026 00:09
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the mana spend restriction liveness check by replacing has_payable_branch with is_coverage_supported. Instead of absorbing a spend restriction when at least one branch is payable, the engine now requires every branch in a disjunction (Any) to be coverage-supported. This ensures that cards with mixed disjunctions containing unsupported branches (such as Tin Street Gossip) correctly remain coverage-red (unimplemented) rather than being partially absorbed. Corresponding tests and documentation comments have been updated to reflect this stricter coverage accounting. No review comments were provided, and there is no additional feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
lgray
added a commit
to lgray/phase
that referenced
this pull request
Jul 6, 2026
…-down casting exists Rebase-adaptation onto phase-rs#5165 ("keep Tin Street coverage honest"), which classified `ManaSpendRestriction::FaceDownSpell` as not coverage-supported with the explicit rationale that it "must stay red until face-down spell casting exists." This PR's engine core makes it exist: `build_spell_meta` now sets `is_face_down = true` at a `PaymentContext::Spell` site for a morph/megamorph/disguise face-down cast, so the `FaceDownSpell` leaf is production-live. - `is_coverage_supported(FaceDownSpell)`: false -> true. The sole production caller is the parser's `Effect::Mana` absorption seam (`oracle_effect/sequence.rs`) — a parse-time coverage-classification decision, NOT runtime payability (that stays on the separate `ManaRestriction::allows_spell`). So this is a coverage-honesty change with zero game-behavior change beyond the card becoming supported. - Tin Street Gossip's `Any([FaceDownSpell, TurnPermanentFaceUp])` now absorbs at the seam (coverage red -> green) instead of leaving `Effect::Unimplemented`. - Non-vacuity: `FaceDownSpell` was the last hardcoded-false leaf. The classifier test is restructured (not just flipped) — renamed to `is_coverage_supported_all_leaves_supported_empty_any_is_false`; every leaf is now asserted supported, with `Any(vec![]) == false` kept as the remaining non-vacuous false exemplar. Reverting `FaceDownSpell => true` flips its positive assertion and Tin Street's mixed-`Any` assertion back to red. - `tin_street_gossip_..._stays_coverage_red` -> `..._is_coverage_supported` (asserts the absorbed restriction + no `Unimplemented` residual). - Refreshes the now-stale "dead today / red until face-down casting exists" docs in ability.rs, mana.rs, sequence.rs, and the restricted-mana test. CR 708.4 / CR 702.37c / CR 106.6. Completes the phase-rs#5155 D10 closure at the parser coverage layer (the engine core closed it at the runtime-gate layer). Assisted-by: ClaudeCode:claude-opus-4.8
2 tasks
lgray
added a commit
to lgray/phase
that referenced
this pull request
Jul 6, 2026
…-down casting exists Rebase-adaptation onto phase-rs#5165 ("keep Tin Street coverage honest"), which classified `ManaSpendRestriction::FaceDownSpell` as not coverage-supported with the explicit rationale that it "must stay red until face-down spell casting exists." This PR's engine core makes it exist: `build_spell_meta` now sets `is_face_down = true` at a `PaymentContext::Spell` site for a morph/megamorph/disguise face-down cast, so the `FaceDownSpell` leaf is production-live. - `is_coverage_supported(FaceDownSpell)`: false -> true. The sole production caller is the parser's `Effect::Mana` absorption seam (`oracle_effect/sequence.rs`) — a parse-time coverage-classification decision, NOT runtime payability (that stays on the separate `ManaRestriction::allows_spell`). So this is a coverage-honesty change with zero game-behavior change beyond the card becoming supported. - Tin Street Gossip's `Any([FaceDownSpell, TurnPermanentFaceUp])` now absorbs at the seam (coverage red -> green) instead of leaving `Effect::Unimplemented`. - Non-vacuity: `FaceDownSpell` was the last hardcoded-false leaf. The classifier test is restructured (not just flipped) — renamed to `is_coverage_supported_all_leaves_supported_empty_any_is_false`; every leaf is now asserted supported, with `Any(vec![]) == false` kept as the remaining non-vacuous false exemplar. Reverting `FaceDownSpell => true` flips its positive assertion and Tin Street's mixed-`Any` assertion back to red. - `tin_street_gossip_..._stays_coverage_red` -> `..._is_coverage_supported` (asserts the absorbed restriction + no `Unimplemented` residual). - Refreshes the now-stale "dead today / red until face-down casting exists" docs in ability.rs, mana.rs, sequence.rs, and the restricted-mana test. CR 708.4 / CR 702.37c / CR 106.6. Completes the phase-rs#5155 D10 closure at the parser coverage layer (the engine core closed it at the runtime-gate layer). Assisted-by: ClaudeCode:claude-opus-4.8
lgray
added a commit
to lgray/phase
that referenced
this pull request
Jul 6, 2026
…-down casting exists Rebase-adaptation onto phase-rs#5165 ("keep Tin Street coverage honest"), which classified `ManaSpendRestriction::FaceDownSpell` as not coverage-supported with the explicit rationale that it "must stay red until face-down spell casting exists." This PR's engine core makes it exist: `build_spell_meta` now sets `is_face_down = true` at a `PaymentContext::Spell` site for a morph/megamorph/disguise face-down cast, so the `FaceDownSpell` leaf is production-live. - `is_coverage_supported(FaceDownSpell)`: false -> true. The sole production caller is the parser's `Effect::Mana` absorption seam (`oracle_effect/sequence.rs`) — a parse-time coverage-classification decision, NOT runtime payability (that stays on the separate `ManaRestriction::allows_spell`). So this is a coverage-honesty change with zero game-behavior change beyond the card becoming supported. - Tin Street Gossip's `Any([FaceDownSpell, TurnPermanentFaceUp])` now absorbs at the seam (coverage red -> green) instead of leaving `Effect::Unimplemented`. - Non-vacuity: `FaceDownSpell` was the last hardcoded-false leaf. The classifier test is restructured (not just flipped) — renamed to `is_coverage_supported_all_leaves_supported_empty_any_is_false`; every leaf is now asserted supported, with `Any(vec![]) == false` kept as the remaining non-vacuous false exemplar. Reverting `FaceDownSpell => true` flips its positive assertion and Tin Street's mixed-`Any` assertion back to red. - `tin_street_gossip_..._stays_coverage_red` -> `..._is_coverage_supported` (asserts the absorbed restriction + no `Unimplemented` residual). - Refreshes the now-stale "dead today / red until face-down casting exists" docs in ability.rs, mana.rs, sequence.rs, and the restricted-mana test. CR 708.4 / CR 702.37c / CR 106.6. Completes the phase-rs#5155 D10 closure at the parser coverage layer (the engine core closed it at the runtime-gate layer). Assisted-by: ClaudeCode:claude-opus-4.8
lgray
added a commit
to lgray/phase
that referenced
this pull request
Jul 6, 2026
…-down casting exists Rebase-adaptation onto phase-rs#5165 ("keep Tin Street coverage honest"), which classified `ManaSpendRestriction::FaceDownSpell` as not coverage-supported with the explicit rationale that it "must stay red until face-down spell casting exists." This PR's engine core makes it exist: `build_spell_meta` now sets `is_face_down = true` at a `PaymentContext::Spell` site for a morph/megamorph/disguise face-down cast, so the `FaceDownSpell` leaf is production-live. - `is_coverage_supported(FaceDownSpell)`: false -> true. The sole production caller is the parser's `Effect::Mana` absorption seam (`oracle_effect/sequence.rs`) — a parse-time coverage-classification decision, NOT runtime payability (that stays on the separate `ManaRestriction::allows_spell`). So this is a coverage-honesty change with zero game-behavior change beyond the card becoming supported. - Tin Street Gossip's `Any([FaceDownSpell, TurnPermanentFaceUp])` now absorbs at the seam (coverage red -> green) instead of leaving `Effect::Unimplemented`. - Non-vacuity: `FaceDownSpell` was the last hardcoded-false leaf. The classifier test is restructured (not just flipped) — renamed to `is_coverage_supported_all_leaves_supported_empty_any_is_false`; every leaf is now asserted supported, with `Any(vec![]) == false` kept as the remaining non-vacuous false exemplar. Reverting `FaceDownSpell => true` flips its positive assertion and Tin Street's mixed-`Any` assertion back to red. - `tin_street_gossip_..._stays_coverage_red` -> `..._is_coverage_supported` (asserts the absorbed restriction + no `Unimplemented` residual). - Refreshes the now-stale "dead today / red until face-down casting exists" docs in ability.rs, mana.rs, sequence.rs, and the restricted-mana test. CR 708.4 / CR 702.37c / CR 106.6. Completes the phase-rs#5155 D10 closure at the parser coverage layer (the engine core closed it at the runtime-gate layer). Assisted-by: ClaudeCode:claude-opus-4.8
rykerwilliams
pushed a commit
to rykerwilliams/phase
that referenced
this pull request
Jul 6, 2026
…phase-rs#5171) * feat(engine): rules-correct morph/disguise face-down spell casting (CR 708.4) Cast a Morph/Megamorph/Disguise card from hand face down as a blank 2/2 creature spell for a fixed {3} (CR 702.37c / 702.168a), placed on the stack (CR 708.4) and resolving to a face-down permanent (CR 702.37c). Closes the phase-rs#5155 D10 deferral: Tin Street Gossip's FaceDownSpell restricted-mana leaf now goes live with no type change, unlocking the full 228-card morph/megamorph/disguise class. Cast path (opt-in hand alt-cast idiom, mirroring Evoke/Blitz): - New CastingVariant::FaceDown + AlternativeCastKeyword::FaceDown; the offer in handle_cast_spell surfaces AlternativeCastChoice{FaceDown} for any card with Morph/Megamorph/Disguise when the fixed {3} is affordable. - continue_cast_face_down turns the object face down (blank 2/2, real card stashed in back_face) via the shared apply_face_down_entry_profile BEFORE the stack (CR 708.4). Visibility redaction, resolution to a face-down permanent, CR 708.9 counter-reveal, and turn-face-up (CR 702.37e) are all inherited from existing machinery. - The {3} is injected as a synthetic constant into the alt-cost .or() chain, never read off the blanked object (whose mana cost is now NoCost). Payment (closes D10): - SpellMeta.is_face_down is derived from the blanked object (face_down && back_face.is_some()) as a single authority in build_spell_meta, with no caller threading. A foretold card (face_down, back_face = None) stays face up, preserving the existing foretell guard test. The OnlyForFaceDownSpell mana gate now goes live. Disguise's face-down 2/2 carries ward {2} (CR 702.168a) via the cloaked_2_2 profile; morph/megamorph use vanilla_2_2. The unmorph/turn-up cost is read downstream from the hidden card's keyword, so CastingVariant::FaceDown is parameterless. Tests (engine_tests.rs): 8 discriminating tests including the D10 closure (Tin Street Gossip's OnlyForAny([FaceDownSpell, TurnFaceUp]) restricted mana funds the {3}), opponent redaction of the stack spell, disguise ward {2}, and the CR 708.9 counter-reveal. Assisted-by: ClaudeCode:claude-opus-4.8 * test(ai): morph face-down cast is enumerated and sanely scored by the AI Slice E of the morph/disguise face-down casting PR. Test-only — the AI already handles the new `AlternativeCastChoice { keyword: FaceDown }` action via the keyword-agnostic candidate-enumeration arm in `engine::ai_support::candidate_actions` (candidates.rs), consumed by the phase-ai search through `build_decision_context`. No production AI code changes; no DeckFeature/policy needed (measure-first showed the action is already enumerated + scored). Two discriminating tests: - (i) both the face-down (Alternative) and printed (Normal) casts are enumerated candidates — the face-down cast is not declined by omission. - (ii) the eval credits the resulting permanent AS a 2/2 (CR 708.2): for a strong real 5/5, the normal cast must OUTSCORE the face-down cast. Distinct + ordered finite non-sentinel scores prove the eval distinguishes the two boards. cargo ai-gate is null by construction for this change (the gate suite has no morph creatures, so no candidate set changes); a direct discriminating scoring test is the meaningful evidence. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(client): surface morph/disguise face-down cast in the alternative-cost modal Slice F of the morph/disguise face-down casting PR. The engine already emits `WaitingFor::AlternativeCastChoice { keyword: FaceDown }` for a Morph/Megamorph/ Disguise card in hand; this adds the display layer only — no game logic, the engine owns the {3} cost and the blank 2/2 body (CR 708.4 / 702.37c / 702.168a). - adapter/types.ts: add `{ type: "FaceDown" }` to the AlternativeCastChoice keyword union (keeps the boundary-guardrails Rust↔TS lockstep test green). - AlternativeCostModal.tsx: add the `case "FaceDown"` display-copy arm — mirrors the existing per-keyword pattern; the `assertNever` fallback enforces exhaustiveness. Costs render from the engine-provided fields. - i18n: `alternativeCost.faceDown*` keys added to all 7 locales (de/en/es/fr/it/pl/pt) — satisfies the resources.test.ts parity gate. Also folds the now-stale D10 comments in tests/restricted_mana_face_down_and_face_up.rs: the OnlyForFaceDownSpell gate was HONEST-DEFERRED (no production path set is_face_down=true); it went LIVE with the face-down cast (build_spell_meta derives is_face_down = face_down && back_face.is_some()). Comment-only — assertions unchanged; end-to-end coverage is engine_tests::tin_street_gossip_restricted_mana_funds_face_down_cast. Assisted-by: ClaudeCode:claude-opus-4.8 * refactor(engine): FaceDownSpell mana coverage-supported now that face-down casting exists Rebase-adaptation onto phase-rs#5165 ("keep Tin Street coverage honest"), which classified `ManaSpendRestriction::FaceDownSpell` as not coverage-supported with the explicit rationale that it "must stay red until face-down spell casting exists." This PR's engine core makes it exist: `build_spell_meta` now sets `is_face_down = true` at a `PaymentContext::Spell` site for a morph/megamorph/disguise face-down cast, so the `FaceDownSpell` leaf is production-live. - `is_coverage_supported(FaceDownSpell)`: false -> true. The sole production caller is the parser's `Effect::Mana` absorption seam (`oracle_effect/sequence.rs`) — a parse-time coverage-classification decision, NOT runtime payability (that stays on the separate `ManaRestriction::allows_spell`). So this is a coverage-honesty change with zero game-behavior change beyond the card becoming supported. - Tin Street Gossip's `Any([FaceDownSpell, TurnPermanentFaceUp])` now absorbs at the seam (coverage red -> green) instead of leaving `Effect::Unimplemented`. - Non-vacuity: `FaceDownSpell` was the last hardcoded-false leaf. The classifier test is restructured (not just flipped) — renamed to `is_coverage_supported_all_leaves_supported_empty_any_is_false`; every leaf is now asserted supported, with `Any(vec![]) == false` kept as the remaining non-vacuous false exemplar. Reverting `FaceDownSpell => true` flips its positive assertion and Tin Street's mixed-`Any` assertion back to red. - `tin_street_gossip_..._stays_coverage_red` -> `..._is_coverage_supported` (asserts the absorbed restriction + no `Unimplemented` residual). - Refreshes the now-stale "dead today / red until face-down casting exists" docs in ability.rs, mana.rs, sequence.rs, and the restricted-mana test. CR 708.4 / CR 702.37c / CR 106.6. Completes the phase-rs#5155 D10 closure at the parser coverage layer (the engine core closed it at the runtime-gate layer). Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): offer face-down cast from every castable zone (CR 702.37c/702.168b) CR 702.37c / CR 702.168b: "You can use a morph/disguise ability to cast a card from any zone from which you could normally cast it." The {3} face-down offer was gated on obj.zone == Zone::Hand, so a morph card castable from the graveyard/exile/command/top-of-library was never offered the face-down cast. Route the offer through the general castable-zone authority (prepare_spell_cast(..).is_ok()) instead of the hand-only special case; the other cast-offer blocks (Awaken/Impending/ Prototype) stay hand-gated because only morph/disguise carry the cross-zone rule. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): roll back a canceled face-down cast (CR 601.2i / CR 708.4) continue_cast_face_down blanks the object to its face-down 2/2 (stashing the real card in back_face) BEFORE payment, but handle_cancel_cast only restored alternate spell faces / prototype / prepared-copy — not a face-down cast. A CancelCast from WaitingFor::ManaPayment (or any pre- finalize step) therefore left the card blanked, nameless, and no-cost in its origin zone. Add a CastingVariant::FaceDown branch that routes through the existing single-authority restore_face_down_cast_object (the same rollback used on the prep-failure error path), reversing the entire cast per CR 601.2i. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(casting): free normal cast not robbed by face-down auto-route (Blocker C) A pending `WithoutPayingManaCost` next-spell modifier (Omniscience-style one-shot) makes the FREE face-up normal cast of a morph/megamorph/disguise card legal even when its printed cost is unpayable. `normal_cast_choice_cost_ and_affordability` only recognized the `Unlimited CastFromHandFree` (Omniscience static) short-circuit, so a pending one-shot modifier was missed: the normal path reported the unpayable printed cost, `normal_affordable` was false, and the face-down offer auto-routed to the {3} face-down cast — robbing the player of the legal free face-up cast. Mirror the real prep authority (prepare_spell_cast_with_variant_override_inner via `next_spell_without_paying`, CR 118.9a): when a `WithoutPayingManaCost` modifier matches the spell, treat the normal path as `ManaCost::NoCost` and affordable so the engine OFFERS free-normal vs {3}-face-down instead of auto-routing. CR 601.2f + CR 118.9a (verified against docs/MagicCompRules.txt). Test `morph_free_normal_cast_not_robbed_by_face_down_autoroute` is discriminating: disabling the short-circuit auto-routes face down (`waiting_for` becomes Priority, not AlternativeCastChoice) → the offer assertion's `other => panic!` fires (measured). Assisted-by: ClaudeCode:claude-opus-4.8 * fix(casting): castable NoCost morph/disguise face down for {3} (Blocker A) A card with no mana cost (`ManaCost::NoCost`) that carries an effective morph/megamorph/disguise keyword is unpayable face up, but IS legally castable FACE DOWN for the fixed {3} alternative cost (CR 118.6a: an alternative cost may be applied to an unpayable cost). Three sites wrongly treated such a card as uncastable: - The `Zone::Hand` NoCost rejection in `handle_cast_spell_with_payment_mode` returned an error before the face-down offer block could run. - Its candidate/legal-action twin `can_cast_prepared_now_with_probe` returned false, so the cast was never surfaced as a legal action even though dispatch would have accepted it. - A soundness trap in `normal_cast_choice_cost_and_affordability`: `can_pay_cost_after_auto_tap` returns true for `NoCost` unconditionally, so the offer reported the unpayable face-up cast as affordable and offered a bogus free face-up cast instead of auto-routing to the {3} face-down. A cost reduced to nothing is `{0}` (CR 601.2f), distinct from `NoCost`, so the new `!matches!(normal_cost, NoCost)` guard never misfires on a cost-reduced-to-zero card. Both hand-gate exceptions and the legal-action twin are narrowed to the morph class AND {3} affordability via a shared `object_has_effective_face_down_keyword` helper (extracted from the inline array at the morph offer block) plus `can_afford_face_down_cast`, so the unpayable-cost rejection still stands for every non-morph NoCost card and for a morph without {3}. CR 118.6 / 118.6a / 601.2f / 702.37c / 702.37b (megamorph) / 702.168b (disguise), all verified against docs/MagicCompRules.txt. Tests (all measured discriminating via temporary edit-revert): - nocost_morph_auto_casts_face_down_for_three: remove E2 → rejected (.expect panics); remove the normal_affordable guard → offers not auto-routes (obj stays Hand). - nocost_non_morph_creature_rejected_from_hand / nocost_morph_without_three_ mana_rejected: keyword + {3}-affordability gating (both stay Err). - nocost_morph_offered_as_legal_action: remove E4 → can_cast_object_now false. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(casting): evaluate face-down cast prohibitions against the blanked profile (CR 708.4) The morph/megamorph/disguise face-down cast castability gate evaluated cast prohibitions against the un-blanked, printed object. A characteristics-dependent prohibition (StaticMode::CantBeCast with a name- or mana-value-conditional filter — e.g. Meddling Mage / Nevermore naming the card) rejected prepare_spell_cast on the printed face, suppressing the legal {3} face-down cast at both the dispatch offer gate and the can_cast_object_now feasibility path (dropping it from legal actions and AI feasibility as well). Per CR 708.2a a face-down spell is a nameless, mana-cost-less 2/2, and per CR 708.4 prohibitions apply to those face-down characteristics, not the face-up object's; CR 601.3a lets a player ignore a qualities-conditional prohibition when a proposal choice (casting face down) changes the qualities it reads. New face_down_cast_is_permitted helper clones the state, blanks the object via the existing face_down_cast_profile + apply_face_down_entry_profile infra (mirroring can_afford_face_down_cast and the real-cast path continue_cast_face_down), then runs prepare_spell_cast_with_variant_override(Some(FaceDown)).is_ok() against the blanked profile. Wired into both the dispatch offer gate and the feasibility twin, so the whole class of characteristics-dependent prohibitions (name, mana value, color, subtype/type) is covered — blanking zeroes all of them. Assisted-by: ClaudeCode:claude-opus-4.8
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
1 participant
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.
No description provided.