fix(engine): label flexible mana lands with the color they will produce (#6944) - #6953
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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)
📝 WalkthroughWalkthroughThe engine now resolves projected mana labels through the resolver that matches each action’s selection authority. Integration tests cover flexible mana sources and verify that every projected candidate has a non-empty ChangesFlexible mana projection
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 docstrings
🧪 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.
🧹 Nitpick comments (1)
crates/engine/tests/integration/interaction_contract.rs (1)
3205-3323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the
ActivateManaSourceflexible-mana path.All seven new tests exercise
TapLandForManaonly. The refactor ininteraction.rsextracted the surface-emission logic into a sharedpush_produced_mana_surfaceshelper used by bothTapLandForManaandActivateManaSource.ActivateManaSourcekeeps its original resolver, but its surface-emission code now runs through the new shared path. Add a test for a flexible mana source activated as a non-land permanent ability (ActivateManaSource, e.g., a creature or artifact with "{T}: Add one mana of any color") to confirm the shared-helper extraction did not change its produced-mana label.Do you want me to draft this test, following the same pattern as
tap_land_for_mana_labels_each_color_of_an_any_one_color_land?As per path instructions for
crates/engine/tests/**: "Test adequacy is the highest-frequency contributor finding — scrutinize it."🤖 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/interaction_contract.rs` around lines 3205 - 3323, Add a regression test in the interaction contract tests for a non-land permanent with an activated mana ability such as "{T}: Add one mana of any color", exercising the ActivateManaSource interaction rather than TapLandForMana. Follow the existing projected-label test pattern and assert that the produced mana labels include the expected flexible color choices, verifying the shared push_produced_mana_surfaces path preserves ActivateManaSource behavior.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.
Nitpick comments:
In `@crates/engine/tests/integration/interaction_contract.rs`:
- Around line 3205-3323: Add a regression test in the interaction contract tests
for a non-land permanent with an activated mana ability such as "{T}: Add one
mana of any color", exercising the ActivateManaSource interaction rather than
TapLandForMana. Follow the existing projected-label test pattern and assert that
the produced mana labels include the expected flexible color choices, verifying
the shared push_produced_mana_surfaces path preserves ActivateManaSource
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e3a3fe6-1ebb-4dd6-ba62-dbb4a87f609a
📒 Files selected for processing (2)
crates/engine/src/game/interaction.rscrates/engine/tests/integration/interaction_contract.rs
|
Agreed on the gap — it's the residual risk called out in the PR body, and it's fair to push on it since the extraction does now carry One correction on the suggested fixture, though: a creature or artifact with
So a label test has to drive the game into a Worth noting for anyone reading this later: nonland mana sources (Llanowar Elves, Mox Amber, Birds of Paradise) surface as I'm attempting the |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
…ce (#6944) City of Brass, Reflecting Pool, Command Tower and friends rendered an unlabelled "Tap for mana" instead of showing the mana each activation would produce. `project_action_payload` handled `TapLandForMana` and `ActivateManaSource` in one arm and resolved both through `live_mana_source_option_for_selection`. But the two actions carry deliberately different selection forms: - `TapLandForMana` is minted from `ManaSourceOption::semantic_selection` -- one concrete row per producible color -- and is executed by `handle_tap_land_for_mana` via `live_land_mana_option_for_selection`. - `ActivateManaSource` is minted from `activatable_mana_source_selections`, whose `manual_selection_for_option` intentionally collapses a flexible source to `Colorless` + `DeferredColorChoice` so the ordinary mana-choice resolver asks for the color, and is executed by `activate_mana_source_selection` via `live_mana_source_option_for_selection`. That divergence is deliberate and is not the bug. The bug is that the label path resolved a planner-minted `TapLandForMana` through the *manual* authority, which can never match a flexible source -- so the lookup failed and the arm returned without pushing a surface. The old code was correct for `ActivateManaSource` and wrong for `TapLandForMana`. Split the arm so each action is labelled through the same resolver its own reducer executes, with the resolver passed to a shared `push_produced_mana_surfaces`. A future mana action variant now has to name an authority to compile, which is what the function's doc comment already claimed. The fix is variant-agnostic: `production_override_for_option` maps all eight `flexible_output` variants to `ProductionOverride::SingleColor`, so nothing is special-cased per card. Seven tests drive the real projection pipeline (`derive_viewer_interaction` over a viewer-filtered state), covering six of the eight flexible variants: City of Brass, Reflecting Pool, Exotic Orchard, Command Tower, Plaza of Heroes, Pit of Offerings, and a Resonating Lute grant. All seven were confirmed red at base by restoring the old resolver and re-running -- 7 failures, no collateral -- reproducing the report verbatim, including mixed rows where a non-flexible sibling ability keeps its label while the flexible one goes blank. Not covered: `AnyCombination` has no bare-{T} land printing (every printing gates it behind a Composite/PaySpeed cost, so it needs a funded pool) and `AnyCombinationOfObjectColors` is unreachable for its only current printing, as already documented at casting_costs.rs. Both share the fixed code path.
The seven tests added with the fix all drive `TapLandForMana`. The `ActivateManaSource` arm keeps its original resolver and is behaviourally unchanged, but its surface emission now runs through the extracted `push_produced_mana_surfaces`, and nothing covered that. Reaching it is not obvious: the reducer accepts `ActivateManaSource` under `WaitingFor::Priority` (`engine.rs`), but `direct_choice_projection` only constructs those actions in its `WaitingFor::ManaSourceSelection` arm, so a label test has to drive the game into a mana-source-selection window rather than activate at priority. A sacrificial mana source gets there. Covers a fixed and a flexible source through the same window, so the shared helper is pinned for both arms rather than only the one the fix changed. Raised by CodeRabbit on #6949's sibling PR.
ebe129e to
8ee25c7
Compare
1 participant
Fixes #6944.
The bug
City of Brass, Reflecting Pool, Command Tower and friends rendered an unlabelled "Tap for mana" instead of showing the mana each activation would produce.
project_action_payloadhandledTapLandForManaandActivateManaSourcein a single arm and resolved both throughlive_mana_source_option_for_selection. But the two actions carry deliberately different selection forms:TapLandForManaManaSourceOption::semantic_selection— one concrete row per producible colorhandle_tap_land_for_mana→live_land_mana_option_for_selectionActivateManaSourceactivatable_mana_source_selections→manual_selection_for_option, which intentionally collapses a flexible source toColorless+DeferredColorChoiceso the ordinary mana-choice resolver asks for the coloractivate_mana_source_selection→live_mana_source_option_for_selectionThat divergence is deliberate and is not the bug —
mana_sources.rsdocuments it: "The planner emits one concrete row per color, but a manual activation must retain the source capability and let the normal mana-choice resolver ask for its color."The bug is that the label path resolved a planner-minted
TapLandForManathrough the manual authority, which can never match a flexible source — so the lookup failed and the arm returned without pushing a surface. The old code was correct forActivateManaSourceand wrong forTapLandForMana. Execution was already right; only labelling called the wrong one of the two resolvers.The fix
Split the arm so each action is labelled through the same resolver its own reducer executes, with the resolver passed to a shared
push_produced_mana_surfaces. A future mana action variant now has to name an authority in order to compile — which is what the function's doc comment already claimed.The fix is variant-agnostic:
production_override_for_optionmaps all eightflexible_outputvariants toProductionOverride::SingleColor(option.mana_type)andlive_mana_output_unitsrepeats itcounttimes. Nothing is special-cased per card.Coverage — all 8 flexible variants enumerated
AnyOneColorcount: 2)AnyTypeProduceableByOpponentLandColorsAnyInCommandersColorIdentityAnyOneColorAmongPermanentsChoiceAmongExiledColorsAnyCombination{T}land printing exists; every printing (Cascading Cataracts, Calciform Pools, Baxter Building, Chicago Loop) gates it behind aComposite/PaySpeedcost, so reaching it needs a funded pool. Covered only by the shared code path.AnyCombinationOfObjectColorscasting_costs.rs:11369-11373.Six of eight tested, one stated gap, one unreachable with code evidence. The shared-code-path argument covers the last two, but that is an argument rather than a measurement and is not counted as coverage.
Discrimination — observed, not asserted
The old resolver was restored and the full suite re-run: exactly 7 failures, all of them the new tests, zero collateral (22,800 passed / 7 failed at base; 22,807 passed with the fix). Base output reproduces the report verbatim:
The mixed rows are the sharpest evidence — on the same land the non-flexible sibling ability keeps its label while the flexible one goes blank, which is exactly the reported symptom. All seven drive the real projection pipeline (
derive_viewer_interactionover a viewer-filtered state); none is an AST-shape test.Notes for review
project_action_payloadprojects a label; it implements no rule, and CLAUDE.md exempts plumbing. An earlier draft cited CR 605.3a — that rule is about when a mana ability may be activated, which does not support a claim about selection representation, so it was removed rather than replaced with another plausible number.returns were kept deliberately. Enumeration runs on the unfiltered state while labelling re-enumerates on the viewer-filtered one, so a legitimate filtering-induced divergence must not panic in a shared multiplayer path. Failing closed mirrors the reducer's own refusal to activate a stale selection. The enforcement added instead is structural (the split arm) plus the seven tests. Atracing::debug!on theErris available if wanted.ActivateManaSourceis the arm that did not change behaviourally, and it has no new test — reaching it needs anAutoExceptSacrificialManapayment window. Its correctness rests on the call-graph trace above, not on observation. Flagging this as the main residual risk.Out of scope, worth follow-ups
ActivateAbility, whose arm pushes onlyability_indexand noProducedMana. Wider than Flexible mana lands render as unlabeled "Tap for mana" — label projection uses the wrong selection authority #6944 and needs its own design decision.ActivateManaSourcelabelling of a flexible source takesmatches.next(), arbitrarily the first color. Pre-existing and untouched here, but arguably wrong for a deferred-choice surface.GameScenario::build()runs no layer pass (the grant test needed an explicitlayers::evaluate_layers), and a grant suppresses a basic land's subtype-inferred mana row becauseland_mana_optionsskips that fallback once any explicitEffect::Manaexists. The second may be a real bug.Summary by CodeRabbit
Bug Fixes
Tests