diff --git a/client/src/components/stack/StackEntry.tsx b/client/src/components/stack/StackEntry.tsx index 03dd969c44..4221cc4852 100644 --- a/client/src/components/stack/StackEntry.tsx +++ b/client/src/components/stack/StackEntry.tsx @@ -71,11 +71,21 @@ export function StackEntry({ entry, index, isTop, isPending, cardSize, style, on // Prefer the engine-pre-resolved source name on triggered abilities (so the // display layer doesn't dereference ObjectId -> GameObject -> name itself). // Fall back to the objects map for spells/activated entries that don't carry - // a captured name, and to "Unknown" for synthetic game-rule triggers whose - // source_id is ObjectId(0). + // a captured name. + // + // There is deliberately NO last-resort literal here. This line used to end in + // `|| "Unknown"` for the rule-defined sourceless abilities this engine + // constructs (CR 725.2 monarch, CR 726.2 initiative, CR 728.1 rad counters, + // and CR 702.179d speed). CR 113.7 defines an ability's source; CR 113.8 + // instead defines its controller. (CR 901.8 separately gives Planechase's + // planeswalking ability no source.) "Unknown" is game-facing text no rule + // ever produced, invented by the display layer because the wire carried + // nothing. The engine names those abilities now, so the invention has nothing + // left to cover; if a name is ever missing again, an empty label is the honest + // answer and the engine-side guard is what should fail. const triggerSourceName = entry.kind.type === "TriggeredAbility" ? entry.kind.data.source_name : undefined; - const sourceName = details?.source_name || triggerSourceName || sourceObj?.name || "Unknown"; + const sourceName = details?.source_name || triggerSourceName || sourceObj?.name || ""; const imageLookup = sourceObj ? cardImageLookup(sourceObj) : { name: "", faceIndex: 0, oracleId: undefined, faceName: undefined }; diff --git a/client/src/components/stack/__tests__/StackEntry.test.tsx b/client/src/components/stack/__tests__/StackEntry.test.tsx index 419c0ba9e6..d12754932f 100644 --- a/client/src/components/stack/__tests__/StackEntry.test.tsx +++ b/client/src/components/stack/__tests__/StackEntry.test.tsx @@ -144,6 +144,73 @@ describe("StackEntry", () => { expect(screen.getByText("Revoke")).toBeInTheDocument(); }); + it("labels a sourceless rule ability with the engine's name and invents nothing", () => { + // CR 113.7 defines an ability's source; the rules for these engine-modeled + // inherent abilities (CR 725.2 monarch, CR 726.2 initiative, CR 728.1 rad + // counters, CR 702.179d speed) give them none. CR 113.8 instead defines an + // ability's controller, and CR 901.8 separately does the same for + // Planechase's planeswalking ability. `objects` holds nothing for this + // entry, so the name has to come off the wire — this line used to fall + // through to a literal "Unknown", which is game-facing text no rule produces. + const entry: StackEntryType = buildStackEntry({ + id: 91, + source_id: 0, + controller: 0, + kind: { + type: "TriggeredAbility", + data: { source_id: 0, ability: { targets: [] }, source_name: "Start your engines!" }, + }, + }); + const gameState = createGameState({ objects: {}, stack: [entry] }); + + act(() => { + useGameStore.setState({ gameState, waitingFor: gameState.waiting_for }); + }); + + render( + , + ); + + // Asserted on the image's alt text, not on rendered copy: the file-level + // `useCardImage` mock always hands back a src, so this entry takes the + // `` branch and `sourceName` surfaces as `alt`. Same variable either + // way — the `CardArtFallback` branch feeds it the identical string. + expect(screen.getByAltText("Start your engines!")).toBeInTheDocument(); + expect(screen.queryByAltText("Unknown")).not.toBeInTheDocument(); + }); + + it("invents no name when the wire carries none", () => { + // The row above cannot reach the deleted `|| "Unknown"` literal: it supplies + // a `source_name`, so the fallback chain short-circuits before the last + // term. This row is the one that exercises it — an entry with no source + // object AND no name, which is exactly the wire shape that produced the + // reported blank "Unknown" card. + // + // An empty label is the honest answer here. Inventing game-facing text is + // not the display layer's call, and the engine-side guard is what should + // fail if a name ever goes missing again. + const entry: StackEntryType = buildStackEntry({ + id: 92, + source_id: 0, + controller: 0, + kind: { + type: "TriggeredAbility", + data: { source_id: 0, ability: { targets: [] }, source_name: "" }, + }, + }); + const gameState = createGameState({ objects: {}, stack: [entry] }); + + act(() => { + useGameStore.setState({ gameState, waitingFor: gameState.waiting_for }); + }); + + render( + , + ); + + expect(screen.queryByAltText("Unknown")).not.toBeInTheDocument(); + }); + it("shows a discoverable yield button on a triggered ability and opens the menu on tap", () => { const entry: StackEntryType = buildStackEntry({ id: 88, diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index a6449d4e37..e41f7e7ba0 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -5280,7 +5280,8 @@ fn collect_pending_triggers_with_collection( let draw_ability = ResolvedAbility::new(draw_effect, Vec::new(), ObjectId(0), monarch_id); let trig_def = TriggerDefinition::new(TriggerMode::Phase) - .description("Monarch draw (CR 725.2)".to_string()); + // CR 725.2: the label a viewer sees for this sourceless ability. + .description("The Monarch".to_string()); pending.push(PendingTriggerContext::single(PendingTrigger { source_id: ObjectId(0), controller: monarch_id, @@ -5316,7 +5317,9 @@ fn collect_pending_triggers_with_collection( let venture_ability = ResolvedAbility::new(venture_effect, Vec::new(), ObjectId(0), init_holder); let trig_def = TriggerDefinition::new(TriggerMode::Phase) - .description("Initiative upkeep venture (CR 725.2)".to_string()); + // CR 726.2 (NOT 725.2, which is the monarch): the label a viewer + // sees for this sourceless ability. + .description("The Initiative".to_string()); pending.push(PendingTriggerContext::single(PendingTrigger { source_id: ObjectId(0), controller: init_holder, @@ -5456,7 +5459,7 @@ fn collect_pending_triggers_with_collection( ); take_init.set_trigger_source_recursive(source_context); let trig_def = TriggerDefinition::new(TriggerMode::DamageDone) - .description("Initiative steal (CR 725.2)".to_string()); + .description("Initiative steal (CR 726.2)".to_string()); pending.push(PendingTriggerContext::single(PendingTrigger { source_id: *source_id, controller: new_holder, @@ -5502,7 +5505,8 @@ fn collect_pending_triggers_with_collection( trigger_controller, ); let trig_def = TriggerDefinition::new(TriggerMode::LifeLost) - .description("Start your engines! (CR 702.179d)".to_string()); + // CR 702.179d: the label a viewer sees for this sourceless ability. + .description("Start your engines!".to_string()); pending.push(PendingTriggerContext::single(PendingTrigger { source_id: speed_key_source(), controller: trigger_controller, @@ -5550,7 +5554,8 @@ fn collect_pending_triggers_with_collection( active, ); let trig_def = TriggerDefinition::new(TriggerMode::Phase) - .description("Rad counters (CR 728.1)".to_string()); + // CR 728.1: the label a viewer sees for this sourceless ability. + .description("Rad counters".to_string()); pending.push(PendingTriggerContext::single(PendingTrigger { source_id: ObjectId(0), controller: active, @@ -6887,6 +6892,48 @@ pub fn drain_order_triggers_with_identity( /// CR 603.3b: Build the next `WaitingFor::OrderTriggers` prompt by finding /// the earliest unordered group in APNAP order. /// Returns `None` if every group is `ordered` (caller should dispatch). +/// The name a viewer shows for a triggered ability's source. +/// +/// CR 113.7 defines an ability's source as the object that generated it; CR +/// 113.8 instead defines an ability's controller. The inherent ability rules +/// modeled here directly make four abilities sourceless: CR 725.2 (the monarch), +/// CR 726.2 (the initiative), CR 728.1 (rad counters), and CR 702.179d (speed). +/// CR 901.8 separately makes Planechase's planeswalking ability sourceless. The +/// engine models the four triggers here faithfully, which leaves the wire with +/// nothing to name. +/// +/// A sourceless ability names itself: the rule that mints it IS its identity, +/// which is what `description` already carries — the same short label +/// ("Prowess", "Storm", "Cascade") every other engine-authored trigger uses. +/// Printed helper cards do exactly this too, standing a card face in for a rule +/// that has no object behind it. +/// +/// Single authority on purpose: the ordering prompt and the stack entry ask the +/// same question and must not answer it differently. Neither may push the +/// question to the display layer, which is not allowed to derive game-facing +/// content. +fn trigger_source_display_name( + state: &GameState, + source_id: ObjectId, + ability: &ResolvedAbility, + description: Option<&str>, +) -> String { + // CR 400.7 + CR 113.7a: the trigger's own captured source is the most + // faithful answer, and the only one that survives the source leaving its + // zone — `lki()` is what makes "From " still right for a dead source. + if let Some(source) = ability.trigger_source.as_ref() { + return source.source_read(state).lki().name; + } + // CR 603.7d: a delayed trigger carries no captured source, but `source_id` + // still points at the live object that set it up. Reading the objects map + // here is what the ordering prompt did before this helper existed, and + // dropping it would have blanked every delayed and co-triggered entry. + if let Some(object) = state.objects.get(&source_id) { + return object.name.clone(); + } + description.unwrap_or_default().to_string() +} + fn build_next_order_triggers_prompt( state: &GameState, ) -> Option { @@ -6899,11 +6946,12 @@ fn build_next_order_triggers_prompt( .iter() .map(|ctx| PendingTriggerSummary { source_id: ctx.pending.source_id, - source_name: state - .objects - .get(&ctx.pending.source_id) - .map(|o| o.name.clone()) - .unwrap_or_default(), + source_name: trigger_source_display_name( + state, + ctx.pending.source_id, + &ctx.pending.ability, + ctx.pending.description.as_deref(), + ), description: ctx.pending.description.clone().unwrap_or_default(), }) .collect(); @@ -7431,13 +7479,31 @@ fn push_pending_trigger_to_stack_with_firing_and_duration_events( .insert(entry_id, trigger_events); } // Capture the observed source name at stack-push time so viewers can render - // "From " without rebinding an old trigger to a reused id. Synthetic - // game-rule triggers carry no trigger source and deliberately display no name. - let source_name = ability - .trigger_source - .as_ref() - .map(|source| source.source_read(state).lki().name) - .unwrap_or_default(); + // "From " without rebinding an old trigger to a reused id. + let source_name = + trigger_source_display_name(state, source_id, &ability, description.as_deref()); + // The four source-less inherent triggers constructed here (CR 725.2 monarch, + // CR 726.2 initiative, CR 728.1 rad counters, CR 702.179d speed) use the + // `ObjectId(0)` no-source sentinel. CR 113.7 governs source; CR 113.8 governs + // controller, while CR 901.8 separately makes the planeswalking ability + // sourceless. A viewer cannot dereference the sentinel, so such an entry MUST + // carry its own name or the display layer is left to invent one. + // + // Keyed on the sentinel rather than on a list of the four rules, so a fifth + // sourceless rule is covered the day it is written. + // + // Two weaker-looking scopes were measured and rejected as the wrong shape: + // `!source_name.is_empty()` for every trigger fails 53 existing tests + // (delayed triggers, CR 603.7d, carry neither a captured source nor a + // description), and `state.objects.contains_key(&source_id)` fails 10 more + // whose fixtures name a source id they never insert. Both of those entries + // are still nameable — the first from `source_id`, the second in a real game + // where the id exists — so neither is this defect. See the PR's open-gap note. + debug_assert!( + source_id != ObjectId(0) || !source_name.is_empty(), + "a source-less rule ability must carry its own name — the display layer \ + has no object from which to read one" + ); let crime_candidate = super::casting::targets_commit_crime( state, &super::ability_utils::flatten_targets_in_chain(&ability), diff --git a/crates/engine/tests/integration/inherent_rule_trigger_display_name.rs b/crates/engine/tests/integration/inherent_rule_trigger_display_name.rs new file mode 100644 index 0000000000..f545ef9531 --- /dev/null +++ b/crates/engine/tests/integration/inherent_rule_trigger_display_name.rs @@ -0,0 +1,123 @@ +//! CR 113.7 defines an ability's source; these inherent triggered abilities have +//! no source object by their own rules. +//! +//! CR 725.2 (monarch), CR 726.2 (initiative), CR 728.1 (rad counters) and +//! CR 702.179d (speed) each say so in the same words — "these triggered +//! abilities have no source". CR 113.8 instead defines an ability's controller; +//! CR 901.8 separately gives Planechase's planeswalking ability no source. The +//! engine models these four constructed triggers with `ObjectId(0)`, which +//! resolves to no `GameObject`. +//! +//! The consequence is a display hole, not a rules hole: `StackEntryKind:: +//! TriggeredAbility::source_name` is filled by looking `source_id` up in the +//! objects map, so these four entries reach the client with an EMPTY name. The +//! client has nothing to render and substitutes a name of its own — which is the +//! display layer deriving game-facing content, the one thing it must never do. +//! +//! Reported from a real game: increasing speed off combat damage briefly showed a +//! blank card on the stack. +//! +//! These rows assert the wire contract the client depends on: a stack entry +//! always carries a name for its own source, whether or not that source is an +//! object. + +use engine::game::scenario::{GameScenario, P0, P1}; +use engine::types::actions::GameAction; +use engine::types::game_state::StackEntryKind; +use engine::types::phase::Phase; + +/// The `source_name` of the single triggered ability on the stack. +/// +/// Panics rather than returning `Option` so a row that fails to produce its +/// trigger at all reports "no triggered ability on the stack" instead of +/// silently passing an emptiness check it never reached. +fn only_trigger_source_name(runner: &engine::game::scenario::GameRunner) -> String { + let names: Vec = runner + .state() + .stack + .iter() + .filter_map(|entry| match &entry.kind { + StackEntryKind::TriggeredAbility { source_name, .. } => Some(source_name.clone()), + StackEntryKind::Spell { .. } + | StackEntryKind::ActivatedAbility { .. } + | StackEntryKind::KeywordAction { .. } => None, + }) + .collect(); + assert_eq!( + names.len(), + 1, + "expected exactly one triggered ability on the stack, found {names:?}" + ); + names.into_iter().next().expect("length was asserted") +} + +/// CR 725.2: "At the beginning of the monarch's end step, that player draws a +/// card." No source. +#[test] +fn the_monarch_draw_trigger_names_its_own_source() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let mut runner = scenario.build(); + runner.state_mut().monarch = Some(P0); + runner.advance_to_end_step(); + + // Asserted by VALUE, not by `!is_empty()`: an emptiness check passes on any + // placeholder the engine might grow later, which is the very thing this row + // exists to forbid. + assert_eq!( + only_trigger_source_name(&runner), + "The Monarch", + "CR 725.2's ability has no source object, so it names itself — otherwise \ + the client has to invent a name" + ); +} + +/// CR 702.179d: "Whenever one or more opponents lose life during your turn, if +/// your speed is less than 4, your speed increases by 1." No source. +/// +/// This is the row the player reported. It differs from the monarch row only in +/// which rule mints the trigger, which is the point: the hole is in the class of +/// sourceless rule triggers, not in one designation. +#[test] +fn the_speed_increase_trigger_names_its_own_source() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let drain = scenario + .add_spell_to_hand_from_oracle(P0, "Drain", true, "Target player loses 1 life.") + .id(); + scenario.add_basic_land(P0, engine::types::mana::ManaColor::Black); + let mut runner = scenario.build(); + // CR 702.179b: speed exists only once a rule or effect sets it. Starting at + // 1 keeps the trigger available (CR 702.179d gates on "less than 4") without + // needing a `Start your engines!` permanent, which would add a second + // ability to the board and blur which trigger the assertion reads. + for player in runner.state_mut().players.iter_mut() { + if player.id == P0 { + player.speed = Some(1); + } + } + // `.resolve()` drives the stack to empty, which would resolve the speed + // trigger too and leave nothing to read. Commit the spell, then pass + // priority just far enough for the drain to resolve and its trigger to land. + drop(runner.cast(drain).target_player(P1).commit()); + for _ in 0..8 { + if runner + .state() + .stack + .iter() + .any(|entry| matches!(entry.kind, StackEntryKind::TriggeredAbility { .. })) + { + break; + } + if runner.act(GameAction::PassPriority).is_err() { + break; + } + } + + assert_eq!( + only_trigger_source_name(&runner), + "Start your engines!", + "CR 702.179d's ability has no source object, so it names itself — \ + otherwise the client has to invent a name" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 705cef5005..e73bb14e7e 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -305,6 +305,7 @@ mod hunters_insight_combat_draw; mod ichneumon_druid; mod inevitable_betrayal_no_mana_cost; mod infantry_shield_mobilize_grant; +mod inherent_rule_trigger_display_name; mod innocent_bystander_whole_event_damage; mod inspiring_call_indestructible_grant; mod integration_adventure;