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
68 changes: 58 additions & 10 deletions crates/engine/src/parser/oracle_nom/condition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,19 @@ fn parse_control_named_pair(input: &str) -> OracleResult<'_, StaticCondition> {
filter: Some(inject_controller_you(filter)),
})
.collect();
Ok((rest_after_pair, connector.combine(conditions)))
let condition = match connector {
Some(connector) => connector.combine(conditions),
None => match conditions.as_slice() {
[condition] => condition.clone(),
_ => {
return Err(nom::Err::Error(nom::error::Error::new(
input,
nom::error::ErrorKind::Fail,
)));
}
},
};
Ok((rest_after_pair, condition))
}

fn parse_control_named_type_filter<'a>(
Expand All @@ -836,7 +848,7 @@ fn parse_control_named_type_filter<'a>(
fn parse_control_named_pair_members<'a>(
input: &'a str,
filter_base: &TargetFilter,
) -> OracleResult<'a, (Vec<TargetFilter>, ControlNamedConnector)> {
) -> OracleResult<'a, (Vec<TargetFilter>, Option<ControlNamedConnector>)> {
if let Some((mut rest, first_name, connector)) =
find_repeated_typed_control_named_connector(input)
{
Expand All @@ -862,7 +874,7 @@ fn parse_control_named_pair_members<'a>(
nom::error::ErrorKind::Fail,
)));
}
None => return Ok((next_rest, (filters, connector))),
None => return Ok((next_rest, (filters, Some(connector)))),
}
}
}
Expand Down Expand Up @@ -937,7 +949,7 @@ fn parse_control_named_typed_member(
fn parse_shared_type_control_named_pair<'a>(
input: &'a str,
filter_base: &TargetFilter,
) -> OracleResult<'a, (Vec<TargetFilter>, ControlNamedConnector)> {
) -> OracleResult<'a, (Vec<TargetFilter>, Option<ControlNamedConnector>)> {
let (rest_after_list, names_text) = parse_control_named_final_name(input)?;
let (names, connector) = parse_shared_control_named_list(input, names_text)?;
let filters = names
Expand All @@ -950,14 +962,18 @@ fn parse_shared_type_control_named_pair<'a>(
fn parse_shared_control_named_list<'a>(
error_input: &'a str,
names_text: &'a str,
) -> Result<(Vec<&'a str>, ControlNamedConnector), nom::Err<OracleError<'a>>> {
) -> Result<(Vec<&'a str>, Option<ControlNamedConnector>), nom::Err<OracleError<'a>>> {
let Some((connector_index, connector_len, connector, serial_comma)) =
find_shared_control_named_final_connector(names_text)
else {
return Err(nom::Err::Error(nom::error::Error::new(
error_input,
nom::error::ErrorKind::Fail,
)));
let name = names_text.trim();
if name.is_empty() {
return Err(nom::Err::Error(nom::error::Error::new(
error_input,
nom::error::ErrorKind::Fail,
)));
}
return Ok((vec![name], None));
Comment on lines +965 to +976

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add a verified CR annotation for singleton named-card parsing.

Lines 965-976 make a connector-less named card valid as one condition. This rules-facing behavior has no verified CR <number>: <description> annotation. Add an annotation that describes the named-card rule basis and the singleton condition semantics.

As per coding guidelines: “Implement MTG behavior according to the Comprehensive Rules; verify the relevant CR section before completion, and annotate rules-related code with a verified CR number and description.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/parser/oracle_nom/condition.rs` around lines 965 - 976, Add
a verified CR annotation directly above the connector-less singleton handling in
the named-card parsing function, documenting the named-card rule basis and that
a trimmed non-empty name is accepted as one condition. Use the applicable
Comprehensive Rules number and description, preserving the existing empty-name
error and singleton return behavior.

Sources: Coding guidelines, Path instructions

};
let before_final = names_text[..connector_index].trim();
let final_name = names_text[connector_index + connector_len..].trim();
Expand All @@ -979,7 +995,7 @@ fn parse_shared_control_named_list<'a>(
nom::error::ErrorKind::Fail,
)));
}
Ok((names, connector))
Ok((names, Some(connector)))
}

fn find_shared_control_named_final_connector(
Expand Down Expand Up @@ -11242,6 +11258,38 @@ mod tests {
)));
}

#[test]
fn you_control_single_named_creature_keeps_another_and_effect_boundary() {
// Faerie Miscreant class: a singleton named object is a single
// presence condition, not a malformed one-member conjunction. The
// trailing effect must remain available to the trigger parser.
let (rest, condition) = parse_inner_condition(
"you control another creature named faerie miscreant, draw a card",
)
.unwrap();
assert_eq!(rest, ", draw a card");
let filter = typed_presence(&condition);
assert!(filter.type_filters.contains(&TypeFilter::Creature));
assert_eq!(filter.controller, Some(ControllerRef::You));
assert!(filter.properties.iter().any(|property| matches!(
property,
FilterProp::Named { name } if name == "faerie miscreant"
)));
assert!(filter
.properties
.iter()
.any(|property| matches!(property, FilterProp::Another)));
assert!(filter.properties.iter().any(|property| matches!(
property,
FilterProp::InZone { zone } if *zone == Zone::Battlefield
)));
}

#[test]
fn you_control_named_rejects_empty_singleton_name() {
assert!(parse_control_named_pair("you control a creature named , draw a card").is_err());
}

#[test]
fn test_you_control_named_pair() {
// CR 201.2: Scepter of Empires class — "you control [type]
Expand Down
64 changes: 64 additions & 0 deletions crates/engine/src/parser/oracle_trigger_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,70 @@ fn extract_hand_cast_battlefield_threshold_leaves_effect_text() {
}));
}

/// Issue #7154 — Faerie Miscreant's singleton named-card intervening-if must
/// lower through the full Oracle pipeline to a live `ControlsType` condition.
/// The source itself is excluded by `another`; the exact name remains a card
/// name filter rather than being swallowed into the draw effect.
#[test]
fn faerie_miscreant_single_named_intervening_if_parses_to_draw_trigger() {
const ORACLE: &str =
"Flying\nWhen this creature enters, if you control another creature named Faerie Miscreant, draw a card.";

let parsed = parse_oracle_text(
ORACLE,
"Faerie Miscreant",
&["Flying".to_string()],
&["Creature".to_string()],
&["Faerie".to_string()],
);
assert_eq!(
parsed.triggers.len(),
1,
"parsed triggers: {:?}",
parsed.triggers
);
let trigger = &parsed.triggers[0];
assert_eq!(trigger.mode, TriggerMode::ChangesZone);
assert_eq!(trigger.destination, Some(Zone::Battlefield));

let TriggerCondition::ControlsType {
filter: TargetFilter::Typed(filter),
} = trigger
.condition
.as_ref()
.expect("intervening-if condition")
else {
panic!(
"expected ControlsType named presence, got {:?}",
trigger.condition
);
};
assert!(filter.type_filters.contains(&TypeFilter::Creature));
assert_eq!(filter.controller, Some(ControllerRef::You));
assert!(filter
.properties
.iter()
.any(|property| matches!(property, FilterProp::Another)));
assert!(filter.properties.iter().any(|property| matches!(
property,
FilterProp::Named { name } if name == "faerie miscreant"
)));

let execute = trigger.execute.as_ref().expect("draw body");
assert!(matches!(
execute.effect.as_ref(),
Effect::Draw {
count: QuantityExpr::Fixed { value: 1 },
target: TargetFilter::Controller,
}
));
assert!(
!matches!(execute.effect.as_ref(), Effect::Unimplemented { .. }),
"Faerie Miscreant body must not fall back to Unimplemented: {:?}",
execute.effect
);
}

/// The zoned cast-and-condition parser shares its condition branch across all
/// supported origin zones. These focused extractor cases prove the graveyard
/// owner scope and shared-exile owner scope without inventing a runtime card.
Expand Down
133 changes: 133 additions & 0 deletions crates/engine/tests/integration/issue_7154_faerie_miscreant.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
//! Regression for issue #7154 — Faerie Miscreant's named-card intervening-if.
//!
//! The exact Oracle condition is checked when Faerie Miscreant enters and again
//! while its trigger resolves (CR 603.4). These tests use the real cast,
//! battlefield-entry, trigger-collection, and resolution pipeline.

use engine::game::scenario::{GameRunner, GameScenario, P0};
use engine::types::ability::{FilterProp, TargetFilter, TriggerCondition, TypeFilter};
use engine::types::actions::GameAction;
use engine::types::game_state::StackEntryKind;
use engine::types::mana::ManaCost;
use engine::types::phase::Phase;
use engine::types::zones::Zone;
use engine::types::ObjectId;

const FAERIE_MISCREANT: &str =
"Flying\nWhen this creature enters, if you control another creature named Faerie Miscreant, draw a card.";
const DESTROY_TARGET_CREATURE: &str = "Destroy target creature.";

fn has_named_companion_condition(runner: &GameRunner, source: ObjectId) -> bool {
runner.state().objects[&source]
.trigger_definitions
.as_slice()
.iter()
.any(|entry| matches!(
&entry.definition.condition,
Some(TriggerCondition::ControlsType {
filter: TargetFilter::Typed(filter),
}) if filter.type_filters.contains(&TypeFilter::Creature)
&& filter.properties.iter().any(|property| matches!(
property,
FilterProp::Named { name } if name == "faerie miscreant"
))
&& filter.properties.iter().any(|property| matches!(property, FilterProp::Another))
))
}

fn faerie_on_stack(runner: &GameRunner, source: ObjectId) -> bool {
runner.state().stack.iter().any(|entry| {
matches!(
&entry.kind,
StackEntryKind::TriggeredAbility { source_id, .. } if *source_id == source
)
})
}

#[test]
fn draws_when_another_faerie_miscreant_is_present() {
let mut scenario = GameScenario::new();
scenario.at_phase(Phase::PreCombatMain);
scenario.with_library_top(P0, &["Drawn card"]);
scenario.add_creature(P0, "Faerie Miscreant", 1, 1);
let faerie = scenario
.add_creature_to_hand_from_oracle(P0, "Faerie Miscreant", 1, 1, FAERIE_MISCREANT)
.with_mana_cost(ManaCost::zero())
.id();

let mut runner = scenario.build();
assert!(
has_named_companion_condition(&runner, faerie),
"reach-guard: exact Oracle must synthesize the named companion condition"
);

let outcome = runner.cast(faerie).resolve();
outcome.assert_hand_drawn(P0, 1);
}

#[test]
fn does_not_draw_without_another_faerie_miscreant() {
let mut scenario = GameScenario::new();
scenario.at_phase(Phase::PreCombatMain);
scenario.with_library_top(P0, &["Drawn card"]);
scenario.add_creature(P0, "Wrong Name Companion", 1, 1);
let faerie = scenario
.add_creature_to_hand_from_oracle(P0, "Faerie Miscreant", 1, 1, FAERIE_MISCREANT)
.with_mana_cost(ManaCost::zero())
.id();

let mut runner = scenario.build();
assert!(
has_named_companion_condition(&runner, faerie),
"reach-guard: the negative starts from the parsed named companion condition"
);

let outcome = runner.cast(faerie).resolve();
outcome.assert_hand_drawn(P0, 0);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

#[test]
fn companion_removed_after_trigger_fires_prevents_resolution_draw() {
let mut scenario = GameScenario::new();
scenario.at_phase(Phase::PreCombatMain);
scenario.with_library_top(P0, &["Drawn card"]);
let companion = scenario.add_creature(P0, "Faerie Miscreant", 1, 1).id();
let faerie = scenario
.add_creature_to_hand_from_oracle(P0, "Faerie Miscreant", 1, 1, FAERIE_MISCREANT)
.with_mana_cost(ManaCost::zero())
.id();
let destroy_spell = scenario
.add_spell_to_hand_from_oracle(P0, "Destroy Companion", true, DESTROY_TARGET_CREATURE)
.with_mana_cost(ManaCost::zero())
.id();

let mut runner = scenario.build();
runner.cast(faerie).commit();

let mut fired = false;
for _ in 0..12 {
let entered = runner.state().objects[&faerie].zone == Zone::Battlefield;
if entered && faerie_on_stack(&runner, faerie) {
fired = true;
break;
}
runner
.act(GameAction::PassPriority)
.expect("advance creature spell to its triggered ability");
}
assert!(
fired,
"reach-guard: the condition held on entry and Faerie Miscreant's trigger reached the stack"
);

let outcome = runner
.cast(destroy_spell)
.target_object(companion)
.resolve();
assert_eq!(
outcome.zone_of(companion),
Zone::Graveyard,
"the production Destroy pipeline must move the companion before the trigger rechecks"
);
outcome.assert_hand_drawn(P0, 0);
}
1 change: 1 addition & 0 deletions crates/engine/tests/integration/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,7 @@ mod issue_7063_library_reorder;
mod issue_7087_recruit_discard_provenance;
mod issue_709_regression;
mod issue_7151_moonlight_bargain;
mod issue_7154_faerie_miscreant;
mod issue_718_dina_sacrifice_draw;
mod issue_7212_recruit_sibling_trigger;
mod issue_7221_forage_trigger;
Expand Down
1 change: 0 additions & 1 deletion docs/parser-misparse-backlog.md
Original file line number Diff line number Diff line change
Expand Up @@ -970,7 +970,6 @@ This is the prioritized "fix N root causes → unlock M cards" backlog: the top
- Exert Influence
- Exile into Darkness
- Extraordinary Journey
- Faerie Miscreant
- Faller's Faithful
- Faramir, Field Commander
- Farideh, Devil's Chosen
Expand Down
Loading