Implement explore replacement with scry prelude (Twists and Turns)#3485
Implement explore replacement with scry prelude (Twists and Turns)#3485Kelvinchen03 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements support for explore replacement effects, specifically targeting the 'Twists and Turns' pattern where a scry prelude occurs before a creature explores. It introduces the ProposedEvent::Explore event, registers an explore replacement handler, and adds parser support and tests. The feedback focuses on two main architectural improvements: first, moving the execution of the scry prelude out of the explore resolver and into the replacement applier (explore_applier) to maintain proper encapsulation; and second, refactoring the parser to dynamically parse the scry count rather than hardcoding 'scry 1' to ensure support for arbitrary scry values.
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.
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review — blocking; needs an engine-implementer plan
Thanks for the ambitious scope, but this can't merge — the card it implements doesn't actually work on the live path, the test can't catch that, and several CR annotations are wrong. These are architectural, so I'm routing it to a reviewed implementation plan rather than inline fixes.
Card context (client/public/card-data.json): Twists and Turns is an Enchantment — "If a creature you control would explore, instead you scry 1, then that creature explores." It currently parses to Unimplemented.
BLOCK 1 — wrong seam: the scry prelude is dead code for the real path
The replacement interception was inserted at the top of resolve_single_explorer, but:
effects/mod.rs:2317routesEffect::Explore => explore::resolve— the real explore entry isresolve()(explore.rs:213-314), which has noreplace_event/ProposedEvent::Explorecall.resolve_single_exploreris reachable only from theEffect::ExploreAllmass-explore path.- Twists and Turns drives single-creature explores (
Effect::Explore→resolve()), so the new code never fires for the card this PR targets. The interception belongs inresolve()(before the reveal), not inresolve_single_explorer.
BLOCK 2 — non-discriminating test
test_explore_scry_prelude_replacement calls resolve(...) (the path that bypasses the new code) and asserts only that a +1/+1 counter was added — plain nonland-explore behavior. It passes with the entire engine change reverted and never asserts a scry happened. Zero protection, and it masks BLOCK 1.
BLOCK 3 — hallucinated CR numbers (verified against docs/MagicCompRules.txt)
Explore is CR 701.44 (line 3665). The PR cites CR 701.37a (= Monstrosity, 3604) and CR 701.40 (= Manifest, 3624) for explore in three places: engine_replacement.rs:189, replacement.rs:204, proposed_event.rs:339. The same files already correctly use CR 701.44a/d elsewhere — these three must be CR 701.44a. Per the repo's CR protocol, every number must be grep-verified before it's written.
Also (architecture, even after the seam is fixed)
- Card-not-class applier.
explore_applierreturnsModified(event)unchanged, then the caller reaches back intoreplacement_definitions[..].execute,matches!-sniffs forEffect::Scry, and manually runs a scry. That violates "callers never inspect effect structure — push it into the resolver." A second explore-replacement that isn't scry would silently do nothing. - Scry by the wrong player. The manual scry uses the replacement source's controller (
obj.controller); it must be the explorer's controller (they diverge for the no-controlTypedFilter::creature()variant the parser also emits). - Missing snapshots. Two new
insta::assert_json_snapshot!tests ship with no committed.snapfiles → CI will fail. - Collision with #3389 (same file, same card). One must be closed.
The ProposedEvent::Explore surface addition itself is a reasonable reusable replacement event — the problem is nothing on the live path proposes it. Please re-approach via an engine-implementer plan: move the interception into resolve(), have the applier own the prelude (or model scry as a genuine pre-event), fix all CRs to 701.44a, add a discriminating pipeline test (creature explores → scry occurs first → explore resolves, failing without the change), commit snapshots, and reconcile with #3389. Happy to review the plan.
|
Heads-up: the explore-replacement (scry prelude / double explore) mechanic has landed via #3499, which carries the full stack in one PR — the Your PR's runtime seam was the right instinct and informed the bar here. Since the mechanic is now covered end-to-end, this one is superseded — recommend closing to avoid a duplicate seam. Thank you for the work on it. |
Summary
Implement full engine and parser support for replacement effects that add a scry prelude before an explore event, enabling cards like Twists and Turns ("If a creature you control would explore, instead you scry 1, then that creature explores").
Closes #3388
Implementation method (required)
How was the engine/parser logic in this PR produced? Check exactly one:
/engine-implementerpipeline (plan → review-plan → implement → review-impl → commit)/engine-implementer— explain why belowIf you did not use
/engine-implementer, state why (e.g. frontend-onlychange, docs/CI/tooling change, release chore, or a fix too small to warrant
the pipeline):
Note
Any change to
crates/engine/game logic — parser, effects, resolver,targeting, rules behavior — is expected to go through
/engine-implementer.The "not used" box is for changes that genuinely fall outside that scope.
CR references
CR 614.1a (replacement effects), CR 701.37a (explore)
Verification
cargo test --package engine replacement_explore_scry_preludecargo test --package engine test_explore_scry_prelude_replacement