test(engine): add coverage for Undying Malice granted trigger + edict sacrifice (#5942) - #6788
Merged
matthewevans merged 2 commits intoJul 30, 2026
Conversation
… sacrifice (phase-rs#5942) Adds runtime coverage for a GenericEffect/Continuous/GrantTrigger static ability (Undying Malice) whose recipient later leaves the battlefield via a resolving edict-style Sacrifice effect, across every shape that seam had zero prior coverage for: single-target player choice, single-target mandatory fast path, casting the grant in response to an edict already on the stack, and a multiplayer simultaneous player_scope::All edict. Investigation of phase-rs#5942 could not reproduce the reported "doesn't trigger with edict effects" defect on current main across any of these shapes — all four pass as written, exercising the CR 603.6d/603.10a leaves-the- battlefield LKI snapshot path. Added as durable coverage for a previously untested engine seam.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change registers the Undying Malice edict-sacrifice integration test module, updates related documentation and assertion messages, and uses the actual edict object ID when constructing an already-on-stack resolved ability. ChangesUndying Malice edict coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Maintainer hold — verification in progressTriaged current head |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/tests/integration/undying_malice_edict_sacrifice_5942.rs (1)
51-66: 📐 Maintainability & Code Quality | 🔵 Trivial
add_manaduplicates an existing helper instead of reusing it.The doc comment states this "mirrors
chord_of_calling.rs::add_mana" — meaning it's a copy-paste of logic that already exists elsewhere in the test suite rather than a shared import.Extract the shared helper into a common test-support module (e.g.
tests/integration/common.rs) and import it from both files, instead of maintaining two copies.As per path instructions: "Before adding logic, search for and reuse existing building blocks; do not duplicate helpers or inline extraction logic that an existing module already provides."
#!/bin/bash # Locate the mirrored helper and check for an existing shared test-utils module. rg -n "fn add_mana" crates/engine/tests -n -A 12 fd -a common.rs crates/engine/tests/integration🤖 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/tests/integration/undying_malice_edict_sacrifice_5942.rs` around lines 51 - 66, Extract the duplicated add_mana helper into the shared integration test-support module, preserving its existing signature and behavior, then import and reuse it from both undying_malice_edict_sacrifice_5942.rs and chord_of_calling.rs. Remove the local helper definitions and update imports as needed.Source: Path instructions
🤖 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/tests/integration/undying_malice_edict_sacrifice_5942.rs`:
- Around line 254-322: Replace the manually constructed Diabolic Edict stack
entry in undying_malice_cast_in_response_to_edict_on_stack with
runner.cast(edict).commit(), using the committed spell’s real source and stack
state. Remove the direct parse/create_object/StackEntry setup and preserve the
priority window so P0 can cast Undying Malice in response.
---
Nitpick comments:
In `@crates/engine/tests/integration/undying_malice_edict_sacrifice_5942.rs`:
- Around line 51-66: Extract the duplicated add_mana helper into the shared
integration test-support module, preserving its existing signature and behavior,
then import and reuse it from both undying_malice_edict_sacrifice_5942.rs and
chord_of_calling.rs. Remove the local helper definitions and update imports as
needed.
🪄 Autofix (Beta)
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: d86040a5-2551-4dd2-8e59-c91968a71272
📒 Files selected for processing (2)
crates/engine/tests/integration/main.rscrates/engine/tests/integration/undying_malice_edict_sacrifice_5942.rs
Comment on lines
+254
to
+322
| fn undying_malice_cast_in_response_to_edict_on_stack() { | ||
| use engine::game::zones::create_object; | ||
| use engine::parser::oracle::parse_oracle_text; | ||
| use engine::types::ability::{ResolvedAbility, TargetRef}; | ||
| use engine::types::card_type::CoreType; | ||
| use engine::types::game_state::{CastingVariant, StackEntry, StackEntryKind}; | ||
| use engine::types::identifiers::CardId; | ||
|
|
||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
|
|
||
| let victim = scenario.add_creature(P0, "Doomed Traveler", 1, 1).id(); | ||
|
|
||
| let malice = scenario | ||
| .add_spell_to_hand_from_oracle(P0, "Undying Malice", true, UNDYING_MALICE_ORACLE) | ||
| .with_mana_cost(ManaCost::Cost { | ||
| shards: vec![ManaCostShard::Black], | ||
| generic: 0, | ||
| }) | ||
| .id(); | ||
|
|
||
| let mut runner = scenario.build(); | ||
| add_mana(&mut runner, P0, ManaType::Black, 1); | ||
|
|
||
| // P1's Diabolic Edict already on the stack, targeting P0. | ||
| let edict_parsed = parse_oracle_text( | ||
| DIABOLIC_EDICT_ORACLE, | ||
| "Diabolic Edict", | ||
| &[], | ||
| &["Instant".to_string()], | ||
| &[], | ||
| ); | ||
| let edict_ability = ResolvedAbility::new( | ||
| edict_parsed.abilities[0].effect.as_ref().clone(), | ||
| vec![TargetRef::Player(P0)], | ||
| ObjectId(0), | ||
| P1, | ||
| ); | ||
| let edict_id = create_object( | ||
| runner.state_mut(), | ||
| CardId(9001), | ||
| P1, | ||
| "Diabolic Edict".to_string(), | ||
| Zone::Stack, | ||
| ); | ||
| { | ||
| let edict_obj = runner.state_mut().objects.get_mut(&edict_id).unwrap(); | ||
| edict_obj.card_types.core_types = vec![CoreType::Instant]; | ||
| } | ||
| runner.state_mut().stack.push_back(StackEntry { | ||
| id: edict_id, | ||
| source_id: edict_id, | ||
| controller: P1, | ||
| kind: StackEntryKind::Spell { | ||
| card_id: CardId(9001), | ||
| ability: Some(Box::new(edict_ability)), | ||
| casting_variant: CastingVariant::Normal, | ||
| actual_mana_spent: 0, | ||
| }, | ||
| }); | ||
|
|
||
| // Hand P0 the priority window to respond. | ||
| runner.state_mut().priority_player = P0; | ||
| runner.state_mut().waiting_for = WaitingFor::Priority { player: P0 }; | ||
|
|
||
| // P0 casts Undying Malice in response, targeting their only creature. | ||
| // `resolve()` drives the whole remaining stack: Malice resolves first | ||
| // (LIFO), then the Edict resolves and forces the mandatory sacrifice. | ||
| let outcome = runner.cast(malice).target_object(victim).resolve(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "fn cast\b" crates/engine/src -n -A 15
rg -n "impl.*CastBuilder|struct CastBuilder" crates/engine/src -n -A 10
rg -n "fn resolve\b" crates/engine/src -n -B3 -A 20
rg -n "struct ResolvedAbility" crates/engine/src -n -A 20Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- cast/builder APIs ---'
rg -n --max-count 40 "struct CastBuilder|impl .*CastBuilder|fn cast\b|leave.*stack|resolve\(\)" crates/engine/src | head -n 200
echo '--- ResolvedAbility definition ---'
rg -n --max-count 20 "struct ResolvedAbility|impl ResolvedAbility|pub fn new\(" crates/engine/src/types crates/engine/src | head -n 120
echo '--- ObjectId\(0\) and manual ability construction ---'
rg -n --max-count 40 "ObjectId\(0\)|ResolvedAbility::new\(" crates/engine/src crates/engine/tests | head -n 200Repository: phase-rs/phase
Length of output: 44341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact source around the relevant APIs and the reviewed test region.
files=(
"crates/engine/src/types/ability.rs"
"crates/engine/src/game/runner.rs"
"crates/engine/src/game/scenario.rs"
"crates/engine/src/game/actions/cast.rs"
"crates/engine/tests/integration/undying_malice_edict_sacrifice_5942.rs"
)
for f in "${files[@]}"; do
if [ -f "$f" ]; then
echo "===== $f ====="
wc -l "$f"
fi
done
echo '--- ability.rs slice ---'
sed -n '1,260p' crates/engine/src/types/ability.rs
echo '--- runner/scenario/cast references ---'
rg -n --max-count 80 "struct GameRunner|impl GameRunner|fn cast\b|struct CastBuilder|impl CastBuilder|leave.*stack|resolve\(" crates/engine/src/game crates/engine/src/types | head -n 200
echo '--- reviewed test slice ---'
sed -n '240,340p' crates/engine/tests/integration/undying_malice_edict_sacrifice_5942.rsRepository: phase-rs/phase
Length of output: 36677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ability.rs definition slice ---'
sed -n '1,260p' crates/engine/src/types/ability.rs
echo '--- cast/runner references ---'
rg -n --max-count 80 "struct GameRunner|impl GameRunner|fn cast\b|struct CastBuilder|impl CastBuilder|leave.*stack|push_back\(StackEntry|ResolvedAbility::new\(" crates/engine/src | head -n 200
echo '--- reviewed test slice ---'
sed -n '254,322p' crates/engine/tests/integration/undying_malice_edict_sacrifice_5942.rsRepository: phase-rs/phase
Length of output: 34045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- GameRunner and cast builder definitions ---'
rg -n --max-count 40 "struct GameRunner|impl GameRunner|struct CastBuilder|impl CastBuilder|pub fn cast\b|fn cast\b|resolve\(\) ->|fn resolve\b" crates/engine/src | head -n 200
echo '--- any API that leaves a spell on stack ---'
rg -n --max-count 80 "leave.*stack|keep.*stack|put.*stack|on the stack|cast.*resolve|resolve.*stack" crates/engine/src crates/engine/tests | head -n 240
echo '--- ResolvedAbility fields ---'
sed -n '1,220p' crates/engine/src/types/ability.rsRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
log=/tmp/coderabbit-shell-logs/shell-output-WwWCgW
echo '--- cast builder hits in previous large search log ---'
rg -n "CastBuilder|fn cast\b|resolve\(\)" "$log" | head -n 120
echo '--- ability/source-id hits in previous large search log ---'
rg -n "ResolvedAbility|ObjectId\(0\)|source_id" "$log" | head -n 120Repository: phase-rs/phase
Length of output: 11536
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ObjectId(0) in tests ---'
rg -n --max-count 80 "ObjectId\(0\)" crates/engine/tests crates/engine/src | head -n 200
echo '--- manual stack assembly in integration tests ---'
rg -n --max-count 80 "StackEntry \{|stack\.push_back\(|ResolvedAbility::new\(" crates/engine/tests/integration | head -n 240Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- scenario.rs around GameRunner::cast ---'
sed -n '1260,1325p' crates/engine/src/game/scenario.rs
echo '--- scenario.rs around SpellCast commit/resolve methods ---'
sed -n '2480,2635p' crates/engine/src/game/scenario.rs
echo '--- search for public commit/use of SpellCast ---'
rg -n --max-count 80 "pub fn commit\b|fn commit\b|try_commit\b|SpellCast<'_>|CastOutcome|try_resolve\b" crates/engine/src/game/scenario.rsRepository: phase-rs/phase
Length of output: 9635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the concrete reviewed pattern and neighboring tests that use the same style.
sed -n '260,320p' crates/engine/tests/integration/undying_malice_edict_sacrifice_5942.rs
echo '--- nearby integration tests with hand-built stack entries ---'
sed -n '20,120p' crates/engine/tests/integration/issue_3996_return_the_favor.rs
sed -n '20,130p' crates/engine/tests/integration/issue_3300_counter_spell.rs
sed -n '20,140p' crates/engine/tests/integration/issue_2925_mystic_confluence_repeated_counter.rsRepository: phase-rs/phase
Length of output: 13387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('crates/engine/src/game/scenario.rs')
text = p.read_text()
for needle in [
'pub fn cast(&mut self, spell: ObjectId) -> SpellCast',
'pub fn commit(self)',
'fn commit(self)',
'pub fn resolve(self) -> CastOutcome',
'pub fn try_resolve(self) -> Result<CastOutcome, EngineError>',
]:
idx = text.find(needle)
if idx != -1:
start = text.rfind('\n', 0, idx)
end = text.find('\n\n', idx)
print(f'--- {needle} ---')
print(text[idx: end if end != -1 else idx+500])
PYRepository: phase-rs/phase
Length of output: 949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- scenario.rs: cast builder + commit/resolve methods ---'
sed -n '1268,1325p' crates/engine/src/game/scenario.rs
sed -n '2535,2625p' crates/engine/src/game/scenario.rs
echo '--- integration tests using manual stack assembly ---'
sed -n '1,120p' crates/engine/tests/integration/issue_3300_counter_spell.rs
sed -n '1,120p' crates/engine/tests/integration/issue_3996_return_the_favor.rs
sed -n '1,120p' crates/engine/tests/integration/issue_2925_mystic_confluence_repeated_counter.rsRepository: phase-rs/phase
Length of output: 19039
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('crates/engine/src/game/scenario.rs').read_text()
for marker in [
'pub fn cast(&mut self, spell: ObjectId) -> SpellCast',
'impl SpellCast',
'pub fn resolve(self) -> CastOutcome',
'pub fn try_resolve(self) -> Result<CastOutcome, EngineError>',
'pub fn commit(self)',
'fn commit(self)',
'pub fn try_commit(self)',
'fn try_commit(self)',
]:
idx = text.find(marker)
if idx == -1:
continue
start = text.rfind('\n', 0, idx)
end = text.find('\n\n', idx)
print(f'===== {marker} =====')
print(text[idx: end if end != -1 else idx + 900])
print()
PYRepository: phase-rs/phase
Length of output: 1700
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact Reviewed test plus comparable hand-built stack tests.
sed -n '280,320p' crates/engine/tests/integration/undying_malice_edict_sacrifice_5942.rs
printf '\n===== issue_3300_counter_spell =====\n'
sed -n '1,120p' crates/engine/tests/integration/issue_3300_counter_spell.rs
printf '\n===== issue_3996_return_the_favor =====\n'
sed -n '1,120p' crates/engine/tests/integration/issue_3996_return_the_favor.rs
printf '\n===== issue_2925_mystic_confluence_repeated_counter =====\n'
sed -n '1,140p' crates/engine/tests/integration/issue_2925_mystic_confluence_repeated_counter.rsRepository: phase-rs/phase
Length of output: 15380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
log=/tmp/coderabbit-shell-logs/shell-output-P2JZIY
if [ -f "$log" ]; then
echo '--- scenario.rs excerpt from previous log around SpellCast methods ---'
rg -n -A 8 -B 8 "pub fn cast\(&mut self, spell: ObjectId\)|pub fn resolve\(self\) -> CastOutcome|pub fn try_resolve\(self\) -> Result<CastOutcome, EngineError>|pub fn commit\(self\)|pub fn try_commit\(self\)" "$log" | head -n 120
fiRepository: phase-rs/phase
Length of output: 1361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- sacrifice resolver source-id usage ---'
sed -n '130,240p' crates/engine/src/game/effects/sacrifice.rs
echo '--- stack resolution path for a StackEntry ---'
rg -n --max-count 40 "ability\.source_id|StackEntryKind::Spell|StackEntry \{|source_id: stack_entry\.source_id|resolved.*source_id" crates/engine/src/game/scenario.rs crates/engine/src/game/stack.rs crates/engine/src/game/effects/sacrifice.rs | head -n 200Repository: phase-rs/phase
Length of output: 9505
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A 12 -B 4 "pub fn cast\(&mut self, spell: ObjectId\) -> SpellCast<'_>|pub fn resolve\(self\) -> CastOutcome|pub fn try_resolve\(self\) -> Result<CastOutcome, EngineError>|pub fn commit\(self\)|pub fn try_commit\(self\)" crates/engine/src/game/scenario.rsRepository: phase-rs/phase
Length of output: 3694
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- stack.rs around source-id comparison ---'
sed -n '2705,2745p' crates/engine/src/game/stack.rs
echo '--- stack.rs around spell resolution dispatch ---'
sed -n '840,940p' crates/engine/src/game/stack.rs
echo '--- any direct use of ability.source_id during stack-entry resolution ---'
rg -n --max-count 60 "ability\.source_id.*source_id|source_id != \*source_id|StackEntryKind::Spell \{|\bability: Some\(Box::new\(ResolvedAbility" crates/engine/src/game/stack.rs crates/engine/src/game/scenario.rs | head -n 120Repository: phase-rs/phase
Length of output: 9097
Use runner.cast(edict).commit() here instead of hand-assembling the stack entry. commit() already leaves the spell on the stack with the real source id; the current ObjectId(0) + edict_id split can diverge from the cast pipeline the test is meant to exercise.
🤖 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/tests/integration/undying_malice_edict_sacrifice_5942.rs`
around lines 254 - 322, Replace the manually constructed Diabolic Edict stack
entry in undying_malice_cast_in_response_to_edict_on_stack with
runner.cast(edict).commit(), using the committed spell’s real source and stack
state. Remove the direct parse/create_object/StackEntry setup and preserve the
priority window so P0 can cast Undying Malice in response.
matthewevans
approved these changes
Jul 30, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Approved: the current-head regression coverage is discriminating and the maintainer fixture correction is clean.
matthewevans
added
test
matthewevans
enabled auto-merge
July 30, 2026 01:12
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.
Summary
Investigates #5942 ("Undying Malice effect doesn't trigger with edict effects").
Undying Malice grants a "dies" trigger to a target creature via a
GenericEffect/Continuousstatic ability with aGrantTriggermodification bound throughParentTarget. That seam — a runtime-granted trigger on a recipient that later leaves the battlefield via a resolving effect rather than a directly-known sacrifice — had zero direct test coverage before this PR.I could not reproduce the reported defect on current
mainacross every edict shape I could construct:EffectZoneChoice)player_scope: Alledict (Innocent Blood — two different creatures from two different controllers departing "simultaneously" per CR 101.4/603.3b)All four pass as written. Each funnels through the same
sacrifice_permanent→zone_pipeline::deliver→zones::move_to_zonepath, and the CR 603.6d/603.10a leaves-the-battlefield LKI snapshot correctly captures the granted trigger before the object's continuous-effect grants are pruned, in every case tested.This PR adds the four tests as durable regression coverage for a previously-untested engine seam. It does not change any engine behavior. Full investigation notes are posted on the issue.
Test plan
cargo test -p phase-engine --test integration undying_malice_edict— 4/4 passcargo clippy -p phase-engine --test integration -- -D warnings— cleancargo fmt --all— no diff in touched filesSummary by CodeRabbit