Skip to content

feat(parser): explore replacement with scry prelude#3389

Open
glorysr1209-png wants to merge 1 commit into
phase-rs:mainfrom
glorysr1209-png:feat/03-explore-scry-replacement
Open

feat(parser): explore replacement with scry prelude#3389
glorysr1209-png wants to merge 1 commit into
phase-rs:mainfrom
glorysr1209-png:feat/03-explore-scry-replacement

Conversation

@glorysr1209-png

Copy link
Copy Markdown
Contributor

Summary

Closes 3388. High-score replacement parser gap: explore replacement with scry prelude (Twists and Turns class). Token score ~2.

Changes

  • Extend crates/engine/src/parser/oracle_replacement.rs with focused parser arm + unit test
  • Minimal diff scoped to one high-leverage replacement pattern family

Test plan

  • New #[test] in oracle_replacement.rs covers representative Oracle text
  • CI cargo test -p engine (authoritative gate — not run locally)

@gemini-code-assist gemini-code-assist Bot left a comment

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.

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.

Comment on lines +6182 to +6204
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()),
)
}

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.

high

[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 the valid_card filter incorrectly applies the replacement effect to opponents' creatures. Suggested fix: Use nom combinators to parse the creature filter and scry amount dynamically, and set valid_card on 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
  1. 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)
  2. 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.

Comment on lines +11855 to +11862
#[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);
}

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.

medium

[MEDIUM] Assert valid_card filter in explore replacement test. Evidence: crates/engine/src/parser/oracle_replacement.rs:11855.
Why it matters: Asserting the valid_card filter ensures that the replacement effect is correctly scoped to the appropriate creature and prevents future regressions. Suggested fix: Add an assertion verifying that def.valid_card matches 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![],
            }))
        );
    }

@glorysr1209-png glorysr1209-png force-pushed the feat/03-explore-scry-replacement branch 2 times, most recently from 365f47f to 8d304d6 Compare June 15, 2026 17:30

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@matthewevans

Copy link
Copy Markdown
Member

Heads-up: the explore-replacement (scry prelude) mechanic has landed via #3499, which implements it end-to-end (runtime ProposedEvent::Explore seam + replace_event routing in effects/explore.rs + parser arm + a discriminating runtime test). That PR is approved and queued.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants