Stop the AI locking its own Room for no reason

[omraj21]

UnlockDoor is mapped to AlwaysPlayAi, whose entire logic is return WillPlay — it never looks at the board. For Mode$ LockOrUnlock that is wrong in two separate ways.

1. It can only lock, so the AI shuts off its own Room

When every door of the targeted Room is already unlocked, UnlockDoorEffect takes the case 0 branch, whose only outcome is lockRoom:

case "LockOrUnlock":
    switch (c.getLockedRooms().size()) {
    case 0:
        // no locked, all unlocked, can only lock door
        List<CardState> unlockStates = c.getUnlockedRooms()...;
        CardState chosenUnlock = activator.getController().chooseSingleCardState(...);
        ...
        c.lockRoom(activator, chosenUnlock.getStateName());

Keys to the House pays {3}, taps and sacrifices itself for that ability:

A:AB$ UnlockDoor | Cost$ 3 T Sac<1/CARDNAME> | Mode$ LockOrUnlock | ValidTgts$ Room.YouCtrl | ...

so the AI was throwing away a permanent to make its own board worse. The added test shows Keys to the House going from one copy on the battlefield to zero under the old mapping.

2. With one door locked, the door it picked was arbitrary

In the case 1 branch the effect offers both doors and locks whichever one is still open. That choice went through SpellAbilityAi.chooseCardState, whose base implementation is:

public CardState chooseCardState(Player ai, SpellAbility sa, List<CardState> faces, Map<String,Object> params) {
    System.err.println("Warning: default (ie. inherited from base class) implementation of chooseCardState is used for " ...);
    return Iterables.getFirst(faces, null);
}

Since bothStates is always built in fixed [LeftSplit, RightSplit] order, whether the AI opened or closed a door came down to state ordering rather than a decision — and it printed a warning to stderr every time.

The fix

UnlockDoorAi:

• declines when no reachable Room has a locked door, and
• picks a locked door when asked to choose, so the ability always opens a Room instead of closing one.

Mode$ Unlock and Mode$ ThisDoor can only ever unlock, so they keep the previous always-play behaviour.

Affects Keys to the House and Marina Vendrell.

Verification

Two tests, both of which fail on the old mapping:

• doesNotSacrificeItselfJustToLockItsOwnRoom — fails with Keys to the House should not have been sacrificed expected:<1> but was:<0>
• unlocksARoomWhenADoorIsStillLocked — fails with AI should have unlocked the remaining door, not locked the open one

forge.ai.** test suite: 247 tests, 0 failures. Full mvn -U -B clean test across all 12 modules passes.

---

This PR was previously part of #11441, which bundled it with an unrelated BecomeMonarch fix. Split out so each can be reviewed on its own.

Written with the help of GitHub Copilot CLI; the commit carries a Co-authored-by trailer for it.

[Hanmac]

@Agetian @tool4ever I think that is good for now?

There might be a small use-case for AI locking Doors that have an Unlock Trigger.

Also, AI should probably do a "Do i want to unlock this Door?" check:
Like checking if it wants to run a possible unlock trigger.

[Agetian]

Agreed about checking for a trigger, otherwise looks good to me :)

[Hanmac]

Yeah, the logic about "should i lock/unlock my Room" per trigger can be extended later

[omraj21]

Thanks both. I've pushed the unlock-trigger check as a second commit.

Those triggers are declared with ThisDoor$ True, and TriggerUnlockDoor.performTest compares the trigger's own state name against the state being unlocked, so matching on that is enough to tell the two halves apart:

if (hasParam("ThisDoor")) {
    CardState state = (CardState) runParams.get(AbilityKey.CardState);
    if (!getHostCard().equals(state.getCard())) return false;
    if (!getCardStateName().equals(state.getStateName())) return false;
}

Two places use it now:

• chooseCardState prefers a locked door whose face carries an unlock trigger, falling back to any locked door when neither pays out. Before this it just took the first locked door, so with both doors locked which half opened came down to state ordering.
• when the ability targets, it picks a Room that has such a door over one that would only swap which half is active.

Test case is Bottomless Pool // Locker Room with both doors locked: the Bottomless Pool face bounces a creature when unlocked, while Locker Room's trigger is a combat damage one that pays out nothing at the moment it opens. I checked the test actually catches a wrong choice by stubbing the preference out and confirming it fails.

On @Hanmac's other point — deliberately locking a door so it can be unlocked again later for the trigger — I've left that out on purpose. It needs a value model for whether the re-unlock is actually reachable (mana, another Keys effect, sorcery timing), and if that model is wrong it reintroduces exactly the "AI closes its own Room" behaviour this PR is fixing. Happy to look at it separately if you'd like it.

forge.ai.**: 248 tests, 0 failures. Rebased on current master.

[Hanmac]

@omraj21 example of trigger check i mean from ClassLevelAi:

forge/forge-ai/src/main/java/forge/ai/ability/ClassLevelUpAi.java

Lines 30 to 33 in 0acfa8f

	SpellAbility effect = t.ensureAbility();
	if (!SpellApiToAi.Converter.get(effect).doTrigger(aiPlayer, effect, false)) {
	return new AiAbilityDecision(0, AiPlayDecision.CantPlayAi);
	}

this doTrigger check is for looking if AI would choose this trigger if it wasn't mandatory

[omraj21]

Good call, that's better than what I had - swapped it over to doTrigger.

Two Rooms show why just checking the trigger exists isn't enough. Cramped Vents (6 damage to a creature an opponent controls) comes back false when they have no creatures, and Derelict Attic (draw two, lose 2 life) comes back false at 2 life - my old check would cheerfully have opened that door and killed the AI. Added a test for the Derelict Attic case; it fails with the presence-only check.

One gotcha in case you hit it elsewhere: the trigger's ability has no activating player until it actually fires, so doTrigger NPEs in DamageDealAi's targeting and in AiCostDecision unless you set it first. ClassLevelUpAi doesn't set it either, so it may have the same issue for any class trigger that targets or has a cost - happy to look at that separately.

249 tests, 0 failures. Rebased on master.

[Agetian]

It looks like the test is failing on a test run?

[omraj21]

It looks like the test is failing on a test run?

yes it was a tricky bug but I caught it, would appreciate a re-approval and another CI run

[omraj21]

That was my doing - the details are in the comment just above, but short version: my door choice fell back to the first locked door, and Card.getLockedRooms builds a HashSet, so the iteration order changed between JVM runs. It passed locally and failed on CI for that reason.

Fixed by ranking the doors rather than relying on ordering, and it turned out to be hiding a real bug: I'd been treating "has a trigger the AI doesn't want" the same as "has no trigger at all", which for Derelict Attic at 2 life meant picking the door that kills you.

CI is green now on both Java 17 and 21.