Fix Fevered Visions#6563
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe parser recognizes opponent-qualified scoped hand and life predicates, while player filters use team topology for opponent relationships. Tests cover parser output, Two-Headed Giant behavior, Fevered Visions resolution, snapshots, and updated misparse metrics. ChangesOpponent-scoped condition handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/effects/mod.rs (1)
341-341: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSibling
matches_player_scopestill has the pre-existing non-topology-aware Opponent bug this PR fixes elsewhere.
scoped_player_matches_filterwas just switched fromcandidate != controllertocrate::game::players::is_opponent(state, controller, candidate)forOpponent/OpponentLostLife/OpponentGainedLife(CR 102.2/102.3: opponent-ness excludes teammates in team formats).matches_player_scope— the primary driver forplayer_scopefan-out (resolve_ability_chain's_ => apnap.filter(|pid| matches_player_scope(...))branch) — still uses rawp.id != controllerforOpponent(line 341),OpponentLostLife/OpponentGainedLife(353-358),OpponentAttacked(384-388), andOpponentAttackingEnchantedPlayer(395-400). Notably,OpponentOtherThanTriggeringandOpponentOfTriggeringPlayerin this very same match block (lines 441, 456-462) already correctly callcrate::game::players::is_opponent(...), so the helper is already in scope here — these arms are simply stale.In a Two-Headed Giant / team-multiplayer game, any "each opponent ..." effect driven through the primary
player_scopepath (not the narrowerScopedPlayerMatchesgate) will incorrectly treat teammates as opponents.🐛 Proposed fix to align the remaining arms with the topology-aware helper
- PlayerFilter::Opponent => p.id != controller, + PlayerFilter::Opponent => crate::game::players::is_opponent(state, controller, p.id),- PlayerFilter::OpponentLostLife => { - p.id != controller && p.life_lost_this_turn > 0 - } - PlayerFilter::OpponentGainedLife => { - p.id != controller && p.life_gained_this_turn > 0 - } + PlayerFilter::OpponentLostLife => { + crate::game::players::is_opponent(state, controller, p.id) + && p.life_lost_this_turn > 0 + } + PlayerFilter::OpponentGainedLife => { + crate::game::players::is_opponent(state, controller, p.id) + && p.life_gained_this_turn > 0 + }- PlayerFilter::OpponentAttacked { subject, scope } => { - p.id != controller - && state - .opponent_attacked(*subject, *scope, controller, source_id, p.id) - } + PlayerFilter::OpponentAttacked { subject, scope } => { + crate::game::players::is_opponent(state, controller, p.id) + && state + .opponent_attacked(*subject, *scope, controller, source_id, p.id) + }- PlayerFilter::OpponentAttackingEnchantedPlayer => { - p.id != controller - && enchanted_player_anchor(state, source_id).is_some_and(|enchanted| { - state.player_attacked_player_this_combat(p.id, enchanted) - }) - } + PlayerFilter::OpponentAttackingEnchantedPlayer => { + crate::game::players::is_opponent(state, controller, p.id) + && enchanted_player_anchor(state, source_id).is_some_and(|enchanted| { + state.player_attacked_player_this_combat(p.id, enchanted) + }) + }Also applies to: 353-358, 384-388, 395-400
🤖 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/mod.rs` at line 341, Update the remaining opponent-related arms in matches_player_scope—Opponent, OpponentLostLife, OpponentGainedLife, OpponentAttacked, and OpponentAttackingEnchantedPlayer—to use crate::game::players::is_opponent(state, controller, candidate) instead of raw ID inequality, matching the already-correct OpponentOtherThanTriggering and OpponentOfTriggeringPlayer arms.
🤖 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/fevered_visions.rs`:
- Line 11: Update FEVERED_VISIONS_ORACLE to use Fevered Visions’ exact generated
Oracle card text rather than the paraphrased “this enchantment” wording.
Preserve the fixture’s intended rules text while ensuring the test exercises the
production card-text parser path.
---
Outside diff comments:
In `@crates/engine/src/game/effects/mod.rs`:
- Line 341: Update the remaining opponent-related arms in
matches_player_scope—Opponent, OpponentLostLife, OpponentGainedLife,
OpponentAttacked, and OpponentAttackingEnchantedPlayer—to use
crate::game::players::is_opponent(state, controller, candidate) instead of raw
ID inequality, matching the already-correct OpponentOtherThanTriggering and
OpponentOfTriggeringPlayer arms.
🪄 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: d43b6a9d-5cf3-4524-807a-a9203a945813
⛔ Files ignored due to path filters (2)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__fevered_visions_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__fevered_visions_lowered.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (8)
crates/engine/src/game/effects/mod.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/tests/integration/fevered_visions.rscrates/engine/tests/integration/main.rsdocs/parser-misparse-backlog.md
| use engine::types::phase::Phase; | ||
| use engine::types::player::PlayerId; | ||
|
|
||
| const FEVERED_VISIONS_ORACLE: &str = "At the beginning of each player's end step, that player draws a card. If the player is your opponent and has four or more cards in hand, this enchantment deals 2 damage to that player."; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use Fevered Visions’ exact Oracle text.
The fixture paraphrases the self-reference as “this enchantment”; that can hit a different parser path and leave the production card text untested. Load the generated card text or replace it with the exact Oracle wording.
🤖 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/fevered_visions.rs` at line 11, Update
FEVERED_VISIONS_ORACLE to use Fevered Visions’ exact generated Oracle card text
rather than the paraphrased “this enchantment” wording. Preserve the fixture’s
intended rules text while ensuring the test exercises the production card-text
parser path.
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
…relation a real alt() axis Maintainer fixup on top of the contributor's work, addressing the two non-blocking review findings: 1. `matches_player_scope` was internally inconsistent under CR 102.3: five arms tested raw `p.id != controller` while `OpponentOtherThanTriggering` in the same match block already routed through the canonical `players::is_opponent` authority. Under team play a teammate is neither the controller nor an opponent, and this is the effect-recipient enumeration, so the raw inequality admitted teammates as recipients of `Opponent`-scoped effects. All five now use `is_opponent`. 2. `parse_scoped_player_opponent_and_has_condition` hardcoded `PlayerFilter::Opponent` at the return while discarding the matched tag, so the documented "relation axis" did not exist. Extracted `parse_scoped_player_relation` returning `PlayerFilter` via `value()` inside `alt()`, mirroring `parse_condition_connector`, so a further relation is a one-line arm. Doc comment corrected to describe what the code does, and the fused `" and has "` connector's justification recorded inline. CR 102.2 / CR 102.3 grep-verified against docs/MagicCompRules.txt. Co-authored-by: invalidCards <842080+invalidCards@users.noreply.github.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sign-off. I landed the two requested fixups myself in e304f9bb55 rather than send a first contribution back for another round — you are credited via Co-authored-by.
(1) Routed the five raw p.id != controller arms in matches_player_scope (Opponent, OpponentLostLife, OpponentGainedLife, OpponentAttacked, OpponentAttackingEnchantedPlayer) through crate::game::players::is_opponent, matching the idiom already used by OpponentOtherThanTriggering in the same match block. CR 102.2 / CR 102.3 annotation grep-verified against docs/MagicCompRules.txt (lines 252 / 254).
(2) Extracted parse_scoped_player_relation returning PlayerFilter via value() inside alt() — mirroring parse_condition_connector — so the relation is a real grammar axis and a further relation is a one-line arm. Corrected the "three grammar axes" doc comment to describe what the code actually does, and recorded inline why the " and has " connector is legitimately fused (parse_condition_connective is typed OracleResult<'_, StaticCondition>, and StaticCondition has no player-relationship variant, so it structurally cannot join a relation leg to a scalar leg without an add-engine-variant-gated proposal).
Findings (3), (4) and (5) need no action.
CodeRabbit's inline finding on tests/integration/fevered_visions.rs is refuted, not skipped. I byte-compared FEVERED_VISIONS_ORACLE against oracle_text for fevered visions in the generated card-data.json: both 185 bytes, cmp clean. "this enchantment" is not a paraphrase — it is the exact generated text the production parser consumes, so the fixture is correct as written.
Genuinely strong first contribution: right seam, zero hallucinated CR citations, discriminating runtime tests through real production wiring, and hostile-adjacency cases you were not asked for.
…mp (#6568) `release: v0.35.2` (21a53d5) bumped `client/src-tauri/Cargo.toml` to 0.35.2 via cargo-release's `pre-release-replacements`, but `client/src-tauri/` is a separate cargo workspace with its own lockfile that cargo-release does not manage. The lock still recorded `phase-tauri 0.35.1`, so the `tauri-check` CI job's `cargo check --locked --manifest-path client/src-tauri/Cargo.toml` refused to reconcile the mismatch and exited 101. That reds the required `Rust (fmt, clippy, test, coverage-gate)` aggregator (which `needs: [... tauri-check]`) on every open PR whose merge ref includes the release commit, blocking the merge queue repo-wide. Evidence: PR #6561 tauri-check PASSED at 20:43:00Z; the release commit landed at 20:49:54Z; PRs #6564 (20:56:21Z) and #6563 (21:15:11Z) both FAILED with the identical --locked error. None of the three touched any Cargo manifest. Follow-up (not in this change): cargo-release should keep the nested lockfile in sync so the next release does not re-break it. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Re-approving on 8c3dfcafe after a maintainer update-branch.
Both red required checks on the previous head (e304f9bb5) were maintainer-caused, verified by ancestry rather than timestamps:
Tauri compile checkwas thecargo check --lockedbreakage from therelease: v0.35.2bump. Its fix,cb7d5a25f(#6568), was not an ancestor ofe304f9bb5— the merge-base was9613bcc22, which predates it.- The
Rust (fmt, clippy, test, coverage-gate)aggregator was red only because itsTAURI_RESULTinput failed; every other input wassuccess.
This PR touches zero client/src-tauri/ paths, so it could not have caused either failure. Per the maintainer-caused-staleness policy, we port rather than asking the contributor to chase main.
The update delta is merge-only: 5 main commits plus the merge commit, touching only main-side files. None of this PR's engine/parser files moved, so the reviewed diff is unchanged.
Auto-merge remains armed.
Resolves the deterministic crates/engine/tests/integration/main.rs mod-line conflict by keeping both test modules in rustfmt-sorted order. Maintainer-side port: main advanced under this PR tonight (phase-rs#6538 phase-rs#6543 phase-rs#6545 phase-rs#6554 phase-rs#6563 phase-rs#6579). The phase-tauri 0.35.1->0.35.2 client/src-tauri/Cargo.lock bump is now a no-op, since main landed the identical bump in phase-rs#6568.
…mp (phase-rs#6568) `release: v0.35.2` (21a53d5) bumped `client/src-tauri/Cargo.toml` to 0.35.2 via cargo-release's `pre-release-replacements`, but `client/src-tauri/` is a separate cargo workspace with its own lockfile that cargo-release does not manage. The lock still recorded `phase-tauri 0.35.1`, so the `tauri-check` CI job's `cargo check --locked --manifest-path client/src-tauri/Cargo.toml` refused to reconcile the mismatch and exited 101. That reds the required `Rust (fmt, clippy, test, coverage-gate)` aggregator (which `needs: [... tauri-check]`) on every open PR whose merge ref includes the release commit, blocking the merge queue repo-wide. Evidence: PR phase-rs#6561 tauri-check PASSED at 20:43:00Z; the release commit landed at 20:49:54Z; PRs phase-rs#6564 (20:56:21Z) and phase-rs#6563 (21:15:11Z) both FAILED with the identical --locked error. None of the three touched any Cargo manifest. Follow-up (not in this change): cargo-release should keep the nested lockfile in sync so the next release does not re-break it. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix: prevent Fevered Visions controller damage * fix(PR-6563): route opponent arms through players::is_opponent; make relation a real alt() axis Maintainer fixup on top of the contributor's work, addressing the two non-blocking review findings: 1. `matches_player_scope` was internally inconsistent under CR 102.3: five arms tested raw `p.id != controller` while `OpponentOtherThanTriggering` in the same match block already routed through the canonical `players::is_opponent` authority. Under team play a teammate is neither the controller nor an opponent, and this is the effect-recipient enumeration, so the raw inequality admitted teammates as recipients of `Opponent`-scoped effects. All five now use `is_opponent`. 2. `parse_scoped_player_opponent_and_has_condition` hardcoded `PlayerFilter::Opponent` at the return while discarding the matched tag, so the documented "relation axis" did not exist. Extracted `parse_scoped_player_relation` returning `PlayerFilter` via `value()` inside `alt()`, mirroring `parse_condition_connector`, so a further relation is a one-line arm. Doc comment corrected to describe what the code does, and the fused `" and has "` connector's justification recorded inline. CR 102.2 / CR 102.3 grep-verified against docs/MagicCompRules.txt. Co-authored-by: invalidCards <842080+invalidCards@users.noreply.github.com> --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
2 participants
Summary
Fixes the dropped resolution-time opponent/hand-size gate for Fevered Visions, so its controller still draws but no longer takes 2 damage. Removes Fevered Visions from the parser misparse backlog after generated evidence confirms
supported=true,gap_count=0, and noCondition_Ifor unimplemented effect.The opponent predicate is team-aware at the existing scoped-player evaluator seam. This PR does not claim the separate pre-existing CR 805.4d multi-trigger fanout behavior for Two-Headed Giant phase triggers.
Files changed
crates/engine/src/game/effects/mod.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rscrates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__fevered_visions_ir.snapcrates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__fevered_visions_lowered.snapcrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/tests/integration/fevered_visions.rscrates/engine/tests/integration/main.rsdocs/parser-misparse-backlog.mdCR references
Implementation method (required)
Method: /engine-implementer
Track
Developer
LLM
Model: codex-5
Tier: Standard
Thinking: high
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all -- --check— PASScargo clippy-strict— environment could not start the workspace wrapper because system OpenSSL development metadata is unavailable; the strict engine-package alternative below passed.cargo clippy -p engine --all-targets -- -D warnings— PASS on the final refreshed upstream base.cargo test -p engine— PASS: 17,603 library tests and 3,879 integration tests; 0 failed; doc tests 0 failed.MTGJSON_SKIP_REFRESH=1 ./scripts/gen-card-data.sh— PASS: 34,724 cards generated; Fevered Visionssupported=true,gap_count=0, noCondition_If, no unimplemented effect.cargo coverage— PASS: 31,528 / 35,501 supported (88.8%).cargo semantic-audit— PASS: 32,520 cards audited; exit 0../scripts/check-skill-doc.sh— PASS.git diff --check— PASS.Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=ad21207144b99f091a0bc99fed7bb7c9fb6f51e5 base=9613bcc22f251adc3e458c059b2b6cf7d7ec5327
Anchored on
crates/engine/src/parser/oracle_nom/condition.rs:85— existing nomalt()connective composition for compound conditions.crates/engine/src/parser/oracle_nom/condition.rs:3522— existing relative-player hand/life predicate composition.crates/engine/src/parser/oracle_effect/conditions.rs:5169— production ability-condition nom dispatcher and context bridge.crates/engine/src/game/effects/mod.rs:456— existing canonical team-aware opponent relation at the runtime player-filter seam.Final review-impl
Final review-impl PASS head=ad21207144b99f091a0bc99fed7bb7c9fb6f51e5
Claimed parse impact
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
Bug Fixes
New Features
Tests & Documentation