fix(engine): compose convoke and delve payments - #7307
Conversation
|
Warning Review limit reached
Next review available in: 2 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 (6)
📝 WalkthroughWalkthroughDelve is detected independently from the selected tap-payment mode. Eligible graveyard cards now combine with convoke, improvise, or waterbend payments. Candidate generation, affordability checks, payment execution, scenario commits, and Hogaak integration coverage were updated. ChangesCombined Delve and tap payment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PendingCast
participant PaymentActions
participant GameEngine
participant Graveyard
PendingCast->>PaymentActions: provide Delve and tap-payment capabilities
PaymentActions->>Graveyard: enumerate eligible Delve objects
Graveyard-->>PaymentActions: return colorless Delve actions
PendingCast->>GameEngine: submit combined payment actions
GameEngine->>Graveyard: exile selected Delve objects
GameEngine-->>PendingCast: resume cast payment or resolve spell
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/ai_support/candidates.rs (1)
4874-4928: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the wildcard with explicit
ConvokeModearms.
ConvokeMode::Delveis handled before this battlefield loop and must not tap battlefield objects. Keep that separate path, but replace_ => {}with explicit no-op arms forDelveand each ineligible current mode. This preserves behavior and lets the compiler detect future variants.🤖 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/ai_support/candidates.rs` around lines 4874 - 4928, Replace the wildcard arm in the mode match within the battlefield candidate loop with explicit no-op arms for ConvokeMode::Delve and each currently supported mode when its eligibility check fails. Keep Delve handling in its existing separate path, preserve all eligible Waterbend, Improvise, and Convoke behavior, and make the match exhaustive so future ConvokeMode variants trigger compiler updates.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/src/ai_support/candidates.rs`:
- Around line 4818-4845: Add a verified “CR 702.66a” annotation describing
exiling a graveyard card to pay one generic mana to both independent Delve logic
sites: annotate the detection and payment-action emission around `has_delve` and
`GameAction::TapForConvoke` in `crates/engine/src/ai_support/candidates.rs`
lines 4818-4845, and the corresponding direct-action detection and emission in
`crates/engine/src/game/interaction.rs` lines 1842-1954. Ensure each site has
its own annotation.
In `@crates/engine/src/game/casting.rs`:
- Around line 14817-14825: Propagate the pending cast’s fused context through
all Delve affordability calculations: in
crates/engine/src/game/casting.rs#L14817-L14825, pass the fused-cast state to
can_pay_with_spell_tap_payments and spell_has_delve_payment_for; in
crates/engine/src/game/casting.rs#L15252-L15260, derive the pending spell’s
CastingVariant::Fuse state before the affordability probe; and in
crates/engine/src/game/casting_costs.rs#L11829-L11831, derive the matching
pending cast’s fused state before calculating delve_capacity.
In `@crates/engine/tests/integration/hogaak_cant_spend_mana_1095.rs`:
- Around line 106-172: Expand coverage beyond
hogaak_combines_convoke_and_delve_from_graveyard by adding production-pipeline
regressions for Delve combined with Improvise and Waterbend. Exercise both
mana_payment_actions and mana_payment_direct_actions, asserting each exposes
only legal Delve selections and tap-payment actions, including candidate
legality and combined-mode behavior rather than relying solely on direct
SpellCast resolution.
---
Outside diff comments:
In `@crates/engine/src/ai_support/candidates.rs`:
- Around line 4874-4928: Replace the wildcard arm in the mode match within the
battlefield candidate loop with explicit no-op arms for ConvokeMode::Delve and
each currently supported mode when its eligibility check fails. Keep Delve
handling in its existing separate path, preserve all eligible Waterbend,
Improvise, and Convoke behavior, and make the match exhaustive so future
ConvokeMode variants trigger compiler updates.
🪄 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: 75637370-aec5-4aab-9296-d1ded39b11dd
📒 Files selected for processing (7)
crates/engine/src/ai_support/candidates.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/engine.rscrates/engine/src/game/interaction.rscrates/engine/src/game/scenario.rscrates/engine/tests/integration/hogaak_cant_spend_mana_1095.rs
| let has_delve = state.pending_cast.as_ref().is_some_and(|pending| { | ||
| crate::game::casting::spell_has_delve_payment_for( | ||
| state, | ||
| player, | ||
| pending.object_id, | ||
| pending.casting_variant == CastingVariant::Fuse, | ||
| ) | ||
| }); | ||
| // Always include PassPriority to finalize payment | ||
| actions.push(candidate( | ||
| GameAction::PassPriority, | ||
| TacticalClass::Pass, | ||
| Some(player), | ||
| )); | ||
| if let Some(mode) = convoke_mode { | ||
| if has_delve { | ||
| for (&obj_id, obj) in &state.objects { | ||
| if obj.is_delve_eligible(player) { | ||
| actions.push(candidate( | ||
| GameAction::TapForConvoke { | ||
| object_id: obj_id, | ||
| mana_type: crate::types::mana::ManaType::Colorless, | ||
| }, | ||
| TacticalClass::Mana, | ||
| Some(player), | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add verified Delve CR annotations to both action generators.
Both blocks decide Delve legality and emit Delve payment actions. Neither block includes the required CR <number>: <description> annotation. Use a verified CR 702.66a annotation that describes exiling a graveyard card to pay one generic mana. (media.wizards.com)
crates/engine/src/ai_support/candidates.rs#L4818-L4845: annotate independent Delve candidate detection and emission.crates/engine/src/game/interaction.rs#L1842-L1954: annotate independent Delve direct-action detection and emission.
As per path instructions: "rules-touching code with no verified CR <number>: <description> annotation" is a finding.
📍 Affects 2 files
crates/engine/src/ai_support/candidates.rs#L4818-L4845(this comment)crates/engine/src/game/interaction.rs#L1842-L1954
🤖 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/ai_support/candidates.rs` around lines 4818 - 4845, Add a
verified “CR 702.66a” annotation describing exiling a graveyard card to pay one
generic mana to both independent Delve logic sites: annotate the detection and
payment-action emission around `has_delve` and `GameAction::TapForConvoke` in
`crates/engine/src/ai_support/candidates.rs` lines 4818-4845, and the
corresponding direct-action detection and emission in
`crates/engine/src/game/interaction.rs` lines 1842-1954. Ensure each site has
its own annotation.
Source: Path instructions
|
|
||
| /// Hogaak's real `{5}{B/G}{B/G}` cost is payable from its graveyard only when | ||
| /// Convoke supplies the two green hybrid pips and Delve exiles five graveyard | ||
| /// cards for the generic component. The payment permissions compose; no pool | ||
| /// mana is available or spent. | ||
| #[test] | ||
| fn hogaak_combines_convoke_and_delve_from_graveyard() { | ||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
| let hogaak = scenario | ||
| .add_creature_to_graveyard(P0, "Hogaak, Arisen Necropolis", 8, 8) | ||
| .from_oracle_text(HOGAAK_ORACLE) | ||
| .with_mana_cost(ManaCost::Cost { | ||
| shards: vec![ManaCostShard::BlackGreen, ManaCostShard::BlackGreen], | ||
| generic: 5, | ||
| }) | ||
| .id(); | ||
| let convoker_a = scenario.add_creature(P0, "Green Convoker A", 1, 1).id(); | ||
| let convoker_b = scenario.add_creature(P0, "Green Convoker B", 1, 1).id(); | ||
| let delve_fuel: Vec<ObjectId> = (0..5) | ||
| .map(|index| { | ||
| scenario | ||
| .add_spell_to_graveyard(P0, &format!("Delve Fuel {index}"), true) | ||
| .id() | ||
| }) | ||
| .collect(); | ||
|
|
||
| let mut runner = scenario.build(); | ||
| for convoker in [convoker_a, convoker_b] { | ||
| runner | ||
| .state_mut() | ||
| .objects | ||
| .get_mut(&convoker) | ||
| .expect("convoker exists") | ||
| .color | ||
| .push(ManaColor::Green); | ||
| } | ||
|
|
||
| let outcome = runner | ||
| .cast(hogaak) | ||
| .delve_with(&delve_fuel) | ||
| .convoke_with(&[convoker_a, convoker_b]) | ||
| .resolve(); | ||
| let state = outcome.state(); | ||
|
|
||
| assert_eq!( | ||
| state.objects[&hogaak].zone, | ||
| Zone::Battlefield, | ||
| "Hogaak must be cast from the graveyard when its real cost is fully covered" | ||
| ); | ||
| assert!( | ||
| delve_fuel | ||
| .iter() | ||
| .all(|fuel| state.objects[fuel].zone == Zone::Exile), | ||
| "Delve must exile exactly the selected graveyard cards" | ||
| ); | ||
| assert!( | ||
| [convoker_a, convoker_b] | ||
| .iter() | ||
| .all(|convoker| state.objects[convoker].tapped), | ||
| "Convoke must tap both green creatures for Hogaak's hybrid pips" | ||
| ); | ||
| assert!( | ||
| !state.objects[&hogaak].mana_spent_to_cast, | ||
| "Hogaak's restriction forbids spending ordinary mana" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add generalized composed-payment coverage.
This test covers only Hogaak with Convoke. SpellCast submits actions directly, so it does not verify mana_payment_actions or mana_payment_direct_actions. Add production-pipeline regressions for Delve with Improvise and Waterbend, and assert that both action-generation surfaces expose only legal Delve and tap-payment actions.
As per path instructions: "Test generalized convoke/delve payment behavior, including combined modes and candidate legality, not only one card scenario."
🤖 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/hogaak_cant_spend_mana_1095.rs` around lines
106 - 172, Expand coverage beyond
hogaak_combines_convoke_and_delve_from_graveyard by adding production-pipeline
regressions for Delve combined with Improvise and Waterbend. Exercise both
mana_payment_actions and mana_payment_direct_actions, asserting each exposes
only legal Delve selections and tap-payment actions, including candidate
legality and combined-mode behavior rather than relying solely on direct
SpellCast resolution.
Source: Path instructions
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
|
Current-head review follow-up for
No unaddressed current-head defect remains from these comments. |
1 participant
Summary by CodeRabbit