diff --git a/docs/BUGS_AND_FIXES.md b/docs/BUGS_AND_FIXES.md new file mode 100644 index 0000000..c75af37 --- /dev/null +++ b/docs/BUGS_AND_FIXES.md @@ -0,0 +1,140 @@ +# 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.