Feat: Mana Ability Rule Change (Apply CR 605.1a) by leriomaggio · Pull Request #11778 · Card-Forge/forge

leriomaggio · 2026-09-03T12:01:08Z

The 2026-08-07 rules update added a fourth criterion to 605.1a:

a mana ability's "cost and effect don't move any card to or from a library".

Forge implemented the other three criteria, so abilities that draw or mill still bypassed the stack.

Selvala, Explorer Returned and Chromatic Sphere are the two cards Wizards' own coverage names as the reason for the change, however this is the list of all the cards that should be affected (most of them verified in Testing - see below).

Chromatic Sphere
Darkwater|Mossfire|Shadowblood|Skycloud|Sungrass Egg
Selvala, Explorer Returned
Rainbow Dash
Charmed Pendant
Deranged Assistant
Millikin
Manakin and Millikin

Approach

Mana-ability status is derived structurally in SpellAbility.isManaAbility(),
so the fix is in the engine and no card scripts change.

SpellAbilityEffect.movesCardToOrFromLibrary(), defaults to false,
overridden in Draw, Mill, Surveil, Dig, DigUntil, Discover, Learn, Explore,
and conditionally in ChangeZone, ChangeZoneAll and Play.
LibraryMovementCostVisitor on the existing ICostVisitor, exposed as
Cost.movesCardToOrFromLibrary().
Both consulted from isManaAbility() as the main point of activation.

Unhandled cases resolve to false, so anything unrecognised stays a mana ability.
The chain is now walked in full (including also sub-abilities) rather than returning on the first mana part, since Chromatic Sphere has the mana part on the root
and the draw on a sub-ability.
Only the root's cost is checked. Nothing reads game state, per the
rule's instruction about replacement effects.

Card impact

12 cards of 1867 lose a mana ability, verified by two independent passes over all
33,669 card scripts and a cross-check against Scryfall's 38,630 unique oracle cards.
(This bit was done with support of a quick Python script I coded to look for specific keywords in cards text and forge card scripts implementation)

Tests

Two data-driven tests in GameSimulationTest: 12 rows for the cards that lose
the ability, 9 controls covering each reason a card is deliberately unaffected,
including a basic land as a guard. The commits are split so the two predicates
land inert and the behavioural change is one revertible commit.

CR 605.1a, as of the August rule update (2026-08-07), adds a fourth criterion to the definition of a mana ability: its cost and effect must not move any card to or from a library. This commit introduces `SpellAbilityEffect.movesCardToOrFromLibrary()`, defaulting to false, so that adding a new effect can never silently redefine what counts as a mana ability. Effects that always move library cards (Draw, Mill, Surveil, Dig, DigUntil, Discover, Learn, Explore) default automatically to true, whereas other effects where it depends on the script's zone parameters (i.e. ChangeZone, ChangeZoneAll, Play) rely on the new `SpellAbilityEffect.zoneParamIsLibrary` utility to assess the mana ability state. Scry, RearrangeTopOfLibrary, Shuffle, Reveal and PeekAndReveal keep the inherited false, since reordering or looking at a library moves nothing. It's worth nothing that the new predicate reads the ability's script rather than game state, because CR 605.1a says to disregard replacement effects other than self-replacement effects when evaluating the criteria. This predicate will be applied in isManaAbility() - nothing else should call this predicate directly!

This commit contains implementation for the second half of the CR 605.1a criterion: a mana ability's cost must not move any card to or from a library, so paying a cost needs the same utility function defined for effects, i.e. `SpellAbilityEffect.movesCardToOrFromLibrary()`. A new `LibraryMovementCostVisitor` over the existing `ICostVisitor` that returns true when paying a cost would move a card to or from a library. `CostMill` and `CostDraw` always do, in the library-to-graveyard and library-to-hand directions. `CostPutCardToLib` always does, in the other direction. `CostExile` does only when its zone list names the library, since that same cost part also exiles from hand, graveyard, battlefield and stack. Every other cost part is left unhandled, inherits null from ICostVisitor.Base, and reads as false. So a cost the visitor does not recognise leaves the ability a mana ability, which is current behaviour. `Cost` now includes `movesCardToOrFromLibrary()` alongside `hasTapCost()` so the call site in `isManaAbility()`` reads like the rule. Costs that only look at cards, such as revealing from hand or from the top of a library, move nothing and stay false. That is what keeps Metalworker and Sacellum Godspeaker mana abilities working.

CR 605.1a now disqualifies an activated ability from being a mana ability when its cost or effect moves a card to or from a library. Wire the predicate from `SpellAbilityEffec`t and the cost visitor from `Cost` into `isManaAbility()`. Only the root ability's cost is examined, since sub-abilities have no cost of their own. The SubAbility chain is however walked in full instead of returning as soon as a mana part turns up. "Chromatic Sphere alike effects" are the main reason: the mana part sits on the root and the draw sits on a sub-ability, so the early return never saw the draw.

Eight cases in GameSimulationTest, asserting on `Card.getManaAbilities()`, which filters on `SpellAbility.isManaAbility()` and is what the rest of the engine consumes. Must lose the mana ability: - Chromatic Sphere for a draw on a sub-ability, - Charmed Pendant and Deranged Assistant for a mill cost on an artifact and on a creature; - Selvala, Explorer Returned for the multi-player parley. Each also asserts the ability is still there as a non-mana ability, so a later bug cannot quietly delete it instead of reclassifying it. Must keep it: - Barbed Sextant, whose draw is in a delayed trigger; - Shaun & Rebecca, Agents, whose mill is in a reflexive trigger (even though this feels more a judgment call than a settled ruling on the ability. The reasoning was: an ability that creates a reflexive trigger moves no card itself, so it stays a mana ability.) - Metalworker, whose cost reveals from hand, and - Forest as a regression guard against the clause catching basic lands.

Hanmac · 2026-09-03T13:00:19Z

I'm unsure if we need extra class for LibraryMovementCostVisitor, but it seems the most clean idea

@tool4ever your opinion?

Hanmac · 2026-09-03T13:34:09Z

While there is no ManaAbility with that, but there are some more that might alter the Library:

ManifestBaseEffect like Cloak or Manifest Dread might move cards from the library.
ConniveEffect kinda like draw+discard

While there is no ManaAbility with that, but these Arena Effects kinda mess with the Library too:

SeekEffect also does kinda add card from library to hand
HeistEffect does exile from library

tool4ever · 2026-09-03T17:19:43Z

I'm unsure if we need extra class for LibraryMovementCostVisitor, but it seems the most clean idea

@tool4ever your opinion?

I guess it's one way to keep the logic together in one class 🤔

leriomaggio · 2026-09-03T18:09:38Z

I'm unsure if we need extra class for LibraryMovementCostVisitor, but it seems the most clean idea
@tool4ever your opinion?

I guess it's one way to keep the logic together in one class 🤔

Yes, that's exactly the idea behind that.
As a matter of facts, I started by inlining into Cost and then abstracted the behaviour away to keep the logic together.
I split it out because CostExile needs a case that depends on its zone list, so it's more than a couple of instanceof checks, and the whole Cost.java is quite long already. Cost.movesCardToOrFromLibrary() would keep the call site the same
whichever implementation we would decide to go with.

Happy to inline that directly into Cost if you prefer. It's a small class either way :)

leriomaggio · 2026-09-03T18:25:33Z

While there is no ManaAbility with that, but there are some more that might alter the Library:

ManifestBaseEffect like Cloak or Manifest Dread might move cards from the library.

ConniveEffect kinda like draw+discard

While there is no ManaAbility with that, but these Arena Effects kinda mess with the Library too:

SeekEffect also does kinda add card from library to hand

HeistEffect does exile from library

This is genuinely a good catch! Thanks. I'll add overrides for those.
Should be pretty quick and straightforward:

Seek, Heist and Connive look like plain true.
ManifestBaseEffect needs a condition since it defaults to TopOfLibrary but can be pointed elsewhere via ChoiceZone;
Cloak, Manifest and Manifest Dread all extend it so one override should cover them

Interestingly, none of these show up in a mana ability today, so no card should be affected, but we would be ready in case that will happen.
I'll re-run the sweep over the cardsfolder to confirm that before pushing, and add some tests to cover those cases as well.

Seek, Heist and Connive always move a card to or from a library. ManifestBase does too unless ChoiceZone points elsewhere, since it defaults to TopOfLibrary, and Cloak, Manifest and Manifest Dread all extend it. No mana ability uses any of these today and no card changes behaviour, the sweep over the whole `cardsfolder` still result into the same list. Nonetheless, the base predicate defaults to false, so leaving those out would have been a silent miss once one is scripted (e.g. Custom cards?). Because there is no card to test these, new tests now exercise the predicates directly: ApiType rows for the effects, zone-parameter cases for ChangeZone, Play and Manifest, and a handful of cost strings.

jamincollins · 2026-09-03T20:35:39Z

Findings

Library movement predicate implementation

1. zoneParamIsLibrary uses substring contains and is case-sensitive

The helper SpellAbilityEffect.zoneParamIsLibrary is implemented as:

return sa.getParamOrDefault(param, "").contains(ZoneType.Library.toString());

ZoneType.Library.toString() is "Library". This is a substring check, case-sensitive, and will match any param containing the substring "Library".

Test case:

SpellAbility sa = new SpellAbility.EmptySa(ApiType.ChangeZone, card);
sa.putParam("Origin", "TopOfLibrary");
boolean moves = ApiType.ChangeZone.getSpellEffect().movesCardToOrFromLibrary(sa);

sa.getParamOrDefault("Origin","") returns "TopOfLibrary". "TopOfLibrary".contains("Library") is true, so the effect is reported as moving a card to/from a library even though the param value is not a zone name but a descriptive string. The same applies to lower-case params: "library".contains("Library") is false, so a legitimately lower-case param would be missed.

Impact: false positives for descriptive param values that happen to contain the substring "Library", and false negatives if card scripts ever use lower-case zone names.

Recommended fix: parse the param as a comma-separated list of zone names and compare equality ignoring case, e.g.:

String v = sa.getParamOrDefault(param, "");
for (String part : v.split(",")) {
    if (part.trim().equalsIgnoreCase(ZoneType.Library.name())) return true;
}

2. LibraryMovementCostVisitor does not cover all library-moving cost types

LibraryMovementCostVisitor only overrides:

CostMill
CostDraw
CostPutCardToLib
CostExile with from.contains(ZoneType.Library)

Costs that move a card to/from a library via other mechanisms are not covered.

Test case:

Cost c = new Cost("T ExileFromTop<1/Card>", true);
boolean moves = c.movesCardToOrFromLibrary(); // true via CostExile.from

Works. However:

Cost c2 = new Cost("T PutOnTop<1/Card>", true); // hypothetical cost that puts a card on top of library
boolean moves = c2.movesCardToOrFromLibrary();

If the parser creates a CostPutCardToLib for "PutOnTop", the visitor returns true. If the parser creates a different cost class, e.g. CostPutOnTop that extends CostPartWithList but is not CostPutCardToLib, the visitor returns null → false, incorrectly keeping a mana ability.

The visitor inherits Base<Boolean> where all unoverridden visit methods return null. The movesCardToOrFromLibrary check uses Boolean.TRUE.equals(...), so null is treated as false. Any new cost class that moves a card to/from a library will silently be treated as non-moving.

Recommended fix: add a default visit(CostPart cost) that inspects the cost's semantic name or add explicit overrides for all known library-moving cost parts, and add a test that enumerates all CostPart subclasses.

3. isManaAbility cost check is root-only

SpellAbility.isManaAbility() checks:

final Cost cost = getPayCosts();
if (cost != null && cost.movesCardToOrFromLibrary()) return false;

Only the root ability's pay costs are inspected. Sub-abilities do not have separate pay costs in Forge's model, so this is likely intentional, but the rule text says "its cost or effect". If a future card script assigns a cost to a sub-ability, the library-moving cost would be missed.

Test case:

// Hypothetical ability with a mana part on the root and a sub-ability that has a pay cost mill
SpellAbility root = ...;
root.setSubAbility(subWithMillCost);
root.isManaAbility() // cost check only looks at root, not sub

Currently returns based on effect check only.

Recommended fix: document the assumption that sub-abilities never carry pay costs, or walk the chain and inspect getPayCosts() for each tail.

4. Test assertions are overly strict for cards with multiple abilities

testLibraryMovementLosesManaAbility asserts:

AssertJUnit.assertTrue(cardName, c.getManaAbilities().isEmpty());

The data provider expects the card to lose its mana ability. If a card has multiple activated abilities and only one of them is a mana ability that moves a library, the other mana abilities would still be present, causing the test to fail even though the targeted ability is correctly classified.

Test case:

Card c = manaAbilityTestCard("SomeCardWithTwoAbilities");
SpellAbility manaSA = manaAddingActivatedAbility(c); // picks first mana-adding SA
AssertJUnit.assertFalse(manaSA.isManaAbility());
// but c.getManaAbilities() may still contain a different mana ability
AssertJUnit.assertTrue(c.getManaAbilities().isEmpty()); // false positive failure

The assertion conflates "the tested ability is no longer a mana ability" with "the card has no mana abilities at all".

Recommended fix: assert only that the specific ability returned by manaAddingActivatedAbility is no longer a mana ability, not that the card's entire mana-ability set is empty.

5. ManifestBaseEffect default assumes library

ManifestBaseEffect.movesCardToOrFromLibrary:

return !sa.hasParam("ChoiceZone") || zoneParamIsLibrary(sa, "ChoiceZone");

If ChoiceZone is present but empty, hasParam is true, zoneParamIsLibrary returns false, so the method returns false. An empty ChoiceZone is likely a scripting error, but the predicate will incorrectly report no library movement.

Test case:

SpellAbility sa = new SpellAbility.EmptySa(ApiType.Manifest, card);
sa.putParam("ChoiceZone", "");
boolean moves = ApiType.Manifest.getSpellEffect().movesCardToOrFromLibrary(sa);
// moves == false, but the intent is ambiguous

Recommended fix: treat blank ChoiceZone as absent, i.e., default to library.

Verdict

The PR correctly implements the CR 605.1a intent and the test suite demonstrates the intended card changes. However, the library-movement predicates rely on fragile string contains checks, the cost visitor is open to silent misses for new cost types, and the test assertions are stricter than necessary. These issues do not break the current card set but create maintenance risk.

Recommended Fixes

Replace zoneParamIsLibrary substring check with proper comma-split, trimmed, case-insensitive zone name comparison.
Add exhaustive coverage for cost visitor: either provide a safe default that flags unknown cost parts, or enumerate all CostPart subclasses and add unit tests for each.
Relax testLibraryMovementLosesManaAbility to assert only on the targeted ability, not on c.getManaAbilities().isEmpty().
Guard ManifestBaseEffect.movesCardToOrFromLibrary against empty ChoiceZone values.
Add a unit test that verifies movesCardToOrFromLibrary returns false for ApiType.Scry, ApiType.Shuffle, ApiType.Reveal and true for all library-moving effects, with both default params and explicit non-library params.

Reviewed by Hermes Agent - muse-glimmer-30b

…r tests zoneParamIsLibrary stays a substring test because card scripts write Destination$ TopOfLibrary and BottomOfLibrary, which name the library specifically without being zone names, but it now ignores case (as correctly pointed out!). ManifestBase treats a blank ChoiceZone as absent, so it falls back to the top of the library rather than out of scope. Added a quick note (top comment) on top of isManaAbility pointing out that only the root ability's cost is inspected, matching the assumption getCostDescription already makes. The card tests now assert that the specific mana-adding ability is no longer offered as a mana source. Also included cases for a TopOfLibrary destination and a blank ChoiceZone.

leriomaggio · 2026-09-04T08:12:50Z

Thank you @jamincollins for the useful insights.

I believe that suggestions #3 and #5 are right and now fixed in my latest commit, as well as suggestion #4.
The first point is "half correct", whilst I don't think #2 holds.

1. Substring check. Destination$ TopOfLibrary and BottomOfLibrary are real values in the cardsfolder. But they
are the library, so a comma-split zone-name comparison would return false for
both and quietly regress. Keeping the substring test, now
StringUtils.containsIgnoreCase for the case point, with a comment and a test
for TopOfLibrary.

2. Cost visitor. ICostVisitor has one method per cost class, so a
CostPutOnTop can't exist without adding a visit for its (as far as I understand it). which would not compile at all ?
Moreover, flagging unknown cost parts would make
Sac, PayLife and every counter cost read as library movement. The false
default is in fact deliberate and originally intended to cover "unrecognised" cases.

3. Root-only cost. Checking "root-only" is intentional.
getCostDescription() makes the same assumption with the comment "SubAbilities don't have Costs or Cost".

4. Test assertion. Fixed, now asserts !c.getManaAbilities().contains(sa)
rather than isEmpty() on the whole set.

5. Blank ChoiceZone. Fixed with StringUtils.isBlank, plus a test.

One last note: the last recommendation has been already included in testEffectLibraryMovement as part of a2f3102, which leads to think that the automated review predates the commit?

Many thanks!

jamincollins

Approved per adversarial review and author fixes in 84a5b49

Hanmac · 2026-09-04T13:58:09Z

+    public boolean movesCardToOrFromLibrary(final SpellAbility sa) {
+        // manifests the top of the library unless ChoiceZone points somewhere else. A blank
+        // ChoiceZone is treated as absent, so it falls back to the library rather than out of scope.
+        return StringUtils.isBlank(sa.getParam("ChoiceZone")) || zoneParamIsLibrary(sa, "ChoiceZone");


Use hasParam instead of checking for Blank

Hanmac · 2026-09-04T14:01:26Z

+     * not zone names, so parsing the value as a ZoneType list would miss them.
+     */
+    protected static boolean zoneParamIsLibrary(final SpellAbility sa, final String param) {
+        return StringUtils.containsIgnoreCase(sa.getParamOrDefault(param, ""),


ZoneType.listValueOf probably better check

With checking hasParam before

leriomaggio and others added 7 commits September 3, 2026 12:05

Expanding Test cases to all affected cards

07368a5

Merge branch 'master' into feat/mana-ability-rule-change

9bea66d

Merge branch 'master' into feat/mana-ability-rule-change

d47125e

tool4ever linked an issue Sep 3, 2026 that may be closed by this pull request

Mana Ability rules update #11508

Open

kevlahnota requested a review from Hanmac September 3, 2026 12:21

Hanmac requested review from tehdiplomat and tool4ever September 3, 2026 12:51

Merge branch 'master' into feat/mana-ability-rule-change

aa4444c

tool4ever previously approved these changes Sep 3, 2026

View reviewed changes

leriomaggio dismissed tool4ever’s stale review via a2f3102 September 3, 2026 18:37

jamincollins approved these changes Sep 4, 2026

View reviewed changes

Hanmac reviewed Sep 4, 2026

View reviewed changes

Navigation Menu

Uh oh!

Feat: Mana Ability Rule Change (Apply CR 605.1a) - #11778

Feat: Mana Ability Rule Change (Apply CR 605.1a)#11778
leriomaggio wants to merge 10 commits into
Card-Forge:masterfrom
leriomaggio:feat/mana-ability-rule-change

leriomaggio commented Sep 3, 2026

Uh oh!

Hanmac commented Sep 3, 2026

Uh oh!

Hanmac commented Sep 3, 2026

Uh oh!

tool4ever commented Sep 3, 2026

Uh oh!

leriomaggio commented Sep 3, 2026

Uh oh!

leriomaggio commented Sep 3, 2026

Uh oh!

jamincollins commented Sep 3, 2026

Uh oh!

leriomaggio commented Sep 4, 2026

Uh oh!

jamincollins left a comment

Uh oh!

Hanmac Sep 4, 2026

Uh oh!

Hanmac Sep 4, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

leriomaggio commented Sep 3, 2026

Approach

Card impact

Tests

Uh oh!

Hanmac commented Sep 3, 2026

Uh oh!

Hanmac commented Sep 3, 2026

Uh oh!

tool4ever commented Sep 3, 2026

Uh oh!

leriomaggio commented Sep 3, 2026

Uh oh!

leriomaggio commented Sep 3, 2026

Uh oh!

jamincollins commented Sep 3, 2026

Findings

Library movement predicate implementation

1. zoneParamIsLibrary uses substring contains and is case-sensitive

2. LibraryMovementCostVisitor does not cover all library-moving cost types

3. isManaAbility cost check is root-only

4. Test assertions are overly strict for cards with multiple abilities

5. ManifestBaseEffect default assumes library

Verdict

Recommended Fixes

Uh oh!

leriomaggio commented Sep 4, 2026

Uh oh!

jamincollins left a comment

Choose a reason for hiding this comment

Uh oh!

Hanmac Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Hanmac Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Hanmac Sep 4, 2026 •

edited

Loading