Skip to content

Fix #863: tag granted Suspend casts via off-zone-aware keyword query - #1277

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
mike-theDude:bugfix/863-suspend-cast-variant-off-zone-keyword
May 28, 2026
Merged

Fix #863: tag granted Suspend casts via off-zone-aware keyword query#1277
matthewevans merged 2 commits into
phase-rs:mainfrom
mike-theDude:bugfix/863-suspend-cast-variant-off-zone-keyword

Conversation

@mike-theDude

Copy link
Copy Markdown
Collaborator

Summary

Fixes #863. Jhoira of the Ghitu (and The Tenth Doctor) grant Suspend with time counters to an exiled card via a permanent-duration AddKeyword { Suspend } continuous effect. The off-zone synthesis path correctly produced the upkeep counter-ticking and the last-counter cast trigger, but the cast itself misrouted to CastingVariant::Normal because two predicates in crates/engine/src/game/casting.rs (casting_variant_candidates and is_suspend_cast inside prepare_spell_cast_with_variant_override_inner) read obj.keywords directly. evaluate_layers does not populate obj.keywords for exile-zone objects, so runtime-granted Suspend was invisible and CR 702.62a's "if you cast a creature spell this way, it gains haste" transient effect was never installed.

Both predicates now query super::keywords::object_has_effective_keyword_kind(state, object_id, KeywordKind::Suspend) — the same off-zone-aware helper Escape, Flashback, Retrace, and Aftermath already use in this file. This removes a printed-Suspend-only special case rather than adding a new one; Suspend now follows the canonical off-zone keyword pattern used by every other alternative-cost keyword in casting.rs.

Files changed

  • crates/engine/src/game/casting.rs
  • crates/engine/src/parser/oracle_effect/mod.rs

CR references

  • CR 702.62a — Suspend; "If you cast a creature spell this way, it gains haste."
  • CR 604.1 — Static abilities granting keywords (runtime keyword grants).

Track

Developer

LLM

Model: Claude Opus 4.7
Thinking: medium

Verification

  • cargo fmt --all — clean
  • ./scripts/check-parser-combinators.sh — clean
  • cargo clippy-strict — clean (qa-tester)
  • cargo test -p engine — 9332 pass / 0 fail (qa-tester)
  • New tests:
    • jhoira_granted_suspend_last_counter_cast_tags_suspend_variant — discriminator: asserts prepare_spell_cast_with_variant_override returns CastingVariant::Suspend for the granted-Suspend exile cast (fails as Normal without the fix).
    • jhoira_granted_suspend_creature_cast_gains_haste — behavioral: drives a real CastSpell + stack::resolve_top on a creature, asserts effective Haste keyword is installed (CR 702.62a final sentence).

Scope Expansion

None.

Validation Failures

None.

CI Failures

None.

Follow-up (not bundled here)

  • has_madness at casting.rs:1692-1698 reads obj.keywords the same way and may have the same off-zone-grant invisibility bug for Madness — separate issue recommended.
  • Parser advisory comment added at oracle_effect/mod.rs flags that a future CostPaidObjectLacksKeyword variant will be needed when a card uses "if it doesn't have X" against the cost-paid object (no current card requires it).

…d query

`casting_variant_candidates` and `is_suspend_cast` in `prepare_spell_cast_with_variant_override_inner` read `obj.keywords` directly, so runtime-granted Suspend (Jhoira of the Ghitu, The Tenth Doctor) was invisible — `evaluate_layers` doesn't populate `obj.keywords` for exile-zone objects. The cast misrouted to `CastingVariant::Normal` and CR 702.62a's "if you cast a creature spell this way, it gains haste" transient effect never installed.

Switch both predicates to `super::keywords::object_has_effective_keyword_kind(state, object_id, KeywordKind::Suspend)` — the same off-zone-aware helper Escape, Flashback, Retrace, and Aftermath already use in this file. Aligns Suspend with the canonical off-zone keyword pattern instead of carrying a printed-only special case.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@github-actions github-actions Bot added the ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow label May 28, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the Suspend keyword detection in crates/engine/src/game/casting.rs to use object_has_effective_keyword_kind instead of querying obj.keywords directly. This ensures that Suspend granted at runtime (e.g., by Jhoira of the Ghitu) is correctly recognized from off-zone sets, and adds two comprehensive unit tests to verify this behavior. The feedback identifies a violation of repository rule R6, where a rules-touching comment lacks the mandatory CR <number>: <description> annotation format.

Comment thread crates/engine/src/game/casting.rs Outdated
@matthewevans matthewevans added the status:ready-to-merge Maintainer-reviewed and ready to merge label May 28, 2026
@matthewevans
matthewevans added this pull request to the merge queue May 28, 2026
Merged via the queue into phase-rs:main with commit f2c9c3a May 28, 2026
9 checks passed
matthewevans added a commit to mike-theDude/phase that referenced this pull request May 28, 2026
…um + drop ParseContext flag

Addresses architecture review on phase-rs#1268: the original PR introduced two new
bool fields (`Effect::Bounce.non_targeting`, `ParseContext.saw_target_keyword`)
which violate the codebase's "no bool fields — parameterize with typed enums"
principle (CLAUDE.md, feedback_no_bool_flags.md).

This commit:

1. Adds `BounceSelection { Targeted, AtResolution }` in `types/ability.rs`.
   Replaces `non_targeting: bool` on `Effect::Bounce` with `selection`.
   Mirror change on the IR-layer `TargetedImperativeAst::Return` variant in
   `oracle_ir/ast.rs`. ~40 construction/destructure sites updated.

2. Drops `ParseContext::saw_target_keyword` and adds
   `parse_target_with_syntax` returning `TargetSyntax { TargetKeyword,
   Descriptor }` as a return-value discriminator. `parse_target_with_ctx` is
   kept as a 2-line wrapper so the ~95 unchanged call sites are unaffected.
   The two consumers (`oracle_effect/imperative.rs`, `oracle_effect/mod.rs`)
   compute `BounceSelection` directly from the returned syntax.

3. Adds `crates/phase-ai/tests/whitemane_lion_bounded.rs` — discriminating
   end-to-end integration test that loads a Whitemane-Lion-heavy deck mirror
   into the AI search engine and asserts the game terminates within 4000
   actions (pre-fix would hit 10,000-action safety cap). `#[ignore]` per the
   existing `greasefang_bounded.rs` pattern (loads card-data.json).

4. Merges `origin/main` into the PR branch, picking up phase-rs#1261/phase-rs#1263/phase-rs#1266/phase-rs#1277
   without conflict. Reverts an unrelated `known-tokens.toml` regeneration
   artifact (author's PR description explicitly excluded this file).

Verification:
- `cargo fmt --all` clean
- `./scripts/check-parser-combinators.sh` clean
- `cargo clippy --all-targets -- -D warnings` clean
- `cargo test -p engine`: 9346 passed, 8 ignored
- `./scripts/gen-card-data.sh` clean; Whitemane Lion exports
  `"selection": "at_resolution"` correctly
- `cargo test -p phase-ai --test whitemane_lion_bounded -- --ignored` passes
  in ~61s (game completes naturally, well under 4000-action bound)
github-merge-queue Bot pushed a commit that referenced this pull request May 28, 2026
…e ETB) (#1268)

* Fix #563: AI infinite loop casting Whitemane Lion (non-targeted bounce ETB)

The Whitemane Lion ETB ("return a creature you control to its owner's
hand") is non-targeted per CR 115.1 and the Whitemane Lion ruling, but
the parser was wiring it as a targeted bounce and surfacing a
TriggerTargetSelection slot. The AI filled that slot with the Lion
itself, returning it to hand on resolution and re-casting it forever.

Fix spans three layers:

- Parser (root cause): add `saw_target_keyword: bool` to ParseContext
  and `non_targeting: bool` to Effect::Bounce so "return a creature
  you control" produces non_targeting=true, distinguished from
  "return target creature".
- Resolver: extract_target_filter_from_effect carves out
  Bounce { non_targeting: true } so no target slot is opened.
  effects/bounce.rs adds a non-targeted branch that surfaces
  WaitingFor::EffectZoneChoice for the controller, mirroring the
  existing Sacrifice pattern (CR 608.2c/d).
- AI (defence-in-depth): cast_facts.rs no longer requires targets for
  non-targeting bounces; search.rs caps same-card casts per turn at
  MAX_CASTS_OF_SAME_CARD_PER_TURN = 3 to prevent any remaining
  loop-prone pathology.

Covers the class of non-targeted controller-scoped bounce ETBs:
Whitemane Lion, Stonecloaker, Aether Channeler, Dream Stalker,
Emancipation Angel, Esperzoa, Cache Raiders, Ambrosia Whiteheart, etc.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor(PR-1268): replace non_targeting bool with BounceSelection enum + drop ParseContext flag

Addresses architecture review on #1268: the original PR introduced two new
bool fields (`Effect::Bounce.non_targeting`, `ParseContext.saw_target_keyword`)
which violate the codebase's "no bool fields — parameterize with typed enums"
principle (CLAUDE.md, feedback_no_bool_flags.md).

This commit:

1. Adds `BounceSelection { Targeted, AtResolution }` in `types/ability.rs`.
   Replaces `non_targeting: bool` on `Effect::Bounce` with `selection`.
   Mirror change on the IR-layer `TargetedImperativeAst::Return` variant in
   `oracle_ir/ast.rs`. ~40 construction/destructure sites updated.

2. Drops `ParseContext::saw_target_keyword` and adds
   `parse_target_with_syntax` returning `TargetSyntax { TargetKeyword,
   Descriptor }` as a return-value discriminator. `parse_target_with_ctx` is
   kept as a 2-line wrapper so the ~95 unchanged call sites are unaffected.
   The two consumers (`oracle_effect/imperative.rs`, `oracle_effect/mod.rs`)
   compute `BounceSelection` directly from the returned syntax.

3. Adds `crates/phase-ai/tests/whitemane_lion_bounded.rs` — discriminating
   end-to-end integration test that loads a Whitemane-Lion-heavy deck mirror
   into the AI search engine and asserts the game terminates within 4000
   actions (pre-fix would hit 10,000-action safety cap). `#[ignore]` per the
   existing `greasefang_bounded.rs` pattern (loads card-data.json).

4. Merges `origin/main` into the PR branch, picking up #1261/#1263/#1266/#1277
   without conflict. Reverts an unrelated `known-tokens.toml` regeneration
   artifact (author's PR description explicitly excluded this file).

Verification:
- `cargo fmt --all` clean
- `./scripts/check-parser-combinators.sh` clean
- `cargo clippy --all-targets -- -D warnings` clean
- `cargo test -p engine`: 9346 passed, 8 ignored
- `./scripts/gen-card-data.sh` clean; Whitemane Lion exports
  `"selection": "at_resolution"` correctly
- `cargo test -p phase-ai --test whitemane_lion_bounded -- --ignored` passes
  in ~61s (game completes naturally, well under 4000-action bound)

---------

Co-authored-by: Michael Briningstool <mbriningstool@gmail.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow status:ready-to-merge Maintainer-reviewed and ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue with multiple suspended cards? — Using Jhoira of the Ghitu, I exile Omniscience with 4 time counters on it.

3 participants