feat(parser): played-by-opponents enter tapped replacement

[kiannidev]

Summary

• Parse Uphill Battle / Contamination class text: [Type] played by your opponents enter [the battlefield] tapped
• Add FilterProp::WasPlayed for cast/played entry discrimination vs tokens

Fixes #3390

Test plan

• cargo test -p engine --lib uphill_battle
• cargo test -p engine --lib played_by_opponents

Made with Cursor

[gemini-code-assist]

Code Review

This pull request introduces a new FilterProp::WasPlayed property and parser support for replacement effects of the form '[Type] played by your opponents enter tapped' (e.g., Uphill Battle). Feedback on the changes highlights three key issues: the parser implementation relies on non-combinator string operations instead of nom combinators (violating rule R1), the WasPlayed logic incorrectly matches any zone change from a prior zone (violating CR 305.1 and CR 601.2a), and the new parser function is missing a mandatory CR annotation (violating rule R6).

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] Non-combinator string operations used for parsing dispatch. Evidence: crates/engine/src/parser/oracle_replacement.rs:3102.

Why it matters: Verbatim string equality for parsing Oracle phrases bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates to prevent combinatorial explosion and improve maintainability.

Suggested fix: Refactor the parser to decompose the phrase into modular parsers (e.g., for the subject and the action) and compose them using nom combinators.

    let (filter, rest) = parse_type_phrase(norm_lower.trim_end_matches('.'));
    let rest = rest.trim();
    let (rest, _) = nom::sequence::tuple::<_, _, (), _>((
        parse_played_by_opponents,
        nom::character::complete::space1,
        parse_enter_tapped,
    ))(rest).ok()?;

References

1. Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. ^(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.

[gemini-code-assist]

[HIGH] FilterProp::WasPlayed matches any zone change from a prior zone, violating CR 305.1 and CR 601.2a. Evidence: crates/engine/src/game/filter.rs:3638.

Why it matters: Implementing WasPlayed as record.from_zone.is_some() in zone_change_record_matches_property means any card put onto the battlefield from a graveyard (e.g., Reanimate) or library (e.g., Natural Order) will incorrectly be treated as "played", causing triggers or replacement effects to apply when they shouldn't.

Suggested fix: If ZoneChangeRecord does not track whether the event was a cast or play, consider adding was_cast and was_played fields to ZoneChangeRecord or return false here if it cannot be determined accurately.

[gemini-code-assist]

[MEDIUM] Missing mandatory CR annotation for new game-rule parser implementation. Evidence: crates/engine/src/parser/oracle_replacement.rs:3094.

Why it matters: Every rules-touching line of engine code must carry a verified CR annotation to ensure strict fidelity to the MTG Comprehensive Rules.

Suggested fix: Add a CR 614.1d annotation to the function documentation.

Suggested change

	/// Parse "[Type] played by your opponents enter [the battlefield] tapped."
	/// Uphill Battle class — distinct from "your opponents control enter tapped"
	/// (Authority of the Consuls): only applies to cast/played entries, not tokens
	/// put directly onto the battlefield.
	fn parse_played_by_opponents_entry(
	/// CR 614.1d: Parse "[Type] played by your opponents enter [the battlefield] tapped."
	/// Uphill Battle class — distinct from "your opponents control enter tapped"
	/// (Authority of the Consuls): only applies to cast/played entries, not tokens
	/// put directly onto the battlefield without a prior zone.
	fn parse_played_by_opponents_entry(

References

1. Every rules-touching line of engine code must carry a comment of the form CR : . ^(link)

[matthewevans]

Good direction — this supersedes #3391 (which used strip_suffix for its dispatch and was DIRTY). Your dispatch gate goes through nom_primitives::scan_contains (a real nom tag()-at-word-boundary building block), WasPlayed is wired into every runtime matcher path (matches_filter_prop, spell_record_matches_property, zone_change_record_matches_property, plus the two population/perturbation predicates and coverage), and controller = Opponent lands on the typed ControllerRef enum. Two things to address before this can merge:

1. fmt — blocking. The Rust lint CI job fails at the formatting step. Run cargo fmt --all.

2. DRY — extend the shared builder instead of duplicating it. parse_played_by_opponents_entry re-implements almost all of build_external_entry_replacement (oracle_replacement.rs:3005) inline — same parse_type_phrase + rest.is_empty() guard, same Effect::SetTapState{ Tap }, same ReplacementDefinition::new(ChangeZone).execute(...).valid_card(...).destination_zone(Battlefield).description(...) tail. The only deltas are controller = Opponent and pushing WasPlayed. The idiomatic shape here is to teach the existing shared suffix helper parse_external_entry_suffix (oracle_replacement.rs:2984) to recognize the " played by your opponents enter[ the battlefield] tapped" variant, and have build_external_entry_replacement (or a thin wrapper) apply the Opponent + WasPlayed modifiers — so the whole external-entry-tapped family stays in one place. As written, the next entry-tapped variant has to be added in two builders instead of one.

3. Runtime test gap. Both new tests assert only the parsed ReplacementDefinition AST shape. Add at least one test that drives apply(): an opponent casts a creature → it enters tapped; a token the opponent puts directly onto the battlefield (no prior zone) → enters untapped. That is the behavior WasPlayed exists to enforce (played_from_zone/cast_from_zone populated at cast/play time vs. token creation), and it's exactly what an AST-shape test cannot catch — particularly the timing of when those fields are set relative to the ChangeZone replacement check at replacement.rs:3552/matches_target_filter_on_battlefield_entry.

Note for the maintainer: this supersedes #3391 (same feature, weaker implementation) — #3391 should be closed once this lands.

[matthewevans]

Heads up — this PR and #3523 overlap: both add FilterProp::WasPlayed and parse_played_by_opponents_entry for the played-by-opponents enter-tapped mechanic, so as separate PRs they'll textually conflict on merge.

They're complementary, though: this PR has the parser + filter wiring but only AST-shape tests, while #3523 carries exactly the runtime apply() test this one is missing (opponent's cast creature enters tapped, directly-created token does not). Please consolidate them into a single PR — fold #3523's runtime test in here (or vice-versa), address the cargo fmt --all debt and the DRY note above (extend the shared parse_external_entry_suffix at oracle_replacement.rs:2984 rather than reimplementing build_external_entry_replacement), and close the other. Once unified with the runtime test + fmt/clippy/parser-gate green, this is ready to land — and it'll supersede the older #3391.

[kiannidev]

Addressed review feedback:

• DRY: played-by-opponents suffix now flows through parse_external_entry_suffix + build_external_entry_replacement (shared with Authority of the Consuls family)
• Runtime test: cast creature matches, token and put-without-cast do not (uphill_battle_was_played_filter_matches_cast_creature_not_token)
• WasPlayed zone-change records use cast_from_zone/played_from_zone instead of from_zone.is_some()
• Consolidated test(engine): Uphill Battle WasPlayed filter runtime discrimination #3523 (closed) into this PR
• Merged latest main, fmt clean

Ready for re-review.

[matthewevans]

Re-reviewed at eb122e0. The three blockers I raised are genuinely fixed — but a new struct-field migration gap breaks CI, so this can't merge yet.

What's resolved (all good):

• DRY — the duplication is gone. parse_external_entry_suffix now returns a typed ExternalEntryKind (Plain / PlayedByOpponents) instead of a bool, and build_external_entry_replacement branches on kind to apply ControllerRef::Opponent + FilterProp::WasPlayed. Everything flows through the one shared suffix helper + shared builder — no separate parse_played_by_opponents_entry reimplementing it. The played-by-opponents suffix is correctly checked before the plain enter-tapped suffix.
• Runtime test — uphill_battle_was_played_filter_matches_cast_creature_not_token drives find_applicable_replacements against the real registry and discriminates: a cast creature (cast_from_zone = Some) matches; a token put directly does not; a nontoken put onto the battlefield without being cast does not. It would fail if WasPlayed were reverted to a control-only filter.
• Typed signal — the WasPlayed matcher uses played_from_zone.is_some() || cast_from_zone.is_some() in all three paths (matches_filter_prop, zone_change_record_matches_property, spell_record_matches_property), not from_zone.is_some() — so reanimate/blink doesn't wrongly match. CR 305.1 / 601.2a / 614.1d verified.
• #3523's content (test + WasPlayed wiring) is fully folded in here.

Blocking — CI is red: this PR doesn't compile.

error[E0063]: missing fields `cast_from_zone` and `played_from_zone` in initializer of `ZoneChangeRecord`
  --> crates/engine/tests/integration/madame_null_integration.rs:86:26

Adding the two new fields to ZoneChangeRecord requires updating every full-literal initializer across the whole workspace, including the test tree. You updated the src/ sites but missed the one full literal in crates/engine/tests/integration/madame_null_integration.rs:86 (set_etb_event). This is the only remaining gap — the src tree compiles clean (other literals use ..ZoneChangeRecord::test_minimal(..)/builder paths). Tilt's lib-only test-engine won't catch it; it only surfaces in CI's full-workspace test build, which is why Rust tests / Rust lint / the combined gate are all FAILURE.

Fix: add cast_from_zone: None, played_from_zone: None to that literal, push, and this is ready — everything else is clean.

(Maintainer note: superseding #3391 and the redundant #3523 are being handled.)

[kiannidev]

@matthewevans - addressed feedback.
Please re-review

[matthewevans]

Approved — all three prior blockers are resolved and CI is green at a3139b38.

• DRY: the played-by-opponents suffix now flows through the typed ExternalEntryKind + shared parse_external_entry_suffix/build_external_entry_replacement (Authority of the Consuls family) — no duplicated builder.
• Discriminating runtime test: uphill_battle_was_played_filter_matches_cast_creature_not_token drives the real registry and proves a cast creature enters tapped while a token / put-without-cast does not.
• Typed WasPlayed: matcher keys on played_from_zone/cast_from_zone (a real "was cast" signal), not from_zone.is_some().
• The E0063 was the workspace-literal migration miss at madame_null_integration.rs — fixed with the two None fields; Rust tests + lint + card-data all green.

Consolidating #3523 into this PR was the right call. Thanks for the thorough rework.