Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 76 additions & 12 deletions crates/engine/src/game/effects/cast_from_zone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::types::identifiers::ObjectId;
use crate::types::mana::ManaCost;
use crate::types::statics::CastFrequency;
use crate::types::zones::{EtbTapState, Zone};
use std::collections::HashSet;

/// CR 400.1/400.2: Recursively extract a filter's own `controller` axis,
/// looking through the composed forms (`Not`/`And`/`Or`) a real card's target
Expand Down Expand Up @@ -48,6 +49,60 @@ pub(crate) fn looked_at_controller_library_cards(
.collect()
}

/// CR 608.2c + CR 115.1: Bind a tracked-set cast anaphor ("you may cast the
/// exiled cards this turn") from the published set ITSELF, not from
/// `ability.targets`.
///
/// A tracked-set filter is a LINKED reference, never a target (CR 115.1): its
/// members were established by an earlier instruction in the same resolution,
/// so nothing was declared on announcement. `ability.targets`, by contrast, can
/// carry whatever the chain seam injected upstream (for Sanar, the whole
/// reveal window), which is how "exile two of the revealed cards" turned into
/// "exile and grant a cast permission to all 76 revealed cards".
///
/// The authority for turning the parser's `TrackedSetId(0)` sentinel into a
/// concrete set is `targeting::resolve_tracked_set_sentinel` — the same call
/// `change_zone::resolve` makes for the identical filter shape. Its ladder has
/// four rungs, and all four are safe here:
/// 1. the active chain set (`chain_tracked_set_id`) — a tracked-set shape;
/// 2. the combat-damage source filter (CR 510.2) — yields `SpecificObject`
/// or `Or` for a bare `TrackedSet`, and `And { [source_filter, filter] }`
/// for the `TrackedSetFiltered` shape all 51 of these cards actually use.
/// The `let … else` below rejects every one of those, so a combat-damage
/// anaphor casts nothing rather than something arbitrary;
/// 3. the latest non-empty published set — a tracked-set shape;
/// 4. no set at all: the sentinel `TrackedSetId(0)` is returned unchanged. It
/// passes the shape check but indexes a key that can never exist, because
/// `GameState::next_tracked_set_id` initialises to `1`. Fail-closed.
///
/// Deduplication is required, not cosmetic: `publish_tracked_set` EXTENDS the
/// set, so a chain that publishes the same object twice stores it twice (12
/// entries observed for 6 objects). Granting the same card two permissions and
/// queueing two zone moves for it is a real defect, so members are deduplicated
/// on first appearance, preserving publication order.
fn tracked_set_cast_candidates(
state: &GameState,
ability: &ResolvedAbility,
target_filter: &TargetFilter,
) -> Vec<ObjectId> {
let bound = crate::game::targeting::resolve_tracked_set_sentinel(state, target_filter.clone());
let (TargetFilter::TrackedSet { id } | TargetFilter::TrackedSetFiltered { id, .. }) = bound
else {
return Vec::new();
};
let Some(members) = state.tracked_object_sets.get(&id) else {
return Vec::new();
};
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()
Comment on lines +96 to +103

@coderabbitai coderabbitai Bot Aug 5, 2026

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.

🎯 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.

Suggested change
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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

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.

🧩 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.

}

/// CR 400.1/400.2 + CR 109.4: Eligible hand-pick pool for a private-zone
/// `CastFromZone` — the cards in `source_zone` belonging to the filter-scoped
/// player (Buster-Sword-class "your hand" filters keep the caster; Silent-Blade
Expand Down Expand Up @@ -324,18 +379,27 @@ pub fn resolve(
_ => return Err(EffectError::MissingParam("CastFromZone".to_string())),
};

// Collect target object IDs from the resolved ability's targets.
let mut target_ids: Vec<_> = ability
.targets
.iter()
.filter_map(|t| {
if let TargetRef::Object(id) = t {
Some(*id)
} else {
None
}
})
.collect();
// Collect target object IDs. CR 115.1: a tracked-set filter is a linked
// reference whose members the chain published, so it binds INTRINSICALLY
// (`tracked_set_cast_candidates`) and must not read whatever the chain seam
// injected into `ability.targets`. Every other filter shape is a genuine
// target list and keeps the announcement-time targets.
let mut target_ids: Vec<_> = match target_filter {
TargetFilter::TrackedSet { .. } | TargetFilter::TrackedSetFiltered { .. } => {
tracked_set_cast_candidates(state, ability, target_filter)
}
_ => ability
.targets
.iter()
.filter_map(|t| {
if let TargetRef::Object(id) = t {
Some(*id)
} else {
None
}
})
.collect(),
};

// CR 701.20e + CR 608.2c: Look-then-cast chains (Kiora) inject the legal
// looked-at library cards as targets at the chain seam
Expand Down
2 changes: 1 addition & 1 deletion crates/engine/src/game/effects/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4992,7 +4992,7 @@ fn affected_objects_with_causes(
/// stamped onto the tracked-set members it publishes. Derived purely from the
/// effect kind (and its declared destination), so it is independent of any
/// replacement that later redirects the members' landing zone.
fn this_way_cause_for_effect(effect: &Effect) -> Option<ThisWayCause> {
pub(crate) fn this_way_cause_for_effect(effect: &Effect) -> Option<ThisWayCause> {
use crate::types::zones::Zone;
// CR 400.7: a generic zone change names a "this way" verb only for the
// destinations a consumer references — Exile (exiled), Battlefield
Expand Down
22 changes: 16 additions & 6 deletions crates/engine/src/parser/oracle_effect/assembly.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,11 @@ use super::{
has_explicit_player_target, inject_chosen_color_choice_grant, mark_uses_tracked_set,
parse_spell_graveyard_replacement_rider,
parse_spells_cast_this_way_graveyard_replacement_rider,
publishes_aggregate_set_from_resolution, publishes_tracked_set_from_resolution,
rebind_tracked_aggregate_to_chain_set, retarget_counter_additional_cost_to_target,
rewrite_grant_parent_to_filter, rewrite_parent_targets_to_tracked_set, rewrite_rounding_mode,
rewrite_that_type_mana_instead, stamp_delayed_returns, try_fold_token_repeat_into_count,
wire_optional_cast_decline_fallback,
publishes_aggregate_set_from_resolution, publishes_exiled_cause_at_resolution,
publishes_tracked_set_from_resolution, rebind_tracked_aggregate_to_chain_set,
retarget_counter_additional_cost_to_target, rewrite_grant_parent_to_filter,
rewrite_parent_targets_to_tracked_set, rewrite_rounding_mode, rewrite_that_type_mana_instead,
stamp_delayed_returns, try_fold_token_repeat_into_count, wire_optional_cast_decline_fallback,
};

/// CR 601.2c: True when the assembled head chose one or more players at
Expand Down Expand Up @@ -2431,9 +2431,19 @@ pub(crate) fn assemble_effect_chain(ir: &EffectChainIr) -> AbilityDefinition {
let has_tracked_ref = contains_explicit_tracked_set_pronoun(&source_text_lower)
|| contains_implicit_tracked_set_pronoun(&source_text_lower);
if has_tracked_ref {
// CR 608.2c + CR 614.6: same walk, narrower predicate —
// does any prior clause publish members stamped `Exiled`?
// Only then may a cast anaphor narrow to
// `caused_by: Exiled`.
let cast_anaphor_is_exiled = defs
.iter()
.any(|d| publishes_exiled_cause_at_resolution(&d.effect));
for current in &mut current_defs {
mark_uses_tracked_set(current);
rewrite_parent_targets_to_tracked_set(&mut current.effect);
rewrite_parent_targets_to_tracked_set(
&mut current.effect,
cast_anaphor_is_exiled,
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}
} else if contains_explicit_tracked_set_pronoun(&source_text_lower) {
Expand Down
146 changes: 142 additions & 4 deletions crates/engine/src/parser/oracle_effect/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25765,8 +25765,81 @@ fn hand_reveal_target_to_controller_ref(target: &TargetFilter) -> Option<Control
}
}

/// CR 608.2c + CR 607.2a: Does this clause, at resolution, publish tracked-set
/// members stamped with [`ThisWayCause::Exiled`]?
///
/// CR 607.2a is the linked-ability rule this implements: an activated or
/// triggered ability that *instructs a player to exile* is linked to an ability
/// referring to "the exiled cards", and the second refers ONLY to cards put in
/// exile as a result of that instruction. Cause-filtering by
/// [`ThisWayCause::Exiled`] is how that restriction is enforced. (CR 607.2b is
/// the replacement-effect variant and does not apply — these cards exile on
/// resolution, not by replacing an event.)
///
/// This is the predicate a CAST anaphor ("you may cast the exiled cards this
/// turn") must be bound against, because the binding it produces is
/// `TrackedSetFiltered { caused_by: Some(Exiled) }` — a cause-filtered read
/// (`game/filter.rs`, `TrackedSetFiltered` arm) that matches only members whose
/// recorded producer ACTION was an exile.
///
/// DELIBERATELY NARROWER than the sibling [`chain_clause_is_exile_producer`],
/// which additionally counts `Dig { destination: Some(Exile) }`, `HeistExile`,
/// `ExileHaunting`, `ExileResolvingSpellInsteadOfGraveyard` and
/// `RevealUntil { kept_destination: Exile }`. Those genuinely exile, but
/// `game/effects/mod.rs::this_way_cause_for_effect` resolves each of them to
/// `None` — they publish members with NO cause stamp — so an anaphor bound to
/// `caused_by: Exiled` after one of them would match nothing. Two predicates
/// because there are two questions: "did an exile happen in this chain?"
/// (`chain_clause_is_exile_producer`, which gates same-chain vs. durable
/// `ExiledBySource` binding) versus "will a cause-filtered `Exiled` read find
/// anything?" (this one).
///
/// `ForEachCategoryAction::ExileFromPool` is added HERE rather than to
/// `is_exile_effect`: its resolver
/// (`game/effects/choose_from_zone.rs::complete_per_category_exile`) publishes
/// its picks through `publish_tracked_set_with_causes(.., ThisWayCause::Exiled)`
/// explicitly, and `is_exile_effect`'s other caller
/// (`chain_clause_is_exile_producer`) already enumerates this producer on its
/// own arm.
///
/// CR 603.7a: this spells the shapes out rather than delegating to
/// [`is_exile_effect`], which recurses into `Effect::CreateDelayedTrigger`.
/// That recursion is right for the WIDE question below — `strip_temporal_suffix`
/// wraps a previous clause's real exile into the delayed node, so the chain did
/// publish a set — but wrong here: a delayed trigger's exile happens in a LATER
/// resolution, so nothing is stamped when THIS clause resolves and a
/// `caused_by: Exiled` anaphor bound after it would match nothing.
///
/// The `publishes_exiled_cause_at_resolution` ⟶ `this_way_cause_for_effect`
/// correspondence is pinned by
/// `exiled_cause_publishers_all_stamp_exiled_at_runtime` in this module's tests.
pub(super) fn publishes_exiled_cause_at_resolution(effect: &Effect) -> bool {
matches!(
effect,
Effect::ChangeZone {
destination: Zone::Exile,
..
} | Effect::ChangeZoneAll {
destination: Zone::Exile,
..
} | Effect::ExileTop { .. }
| Effect::ForEachCategory {
action: crate::types::ability::ForEachCategoryAction::ExileFromPool { .. },
..
}
)
}

/// `is_exile_effect` is listed separately from
/// [`publishes_exiled_cause_at_resolution`] rather than being subsumed by it:
/// only the former recurses into `CreateDelayedTrigger`, and dropping that
/// recursion here would take six scopes whose sole producer is a delayed
/// wrapper (`conqueror's galleon`, `end-blaze epiphany`, `fire giant's fury`,
/// `priority boarding`, `storm herald`, `waltz of rage`) back to the
/// unrewritten `ParentTarget` binding.
fn publishes_tracked_set_from_resolution(effect: &Effect) -> bool {
is_exile_effect(effect)
|| publishes_exiled_cause_at_resolution(effect)
|| is_battlefield_return_effect(effect)
|| is_token_creating_effect(effect)
|| is_mass_coerce_static(effect)
Expand Down Expand Up @@ -26291,7 +26364,51 @@ fn fold_cast_copy_of_card_defs(defs: &mut Vec<AbilityDefinition>) {
}
}

fn rewrite_parent_targets_to_tracked_set(effect: &mut Effect) {
/// CR 608.2c + CR 607.2a: the cause-filtered sibling of
/// [`rewrite_filter_parent_to_tracked_set`], used for the CAST anaphor only.
///
/// `publish_tracked_set` EXTENDS the chain set rather than replacing it
/// (`game/effects/mod.rs`), so any intervening publisher in the same chain
/// merges its own objects into the set the anaphor will read. A BARE
/// `TrackedSet{0}` binding hands all of them to
/// `cast_from_zone::grant_lingering_permissions`, whose exile-delivery batch
/// moves every non-exile-zone member into exile — i.e. an
/// "exile the top two cards; put a +1/+1 counter on each creature you control;
/// you may cast the exiled cards" chain would rip the countered battlefield
/// creature into exile and grant it a cast permission. Binding
/// `caused_by: Some(Exiled)` reads only the members whose producer action was
/// an exile (`game/filter.rs`, `TrackedSetFiltered` arm), which is exactly what
/// "the exiled cards" names. Portent of Calamity already ships this filter
/// shape from the "this way" anaphor path.
///
/// Recurses through `Not`/`Or`/`And` so a composed cast filter is rewritten at
/// every leaf, mirroring its bare sibling.
fn rewrite_filter_parent_to_exiled_tracked_set(filter: &mut TargetFilter) {
match filter {
TargetFilter::ParentTarget => {
*filter = TargetFilter::TrackedSetFiltered {
id: TrackedSetId(0),
filter: Box::new(TargetFilter::Any),
caused_by: Some(ThisWayCause::Exiled),
}
}
TargetFilter::Not { filter } => rewrite_filter_parent_to_exiled_tracked_set(filter),
TargetFilter::Or { filters } | TargetFilter::And { filters } => {
for filter in filters {
rewrite_filter_parent_to_exiled_tracked_set(filter);
}
}
_ => {}
}
}

/// `cast_anaphor_is_exiled` says whether the prior clauses of this chain
/// publish `ThisWayCause::Exiled` members (see
/// [`publishes_exiled_cause_at_resolution`]). It is consulted by exactly one
/// arm — `Effect::CastFromZone` — because that is the only rewritten effect
/// whose resolver has an exile-DELIVERY side effect on the objects it binds;
/// every other arm keeps the bare [`tracked_set_filter`].
fn rewrite_parent_targets_to_tracked_set(effect: &mut Effect, cast_anaphor_is_exiled: bool) {
match effect {
// CR 701.26a/b: only single-target tap/untap carries a rewritable target.
Effect::SetTapState {
Expand Down Expand Up @@ -26321,7 +26438,7 @@ fn rewrite_parent_targets_to_tracked_set(effect: &mut Effect) {
| Effect::PutCounter { target, .. }
| Effect::RemoveCounter { target, .. }
| Effect::ChangeZone { target, .. }
| Effect::ChangeZoneAll { target, .. }
| Effect::ChangeZoneAll { target, .. } => rewrite_filter_parent_to_tracked_set(target),
// CR 603.7 + CR 608.2c: A cross-clause "cast/play that card / those
// cards" anaphor following an exile resolves to the *tracked set*
// (the cards exiled by the prior clause), not the trigger source.
Expand All @@ -26330,7 +26447,18 @@ fn rewrite_parent_targets_to_tracked_set(effect: &mut Effect) {
// and similar cross-clause cast forms reach `try_parse_cast_effect`
// with `target: ParentTarget`; this rewrite binds them to the
// tracked exile set during chain stitching.
| Effect::CastFromZone { target, .. } => rewrite_filter_parent_to_tracked_set(target),
//
// CR 607.2a: when the chain's publishers stamp `Exiled`, narrow the
// binding to those members — see
// `rewrite_filter_parent_to_exiled_tracked_set` for why a bare binding
// is destructive here and nowhere else.
Effect::CastFromZone { target, .. } => {
if cast_anaphor_is_exiled {
rewrite_filter_parent_to_exiled_tracked_set(target)
} else {
rewrite_filter_parent_to_tracked_set(target)
}
}
Effect::Attach { target, .. } => rewrite_filter_parent_to_tracked_set(target),
Effect::UnattachAll { target, .. } => rewrite_filter_parent_to_tracked_set(target),
Effect::GenericEffect {
Expand Down Expand Up @@ -32457,6 +32585,13 @@ pub(crate) fn parse_effect_chain_ir(
let needs_tracked_set = any_prior_publishes
&& (contains_explicit_tracked_set_pronoun(&lower_check)
|| contains_implicit_tracked_set_pronoun(&lower_check));
// CR 608.2c + CR 607.2a: same walk, narrower predicate — does any prior
// clause publish members stamped `Exiled`? Only then may a cast anaphor
// narrow to `caused_by: Exiled`.
let cast_anaphor_is_exiled = builder.clauses().iter().any(|c| {
!matches!(c.disposition, ClauseDisposition::Continue { .. })
&& publishes_exiled_cause_at_resolution(&c.parsed.effect)
});

// Continuation recognition — store on ClauseIr, application moves to lowering.
//
Expand Down Expand Up @@ -32728,7 +32863,10 @@ pub(crate) fn parse_effect_chain_ir(
check_def.player_scope = lifted_player_scope;
}
if needs_tracked_set {
rewrite_parent_targets_to_tracked_set(&mut check_def.effect);
rewrite_parent_targets_to_tracked_set(
&mut check_def.effect,
cast_anaphor_is_exiled,
);
}
let mut check_defs = vec![check_def];
let is_target_only = matches!(clause.effect, Effect::TargetOnly { .. });
Expand Down
Loading
Loading