140 lines
9.7 KiB
Markdown
140 lines
9.7 KiB
Markdown
# Bugs & Fixes Log
|
|
|
|
This document tracks bugs found, their root cause, the fix applied, and which tests guard against regression. Refer to this before making changes to the parser, UI, or bridge.
|
|
|
|
---
|
|
|
|
## Fixed Bugs
|
|
|
|
### Bug #1: Trade doesn't transfer properties
|
|
**Found:** 2026-02-21
|
|
**Severity:** High — property ownership map becomes permanently wrong after any trade
|
|
**Root cause:** `_execute_trade()` transferred cash and GOJF cards but ignored property lists entirely. Trade summary lines from `printsq()` (e.g. ` Tennessee 3 Orange 180 * 14`) were swallowed by catch-all returns.
|
|
**C behavior:** `do_trade()` → `move_em()` iterates `from->prop_list`, calls `add_list`/`del_list` to transfer each property's ownership (`sqr->owner = to->trader`).
|
|
**Fix:** Added `TRADE_PROP_RE` regex to parse `printsq`-format lines during trade state. Built `resolve_trade_property()` using truncated-name+cost lookup (handles the "Pennsylvan" ambiguity between RR and ave via cost). Properties tracked in `_trade_props1`/`_trade_props2` and transferred in `_execute_trade()`.
|
|
**Tests:** `test_parser_bugs.py::TestBug1_TradeProperties` (3 tests including real log replay at line 789)
|
|
|
|
### Bug #2: UI owner indicator color off-by-one
|
|
**Found:** 2026-02-21
|
|
**Severity:** Medium — wrong colors on board squares
|
|
**Root cause:** `sq.owner` is a 1-based player number, but the UI used it directly as an array index into `PLAYER_COLORS`. Additionally, after turn-order rotation, player numbers don't correspond to array positions.
|
|
**Fix:** Changed board rendering to `players.findIndex(p => p.number === sq.owner)` — looks up the player by number in the turn-ordered array, ensuring the owner dot color matches the player token color.
|
|
**Tests:** `test_parser_bugs.py::TestBug2_OwnerColorIndex` (data contract verification)
|
|
|
|
### Bug #3: `_first_player_idx` not set on mid-stream pickup
|
|
**Found:** 2026-02-21
|
|
**Severity:** Low — turn order display wrong when parser picks up an in-progress game
|
|
**Root cause:** `_first_player_idx` is only set in the "goes first" setup path. When a game is detected from a checkpoint line (mid-stream), this attribute is never set, so `get_state()` can't rotate the player list correctly.
|
|
**Fix:** Set `g._first_player_idx = 0` when creating a game from a checkpoint.
|
|
**Tests:** `test_parser_bugs.py::TestBug3_FirstPlayerIdx`
|
|
|
|
### Bug #4: Resign-to-player with unresolved target doesn't clear properties
|
|
**Found:** 2026-02-21
|
|
**Severity:** Medium — ghost ownership if resign target name can't be matched
|
|
**Root cause:** In `_execute_resign_to_player`, the else branch (no target found) logged "resigned to the bank" but didn't clear `property_owner`, `property_mortgaged`, or `property_houses` — unlike the explicit bank resignation path.
|
|
**Fix:** Added bank-style property cleanup in the else branch.
|
|
**Tests:** `test_parser_bugs.py::TestBug4_ResignNoTarget`
|
|
|
|
### Bug #5: Resign-to-bank doesn't clear properties
|
|
**Found:** 2026-02-21
|
|
**Severity:** High — properties stay "owned" by a removed player after bank resignation
|
|
**Root cause:** `_remove_player()` removed the player from the list but left their entries in `property_owner`. The explicit bank resignation handler also didn't clear properties, mortgages, or houses.
|
|
**C behavior:** `resign()` for bank case iterates `cur_p->own_list`, sets `sqp->owner = -1`, `sqp->desc->morg = FALSE`, `sqp->desc->houses = 0`.
|
|
**Fix:** Bank resignation path now deletes ownership, mortgage, and house entries. Also fixed property owner renumbering after `_remove_player()` shifts remaining player numbers.
|
|
**Tests:** `test_parser_resign.py::TestResignToBank` (7 tests)
|
|
|
|
### Bug #6: Resign-to-player doesn't transfer properties
|
|
**Found:** 2026-02-21
|
|
**Severity:** High — transferred assets incomplete
|
|
**Root cause:** `_execute_resign_to_player` transferred money and GOJF cards but not properties.
|
|
**C behavior:** `resign()` for player case uses `do_trade()` with `trades[1].prop_list = cur_p->own_list`, which transfers all properties via `move_em()`.
|
|
**Fix:** Added property transfer loop in the target branch of `_execute_resign_to_player`.
|
|
**Tests:** `test_parser_resign.py::TestResignToPlayer` (6 tests), `test_parser_resign.py::TestResignWithChosenTarget`
|
|
|
|
### Bug #7: Property owner numbers stale after player removal
|
|
**Found:** 2026-02-21
|
|
**Severity:** High — after resignation, `property_owner` references point to wrong players
|
|
**Root cause:** `_remove_player()` renumbers remaining players (C code shifts the array), but `property_owner` entries still reference old numbers.
|
|
**C behavior:** After removing a player: `for (i = 0; i < N_SQRS; i++) if (board[i].owner > player) --board[i].owner;`
|
|
**Fix:** Added renumber pass in `_remove_player()` that builds old→new number mapping and updates all `property_owner` values.
|
|
**Tests:** `test_parser_resign.py::TestResignToBank::test_remaining_players_renumbered`, `test_parser_resign.py::TestResignToPlayer::test_other_player_properties_preserved`
|
|
|
|
### Bug #8: `spec` flag never cleared
|
|
**Found:** 2026-02-21
|
|
**Severity:** Low — could affect rent calculation display for railroads/utilities
|
|
**Root cause:** `spec` is set when "Advance to nearest Railroad/Utility" Chance cards are drawn (doubles RR rent, forces 10x utility multiplier), but never cleared afterward.
|
|
**C behavior:** `spec = FALSE;` at the end of `get_card()`, after movement and rent have been processed.
|
|
**Fix:** Clear `g.spec = False` in `_pay_rent()`.
|
|
**Tests:** `test_parser_bugs.py::TestBug6_SpecFlag`
|
|
|
|
### Bug #9: `get_state()` doesn't emit `phase: "playing"`
|
|
**Found:** 2026-02-21
|
|
**Severity:** Low — UI had to infer playing state by absence of phase field
|
|
**Root cause:** `get_state()` only added `phase` for `"setup"` and `"over"`, not `"playing"`.
|
|
**Fix:** Changed to always emit `result["phase"] = g.phase`.
|
|
**Tests:** `test_parser_bugs.py::TestBug13_PhasePlaying`
|
|
|
|
### Bug #10: Resign and trade log entries missing timestamps
|
|
**Found:** 2026-02-21
|
|
**Severity:** Low — log entries for trades/resignations had no timestamp in the game log
|
|
**Root cause:** `_execute_resign_to_player()` and `_execute_trade()` called `g.add_log()` without passing `timestamp`.
|
|
**Fix:** Threaded `timestamp` parameter from the "Trade is done!" handler through to both methods.
|
|
**Tests:** `test_parser_bugs.py::TestBug16_ResignTimestamp`
|
|
|
|
### Bug #11: House counts from rent lines not tracked
|
|
**Found:** 2026-02-21
|
|
**Severity:** Medium — UI always showed 0 houses on properties
|
|
**Root cause:** When "with N houses, rent is X" or "with a hotel, rent is X" appeared, the parser extracted rent but didn't update `property_houses`.
|
|
**Fix:** Update `property_houses` for the current player's location when these rent lines appear.
|
|
**Note:** This is a partial fix — houses are synced when rent is charged, but not at the moment of purchase (see Known Limitations).
|
|
**Tests:** Covered by existing parser checkpoint tests (1551 checkpoints)
|
|
|
|
---
|
|
|
|
## Known Limitations
|
|
|
|
These are behaviors we can't fully track from parser output alone:
|
|
|
|
### GOJF card usage invisible
|
|
When a player types `.card` in jail, the C code silently decrements `num_gojf`, sets `loc = 10`, and `in_jail = 0` — with **no output**. The parser can't detect this, so `getOutOfJailFreeCards` count stays wrong. Location self-corrects at the next checkpoint.
|
|
|
|
### House counts not tracked at purchase time
|
|
The interactive house buying dialog (`"PropName (N): "` prompts) is ignored by the parser. We only learn house counts when rent is later charged on that property. A full fix would require parsing the entire house buy/sell dialog, matching truncated property names to squares.
|
|
|
|
### Holdings display ignored
|
|
`printhold()` outputs detailed property info (owner, group, cost, mortgage flag, house count) that could be used to periodically resync state. Currently the parser discards these lines. Parsing them would provide self-correction for houses, mortgages, and ownership.
|
|
|
|
### Unmortgage tracking incomplete
|
|
"That cost you $X" for unmortgage is handled for money deduction, but `property_mortgaged` isn't cleared because we can't determine which property was unmortgaged from the cost alone. Mortgage status self-corrects if a holdings display is parsed (not yet implemented) or when the property appears in a rent line (mortgaged properties show "The thing is mortgaged").
|
|
|
|
### `printsq` property name truncation
|
|
The C code truncates property names to 10 characters (`%-10.10s`). This creates one ambiguity: "Pennsylvan" maps to both Pennsylvania RR (cost $200) and Pennsylvania ave. (cost $320). Resolved by cost in trade parsing, but could be an issue if future code matches by name alone.
|
|
|
|
---
|
|
|
|
## Test Coverage Summary
|
|
|
|
| Test File | Count | Coverage |
|
|
|---|---|---|
|
|
| `test_parser.py` | 1551 checkpoints | Full log replay — money, location, player list at every checkpoint |
|
|
| `test_parser_resign.py` | 19 tests | Both resign types, property/money/GOJF transfer, renumbering, game over, real log replay |
|
|
| `test_parser_bugs.py` | 10 tests | Trade property transfer, owner data contract, mid-stream pickup, spec flag, phase field, timestamps |
|
|
| `test_players.py` | 38 tests | Player bot behavior — setup, prompts, jail, trading, debt, doubles |
|
|
|
|
**Acceptable mismatches:** 2 checkpoints in `test_parser.py` (line ~11682) caused by IRC disconnects in the original log, not parser bugs.
|
|
|
|
---
|
|
|
|
## How to Verify
|
|
|
|
```bash
|
|
cd /tmp/monop-state
|
|
|
|
# Full test suite
|
|
python3 test_parser.py # Log replay (1551 checkpoints)
|
|
python3 test_parser_resign.py # Resignation tests (19)
|
|
python3 test_parser_bugs.py # Bug regression tests (10)
|
|
python3 test_players.py # Player bot tests (38)
|
|
```
|
|
|
|
All tests should pass. If any fail after a change, check this document for the relevant bug — the regression likely reintroduced it.
|