feat(parser): explore replacement scry prelude and double explore

[kiannidev]

Summary

• Add parse_explore_replacement for creature-you-control explore replacements
• Cover Twists and Turns (scry prelude) and Topography Tracker (double explore)

Token score

Shared ReplacementEvent::Explore primitive — 2+ cards, high parser leverage.

Test plan

• cargo test -p engine --lib parses_explore_replacement

Closes #3388, Closes #3498.

Made with Cursor

[gemini-code-assist]

Code Review

This pull request introduces parsing support for explore replacement effects (e.g., Twists and Turns, Topography Tracker) in the engine's oracle replacement parser, complete with unit tests. However, the implementation violates the repository's hard architectural rule R1 by using raw string operations (.find()) for parsing dispatch instead of nom-based combinators. The feedback advises refactoring the parser to use nom_on_lower with decomposed combinators to ensure robust and idiomatic parsing.

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.

[gemini-code-assist]

[HIGH] Use of raw string .find() for parsing dispatch. Evidence: crates/engine/src/parser/oracle_replacement.rs:4640.
Why it matters: Raw string operations bypass the robust nom-based parser and violate the repository's hard architectural rule R1. Additionally, avoid verbatim string matching for compound Oracle phrases; instead, decompose them into modular, reusable parsers for constituent parts (like subjects) and compose them using combinators.
Suggested fix: Use nom_on_lower with decomposed combinators to extract the effect text.

fn parse_explore_replacement(lower: &str, original_text: &str) -> Option<ReplacementDefinition> {
    let ((), execute_text) = nom_on_lower(original_text, lower, |i| {
        value(
            (),
            (
                tag("if "),
                parse_subject,
                tag(" would explore, "),
                tag("instead "),
            ),
        )
        .parse(i)
    })?;
    let execute_text = execute_text.trim().trim_end_matches('.');

References

1. R1. Nom combinators on the first pass — no exceptions. Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. Avoid any new .find() used for parsing dispatch in non-test parser code. ^(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 (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.

[matthewevans]

Maintainer review — CHANGES REQUESTED

Two blocking issues: (1) the explore replacement is a parse-only no-op — nothing at runtime can ever fire it; and (2) this duplicates two open PRs.

1. Parse-only no-op — the replacement can never fire (BLOCKING)

ReplacementEvent::Explore exists in the enum (types/replacements.rs:73), but the runtime seam that would feed it does not:

• There is no ProposedEvent::Explore variant (grep ProposedEvent crates/engine/src/types/proposed_event.rs for Explore returns nothing).
• game/effects/explore.rs::resolve_single_explorer does not call replacement::replace_event for the explore itself — it only proposes AddCounter mid-resolution. So no proposed explore event is ever generated for a ReplacementEvent::Explore definition to replace.

Net effect: this PR parses Twists and Turns / Topography Tracker into a ReplacementDefinition that the engine will never apply. The parser test asserts def.event == Explore and def.execute.is_some() — both pass against a definition that is inert at runtime. There is no discriminating runtime test, because there is no runtime path to discriminate. Per the maintainer bar, a parsed AST field that is consumed nowhere is a silent no-op, not a feature.

The real seam this needs is in effects/explore.rs: add a ProposedEvent::Explore { object_id, applied } variant, route resolve_single_explorer through replace_event before the reveal/counter/land logic, and handle Execute/Prevented/NeedsChoice. That is engine work, not a parser-only change.

2. Duplicate of #3389 and #3485 (BLOCKING)

Three open PRs implement explore replacement with scry prelude for the same cards:

• #3499 (this PR, kiannidev) — parser only; inert as above.
• #3389 (glorysr1209-png) — parser only; same inert problem, but builds the execute chain with typed effects (Effect::Scry { count, target } .sub_ability(Effect::Explore)) instead of round-tripping through parse_effect_chain on the already-parsed remainder.
• #3485 (Kelvinchen03) — adds the runtime seam: ProposedEvent::Explore + a replace_event call in effects/explore.rs with Execute/Prevented/NeedsChoice handling, plus a runtime test that drives the explore pipeline.

#3485 is the most rules-correct base because it is the only one that makes the replacement actually fire. Two (here three) PRs for one mechanic are never both merged. Recommend: consolidate on #3485's runtime seam (it still needs the parser arm to produce the definition), and close #3499 + #3389 as superseded, or fold their parser arm into #3485.

3. Idiom note (non-blocking, for whichever base wins)

parse_explore_replacement re-feeds the post-instead remainder through parse_effect_chain(execute_text, AbilityKind::Spell). The #3389 approach of composing the typed Effect::Scry { target: Controller }.sub_ability(Effect::Explore) directly is cleaner and avoids a re-parse of text the dispatcher already classified. Also, the inner guard re-checks "if a creature you control would explore" after the outer "would explore" scan — collapse to one combinator.

Action: not enqueued. Recommend closing in favor of #3485 (with its parser arm completed), or re-implementing here at the effects/explore.rs runtime seam.

[kiannidev]

Thanks for the detailed trace — both blocking points addressed in the latest commits:

1. Runtime seam (was parse-only no-op): Added ProposedEvent::Explore { object_id, applied }, registered explore matcher/applier in build_replacement_registry(), and routed resolve_single_explorer through replace_event before reveal/counter/land logic. Nested execute chains (scry → explore, double explore) run via resolve_explore_effect without re-entering replacement. Runtime test: explore_scry_prelude_replacement_runs_before_explore.

2. Duplicate PRs: This branch now includes the runtime work from Implement explore replacement with scry prelude (Twists and Turns) #3485's seam, not parser-only. Happy to defer to whichever PR you prefer to land, but this one is end-to-end wired.

Idiom: Replaced forbidden .find("instead ") with split_once_on_lower (parser combinator gate). Acknowledged on typed Effect::Scry composition vs re-parse — can follow up if you want that refactor.

Ready for re-review.

[matthewevans]

Maintainer re-review — APPROVED (runtime seam now present; this is the cluster winner) — head f931a5c

My prior blocker was that the explore replacement was a parse-only no-op with no runtime seam. That is resolved end-to-end:

1. ProposedEvent::Explore { object_id, applied } added (types/proposed_event.rs), wired through applied/affected_player/object_id.
2. effects/explore.rs::resolve now routes through replacement::replace_event BEFORE the reveal/counter/land logic (split into the non-re-entrant resolve_explore_effect, which the applier calls so nested explores don't re-enter the replacement). Execute / Prevented / NeedsChoice all handled.
3. matcher + applier registered in build_replacement_registry() (replacement.rs), plus the handle_replacement_choice arm.
4. Parser arm (parse_explore_replacement) produces the ReplacementDefinition from Oracle text for the Twists-and-Turns / Topography-Tracker class.
5. Discriminating runtime test explore_scry_prelude_replacement_runs_before_explore drives resolve() and asserts the creature still ends up with a +1/+1 counter after the scry prelude — i.e. the replacement fires AND the explore still happens.

This is the only PR in the #3389/#3485/#3499 cluster that lands the parser arm AND the runtime seam AND a runtime test in one PR. CI green. Enqueuing as the cluster winner.

Minor non-blocking notes for follow-up: the inner parse_explore_replacement re-checks "if a creature you control would explore" after the outer "would explore" scan (collapse to one); and the execute chain is re-fed through parse_effect_chain rather than composed as typed Effect::Scry.sub_ability(Effect::Explore). Neither blocks.