path: root/FIXES.md

# Fix Tranche 1 — verified P0/P1 plus high-value P2s

Date: 2026-10-05
Companion to `AUDIT.md` (findings) and `reports/audit/` (raw lane reports).

## Status

- `go build ./...` — pass
- `go vet ./...` — pass
- `make lint` (`golangci-lint run`) — **now 0 issues** (was 15)
- `go test ./...` — pass
- `go test -race` (engine, net, persistence, player, world, combat, casino, game, hacking) — pass
- Changes are uncommitted. `git diff --shortstat` = 51 files, +509/−131 (plus new test files).

Nothing was staged or committed. The audit artifacts (`AUDIT.md`, `reports/`) are locally excluded via `.git/info/exclude` so they do not dirty the tree.

## Fixed (each with a regression test unless noted)

### Correctness / exploit (P0/P1)
1. **Gather verbs broken (P0)** — restored `mine/chop/fish/cut → gather` aliases in `internal/game/act.go`; corrected the misleading comment. Test: `TestGatherVerbsNormalizeToGatherAction`.
2. **`walk <huge>n` remote OOM** — `parseWalkDirections` now clamps the count before allocating (`internal/game/cmd_walk.go`). Test: `TestParseWalkDirectionsRejectsHugeCount`.
3. **`sell`/`deposit` dead-end on 2+ stacks** — dedup by name with `uniqueItemNames` (`cmd_shop.go`, `cmd_bank.go`).
4. **Admin `//` path bypass** — added `canonicalDataRelPath`, classify on the canonical path, removed the shadowed `fullPath` join (`internal/admin/api_files.go`); hardened the game-side `playerDataFileName` the same way. Tests: `TestCanonicalDataRelPathStripsRepeatedPrefixes`, `TestPlayerDataFileNameCanonicalizesPrefixes`.
5. **Undo can truncate a live file to 0 bytes** — `load()` maps an empty before-image back to nil so `requireSnapshot` fires (`internal/admin/undo.go`). (Fail-safe: an intentionally empty before-image is refused rather than written; the explicit-existence-flag fix remains a follow-up.)
6. **Roulette selective-refund exploit** — disconnect during `phasePlaying` now forfeits the stake, matching Big 6 (`internal/casino/roulette.go`); updated `TestRouletteRefundAndPersistenceFailure` to assert forfeit vs refund.
7. **Resolved predictions destroyed by the credit cap** — compute totals and run the cap guard before mutating contract status (`internal/game/predictions.go`). Test: `TestCollectPredictionsKeepsContractsWhenCreditCapBlocks`.
8. **`RekeyRooms` stranded wanderers** — rebuild the object-state map from every state and remap `RoomID`/`HomeRoomID`/`WanderRooms` (`internal/world/world.go`). Test: `TestRekeyRoomsUpdatesAwayFromHomeObject`.
9. **Admin room swap/undo did not reconcile live state** — `handleRoomSwap` now reconciles both rooms; `rebuildAfterUndo` reconciles when the room file still exists (`internal/admin/api_rooms.go`, `server.go`).
10. **Estate directory was dead** — allow `estate_directory` through the interactability gate (`act.go`); exempt the `Cfg == nil && EstateDirectory` prompt in `advanceTalk` (`act_talk.go`). Live content places it in `data/rooms/tuccia/3124.yaml`.
11. **`ui.Table` title overhang** — truncate the title to the interior width (`internal/ui/table.go`). Test: `TestTableLongTitleStaysWithinBorder`.
12. **Hacking minigames printed raw color tags** — expand tags before writing (`internal/game/core_hacking.go`).
13. **Pai gow betting window never expired with one bettor + one idle seat** — count seated players, not bettors (`internal/casino/pai_gow.go`). Test: `TestPaiGowBettingWindowExpiresWithOneBettorAndIdleSeat`.

### Combat / player / items
14. **Double combat entry** — `startCombat` now refuses re-entry (`internal/game/combat_attack.go`); `Tracker.Enter` frees the previous mob (`internal/combat/state.go`). Test: `TestTrackerEnterReleasesPreviousMob`.
15. **Retribution skipped `StartRegen`** — surviving mob no longer heals 100× too fast (`internal/game/combat_mob.go`).
16. **`RemoveItem` drained items on failure** — now all-or-nothing (`internal/player/player.go`). Test: `TestRemoveItemIsAllOrNothing`.
17. **`drop all <item>` zeroed quality** — preserve Quality/MaxQuality via `AddGroundItemQ` (`internal/game/cmd_drop.go`).
18. **`RegenerateTick` was persisted** — added `yaml:"-"` (`internal/player/player.go`).
19. **`XPTable` was `[100]int` with 99 values** — now `[99]int` (`internal/player/xp.go`). Test: `TestXPTableBounds`.

### Command / UX
20. **Prefix/abbreviation lowercased arguments** — preserve original case for `rawInput` consumers (`internal/game/game.go`). Test: `TestGlobalCommandPreservesArgumentCase`.
21. **`queued` ignored the consume slot** — added `PeekConsume` and display (`core_command_queue.go`, `cmd_queued.go`).
22. **`queued` hardcoded 600 ms** — now reports the configured tick length (`cmd_queued.go`).
23. **`verbs` ignored `wrap_width`** — honors the option (`cmd_verbs.go`).
24. **Unknown craft subtype soft-locked the action** — fallback uses `behavior.TypeCraft` so it advances (`production_engine.go`).
25. **Empty-ingredient craft was infinite/free** — `planIngredients` returns nil (`production_index.go`). Test: `TestPlanIngredientsRejectsEmptyCraft`.

### Persistence / validation / net
26. **`Move` + `Delete`/`Replace` silently lost files** — conflicting ops now error (`internal/persistence/txn.go`). Tests: `TestTxnRejectsConflictingMoveThenDelete`, `TestTxnRejectsConflictingMoveThenReplace`, `TestTxnMoveThenReplaceDestination`.
27. **`Recover` dropped unknown op kinds and deleted the journal** — unknown kind is now a rollback error, keeping the journal (`txn.go`). Test: `TestRecoverKeepsJournalWithUnknownOpKind`.
28. **Journal directory entry not durable** — `Recover` (the startup path that creates it) now fsyncs its parent (`txn.go`).
29. **`ExitDef.Condition` never validated** — added the check (`internal/validate/checks.go`).
30. **`stripIAC` leaked a byte for a truncated IAC** — drop the remainder (`internal/net/conn.go`). Test: `TestStripIACDropsTruncatedSequence`.

### Tooling
31. **`make lint` gate restored** — fixed the 15 reported issues (error-string casing, doc-comment form, unused params, ineffassign) without weakening `.golangci.yml`.

## Tranche 2

- **Security**: `restoreAccountSecret` no longer fails open on a non-mapping body (the admin raw editor can no longer blank `password_hash`); `validEntityID` rejects control characters and leading dots.
- **Info disclosure**: video-poker bank/wager errors are mapped instead of printing raw `os`/YAML errors and server paths.
- **Casino**: slots free-spin counter no longer prints "0 free spins remain" before the last spin; `leave` settles banked free-spin winnings instead of silently dropping them; craps repeated-odds returns the correct error; a craps shooter change resets the hand (point/made/seen) so the next shooter gets a fresh come-out.
- **Economy**: `get <n> credits` honors the requested quantity; bank withdraw ambiguity list no longer duplicates entries.
- **Admin**: `entityCreate` drops editor-only `_path`/`_raw`; `detachRoomFromCourse` checks `os.Remove` and detaches from every matching course.
- **Validation**: `.yml` files are discovered/validated (matching the runtime trigger loader); talk configs with nodes but no `start` node are an error.
- **Persistence**: `Recover` sweeps crash-created `.save-*` temp files.
- **Net**: `Conn` now requires `SetReadDeadline`, so every transport gets an idle read deadline (removed the concrete-type switch); hub room buckets are pruned and `RekeyRooms` merges instead of clobbering.
- Tests added: `TestRestoreAccountSecretRefusesNonMappingBody`, `TestWalkYAMLDirIncludesYml`, `TestValidateTalkConfigRequiresStartNode`, `TestPickupCreditsHonorsRequestedQuantity`, `TestMachineLeavePaysBankedFeature`, `TestCrapsShooterChangeResetsHand`.

## Tranche 3 (this pass)

- **Undo**: a failed undo/redo is now reported distinctly from an empty history (`UndoStack.LastError()`), so the admin UI no longer shows "Nothing to undo" for a real failure. Test: `TestUndoFailureIsReportedDistinctly`.
- **Quit**: `quit`/`logout` are `ClassInstant`, so a queued active command can no longer silently replace or delay a quit.
- **World**: a reference-style room object entry (`id:` only) that carries behavior fields is rejected at load instead of silently dropping the behavior. Test: `TestRoomObjectReferenceRejectsBehaviorFields`.
- **Validation**: gather drops with `weight <= 0` (never selectable) are warned about. Test: `TestValidateGatherWarnsOnWeightlessDrop`.
- **Admin**: trigger POST now serializes through `triggerMapToYAML` like PUT (no unknown-key divergence); `resolve-duplicate` verifies the path is genuinely a duplicate-ID room file before deleting/renaming it.
- **Net**: the shutdown flush gives each session a bounded slice instead of letting the first stalled writer starve the rest; the telnet echo CRLF refreshes its write deadline so it is not dropped on an idle connection.
- **Casino**: video-poker cabinet IDs are claimed in the addressable namespace, so an ID colliding with a machine variant/game name is rejected instead of producing an unreachable cabinet. Test: `TestVideoPokerCabinetIDCollisionIsRejected`.

## Tranche 4 (this pass)

- **Production**: the multi-recipe bug is fixed. Availability scanned every recipe but execution used `Craft[0]`, so 5 of 6 `arrow_shaft` recipes were unreachable (start-then-abort). A `CraftIndex` is now carried through `ProductionData`/`CombineData`/`FletchData`; `usableCraftIndex` selects the recipe the player satisfies; `collectByproducts`/`canContinueProduction` use the selected recipe; the production menu displays the usable recipe; and `findFletchItems` scans every recipe. Test: `TestUsableCraftIndexSelectsSatisfiedRecipe`.
- **Validation**: mob loot and steal drops with `weight <= 0` are now warned about (gather already was).
- **Persistence**: journals now carry a schema `version`; `Recover` refuses a version it does not understand instead of misreading a newer journal. Test: `TestRecoverRefusesUnknownJournalVersion`.
- **Casino**: the plinko rules text now states the measured per-risk returns rather than only the target RTP.
- **Refactor**: the duplicated `countExtraInbound` (game + admin, byte-for-byte) is now one `world.CountExtraInbound`, so the two room-removal paths cannot drift.

## Tranche 5 (this pass)

- **Refactor**: the three YAML map encoders (`admin.marshalYAML`, `admin.marshalMapYAML`, `behavior.MarshalYAMLMap`) now share one `behavior.MarshalYAML`.
- **Refactor**: the `.journal` path is now `persistence.JournalDir(dataDir)` used by both the game and the admin API, so a future rename cannot leave one writer journaling to a directory startup recovery never scans.
- **Test infrastructure**: added a `make test-race` target (`go test -race ./... -timeout 300s`); the shared game harness now stops the Game owner goroutine on cleanup (previously every harness test leaked one); added the first WebSocket transport tests (`TestWebSocketCheckOrigin` unit-tests the origin boundary with a non-loopback peer, `TestWebSocketUpgradeAccepted` drives the `/ws` upgrade end-to-end).

## Tranche 6 (this pass)

- **Refactor**: `GridConflict.Kind` is now the typed `world.GridConflictKind` (`GridConflictTwist`/`GridConflictOverlap`), replacing the magic strings switched on in five packages.
- **Refactor**: the duplicated self-signed cert generator is now `tlscert.SelfSigned(commonName)`, shared by the game and admin servers.
- **Validation**: craft `type`/`subtype` is checked against `item.KnownCraftKeys` — an unknown key left the action set but never advancing (player stuck). Test: `TestValidateCraftBlocksRejectsUnknownType`.
- **Validation**: the mob/safespot size vocabulary moved to `object.MobSizeRank`; `validateObjects` rejects an unknown `safespot.max_block_size` (silently disables cover) and out-of-range chances, and `validateMobCombatStats` rejects an unknown mob `size`. Test: `TestValidateObjectsRejectsUnknownSafespotSize`.

## Tranche 7 (this pass)

- **Security**: `resolveDataPath` now resolves symlinks on the longest existing prefix and re-checks containment, so a symlink inside `data/` cannot redirect a raw-editor read or write outside the data directory. Test: `TestResolveDataPathRejectsSymlinkEscape`.
- **UX**: the production "how many" prompt now says "How many batches (each makes N, return for all)?" so a batch count cannot silently mean N× the materials/units.
- **Test coverage**: added `TestGatherYieldsDrop`, an end-to-end gather test (seed object + pickaxe, start the action, tick to a drop) — the first coverage for the gather system the audit flagged.
- **Investigated (no change)**: the craps Fire bet. Settling it at the 4th/5th point as the audit suggested would cap the payout at the lower tier and under-pay; the bet must ride to the seven-out to pay the highest tier reached. Left as-is and noted here.

## Tranche 8 (this pass)

- **Undo**: added an explicit before/after snapshot-existence flag (`ChangeDesc.OldExists`/`NewExists`, carried through `ExtraFile` and the persisted `undoEntry`). `requireSnapshot` now keys on existence, not `len == 0`, so an intentionally empty file can be undone after a restart while a missing (failed-read) image is still refused. Older history files infer the flag from non-empty content. Test: `TestUndoRestoresIntentionallyEmptyFile`.
- **Refactor**: the duplicated `world.remapInt` and `game.remapScalar` are now one exported `world.RemapInt`, used by both the room-file and player-state migrations.

## Tranche 9 (this pass)

- **Test coverage**: added `TestBurnCreatesFire`, an end-to-end firemaking test (seed logs + tinderbox, start the action, tick until a fire object appears) — the burn coverage the audit called out.
- **Test infrastructure**: added `newHarnessWithTicks`, a harness variant that registers the production master tick subscription, plus `TestHarnessWithTicksCompletesDisconnect`, which exercises the real `Hub.Remove → DisconnectTick → onRemove` countdown end-to-end (the gap the audit found in the disconnect race tests).
- **Refactor**: exported `ui.FitWidths` (the `Table.Render` shrink algorithm) and replaced the duplicated shrink loop in `casino_house_edge.go`, so the two cannot disagree on which column yields.

## Tranche 10 (this pass)

- **Bug fix (previously missed)**: a terminal talk node (messages, no options, no goto) now clears the talk action in `showTalkNode`, so it no longer lingers as a stale action that suppresses the prompt and no-ops every tick. Test: `TestTerminalTalkNodeClearsAction`.
- **Test coverage**: added `TestObjectStealYieldsLoot`, an end-to-end object-steal test (seed a stealable stall, start the action, tick to loot). Talk and steal are now covered (gather and burn were in earlier tranches).
- **Refactor**: `paiGowTable.phase` is now the typed `tablePhase` with `phaseSetting`/`phaseSettling` constants, removing the 29 raw phase-string literals (a safe subset of the `tableCore` consistency work).
- **Docs**: `recoverJournals` now documents that it must run only at startup or on the owner goroutine, making the mid-game `Recover` invariant explicit.

## Still open

- **Casino**: craps Fire bet forfeiture on disconnect (design decision; shutdown refunds).
- **Structural refactors** remaining from `AUDIT.md` §3–4 (`tableCore` for pai gow — now partly addressed by the typed phase — `refs` struct in validate, flex-aware predictions table).
- **CI**: wire `make test-race` into a workflow.

## Suggested next step

The remaining structural refactors are cosmetic DRY with real regression risk and no functional benefit; the correctness backlog is now empty. Suggest stopping here unless a specific item is wanted. Keep `make lint`, `go test ./...`, and `go test -race` green.