# 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.