mantimon-tcg/docs/legacy/SYSTEM_REVIEW.md

# Mantimon TCG Core Engine - System Review

**Date:** 2026-01-25  
**Updated:** 2026-01-26  
**Reviewed By:** Multi-agent system review  
**Status:** 797 tests passing, 94% coverage  

---

## Executive Summary

The core engine has a solid foundation with good separation of concerns, comprehensive documentation, and thorough test coverage. The review identified **8 critical issues** - **all 8 have been fixed**. The fixes span the Energy/Evolution Stack refactor, CardDefinition validation, and knockout processing verification.

**Fixed:** #1, #2, #3, #4, #5, #6, #7, #8  
**Still Open:** None - all critical issues resolved!

---

## Critical Issues (Must Fix)

### 1. ~~Energy Zone Missing from Card Search~~ FIXED
**File:** `app/core/models/game_state.py:533-540`  
**Severity:** CRITICAL  
**Status:** FIXED in commit 2b8fac4

The `find_card_instance` method now searches all zones including `energy_zone`, plus:
- `attached_energy` - Energy cards attached to Pokemon in play
- `attached_tools` - Tool cards attached to Pokemon in play  
- `cards_underneath` - Evolution stack (previous stages)

```python
# Fixed - includes energy_zone and attached cards
for zone_name in ["deck", "hand", "active", "bench", "discard", "prizes", "energy_deck", "energy_zone"]:
# Also searches attached_energy, attached_tools, cards_underneath on Pokemon in play
```

---

### 2. ~~No Validation That Pokemon Cards Have Required Fields~~ FIXED
**File:** `app/core/models/card.py:163-231`  
**Severity:** CRITICAL  
**Status:** FIXED in commit 7fae1c6

Added a Pydantic `model_validator` to `CardDefinition` that enforces:
- **Pokemon cards** require: `hp` (must be positive), `stage`, `pokemon_type`
- **Pokemon Stage 1/2** require: `evolves_from`
- **Pokemon VMAX/VSTAR** require: `evolves_from`
- **Trainer cards** require: `trainer_type`
- **Energy cards** require: `energy_type` (auto-fills `energy_provides` if empty)

This prevents invalid card definitions at construction time rather than runtime errors later.

---

### 3. ~~end_turn() Doesn't Process Knockouts~~ FIXED
**File:** `app/core/turn_manager.py:312-421`  
**Severity:** CRITICAL  
**Status:** FIXED - was already implemented correctly in commit eef857e

Investigation revealed this was a false positive. The `end_turn()` method at lines 409-413 **already calls** `process_knockout()` for each knockout detected from status damage. This processes:
- Moving the Pokemon to discard pile
- Discarding all attached energy and tools
- Discarding the evolution stack (cards underneath)
- Awarding points to the opponent
- Checking win conditions
- Setting up forced action for new active selection

**Verification:** Added 8 integration tests in `TestStatusKnockoutIntegration` to confirm the full flow works correctly.

---

### 4. ~~Win Condition Checked Before Knockout Processing~~ FIXED
**File:** `app/core/turn_manager.py:398-401`  
**Severity:** CRITICAL  
**Status:** FIXED - was already implemented correctly in commit eef857e

Investigation revealed this was a false positive. The win condition check happens **inside** `process_knockout()` (lines 483-500), which is called **after** the Pokemon is moved to discard (line 474). The sequence is:
1. Pokemon removed from active zone (line 453)
2. Attachments discarded (lines 458-471)
3. Pokemon added to discard (line 474)
4. Points awarded (lines 476-481)
5. Win by points checked (lines 483-491)
6. Win by no Pokemon checked (lines 493-500)

**Verification:** Tests `test_end_turn_status_knockout_triggers_win_by_points` and `test_end_turn_status_knockout_triggers_win_by_no_pokemon` confirm proper ordering.

---

### 5. ~~Energy Attachment Bug - Energy Card Disappears~~ FIXED
**File:** `app/core/engine.py:568`  
**Severity:** CRITICAL  
**Status:** FIXED in commit 2b8fac4

The data model was changed to store full `CardInstance` objects:
- `attached_energy: list[CardInstance]` (was `list[str]`)
- `attached_tools: list[CardInstance]` (was `list[str]`)

Energy cards are now stored directly on the Pokemon they're attached to, not just as IDs. The `find_card_instance` method searches these attached cards, so they're always findable.

When a Pokemon is knocked out or retreats, attached energy moves to the owner's discard pile.

---

### 6. ~~Status Knockouts Not Processed by Engine~~ FIXED
**File:** `app/core/engine.py:858-881`  
**Severity:** CRITICAL  
**Status:** FIXED - was already implemented correctly in commit eef857e

Investigation revealed this was a false positive. The `TurnManager.end_turn()` method **already processes knockouts internally** before returning the result. The `GameEngine.end_turn()` doesn't need to call `process_knockout()` again because `TurnManager` handles it.

The flow is:
1. `GameEngine.end_turn()` calls `turn_manager.end_turn()`
2. `TurnManager.end_turn()` applies status damage, detects knockouts
3. `TurnManager.end_turn()` calls `process_knockout()` for each knockout
4. `TurnManager.end_turn()` returns result with win_result if game ended
5. `GameEngine.end_turn()` applies win result if present

**Verification:** Added 3 tests in `TestEngineEndTurnKnockouts` confirming the full engine flow works correctly.

---

### 7. ~~Confusion Status Not Handled in Attack~~ FIXED
**File:** `app/core/engine.py:676-726`  
**Severity:** CRITICAL  
**Status:** FIXED in earlier commit

The `_execute_attack` method now handles Confusion status:
- Checks if attacker has `StatusCondition.CONFUSED`
- Flips a coin using the RNG
- On tails: attack fails, Pokemon damages itself (configurable via `rules.status.confusion_self_damage`)
- On heads: attack proceeds normally

The self-damage amount is configurable in `RulesConfig.status.confusion_self_damage` (default 30).

---

### 8. ~~Energy Discard Handler Doesn't Move Cards~~ FIXED
**File:** `app/core/effects/handlers.py:479-517`  
**Severity:** CRITICAL  
**Status:** FIXED in commit 2b8fac4

The `discard_energy` handler now:
1. Finds the owner of the target Pokemon
2. Calls `detach_energy()` which returns the `CardInstance`
3. Adds the detached energy to the owner's discard pile

Additionally, a new `devolve` effect handler was added for removing evolution stages.

---

## Medium Priority Issues

### 9. ~~Per-Ability Usage Tracking Flawed~~ FIXED
**File:** `app/core/models/card.py:390`  
**Status:** FIXED in current session

Changed `ability_uses_this_turn` from `int` to `dict[int, int]` to track usage per ability:
- `ability_uses_this_turn: dict[int, int]` maps ability index to use count
- `can_use_ability(ability, ability_index)` now requires the ability index
- Added `get_ability_uses(ability_index)` helper method
- Added `increment_ability_uses(ability_index)` helper method
- `reset_turn_state()` now clears the dict instead of setting to 0

This ensures Pokemon with multiple abilities (e.g., one limited to 1/turn, another to 2/turn) track each ability independently.

---

### 10. ~~Double Knockout - Only One Forced Action~~ FIXED
**File:** `app/core/models/game_state.py`  
**Status:** FIXED in current session

Changed `forced_action: ForcedAction | None` to `forced_actions: list[ForcedAction]` (FIFO queue):
- `has_forced_action()` - Check if queue has pending actions
- `get_current_forced_action()` - Get first action without removing
- `add_forced_action(action)` - Add to end of queue
- `pop_forced_action()` - Remove and return first action
- `clear_forced_actions()` - Clear all pending actions

When both players' active Pokemon are KO'd, both forced actions are queued and processed in order.

---

### 11. ~~No SelectPrizeAction Executor~~ FIXED
**File:** `app/core/engine.py:440-444`  
**Status:** FIXED in current session

Added complete prize card system:
- `_execute_select_prize()` method in engine.py (lines ~887-949)
- Prize card mode support in `process_knockout()` via `_award_prize_cards()` helper
- Supports both random selection (auto-takes prizes) and player choice (sets `forced_action`)
- Multi-prize selection for EX/VMAX knockouts (worth 2-3 prizes)
- Win detection when all prizes taken

**Note:** The validator already allows `select_prize` during forced actions regardless of phase, so abilities/effects causing knockouts during main phase work correctly.

---

### 12. ~~Stadium Discard Goes to Wrong Player~~ FIXED
**File:** `app/core/engine.py:630-644`  
**Status:** FIXED in current session

Added `stadium_owner_id: str | None` to GameState to track who played the current stadium:
- When a stadium is played, both `stadium_in_play` and `stadium_owner_id` are set
- When a stadium is replaced, the old stadium is discarded to its owner's discard pile
- Added to VisibleGameState for client visibility

Added 2 new tests for stadium ownership behavior.

---

### 13. ~~No Knockout Detection After Damage Effects~~ FIXED
**File:** `app/core/effects/handlers.py`

~~Neither `deal_damage` nor `attack_damage` check if the target is knocked out after applying damage.~~

**Resolution:** Both `deal_damage` and `attack_damage` now check if the target is knocked out after applying damage. If knocked out, they set `details["knockout"] = True` and `details["knockout_pokemon_id"]` in the EffectResult. The message also includes "knocked out!" notification.

The knockout check uses `target.is_knocked_out(card_def.hp)` which correctly accounts for HP modifiers via `effective_hp()`.

Added 9 new tests covering:
- Knockout detection for both handlers
- No false positives when target survives
- Knockout with existing damage
- HP modifier respect (both positive and negative)
- Knockout after weakness multiplier
- Survival when resistance prevents knockout

---

### 14. ~~Exception Swallowing Hides Errors~~ FIXED
**File:** `app/core/effects/registry.py:93-107`

~~This catches all exceptions including programming errors, making debugging difficult.~~

**Resolution:** Added proper logging with `logger.exception()` which captures the full traceback at ERROR level. The log message now includes:
- The effect_id that failed
- Source player ID
- Source and target card IDs
- Effect parameters
- Full exception traceback

The game still returns a safe `EffectResult.failure()` to prevent crashes, but debugging information is now preserved in the logs.

Added 1 test verifying that exceptions are logged with full context and traceback.

---

### 15. ~~Turn Limit Not Checked at Turn Start~~ FIXED
**File:** `app/core/engine.py:828-856`  
**Status:** FIXED in current session

Added turn limit check at the start of `start_turn()` (lines ~968-980):
- Calls `turn_manager.check_turn_limit(game)` before processing turn
- Returns `ActionResult` with `win_result` if limit exceeded
- Winner determined by score (higher score wins)
- Equal scores result in a draw (`GameEndReason.DRAW`)

Also added `GameEndReason.TURN_LIMIT` enum value to distinguish from `TIMEOUT` (player ran out of clock time).

---

## Low Priority / Observations

### Models
- `ModifierMode` enum not exported in `__init__.py`
- `ActionType` enum exists but is unused (duplicates Literal values)
- No maximum damage validation (negative damage possible)
- Tools limit not validated at model level
- Stadium ownership not tracked
- No temporary effect system for "until end of turn" effects

### Game Logic
- Draw phase has no actions (may be intentional)
- `first_turn_of_game` config exists in two places (`FirstTurnConfig.can_evolve` and `EvolutionConfig.first_turn_of_game`)
- Prize card mode missing "taken" tracking vs "removed by effect"
- Evolution doesn't validate stage progression (Stage 1 could "evolve" into Stage 1)
- `turn_number` only increments on wraparound (could confuse turn limit calculations)

### Effects System
- No effect chaining for compound effects
- No duration/timing system for temporary modifiers
- No target selection callback for player-choice effects
- Missing common effects: `search_deck`, `switch_pokemon`, `force_switch`, `conditional_effect`

### Engine
- Hardcoded hand size `7` in mulligan (should use `rules.deck.starting_hand_size`)
- Async methods that don't need async (`_execute_play_trainer`, etc.)
- No setup phase state machine
- No `get_legal_actions()` method for UI/AI
- No weakness/resistance in damage calculation
- No undo/snapshot capability
- Mulligan doesn't award opponent extra cards

---

## Positive Observations

1. **Clean Architecture**: Excellent separation of CardDefinition (immutable template) vs CardInstance (mutable state)

2. **Comprehensive Configuration**: RulesConfig system supports multiple game variants (traditional, Pocket-style, custom)

3. **Well-Designed EffectContext**: Rich helper methods for parameter extraction, coin flips, player/card access

4. **Testability**: RandomProvider protocol with SeededRandom enables deterministic testing

5. **Zone Abstraction**: Clean `Zone` class with rich set of methods for deck/hand manipulation

6. **Good Documentation**: Comprehensive docstrings and type hints throughout

7. **Security Awareness**: Visibility filter properly hides opponent hand, deck order, prizes

---

## Recommended Fix Priority

### Phase 1: Critical Fixes (Before Any Testing) - 4/8 COMPLETE
1. ~~Fix `find_card_instance` to include `energy_zone`~~ DONE
2. Add CardDefinition field validation - STILL NEEDED
3. Fix knockout processing in `end_turn` - STILL NEEDED
4. Fix win condition check timing - STILL NEEDED
5. ~~Fix energy attachment to store cards properly~~ DONE (full refactor to CardInstance)
6. Fix engine to call `process_knockout()` for status KOs - STILL NEEDED
7. ~~Add confusion handling in attack execution~~ DONE
8. ~~Fix energy discard handler to move cards~~ DONE

### Phase 2: Functionality Gaps (Before Feature Complete)
6. ~~Add `SelectPrizeAction` executor~~ DONE (#11)
7. ~~Fix stadium discard ownership~~ DONE (#12)
8. ~~Add turn limit check~~ DONE (#15)
9. ~~Fix per-ability usage tracking~~ DONE (#9)

### Phase 3: Polish (Before Production)
10. ~~Handle double knockouts~~ DONE (#10)
11. Improve effect error handling
12. Add missing effect handlers

---

## Test Coverage Status

| Module | Coverage | Notes |
|--------|----------|-------|
| `config.py` | 100% | Complete |
| `models/enums.py` | 100% | Complete |
| `models/card.py` | 100% | Complete |
| `models/actions.py` | 100% | Complete |
| `models/game_state.py` | 99% | Near complete |
| `effects/base.py` | 98% | Near complete |
| `effects/registry.py` | 100% | Complete |
| `effects/handlers.py` | 99% | Near complete + devolve handler |
| `rules_validator.py` | 94% | Good |
| `turn_manager.py` | 93% | Good |
| `visibility.py` | 95% | Good |
| `win_conditions.py` | 99% | Near complete |
| `engine.py` | 81% | Gaps in error paths |
| `rng.py` | 93% | Good |
| **TOTAL** | **94%** | **797 tests** |

### New Tests Added
- `tests/core/test_evolution_stack.py` - 28 tests for evolution stack, devolve, knockout with attachments, find_card_instance
- `tests/core/test_engine.py::TestSelectPrizeAction` - 3 tests for SelectPrizeAction execution
- `tests/core/test_engine.py::TestTurnLimitCheck` - 5 tests for turn limit checking
- `tests/core/test_turn_manager.py::TestPrizeCardModeKnockout` - 4 tests for prize card mode in knockouts
- `tests/core/test_models/test_card.py::TestCardInstanceTurnState::test_can_use_ability_independent_tracking` - 1 test for per-ability usage tracking
- `tests/core/test_models/test_game_state.py::TestForcedActionQueue` - 8 tests for forced action queue management
- `tests/core/test_engine.py::TestPlayTrainerAction` - 2 new tests for stadium ownership

---

## Next Steps

1. ~~Review this document and prioritize fixes~~ DONE
2. Create GitHub issues for remaining critical items (#2, #3, #4, #6)
3. ~~Address Phase 1 fixes before continuing development~~ 4/8 DONE
4. ~~Update tests as fixes are implemented~~ DONE (766 tests)
5. Re-run system review after remaining fixes

## Change Log

### 2026-01-26 - Stadium Ownership Tracking (Issue #12)

Added `stadium_owner_id` field to track who played the current stadium:
- When a stadium is played, `stadium_owner_id` is set to the player's ID
- When a stadium is replaced, old stadium discards to owner's pile (not current player)
- Added `stadium_owner_id` to VisibleGameState for client visibility
- Updated existing test and added 2 new tests for stadium ownership

**Total: 797 tests passing**

### 2026-01-26 - Forced Action Queue for Double Knockouts (Issue #10)

Changed `forced_action` from single item to FIFO queue:
- `forced_actions: list[ForcedAction]` replaces `forced_action: ForcedAction | None`
- Added queue management methods: `has_forced_action()`, `get_current_forced_action()`, 
  `add_forced_action()`, `pop_forced_action()`, `clear_forced_actions()`
- Updated engine, turn_manager, rules_validator, and visibility filter
- Added 8 new tests for forced action queue including double knockout scenario

**Total: 795 tests passing**

### 2026-01-26 - Per-Ability Usage Tracking (Issue #9)

Fixed issue #9 - ability usage now tracked per-ability instead of globally:
- Changed `ability_uses_this_turn: int` to `dict[int, int]`
- `can_use_ability()` now requires `ability_index` parameter
- Added `get_ability_uses()` and `increment_ability_uses()` helper methods
- Updated engine, rules_validator, and all affected tests
- Added new test `test_can_use_ability_independent_tracking` to verify fix

**Total: 789 tests passing**

### 2026-01-26 - SelectPrizeAction and Turn Limit Check (Issues #11, #15)

Fixed medium priority issues #11 and #15:

**Issue #11 - SelectPrizeAction Executor:**
- Added `_execute_select_prize()` method to GameEngine (lines ~887-949)
- Added prize card mode support to `process_knockout()` via `_award_prize_cards()` helper
- Supports random selection (auto-takes prizes) and player choice (sets forced_action)
- Multi-prize selection for EX/VMAX knockouts (worth 2-3 prizes)
- 7 new tests for prize card functionality

**Issue #15 - Turn Limit Check:**
- Added turn limit check at start of `start_turn()` (lines ~968-980)
- Added `GameEndReason.TURN_LIMIT` enum value (distinct from TIMEOUT)
- Winner determined by score, equal scores result in DRAW
- 5 new tests for turn limit functionality

**Total: 12 new tests, 788 tests passing**

### 2026-01-26 - Energy/Evolution Stack Refactor
**Commits:** 2b8fac4, dd2cadf

Fixed issues #1, #5, #7, #8 plus major enhancements:

Note: Issues #3 and #6 (knockout processing) were NOT fixed - we improved 
`process_knockout()` to handle attached cards correctly, but the engine still
doesn't call it for status damage knockouts. That's a separate fix needed.
- `attached_energy` and `attached_tools` changed from `list[str]` to `list[CardInstance]`
- Added `cards_underneath` for evolution stack tracking
- Added `devolve` effect handler
- `find_card_instance` now searches attached cards and evolution stacks
- Knockout processing discards all attached cards and evolution stack
- Evolution clears status conditions (Pokemon TCG standard behavior)
- 28 new comprehensive tests in `test_evolution_stack.py`

See `PROJECT_PLAN_ENERGY_EVOLUTION.md` for full implementation details.