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

## Still open

- **Explicit undo snapshot-existence flag** (the current fix safely refuses an intentionally empty before-image).
- **Production `FirstCraft()` vs multi-recipe availability** (5 of 6 `arrow_shaft` recipes unreachable); **"how many" batches vs units**.
- **Craft `type`/`subtype` validator rule**; **safespot validation** (needs the size vocabulary moved to a shared package); **drop weight in mob loot/steal readers**.
- **Persistence** `Recover` mid-game misuse; journal versioning.
- **Casino**: craps Fire/ATS settlement deferral; plinko RTP wording.
- **Admin**: symlink handling in the raw file API.
- **Structural refactors** from `AUDIT.md` §3–4.
- **Test infrastructure**: `make test-race`/CI, harness owner-goroutine leak, master-tick harness option, and coverage for steal/gather/burn/talk and the WebSocket transport.

## Suggested next step

Tranche 4: the production multi-recipe fix, the remaining validation gaps, the persistence hardening, and then the structural refactors. Keep `make lint`, `go test ./...`, and `go test -race` green.