Partial: Avenge - #6994
Conversation
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughThis change adds ChangesAvenge attack condition
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant execute_cleanup
participant GameState
participant ModifyCost
Player->>execute_cleanup: complete turn with attack targets
execute_cleanup->>GameState: snapshot attacked defenders
ModifyCost->>GameState: query last-turn attack condition
GameState-->>ModifyCost: return whether an opponent attacked controller
ModifyCost-->>Player: apply conditional cost reduction
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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/game/casting_tests.rsast-grep timed out on this file Comment |
|
Generated for head Parse changes introduced by this PR · 1 card(s), 1 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: 2
🤖 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/casting_tests.rs`:
- Around line 2886-2901: Add a test in the existing AnyPlayerAttackedYouLastTurn
cases near evaluate_condition_for_test where P2 records attacking you, then
eliminate or remove P2 from the game using the established GameState API, and
assert the condition evaluates false. Ensure the stale
attacked_defenders_last_turn entry remains present so the test verifies
eliminated attackers are ignored.
In `@crates/engine/src/game/layers.rs`:
- Around line 1617-1621: Update the
StaticCondition::AnyPlayerAttackedYouLastTurn evaluation to stop filtering
attackers with !p.is_eliminated, allowing departed opponents’ recorded attacks
to remain valid. Enforce expiry when the departed player’s skipped next-turn
boundary is reached in the existing last-turn attack record lifecycle,
preserving attacks until that boundary.
🪄 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: 399d8fd8-31f7-4349-82dc-b5ed15b5f341
📒 Files selected for processing (14)
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/coverage.rscrates/engine/src/game/layers.rscrates/engine/src/game/quantity.rscrates/engine/src/game/turns.rscrates/engine/src/parser/oracle_condition.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/types/ability.rscrates/engine/src/types/game_state.rs
| // Only P2 attacked you (P1 attacked no one) ⇒ true (existential over players). | ||
| let mut state = GameState::new(FormatConfig::standard(), 3, 7); | ||
| state | ||
| .attacked_defenders_last_turn | ||
| .insert(PlayerId(2), [you].into_iter().collect()); | ||
| assert!(evaluate_condition_for_test(&state, &cond, you, src)); | ||
|
|
||
| // Opponents attacked each other but not you ⇒ false (the defender must be you). | ||
| let mut state = GameState::new(FormatConfig::standard(), 3, 7); | ||
| state | ||
| .attacked_defenders_last_turn | ||
| .insert(PlayerId(1), [PlayerId(2)].into_iter().collect()); | ||
| state | ||
| .attacked_defenders_last_turn | ||
| .insert(PlayerId(2), [PlayerId(1)].into_iter().collect()); | ||
| assert!(!evaluate_condition_for_test(&state, &cond, you, src)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover an eliminated attacker.
Add a case where P2 attacked P0, then P2 leaves the game, and assert that AnyPlayerAttackedYouLastTurn is false. These cases only cover active opponents. A stale attack-history entry must not grant Avenge's reduction after its attacker leaves the multiplayer game. (media.wizards.com)
As per path instructions, check edge cases where eliminated players remain referenced.
🤖 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/casting_tests.rs` around lines 2886 - 2901, Add a test
in the existing AnyPlayerAttackedYouLastTurn cases near
evaluate_condition_for_test where P2 records attacking you, then eliminate or
remove P2 from the game using the established GameState API, and assert the
condition evaluates false. Ensure the stale attacked_defenders_last_turn entry
remains present so the test verifies eliminated attackers are ignored.
Source: Path instructions
| StaticCondition::AnyPlayerAttackedYouLastTurn => state.players.iter().any(|p| { | ||
| !p.is_eliminated | ||
| && p.id != controller | ||
| && state.player_attacked_player_last_turn(p.id, controller) | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve a departed opponent’s last-turn attack until the required expiry boundary.
Line 1618 excludes an attacker as soon as that player leaves the game. CR 800.4i requires effects that inspect actions during a player’s last turn to retain that information until that player’s next turn would have begun. An opponent can attack the controller, leave the game, and still satisfy Avenge before that boundary. Expire the stored record at the departed player’s skipped turn boundary instead of filtering the player here. (media.wizards.com)
As per path instructions, “Implement MTG behavior according to the Comprehensive Rules.”
🤖 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/layers.rs` around lines 1617 - 1621, Update the
StaticCondition::AnyPlayerAttackedYouLastTurn evaluation to stop filtering
attackers with !p.is_eliminated, allowing departed opponents’ recorded attacks
to remain valid. Enforce expiry when the departed player’s skipped next-turn
boundary is reached in the existing last-turn attack record lifecycle,
preserving attacks until that boundary.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the last-turn defender ledger is preserved but not given the CR-required departed-player expiry.
🔴 Blocker
[HIGH] Eliminated players are excluded too early from the layer predicate. Evidence: crates/engine/src/game/layers.rs:1617-1621 filters eliminated players, while docs/MagicCompRules.txt:6429-6433 (CR 800.4i) says actions during a player's last turn remain findable until that player's next turn would have begun. types/game_state.rs:14142-14152 and the cleanup in turns.rs:2148-2161 preserve the record, but introduce no departed-player expiry. Why it matters: the engine loses a required last-turn attack relationship immediately on elimination instead of retaining it through the specified skipped-turn boundary. Suggested fix: carry the relationship through that CR boundary, then expire it when the eliminated player's next turn would begin; add a production-pipeline test for both sides of that boundary.
[HIGH] Required CI is failing the CR733 authority census. Evidence: run 30930044012, job 92062093858, reports attacked_defenders_last_turn as missing. Why it matters: the lifecycle field lacks the required authoritative classification/fixture evidence. Suggested fix: classify it at the authority that owns its lifecycle and update the corresponding fixture consistently with the retention/expiry behavior above.
✅ Evidence checked
The current head-bound parse-diff is scoped to Avenge only: one changed static/ReduceCost signature, matching the claimed scope. I also checked CodeRabbit's immediate-elimination suggestion against CR 800.4i; it conflicts with the verified rule and is not the requested fix.
Recommendation: request changes for the CR 800.4i retention boundary and the CR733 census failure, then re-request review on a new head.
2 participants
Summary
Fixes a parse-fidelity defect on Avenge.
Issue: Cost-reduction condition "if a player attacked you during their last turn" is dropped (ModifyCost condition=null), so the {2} reduction applies unconditionally instead of only when the attack condition holds.
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— pass (clean)./scripts/check-parser-combinators.sh— pass (Gate G PASS + Gate A PASS; Family D NOT skipped — ran via working msys64 python3 3.9.7, detector self-test suite 10/10 ok; WindowsApps python3 stub bypassed by removing it from PATH)cargo clippy-strict— pass (exit 0, no warnings)cargo test -p phase-engine— fail (18475 passed, 1 failed: stage2_injector_tests::the_cr_603_5_prompt_census... — Windows-only path-separator artifact, unrelated to Avenge; also cleared a 97GB target/debug/incremental dir that had filled the disk before this run)cargo export-cards data --stats --sidecar-dir client/public && cp client/public/card-data.json data/card-data.json— pass (card-data regenerated fresh against this branch; sidecar copied to data/, 98176031 bytes)cargo coverage— fail-result (command exit 0, but Avenge supported:false gap_count:1 — Swallow:Condition_If on the cost-reduction clause)cargo semantic-audit— pass (command exit 0; 32700 supported audited, 295 flagged; Avenge has 0 findings — it is not in the supported-audit set)Scope Expansion
Scope grew by 3 files beyond the plan's list: the plan missed three exhaustive StaticCondition mappers (oracle_trigger.rs, oracle_effect/conditions.rs, oracle_condition.rs) that require a compile-mandatory "-> None" arm.
Validation Failures
See review/cross-check notes.
CI Failures
Summary by CodeRabbit
New Features
Bug Fixes
Tests