[PIP] Implement Codsworth, Handy Helper, improved attachment to permanent logic

[PurpleCrowbar]

[PurpleCrowbar]

[[Codsworth, Handy Helper]]

[github-actions[bot]]

Codsworth, Handy Helper - (Gatherer) (Scryfall) (EDHREC)

{2}{W} Legendary Artifact Creature — Robot 2/3 Commanders you control have ward {2}. {T}: Add {W}{W}. Spend this mana only to cast Aura and/or Equipment spells. {T}: Attach target Aura or Equipment you control to target creature you control. Activate only as a sorcery.

[PurpleCrowbar]

Closes #12108

[Susucre]

I feel like the attachment actions are not setup the best way. There is currently Permanent::attachTo, Card::addAttachment and Player::addAttachment. Those methods should probably handle the invalid attachment (and not attach if invalid), and unattach if attachment to another object is done. I think part of that is already there, but apparently they don't do all of that since you needed to take care of it for those two cards.

That being said, both cards here should be fine, so I'm not against merging as is, but could you please add a couple tests to make a future rework a little safer? Unit tests are really a great way to ensure current effort will not be undone by mistake in the future.

[PurpleCrowbar]

I wrote some tests and hit a roadblock with one, in which moving an "Enchant land" aura to a creature would cause it to be moved to the graveyard, as opposed to not moving at all. Looking into this, it seems to be because Permanent::cantBeAttachedBy doesn't check for whether or not the aura is actually meant to be able to enchant that object. This is presumably because the Enchant Ability is actually cosmetic (#9583) and can't be used to check whether or not the move is legal. Accordingly, the aura is moved to the creature, and then moves to the graveyard (due to state-based actions). Not sure how to resolve yet as I suppose the answer is buried in the state-based action code, the relevant section of which (I believe) is below:

https://github.com/magefree/mage/blob/fd0da67e468bbb2f15cfa857a25104db2e1b025f/Mage/src/main/java/mage/game/GameImpl.java#L2493-L2559 Pushing tests now, including failing test (testIllegalAttachmentDenied)

[PurpleCrowbar]

Fixed the problem of Permanent::cantBeAttachedBy not checking for an aura's "Enchant" target, causing enchantments to illegally attach and then immediately go to the graveyard (now they simply don't move in the first place, as intended). Also added a test for ensuring equipment doesn't move when an attempt to attach it to a noncreature permanent is made.

[JayDi85]

Looks like there are many bug reports with attach problems, maybe something can be closed by that PR

[PurpleCrowbar]

Looks like there are many bug reports with attach problems, maybe something can be closed by that PR

I believe that #11328 is fixed by this PR Halvar has issues fixed here

[PurpleCrowbar]

Seems like Travis is ignoring this PR? Edit: nope, looks like Travis is ignoring all PRs

[PurpleCrowbar]

Not ready to merge - despite not shown on GitHub there is a failing test and a couple more tests I plan to write

[PurpleCrowbar]

Fixed interaction with Unfinished Business; fixed auras without a legal target still being moved to the battlefield and then going to the graveyard, as opposed to just remaining in their original zone (303.4g); fixed shroud preventing the attachment of uncast auras; increased test coverage; and improved functionality of Dream Leash.

Of note is that [[Dream Leash]] is not 100% perfect, but this is because it is implemented in a necessarily hacky way due to #9583. As it stands, if Dream Leash is "sneaked" onto the battlefield without being cast (e.g., via [[Show and Tell]]), it will go to the graveyard immediately. In reality, Dream Leash has "Enchant permanent" and should be able to attach to any permanent as long as it isn't being cast, but enchanting doesn't work correctly in xmage so the workaround to achieve its unique effect isn't perfect. Gameplay-wise this isn't important as it is more than likely that not once in the future will anyone ever sneak a Dream Leash onto the battlefield, having not cast it, and having not spent any mana to do so (and, thus, having no tapped permanents). I wrote a currently ignored, failing test for the interaction in 48aac3329ca16f369ccf858eac402c6e935d8511.

This PR should be fine to merge, unless anyone spots any obvious mistakes

[github-actions[bot]]

Dream Leash - (Gatherer) (Scryfall) (EDHREC)

{3}{U}{U} Enchantment — Aura Enchant permanent You can't choose an untapped permanent as this spell's target as you cast it. You control enchanted permanent.

Show and Tell - (Gatherer) (Scryfall) (EDHREC)

{2}{U} Sorcery Each player may put an artifact, creature, enchantment, or land card from their hand onto the battlefield.

[PurpleCrowbar]

Maybe I’ll look at #9583 later — that’s workarounds and broken game states are bad.

I agree it would be better to resolve that issue - this PR doesn't break any tests and improves functionality of moving auras so might be worth merging if only as a temporary solution, I don't really mind either way (though it would be nice to have Codsworth implemented)

[JayDi85]

Sorry, looks like I forgot to save code comment before send review. It was about that part of changes — copy and modification to non-required target smells bad:

[PurpleCrowbar]

Sorry, looks like I forgot to save code comment before send review. It was about that part of changes — copy and modification to non-required target smells bad

I assume the copy of the target is destroyed when leaving scope(?) so hopefully not a memory concern. It's a bit of a workaround but doesn't appear to create any problems. Ultimately this PR is a band aid fix to the problems outlined in #9583 without fixing the fundamental problem, so need not be merged if it's feasible to resolve that issue, but otherwise fixes many interactions

[JayDi85]

Is it fixed #9583?

[xenohedron]

No, #9583 is still an open issue