fix(engine): bind "cast the exiled cards" to the cards actually exiled - #7034
Conversation
A user reported that exiling more than one card at a time did not work. On
Sanar, Innovative First-Year, the trigger exiled all 76 revealed library cards
and granted every one a cast permission, instead of exiling only the two
per-colour picks and shuffling the rest back. Their game ended with 74 basic
lands in exile and an 11-card library.
Two independent defects compose into that symptom.
**Parser — a registry gap.** `publishes_tracked_set_from_resolution` gates the
`ParentTarget` -> tracked-set widening, and its helper `is_exile_effect` does
not know `Effect::ForEachCategory { action: ExileFromPool }` — even though
`complete_per_category_exile` publishes exactly that set at runtime with
`ThisWayCause::Exiled`. The sibling predicate `chain_clause_is_exile_producer`
already lists that producer and names Sanar. So "You may cast the exiled cards
this turn" stayed on the `ParentTarget` sentinel.
**Engine — an unconditional target read.** `cast_from_zone::resolve` collected
`ability.targets` regardless of what its own `target_filter` denoted, so
`ParentTarget` resolved to whatever the chain had propagated — here the whole
reveal window, stamped by `inject_last_revealed_targets`' untyped tail and
carried through `Shuffle` by chain inheritance. `grant_lingering_permissions`
then routes every target not already in exile through an exile-delivery batch,
which is what moved 74 lands out of the library.
Ablation confirms both are load-bearing: the parser fix alone corrects the
parsed shape and changes nothing at runtime; the engine fix alone cannot help
a card whose anaphor never became a tracked-set reference.
The binding is cause-filtered, not a bare `TrackedSet{0}`, and that is
load-bearing rather than defensive. `publish_tracked_set` EXTENDS the chain
set, so an intervening publisher merges its own objects in. A constructed
`ExileTop -> PutCounterAll -> CastFromZone` fixture shows the bare binding
grants a cast permission to a battlefield creature and moves it to exile;
`caused_by: Exiled` reads only members whose producer action was an exile.
Portent of Calamity already ships that filter shape for the same reason.
`cast_from_zone` now binds tracked-set filters through
`targeting::resolve_tracked_set_sentinel` — the same authority
`change_zone::resolve` already uses — deduplicated, since the extend semantics
let an id appear twice.
Also fixed, found while testing: Praetor's Grasp was silently broken. It exiled
the card but the "you may play that card" grant landed nowhere, because the
anaphor bound to an empty target list. It now works.
Scope: 50 existing cards carry a `CastFromZone` tracked-set target and change
binding source from inherited chain targets to the published set; Sanar makes
51. Every one of those 51 scopes was verified to contain an in-scope producer
that stamps `Exiled` at runtime, with zero exceptions, and a new invariant test
keeps that correspondence true. Coverage is unchanged: 31645/35657 supported
before and after, identical `Unimplemented` counts.
…tion Review follow-up to the ForEachCategory exile fix. `publishes_exiled_cause_at_resolution` delegated to `is_exile_effect`, which recurses into `CreateDelayedTrigger`. That made it answer yes to a clause that stamps nothing when it resolves, so a same-chain "the exiled cards" anaphor bound after one would match nothing at runtime. It cannot simply be excluded: the same predicate is the base term of `publishes_tracked_set_from_resolution`, where the delayed wrapper's yes is correct — `strip_temporal_suffix` folds a previous clause's real exile into the wrapper, so the chain did publish a set. Six scopes have a tracked-set consumer preceded only by a delayed wrapper (conqueror's galleon, end-blaze epiphany, fire giant's fury, priority boarding, storm herald, waltz of rage) and would lose their binding entirely. So the two questions get two predicates: the narrow one spells its shapes out and drops the recursion; the wide one lists `is_exile_effect` separately. Both directions are pinned. Revert probe run, not assumed: collapsing the split fails the six-card guard. Also: - Refresh the 11 integration_cards.json entries this change invalidates, field scoped (abilities/static_abilities/triggers only). The Urza tests load their AST from that fixture, so they were asserting the pre-fix shape and passing for the wrong reason. The 62 entries stale for unrelated reasons are left alone; --check only verifies presence and cannot see any of this. - CR 614.6 -> CR 607.2a on the anaphor-binding sites added here. 614.6 is "if an event is replaced, it never happens" and says nothing about which objects a later reference resolves to. 607.2a is the linked-ability rule for an activated or triggered ability that instructs a player to exile; 607.2b and 614.14 are the replacement-effect variants, which these cards are not. - Drop CR 105.1 from a test comment: it names the five colors and mandates no iteration order. - Correct the rung-2 doc to name the And shape all 51 cards produce.
📝 WalkthroughWalkthroughThe parser now distinguishes exile-caused tracked-set members and rewrites cast targets accordingly. Runtime casting resolves tracked-set candidates directly, filters them, and removes duplicates. Parser and Sanar Vivid tests cover cause stamping, filtering, optional selections, and multi-publisher cases. ChangesTracked-set casting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)crates/engine/src/parser/oracle_effect/mod.rsast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Generated for head Parse changes introduced by this PR · 51 card(s), 2 signature(s) (baseline: main
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_effect/mod.rs (1)
26411-26411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider carrying the cause as a typed value instead of a
bool.
cast_anaphor_is_exiled: boolencodes whichThisWayCausethe cast anaphor may narrow to. The value the rewrite actually needs isSome(ThisWayCause::Exiled)orNone. AnOption<ThisWayCause>parameter carries the same information with more meaning, removes the two-arm branch in theCastFromZonearm, and extends without a second boolean when a further cause (for example a sacrifice or reveal cause) becomes narrowable.CLAUDE.mdrequires typed carriers over booleans for distinguishable cases.♻️ Sketch of the typed signature
-fn rewrite_parent_targets_to_tracked_set(effect: &mut Effect, cast_anaphor_is_exiled: bool) { +fn rewrite_parent_targets_to_tracked_set(effect: &mut Effect, cast_cause: Option<ThisWayCause>) {Effect::CastFromZone { target, .. } => { - if cast_anaphor_is_exiled { - rewrite_filter_parent_to_exiled_tracked_set(target) - } else { - rewrite_filter_parent_to_tracked_set(target) - } + match cast_cause { + Some(cause) => rewrite_filter_parent_to_caused_tracked_set(target, cause), + None => rewrite_filter_parent_to_tracked_set(target), + } }Also applies to: 26455-26461
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/mod.rs` at line 26411, Update rewrite_parent_targets_to_tracked_set to carry the cast-anaphor narrowing cause as an Option<ThisWayCause> instead of cast_anaphor_is_exiled: bool, and pass through Some(ThisWayCause::Exiled) or None at the call sites. In the CastFromZone handling, remove the two-arm boolean branch and use the typed option directly when deciding the tracked-set cause. Preserve the existing behavior for the exiled and non-exiled cases, and keep the change scoped to the rewrite_parent_targets_to_tracked_set path and its immediate callers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/effects/cast_from_zone.rs`:
- Around line 96-103: Update the tracked-set filtering in cast_from_zone so the
FilterContext is built from an ability clone whose targets are replaced with the
tracked-set members, matching the existing sibling patterns in this resolver.
Keep the deduped members list as the candidate set, but do not pass the original
ability directly into FilterContext::from_ability; instead, bind the context to
the same exact ids being filtered so ParentTarget-relative and name/type
comparisons evaluate against the published set.
In `@crates/engine/src/parser/oracle_effect/assembly.rs`:
- Around line 2434-2446: Update the comment in the exiled-cause narrowing block
that sets cast_anaphor_is_exiled and calls rewrite_parent_targets_to_tracked_set
so it cites CR 607.2a instead of CR 614.6. Keep the existing behavior unchanged,
and align the wording with publishes_exiled_cause_at_resolution, which already
uses CR 607.2a and excludes the replacement-effect case.
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 22560-22588: Update the `uncaused_exilers` test fixture in
`oracle_effect/tests.rs` so it includes the three missing uncaused exile shapes
alongside `Effect::HeistExile` and `Effect::RevealUntil`: add `Effect::Dig {
destination: Some(Zone::Exile), .. }`, `Effect::ExileHaunting { .. }`, and
`Effect::ExileResolvingSpellInsteadOfGraveyard { .. }`. Keep the existing loop
and assertions in place so these shapes are covered by the same positive
`chain_clause_is_exile_producer` check and the negative
`publishes_exiled_cause_at_resolution` / `this_way_cause_for_effect` checks.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Line 26411: Update rewrite_parent_targets_to_tracked_set to carry the
cast-anaphor narrowing cause as an Option<ThisWayCause> instead of
cast_anaphor_is_exiled: bool, and pass through Some(ThisWayCause::Exiled) or
None at the call sites. In the CastFromZone handling, remove the two-arm boolean
branch and use the typed option directly when deciding the tracked-set cause.
Preserve the existing behavior for the exiled and non-exiled cases, and keep the
change scoped to the rewrite_parent_targets_to_tracked_set path and its
immediate callers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 38035f1d-8f33-42f6-a48a-0c3b302f843a
⛔ Files ignored due to path filters (1)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__nashi_moon_sages_scion_lowered.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (7)
crates/engine/src/game/effects/cast_from_zone.rscrates/engine/src/game/effects/mod.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/issue_4253_sanar_vivid.rs
| let ctx = crate::game::filter::FilterContext::from_ability(ability); | ||
| let mut seen = HashSet::new(); | ||
| members | ||
| .iter() | ||
| .copied() | ||
| .filter(|obj_id| seen.insert(*obj_id)) | ||
| .filter(|obj_id| crate::game::filter::matches_target_filter(state, *obj_id, &bound, &ctx)) | ||
| .collect() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bind the filter context to the tracked-set members, not to ability.targets.
The helper stops reading ability.targets for candidate ids, then builds the FilterContext from that same polluted ability. FilterContext::from_ability(ability) carries ability.targets, which for Sanar is the whole reveal window the chain seam injected. A residual leg in the tracked-set filter that performs an object-scope read (a ParentTarget-relative comparison, a same-name or shares-a-type leg) therefore evaluates against the injected window rather than against the published set. The two sibling sites in this same resolver already avoid that: lines 429-435 and lines 502-504 both clone the ability and replace targets with the exact candidate set before constructing the context.
Mirror that pattern so the context and the candidates describe the same set.
🛡️ Proposed fix
- let ctx = crate::game::filter::FilterContext::from_ability(ability);
let mut seen = HashSet::new();
- members
+ let deduped: Vec<ObjectId> = members
.iter()
.copied()
.filter(|obj_id| seen.insert(*obj_id))
+ .collect();
+ // Bind the filter's object-scope reads to exactly the published set,
+ // mirroring the scoped contexts used by the `ExiledBySource` paths below.
+ let mut scoped_ability = ability.clone();
+ scoped_ability.targets = deduped.iter().copied().map(TargetRef::Object).collect();
+ let ctx = crate::game::filter::FilterContext::from_ability(&scoped_ability);
+ deduped
+ .into_iter()
.filter(|obj_id| crate::game::filter::matches_target_filter(state, *obj_id, &bound, &ctx))
.collect()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let ctx = crate::game::filter::FilterContext::from_ability(ability); | |
| let mut seen = HashSet::new(); | |
| members | |
| .iter() | |
| .copied() | |
| .filter(|obj_id| seen.insert(*obj_id)) | |
| .filter(|obj_id| crate::game::filter::matches_target_filter(state, *obj_id, &bound, &ctx)) | |
| .collect() | |
| let mut seen = HashSet::new(); | |
| let deduped: Vec<ObjectId> = members | |
| .iter() | |
| .copied() | |
| .filter(|obj_id| seen.insert(*obj_id)) | |
| .collect(); | |
| // Bind the filter's object-scope reads to exactly the published set, | |
| // mirroring the scoped contexts used by the `ExiledBySource` paths below. | |
| let mut scoped_ability = ability.clone(); | |
| scoped_ability.targets = deduped.iter().copied().map(TargetRef::Object).collect(); | |
| let ctx = crate::game::filter::FilterContext::from_ability(&scoped_ability); | |
| deduped | |
| .into_iter() | |
| .filter(|obj_id| crate::game::filter::matches_target_filter(state, *obj_id, &bound, &ctx)) | |
| .collect() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/engine/src/game/effects/cast_from_zone.rs` around lines 96 - 103,
Update the tracked-set filtering in cast_from_zone so the FilterContext is built
from an ability clone whose targets are replaced with the tracked-set members,
matching the existing sibling patterns in this resolver. Keep the deduped
members list as the candidate set, but do not pass the original ability directly
into FilterContext::from_ability; instead, bind the context to the same exact
ids being filtered so ParentTarget-relative and name/type comparisons evaluate
against the published set.
There was a problem hiding this comment.
Confirmed and fixed — but not on this PR. The merge queue landed #7034 before the fix could be pushed (queued branches reject updates, and by the time I dequeued it had already merged), so all three findings ship in follow-up #7035.
Verified rather than accepted: both sibling sites in cast_from_zone already clone-and-rescope the ability before building the context, one of them under the same CR 607.2a — so the third site was genuinely inconsistent. It is latent today (all 51 cards bind filter: Any, which reads no object scope), closed so it stays that way. The assembly.rs citation was a real miss on my side: my sweep used a hand-listed file set that omitted it. And the three added exile shapes each pass both the positive and negative assertions, which is what establishes they are genuinely uncaused.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="phase-rs/phase"
gh pr view 7035 --repo "$repo" --json number,state,mergedAt,url,title,files \
--jq '{number, state, mergedAt, url, title, files: [.files[].path]}'Length of output: 689
@matthewevans, I verified that #7035 is open and targets this exact correction in crates/engine/src/game/effects/cast_from_zone.rs.
The follow-up also includes crates/engine/src/parser/oracle_effect/assembly.rs and crates/engine/src/parser/oracle_effect/tests.rs. Since #7035 is not merged yet, I will leave this finding unresolved on #7034 until the fix lands.
✏️ Learnings added
Learnt from: matthewevans
Repo: phase-rs/phase PR: 7034
File: crates/engine/src/game/effects/cast_from_zone.rs:96-103
Timestamp: 2026-08-05T15:38:05.074Z
Learning: In `crates/engine/src/game/effects/cast_from_zone.rs`, tracked-set cast filters must build `FilterContext` from a `ResolvedAbility` whose `targets` are replaced with the deduplicated tracked-set members. This keeps object-scope filter evaluation bound to the published set rather than chain-injected targets.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
1 participant
Fixes a user-reported bug: "exiling more than one card at a time isn't working", reported with a turn-10 game state dump.
The bug
Sanar, the Vivid Blade exiles one card per color, then grants "you may cast the exiled cards this turn". In the reported state, all 75 revealed cards were exiled and granted, not the per-color picks.
Two defects compose:
publishes_tracked_set_from_resolution's helperis_exile_effectdoesn't recogniseForEachCategory { action: ExileFromPool }, so the cast anaphor never got rewritten offParentTarget.cast_from_zone::resolvereadability.targetsunconditionally, so thatParentTargetresolved to the whole reveal window stamped byinject_last_revealed_targets' untyped tail.Ablation over the real card: base exiles 5/5/8 → 3 cards; parser fix alone 5/5/8 → 3; both together 2/2/8 → 6. Only the pair fixes it.
The fix
cast_from_zoneroutes tracked-set casts throughtargeting::resolve_tracked_set_sentinel— the same binding authoritychange_zone::resolvealready uses — and the anaphor binds toTrackedSetFiltered { caused_by: Exiled }rather than a bare set.The cause filter is load-bearing, not decorative:
publish_tracked_setextends the chain set, so a laterPutCounterAllclause merges a battlefield creature into the same set the exile published. Without the filter,grant_lingering_permissionswould rip that creature off the battlefield into exile.non_exile_publisher_members_are_excluded_from_the_cast_anaphoris the fixture for exactly that.Second bug fixed on the way: Praetor's Grasp was silently broken at base — its grant landed nowhere, because the chain's inherited targets are
[Player(P1)]andfilter_map(Object)yielded nothing.Second commit — review follow-up
publishes_exiled_cause_at_resolutiondelegated tois_exile_effect, which recurses intoCreateDelayedTrigger. That makes it answer yes for a clause that stamps nothing when it resolves, so an anaphor bound after one would match nothing.It can't simply be excluded — the same predicate is the base term of
publishes_tracked_set_from_resolution, where the delayed wrapper's yes is correct (strip_temporal_suffixfolds a previous clause's real exile into it). Six scopes have a tracked-set consumer preceded only by a delayed wrapper —conqueror's galleon,end-blaze epiphany,fire giant's fury,priority boarding,storm herald,waltz of rage— and would lose their binding entirely.Two questions, two predicates. Both directions pinned in the invariant test. Revert probe run rather than assumed: collapsing the split fails the six-card guard.
Fixture refresh
The 11
integration_cards.jsonentries this change invalidates are refreshed, field-scoped toabilities/static_abilities/triggers.This matters because
add_real_cardloads the AST from that fixture instead of parsing live — sourza_lord_high_artificer_shuffle_exile_free_castwas asserting the pre-fix shape and passing for the wrong reason. It passes before and after; only now does it assert what the engine produces.62 entries stale for unrelated reasons are deliberately left alone — a separate refresh PR is owed. Note
gen-test-fixture.py --checkverifies key presence only and cannot detect any of this.Blast radius
51 cards bind a tracked-set cast; all 51 resolve to
TrackedSetFiltered { filter: Any, caused_by: Exiled }, and all 51 have a strict non-delayedExiled-stamping producer earlier in the chain.Honest residual: 30 of those 51 are named nowhere in
crates/engine— no parser shape test, no runtime test. They had no coverage before this change either. The whole-population invariant proves the filter matches something, not that each chain publishes the right members.CR annotations
CR 614.6→CR 607.2aon the anaphor-binding sites added here. 614.6 is "if an event is replaced, it never happens" and says nothing about which objects a later reference resolves to. 607.2a is the linked-ability rule for an activated or triggered ability that instructs a player to exile; 607.2b and 614.14 are the replacement-effect variants, which these cards are not.CR 105.1dropped from a test comment — it names the five colors and mandates no iteration order.The existing
608.2c + 614.6pairing elsewhere in the parser is pre-existing and left alone.Deferred, recorded
The Key to the Vault,Black Cat, Cunning Thief,Djeru and Hazoret,Keldon Flamesageexile their entire looked-at pile. Verified byte-identical before and after this change; different seam. Its pinned test is green on stale oracle text and never resolves the trigger.GrantCastingPermission { PlayFromExile }anaphor keeps the bare binding. Inert today (no exile delivery).Verification
--lib18523 passed / 0 failed;--test integration4562 passed / 0 failedSummary by CodeRabbit
Bug Fixes
Tests