Keep offered casts payable through completion - #1
Merged
Conversation
…hase-rs#7609) * fix(engine,parser): manifest a card from your hand (Scroll of Fate) (phase-rs#7608) Mirror the supported cloak from-hand twin (Vannifar) on the manifest side: a from-hand recognizer arm in the imperative "manifest" dispatch, a lowering intercept to a ChooseFromZone{Hand} parent with a Manifest{object_source: ParentTarget} sub-ability, the new Effect::Manifest.object_source field (serde default None = library-top source), and a resolver branch manifesting the chosen objects through the shared morph::manifest_card authority (CR 701.40a vanilla 2/2, no ward). Rule-13 double parse over the full 35,798-card corpus: exactly one card changes (scroll of fate). Remaining gap: Kozilek, the Broken Reality's targeted per-player form (documented in phase-rs#7608). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(engine): pin the from-hand manifest object source to ParentTarget CodeRabbit round 1: `object_source: Some(_)` only proves the field is populated — a wrong explicit source would still pass. Both the parser test and the integration test now match the concrete `Some(TargetFilter::ParentTarget)` the lowering installs (the value the ChooseFromZone parent forwards per CR 608.2c), mirroring the Vannifar cloak integration test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(engine): drive Scroll of Fate's registered ability, pin the full manifest profile Maintainer round 1: 1. The integration test now builds the artifact from its printed Oracle text (new scenario helper add_artifact_from_oracle, mirroring the enchantment/land builders), activates the registered ability through GameAction::ActivateAbility, asserts the {T} cost tapped the source (CR 602.2b), resolves off the stack, and answers the ChooseFromZoneChoice through GameAction::SelectCards — no synthetic parse_effect_chain resolve. Probe: disabling the from-hand parser arm makes the activation illegal and the test red. 2. The chosen hand card is now a NONCREATURE (plain sorcery) and the complete CR 701.40a profile is pinned: empty name, exactly [Creature] (printed Sorcery hidden per CR 708.2a), no super-/subtypes, 2/2, ManaCost::NoCost, and no keywords (vs. cloak's ward {2}). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(engine): condition the manifest library-move classification on its source Maintainer round 2: moves_card_to_or_from_library still returned true for every Effect::Manifest — the arm's own comment required it to become conditional the moment a source field arrived. It now mirrors the neighboring Cloak arm: None (library-top default) stays a library move, Some(filter) follows filter.extract_zones(), so Scroll of Fate's from-hand manifest no longer misclassifies (and can no longer reject a valid mana ability under the CR 605.1a classifier). Discriminating assertion added alongside the runtime coverage: the from-hand chain classifies false on both levels (probe: reverting the arm to an unconditional true turns it red), the library-top control stays true. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(engine): harden manifest-from-hand scenario coverage * fix(engine): correct manifest color assertion citation --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…zilek, the Broken Reality) (phase-rs#7697) * fix(engine,parser): target players each manifest from their hands (Kozilek) (phase-rs#7691) Kozilek, the Broken Reality's cast trigger resolved to nothing: everything around the body already parsed (SpellCast mode, the up-to-two player multi_target, the repeat_for: TrackedSetSize draw rider), but the predicate was Effect::Unimplemented. Built on the Breach the Multiverse shape (per-player ChooseFromZone feeding one accumulated tracked set), with two leaves added to existing axes: ZoneOwner::EachTargetedPlayer (iterate the chosen Player targets in APNAP order, CR 101.4) and Chooser::OwningPlayer (each player picks from their own hidden hand, CR 608.2c). Three existing authorities gained one arm each: - Effect::target_filter() answers Some(Player) for the EachTargetedPlayer form so the player slots exist to iterate (all other ChooseFromZone forms stay None). - The multi-target player fan-out excludes that form: it is the missing iteration layer for single-player-recipient handlers, and this effect already iterates the chosen players itself — left in, it split the chain per player so each iteration got its own tracked set. - manifest.rs gains the tracked-set source arm (Cloak's, minus the exile dance) and rebinds to a fresh chain set first, so the "manifested this way" rider counts the manifests and not also the picks. Rule-13 double parse over the full 35,798-card corpus: exactly one card changes (kozilek, the broken reality). Census pin re-pinned for a line shift (same producer: WaitingFor::OptionalEffectChoice). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(engine): assert the typed Kozilek chain and pin the APNAP prompt order CodeRabbit round 1: - The parser test matched only on the absence of "Unimplemented" in a debug string, which an unrelated implemented chain would also pass. It now matches the typed chain: ChooseFromZone { count: 2, zone: Hand, zone_owner: EachTargetedPlayer, chooser: OwningPlayer }, the up-to-two player multi_target spec, the Manifest sub-ability with its TrackedSet object source and owner-default entry, and the draw rider with repeat_for: TrackedSetSize. - The integration test branched on the prompted player and so accepted either order; it now records the prompt sequence and asserts P0 (the active player) is prompted first, per CR 101.4. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(engine): pin the Kozilek choose's selection mode and tracked-set id CodeRabbit round 2: the ChooseFromZone match accepted any selection mode (a regression to random/non-choice selection would have passed, though each targeted player must CHOOSE — CR 608.2d), and the Manifest match accepted any TrackedSet id. Both are now concrete: `selection: CardSelectionMode::Chosen` and `object_source: Some(TargetFilter::TrackedSet { id: TrackedSetId(0) })`, so the test proves the manifest consumes THIS choose chain's picks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(engine): parameterize the per-player zone-owner scope Maintainer round 1 (architecture blocker): EachPlayer / EachOpponent / EachTargetedPlayer were three serialized siblings for one axis — the machinery (one parked choice per player in APNAP order, accumulating into the chain's tracked set) is identical; only the population differs. They collapse into ZoneOwner::Each(PerPlayerScope) with AllPlayers / Opponents / TargetedPlayers leaves, and the population resolution moves into one authority (`per_player_iteration_population`), so a future leaf is a match arm there rather than a new ZoneOwner sibling. The iteration gate, the single-owner error arm, and the multi-target fan-out exclusion all now match on `Each(_)` instead of enumerating siblings. Serialized-data migration is explicit: ZoneOwner (de)serializes through ZoneOwnerRepr, which still ACCEPTS the three pre-refactor unit names and maps them onto the parameterized form, while only ever WRITING {"Each":<scope>}. Covered by zone_owner_migration_tests; the pre-existing round-trip pin was updated to the parameterized wire shape and extended with the TargetedPlayers leaf. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(engine): name the per-player scope after what it iterates `PerPlayerScope::Opponents` is populated as "every player but the controller", which is what the only wording that produces it asks for: "For each other player, ..." (Kaya, Spirits' Justice). CR 102.3 keeps the two notions apart — a teammate is one of the "other players on their team" and is not that player's opponent — so resolving this population through `players::opponents` would drop, in a team format, exactly the player the card includes. Rename the leaf to `OtherPlayers` and state at the arm why it deliberately does not route through `players::opponents`. The pre-refactor sibling (`EachOpponent`) carried the same misnomer and the same population, so the legacy wire name keeps deserializing onto it and stored data is unaffected. A genuinely opponent-scoped population remains a further leaf in the same match. Carry verified CR citations on the scope mapping (CR 101.4 for the APNAP walk, CR 102.3 for other-player vs opponent) and correct a doc paragraph that still named the pre-refactor variants. No behavior change: the population, the accepted legacy wire names and every card's parse are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(engine): make the random per-player zone-owner swallow explicit `resolve_random_in_chain` reached `resolve_candidate_cards` with the effect's zone owner and turned its error into an empty pool via `unwrap_or_default()`. `ZoneOwner::Each` is the one owner with no single candidate pool — it resolves one prompt per player — so a random selection over it silently resolved to nothing instead of failing. No card drives that combination: of the 43 cards whose text carries per-player wording, none say "at random", and no parser arm emits `Each` together with `CardSelectionMode::Random`. Rather than build a speculative per-player random pick, spell the `Err` arm out and put a `debug_assert!` in the function itself, so a future parse producing the shape trips the suite instead of resolving to nothing. Release behavior is unchanged. Also correct a comment at the iteration gate that still named the pre-refactor `EachOpponent`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(engine): reject a per-player zone owner in the pool authority The previous commit put the guard on `resolve_candidate_cards`'s error, which the tracked-set fast paths never reach: `chain_tracked_set_cards` returns before the zone owner is read, so a random `ChooseFromZone` over `ZoneOwner::Each` would have been answered from the whole global or prior-chain set instead of failing. Reject `Each` as the pool authority's first act, ahead of both fast paths — "no single pool" is a property of the owner, not of the caller. No supported path is affected: `resolve` returns to `prompt_next_each_player` before reaching the pool authority, and `resolve_with_choosing_player` is only entered from below that return, so `Each` reaches this function on no legitimate route. The random caller keeps its `debug_assert!`, which stays silent across the full suite (19,547 lib + 5,358 integration). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Parse changes introduced by this PRBaseline pending for |
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification