Keyword deserialization silently substitutes constants for four payloads (Gift, Ripple, CumulativeUpkeep, Discover) #7234

Card(s)

Longstalk Brawl (reported), Surging Aether, Aboroth. Any card whose keyword carries a payload in one of four variants.

Build/version

phase-engine 0.50.0, base 1bb6c1d.

Game mode

P2P

Actual behavior

Longstalk Brawl offers "Promise a card". It gifts a tapped Fish.

Keyword's hand-written Deserialize (keyword_from_tagged, types/keywords.rs) has four arms that ignore the variant's payload and substitute a constant:

"Gift"     => Ok(Keyword::Gift(GiftKind::Card)),   // every promised kind becomes Card
"Discover" => Ok(Keyword::Discover(0)),            // every N becomes 0
"Ripple"   => Ok(Keyword::Ripple(1)),              // every N becomes 1
"CumulativeUpkeep" => Ok(Keyword::CumulativeUpkeep(AbilityCost::Mana { cost: ManaCost::zero() })),

Serialization is correct — {"Gift":{"type":"TappedFish"}} is written faithfully — so the value is right in a live game and wrong the moment it is read back.

This is not limited to saved games. card-data.json is parsed through the same Deserialize (CardDatabase::from_json_str, called by engine-wasm::load_card_database at startup), so the corrupted values are what every game runs on. Against the shipped data:

Expected behavior

A keyword's payload survives deserialization. CR 702.174a (the promised gift kind), CR 702.19a (Ripple N), CR 702.24a (the upkeep cost).

Root cause

Same class as #3592 (For Mirrodin!) and #3598 (Specialize / Offering), but the silent half of it. Those were missing arms falling through to Keyword::Unknown — loud, and the coverage report flags them. These arms exist and return a well-formed keyword, so nothing reports anything; the card is simply a different card.

CumulativeUpkeep's substitution is deliberate and documented: legacy data stored the cost as a raw string needing the Oracle parser, which is not available on this path. That accommodation is sound, but it fires unconditionally, including for the typed AbilityCost this serializer itself writes.

Not a duplicate of #4040, which is a parse-time routing gap for granted cumulative upkeep (parse_keyword_from_oracle), not a deserialize-time one.

Suggested fix

Read the payload; keep the legacy fallback only where it was actually needed.

"Gift" => serde_json::from_value(data.clone())
    .map(Keyword::Gift)
    .map_err(|e| format!("GiftKind: {e}")),
"Discover" => Ok(Keyword::Discover(uint(data))),
"Ripple" => Ok(Keyword::Ripple(uint(data).max(1))),
"CumulativeUpkeep" => Ok(Keyword::CumulativeUpkeep(
    serde_json::from_value(data.clone())
        .unwrap_or(AbilityCost::Mana { cost: ManaCost::zero() }),
)),

Verified locally: cargo test --test integration 4819 passed | 0 failed, cargo test -p phase-engine --lib 18832 passed | 0 failed, clippy clean.

Steps to reproduce

Load a saved game containing Longstalk Brawl and cast it — the promise prompt reads "Promise a card". The client labels that button from WaitingFor::OptionalCostChoice's gift_kind and falls back to "a card" when the kind is absent or wrong, so the prompt reports the corrupted keyword faithfully.

As a test (crates/engine/tests/integration/):

#[test]
fn real_card_data_payloads_deserialize_faithfully() {
    // Surging Aether, exactly as shipped in card-data.json.
    let ripple: Keyword = serde_json::from_str(r#"{"Ripple": 4}"#).unwrap();
    assert_eq!(ripple, Keyword::Ripple(4));

    // Aboroth: cumulative upkeep is an EffectCost, not a mana cost.
    let upkeep: Keyword = serde_json::from_str(
        r#"{"CumulativeUpkeep": {"type": "EffectCost", "effect": {"type": "PutCounter",
            "counter_type": "M1M1", "count": {"type": "Fixed", "value": 1},
            "target": {"type": "SelfRef"}}}}"#,
    ).unwrap();
    assert!(!matches!(
        upkeep,
        Keyword::CumulativeUpkeep(AbilityCost::Mana { ref cost }) if cost.mana_value() == 0
    ));
}

Reverting the Ripple arm fails it at Ripple(1) vs Ripple(4); reverting the Gift arm fails a Longstalk Brawl cast driven through a save/load round-trip at Some(Card) vs Some(TappedFish).

Worth adding beyond the per-variant cases: a guard asserting a distinctive payload never round-trips into the constant that used to replace it. Without it, any future dropped payload still passes whichever per-variant test happens to use the substitute — which is exactly how this survived. Archival Whorl's Gift really is a card, so on that card the corrupted and correct values coincide.

Notes on scope

• Effect::GiftDelivery carries its own kind and round-trips, so the gift actually delivered was correct. The damage is confined to what reads the keyword — prompts, and anything gating on the keyword payload.
• Discover is currently latent: card-data.json has 0 cards carrying Keyword::Discover (37 mention "discover" in Oracle text and route elsewhere). The arm is still wrong; it just has no live victim today.

Logs/screenshots/game-state

Reported from a live P2P game after loading a save; the card-data.json extracts above are from the shipped file.

Diagnosed with an LLM (claude-opus-5). No PR opened: CONTRIBUTING.md routes crates/engine/ changes through /engine-implementer, which this session did not run.