feat(parser): explore replacement with scry prelude#3389
feat(parser): explore replacement with scry prelude#3389glorysr1209-png wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for parsing explore replacement effects (such as Twists and Turns) and includes a corresponding unit test. The review feedback highlights that the implementation uses fragile verbatim string matching instead of robust nom combinators, which violates style guide rules R1 and R3. Additionally, the parser fails to set the valid_card filter (allowing opponents' creatures to trigger the effect), and the unit test should be updated to assert this filter once implemented.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn parse_explore_replacement( | ||
| norm_lower: &str, | ||
| original_text: &str, | ||
| ) -> Option<ReplacementDefinition> { | ||
| let (rest, _) = tag::<_, _, OracleError<'_>>("if a creature you control would explore, ") | ||
| .parse(norm_lower) | ||
| .ok()?; | ||
| let (rest, _) = tag("instead you scry 1, then that creature explores").parse(rest).ok()?; | ||
| let (_, _) = all_consuming(opt(tag::<_, _, OracleError<'_>>("."))).parse(rest).ok()?; | ||
| let scry = AbilityDefinition::new( | ||
| AbilityKind::Spell, | ||
| Effect::Scry { | ||
| count: QuantityExpr::Fixed { value: 1 }, | ||
| target: TargetFilter::Controller, | ||
| }, | ||
| ); | ||
| let explore = AbilityDefinition::new(AbilityKind::Spell, Effect::Explore); | ||
| Some( | ||
| ReplacementDefinition::new(ReplacementEvent::Explore) | ||
| .execute(scry.sub_ability(explore)) | ||
| .description(original_text.to_string()), | ||
| ) | ||
| } |
There was a problem hiding this comment.
[HIGH] Decompose verbatim string matching and fix missing valid_card filter. Evidence:
crates/engine/src/parser/oracle_replacement.rs:6182.
Why it matters: Verbatim string matching is fragile and fails to support other creature types or scry amounts, and omitting thevalid_cardfilter incorrectly applies the replacement effect to opponents' creatures. Suggested fix: Use nom combinators to parse the creature filter and scry amount dynamically, and setvalid_cardon the definition.
fn parse_explore_replacement(
norm_lower: &str,
original_text: &str,
) -> Option<ReplacementDefinition> {
let (rest, _) = tag::<_, _, OracleError<'_>>("if ").parse(norm_lower).ok()?;
let (rest, filter_text) = take_until::<_, _, OracleError<'_>>(" would explore, ").parse(rest).ok()?;
let filter_text = alt((tag("a "), tag("an "), tag("any ")))
.parse(filter_text.trim())
.map_or(filter_text.trim(), |(r, _)| r);
let (filter, leftover) = parse_type_phrase(filter_text);
if !leftover.trim().is_empty() {
return None;
}
let (rest, _) = tag(" would explore, ").parse(rest).ok()?;
let (rest, _) = tag("instead you scry ").parse(rest).ok()?;
let (rest, scry_amount) = nom_primitives::parse_number(rest).ok()?;
let (rest, _) = tag(", then that creature explores").parse(rest).ok()?;
let (_, _) = all_consuming(opt(tag::<_, _, OracleError<'_>>("."))).parse(rest).ok()?;
// CR 701.20a: "you scry N" effect definition
let scry = AbilityDefinition::new(
AbilityKind::Spell,
Effect::Scry {
count: QuantityExpr::Fixed { value: scry_amount as i32 },
target: TargetFilter::Controller,
},
);
// CR 701.44a: "that creature explores" effect definition
let explore = AbilityDefinition::new(AbilityKind::Spell, Effect::Explore);
// CR 614.1a: Construct the replacement definition with the parsed filter as valid_card
Some(
ReplacementDefinition::new(ReplacementEvent::Explore)
.valid_card(filter)
.execute(scry.sub_ability(explore))
.description(original_text.to_string()),
)
}References
- Rule R1 requires using nom combinators and avoiding verbatim string matching for parsing dispatch. Rule R3 requires parameterizing parsers rather than proliferating leaf-level variants. (link)
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts.
| #[test] | ||
| fn parses_explore_replacement_with_scry() { | ||
| let def = parse_replacement_line( | ||
| "If a creature you control would explore, instead you scry 1, then that creature explores.", | ||
| "Twists and Turns", | ||
| ).expect("explore replacement"); | ||
| assert_eq!(def.event, ReplacementEvent::Explore); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Assert valid_card filter in explore replacement test. Evidence:
crates/engine/src/parser/oracle_replacement.rs:11855.
Why it matters: Asserting thevalid_cardfilter ensures that the replacement effect is correctly scoped to the appropriate creature and prevents future regressions. Suggested fix: Add an assertion verifying thatdef.valid_cardmatches the expected creature filter.
#[test]
fn parses_explore_replacement_with_scry() {
let def = parse_replacement_line(
"If a creature you control would explore, instead you scry 1, then that creature explores.",
"Twists and Turns",
).expect("explore replacement");
assert_eq!(def.event, ReplacementEvent::Explore);
assert_eq!(
def.valid_card,
Some(TargetFilter::Typed(TypedFilter {
type_filters: vec![TypeFilter::Creature],
controller: Some(ControllerRef::You),
properties: vec![],
}))
);
}365f47f to
8d304d6
Compare
8d304d6 to
17fc0c4
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review — changes requested (parser-only; runtime never consumes it)
Thanks — the parser arm is clean (combinator-based, discriminating parse test). But this can't fix Twists and Turns on its own, so it isn't mergeable yet.
The PR emits ReplacementDefinition::new(ReplacementEvent::Explore), but nothing consumes that event at runtime: grep -rn "ReplacementEvent::Explore" crates/engine/src/game/ is empty, and the explore resolver (crates/engine/src/game/effects/explore.rs::resolve) never calls replace_event/proposes a ProposedEvent::Explore. So the parsed replacement is a silent no-op — the creature still explores with no scry prelude. (This is the recurring "parsed AST not consumed" trap: a correct parse field that no resolver reads.)
Making this card actually work needs the engine side — the explore resolver must propose the explore event through the replacement pipeline and an applier must run the scry prelude before the reveal. That's the engine-implementer work being attempted in the competing PR #3485 (which I've also sent back for a plan — its interception was at the wrong explore entry point). These two PRs collide on oracle_replacement.rs and the same card; one should win.
Two smaller parser notes for whoever lands the combined fix:
- The arm
tags the entire tail ("instead you scry 1, then that creature explores") as one literal, so it only matches scry-1 verbatim — card-shaped rather than class-shaped. Consider parsing the scry count + the explore clause compositionally. Effect::Scry { target: TargetFilter::Controller }resolves the scry to the replacement source's controller; it should be the explorer's controller (matters for the no-control creature variant).
Recommend coordinating with #3485 into one PR that includes the runtime, a discriminating pipeline test (creature explores → scry happens first → explore resolves, failing without the change), and committed snapshots.
|
Heads-up: the explore-replacement (scry prelude) mechanic has landed via #3499, which implements it end-to-end (runtime This PR is parser-only and currently shows CONFLICTING against main, and the mechanic is now fully covered, so it's superseded — recommend closing. Thanks for the contribution. |
Summary
Closes 3388. High-score replacement parser gap: explore replacement with scry prelude (Twists and Turns class). Token score ~2.
Changes
crates/engine/src/parser/oracle_replacement.rswith focused parser arm + unit testTest plan
#[test]inoracle_replacement.rscovers representative Oracle textcargo test -p engine(authoritative gate — not run locally)