Skip to content

Add Winds of Abandon - #124

Merged
matthewevans merged 11 commits into
phase-rs:mainfrom
AlexD-Richardson:card/winds-of-abandon
Apr 27, 2026
Merged

Add Winds of Abandon#124
matthewevans merged 11 commits into
phase-rs:mainfrom
AlexD-Richardson:card/winds-of-abandon

Conversation

@AlexD-Richardson

Copy link
Copy Markdown
Contributor

Summary

Adds engine support for Winds of Abandon.

Files changed

  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/effects/overload.rs
  • crates/engine/src/parser/oracle_effect/mod.rs
  • crates/engine/src/parser/oracle_effect/sequence.rs
  • crates/engine/src/types/game_state.rs
  • Cargo.lock

CR references

  • CR 109.5 (you/your → controller of an object) — replaces several misuses of CR 117.3a from prior code
  • CR 603.7 / 603.7c (acting subject + tracked-set context) — per-iteration parent-target rebinding
  • CR 608.2c / 608.2k (selection restrictions, context-ref pronouns)
  • CR 609.3 (replacement / continuous interaction during resolution) — pending_repeat_iteration resumption
  • CR 701.13a (Exile keyword action) — corrected from a prior CR 701.10a (Double) misattribution in overload.rs
  • CR 701.21a (subject) — actor identification in optional/searcher resolution
  • CR 701.23 / 701.23a (Search) — library-owner / searcher disambiguation
  • CR 702.96a–c (Overload) — alternative cast cost + "target → each" rewrite

Track

Developer

LLM

Model: claude-opus-4-7
Thinking: medium

Verification

  • cargo fmt --all — clean
  • cargo clippy-strict (CI-parity: cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings) — clean
  • ./scripts/check-parser-combinators.sh — clean
  • cargo test -p engine — 5241 pass / 0 fail (workspace-wide: clean)
  • cargo check --package engine-wasm --target wasm32-unknown-unknown — clean
  • cargo run --bin card-data-validate -- client/public/card-data.json — OK (34645 cards parsed)
  • ./scripts/gen-card-data.shWinds of Abandon: 0 Unimplemented entries
  • cargo coverageWinds of Abandon: supported: true, gap_count: 0
  • cargo semantic-auditWinds of Abandon: 0 findings

Scope Expansion

Adds general repeat_for iteration-resumption infrastructure (PendingRepeatIteration in types/game_state.rs, drained alongside pending_continuation). Without this, any repeat_for: TrackedSetSize loop whose inner effect transitions to an interactive WaitingFor (e.g. SearchChoice) would silently drop iterations after the first — a pre-existing engine limitation surfaced by overloaded Winds of Abandon, where each exiled creature's controller must search their own library. The new state field carries the residual loop bound and the per-iteration tracked-member snapshot used by the parent-target rebind, so resumption is class-general and not Winds-specific. Also extends the parent-target rebind path (effect_refs_parent_target / rebind_first_object_target) to keep "its controller" (CR 109.5) bound to the i-th tracked-set member per iteration. Two new engine tests cover the rebind + resumption invariants end-to-end.

Validation Failures

None.

CI Failures

None.

@github-actions github-actions Bot added ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) labels Apr 27, 2026
matthewevans added a commit that referenced this pull request Apr 27, 2026
…edicate

The previous version of `drain_pending_repeat_iteration_restashes_on_synchronous_continuation`
constructed a Draw + sub-Draw setup that completed each iteration cleanly
without ever installing a `pending_continuation` synchronously — meaning
the `installed_continuation` predicate was never evaluated true. The
test passed regardless of whether the predicate existed.

Rewrite the test to use `ConditionInstead` with an `else_ability` and a
non-Priority pre-set `waiting_for`, which exercises the line-1486 path
that synchronously stashes the else branch into `pending_continuation`
without changing `waiting_for`. Verified by temporarily disabling the
`installed_continuation` predicate: the test now FAILS with the exact
"pending_continuation overwritten before consumption" debug_assert that
finding #2 describes.

Asserts after one resumed iteration:
- `pending_continuation` is Some (else_ability was stashed synchronously)
- `pending_repeat_iteration` is re-stashed with `next_iteration = 2`
- only iteration 1's parent Draw fired (1 card), proving the drain
  broke immediately on the synchronous-continuation transition

PR #124 review feedback (finding #2 — proper test coverage).
AlexD-Richardson and others added 5 commits April 27, 2026 08:18
…est)

The overload `Effect::ChangeZone → ChangeZoneAll` arm previously used
`..` to absorb every field other than `origin`/`destination`/`target`.
Today's dropped fields are semantically inert for hidden-zone exile,
but `..` would silently absorb any newly-added `ChangeZone` field too,
meaning a future field addition could go missing in the overloaded form
without compiler help.

Bind every field by name and annotate the rationale for each drop. Adding
a new `ChangeZone` field will now fail to compile here, forcing a
deliberate decision about overload semantics.

PR phase-rs#124 review feedback (finding phase-rs#3).
…handle synchronous-continuation drain

Two interlocking bugs in the `repeat_for` resume path surfaced during
PR phase-rs#124 review of overloaded Winds of Abandon:

1. Iteration-resume path dropped sub_ability for iterations 2+. The
   stash site cleared `resume_ability.sub_ability = None` and the drain
   called `resolve_effect` directly, bypassing the chain-level wiring at
   line 1461 that stashes the SearchChoice continuation. For iteration
   0, the put-onto-battlefield + shuffle continuation runs correctly;
   for iterations 1+, it never ran at all — opponents 2+ would be
   prompted to pick a card but their chosen card stayed in their library.

   Fix: keep `sub_ability` on the resumed copy and clear `repeat_for`
   instead (so the resume call doesn't re-enter the outer iteration
   loop). The drain now calls `resolve_ability_chain` (depth=1, to
   preserve chain-local tracked-set state) so each resumed iteration
   goes through the same line-1660 SearchChoice continuation wiring as
   iteration 0. Per-iteration parent-target rebinding still propagates
   correctly because `rebind_first_object_target` updates `iter_ability.targets`,
   which the line-1651 sub_ability propagation copies onto the
   continuation chain.

2. Synchronous-continuation case was unhandled. If an iteration set
   `pending_continuation` without changing `waiting_for`, the inner
   loop would increment and run the next iteration — clobbering the
   continuation — or, on the trailing iteration, fall through and break
   without re-stashing remaining iterations.

   Fix: detect a None→Some `pending_continuation` transition and
   re-stash with `next_iteration = iteration + 1` before breaking, so
   the outer drain runs the continuation and then re-enters this drain
   for the next iteration.

Tests:
- `repeat_for_resumed_iteration_runs_full_sub_ability_chain`:
  end-to-end across two distinct opponents, asserts both chosen lands
  land on the battlefield AND both Shuffle resolutions emit
  EffectResolved events. Would have caught finding phase-rs#1 directly.
- `drain_pending_repeat_iteration_restashes_on_synchronous_continuation`:
  exercises the synchronous-continuation case with a multi-iteration
  resume that completes without entering any choice state.

PR phase-rs#124 review feedback (findings phase-rs#1, phase-rs#2, phase-rs#5 first/second tests).
…e verbs

The previous orthographic rule stripped `-es` whenever the resulting
stem ended in `ch`/`sh`/`ss`/`x`/`z`. This was correct for
`searches → search` but over-applied to the `-eze`/`-eeze` family,
producing invented stems like `freezes → freez`, `breezes → breez`,
`sneezes → sneez` — none of which match any downstream lookup, but they
violate the project's "parser must not swallow" rule by silently
fabricating non-existent words for unknown inputs.

Narrow the rule to only strip `-es` when the resulting stem is a
registered `PREDICATE_VERBS` member. Unknown verbs now pass through
unchanged, and the only verbs that take the strip are ones the parser
explicitly knows about. Adds a regression test that asserts neither
`freezes` nor `breezes` nor `sneezes` produces an invented stem, while
the registered `searches → search` case still works.

PR phase-rs#124 review feedback (findings phase-rs#4, phase-rs#5 third test).
…edicate

The previous version of `drain_pending_repeat_iteration_restashes_on_synchronous_continuation`
constructed a Draw + sub-Draw setup that completed each iteration cleanly
without ever installing a `pending_continuation` synchronously — meaning
the `installed_continuation` predicate was never evaluated true. The
test passed regardless of whether the predicate existed.

Rewrite the test to use `ConditionInstead` with an `else_ability` and a
non-Priority pre-set `waiting_for`, which exercises the line-1486 path
that synchronously stashes the else branch into `pending_continuation`
without changing `waiting_for`. Verified by temporarily disabling the
`installed_continuation` predicate: the test now FAILS with the exact
"pending_continuation overwritten before consumption" debug_assert that
finding phase-rs#2 describes.

Asserts after one resumed iteration:
- `pending_continuation` is Some (else_ability was stashed synchronously)
- `pending_repeat_iteration` is re-stashed with `next_iteration = 2`
- only iteration 1's parent Draw fired (1 card), proving the drain
  broke immediately on the synchronous-continuation transition

PR phase-rs#124 review feedback (finding phase-rs#2 — proper test coverage).
@matthewevans

Copy link
Copy Markdown
Member

Thanks for the solid groundwork on this one — the repeat_for iteration-resumption infrastructure was a real engine-level extension and the CR annotations made it easy to reason about. Pushed four follow-up commits to take it the rest of the way:

a529602ed — overload ChangeZone arm: explicit field drops. Replaced the .. rest pattern with named binds (owner_library: _, enter_with_counters: _, …) plus a one-line rationale per dropped field. Functionally identical today, but the next person who adds a field to Effect::ChangeZone will get a hard compile error here instead of a silent behavior change.

64d2f7bf1 — preserve sub_ability on resumed iterations + synchronous-continuation drain. The biggest one. The original resume path stripped sub_ability from the stash and called resolve_effect directly, so iterations 2..N skipped the chain-level wiring at mod.rs:1461 — meaning subsequent opponents' chosen lands were searched but never placed onto the battlefield, and their libraries were never shuffled. The fix keeps sub_ability intact on the stash, clears repeat_for to bound the resumed call to a single iteration, and routes through resolve_ability_chain at depth=1 to preserve chain-local state (chain_tracked_set_id, last_revealed_ids, etc.) that depth=0's prelude would have reset. Same commit also fixes a synchronous-continuation edge case where an iteration that installed pending_continuation without changing waiting_for could silently drop the rest of the loop.

ee4d32617 — narrow normalize_verb_token's -es rule. The orthographic strip was producing invented stems like freezes → freez for any -eze/-eeze word. Gated the strip on PREDICATE_VERBS membership so unknown words fall through to the -s arm and yield real stems. No current behavior changed (the closed lookup never matched the invented stems anyway), but the parser now refuses to swallow letters from words it doesn't recognize.

5092f09cd — strengthened the synchronous-continuation regression. Verified each new test fails when its corresponding bug is reintroduced, including the end-to-end Winds of Abandon shape that asserts both opponents' chosen lands actually reach the battlefield.

Full gate clean: cargo fmt, clippy-strict, cargo test -p engine (5244 + 218), parser combinator gate, pnpm lint, pnpm type-check. Coverage + semantic-audit still show Winds of Abandon supported with 0 findings.

Nice work on the core mechanic — the resumption infrastructure is going to keep paying for itself on every future iterated-search card.

@matthewevans
matthewevans enabled auto-merge (squash) April 27, 2026 14:25
parse_mana_value_suffix only matched the trigger-anaphoric "that <type>"
form on the right-hand side of "less than [or equal to]" / "greater
than [or equal to]". Cards with non-anaphoric quantity-ref RHS phrases
(Beseech the Queen "the number of lands you control", Bring to Light
"the number of colors of mana spent to cast this spell", Vhal of
Creation, Dominating Vampire, Drown in the Loch, Dreadhorde Arcanist,
etc.) silently dropped the comparator — the search filter parser
warned but the AST silently fell back to no MV constraint.

Extended the combinator to delegate the RHS to the shared
parse_quantity_ref building block when "that " doesn't follow. The
phrase boundary is the next sentence terminator (',', '.'), preventing
over-consumption into trailing search-and-shuffle clauses.

Inverted cast_free_unqualified_rejects_complex_mv_filter to
cast_free_unqualified_accepts_dynamic_mv_filter — the test's premise
was specifically about the missing capability that this commit adds.

+6 cards gained vs baseline, 0 regressions.
@matthewevans
matthewevans merged commit 65db15b into phase-rs:main Apr 27, 2026
3 checks passed
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 needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants