feat: hand history logging, tournament series, MTT consolidation, Meerkat API
Major changes: - Add hand history module (iPoker format) with HandHistoryLogger - Add run_sng_series() for multi-tournament aggregate statistics - Fix MTT table consolidation deadlock (sub-minimum-player tables) - Add Meerkat-compatible convenience methods on Action and GameInfo - Add custom blind levels, prize structures, auto-rebuy to GameConfig - Add SngSeriesResult/SngPlayerSeriesResult for series tracking - Add Tournament::winner(), tables_mut(), shuffle_seats() Review fixes: - Fix TOML prizes field ordering (must precede [[game.blind_levels]]) - Add sensible serde defaults for small_blind (10) and big_blind (20) - Fix prize assignment in run_sng_series (use raw_placement directly) - Handle hand history filename collisions with suffixed retry - Add debug_assert guard on collect_hand_history_data double-consumption
This commit is contained in:
parent
9dfd9c3ee3
commit
c041cde3fe
18 changed files with 4601 additions and 208 deletions
193
plans/meerkat-alignment-fixes.md
Normal file
193
plans/meerkat-alignment-fixes.md
Normal file
|
|
@ -0,0 +1,193 @@
|
|||
# Meerkat API Alignment — Bug Fixes and Improvements Plan
|
||||
|
||||
## Executive Summary
|
||||
|
||||
After a thorough comparison of the Rust `holdem_core` implementation against the Java Meerkat API reference (`Meerkat-API`), I identified several bugs, behavioral discrepancies, and missing features. The core architecture is solid and well-structured, but there are concrete issues that need fixing for correct poker gameplay.
|
||||
|
||||
---
|
||||
|
||||
## Bug Fixes
|
||||
|
||||
### BUG-1: FixedLimit raise validation uses `small_bet` for all streets
|
||||
|
||||
**File:** [`action.rs`](holdem_core/src/game/action.rs:173)
|
||||
|
||||
**Problem:** In `validate_action()`, the `FixedLimit` branch always compares `raise_increment` against `small_bet`, even on turn/river streets where `big_bet` should be used. The validator doesn't know the current street.
|
||||
|
||||
```rust
|
||||
// Current (WRONG for turn/river):
|
||||
BetLimit::FixedLimit { small_bet, big_bet: _ } => {
|
||||
if raise_increment != *small_bet { ... }
|
||||
}
|
||||
```
|
||||
|
||||
**Fix:** Pass the current `Street` to `validate_action()` so it can use the correct bet size. On preflop/flop use `small_bet`, on turn/river use `big_bet`.
|
||||
|
||||
**Impact:** Fixed-limit games on turn/river incorrectly reject valid raises or accept invalid ones.
|
||||
|
||||
---
|
||||
|
||||
### BUG-2: Pot-limit `max_raise_amount` calculation is wrong
|
||||
|
||||
**File:** [`betting.rs`](holdem_core/src/game/betting.rs:106)
|
||||
|
||||
**Problem:** The pot-limit max raise formula returns `pot + call_amount`, but the standard pot-limit formula should be:
|
||||
|
||||
```
|
||||
max_raise_total = pot + call_amount + current_bet
|
||||
```
|
||||
|
||||
Wait, let me re-examine. The trait says it returns the max raise *amount*, but the semantics are unclear. In standard pot-limit:
|
||||
- A player can raise up to: `pot_size + amount_to_call`
|
||||
- Where `pot_size` is the pot BEFORE the player's call
|
||||
- So the max *total* they can put in is: `current_bet + call_amount + pot_size`
|
||||
|
||||
The current code returns `pot + call_amount` which is ambiguous. The caller needs to know whether this is the raise increment or the total. This needs clarification and the actual pot-limit cap needs to be enforced in `HandRunner`.
|
||||
|
||||
**Fix:** Clarify the semantics of `max_raise_amount` — it should return the maximum *total* wager for the player (not just the increment). Then enforce it in `validate_action` or `apply_action`.
|
||||
|
||||
---
|
||||
|
||||
### BUG-3: FixedLimit `max_raise_amount` returns `None`
|
||||
|
||||
**File:** [`betting.rs`](holdem_core/src/game/betting.rs:112)
|
||||
|
||||
**Problem:** For `FixedLimit`, `max_raise_amount()` returns `None`, meaning "no cap." But Fixed-Limit definitely has a cap — the fixed bet size. The comment even acknowledges this confusion.
|
||||
|
||||
**Fix:** Return `Some(fixed_bet_size)` for the current street. This requires passing `street` to the method, or restructuring the trait.
|
||||
|
||||
---
|
||||
|
||||
### BUG-4: All-in that doesn't exceed current bet doesn't reopen action
|
||||
|
||||
**File:** [`hand_runner.rs`](holdem_core/src/game/hand_runner.rs:597)
|
||||
|
||||
**Problem:** When a player goes all-in for less than the current bet, the code calls `br.apply_call()` and does NOT reset `has_acted_this_round` for other players. However, consider this scenario:
|
||||
|
||||
- Player A bets 100
|
||||
- Player B raises to 200
|
||||
- Player C goes all-in for 150 (doesn't exceed 200)
|
||||
- Player D has already acted and called the 200
|
||||
|
||||
In this case, Player D doesn't need to act again, which is correct. BUT if Player C's all-in somehow changes the action flow, there could be issues. Actually, this behavior is correct for standard poker rules — an all-in that doesn't exceed the current bet does NOT reopen the betting. So this is actually fine.
|
||||
|
||||
**Revised:** This is NOT a bug. The current behavior is correct.
|
||||
|
||||
---
|
||||
|
||||
### BUG-5: `run_to_completion` may double-credit winnings
|
||||
|
||||
**File:** [`hand_runner.rs`](holdem_core/src/game/hand_runner.rs:968)
|
||||
|
||||
**Problem:** When `step()` resolves the hand via `resolve_last_player_wins()`, it calls `finalize_result()` which credits winnings to stacks and sets state to `Complete`. Then `run_to_completion()` enters the `Complete` branch and calls `distribute()` again, computing chip movements from a second distribution. However, `distribute()` only reads pot state (doesn't mutate), and `credit_winnings()` is NOT called again in the `Complete` branch. So the stacks are only credited once.
|
||||
|
||||
Wait, let me re-read more carefully...
|
||||
|
||||
In `resolve_last_player_wins()`:
|
||||
1. Calls `finalize_result()` which credits winnings, sets state to Complete
|
||||
2. Returns `Ok(HandResult)` but the result is discarded by `step()` which only returns `HandState`
|
||||
|
||||
Then in `run_to_completion()`:
|
||||
- The `Complete` branch reconstructs a `HandResult` but does NOT call `credit_winnings()` again
|
||||
- It only computes `chip_movements` for the result struct
|
||||
|
||||
So actually the stacks are only credited once (in `finalize_result`), but the `HandResult` constructed in the `Complete` branch of `run_to_completion` calls `distribute()` again, which returns a fresh result from the pot manager. This is fine because `distribute` is read-only.
|
||||
|
||||
**Revised:** This is NOT a double-credit bug. The code is correct but slightly confusing. The `Complete` branch in `run_to_completion` should ideally reuse the result from `finalize_result` rather than reconstructing it, but functionally it's correct.
|
||||
|
||||
---
|
||||
|
||||
### BUG-6: Missing `game_state_changed` callback
|
||||
|
||||
**File:** [`player.rs`](holdem_core/src/game/player.rs)
|
||||
|
||||
**Problem:** The Meerkat API has a `gameStateChanged()` callback that is called after an action event has been fully processed. The Rust implementation does not have this callback. In Meerkat, the sequence is:
|
||||
|
||||
1. `actionEvent(pos, act)` — the action is observed
|
||||
2. `gameStateChanged()` — the game info state has been updated
|
||||
|
||||
This is important because in Meerkat, `actionEvent` is called BEFORE the game state is updated, and `gameStateChanged` is called AFTER. In the Rust implementation, `action_event` is called with an already-updated `GameInfo`, which is actually better. But the `gameStateChanged` callback is missing.
|
||||
|
||||
**Fix:** This is a minor API gap. The current Rust design is arguably better (providing updated state in `action_event`), so this is low priority. Could add a `game_state_changed` method with a default no-op implementation.
|
||||
|
||||
---
|
||||
|
||||
### BUG-7: `validate_action` doesn't validate FixedLimit raise count cap
|
||||
|
||||
**File:** [`action.rs`](holdem_core/src/game/action.rs)
|
||||
|
||||
**Problem:** In Meerkat's Fixed-Limit, there's a cap of 3 raises per round (or 4 total bets). The `can_raise` method in `BettingRules` checks this, but `validate_action` doesn't check it. The validation only checks the raise amount, not the number of raises.
|
||||
|
||||
**Fix:** Pass `raises_this_round` to `validate_action` and check `bet_limit.can_raise(street, raises_this_round)` for FixedLimit.
|
||||
|
||||
---
|
||||
|
||||
## Meerkat API Alignment
|
||||
|
||||
### ALIGN-1: Add `get_amount_to_call` convenience method to `GameInfo`
|
||||
|
||||
**File:** [`game_info.rs`](holdem_core/src/game/game_info.rs)
|
||||
|
||||
**Problem:** Meerkat's `GameInfo.getAmountToCall(seat)` is used extensively by bots. The Rust `GameInfo` has `current_bet` and players have `wagered_this_round`, but there's no convenience method to compute `amount_to_call = current_bet - wagered_this_round`.
|
||||
|
||||
**Fix:** Add:
|
||||
```rust
|
||||
pub fn get_amount_to_call(&self, player_id: PlayerId) -> ChipCount
|
||||
pub fn is_preflop(&self) -> bool
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### ALIGN-2: Add `check_or_fold` Action helper
|
||||
|
||||
**Problem:** Meerkat has `Action.checkOrFoldAction(toCall)` which returns CHECK if toCall is 0, FOLD otherwise. The Rust implementation doesn't have this convenience.
|
||||
|
||||
**Fix:** Add a method to `Action`:
|
||||
```rust
|
||||
pub fn check_or_fold(amount_to_call: ChipCount) -> Action
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### ALIGN-3: Add `bet_action` helper
|
||||
|
||||
**Problem:** Meerkat has `Action.betAction(gi)` which creates a raise/bet of the minimum amount. The Rust implementation requires manually computing the raise total.
|
||||
|
||||
**Fix:** Add a constructor or helper that computes the correct raise total from game state.
|
||||
|
||||
---
|
||||
|
||||
## Test Coverage Gaps
|
||||
|
||||
### TEST-1: Fixed-limit raise validation on turn/river
|
||||
|
||||
No tests verify that Fixed-Limit raises on turn/river use `big_bet` instead of `small_bet`.
|
||||
|
||||
### TEST-2: Heads-up blind positions
|
||||
|
||||
No tests verify that in heads-up, the dealer is the small blind and acts first preflop.
|
||||
|
||||
### TEST-3: Side pot edge cases
|
||||
|
||||
No tests for:
|
||||
- Three players all-in with different amounts
|
||||
- Folded player with money in the pot creating side pots
|
||||
- Split pots with odd chips
|
||||
|
||||
### TEST-4: All-in that reopens betting
|
||||
|
||||
No tests verify that a full all-in raise correctly resets `has_acted_this_round` for other players.
|
||||
|
||||
---
|
||||
|
||||
## Priority Order
|
||||
|
||||
1. **BUG-1** — FixedLimit validation uses wrong bet size (gameplay-breaking for fixed-limit)
|
||||
2. **BUG-7** — FixedLimit raise count not validated (gameplay-breaking for fixed-limit)
|
||||
3. **BUG-2** — Pot-limit max raise calculation needs enforcement (gameplay-breaking for pot-limit)
|
||||
4. **BUG-3** — FixedLimit max_raise_amount returns None (related to BUG-2)
|
||||
5. **ALIGN-1** — Missing GameInfo convenience methods (bot compatibility)
|
||||
6. **ALIGN-2** — Missing check_or_fold helper (bot compatibility)
|
||||
7. **ALIGN-3** — Missing bet_action helper (bot compatibility)
|
||||
8. **TEST-1 through TEST-4** — Comprehensive test coverage
|
||||
9. **BUG-6** — Missing game_state_changed callback (low priority)
|
||||
Loading…
Add table
Add a link
Reference in a new issue