fix(engine): let Waterbend cost affordability check consider tap-to-help

[tryeverything24]

Fixes #4966.

Waterbender Ascension: "Waterbend {4}: Target creature can't be blocked this turn."

Root cause & fix

AbilityCost::Waterbend's is_payable affordability pre-check (consulted by casting.rs before an activated ability is even offered as a legal action) delegated to can_pay_cost_after_auto_tap, which only considers real mana-producing sources (lands, mana rocks) and has no notion of Waterbend's own tap-artifacts-or-creatures-to-help mechanic (CR 601.2b, the entire point of the keyword). A player with zero floating/land mana for the generic leg but plenty of untapped eligible creatures to tap was therefore told the ability wasn't payable at all — the ability could never be activated, matching the reported "remains blockable" symptom.

Fixed by delegating to can_feasibly_pay_mana_cost_with_tap_payment_mode (the existing, already-correct helper used by the spell-cast "additional cost: you may waterbend N" path), which falls back to the plain auto-tap check first, so pool/land-funded payment is unaffected.

Testing

Two new end-to-end tests: one pays the Waterbend {4} cost from a pre-funded mana pool (baseline), one pays it via real TapForConvoke tap-to-help with zero floating mana (the actual previously-broken path). Both confirm the targeted creature becomes unblockable.

Verified locally: fmt clean, clippy clean (-D warnings), 3261 integration tests pass (3259 pre-existing + 2 new), 0 regressions.

Summary by CodeRabbit

• Bug Fixes

  • Corrected Waterbend (Waterbender Ascension) activation cost handling to use activation-appropriate tap-to-help affordability rules.
  • Fixed resolution so unblockability applies when the ability is properly activated via available mana or helper taps.
  • Improved mana restriction enforcement: activations that rely on spell-only (or missing/mismatched) tagged mana are now correctly rejected up front.

• Tests

  • Added an integration regression suite for Waterbender Ascension covering end-to-end unblockability, tap-for-convoke flow, and mana restriction gating.

[gemini-code-assist]

Code Review

This pull request resolves issue #4966 by correcting the affordability pre-check for the AbilityCost::Waterbend activated ability. Instead of using the standard auto-tap check which only considers mana-producing sources, it now utilizes can_feasibly_pay_mana_cost_with_tap_payment_mode to correctly account for the tap-to-help mechanic of Waterbend. Comprehensive integration tests have been added to verify this behavior under both pool-funded and tap-to-help payment scenarios. No review comments were provided, and the changes conform to the repository's architectural guidelines, so there is no additional feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[matthewevans]

Request changes — the activation-path regression is strong, but required current-head parse-diff evidence is absent.

🔴 Blocker

crates/engine/src/game/cost_payability.rs:637 changes engine source, but this PR has no <!-- coverage-parse-diff --> sticky comment for head d2cdc6b499e8c919109bf1360b90761e86491566. The Card data job completed successfully at 13:40 UTC, so a current card-level artifact is required to establish that the payability helper change did not alter parser/card coverage unexpectedly. The review protocol requires this evidence for engine/parser-surface PRs.

Regenerate or restore the current-head parse-diff artifact, then request re-review. No source rewrite is requested by this finding.

✅ Clean

The change uses the existing can_feasibly_pay_mana_cost_with_tap_payment_mode(..., ConvokeMode::Waterbend) authority, and issue_4966_waterbenders_ascension.rs drives the real activation, tap-to-help payment, target selection, resolution, and combat-legality path. CR 701.67a and CR 602.2b verify that a Waterbend activation may substitute controlled artifacts/creatures for its generic mana.

Recommendation: supply the current-head parse-diff, then request re-review.

[matthewevans]

Changes requested — the Waterbend helper path is correct to inspect, but this pre-check uses the wrong payment context for an activated ability.

🔴 Blocker

[MED] Waterbend activated-ability affordability is evaluated as a spell payment. Evidence: crates/engine/src/game/cost_payability.rs:637-644 calls can_feasibly_pay_mana_cost_with_tap_payment_mode, which constructs PaymentContext::Spell in crates/engine/src/game/casting.rs:13040-13074; the actual activated-ability payment builds PaymentContext::Activation in crates/engine/src/game/casting.rs:13700-13715 and 13833-13845. Why it matters: spell-only and activation-only mana restrictions can make the legal-action gate disagree with the later payment path, respectively offering an activation that cannot be paid or suppressing one that can. Suggested fix: make the tap-payment feasibility path accept an activation context (including the ability tag), or add an activation-specific sibling and thread the required source/type/tag data from the ability gate.

Please add discriminating runtime regressions for both an activation-only mana source that must make Waterbend available and a spell-only mana source that must not. The existing zero-floating-mana TapForConvoke scenario is useful coverage for the reported helper-counting bug, but it does not exercise this restriction-context boundary.

✅ Clean

The TapForConvoke test is registered and drives the real activation, payment, targeting, and resolution sequence. The review above does not challenge that coverage; it identifies a sibling payment class that the shared affordability gate also owns.

Recommendation: request changes, then re-review the new head before approval.

[github-actions]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[tryeverything24]

Addressed the payment-context blocker.

The feasibility helper is now split into two context wrappers over a shared core: the existing spell-context entry point (kept for the spell-cast "additional cost: you may waterbend N" path) and a new can_feasibly_pay_activation_mana_cost_with_tap_payment_mode that builds PaymentContext::Activation from the source permanent's current types — mirroring the real activation payment path's context construction — with an ability_tag parameter threaded for tag-scoped restrictions (OnlyForTaggedActivation). AbilityCost::Waterbend's is_payable arm uses the activation sibling; ability_tag is None there because a Waterbend-costed printed ability is not a tagged keyword ability and is_payable's CR 601.2b gate has no ability index to look one up — the exact tag-scoped gate runs at payment time (CR 601.2g), matching can_pay_ability_cost_after_auto_tap's documented preview behavior.

Added both requested discriminating runtime regressions: (a) activation-only restricted mana (OnlyForActivation) as the sole funding must make Waterbend available and resolve through to the unblockable effect; (b) spell-only mana (OnlyForSpell) as the sole funding must reject ActivateAbility at the affordability gate. Each fails under the previous spell-context probe.

This push also regenerates the current-head parse-diff artifact that the first review noted was missing.

Verified locally in an isolated target dir: fmt clean, workspace clippy -D warnings clean, engine lib 17515 passed, engine integration 3752 passed. Rebased onto current main.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0d377ea3-24ce-444c-ab46-e894c44281d3

📥 Commits

Reviewing files that changed from the base of the PR and between ff2651d and 7140e03.

📒 Files selected for processing (4)

• crates/engine/src/game/casting.rs
• crates/engine/src/game/cost_payability.rs
• crates/engine/tests/integration/issue_4966_waterbenders_ascension.rs
• crates/engine/tests/integration/main.rs

🚧 Files skipped from review as they are similar to previous changes (3)

• crates/engine/tests/integration/main.rs
• crates/engine/tests/integration/issue_4966_waterbenders_ascension.rs
• crates/engine/src/game/casting.rs

---

📝 Walkthrough

Walkthrough

Changes

Waterbend activation affordability

Layer / File(s)	Summary
Activation-context feasibility core crates/engine/src/game/casting.rs	Adds activation-specific tap-payment feasibility, handles X costs, threads ability tags, and shares context-aware affordability logic with spell checks.
Waterbend payability wiring crates/engine/src/game/cost_payability.rs	Adds tag-aware activation payability and routes Waterbend and nested costs through activation-context checks.
Waterbend regression coverage crates/engine/tests/integration/issue_4966_waterbenders_ascension.rs, crates/engine/tests/integration/main.rs	Adds coverage for pool payment, tap-to-help payment, restricted mana, unblockability, and test registration.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Player
  participant AbilityCost
  participant ActivationFeasibility
  participant ManaPayment
  participant WaterbenderAscension
  Player->>AbilityCost: activate Waterbend ability
  AbilityCost->>ActivationFeasibility: check tagged activation affordability
  ActivationFeasibility->>ManaPayment: probe pool and tap-to-help payment
  ManaPayment-->>ActivationFeasibility: return payment feasibility
  ActivationFeasibility-->>AbilityCost: accept or reject activation
  AbilityCost->>WaterbenderAscension: resolve targeted ability
  WaterbenderAscension-->>Player: targeted creature cannot be blocked

Loading

Suggested labels: bug

Suggested reviewers: matthewevans, lgray, dripsmvcp

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	It clearly summarizes the main fix: Waterbend affordability now accounts for tap-to-help payment.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

crates/engine/src/game/casting.rs (1)

13745-13796: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor duplication between the spell- and activation-mode wrappers.

can_feasibly_pay_activation_mana_cost_with_tap_payment_mode (13759-13796) and can_feasibly_pay_mana_cost_with_tap_payment_mode (13712-13743) share an identical skeleton — X-cost recursion, state.clone() + flush_layers, then delegate to the shared core — differing only in how the PaymentContext is built. Since context construction differs by kind (spell metadata vs. activation source types + tag), consider extracting the shared skeleton into a private helper that takes a context-builder closure, leaving only the two context constructions distinct.

🤖 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/casting.rs` around lines 13745 - 13796, Extract the
duplicated X-cost recursion, cloned-state layer flushing, and shared payment
delegation from can_feasibly_pay_activation_mana_cost_with_tap_payment_mode and
can_feasibly_pay_mana_cost_with_tap_payment_mode into a private helper accepting
a context-builder closure. Keep spell context construction and
activation_source_types-based PaymentContext::Activation construction in their
respective wrappers, and have both wrappers delegate through the shared helper
without changing behavior.

🤖 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/src/game/casting.rs`:
- Around line 13745-13796: Extract the duplicated X-cost recursion, cloned-state
layer flushing, and shared payment delegation from
can_feasibly_pay_activation_mana_cost_with_tap_payment_mode and
can_feasibly_pay_mana_cost_with_tap_payment_mode into a private helper accepting
a context-builder closure. Keep spell context construction and
activation_source_types-based PaymentContext::Activation construction in their
respective wrappers, and have both wrappers delegate through the shared helper
without changing behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0a9346c2-e1f8-4f08-b810-dbc1726ef005

📥 Commits

Reviewing files that changed from the base of the PR and between df2ab2d and 28ead6e.

📒 Files selected for processing (4)

• crates/engine/src/game/casting.rs
• crates/engine/src/game/cost_payability.rs
• crates/engine/tests/integration/issue_4966_waterbenders_ascension.rs
• crates/engine/tests/integration/main.rs

[matthewevans]

Blocked — Waterbend’s early affordability check loses the activation tag.

🔴 Blocker

crates/engine/src/game/cost_payability.rs:653-660 always passes None for ability_tag when evaluating AbilityCost::Waterbend, although the actual activation gate receives ability_def.ability_tag in crates/engine/src/game/casting.rs:16718-16724. OnlyForTaggedActivation mana is consequently excluded by the early affordance check even though PaymentContext::Activation would allow it during payment.

Thread the ability tag through the activation-payability route and add a PowerUp-tagged Waterbend regression. The added tests cover generic activation and spell contexts, but not the tag-scoped sibling.

✅ Clean

The prior parse-artifact blocker is resolved: the current sticky reports no card-parse changes.

Recommendation: request changes for tag propagation and the tagged-Waterbend regression.

[tryeverything24]

Addressed the blocking finding: the activation tag is now threaded through the affordability route, with a PowerUp-tagged Waterbend regression.

Root of the blocker
activation_cost_passes_early_affordability_gate -> AbilityCost::is_payable dropped the tag before it reached the Waterbend arm, so the early gate probed with ability_tag = None while the real payment step (casting.rs) receives ability_def.ability_tag. ManaRestriction::OnlyForTaggedActivation then hid tag-scoped mana from the gate, suppressing an activation the payment pipeline would have allowed.

Fix (tag threaded, no duplicated traversal)

• Added AbilityCost::is_payable_for_activation(state, player, source, ability_tag) and made the existing is_payable a thin None-tag delegate to it — single body, so the Composite / OneOf recursion isn't forked into a tagged and an untagged copy. Both recursive arms forward ability_tag.
• The Waterbend arm now passes ability_tag (not None) into can_feasibly_pay_activation_mana_cost_with_tap_payment_mode, which already builds PaymentContext::Activation { ability_tag, .. }.
• activation_cost_passes_early_affordability_gate's non-OneOf branch calls is_payable_for_activation(..., ability_tag), so the gate and can_pay_ability_cost_now (the OneOf branch, already tag-aware) now agree.

Regression (the tag-scoped sibling you asked for)
waterbend_payability_sees_tag_scoped_activation_mana (in game::cost_payability): four OnlyForTaggedActivation(PowerUp)-restricted mana, no other funding.

• Some(PowerUp) -> payable; Some(Equip) -> not; None -> not; tag-agnostic is_payable -> not.
• Stash-verified RED: reverting only the Waterbend arm to None fails the first assertion ("power-up-restricted mana must fund a Power-up-tagged Waterbend activation"); GREEN with the tag threaded.
  The pre-existing generic-activation and spell-context integration tests are unchanged and still pass, so all three CR 106.6 axes (activation-only, spell-only, tag-scoped) are now covered. I kept your fix(PR-6097): keep test imports module scoped cleanup (top-level ManaRestriction import).

Full local parity (isolated CARGO_TARGET_DIR, rebased onto c754a087a)

• cargo fmt --all -- --check: clean
• cargo clippy --workspace --all-targets --features engine/proptest -- -D warnings: clean
• cargo test -p engine --lib: 17591 passed / 0 failed
• cargo test -p engine --test integration: 3873 passed / 0 failed

[matthewevans]

Blocked — the activation tag now reaches one of the two activation affordability gates; the legal-action gate still drops it.

The previously-blocking finding is genuinely addressed on the submit path, and the shape of the fix is right (single parameterized body, thin None delegate, no forked traversal). What remains is the same defect on the sibling gate — the one that decides whether the ability is offered at all, which is the actual issue #4966 symptom class.

🔴 Blocker

[MED] costs::can_pay's activation arm discards the ability_tag it is handed, so can_activate_ability_now still probes AbilityCost::Waterbend untagged.

Evidence — crates/engine/src/game/costs.rs:1629-1631 (unchanged by this PR):

match scope {
    PaymentScope::Activation { .. } => {
        if !cost.is_payable(state, payer, source_id) {
            return false;
        }

The { .. } pattern throws away a field the enum carries specifically for this concern — crates/engine/src/game/costs.rs:201-209:

pub(crate) enum PaymentScope<'a> {
    Activation {
        excluded_sources: &'a HashSet<ObjectId>,
        /// CR 106.6: Keyword tag of the activated ability whose cost is being
        /// paid. Threaded into `PaymentContext::Activation` so tag-scoped mana
        /// spend restrictions (Quinjet → power-up) gate eligible mana. …
        ability_tag: Option<crate::types::ability::AbilityTag>,
    },

The two activation gates therefore still disagree:

gate	route	tag
handle_activate_ability (casting.rs:16765)	activation_cost_passes_early_affordability_gate → is_payable_for_activation (casting.rs:15775)	✅ threaded by this PR
can_activate_ability_now (casting.rs:16376)	can_pay_ability_cost_now → costs::can_pay → is_payable (costs.rs:1630)	❌ dropped

For a bare Waterbend cost, can_pay returns is_payable's verdict verbatim (the matches!(cost, AbilityCost::Waterbend { .. }) dry-run skip at costs.rs:1653 sits after that early return), and ManaRestriction::OnlyForTaggedActivation gates strictly on tag equality — crates/engine/src/types/mana.rs:1089-1091:

ManaRestriction::OnlyForTaggedActivation(required_tag) => {
    ability_tag == Some(*required_tag)
}

So the exact scenario your new unit test pins — tag-scoped mana as the sole funding — still yields "not activatable" at the offer gate: can_activate_ability_now returns false, the action is never generated for the UI or the AI, and the player never gets to the submit path you fixed. That is the same false-unactivatable verdict #4966 reports, one gate upstream. The OneOf branch at casting.rs:15768 inherits it too, so the PR comment's "the OneOf branch, already tag-aware" doesn't hold: can_pay_ability_cost_now threads the tag into PaymentScope for the dry run, but the is_payable pre-gate in front of it discards it.

The same asymmetry falsifies the invariant your new doc comment asserts at crates/engine/src/game/cost_payability.rs:258-262 — "Callers that KNOW the ability whose cost this is (the activation pipeline) must use that method instead" — while the activation pipeline's other authority does not.

Fix — one line, plus a discriminator:

PaymentScope::Activation { ability_tag, .. } => {
    if !cost.is_payable_for_activation(state, payer, source_id, *ability_tag) {
        return false;
    }

[MED] The tag regression is unit-scoped, which is why the gap above survived. Evidence: crates/engine/src/game/cost_payability.rs:1545 calls cost.is_payable_for_activation(...) directly. Production reaches Waterbend affordability through GameAction::ActivateAbility and through legal-action generation; a test at the AbilityCost layer cannot observe a caller that drops the argument. Add a runtime sibling in issue_4966_waterbenders_ascension.rs in the shape of your existing waterbend_activation_only_mana_makes_ability_available — tag-scoped mana as sole funding on a tagged ability, asserting the activation is offered (legal-action / can_activate_ability_now reachable) and not merely accepted on submit. That test fails today.

🟡 Non-blocking

[LOW] The test module header states the Oracle text is "verified against data/mtgish-cards.json, card 'Waterbender Ascension'" (crates/engine/tests/integration/issue_4966_waterbenders_ascension.rs:6-8). That file stores typed rule trees, not prose — the entry is {"_Cost":"Waterbend","args":[{"_ManaSymbol":"ManaCostGeneric","args":4}]} with a CreatePermanentRuleEffectUntil / CantBeBlocked / UntilEndOfTurn action, so the quoted string is a faithful reconstruction rather than a quotation from that file. Cite the prose source you actually checked.

✅ Clean

• Right seam and reuse. can_feasibly_pay_activation_mana_cost_with_tap_payment_mode (casting.rs:13778) builds PaymentContext::Activation from the existing activation_source_types helper (casting.rs:14965), mirroring the real payment path's construction at casting.rs:13994 rather than re-deriving type strings. Both CR 106.6 directions are pinned by runtime tests that drive ActivateAbility end-to-end (waterbend_activation_only_mana_makes_ability_available, waterbend_spell_only_mana_does_not_make_ability_available), and the negatives are non-vacuous — the first test in the file activates the identical construction successfully, so the rejection is proven to come from the affordability gate.
• Parameterized, not proliferated. is_payable_for_activation (cost_payability.rs:280) holds the single match body and is_payable (cost_payability.rs:263) is a None delegate; the Composite (:645) and OneOf (:654) arms forward the tag. No forked traversal, no new enum sibling.
• CR annotations grep-verified against docs/MagicCompRules.txt: 106.6 (line 425), 118.3 (972), 118.12a (1035), 601.2b (2459), 601.2g (2470), 601.2h (2472), and 701.67a (3851) — "'Waterbend [cost]' means 'Pay [cost]. For each generic mana in that cost, you may tap an untapped artifact or creature you control rather than pay that mana.'", which is exactly what the Waterbend arm's comment claims. All accurate.
• Test registered. mod issue_4966_waterbenders_ascension; is added to crates/engine/tests/integration/main.rs — not an inert false green.
• Parse-diff evidence is current-head. The <!-- coverage-parse-diff --> sticky was refreshed at 2026-07-23T19:46:01Z (after the 7140e03a push) and reports "No card-parse changes detected", matching an engine-only diff. All checks green on 7140e03a.
• CodeRabbit's wrapper-duplication nitpick — refuted. The shared core is already extracted (feasibly_payable_with_tap_payment_mode_in_context, casting.rs:13822); what remains in each wrapper is X-cost recursion plus clone + flush_layers. A context-builder closure cannot cleanly own that, since PaymentContext::Activation borrows the source_types/source_subtypes locals the closure would have to produce. Leave it.

Recommendation: thread ability_tag through costs.rs:1630 (PaymentScope::Activation { ability_tag, .. } → is_payable_for_activation) and add the runtime discriminator that asserts a tagged Waterbend ability funded only by OnlyForTaggedActivation mana is offered as a legal action. Everything else on this head is ready. If you'd rather I land that one-liner plus test as a maintainer fixup on your branch, say the word.