Select the configured active player

[janmarsino98]

Fixes #1038

Step now accepts an explicit active player. When Simulate receives a map of player controllers, it selects an active player that has a matching configuration and stops cleanly when none is available.

Previously, both paths always selected the first key in ctx.activePlayers. That ignored the player requested by the caller and could choose an entry missing from the supplied controller map.

Tests:

• focused regression suite: 23 tests passed
• pnpm run lint
• pnpm test: 43 suites passed, 904 tests passed, 1 todo
• pnpm run test:coverage: 100% statements, branches and lines
• pnpm run ts
• pnpm run build

[devill]

"Fixes #1038" closes an issue this only partly covers

@delucis' comment on #1038 asks for three things: let the developer control who plays, consolidate the three copies of the selection logic (GetBotPlayer in the Local transport, Step, Simulate), and possibly allow selecting more than one active player. This delivers the first for Step and takes the three copies down to two — Simulate still has no way for a caller to name the player, and src/client/transport/local.ts:28 keeps its own copy. (They agree today: both resolve to the lowest player ID present in both activePlayers and bots.)

Keeping the PR narrow is the right call. Could you switch the body to Refs #1038 and add a "Remaining limitations" section, the way you did on #1267? Otherwise merging quietly drops the rest of the issue.

[devill]

This test doesn't reach the branch it's for. TicTacToe sets only minMoves/maxMoves, so ctx.activePlayers is null and the test exits through the state.ctx.currentPlayer fallback — activePlayers.find(...) === undefined never runs. Branch coverage doesn't catch it because .find isn't a branch. (Adding ?? activePlayers[0] to that find leaves all 23 tests green.)

Suggested change

	test('stops when no bot is configured for the active player', async () => {
	const state = InitializeGame({ game: TicTacToe });
	const result = await Simulate({
	game: TicTacToe,
	bots: {},
	state,
	depth: 1,
	});
	expect(result).toEqual({ state, metadata: null });
	});
	test('stops when no bot is configured for the active player', async () => {
	const game = ProcessGameConfig({
	setup: () => ({ movedBy: null }),
	moves: {
	A: ({ G, playerID }) => {
	G.movedBy = playerID;
	},
	},
	turn: { activePlayers: { all: Stage.NULL } },
	});
	const state = InitializeGame({ game, numPlayers: 3 });
	const result = await Simulate({ game, bots: {}, state, depth: 1 });
	expect(result).toEqual({ state, metadata: null });
	});

[devill]

Simulate now stops silently where it used to throw. Simulate({ game, bots: { '1': bot }, state }) on a game whose current player is '0' returns the untouched initial state — indistinguishable from a finished simulation.

Suggested change

	const bot = bots instanceof Bot ? bots : bots[playerID];
	if (!bot) break;
	const bot = bots instanceof Bot ? bots : bots[playerID];
	if (!bot) {
	logging.info(`no bot configured for player ${playerID}, stopping`);
	break;
	}

with import * as logging from '../core/logger'; at the top.

Two nits while you're in here: getBotPlayer can return undefined, so : PlayerID | undefined on the signature; and Simulate's JSDoc still calls bots "an array of bots" — now that its keys decide who moves, that line is worth correcting.

[devill]

Following on from this one — what makes the guard awkward is that bots is Bot | Record<PlayerID, Bot>, told apart by bots instanceof Bot at the point of use. That's a prototype check on an abstract class rather than on the data, and routing Step through the shared helper spreads it to a second call site.

I've pushed devill/simulate-bots-map (603dd4e, on top of your 545d64e) which normalises once at the entry point instead: a single Bot becomes a map covering every seat, and everything downstream works on a plain Record<PlayerID, Bot>. The public signature is unchanged — no caller or test needed touching. One file, +19/−16, 904 tests green, coverage still 100%.

Would you fold it in? Cherry-pick or merge, whichever suits. Happy to drop it if you'd rather keep this PR to the selection fix.

[janmarsino98]

Thanks — this is a cleaner approach. I've folded 603dd4e into the PR.

[devill]

Thanks for folding in 603dd4e. Three points still open from last round:

• Body still says Fixes #1038 — needs Refs, plus the "Remaining limitations" section
  (Simulate's missing caller-supplied player; the third copy at
  src/client/transport/local.ts:28).
• ai.test.ts:257 still initialises with TicTacToe, so the test misses its branch. Confirmed:
  adding ?? Object.keys(state.ctx.activePlayers)[0] to the find leaves all 23 green.
• ai.ts:89 is still a bare break.

The suggestions from last time still apply as-is.

Moving it to draft, please reopen once comments are addressed.