Fix Coalition Relic zero-counter mana prompt - #7218
Conversation
|
Warning Review limit reached
Next review available in: 21 seconds 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 (2)
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMana color prompts now use separate abilities for color discovery and production-count resolution. Zero-production prompts are suppressed. Integration tests cover Coalition Relic and separate recipient and count-source targets. ChangesMana prompt resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
![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
🤖 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/effects/mana.rs`:
- Line 201: Update the color-discovery argument at the `Some(count_ability)`
call site to pass the original unscoped ability, while retaining `count_ability`
exclusively for quantity resolution. Add a regression covering
`ManaTargetRole::Both` with `ControllerRef::TargetPlayer` or `TargetOpponent`,
using distinct recipient and count-source players to verify colors come from the
recipient.
In `@crates/engine/src/game/mana_abilities.rs`:
- Around line 946-951: Add a `CR 106.5` comment annotation to the
`produces_mana` prompt logic, preferably at `mana_choice_prompt` or immediately
above the `AnyOneColor`, `AnyOneColorAmongPermanents`, and
`AnyTypeProduceableBy` gates, while leaving the existing resolver and behavior
unchanged.
In `@crates/engine/tests/integration/coalition_relic_integration.rs`:
- Around line 238-245: Replace the manually constructed
ResolvedAbility/resolve_ability_chain setup with the normal Coalition Relic
production flow through apply, WaitingFor, GameAction, or the scenario runner.
Add a paired positive-counter case that reaches WaitingFor::ChooseManaColor,
while retaining the zero-counter assertion that the state remains at
WaitingFor::Priority.
🪄 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: 988eb3a8-fae3-4918-acda-4dbf8223d314
📒 Files selected for processing (3)
crates/engine/src/game/effects/mana.rscrates/engine/src/game/mana_abilities.rscrates/engine/tests/integration/coalition_relic_integration.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the zero-count prompt gate is on the right authority, but it now conflates two independently scoped target contexts.
🔴 Blocker
crates/engine/src/game/effects/mana.rs:189-202 deliberately creates count_ability by retaining only the count-source player so QuantityRef::TargetZoneCardCount and LifeTotal { player: Target } resolve the right quantity. Passing that view as the sole ability argument to mana_choice_prompt changes the context used at crates/engine/src/game/mana_abilities.rs:978-986 for AnyOneColorAmongPermanents: distinct_colors_among_permanents constructs FilterContext::from_ability, so a ManaTargetRole::Both production whose color filter is relative to the recipient (TargetPlayer/TargetOpponent) derives its color options from the count source instead. AnyCombinationOfObjectColors likewise reads its target from that same argument at mana_abilities.rs:1096-1104.
The prompt needs two explicit contexts: the original resolving ability for color/target-scope discovery, and the count-scoped view only for resolve_mana_types_for_ability. Thread them separately through mana_choice_prompt (the activated-mana-ability caller can use its one context for both), then add a regression with distinct recipient and count-source players proving the offered colors follow the recipient while the produced count follows the count source.
✅ Confirmed
The new zero-counter assertion in coalition_relic_integration.rs:217-248 is discriminating for this regression: resolve_ability_chain reaches the production resolver, and the pre-fix behavior leaves WaitingFor::ChooseManaColor; it need not reconstruct unrelated optional-trigger dispatch to establish the prompt boundary. The existing CR 106.5 annotations at the affected prompt branches correctly describe the no-production behavior.
Recommendation: split the prompt's color-discovery and count-resolution contexts, cover the Both case, then request re-review on the new head.
|
Correction to my formal review: the reference to The requested change remains: |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
|
Current-head review is complete; holding only for the current CI evidence. The prior requested change is resolved at d099d43: effects/mana.rs:197-203 now passes the original ability for dynamic color discovery and the count-scoped view only for quantity resolution; mana_abilities.rs:984-1005 consumes those contexts separately. mana_target_recipient_and_count_source.rs:204-321 drives distinct recipient/count-source targets through casting, verifies the offered colors come from the recipient, the amount comes from the count source, and mana is deposited only to the recipient. The required Card data (generate, validate, coverage) job is still in progress, so the only parse-diff sticky is bound to superseded a50d51b. I will re-check the fresh current-head parse diff and required checks on the next sweep, then approve/enqueue if they are clean. The bug label is applied. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — zero production is still promptable for one count-bearing mana variant.
[MED] ManaProduction::ChosenColor { fixed_alternative: Some(_), count } does not consult count_ability before returning ManaChoicePrompt::SingleColor. Evidence: crates/engine/src/game/mana_abilities.rs:1018-1049; ChosenColor carries a general QuantityExpr count at crates/engine/src/types/ability.rs:2086-2100, and the parser threads that count through the fixed-alternative form at crates/engine/src/parser/oracle_effect/mana.rs:1435-1445. Why it matters: when such a count resolves to zero (the same legal dynamic-count condition Coalition Relic exercises), the engine still asks a player to select a color even though resolution produces vec![color; 0] in crates/engine/src/game/effects/mana.rs:689-711. CR 106.5 confirms the required result: an ability that would produce mana of an undefined type produces no mana; a zero-unit production must not leave a useless choice pending. Suggested fix: route the ChosenColor fixed-alternative prompt through the same resolved-production gate used by the other count-bearing prompt branches, and add a zero-count regression that reaches the prompt authority.
The current-head CI and current-head parse-diff sticky are otherwise green/current, and the earlier recipient-vs-count-source target-context blocker is resolved at d099d43.
|
Current-head review remains complete; holding only for current CI and parse-diff evidence. Maintainer update The new head's required CI is running, including Card data (generate, validate, coverage), and no |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on 6798438f974f06f31b12aecd4facafb89a7a323d.
Current-head review confirms the prompt authority keeps the original ability for recipient-relative color discovery and uses the count-scoped view only for quantity resolution. The added role-split runtime regression proves colors come from the recipient, quantity from the count source, and deposit reaches only the recipient; the zero-count and fixed-alternative gates suppress no-op prompts. Required CI is green and the clean maintainer main merge was the one permitted parse-baseline refresh.
2 participants
Summary
Coalition Relic no longer prompts for a mana color when removing zero charge counters. The shared mana prompt path now checks resolved production before offering a color choice, while positive production and existing count-scoped resolution remain intact.
Closes #7164
Files changed
Track
Developer
LLM
Model: GitHub Copilot (via GitHub Copilot; canonical id not exposed)
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 106.5 — existing mana resolver/prompt authority; no new CR annotation was added.
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— passed.git diff --check— passed.cargo clippy -p phase-engine --all-targets -- -D warnings— passed.cargo test -p phase-engine --test integration coalition_relic_integration::coalition_relic— 2 passed, 0 failed.cargo test -p phase-engine --quiet— 18,832 + 21 + 9 + 4,798 passed, 0 failed; ignored tests: 6 + 0 + 0 + 2 + 7../scripts/check-parser-combinators.sh— passed with Gate G and Gate A.Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=a50d51ba496eb366c6a5a53c6908d50eadf329d0 base=97591656218103d8e8c7315725b24cfe64645dd4
Anchored on
AnyCombinationbranch resolves the production count before creating a combination prompt.AnyCombinationOfObjectColorsbranch resolves the production count before creating a dynamic combination prompt.Final review-impl
Final review-impl PASS head=a50d51ba496eb366c6a5a53c6908d50eadf329d0
Claimed parse impact
None.
Scope Expansion
None. The sibling guards remain within the same shared mana prompt authority and enforce the same zero-production invariant for existing dynamic production variants.
Validation Failures
cargo semantic-auditbuilt successfully but could not run because this checkout lacksclient/public/card-data.json; the command reported:card-data.json not found at client/public/card-data.json. CI or a generated card-data checkout is required for that audit.Workspace-wide
cargo clippy --all-targets -- -D warningswas blocked while compiling unrelatedserver-corebecauseopenssl-sys v0.9.116could not find an OpenSSL installation. The touched package passed the scoped Clippy command above.cargo coverage— built successfully but could not loaddata/card-data.json(No such file or directory); it returned an empty coverage report. CI or generated card data is required for this report.CI Failures
None known.
Summary by CodeRabbit
Bug Fixes
Tests