[don't merge] Linked ability framework for Zinnia Valley's Voice

[sneddigrolyat]

This targets the engine bug in #12752, not just the Zinnia card implementation in #12568.

Problem

Offspring is an additional cost with a linked enters-the-battlefield ability. The bug was that XMage could lose or blur that linkage when the ability was granted by another object, reapplied through applyEffects, copied on the stack, or carried through spell-to-permanent transitions for copied permanent spells. In practice that showed up as the wrong Offspring payment state being remembered, copied, or rechecked.

This is why the issue is broader than Zinnia itself. Zinnia exposed the failure, but the underlying problem is generic linked-ability state management.

Why the previous attempts were not enough

PR #14595 and PR #14609 improved the immediate Zinnia behavior, but they still leaned on workaround-style handling around Offspring resolution. That did not fully solve the engine problem described in #12752, and it left the linkage model too specific to one mechanic path.

Rules claim

This change is meant to preserve linked-ability identity per instance across the relevant engine copy and reapplication paths:

• CR 607.2i and 607.5 for linked abilities, including abilities gained from other effects
• CR 702.175 for Offspring as an additional cost with a linked ETB result
• The same linkage model is also exercised against Squad, which has similar paid-status and copy-path concerns

What changed

• Added linkage-preserving identity support to Ability, so engine copy paths can explicitly request a fresh ability id while preserving the same linkage grouping.
• Kept gained-ability linkage stable across applyEffects cycles instead of re-randomizing it every time the granted ability is rebuilt.
• Preserved linkage across stack copies, copied spells, and copied triggered abilities.
• Preserved granted entering abilities across spell-to-permanent transitions.
• Fixed the copied permanent spell -> token permanent path so it keeps linkage without changing the normal fromExistingObject subability handling.
• Kept the test split clearer: generic Offspring engine regressions live in OffspringTest, while actual Zinnia card integration stays in ZinniaValleysVoiceTest.

Why the generic grant-source test helper still exists

I checked the current card pool in this branch. Zinnia is the only printed card here that grants Offspring to creature spells. Because of that, the generic helper remains for mechanic-level granted-Offspring tests that need to isolate the engine path or create multiple independent grant sources without mixing in Zinnia-specific creature, legend, or copy behavior. Actual Zinnia integration coverage stays in ZinniaValleysVoiceTest.

Validation rerun after the latest cleanup

mvn -pl Mage.Tests -am "-Dtest=org.mage.test.cards.abilities.keywords.OffspringTest,org.mage.test.cards.single.blc.ZinniaValleysVoiceTest,org.mage.test.cards.abilities.keywords.SquadTest" "-Dsurefire.failIfNoSpecifiedTests=false" test

mvn -pl Mage.Tests -am "-Dtest=org.mage.test.cards.abilities.keywords.MeldTest#testMeld_Urza_Eliminate_After_Rollback,org.mage.test.cards.planeswalker.JaceTest#rollbackDoesntUnflipJaceTest,org.mage.test.rollback.StateValuesTest#rollbackTokenCreationTest" "-Dsurefire.failIfNoSpecifiedTests=false" test

Targeted coverage now includes:

• Offspring pay and no-pay behavior
• Granted Offspring from another source
• Printed plus granted Offspring with one payment and with two payments
• Humility before resolution
• Removing the granting source before ETB
• Copying a paid Offspring spell
• Copying an Offspring ETB trigger
• Rollback clearing stale linked payment state
• Multiple independent grant sources
• Squad copy and payment regressions
• Rollback and token-state sanity checks

Main review question

Is preserving linkage across stack copies, spell copies, applyEffects reapplication, and spell-to-permanent transitions, while still giving each copied ability instance a fresh normal ability id, the right CR 607 model for XMage?

[sneddigrolyat]

@ssk97 Thanks, your comment was helpful. Replying here since you noted it belongs on the PR.

I ended up changing the approach in a few important ways based on the concerns you raised.
The final version does not rely on storing raw ability references across object-copy boundaries. Instead, the granted-ability path now remaps copied abilities to a deterministic per-source identity/linkage, so separate granted instances stay independent without depending on shared Java-object references. That also made it possible to cover rollback-sensitive cases directly.

AbilityImpl point taken. Moved the remap hook onto Ability (remapForSource(UUID)), with AbilityImpl providing the shared implementation and StackAbility delegating.

Also backed away from the broader PermanentCard / copied-permanent remap experiment. A full Mage.Tests sweep showed that approach was too broad and caused unrelated regressions. The remaining permanent-side fix is narrower:

• ETB trigger validation now prefers game.getPermanentEntering(sourceId) over the copied card/spell object
• copied permanent spells that resolve as token permanents preserve the linked ability instances they already had through the spell-to-permanent transition

I added coverage for the cases you called out:

• copying a paid Offspring spell
• copying an Offspring ETB trigger
• copied Zinnia via Spark Double with one granted payment and with two granted payments
• rollback clearing stale printed+granted Offspring payment state

Current local validation:

• OffspringTest, ZinniaValleysVoiceTest, SquadTest, and the rollback sanity tests passed
• today’s manual tests also passed
• latest broad Mage.Tests run had one remaining failure in SimulationTriggersAITest.test_DeepglowSkate_PerformanceOnTooManyChoices, and that same failure reproduced on master

The main thing I’d like review on is whether this narrower linkage-preservation model for granted abilities plus copied permanent spells is the right way to go.

[ssk97]

It's hard to review code like this since I can't trust that the kind of mistakes made are human mistakes, and much of your "helpful" explanatory text here in the PR is either misleading or irrelevant.

Why do you check that it's an AbilityImpl everywhere? It seems like these functions need to be added to the Ability interface.

In copyFromToken, don't you need to not add subabilities if preserveCopiedSpellLinkage is true? Or am I misunderstanding how that is working?

Several of your tests are weird. Why have a bunch of Zinnia tests inside the main OffspringTest file and then others split off? Why are you making your own custom card that seems to be identical to Zinnia (save for being an enchantment)? I haven't manually checked over all of your tests for compliance with the real MtG rules, that's something that probably needs to be done.

[sneddigrolyat]

Thanks @ssk97 for taking time to look at the code and provide feedback. I definitely need the help to get this implemented correctly.

On the AbilityImpl checks:
I moved newIdKeepingLinkage() onto the Ability interface and had StackAbility delegate it, so the linkage-preserving copy and apply paths no longer need repeated instanceof AbilityImpl checks. The relevant engine call sites now go through the interface instead of special-casing AbilityImpl.

On copyFromToken and subabilities:
I changed the copied-spell token path so it preserves linkage through a dedicated PermanentImpl.addAbility(... preserveLinkage) path instead of directly reusing the existing ability objects. That keeps the old fromExistingObject subability behavior, so the copied-spell token path is no longer handling subabilities differently from the normal token-copy path.

On the test layout:
I cleaned up the split between mechanic tests and card tests. OffspringTest now stays focused on generic Offspring and granted-Offspring engine regressions, while ZinniaValleysVoiceTest keeps the actual Zinnia integration cases. I also removed the redundant noncreature-source Humility variant and renamed the Zinnia-specific source-removal test so the distinction is clearer.

On the custom grant-source fixture:
I checked the current card pool in this branch. Zinnia is the only printed card here that grants Offspring to creature spells. Because of that, I kept the helper for the generic granted-Offspring engine cases, but I tightened the comments so it is explicit that this helper exists to isolate the generic granted-Offspring engine path and to cover multiple independent grant sources without mixing in Zinnia-specific creature, legend, and copy behavior.

I reran the targeted regressions after these changes:
mvn -pl Mage.Tests -am "-Dtest=org.mage.test.cards.abilities.keywords.OffspringTest,org.mage.test.cards.single.blc.ZinniaValleysVoiceTest,org.mage.test.cards.abilities.keywords.SquadTest" "-Dsurefire.failIfNoSpecifiedTests=false" test

mvn -pl Mage.Tests -am "-Dtest=org.mage.test.cards.abilities.keywords.MeldTest#testMeld_Urza_Eliminate_After_Rollback,org.mage.test.cards.planeswalker.JaceTest#rollbackDoesntUnflipJaceTest,org.mage.test.rollback.StateValuesTest#rollbackTokenCreationTest" "-Dsurefire.failIfNoSpecifiedTests=false" test

Those targeted runs passed.