Partial: Avenge - #6994
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 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)
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 |
|
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
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.
matthewevans
removed
the
needs-maintainer
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 3167-3176: Replace the direct StaticDefinition construction in the
affected test with an end-to-end setup that parses the Avenge Oracle text, then
prepares a spell cast through the real parsing and lowering path. Verify the
parsed AnyPlayerAttackedYouLastTurn condition reaches the cost modifier and
applies the reduction, ensuring the test fails if lowering swallows the
condition.
🪄 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: 33129be0-551f-48aa-841c-858dcee10310
⛔ Files ignored due to path filters (1)
crates/engine/tests/fixtures/cr733/authority_matrix.json.gzis excluded by!**/*.gz
📒 Files selected for processing (15)
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.rscrates/engine/tests/integration/deterministic_game_state_serde.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/engine/src/parser/oracle_effect/conditions.rs
- crates/engine/src/game/ability_rw.rs
- crates/engine/src/game/coverage.rs
- crates/engine/src/parser/oracle_static/tests.rs
- crates/engine/src/parser/oracle_condition.rs
- crates/engine/src/parser/oracle_trigger.rs
- crates/engine/src/types/ability.rs
- crates/engine/src/parser/oracle_nom/condition.rs
- crates/engine/src/game/quantity.rs
- crates/engine/src/types/game_state.rs
| let mut def = StaticDefinition::new(StaticMode::ModifyCost { | ||
| mode: CostModifyMode::Reduce, | ||
| amount: ManaCost::generic(2), | ||
| spell_filter: None, | ||
| dynamic_count: None, | ||
| }) | ||
| .affected(TargetFilter::SelfRef) | ||
| .condition(StaticCondition::AnyPlayerAttackedYouLastTurn); | ||
| def.active_zones = crate::types::zones::self_spell_cost_mod_active_zones(); | ||
| obj.static_definitions.push(def); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Test the parsed Avenge path before merge.
Lines 3167-3176 construct StaticDefinition::ModifyCost and StaticCondition::AnyPlayerAttackedYouLastTurn directly. The documented coverage audit still reports that parsed Avenge cost reduction is unsupported because lowering swallows the condition. This test passes while the real Oracle card remains unsupported.
Route the parsed condition into the real cost modifier. Add an end-to-end test that parses Avenge Oracle text and prepares the spell cast.
🤖 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 3167 - 3176, Replace
the direct StaticDefinition construction in the affected test with an end-to-end
setup that parses the Avenge Oracle text, then prepares a spell cast through the
real parsing and lowering path. Verify the parsed AnyPlayerAttackedYouLastTurn
condition reaches the cost modifier and applies the reduction, ensuring the test
fails if lowering swallows the condition.
Source: Path instructions
|
Maintainer hold — current head The source/serializer correction is clean. This branch is behind current |
|
Maintainer hold — current head The current source is clean, but |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Re-reviewed current main-refreshed head 6ab4ce9: deterministic attack-history serialization and its exact census coverage are complete; CI is green.
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