Critical Bug: Game Logic Flaw In Move Execution

Nov 8, 2025 by Admin 48 views

Understanding the Core Issue: Dual Sources of Truth

Hey guys, let's dive deep into a critical issue we've uncovered in the game's architecture. We've got a serious problem on our hands, specifically a Single Source of Truth (SSOT) violation. In simpler terms, the game uses two different sources of information for what you see on the screen (the display) and what actually happens when you make a move (the execution). This discrepancy is causing moves to be executed incorrectly, and it's likely the root cause of some nasty bugs we've been experiencing.

This isn't just a minor glitch; it's a fundamental architectural flaw. It's like having two different rulebooks for the same game, and the game is randomly picking which rulebook to follow. This is not good, and it needs to be addressed ASAP. The main focus is to ensure that the game’s core mechanics work as expected.

The Broken Architecture: A Step-by-Step Breakdown

Let's break down how the current system works, and where things go wrong:

engine.getValidMoves(state) → validMoves: The game engine figures out all the legal moves you can make, based on the current state of the game. This is Source A.
generateMoveOptions(state, validMoves) → MoveOption[]: This function is supposed to create the options you see on the screen. However, it ignores the validMoves from Step 1 and generates its own options from the game's state. This is Source B.
Display options from MoveOption[]: The game displays the options generated in Step 2, which come from Source B.
User selects option 6: You, the player, choose an option from what you see on the screen.
Validate against validMoves.length: The game checks if your selection is valid by comparing it to the number of valid moves, which comes from Source A.
executeMove(validMoves[5]): The game executes your move based on the validMoves array, which comes from Source A.

The Problem: The display (Source B) shows options that might not match what the engine (Source A) actually considers valid. When you select an option, the game might execute a completely different move than what you intended, leading to all sorts of chaos.

Where the Wheels Fall Off: Evidence of the Problem

The code shows us exactly where this disconnect happens.

Location 1: generateMoveOptions is Ignoring Valid Moves

In the file packages/core/src/presentation/move-options.ts, we see that the function generateMoveOptions is supposed to take the validMoves as a parameter, but it completely ignores it. Instead, it generates options based solely on the current game state. This means the display is showing options that might not be actually valid, and the engine is using a different set of rules to determine what's legal.

This is a huge red flag because it breaks the fundamental principle of a single source of truth. We need to have one source for valid moves, and the display and execution should both be using it.

Location 2: The CLI is Caught in the Crossfire

In the packages/cli/src/cli.ts file, the command-line interface (CLI) is trying to handle pending effects (like choosing which card to discard). It gets the validMoves from the engine (Source A) to determine what moves are legal, but then it uses generateMoveOptions (Source B) to display the options to the user. Finally, it executes the move based on the validMoves from Source A.

This creates a huge potential for a mismatch. The user sees one set of options, but the engine executes a different set. This is where the bugs start to creep in.

The Impact: Bugs in Action

This architecture flaw is not just a theoretical problem; it's causing real bugs that are ruining the game experience.

Bug #13: "End Phase" Executes "Play Copper"

Imagine this scenario:

The generateMoveOptions function returns (and displays to the user):

MoveOption[] = [
  { index: 1, description: "Buy Copper", ... },
  { index: 2, description: "Buy Curse", ... },
  ...
  { index: 6, description: "End Phase", ... }
]

But the validMoves array (used for execution) contains:

Move[] = [
  buyCopper,
  buyCurse,
  ...
  playCopper,  // At index 5
  endPhase     // At index 6
]

The user selects option 6, which is "End Phase". The game then executes validMoves[5], which is playCopper – completely the wrong move! This is because the display and execution are out of sync.

Bug #12: Remodel Option 4 Rejected

Another example:

The display shows three options:

MoveOption[] = [opt1, opt2, opt3]

But the validMoves array has four valid moves:

Move[] = [move1, move2, move3, move4]

The user selects option 4 (which they see on the screen). The game checks if the selection is valid. Since the selection is not greater than the valid moves length, it should pass. However, because the display only shows three options, the user is confused, and the game might reject a valid move.

These are just two examples of how this architecture flaw can lead to frustrating and incorrect gameplay.

The Root Cause: Why Did This Happen?

The root cause of this problem is that the generateMoveOptions function was designed to generate options independently of the validMoves. It was intended to provide formatted display text. The CLI incorrectly assumed that it was the authoritative source of options. This design violates the Single Source of Truth principle. The presentation layer, while providing helpful formatting, has been given too much authority, leading to the inconsistencies we are seeing.

Possible Solutions: How Do We Fix This?

We have a few options for fixing this critical issue:

Option A: Fix generateMoveOptions to Use validMoves

export function generateMoveOptions(
  state: GameState,
  validMoves: readonly Move[]  // ← Remove underscore, actually use it!
): MoveOption[] {
  // Transform validMoves into MoveOption[] with formatted descriptions
  return validMoves.map((move, index) => ({
    index: index + 1,
    move: move,
    description: formatMoveDescription(move, state),
    details: {}
  }));
}

Pros: Guarantees that the display matches execution.

Cons: Requires refactoring of all generator functions.

Option B: Change Execution to Use MoveOption[]

// In handlePendingEffect
const options = generateMoveOptions(state, validMoves);
this.display.displayPendingEffectPrompt(options);

// Execute using MoveOption
const selectedOption = options[selection - 1];
const result = this.engine.executeMove(state, selectedOption.move);

Pros: Simpler change, uses a single source (generateMoveOptions).

Cons: Still doesn't use validMoves from the engine.

Option C: Remove generateMoveOptions Entirely

// Display validMoves directly, format in the display layer
displayPendingEffectPrompt(validMoves: Move[]): void {
  validMoves.forEach((move, index) => {
    const description = this.formatMoveDescription(move);
    console.log(`  [${index + 1}] ${description}`);
  });
}

Pros: Simplest solution, guaranteed SSOT.

Cons: Loses the rich formatting provided by generateMoveOptions.

The Recommended Fix: What Should We Do?

We recommend Option B. This approach involves changing the execution to use MoveOption[] as the single source of truth. It's the most straightforward way to address the issue while preserving the rich formatting of the options.

Rationale:

Minimal code changes.
Preserves the rich formatting provided by generateMoveOptions.
Guarantees that the display and execution use the same array.
We can still validate against the array length.

Implementation Steps: How to Put the Fix in Place

Here are the steps we need to take to implement the fix:

Update CLI to Use MoveOption[] for Execution: Store the options from generateMoveOptions. Execute options[selection - 1].move instead of validMoves[selection - 1].

Add Runtime Validation: Add the following validation code to ensure that the lengths of the options and validMoves arrays always match:

if (options.length !== validMoves.length) {
  console.error('CRITICAL: Display/execution array mismatch!');
  console.error(
    `  Options: ${options.length}, ValidMoves: ${validMoves.length}`
  );
  throw new Error('SSOT violation detected');
}

Add Unit Tests: Create unit tests to verify that the display and execution arrays match. Test all interactive cards (Cellar, Chapel, Remodel, etc.) and various hand sizes (1-6 cards).

Test Scenario: Verifying Bug #13 Fix

Here's how we'll create a test to confirm that Bug #13 is fixed:

test('Bug #13: End Phase executes correct move', async () => {
  // Setup buy phase with multiple options
  const state = createBuyPhaseState();
  const validMoves = engine.getValidMoves(state);

  // Find "End Phase" in display
  const options = generateMoveOptions(state, validMoves);
  const endPhaseIndex = options.findIndex(o => o.description.includes('End Phase'));

  // Execute "End Phase"
  const result = engine.executeMove(state, options[endPhaseIndex].move);

  // Verify phase changed (not "played Copper")
  expect(result.newState.phase).not.toBe('buy');
  expect(result.message).toContain('Phase ended');
  expect(result.message).not.toContain('played Copper');
});

Files to Modify: Where the Changes Will Happen

Here's a list of the files we need to update:

packages/cli/src/cli.ts - Update handlePendingEffect.
packages/cli/src/display.ts - Update displayPendingEffectPrompt signature.
packages/cli/tests/pending-effect-consistency.test.ts - Add new tests.

Related Issues: What Else Is This Related To?

This issue is connected to several other issues in the game, including:

#17 - Audit: Move Validation Consistency (the parent issue).
#13 - Buy Phase: 'End Phase' option executes 'Play Copper' instead.
#12 - Remodel: Input validation rejects valid option 4.
#10 - Bureaucrat: Invalid input validation.

Priority: How Important Is This?

This issue is CRITICAL. It's a fundamental architectural flaw that directly causes incorrect move execution. This must be fixed before we add more interactive cards.

Audit Reference: Where to Find More Details

For a complete overview, see the full audit report: .claude/sessions/2025-11-08/audit-issue-17-011CUvzbKQ2dQRXHtmvH7rMr.md