Add system review document from multi-agent code review
Comprehensive review of core engine implementation identifying: - 8 critical issues (energy tracking, knockout processing, confusion handling) - 7 medium priority issues (prize selection, stadium ownership, etc.) - Low priority observations and missing functionality - Recommended fix priority in 3 phases - Test coverage status per module (94% overall)
This commit is contained in:
Cal Corum
parent
fe2e1091f9
commit
8668a0d824
298
backend/SYSTEM_REVIEW.md
Normal file
298
backend/SYSTEM_REVIEW.md
Normal file
@ -0,0 +1,298 @@
|
||||
# Mantimon TCG Core Engine - System Review
|
||||
|
||||
**Date:** 2026-01-25
|
||||
**Reviewed By:** Multi-agent system review
|
||||
**Status:** 738 tests passing, 94% coverage
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
The core engine has a solid foundation with good separation of concerns, comprehensive documentation, and thorough test coverage. However, the review identified **8 critical issues** that must be fixed before production, along with several medium-priority gaps and missing functionality.
|
||||
|
||||
---
|
||||
|
||||
## Critical Issues (Must Fix)
|
||||
|
||||
### 1. Energy Zone Missing from Card Search
|
||||
**File:** `app/core/models/game_state.py:533-540`
|
||||
**Severity:** CRITICAL
|
||||
|
||||
The `find_card_instance` method searches through zones but is missing `energy_zone`. When using Pokemon Pocket-style rules, cards in the energy zone (flipped from energy deck, waiting to be attached) would be unfindable.
|
||||
|
||||
```python
|
||||
# Current - missing energy_zone
|
||||
for zone_name in ["deck", "hand", "active", "bench", "discard", "prizes", "energy_deck"]:
|
||||
```
|
||||
|
||||
**Fix:** Add `"energy_zone"` to the search list.
|
||||
|
||||
---
|
||||
|
||||
### 2. No Validation That Pokemon Cards Have Required Fields
|
||||
**File:** `app/core/models/card.py:163-231`
|
||||
**Severity:** CRITICAL
|
||||
|
||||
`CardDefinition` accepts Pokemon-specific fields as optional (`hp: int | None`, `stage: PokemonStage | None`), but there's no validation that Pokemon cards actually have these fields set. This could lead to runtime errors.
|
||||
|
||||
**Fix:** Add a Pydantic `model_validator` to ensure:
|
||||
- If `card_type == POKEMON`, then `hp`, `stage`, and `pokemon_type` must be set
|
||||
- If `card_type == TRAINER`, then `trainer_type` must be set
|
||||
- If `card_type == ENERGY`, then `energy_type` must be set
|
||||
|
||||
---
|
||||
|
||||
### 3. end_turn() Doesn't Process Knockouts
|
||||
**File:** `app/core/turn_manager.py:312-421`
|
||||
**Severity:** CRITICAL
|
||||
|
||||
`end_turn()` identifies knockouts from status damage and adds them to a `knockouts` list, but it **never moves the KO'd Pokemon to discard**. The comment says "should be handled by the game engine" but the engine also doesn't process them.
|
||||
|
||||
**Impact:** Pokemon knocked out by poison/burn damage remain in play.
|
||||
|
||||
**Fix:** Either:
|
||||
- Process knockouts fully in `turn_manager.end_turn()`
|
||||
- Or ensure `GameEngine.end_turn()` calls `process_knockout()` for each KO
|
||||
|
||||
---
|
||||
|
||||
### 4. Win Condition Checked Before Knockout Processing
|
||||
**File:** `app/core/turn_manager.py:398-401`
|
||||
**Severity:** CRITICAL
|
||||
|
||||
The code checks `no_pokemon_in_play` BEFORE the knockout is actually processed. The KO'd Pokemon is still in `active.cards` at this point.
|
||||
|
||||
**Impact:** Win condition check will always fail because the KO'd Pokemon is still technically "in play".
|
||||
|
||||
**Fix:** Move win condition check to AFTER knockout processing completes.
|
||||
|
||||
---
|
||||
|
||||
### 5. Energy Attachment Bug - Energy Card Disappears
|
||||
**File:** `app/core/engine.py:568`
|
||||
**Severity:** CRITICAL
|
||||
|
||||
The `_execute_attach_energy` method removes the energy card from hand/energy_zone and stores only its `instance_id` in `attached_energy`. The card itself is never stored anywhere - it effectively disappears from the game.
|
||||
|
||||
When `rules_validator.py:254-270` tries to find attached energy via `game.find_card_instance()`, it will fail because the card was never added to any zone.
|
||||
|
||||
**Fix:** Either:
|
||||
- Store energy cards in the player's discard pile when attached (standard TCG behavior)
|
||||
- Or change the data model to track attached energy cards differently
|
||||
|
||||
---
|
||||
|
||||
### 6. Status Knockouts Not Processed by Engine
|
||||
**File:** `app/core/engine.py:858-881`
|
||||
**Severity:** CRITICAL
|
||||
|
||||
`GameEngine.end_turn()` receives knockout information from `TurnManager` but doesn't process them:
|
||||
|
||||
```python
|
||||
def end_turn(self, game: GameState) -> ActionResult:
|
||||
result = self.turn_manager.end_turn(game, self.rng)
|
||||
if result.win_result:
|
||||
apply_win_result(game, result.win_result)
|
||||
return ActionResult(...) # Knockouts not processed!
|
||||
```
|
||||
|
||||
**Fix:** Add knockout processing loop that calls `turn_manager.process_knockout()` for each knockout in `result.knockouts`.
|
||||
|
||||
---
|
||||
|
||||
### 7. Confusion Status Not Handled in Attack
|
||||
**File:** `app/core/engine.py:676-726`
|
||||
**Severity:** CRITICAL
|
||||
|
||||
The `_execute_attack` method doesn't handle the Confusion status condition. Per Pokemon TCG rules, a confused Pokemon must flip a coin before attacking - tails means the attack fails and the Pokemon damages itself.
|
||||
|
||||
The validator (line 781) notes: "Confused Pokemon CAN attempt to attack (may fail at execution time)" but the engine doesn't implement this.
|
||||
|
||||
**Fix:** Add confusion flip logic at the start of `_execute_attack`.
|
||||
|
||||
---
|
||||
|
||||
### 8. Energy Discard Handler Doesn't Move Cards
|
||||
**File:** `app/core/effects/handlers.py:479-517`
|
||||
**Severity:** CRITICAL
|
||||
|
||||
The `discard_energy` handler removes energy from `attached_energy` list but doesn't move the energy CardInstance to the discard pile. Energy cards could be "lost" from the game.
|
||||
|
||||
**Fix:** After detaching energy, add the card instance to the player's discard pile.
|
||||
|
||||
---
|
||||
|
||||
## Medium Priority Issues
|
||||
|
||||
### 9. Per-Ability Usage Tracking Flawed
|
||||
**File:** `app/core/models/card.py:334`
|
||||
|
||||
`CardInstance.ability_uses_this_turn` is a single integer, but a Pokemon can have multiple abilities with different `uses_per_turn` limits. The counter can't distinguish between them.
|
||||
|
||||
**Fix:** Change to `dict[int, int]` tracking uses per ability index.
|
||||
|
||||
---
|
||||
|
||||
### 10. Double Knockout - Only One Forced Action
|
||||
**File:** `app/core/turn_manager.py:478-486`
|
||||
|
||||
If both players' active Pokemon are KO'd simultaneously, only one `forced_action` can be set (last writer wins).
|
||||
|
||||
**Fix:** Support a queue of forced actions or handle simultaneous KOs specially.
|
||||
|
||||
---
|
||||
|
||||
### 11. No SelectPrizeAction Executor
|
||||
**File:** `app/core/engine.py:440-444`
|
||||
|
||||
The `SelectPrizeAction` validator exists but there's no handler in `_execute_action_internal`. Prize card mode is broken.
|
||||
|
||||
**Fix:** Add `_execute_select_prize` method.
|
||||
|
||||
---
|
||||
|
||||
### 12. Stadium Discard Goes to Wrong Player
|
||||
**File:** `app/core/engine.py:608-624`
|
||||
|
||||
When a new stadium replaces an existing one, the old stadium is discarded to the current player's discard pile instead of its owner's.
|
||||
|
||||
**Fix:** Track stadium ownership and discard to correct player.
|
||||
|
||||
---
|
||||
|
||||
### 13. No Knockout Detection After Damage Effects
|
||||
**File:** `app/core/effects/handlers.py`
|
||||
|
||||
Neither `deal_damage` nor `attack_damage` check if the target is knocked out after applying damage.
|
||||
|
||||
**Fix:** Either return a knockout flag in EffectResult or document that knockout checking happens at engine level.
|
||||
|
||||
---
|
||||
|
||||
### 14. Exception Swallowing Hides Errors
|
||||
**File:** `app/core/effects/registry.py:97`
|
||||
|
||||
```python
|
||||
except Exception as e:
|
||||
return EffectResult.failure(f"Effect '{effect_id}' failed: {e}")
|
||||
```
|
||||
|
||||
This catches all exceptions including programming errors, making debugging difficult.
|
||||
|
||||
**Fix:** Log full traceback at ERROR level, or only catch expected exceptions.
|
||||
|
||||
---
|
||||
|
||||
### 15. Turn Limit Not Checked at Turn Start
|
||||
**File:** `app/core/engine.py:828-856`
|
||||
|
||||
`TurnManager.check_turn_limit()` exists but `GameEngine.start_turn()` doesn't call it.
|
||||
|
||||
**Fix:** Add turn limit check before calling `start_turn`.
|
||||
|
||||
---
|
||||
|
||||
## 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)
|
||||
1. Fix `find_card_instance` to include `energy_zone`
|
||||
2. Fix energy attachment to store cards properly
|
||||
3. Fix knockout processing in `end_turn`
|
||||
4. Fix win condition check timing
|
||||
5. Add confusion handling in attack execution
|
||||
|
||||
### Phase 2: Functionality Gaps (Before Feature Complete)
|
||||
6. Add `SelectPrizeAction` executor
|
||||
7. Fix stadium discard ownership
|
||||
8. Add turn limit check
|
||||
9. Fix per-ability usage tracking
|
||||
|
||||
### Phase 3: Polish (Before Production)
|
||||
10. Add CardDefinition field validation
|
||||
11. Handle double knockouts
|
||||
12. Improve effect error handling
|
||||
13. 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 |
|
||||
| `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%** | |
|
||||
|
||||
---
|
||||
|
||||
## Next Steps
|
||||
|
||||
1. Review this document and prioritize fixes
|
||||
2. Create GitHub issues for critical items
|
||||
3. Address Phase 1 fixes before continuing development
|
||||
4. Update tests as fixes are implemented
|
||||
5. Re-run system review after fixes
|
||||
Loading…
Reference in New Issue
Block a user