Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions client/src/components/stack/StackEntry.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down
67 changes: 67 additions & 0 deletions client/src/components/stack/__tests__/StackEntry.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<StackEntry entry={entry} index={0} isTop isPending cardSize={{ width: 120, height: 168 }} />,
);

// 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
// `<img>` 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(
<StackEntry entry={entry} index={0} isTop isPending cardSize={{ width: 120, height: 168 }} />,
);

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,
Expand Down
100 changes: 83 additions & 17 deletions crates/engine/src/game/triggers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 <name>" 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<crate::types::game_state::WaitingFor> {
Expand All @@ -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();
Expand Down Expand Up @@ -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 <name>" 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 <name>" 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),
Expand Down
Original file line number Diff line number Diff line change
@@ -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<String> = 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"
);
}
1 change: 1 addition & 0 deletions crates/engine/tests/integration/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading