fix(engine): bind combat-damage sacrifice pools and effect-sacrificed referents (Descendants' Fury #4795)#5862
Merged
matthewevans merged 1 commit intoJul 16, 2026
Conversation
… referents (Descendants' Fury phase-rs#4795) "Whenever one or more creatures you control deal combat damage to a player, you may sacrifice one of them. If you do, reveal cards ... until you reveal a creature card that shares a creature type with the sacrificed creature. Put that card onto the battlefield ..." never put the revealed card onto the battlefield. Two class-level defects, both a bypassed single authority: 1. The sacrifice was a silent no-op: sacrifice::resolve built its eligible pool from the raw TrackedSet(0) sentinel via matches_target_filter, whose id-level ladder (resolve_tracked_set_id) has no combat-damage rung. "One of them" on a combat-damage trigger lives only in the CombatDamageDealtToPlayer event, reachable via resolve_tracked_set_sentinel's current_combat_damage_source_filter rung (CR 510.2) — which the sacrifice resolver, alone among tracked-set consumers (change_zone, shuffle, token_copy, cloak, ...), never called. Route the filter through resolve_tracked_set_sentinel before deriving the pool; non-sentinel filters pass through unchanged. 2. Even with the sacrifice landed, SharesQuality{CreatureType, reference: CostPaidObject} matched nothing: both runtime TargetFilter:: CostPaidObject arms (filter.rs eval + targeting.rs resolved_targets) read only ability.cost_paid_object. An EFFECT sacrifice (CR 608.2c later-instruction referent, captured by parent_referent_context_from_ events) lands in effect_context_object; the documented cost_paid_object -> effect_context_object ladder (ability.rs) was implemented for the ObjectScope::CostPaidObject quantity arms but missing from the filter layer. Implement the same fallback in both arms so every CostPaidObject reader binds the same referent (CR 608.2k). The triage's original hypothesis (fast path not stamping effect_context_object) was stale: the capture machinery works — the breaks were the pool binder and the reader side. Tests (integration, real combat -> trigger -> optional-accept -> reveal pipeline, verbatim Oracle text; each fails on revert of its seam): - single attacker: sacrifice happens, reveal digs past a non-match, the shared-type creature enters the battlefield, rest to the bottom; - multi-attacker: the interactive EffectZoneChoice pick's creature type (not a board rescan) drives the reveal — the unchosen type stays in the library; - negative: no shared-type creature -> whole library revealed, nothing enters, all to the bottom (fail-closed referent, with sacrifice reach-guard). Full lib: 16672 passed. Integration: 3130 passed. Clippy clean. Fixes phase-rs#4795. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request resolves runtime issues with Descendants' Fury (#4795) by ensuring that TrackedSetId(0) sentinels are resolved to concrete filters using resolve_tracked_set_sentinel (per CR 608.2c and CR 510.2) and implementing a fallback from cost_paid_object to effect_context_object for TargetFilter::CostPaidObject (per CR 608.2k). It also adds a comprehensive integration test suite to prevent regressions. I have no feedback to provide as the changes are well-implemented and adhere to the repository's architectural guidelines.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
approved these changes
Jul 16, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Approved after current-head implementation review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4795 (claimed there with the corrected root-cause). Verified the Oracle text on Scryfall and reproduced end-to-end on current main before building.
Reproduction (real
applypipeline)Combat → trigger → accept "you may sacrifice one of them" → two divergences:
EffectResolved{Sacrifice, subject: None}).The parse is correct; the triage's fast-path hypothesis was stale (the
effect_context_objectcapture machinery works). The real breaks are two bypassed single authorities:Defect 1 — sacrifice pool never bound the combat-damage set
sacrifice::resolvematched the rawTrackedSet(0)sentinel viamatches_target_filter, whose id-level ladder has no combat-damage rung. "One of them" on a combat-damage trigger lives only in theCombatDamageDealtToPlayerevent, reachable viaresolve_tracked_set_sentinel'scurrent_combat_damage_source_filterrung (CR 510.2) — which the sacrifice resolver, alone among tracked-set consumers (change_zone,shuffle,token_copy,cloak, …), never called. Fix: route the filter throughresolve_tracked_set_sentinelbefore deriving the pool (non-sentinel filters pass through unchanged). Same unification discipline as #5512.Defect 2 —
CostPaidObjectreaders ignored effect-sacrificed referentsBoth runtime
TargetFilter::CostPaidObjectarms (filter eval +resolved_targets) read onlyability.cost_paid_object. An effect sacrifice (CR 608.2c later-instruction referent) is captured intoeffect_context_object— and the documentedcost_paid_object → effect_context_objectladder was already implemented for theObjectScope::CostPaidObjectquantity arms (P/T, mana value) but missing from the filter layer. Fix: implement the same fallback in both arms so everyCostPaidObjectreader binds the same referent (CR 608.2k). Class-level: the whole "sacrifice one → later instruction reads the sacrificed object's properties" family.Tests (each fails on revert of its seam; revert-probed)
EffectZoneChoice— the chosen creature's type (not a board rescan) drives the reveal; the unchosen type's match stays in the library. This is the identity-binding hostile fixture.Verification
Full engine lib: 16672 passed, 0 failed. Integration: 3130 passed, 0 failed (incl.
issue_735_cost_paid_object_non_regression). Clippy-D warningsclean; fmt clean. CR annotations (510.2, 608.2c, 608.2k, 400.7) grep-verified againstdocs/MagicCompRules.txt.🤖 Generated with Claude Code