test(engine): regression-guard Sun Titan enters-or-attacks graveyard-return trigger (#5264)

[mike-theDude]

Summary

Regression coverage for Sun Titan (#5264), which was reported as "its triggers don't trigger — ETB or when it attacks."

Investigation found no reproducible engine defect. Sun Titan's EntersOrAttacks trigger fires on both halves, and its controller: You + InZone: Graveyard target filter correctly surfaces a your-graveyard permanent (MV ≤ 3) as a legal target (single legal target auto-selects straight onto the stack). The reported symptom is consistent with a graveyard that held no MV ≤ 3 permanent card — instants/sorceries and MV > 3 permanents don't qualify, so the optional targeted trigger correctly offers nothing.

This PR locks in the previously-uncovered your-graveyard target path for a marquee card. No engine logic is changed — tests only.

Tests added (crates/engine/src/game/triggers.rs, inline #[cfg(test)])

• sun_titan_etb_surfaces_graveyard_target — ETB half: firing a Hand→Battlefield event places the trigger with the your-graveyard MV-2 creature as a legal target.
• sun_titan_attack_surfaces_graveyard_target — attack half: declaring Sun Titan as an attacker fires the same trigger with the same legal target.
• sun_titan_no_legal_target_is_not_placed — negative control: with only an Instant, or an MV-4 permanent, in the graveyard, the optional targeted trigger is not placed (neither pending nor on the stack), on both halves. This is what makes the positive assertions discriminating: placement respects target legality.

Rules

CR 108.3 / CR 109.4 — cards in a graveyard have an owner, not a controller, so "from your graveyard" resolves against ownership. CR 601.2c / 603.3d — single-legal-target triggers auto-select and go straight to the stack. All citations verified against the Comprehensive Rules.

Closes the investigation for #5264 (recommend requesting the reporter's game state to confirm; no engine change was warranted).

🤖 Generated with Claude Code

[gemini-code-assist]

Note

Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported.

[github-actions]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Your investigation is right and useful — but all three tests restate coverage that already exists, and none of them drives the one path that genuinely isn't covered. Redirect them and this becomes worth merging.

✅ Your diagnosis is correct — I confirmed it independently

Sun Titan parses correctly on main. From the generated card data:

"triggers": [{"mode": "EntersOrAttacks", "execute": {"effect": {"type": "ChangeZone",
  "origin": "Graveyard", "destination": "Battlefield",
  "target": {"type": "Typed", "type_filters": ["Permanent"], "controller": "You",
    "properties": [{"type": "Cmc", "comparator": "LE", "value": {"type":"Fixed","value":3}},
                   {"type": "InZone", "zone": "Graveyard"}]}}}}]

EntersOrAttacks ✓, graveyard→battlefield ✓, Permanent + Cmc ≤ 3 + InZone Graveyard + controller: You ✓. There is no engine defect, and your read of the reported symptom (a graveyard holding no MV ≤ 3 permanent) is the right one. That conclusion belongs on #5264 as a comment — it is the valuable output here, and it stands on its own without this PR.

🔴 Blocker — the coverage bar (all three tests are already covered)

Per the repo's test-only bar, a standalone test PR must add coverage that does not already exist. Each of these does:

Your test	Already covered by
sun_titan_etb_surfaces_graveyard_target	tests/integration/issue_3309_rise_etb_returns.rs:237 — drives Sun Titan through the real parse pipeline, fires the ETB, hits WaitingFor::TriggerTargetSelection, chooses a graveyard permanent, and asserts it lands on the battlefield. That is the ControllerRef::You + InZone{Graveyard} path end-to-end, and it is stronger than yours — it asserts the return resolves, not merely that a slot surfaces.
sun_titan_attack_surfaces_graveyard_target	tests/integration/ureni_attack_trigger.rs — pins the attack branch of TriggerMode::EntersOrAttacks (incl. CR 506.4, second combat).
sun_titan_no_legal_target_is_not_placed	trigger_target_zero_targets_skips, in the very file you're adding to — "trigger with targeting + 0 legal targets → skipped entirely", pinning the same gate at triggers.rs:3179.

So the doc-comment claim — "the untested combination of ControllerRef::You with FilterProp::InZone { Graveyard }" — is not accurate; issue_3309 exercises exactly that at runtime.

This is also what CLAUDE.md means by "test the building block, not the special case." The trigger-mode dispatch and the target-slot builder are orthogonal — build_target_slots never learns which mode fired — so the Sun-Titan-specific cross-product of two independently-covered axes carries no risk the parts don't already carry. 219 lines into an already-enormous triggers.rs for no new signal is net-negative.

🔴 Blocker — the tests hand-build the trigger, so they cannot prove what they claim

sun_titan_scenario constructs TriggerDefinition::new(TriggerMode::EntersOrAttacks) by hand, and the doc comment asserts it "Builds Sun Titan's exact parsed trigger shape." That assertion is the thing a test should prove, not assume. Nothing here parses Sun Titan's Oracle text, so if the parser stopped producing that shape tomorrow, all three tests stay green while the card is broken — the definition of a non-discriminating guard.

The test that WOULD clear the bar

There is a real gap, and it is not the one you targeted: nothing drives Sun Titan's actual printed Oracle text end-to-end. Verified against Scryfall just now:

Vigilance
Whenever this creature enters or attacks, you may return target permanent card with mana value 3 or less from your graveyard to the battlefield.

Note what issue_3309 feeds the parser (issue_3309_rise_etb_returns.rs:20-21):

const SUN_TITAN_ORACLE: &str =
    "Vigilance\nWhen Sun Titan enters, you may return target permanent card with mana value 3 or less from your graveyard to the battlefield.";

That is not Sun Titan's text — it is an ETB-only fabrication with the "or attacks" half missing. So our existing "Sun Titan" test parses a card that does not exist, and the attack half of the real card is genuinely unpinned. (Our bug, not yours — I'm filing it.)

Rewrite around a single test that calls parse_oracle_text — the entry point database::synthesis actually uses to build card-data.json — on the verbatim Oracle text above, and asserts both halves fire with the graveyard target. That test would be unique, discriminating, and would have caught the fabricated const. It is the one worth having.

Recommendation

request-changes. Post the "no defect" finding on #5264 (it's the real deliverable). Replace all three tests with one that drives parse_oracle_text on Sun Titan's verbatim Oracle text across both halves. If you'd rather not, closing this and just commenting on #5264 is a perfectly good outcome — the investigation was the valuable part.

[mike-theDude]

Addressed in d6b7051 — rewrote per your review.

What changed

• Removed all three hand-built tests (the 219-line block in triggers.rs). You're right: each restated existing coverage (issue_3309 for the ETB→graveyard-return runtime path, ureni_attack_trigger for the attack branch, trigger_target_zero_targets_skips for the no-legal-target gate), and none drove the parser.
• Added one integration test, crates/engine/tests/integration/issue_5264_sun_titan_enters_or_attacks.rs, that runs Sun Titan's verbatim printed Oracle text through parse_oracle_text — the same entry point database::synthesis uses — and pins:
  • exactly one trigger, mode EntersOrAttacks (the single mode is both halves; a split/dropped "or attacks" clause changes the count or mode),
  • optional ChangeZone Graveyard→Battlefield,
  • target filter Permanent + controller: You + Cmc LE 3 + InZone Graveyard.

This is the discriminating guard the hand-built version couldn't be: if the parser stops producing that shape, it flips red.

Two notes on your suggested assertions:

• I dropped a Vigilance assertion I'd initially added — parse_oracle_text correctly does not surface evergreen keywords in extracted_keywords (Vigilance flows in via MTGJSON mtgjson_keyword_names and is merged in the loader; extracted_keywords is only for keywords MTGJSON doesn't supply). Asserting it through the parse output tested the wrong layer. The Vigilance\n line is still in the verbatim const so the test proves the leading keyword line doesn't break trigger parsing.
• I did not touch issue_3309's fabricated SUN_TITAN_ORACLE const, since you said you're filing that separately. This new file's doc comment references it as the gap it closes.

Verification (worktree, direct cargo — Tilt doesn't watch it):

• cargo fmt --all clean
• new test passes: sun_titan_verbatim_parses_enters_or_attacks_graveyard_return ... ok
• cargo test -p engine --lib --no-run compiles clean with zero warnings (confirms removing the block orphaned no imports)

The "no engine defect" finding for #5264 still stands as the real deliverable — will post it there.

[mike-theDude]

@matthewevans — rewrote per your review; final head is aea1e3671 (rebased onto current main, single commit).

Addressed your blockers

• Coverage / already-covered: removed all three tests and the 219-line hand-built block from triggers.rs. You're right that each restated existing coverage — issue_3309 for the ETB→graveyard-return runtime path, ureni_attack_trigger for the attack branch, and trigger_target_zero_targets_skips for the no-legal-target gate — and none drove the parser.

• Non-discriminating (hand-built trigger): replaced them with a single integration test, crates/engine/tests/integration/issue_5264_sun_titan_enters_or_attacks.rs, that runs Sun Titan's verbatim printed Oracle text through parse_oracle_text — the entry point database::synthesis uses to build card-data.json. It pins:

  • exactly one trigger, mode EntersOrAttacks (this single mode is both halves — a split or dropped "or attacks" clause changes the count or mode),
  • the optional ChangeZone Graveyard→Battlefield,
  • target filter Permanent + controller: You + Cmc LE 3 + InZone Graveyard.

  If the parser stops producing that shape, it flips red — the discriminating guard the hand-built version couldn't be. This would also have caught a fabricated const, since it parses the real text end-to-end.

Two notes

• I dropped a Vigilance assertion I'd briefly added: parse_oracle_text correctly does not surface evergreen keywords in extracted_keywords (Vigilance flows in via MTGJSON mtgjson_keyword_names, merged in the loader; extracted_keywords is only for keywords MTGJSON doesn't supply, e.g. "Protection from multicolored"). Asserting it through the parse output tested the wrong layer. The Vigilance\n line stays in the verbatim const so the test still proves the leading keyword line doesn't break trigger parsing.
• I did not touch issue_3309's fabricated SUN_TITAN_ORACLE const, since you're filing that separately. The new file's module doc references it as the gap it closes.

Verification (worktree, direct cargo — Tilt doesn't watch it): cargo fmt --all clean; the new test passes on the rebased tree (sun_titan_verbatim_parses_enters_or_attacks_graveyard_return ... ok, engine v0.25.0 against current main); cargo test -p engine --lib --no-run compiles clean with zero warnings (confirms removing the block orphaned no imports).

The "no defect" finding for #5264 is already documented there (the graveyard held no MV≤3 permanent card, so the optional targeted trigger correctly had no legal target).

[matthewevans]

Verdict: Blocked — this test now duplicates stronger coverage already on main.

🔴 Blocker

• crates/engine/tests/integration/issue_5264_sun_titan_enters_or_attacks.rs:10 and #L28 say the existing Sun Titan fixture uses an ETB-only fabrication. That is no longer true: issue_3309_rise_etb_returns.rs:21 contains the same verbatim Oracle text, and #L41 already parses it through parse_oracle_text, verifies EntersOrAttacks, its optional graveyard-to-battlefield effect, and that the trigger was not split. This PR's AST assertions at #L48-#L113 add no distinct failure mode.
• Main also covers the stronger runtime behavior: issue_3309_rise_etb_returns.rs:350 drives the same parsed Sun Titan through combat, target selection, the optional choice, and asserts that the graveyard permanent returns at #L414. An "or attacks" regression therefore already fails a production-pipeline test. A parser-only duplicate cannot clear the standalone-test value bar.

🟡 Non-blocking

• The previous review's suggested gap was real when it was written, but it was filled on main before this rewrite. The current head was rebased successfully; the stale premise is the only problem.

✅ Clean

• The test is correctly registered in tests/integration/main.rs and does not introduce production behavior or unrelated churn.

Recommendation: Request changes — remove the duplicate test (or close this PR) and retain the #5264 investigation as an issue comment rather than adding redundant suite cost.

[mike-theDude]

Closing per review: main already has stronger Sun Titan enters-or-attacks parser and runtime coverage in issue_3309_rise_etb_returns.rs, so this PR no longer adds distinct regression value.