fix(engine): stop basic-land mana fallback from bypassing CantBeActivated - #6841
Conversation
…ated
land_mana_options() fell back to unconditional subtype-inferred mana
production whenever scan_mana_abilities() returned no options, without
checking whether that emptiness meant "no mana ability exists" or "a
real mana ability exists but was just filtered out by a legality gate."
Karn, the Great Creator correctly filtered a Liquimetal-Coating-turned-
artifact land's own {T}: Add ability out of scan_mana_abilities, but the
fallback then silently re-added it via bare subtype inference, letting
the opponent tap the blocked land for mana anyway (phase-rs#6469). Gate the
fallback on the object genuinely carrying no Effect::Mana ability at
all, so Urborg/Blood-Moon-class subtype-only production still works
while a real, currently-prohibited ability stays prohibited.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change applies activation-prohibition checks to synthesized basic-land mana abilities. It preserves explicit ability handling and adds regression tests for Karn, artifact-coated lands, blocked Forest mana, and intrinsic land restrictions. ChangesKarn intrinsic land-mana restriction
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant GameState
participant ManaSources
participant ManaAbilities
participant ActivationChecks
GameState->>ManaSources: request legal mana actions
ManaSources->>ManaAbilities: evaluate synthesized land-mana ability
ManaAbilities->>ActivationChecks: evaluate activation gates
ActivationChecks-->>ManaSources: return activation legality
ManaSources-->>GameState: include or exclude mana action
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)crates/engine/src/game/casting_tests.rsast-grep timed out on this file Comment |
Parse changes introduced by this PR✓ No card-parse changes detected. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mana_sources.rs`:
- Around line 2351-2364: The CR annotation in
crates/engine/src/game/mana_sources.rs:2351-2364 must document both intrinsic
basic-land mana and the activation prohibition by adding CR 305.6 alongside CR
602.5. In crates/engine/src/game/casting_tests.rs:30960-30966, add CR 613.1d for
the layer-4 Artifact type change. In
crates/engine/src/game/casting_tests.rs:31059-31068, add CR 305.6 and CR 602.5
for intrinsic Forest mana and its activation prohibition; ensure each annotation
includes the rule description matching the 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: 17cbc852-034b-410b-ace5-4bd485f17839
📒 Files selected for processing (2)
crates/engine/src/game/casting_tests.rscrates/engine/src/game/mana_sources.rs
|
Maintainer follow-up on current head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the subtype-only fallback still bypasses activation prohibitions.
🔴 Blocker
[HIGH] land_mana_options still emits a fallback mana action for a land with a basic land subtype, no explicit Effect::Mana, and a prohibition such as Karn's. Evidence: crates/engine/src/game/mana_sources.rs:2363 gates the fallback only on !has_explicit_mana_ability, then :2369 synthesizes the option without consulting CantBeActivated; docs/MagicCompRules.txt:1705 says the basic-land-type mana ability is intrinsic, and :2541 says a player cannot begin an activation prohibited from activation. Why it matters: a subtype-only artifact land (the Urborg/Blood Moon class this fallback explicitly preserves) remains a legal mana source under Karn even though its intrinsic {T}: Add ... ability is also an activated ability. The new Forest regression has an explicit Effect::Mana, so it exercises the new guard but cannot reach this remaining fallback branch. Suggested fix: apply the same activation-gate decision to the synthesized intrinsic fallback, then add a real legal-action regression with a bare subtype-only artifact land under the Karn prohibition alongside the existing positive bare-subtype fallback test.
Recommendation: request changes — preserve the ordinary bare-subtype fallback, but make its synthesized action obey the same prohibitions as every other activated mana ability.
The first fix only closed the gap for a land carrying an explicit
Effect::Mana ability. A land with NO explicit ability at all - just a
basic land subtype, the genuine Urborg/Blood-Moon-class case the
fallback exists for - hits land_mana_options()'s bare-subtype branch
directly, bypassing scan_mana_abilities() entirely and any
CantBeActivated check with it. Per CR 305.6 that intrinsic "{T}: Add
[mana symbol]" ability is still an activated mana ability, so CR 602.5
prohibitions must block it exactly like a printed one.
Add mana_abilities::intrinsic_land_mana_ability_blocked, which builds a
minimal synthetic AbilityDefinition for the intrinsic ability and
delegates to the single-authority is_blocked_by_cant_be_activated /
is_blocked_by_cant_activate_during checks - never re-implements them.
Wire it into the fallback, gated on require_current_payability to match
how is_active_tap_mana_ability treats real abilities (the auto-tap
planning pass doesn't consult per-source legality gates for any mana
source, real or intrinsic).
Adds a regression test with a bare-subtype artifact land under Karn's
prohibition (the case the prior fix's Forest test couldn't reach, since
Forest carries an explicit ability) and a positive companion test
confirming the ordinary bare-subtype fallback still works with no
prohibition in play.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed — thanks for the catch, and for the direct CR 305.6 citation fixup. The blocker was correct: the first commit only closed the gap for a land carrying an explicit Fixed in the new commit: Added Full suite (18,109 tests), clippy, and fmt all clean. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mana_abilities.rs`:
- Around line 1339-1362: Update intrinsic_land_mana_ability_blocked to route the
synthesized land-mana AbilityDefinition through the full activation-readiness
logic, including phased-out, detained, and untappable-source checks. Refactor
the shared readiness helper as needed to accept synthesized abilities while
preserving existing mana_ability_ready_without_simulation_gated behavior for the
planning path, and ensure mana_sources does not emit an option when readiness
fails.
🪄 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: b54f0b8d-6e1d-441b-a1e2-8f5b01100a59
📒 Files selected for processing (3)
crates/engine/src/game/casting_tests.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/mana_sources.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/game/mana_sources.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the intrinsic land-mana fallback still bypasses shared readiness gates.
🔴 Blocker
[HIGH] intrinsic_land_mana_ability_blocked reimplements only two of the legal-action checks for the synthesized CR 305.6 ability. Evidence: crates/engine/src/game/mana_abilities.rs:1339-1362 constructs the synthetic {T}: Add ability and checks only is_blocked_by_cant_be_activated and is_blocked_by_cant_activate_during; the established readiness authority at crates/engine/src/game/mana_abilities.rs:1393-1478 additionally excludes phased-out and detained sources and {T} costs on object_cant_tap sources. activatable_land_mana_options_indexed_gated takes the interactive legal-action path with require_current_payability = true at crates/engine/src/game/mana_sources.rs:1324-1339, then emits the fallback option after this partial check at :2369-2403.
Why it matters: a bare-subtype land is a synthesized activated mana ability, so a phased-out, detained, or can't-become-tapped Forest can still be offered through activatable_mana_actions_for_player, even though the equivalent printed mana ability is excluded by the shared readiness authority. The current bare-Forest/Karn test correctly covers the prior CantBeActivated gap, but it does not exercise these sibling gates.
Suggested fix: route the synthetic ability through the same simulation-free readiness authority (refactoring that authority as needed to support a synthesized definition without inventing a parallel legality predicate), while preserving the deliberately different auto-tap planning path. Add real legal-action regressions for at least detained/phased-out and CantTap bare-subtype lands, plus a positive unaffected fallback case.
… readiness authority
intrinsic_land_mana_ability_blocked reimplemented only two of the checks
a printed mana ability goes through (CantBeActivated,
CantActivateDuring), missing phased-out (CR 702.26b), detained
(CR 701.35a), and can't-tap (CR 701.26a + CR 508.1f) sources. A
bare-subtype land in any of those states was still offered as a legal
mana source, since the equivalent printed mana ability is excluded by
mana_ability_ready_without_simulation_gated but the synthesized
intrinsic ability was checked against a narrower, hand-picked subset.
That readiness authority takes an AbilityDefinition by reference and
never indexes obj.abilities, so a synthesized definition with no real
storage slot is exactly as valid an input as a printed one - no
refactor of the authority needed. Route the synthetic {T}: Add ability
through it directly instead of re-implementing any subset of its
checks, preserving the require_current_payability split that already
keeps the auto-tap planning path (which doesn't consult per-source
legality gates for any mana source, real or intrinsic) distinct from
the interactive legal-action path this whole family targets.
Adds three regression tests (detained, phased-out, can't-tap
bare-subtype lands, none of which need a CantBeActivated static to
demonstrate the gap) alongside the existing CantBeActivated and
positive-fallback coverage.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed — good catch, my two-check version was exactly the "parallel legality predicate" you flagged. You were right that Added the three regressions you asked for: Full suite (18,112 tests), clippy, and fmt all clean. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/casting_tests.rs`:
- Around line 31296-31298: Update the rules annotation above the CantTap test to
remove CR 508.1f and cite the verified rules covering the tap symbol, ability
activation, unpayable costs, activation prohibitions, and intrinsic basic-land
abilities, including CR 305.6 and CR 602.5. Keep the annotation limited to rules
that directly describe why a permanent unable to become tapped cannot pay the
intrinsic {T} activation cost.
🪄 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: e68bd082-1a9a-4822-8b86-da42aa3224df
📒 Files selected for processing (3)
crates/engine/src/game/casting_tests.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/mana_sources.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/src/game/mana_sources.rs
- crates/engine/src/game/mana_abilities.rs
| // CR 701.26a + CR 508.1f + CR 305.6: a permanent that can't become tapped | ||
| // can't pay a {T} activation cost — including a bare-subtype land's | ||
| // intrinsic {T}: Add mana ability. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the CantTap rules annotation.
Line 31296 cites CR 508.1f, but that rule describes tapping creatures as attackers. It does not describe paying an activation cost. (media.wizards.com)
For this test, cite the rules for the tap symbol, ability activation, unpayable costs, activation prohibitions, and intrinsic basic-land abilities instead. (media.wizards.com)
Proposed annotation fix
- // CR 701.26a + CR 508.1f + CR 305.6: a permanent that can't become tapped
+ // CR 107.5 + CR 602.2b + CR 602.5 + CR 601.2h + CR 305.6: a permanent that can't become tappedAs per path instructions, rules-touching code requires verified CR annotations, including CR 305.6 and CR 602.5, and each cited rule must describe the annotated behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // CR 701.26a + CR 508.1f + CR 305.6: a permanent that can't become tapped | |
| // can't pay a {T} activation cost — including a bare-subtype land's | |
| // intrinsic {T}: Add mana ability. | |
| // CR 107.5 + CR 602.2b + CR 602.5 + CR 601.2h + CR 305.6: a permanent that can't become tapped | |
| // can't pay a {T} activation cost — including a bare-subtype land's | |
| // intrinsic {T}: Add mana ability. |
🤖 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_tests.rs` around lines 31296 - 31298, Update
the rules annotation above the CantTap test to remove CR 508.1f and cite the
verified rules covering the tap symbol, ability activation, unpayable costs,
activation prohibitions, and intrinsic basic-land abilities, including CR 305.6
and CR 602.5. Keep the annotation limited to rules that directly describe why a
permanent unable to become tapped cannot pay the intrinsic {T} activation cost.
Source: Path instructions
|
Maintainer fixup on current head The behavioral readiness fix and its detained, phased-out, and |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved: intrinsic basic-land mana now uses the existing full readiness authority, preserving the legal fallback while enforcing activation gates. Current-head CI, parse-diff, and independent review are clean.
2 participants
Summary
Fixes #6469 — Karn, the Great Creator failed to block an opponent's activated ability on a land that Liquimetal Coating had turned into an artifact.
land_mana_options()(mana_sources.rs) fell back to unconditional, subtype-inferred mana production wheneverscan_mana_abilities()returned no options — without checking why it was empty. The fallback exists for lands that carry a basic land type but no explicitEffect::Manaability (e.g. Urborg/Blood-Moon-class grants). But it also fired when a real mana ability exists and was correctly filtered out by a legality gate (CantBeActivated,CantActivateDuring, an unsatisfied activation condition) — silently re-adding the land as a legal mana source and defeating the gate that just blocked it.Activated abilities of artifacts your opponents control can't be activated.) correctly matched the Coating-turned-artifact land and correctly filtered its{T}: Add {G}ability out ofscan_mana_abilities. The fallback then mistook that filtering for "no ability exists" and re-added the land anyway, letting the opponent still tap it for mana.Effect::Manaability at all, so the Urborg/Blood-Moon case still works while a real, currently-prohibited ability stays prohibited.I verified the parser/AST side is not at fault first: Karn's
CantBeActivated { source_filter: Typed(Artifact, controller: Opponent) }filter parses and is stored correctly (confirmed againstcard-data.json), and a synthetic test driving the realGenericEffect→ transient continuous effect → layer-4AddTypepipeline confirms the land'scard_typesare updated correctly before the block is checked. The actual defect is downstream in mana-source legal-action generation, not in parsing or the layer system.Test plan
karn_blocks_liquimetal_coated_opponent_land— drives the realEffect::GenericEffect→TransientContinuousEffect→evaluate_layerspipeline (not hand-setcard_types) to confirm Karn blocks a Coating-turned-artifact opponent land.karn_blocks_liquimetal_coated_forest_from_legal_mana_actions— reproduces the actual bug viaactivatable_mana_actions_for_player(the real legal-action surface behind both the manual "tap for mana" UI and AI candidate generation) with a real Forest object (explicit{T}: Add {G}ability +Forestsubtype); fails before the fix, passes after.cargo test -p phase-engine --lib— full crate suite green (18064 passed) before rebasing onto latest upstream; re-verified green in the ship worktree against currentupstream/mainafter cherry-pick.cargo clippy -p phase-engine --lib --tests -- -D warnings— clean.cargo fmt— clean.game::mana_sources(46 tests, incl.manual_fallback_land_action_*which exercise the genuine bare-subtype fallback),game::mana_abilities(130 tests),game::casting_costs(121 tests),urborg/karn/pithing_needle/cant_be_activatedfilters — all pass.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests