Fix Onakke Oathkeeper - #6850
Conversation
Co-Authored-By: Codex gpt-5.6 <noreply@openai.com>
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe parser now supports planeswalker-only attack restrictions and combat taxes. Tests cover parser output, IR lowering, Onakke Oathkeeper activation, combat taxes, and restriction snapshots from Orzhov Advokist and Willie Lumpkin. ChangesPlaneswalker restriction support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant RestrictionState
participant CombatEngine
participant IntegrationTests
OracleParser->>RestrictionState: Create planeswalker-only restriction
RestrictionState->>CombatEngine: Apply scope and per-attacker tax
CombatEngine->>IntegrationTests: Accept or reject attack declaration
Possibly related PRs
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: 6
🧹 Nitpick comments (1)
crates/engine/tests/integration/willie_lumpkin_cant_attack.rs (1)
411-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
attack_legal_from_runnerduplicatesattack_is_legal.
crates/engine/tests/integration/orzhov_advokist.rsLines 126-136 already defines the same clone-state, set-active_player,declare_attackersprobe. The only difference is that this version takes&GameRunnerinstead of&GameState. Extract one shared helper into the integration test support module and call it from both files.As per coding guidelines: "Before adding logic, search for and reuse existing building blocks".
♻️ Suggested shared helper
// in the integration test support module pub fn attack_is_legal( state: &GameState, controller: PlayerId, attacker: ObjectId, target: AttackTarget, ) -> bool { let mut state = state.clone(); state.active_player = controller; let mut events = Vec::new(); declare_attackers(&mut state, &[(attacker, target)], &mut events).is_ok() }Call it with
runner.state()from this file.🤖 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/willie_lumpkin_cant_attack.rs` around lines 411 - 421, Extract the shared clone-state, active-player assignment, and declare_attackers probe into the integration test support module as attack_is_legal accepting &GameState, then remove the duplicate logic from attack_legal_from_runner and call the helper with runner.state(). Update the existing attack_is_legal usage in orzhov_advokist.rs to use the shared support helper, preserving its current boolean behavior.Source: Coding guidelines
🤖 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/parser/oracle_static/evasion.rs`:
- Around line 2643-2646: Correct the CR annotations for the combat-tax behavior:
in crates/engine/src/parser/oracle_static/evasion.rs lines 2643-2646, cite CR
508.1b/c for attack targets and restrictions and CR 508.1h where the annotation
describes attack costs; update the corresponding test annotation in
crates/engine/src/parser/oracle_static/tests.rs lines 25869-25871 to cite the
asserted restriction and cost rules; update the fixture annotation in
crates/engine/src/parser/oracle_ir/snapshot_tests.rs lines 2490-2493 to
distinguish the combat restriction from the activated-ability rule.
In `@crates/engine/tests/integration/onakke_oathkeeper.rs`:
- Around line 62-79: Update the Oathkeeper integration test to first submit an
activation targeting opponent_planeswalker through
try_activate(...).target_object(...).pay_with(...).attempt(), assert the attempt
fails, and add a paired positive reach-guard confirming this illegal target
reached the action pipeline. Then retain the existing legal activation targeting
own_planeswalker and its resulting state assertions.
- Around line 20-23: Update the documentation comment above the graveyard
activation test to replace the incorrect CR 118.12 citation with CR 602.2,
leaving the activation behavior description and other citation unchanged.
In `@crates/engine/tests/integration/orzhov_advokist.rs`:
- Around line 402-418: The snapshot-immutability tests must use the production
control-change path instead of raw controller writes. In
crates/engine/tests/integration/orzhov_advokist.rs lines 402-418, replace the
direct assignment with add_transient_continuous_effect targeting source and P2,
call evaluate_layers, and assert the updated controller before move_to_zone;
apply the same change with THIRD in
crates/engine/tests/integration/willie_lumpkin_cant_attack.rs lines 393-399,
including the controller assertion before move_to_zone.
In `@crates/engine/tests/integration/rules/combat.rs`:
- Around line 858-868: Replace the hand-built WaitingFor::DeclareAttackers
assignment in the combat fixture with the normal phase-pipeline transition into
DeclareAttackers, allowing the engine to compute attacker_constraints and
valid_attack_targets_by_attacker from current restrictions and static abilities.
Before declaring attackers, assert the generated window includes the expected
attackers, targets, and planeswalker-scope constraint data, then preserve the
existing attack actions and no-tax assertions.
- Around line 1004-1053: Update the combat test scenario around the repeated
attacker declarations to use a fresh runner/setup for each declaration, matching
the independent setup used near the first declaration instead of reusing stale
combat state. Ensure each scenario starts without prior attacking or tax-payment
state, and replace the first broad CombatTaxPayment presence assertion with an
assertion of the exact expected tax cost for the P2 planeswalker; retain the
post-Oathkeeper-removal assertion that no tax window is created.
---
Nitpick comments:
In `@crates/engine/tests/integration/willie_lumpkin_cant_attack.rs`:
- Around line 411-421: Extract the shared clone-state, active-player assignment,
and declare_attackers probe into the integration test support module as
attack_is_legal accepting &GameState, then remove the duplicate logic from
attack_legal_from_runner and call the helper with runner.state(). Update the
existing attack_is_legal usage in orzhov_advokist.rs to use the shared support
helper, preserving its current boolean behavior.
🪄 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: 15f21bab-e3d9-44f8-8907-8777ca28541b
⛔ Files ignored due to path filters (2)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__onakke_oathkeeper_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__onakke_oathkeeper_lowered.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (11)
crates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rscrates/engine/src/parser/oracle_static/evasion.rscrates/engine/src/parser/oracle_static/shared.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/onakke_oathkeeper.rscrates/engine/tests/integration/orzhov_advokist.rscrates/engine/tests/integration/rules/combat.rscrates/engine/tests/integration/willie_lumpkin_cant_attack.rsdocs/parser-misparse-backlog.md
💤 Files with no reviewable changes (1)
- docs/parser-misparse-backlog.md
| /// CR 602.1 + CR 118.12: the printed graveyard activation pays | ||
| /// `{4}{W}{W}` and exiles its source, then can return only its controller's | ||
| /// planeswalker card from that graveyard. This intentionally uses the normal | ||
| /// `GameRunner::activate` action pipeline rather than a spell-cast driver. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Magic Comprehensive Rules 118.12 text costs
💡 Result:
In the Magic: The Gathering Comprehensive Rules, rule 118.12 defines specific templates for spells and abilities where an action acts as a cost [1][2]. Rule 118.12 states: Some spells, activated abilities, and triggered abilities read, “[Do something]. If [a player] [does, doesn’t, or can’t], [effect].” Or “[A player] may [do something]. If [that player] [does, doesn’t, or can’t], [effect].” The action [do something] is a cost, paid when the spell or ability resolves. The “If [a player] [does, doesn’t, or can’t]” clause checks whether the player chose to pay an optional cost or started to pay a mandatory cost, regardless of what events actually occurred [1][3]. Additionally, the rules provide two sub-rules for variations of these templates: 118.12a: Some spells, activated abilities, and triggered abilities read, “[Do something] unless [a player does something else].” This means the same thing as “[A player may do something else]. If [that player doesn’t], [do something].” [1][3] 118.12b: Some effects offer a player a choice to search a zone and take additional actions with the cards found in that zone, followed by an “If [a player] does” clause. This clause checks whether the player chose to search, not whether the player took any of the additional actions [1][3].
Citations:
- 1: https://mtg-rules.vercel.app/chapters/118
- 2: https://mtg.fandom.com/wiki/Cost
- 3: https://ancestral.vision/game-concepts/costs.html
🏁 Script executed:
#!/bin/bash
sed -n '1,45p' crates/engine/tests/integration/onakke_oathkeeper.rsRepository: phase-rs/phase
Length of output: 2140
🌐 Web query:
official Magic Comprehensive Rules 118.12 602.2 activation costs
💡 Result:
According to the Magic: The Gathering Comprehensive Rules, rules 118.12 and 602.2 govern distinct aspects of costs and activated abilities [1][2]. Rule 118.12 This rule clarifies how "if/do" clauses operate in the context of costs [3][4]. It states that when a spell, activated ability, or triggered ability contains a clause like "[Do something]. If [a player] [does, doesn't, or can't], [effect]," the action described as "[Do something]" is a cost [3][5]. This cost is paid when the spell or ability resolves, not when it is activated or cast [3][6]. The clause checks whether the player chose to pay an optional cost or initiated a mandatory cost, regardless of the actual outcome of the event [3][4]. Rule 602.2 This rule defines the process of activating an activated ability [2][7]. It states that to activate an ability is to place it on the stack and pay its costs so that it may resolve [2][7]. The activation process involves several steps, and if a player is unable to comply with any of these steps, the activation is illegal, and the game state reverts to the moment before activation [2][7]. Specifically, rule 602.2b notes that the remainder of the activation process is identical to the process for casting a spell (rules 601.2b–i), and an activated ability's "activation cost" is the analog to a spell's mana cost [2][7].
Citations:
- 1: https://media.wizards.com/2026/downloads/MagicCompRules%2020260116.pdf
- 2: https://mtg-rules.vercel.app/chapters/602
- 3: https://mtg-rules.vercel.app/chapters/118
- 4: https://crypticstudy.com/rules/
- 5: https://mtg.wiki/page/Cost
- 6: https://ancestral.vision/game-concepts/costs.html
- 7: https://mtg.wiki/page/Activated_ability
Replace CR 118.12 with CR 602.2. CR 118.12 covers actions treated as costs paid on resolution, not this printed activation cost. Cite CR 601.2f/CR 601.2h only if the comment describes cost calculation and payment.
🤖 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/onakke_oathkeeper.rs` around lines 20 - 23,
Update the documentation comment above the graveyard activation test to replace
the incorrect CR 118.12 citation with CR 602.2, leaving the activation behavior
description and other citation unchanged.
Source: Path instructions
| runner | ||
| .state_mut() | ||
| .objects | ||
| .get_mut(&source) | ||
| .unwrap() | ||
| .controller = P2; | ||
| let mut events = Vec::new(); | ||
| move_to_zone(runner.state_mut(), source, Zone::Graveyard, &mut events); | ||
| assert!( | ||
| !attack_is_legal( | ||
| runner.state(), | ||
| P1, | ||
| p1_a, | ||
| AttackTarget::Planeswalker(p0_walker) | ||
| ), | ||
| "changing or removing the source cannot mutate the resolved snapshot" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Both snapshot-immutability tests change controller by raw field write. Layer 2 owns GameObject::controller, so the next evaluate_layers pass can recompute it from continuous effects and discard the write. declare_attackers runs on a cloned state and can trigger that recomputation, which leaves the control-change half of each assertion unproven. crates/engine/tests/integration/rules/combat.rs Lines 977-985 already uses the production path for this.
crates/engine/tests/integration/orzhov_advokist.rs#L402-L418: replace the.controller = P2write withadd_transient_continuous_effect(source, P2, Duration::Permanent, TargetFilter::SpecificObject { id: source }, vec![ContinuousModification::ChangeController], None)plusevaluate_layers, then assertobjects[&source].controller == P2beforemove_to_zone.crates/engine/tests/integration/willie_lumpkin_cant_attack.rs#L393-L399: replace the.controller = THIRDwrite with the same control-change effect targetingTHIRD, callevaluate_layers, and assert the new controller beforemove_to_zone.
📍 Affects 2 files
crates/engine/tests/integration/orzhov_advokist.rs#L402-L418(this comment)crates/engine/tests/integration/willie_lumpkin_cant_attack.rs#L393-L399
🤖 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/orzhov_advokist.rs` around lines 402 - 418,
The snapshot-immutability tests must use the production control-change path
instead of raw controller writes. In
crates/engine/tests/integration/orzhov_advokist.rs lines 402-418, replace the
direct assignment with add_transient_continuous_effect targeting source and P2,
call evaluate_layers, and assert the updated controller before move_to_zone;
apply the same change with THIRD in
crates/engine/tests/integration/willie_lumpkin_cant_attack.rs lines 393-399,
including the controller assertion before move_to_zone.
Source: Path instructions
| state.waiting_for = WaitingFor::DeclareAttackers { | ||
| player: P0, | ||
| valid_attacker_ids: vec![attacker, second_attacker], | ||
| valid_attack_targets: vec![ | ||
| AttackTarget::Player(P1), | ||
| AttackTarget::Planeswalker(protected), | ||
| AttackTarget::Planeswalker(other), | ||
| ], | ||
| valid_attack_targets_by_attacker: None, | ||
| attacker_constraints: Default::default(), | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The hand-built declaration window discards engine-computed constraint data.
The fixture writes waiting_for directly with a literal target list, valid_attack_targets_by_attacker: None, and attacker_constraints: Default::default(). The production path computes those fields from the current restrictions and static abilities. A bug that fails to publish planeswalker scope into attacker_constraints or valid_attack_targets_by_attacker is therefore invisible here, and the two "no tax" negatives at Lines 917 and 932 can pass because the constraint data was never computed at all.
Advance the scenario into DeclareAttackers through the phase pipeline so the engine builds the window, then assert the window contents before acting.
🤖 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/rules/combat.rs` around lines 858 - 868,
Replace the hand-built WaitingFor::DeclareAttackers assignment in the combat
fixture with the normal phase-pipeline transition into DeclareAttackers,
allowing the engine to compute attacker_constraints and
valid_attack_targets_by_attacker from current restrictions and static abilities.
Before declaring attackers, assert the generated window includes the expected
attackers, targets, and planeswalker-scope constraint data, then preserve the
existing attack actions and no-tax assertions.
Source: Path instructions
| runner | ||
| .state_mut() | ||
| .objects | ||
| .get_mut(&attacker) | ||
| .unwrap() | ||
| .tapped = false; | ||
| runner.state_mut().waiting_for = WaitingFor::DeclareAttackers { | ||
| player: P0, | ||
| valid_attacker_ids: vec![attacker], | ||
| valid_attack_targets: vec![AttackTarget::Planeswalker(other)], | ||
| valid_attack_targets_by_attacker: None, | ||
| attacker_constraints: Default::default(), | ||
| }; | ||
| runner | ||
| .act(GameAction::DeclareAttackers { | ||
| attacks: vec![(attacker, AttackTarget::Planeswalker(other))], | ||
| bands: vec![], | ||
| }) | ||
| .expect("P2's planeswalker should now be taxed"); | ||
| assert!(matches!( | ||
| runner.state().waiting_for, | ||
| WaitingFor::CombatTaxPayment { .. } | ||
| )); | ||
|
|
||
| let mut events = Vec::new(); | ||
| engine::game::zones::move_to_zone(runner.state_mut(), oathkeeper, Zone::Graveyard, &mut events); | ||
| runner | ||
| .state_mut() | ||
| .objects | ||
| .get_mut(&attacker) | ||
| .unwrap() | ||
| .tapped = false; | ||
| runner.state_mut().waiting_for = WaitingFor::DeclareAttackers { | ||
| player: P0, | ||
| valid_attacker_ids: vec![attacker], | ||
| valid_attack_targets: vec![AttackTarget::Planeswalker(other)], | ||
| valid_attack_targets_by_attacker: None, | ||
| attacker_constraints: Default::default(), | ||
| }; | ||
| runner | ||
| .act(GameAction::DeclareAttackers { | ||
| attacks: vec![(attacker, AttackTarget::Planeswalker(other))], | ||
| bands: vec![], | ||
| }) | ||
| .expect("removing Oathkeeper removes its static tax"); | ||
| assert!(!matches!( | ||
| runner.state().waiting_for, | ||
| WaitingFor::CombatTaxPayment { .. } | ||
| )); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale combat state leaks between the three declarations on one runner.
Lines 1004-1016 and Lines 1030-1042 reuse the runner after a completed declaration. They reset only tapped and rewrite waiting_for. The attacker keeps the attacking state recorded by the previous declaration, and the previous tax window is not cleared.
Onakke Oathkeeper's tax counts each creature the attacking player controls that is attacking a protected planeswalker. Residual attacking state can inflate that count. The assertion at Line 1023 checks only that a CombatTaxPayment exists, so an inflated cost still passes. The same residue reaches the post-removal assertion at Line 1049.
Build a fresh scenario per declaration, as Line 972 already does, and assert the exact cost.
As per path instructions: "Flag constructor shortcuts ... setUp that pre-populates post-fix state ... that can silently mask the very bug a regression test claims to catch".
💚 Assert the exact cost for the P2 planeswalker
- assert!(matches!(
- runner.state().waiting_for,
- WaitingFor::CombatTaxPayment { .. }
- ));
+ match &runner.state().waiting_for {
+ WaitingFor::CombatTaxPayment { total_cost, .. } => {
+ assert_eq!(
+ total_cost.mana_value(),
+ 1,
+ "one attacker at P2's planeswalker taxes exactly {{1}}"
+ );
+ }
+ other => panic!("expected CombatTaxPayment after the control change, got {other:?}"),
+ }🤖 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/rules/combat.rs` around lines 1004 - 1053,
Update the combat test scenario around the repeated attacker declarations to use
a fresh runner/setup for each declaration, matching the independent setup used
near the first declaration instead of reusing stale combat state. Ensure each
scenario starts without prior attacking or tax-payment state, and replace the
first broad CombatTaxPayment presence assertion with an assertion of the exact
expected tax cost for the P2 planeswalker; retain the post-Oathkeeper-removal
assertion that no tax window is created.
Source: Path instructions
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — current head a971c07480ee0c6b1b1e37239c81a4e7c0a8b94f
The parser uses the right existing AttackTargetFilter::Planeswalker axis, and I verified the tested Oracle text against Scryfall. Required CI and the current parse-diff are green. Two current-head items still need correction:
-
The new CR annotations cite the wrong attacker-declaration subrule. The June 19, 2026 Comprehensive Rules define
508.1bas choosing player/planeswalker/battle targets,508.1cas checking attack restrictions,508.1das checking attack requirements, and508.1has determining attack costs. The new combat-tax comments/tests/snapshot atoracle_static/evasion.rs,oracle_static/tests.rs, andoracle_ir/snapshot_tests.rsuse508.1dfor a restriction/tax. Replace it with the verified508.1b + CR 508.1c + CR 508.1hrelationship as appropriate. Likewise,onakke_oathkeeper.rsdescribes normal activated-ability announcement/payment but cites CR 118.12 (the resolving "If [a player] does" cost form); use CR 602.2 instead. These are rules annotations, so they must be accurate before merge. -
The new graveyard-activation integration test does not prove the new target filter rejects the opponent's planeswalker. It creates the opponent card but calls
.target_object(own_planeswalker)immediately; that would also pass if the target filter wrongly admitted every planeswalker card. First drivetry_activate(...).target_object(opponent_planeswalker).pay_with(...).attempt()and assert the action is rejected, then keep the existing legal own-graveyard activation as the positive path. This gives the card’s new target-scope claim a production-pipeline regression.
Please push that focused follow-up and request re-review. No maintainer fixup was applied.
Co-Authored-By: Codex gpt-5.6 <noreply@openai.com>
|
Addressed the maintainer feedback in c86a76d:
Verification: full validation batch passed, independent review was clean, and Gate A passed on the committed head. Please re-review when convenient. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review passed. The shared defended-scope parser preserves the existing typed AttackTargetFilter::Planeswalker through combat-tax and restriction consumers. The revised activation test now reaches target selection through the production action pipeline and proves an opponent's graveyard planeswalker is rejected without state mutation. Current CI, card-data, and parse-diff evidence are green.
2 participants
Summary
Fixes the planeswalker-only combat-tax misparse for Onakke Oathkeeper and removes its verified stale backlog entry. The shared parser now preserves
AttackTargetFilter::Planeswalkerfor all consumers, while runtime tests cover live static authority, snapshotted temporary restrictions, and the card's graveyard activation.Files changed
Track
Developer
LLM
Model: gpt-5.6
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 118.12a, CR 508.1b, CR 508.1c, CR 508.1d, CR 508.1h, CR 602.1, CR 608.2c, CR 611.2c.
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— passed.cargo clippy-strict— passed.cargo test -p phase-engine— 4,308 passed; 0 failed; 2 ignored../scripts/gen-card-data.sh— passed; regenerated data reports Onakke Oathkeeper supported with zero gaps.cargo coverage— passed; Onakke Oathkeepersupported: true,gap_count: 0.cargo semantic-audit— passed; no Onakke-specific finding../scripts/check-parser-combinators.sh— passed for current head.Gate A
Gate A PASS head=a971c07480ee0c6b1b1e37239c81a4e7c0a8b94f base=d947f97469a5de1c332701f2062e1852d81ca011
Anchored on
UnlessPaywiring.can't attackdefender scopes.Final review-impl
Final review-impl PASS head=a971c07480ee0c6b1b1e37239c81a4e7c0a8b94f
Claimed parse impact
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
New Features
Bug Fixes
Tests