feat(parser+engine): dynamic-cost keyword grants for Scavenge/Encore/…#5054
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review on bd8f217a23ddd49cc194d3277cd57fb00d9b9ff8:
- Real parsed Foretell grants can recurse through effective keyword filtering. The parser preserves the
without foretellaffected filter, off-zone keyword collection evaluates that filter throughmatches_target_filter, andWithoutKeywordKind(Foretell)asks for effective keyword presence again. The runtime proof misses this production shape because it manually omits theWithoutKeywordKindfilter. Please break the self-recursion for off-zoneAddKeywordaffected-filter evaluation and add a runtime test using the parsed Bohn/Dream Devourer static, including a card that already has Foretell. - This is parser/engine work and there is no current
coverage-parse-diffsticky yet. Please wait for/regenerate current-head parser proof before asking for approval.
Parse changes introduced by this PR✓ No card-parse changes detected. |
…tamun) + filter-scoped graveyard grant test Upstream PR phase-rs#5032 (e2cf73a) landed while this branch was in flight and already generalized the graveyard keyword-cost-grant machinery to GrantedCastKeywordKind with hand-zone support (Foretell/Miracle) and ManaCost::SelfManaCostReduced — covering Dream Devourer, Bohn Beguiling Balladeer, and the "without foretell" KeywordMatch::Kind fix this branch had independently arrived at. Rebased onto it and dropped the now-redundant duplicate work; what's left is the non-overlapping remainder: - Add GrantedCastKeywordKind::Embalm (CR 702.128a) — Naktamun's "Each creature card in your graveyard has embalm. Its embalm cost is equal to its mana cost." was a pure parser-recognition gap; the runtime resolver (resolve_self_cost_graveyard_activated_keyword) already handled Embalm. - Add a runtime test proving a *filter-scoped* (2+ matching objects, not SpecificObject) continuous Scavenge grant resolves each recipient's own mana cost independently — this exact shape (a real continuous static matching multiple graveyard cards at once) was previously unproven; every existing test used a single SpecificObject target. Closes phase-rs#5045 Co-Authored-By: Claude Sonnet 5 <[email protected]>
bd8f217 to
38f4df6
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the update. The current head adds the recursion guard and the parse-diff sticky is now present, but the original proof gap is still open before I can clear the requested-changes state.
The previous review asked for a runtime test using the parsed Dream Devourer/Bohn-style static, including a hand card that already has printed Foretell. The current tests still do not cover that shape: dream_devourer_foretell_negatives says it covers a printed-Foretell card, but the actual fixtures are only a land and a cheap nonland. The printed-Foretell runtime coverage that exists elsewhere uses a synthetic SpecificObject remover, not the parsed without foretell affected filter that caused the recursion concern.
Please add a parsed-static runtime regression with a hand card that already has Keyword::Foretell, and assert it keeps its printed foretell cost / does not receive the Dream Devourer reduced-cost grant. That will prove the WithoutKeywordKind(Foretell) affected filter and the off-zone recursion guard work together on the production path.
…e parsed-static path dream_devourer_foretell_negatives claimed to cover a card with printed foretell, but its actual fixtures were only a land and a cheap nonland — the printed-foretell case was never exercised. The only existing coverage for "a card that already has foretell" used a synthetic SpecificObject keyword remover, not the parsed WithoutKeywordKind(Foretell) affected filter that the CR 613.1f off-zone recursion guard exists to support. Add dream_devourer_declines_grant_for_printed_foretell_card: a hand card with its own printed Foretell keyword, granted via Dream Devourer's real parsed static (from_oracle_text), asserting it keeps its printed cost rather than receiving Dream Devourer's MV-2 grant. This is the first test to prove the affected filter's self-referential "does this card already have foretell" check and the recursion guard work together on the production path, rather than in isolation. Co-Authored-By: Claude Sonnet 5 <[email protected]>
matthewevans
left a comment
There was a problem hiding this comment.
Current head addresses the prior blocker. The new dream_devourer_declines_grant_for_printed_foretell_card regression uses the parsed Dream Devourer static, seeds a hand card with printed Foretell in base_keywords, and asserts the printed {5} cost wins over the possible Dream Devourer MV-2 {4} grant. That exercises the WithoutKeywordKind(Foretell) affected filter together with the off-zone recursion guard on the production path requested in the previous review. The parse-diff sticky is current for this head and reports no card-parse changes. Approving for merge queue; required checks can gate the actual merge.
|
@matthewevans Would you review this PR pleaes? it's removed from merge queue, even CI all passed |
The engine already has the building blocks for a keyword-with-cost-formula grant:
ManaCost::SelfManaCost(the card's own mana cost) plusContinuousModification::AddKeyword, used for one-shot single-target grants like Snapcaster Mage's flashback trigger. A continuous, filter-scoped variant of the same idea already existed for a subset of graveyard keywords (Flashback/Escape/Mayhem/Scavenge/Encore viaGraveyardGrantedKeywordKind), but stopped short of several real cards:GraveyardGrantedKeywordKindeven though the runtime resolver already supported it.can_foretell_card/foretell_costreadobj.keywordsdirectly instead of the off-zone-aware path graveyard keywords already used, so a continuously granted Foretell on a hand card would silently never be castable.SpecificObject) continuous grant through to activation.Fix
ManaCost::SelfManaCostAdjusted { delta: i32 }(types/mana.rs) — a sibling ofSelfManaCost, not a field added to it (the bare marker is matched in ~20 call sites across casting/payment/restriction/AI code; keeping them distinct avoids a wire-format change to every existing granted keyword). Resolved at the single existing choke point,game::keywords::resolve_keyword_mana_cost, flooring the adjusted generic component at{0}per CR 601.2f.GraveyardGrantedKeywordKind::Embalm(CR 702.128a) — pure parser-recognition gap; the runtime resolver already handled it.try_parse_hand_foretell_grant_clause/parse_hand_foretell_continuation) mirroring the graveyard shape, reusing the already-genericFilterProp::WithoutKeywordKindfor "without foretell".effective_foretell_cost(game/keywords.rs) — mirrors the existingeffective_harmonize_cost/effective_sneak_costfamily;can_foretell_card/handle_foretellnow resolve through it instead of the raw battlefield-onlyforetell_cost."foretell"token fell through toKeyword::from_str, which concretizes viaparse_keyword_mana_cost("")toKeyword::Foretell(ManaCost::zero())— an exact-{0}-cost match, not a discriminant match."foretell"now joins the existingflashback/embalm/harmonize/... discriminant-only keyword list inoracle_target.rs.SpecificObject) continuous grant resolves each recipient's own mana cost correctly, for both the graveyard and hand-zone paths — this exact shape was previously unproven.Real cards unlocked
Verified against
data/mtgjson/AtomicCards.json(current, not the checked-incard-data.jsonexport).Fossilize (Sue, Everlasting Dinosaur) and Madness (Falkenrath Gorger's "isn't on the battlefield" multi-zone filter) are deliberately out of scope — Fossilize has no Comprehensive Rules entry to annotate against, and Falkenrath Gorger's zone scope is broader than hand+graveyard.
Testing
top_level_static_embalm_grant_stays_on_graveyard_cards,top_level_static_foretell_grant_stays_on_hand_cards_with_cost_delta(parser/oracle_tests.rs).filter_scoped_scavenge_grant_resolves_per_object_cost_for_every_match,filter_scoped_foretell_grant_is_visible_to_hand_cast_path_with_cost_delta(game/casting_tests.rs).cargo fmt --allclean.Note on verification: this environment's local Rust toolchain has no working MSVC linker (verified independently of this change — even
cargo new; cargo runon a trivial project fails to link), so I could not runcargo test/cargo clippylocally. I did a manual, file-by-file audit of every exhaustivematchoverManaCostacross the whole workspace (18 files, ~20 match sites) to add the new variant safely, and traced every call site through to its caller by hand. CI (ubuntu-latest) is the first real compile/test signal for this PR — flagging this explicitly rather than silently.Closes #5045