fix(parser): fold article-led "or a <type> card" disjunct into the return filter (#5331)

[philluiz2323]

Summary

Fixes #5331. Overlord of the Balemurk reads "… then you may return a non-Avatar creature card or a planeswalker card from your graveyard to your hand", but a planeswalker card in the graveyard was never offered. The named alternative was being dropped at parse time.

Implementation method (required)

How was the engine/parser logic in this PR produced? Check exactly one:

• Produced via the /engine-implementer pipeline (plan → review-plan → implement → review-impl → commit)
• Not /engine-implementer — explain why below

Single-combinator parser fix, root-caused from a parse dump and covered by three building-block tests plus a full --lib regression. The change adds one composable combinator and one call-site wiring; it does not touch effects, resolver, or targeting semantics.

Root cause

parse_type_phrase deliberately stops at an article-led or connector and returns the RHS as remainder — a bare "or <type>" union and an article-led "or a <type>" continuation are different productions. That contract is asserted by the existing parse_type_phrase_leaves_article_led_or_rhs_as_remainder (added with the Plan card type, #4182).

The return/ChangeZone parser then bound that remainder to _rem and discarded it. The parsed filter collapsed to:

Typed { type_filters: [Creature, Non(Subtype("Avatar"))] }

so the planeswalker alternative vanished. assert_no_compound_remainder — the debug guard that exists to catch precisely this — only rejects " and " remainders, so an " or a …" disjunct was lost silently.

Fix

fold_article_led_or_type_phrases (in oracle_target) folds any number of trailing article-led disjuncts into a TargetFilter::Or, one leg per named alternative, each leg parsed by the full parse_type_phrase grammar. Built with nom combinators throughout (space0 + tag("or ") + opt(parse_article)), so it covers the whole "return an X card or a Y card" class rather than this one card's shape.

It is a no-op when the remainder is not an article-led type phrase — so the call site invokes it unconditionally, and a trailing non-filter or clause (e.g. "or you lose 2 life") is still handed back to the caller rather than swallowed, and cannot loop.

CR references

• CR 608.2c — a spell's instructions are followed as written, so every alternative the Oracle text names must remain selectable.

Verification

• cargo test -p engine --lib → 15884 passed, 0 failed (full suite; a target-filter change is exactly the kind that silently breaks unrelated cards).
• Three new building-block tests in oracle_target:
  • fold_article_led_or_unions_planeswalker_disjunct — both alternatives survive as Or legs.
  • fold_article_led_or_is_noop_without_disjunct — filter and remainder returned untouched.
  • fold_article_led_or_leaves_non_type_phrase_rhs — a non-type-phrase or clause is neither swallowed nor looped on.
• cargo fmt --all clean.

Note on the issue title

The report says "does not return a creature". The parse dump shows the creature alternative was always intact; what was unreachable is the planeswalker alternative. This PR restores the dropped disjunct. If the reporter also saw a creature fail to return, that would be a separate defect and I could not reproduce it — happy to dig further if a repro surfaces.

[gemini-code-assist]

Code Review

This pull request adds support for parsing article-led disjunctions in target phrases (such as "a non-Avatar creature card or a planeswalker card") by introducing a helper function fold_article_led_or_type_phrases that folds these disjuncts into a TargetFilter::Or union. Feedback on the changes highlights a critical safety and performance issue in the new helper function: calling to_lowercase() inside a loop allocates on every iteration, and calculating slice offsets based on the lowercase string's length is unsafe because UTF-8 byte lengths can change during case conversion, potentially causing runtime panics. The reviewer suggests using tag_no_case to parse case-insensitively directly on the original slice.

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]

Calling to_lowercase() inside the loop allocates a new String on every iteration, which is inefficient. More importantly, using rest.len() - after_connector.len() to slice the original string rest is unsafe: because converting a string to lowercase can change its byte length in UTF-8 (e.g., with certain Unicode characters), this offset calculation can point to an invalid byte boundary and cause a panic at runtime.

Using tag_no_case allows parsing the connector case-insensitively directly on the original rest slice, completely avoiding the allocation and the dangerous slicing logic.

fn parse_or_connector_with_optional_article(input: &str) -> OracleResult<'_, ()> {
    let (input, _) = nom::character::complete::space0.parse(input)?;
    let (input, _) = nom::bytes::complete::tag_no_case("or ").parse(input)?;
    let (input, _) = opt(nom_primitives::parse_article).parse(input)?;
    Ok((input, ()))
}

/// CR 608.2c: A filter phrase may name its alternatives as an article-led
/// disjunction - "a non-Avatar creature card or a planeswalker card"
/// (Overlord of the Balemurk). parse_type_phrase stops at the connector and
/// hands the right-hand side back as remainder (see
/// parse_type_phrase_leaves_article_led_or_rhs_as_remainder), because a bare
/// "or <type>" union and an article-led "or a <type>" continuation are
/// different productions.
///
/// Callers that would otherwise discard that remainder must fold it in, or the
/// spell silently loses an alternative the Oracle text named - the planeswalker
/// half of Overlord's return was unreachable this way. Each disjunct is parsed
/// by the full parse_type_phrase grammar and unioned into TargetFilter::Or,
/// so this composes for any number of alternatives rather than one card's shape.
///
/// Returns head untouched (and the remainder unconsumed) when the remainder
/// does not open an article-led disjunct, so it is safe to call unconditionally.
pub(crate) fn fold_article_led_or_type_phrases<'a>(
    head: TargetFilter,
    remainder: &'a str,
) -> (TargetFilter, &'a str) {
    let mut filters = vec![head];
    let mut rest = remainder;

    loop {
        let Ok((next_rest, ())) = parse_or_connector_with_optional_article(rest) else {
            break;
        };
        let (filter, next) = parse_type_phrase(next_rest);
        // A leg that consumed nothing is not a type phrase (e.g. "or a card in
        // your hand"); leave it for the caller rather than looping forever.
        if next.len() >= next_rest.len() {
            break;
        }
        filters.push(filter);
        rest = next;
    }

    match filters.len() {
        1 => (
            filters.pop().expect("head filter is always present"),
            remainder,
        ),
        _ => (TargetFilter::Or { filters }, rest),
    }
}

References

1. 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]

[MED] fold_article_led_or_type_phrases maps a lowercased copy back to the original string by suffix byte length. Evidence: crates/engine/src/parser/oracle_target.rs:1901-1909 calls rest.to_lowercase(), parses the connector on that transformed string, then computes &rest[rest.len() - after_connector.len()..]; nearby parser code uses direct case-insensitive combinators such as tag_no_case instead of this transformed-offset pattern. Why it matters: Unicode case conversion is not byte-length preserving, so an uppercase non-ASCII character in the remaining Oracle text can put that slice on the wrong byte boundary or panic, and it leaves a brittle parser idiom for the next contributor to copy. Suggested fix: parse the connector on the original rest with space0 + tag_no_case("or ") + the existing article parser, or otherwise compute the consumed byte count from the original input before passing the original RHS to parse_type_phrase.

The parse-diff sticky is also absent for this parser-source head while CI is still pending, so please wait for the current-head parse-diff evidence before re-requesting merge.

[github-actions]

Parse changes introduced by this PR

Baseline pending for 79cf19fcebe3534b343b375d7de05460262890b6 — this populates once main publishes its coverage snapshot (a few minutes after that commit landed).