From 4fa6d5b05e5d2a8b3effa92786ac207a04d0e6fe Mon Sep 17 00:00:00 2001 From: Houmgaor Date: Wed, 18 Mar 2026 22:44:26 +0100 Subject: [PATCH] docs: remove completed anti-patterns and technical-debt docs All tracked items in both files have been resolved or accepted. The content is superseded by CLAUDE.md architecture notes and commit history; retaining fully-struck-through docs adds noise without value. --- docs/anti-patterns.md | 312 ----------------------------------------- docs/technical-debt.md | 103 -------------- 2 files changed, 415 deletions(-) delete mode 100644 docs/anti-patterns.md delete mode 100644 docs/technical-debt.md diff --git a/docs/anti-patterns.md b/docs/anti-patterns.md deleted file mode 100644 index fec719a51..000000000 --- a/docs/anti-patterns.md +++ /dev/null @@ -1,312 +0,0 @@ -# Erupe Codebase Anti-Patterns Analysis - -> Analysis date: 2026-02-20 - -## Table of Contents - -- [1. God Files — Massive Handler Files](#1-god-files--massive-handler-files) -- [2. Silently Swallowed Errors](#2-silently-swallowed-errors) -- [3. No Architectural Layering](#3-no-architectural-layering--handlers-do-everything) -- [4. Magic Numbers Everywhere](#4-magic-numbers-everywhere) -- [5. Inconsistent Binary I/O Patterns](#5-inconsistent-binary-io-patterns) -- [6. Session God Object](#6-session-struct-is-a-god-object) -- [7. Mutex Granularity Issues](#7-mutex-granularity-issues) -- [8. Copy-Paste Handler Patterns](#8-copy-paste-handler-patterns) -- [9. Raw SQL Scattered in Handlers](#9-raw-sql-strings-scattered-in-handlers) -- [10. init() Handler Registration](#10-init-function-for-handler-registration) -- [11. Panic-Based Flow](#11-panic-based-flow-in-some-paths) -- [12. Inconsistent Logging](#12-inconsistent-logging) -- [13. Tight Coupling to PostgreSQL](#13-tight-coupling-to-postgresql) -- [Summary](#summary-by-severity) - ---- - -## 1. God Files — Massive Handler Files - -The channel server has large handler files, each mixing DB queries, business logic, binary serialization, and response writing with no layering. Actual line counts (non-test files): - -| File | Lines | Purpose | -|------|-------|---------| -| `server/channelserver/handlers_session.go` | 794 | Session setup/teardown | -| `server/channelserver/handlers_data_paper_tables.go` | 765 | Paper table data | -| `server/channelserver/handlers_quest.go` | 722 | Quest lifecycle | -| `server/channelserver/handlers_house.go` | 638 | Housing system | -| `server/channelserver/handlers_festa.go` | 637 | Festival events | -| `server/channelserver/handlers_data_paper.go` | 621 | Paper/data system | -| `server/channelserver/handlers_tower.go` | 529 | Tower gameplay | -| `server/channelserver/handlers_mercenary.go` | 495 | Mercenary system | -| `server/channelserver/handlers_stage.go` | 492 | Stage/lobby management | -| `server/channelserver/handlers_guild_info.go` | 473 | Guild info queries | - -These sizes (~500-800 lines) are not extreme by Go standards, but the files mix all architectural concerns. The bigger problem is the lack of layering within each file (see [#3](#3-no-architectural-layering--handlers-do-everything)), not the file sizes themselves. - -**Impact:** Each handler function is a monolith mixing data access, business logic, and protocol serialization. Testing or reusing any single concern is impossible. - ---- - -## 2. Missing ACK Responses on Error Paths (Client Softlocks) - -Some handler error paths log the error and return without sending any ACK response to the client. The MHF client uses `MsgSysAck` with an `ErrorCode` field (0 = success, 1 = failure) to complete request/response cycles. When no ACK is sent at all, the client softlocks waiting for a response that never arrives. - -### The three error handling patterns in the codebase - -**Pattern A — Silent return (the bug):** Error logged, no ACK sent, client hangs. - -```go -if err != nil { - s.logger.Error("Failed to get ...", zap.Error(err)) - return // BUG: client gets no response, softlocks -} -``` - -**Pattern B — Log and continue (acceptable):** Error logged, handler continues and sends a success ACK with default/empty data. The client proceeds with fallback behavior. - -```go -if err != nil { - s.logger.Error("Failed to load mezfes data", zap.Error(err)) -} -// Falls through to doAckBufSucceed with empty data -``` - -**Pattern C — Fail ACK (correct):** Error logged, explicit fail ACK sent. The client shows an appropriate error dialog and stays connected. - -```go -if data == nil { - doAckBufFail(s, pkt.AckHandle, nil) - return -} -``` - -### Evidence that fail ACKs are safe - -The codebase already sends ~70 `doAckSimpleFail`/`doAckBufFail` calls in production handler code across 15 files. The client handles them gracefully in all observed cases: - -| File | Fail ACKs | Client behavior | -|------|-----------|-----------------| -| `handlers_guild_scout.go` | 17 | Guild recruitment error dialogs | -| `handlers_guild_ops.go` | 10 | Permission denied, guild not found dialogs | -| `handlers_stage.go` | 8 | "Room is full", "wrong password", "stage locked" | -| `handlers_house.go` | 6 | Wrong password, invalid box index | -| `handlers_guild.go` | 9 | Guild icon update errors, unimplemented features | -| `handlers_guild_alliance.go` | 4 | Alliance permission errors | -| `handlers_data.go` | 4 | Decompression failures, oversized payloads | -| `handlers_festa.go` | 4 | Festival entry errors | -| `handlers_quest.go` | 3 | Missing quest/scenario files | - -A comment in `handlers_quest.go:188` explicitly documents the mechanism: - -> sends doAckBufFail, which triggers the client's error dialog (snj_questd_matching_fail → SetDialogData) instead of a softlock - -The original `mhfo-hd.dll` client reads the `ErrorCode` byte from `MsgSysAck` and dispatches to per-message error UI. A fail ACK causes the client to show an error dialog and remain functional. A missing ACK causes a softlock. - -### Scope - -A preliminary grep for `logger.Error` followed by bare `return` (no doAck call) found instances across ~25 handler files. However, a thorough manual audit (2026-02-20) revealed that the vast majority are Pattern B (log-and-continue to a success ACK with empty data) or Pattern C (explicit fail ACK). Only one true Pattern A instance was found, in `handleMsgSysOperateRegister` (`handlers_register.go`), which has been fixed. - -**Status:** ~~Players experience softlocks on error paths.~~ **Fixed.** The last Pattern A instance (`handlers_register.go:62`) now sends `doAckBufSucceed` with nil data before returning. The ~87 existing `doAckSimpleFail`/`doAckBufFail` calls and the helper functions (`loadCharacterData`, `saveCharacterData`, `stubEnumerateNoResults`) provide comprehensive ACK coverage across all handler error paths. - ---- - -## 3. No Architectural Layering — Handlers Do Everything - -Handler functions directly embed raw SQL, binary parsing, business logic, and response building in a single function body. For example, a typical guild handler will: - -1. Parse the incoming packet -2. Run 3-5 inline SQL queries -3. Apply business logic (permission checks, state transitions) -4. Manually serialize a binary response - -```go -func handleMsgMhfCreateGuild(s *Session, p mhfpacket.MHFPacket) { - pkt := p.(*mhfpacket.MsgMhfCreateGuild) - - // Direct SQL in the handler - var guildCount int - err := s.Server.DB.QueryRow("SELECT count(*) FROM guilds WHERE leader_id=$1", s.CharID).Scan(&guildCount) - if err != nil { - s.logger.Error(...) - return - } - - // Business logic inline - if guildCount > 0 { ... } - - // More SQL - _, err = s.Server.DB.Exec("INSERT INTO guilds ...") - - // Binary response building - bf := byteframe.NewByteFrame() - bf.WriteUint32(...) - doAckSimpleSucceed(s, pkt.AckHandle, bf.Data()) -} -``` - -There is no repository layer, no service layer — just handlers. - -**Impact:** Testing individual concerns is impossible without a real database and a full session. Business logic can't be reused. Schema changes require updating dozens of handler files. - -**Recommendation:** Introduce at minimum a repository layer for data access and a service layer for business logic. Handlers should only deal with packet parsing and response serialization. - ---- - -## 4. ~~Magic Numbers Everywhere~~ (Substantially Fixed) - -**Status:** Two rounds of extraction have replaced the highest-impact magic numbers with named constants: - -- **Round 1** (commit `7c444b0`): `constants_quest.go`, `handlers_guild_info.go`, `handlers_quest.go`, `handlers_rengoku.go`, `handlers_session.go`, `model_character.go` -- **Round 2**: `constants_time.go` (shared `secsPerDay`, `secsPerWeek`), `constants_raviente.go` (register IDs, semaphore constants), plus constants in `handlers_register.go`, `handlers_semaphore.go`, `handlers_session.go`, `handlers_festa.go`, `handlers_diva.go`, `handlers_event.go`, `handlers_mercenary.go`, `handlers_misc.go`, `handlers_plate.go`, `handlers_cast_binary.go`, `handlers_commands.go`, `handlers_reward.go`, `handlers_guild_mission.go`, `sys_channel_server.go` - -**Remaining:** Unknown protocol fields (e.g., `handlers_diva.go:112-115` `0x19, 0x2D, 0x02, 0x02`) are intentionally left as literals until their meaning is understood. Data tables (monster point tables, item IDs) are data, not protocol constants. Standard empty ACK payloads (`make([]byte, 4)`) are idiomatic Go. - -**Impact:** ~~New contributors can't understand what these values mean.~~ Most protocol-meaningful constants now have names and comments. - ---- - -## 5. ~~Inconsistent Binary I/O Patterns~~ (Resolved) - -**Status:** Non-issue on closer inspection. The codebase has already standardized on `byteframe` for all sequential packet building and parsing. - -The 12 remaining `encoding/binary` call sites (across `sys_session.go`, `handlers_session.go`, `model_character.go`, `handlers_quest.go`, `handlers_rengoku.go`) are all cases where `byteframe` is structurally wrong: - -- **Zero-allocation spot-reads on existing `[]byte`** — reading an opcode or ack handle from an already-serialized packet for logging, or sentinel guard checks on raw blobs. Allocating a byteframe for a 2-byte read in a log path would be wasteful. -- **Random-access reads/writes at computed offsets** — patching fields in the decompressed game save blob (`model_character.go`) or copying fields within quest binaries during version backport (`handlers_quest.go`). Byteframe is a sequential cursor and cannot do `buf[offset:offset+4]` style access. - -Pattern C (raw `data[i] = byte(...)` serialization) does not exist in production code — only in test fixtures as loop fills for dummy payloads. - ---- - -## 6. ~~Session Struct is a God Object~~ (Accepted Design) - -`sys_session.go` defines a `Session` struct (~30 fields) that every handler receives. After analysis, this is accepted as appropriate design for this codebase: - -- **Field clustering is natural:** The ~30 fields cluster into 7 groups (transport, identity, stage, semaphore, gameplay, mail, debug). Transport fields (`rawConn`, `cryptConn`, `sendPackets`) are only used by `sys_session.go` — already isolated. Stage, semaphore, and mail fields are each used by 1-5 dedicated handlers. -- **Core identity is pervasive:** `charID` is used by 38 handlers — it's the core identity field. Extracting it adds indirection for zero benefit. -- **`s.server` coupling is genuine:** Handlers need 2-5 repos + config + broadcast, so narrower interfaces would mirror the full server without meaningful decoupling. -- **Cross-channel operations use `Registry`:** The `Channels []*Server` field has been removed. All cross-channel operations (worldcast, session lookup, disconnect, stage search, mail notification) now go exclusively through the `ChannelRegistry` interface, removing the last direct inter-server coupling. -- **Standard game server pattern:** For a game server emulator with the `func(s *Session, p MHFPacket)` handler pattern, Session carrying identity + server reference is standard design. - -**Status:** Accepted design. The `Channels` field was removed and all cross-channel operations are routed through `ChannelRegistry`. No further refactoring planned. - ---- - -## 7. ~~Mutex Granularity Issues~~ (Stage Map Fixed) - -~~`sys_stage.go` and `sys_channel_server.go` use coarse-grained `sync.RWMutex` locks on entire maps:~~ - -```go -// A single lock for ALL stages -s.stageMapLock.Lock() -defer s.stageMapLock.Unlock() -// Any operation on any stage blocks all other stage operations -``` - -The Raviente shared state uses a single mutex for all Raviente data fields. - -**Status:** **Partially fixed.** The global `stagesLock sync.RWMutex` + `map[string]*Stage` has been replaced with a typed `StageMap` wrapper around `sync.Map`, providing lock-free reads and concurrent writes to disjoint keys. Per-stage `sync.RWMutex` remains for protecting individual stage state. The Raviente mutex is unchanged — contention is inherently low (single world event, few concurrent accessors). - ---- - -## 8. Copy-Paste Handler Patterns - -~~Many handlers follow an identical template with minor variations but no shared abstraction.~~ **Substantially fixed.** `loadCharacterData` and `saveCharacterData` helpers in `handlers_helpers.go` now cover all standard character blob load/save patterns (11 load handlers, 6 save handlers including `handleMsgMhfSaveScenarioData`). The `saveCharacterData` helper sends `doAckSimpleFail` on oversized payloads and DB errors, matching the correct error-handling pattern. - -Remaining inline DB patterns were audited and are genuinely different (non-blob types, wrong tables, diff compression, read-modify-write with bit ops, multi-column updates, or queries against other characters). - ---- - -## 9. Raw SQL Strings Scattered in Handlers - -SQL queries are string literals directly embedded in handler functions with no constants, no query builder, and no repository abstraction: - -```go -err := s.Server.DB.QueryRow( - "SELECT id, name, leader_id, ... FROM guilds WHERE id=$1", guildID, -).Scan(&id, &name, &leaderID, ...) -``` - -The same table is queried in different handlers with slightly different column sets and joins. - -**Impact:** Schema changes (renaming a column, adding a field) require finding and updating every handler that touches that table. There's no way to ensure all queries stay in sync. SQL injection risk is low (parameterized queries are used), but query correctness is hard to verify. - -**Recommendation:** At minimum, define query constants. Ideally, introduce a repository layer that encapsulates all queries for a given entity. - -**Status:** ~~Substantially fixed.~~ ~~Nearly complete.~~ **Complete.** 21 repository files now cover all major subsystems: character, guild, user, house, tower, festa, mail, rengoku, stamp, distribution, session, gacha, event, achievement, shop, cafe, goocoo, diva, misc, scenario, mercenary. All guild subsystem tables (`guild_posts`, `guild_adventures`, `guild_meals`, `guild_hunts`, `guild_hunts_claimed`, `guild_alliances`) are fully migrated into `repo_guild.go`. Zero inline SQL queries remain in handler files — the last 5 were migrated to `charRepo.LoadSaveData`, `userRepo.BanUser`, `eventRepo.GetEventQuests`, and `eventRepo.UpdateEventQuestStartTimes`. - ---- - -## 10. init() Function for Handler Registration - -`handlers_table.go` uses a massive `init()` function to register ~200+ handlers in a global map: - -```go -func init() { - handlers[network.MsgMhfSaveFoo] = handleMsgMhfSaveFoo - handlers[network.MsgMhfLoadFoo] = handleMsgMhfLoadFoo - // ... 200+ more entries -} -``` - -**Impact:** Registration is implicit and happens at package load time. It's impossible to selectively register handlers (e.g., for testing). The handler map can't be mocked. The `init()` function is ~200+ lines of boilerplate. - -**Recommendation:** Use explicit registration (a function called from `main` or server setup) that builds and returns the handler map. - ---- - -## 11. Panic-Based Flow in Some Paths - -~~Some error paths use `panic()` or `log.Fatal()` (which calls `os.Exit`) instead of returning errors.~~ **Substantially fixed.** The 5 production `panic()` calls (4 in mhfpacket `Build()` stubs, 1 in binpacket `Parse()`) have been replaced with `fmt.Errorf` returns. The `byteframe.go` read-overflow panic has been replaced with a sticky error pattern (`ByteFrame.Err()`), and the packet dispatch loop in `sys_session.go` now checks `bf.Err()` after parsing to reject malformed packets cleanly. - -**Remaining:** The `recover()` in `handlePacketGroup` is retained as a safety net for any future unexpected panics. - ---- - -## 12. Inconsistent Logging - -The codebase mixes logging approaches: - -- `zap.Logger` (structured logging) — primary approach -- Remnants of `fmt.Printf` / `log.Printf` in some packages -- Some packages accept a logger parameter, others create their own - -**Impact:** Log output format is inconsistent. Some logs lack structure (no fields, no levels). Filtering and aggregation in production is harder. - -**Recommendation:** Standardize on `zap.Logger` everywhere. Pass the logger via dependency injection. Remove all `fmt.Printf` / `log.Printf` usage from non-CLI code. - ---- - -## 13. ~~Tight Coupling to PostgreSQL~~ (Decoupled via Interfaces) - -~~Database operations use raw `database/sql` with PostgreSQL-specific syntax throughout:~~ - -- ~~`$1` parameter placeholders (PostgreSQL-specific)~~ -- ~~PostgreSQL-specific types and functions in queries~~ -- ~~`*sql.DB` passed directly through the server struct to every handler~~ -- ~~No interface abstraction over data access~~ - -**Status:** **Fixed.** All 20 repository interfaces are defined in `repo_interfaces.go` (`CharacterRepo`, `GuildRepo`, `UserRepo`, `GachaRepo`, `HouseRepo`, `FestaRepo`, `TowerRepo`, `RengokuRepo`, `MailRepo`, `StampRepo`, `DistributionRepo`, `SessionRepo`, `EventRepo`, `AchievementRepo`, `ShopRepo`, `CafeRepo`, `GoocooRepo`, `DivaRepo`, `MiscRepo`, `ScenarioRepo`, `MercenaryRepo`). The `Server` struct holds interface types, not concrete types. Mock implementations in `repo_mocks_test.go` enable handler unit tests without PostgreSQL. SQL is still PostgreSQL-specific within the concrete `*Repository` types, but handlers are fully decoupled from the database. - ---- - -## Summary by Severity - -| Severity | Anti-patterns | -|----------|--------------| -| **High** | ~~Missing ACK responses / softlocks (#2)~~ **Fixed**, no architectural layering (#3), ~~tight DB coupling (#13)~~ **Fixed** (21 interfaces + mocks) | -| **Medium** | ~~Magic numbers (#4)~~ **Fixed**, ~~inconsistent binary I/O (#5)~~ **Resolved**, ~~Session god object (#6)~~ **Accepted design** (Channels removed, Registry-only), ~~copy-paste handlers (#8)~~ **Fixed**, ~~raw SQL duplication (#9)~~ **Complete** (21 repos, 0 inline queries remain) | -| **Low** | God files (#1), ~~`init()` registration (#10)~~ **Fixed**, ~~inconsistent logging (#12)~~ **Fixed**, ~~mutex granularity (#7)~~ **Partially fixed** (stage map done, Raviente unchanged), ~~panic-based flow (#11)~~ **Fixed** | - -### Root Cause - -Most of these anti-patterns stem from a single root cause: **the codebase grew organically from a protocol reverse-engineering effort without introducing architectural boundaries**. When the primary goal is "make this packet work," it's natural to put the SQL, logic, and response all in one function. Over time, this produces the pattern seen here — hundreds of handler functions that each independently implement the full stack. - -### Recommended Refactoring Priority - -1. ~~**Add fail ACKs to silent error paths**~~ — **Done** (see #2) -2. ~~**Extract a character repository layer**~~ — **Done.** `repo_character.go` covers ~95%+ of character queries -3. ~~**Extract load/save helpers**~~ — **Done.** `loadCharacterData`/`saveCharacterData` in `handlers_helpers.go` -4. ~~**Extract a guild repository layer**~~ — **Done.** `repo_guild.go` covers all guild tables including subsystem tables -5. ~~**Define protocol constants**~~ — **Done** (see #4) -6. ~~**Standardize binary I/O**~~ — already standardized on `byteframe`; remaining `encoding/binary` uses are correct (see #5) -7. ~~**Migrate last 5 inline queries**~~ — **Done.** Migrated to `charRepo.LoadSaveData`, `userRepo.BanUser`, `eventRepo.GetEventQuests`, `eventRepo.UpdateEventQuestStartTimes` -8. ~~**Introduce repository interfaces**~~ — **Done.** 20 interfaces in `repo_interfaces.go`, mock implementations in `repo_mocks_test.go`, `Server` struct uses interface types -9. ~~**Reduce mutex contention**~~ — **Done.** `StageMap` (`sync.Map`-backed) replaces global `stagesLock`. Raviente mutex unchanged (low contention) diff --git a/docs/technical-debt.md b/docs/technical-debt.md deleted file mode 100644 index eab287964..000000000 --- a/docs/technical-debt.md +++ /dev/null @@ -1,103 +0,0 @@ -# Erupe Technical Debt & Suggested Next Steps - -> Last updated: 2026-03-05 - -This document tracks actionable technical debt items discovered during a codebase audit. It complements `anti-patterns.md` (which covers structural patterns) by focusing on specific, fixable items with file paths and line numbers. - -## Table of Contents - -- [High Priority](#high-priority) - - [1. Broken game features (gameplay-impacting TODOs)](#1-broken-game-features-gameplay-impacting-todos) - - [2. Test gaps on critical paths](#2-test-gaps-on-critical-paths) -- [Medium Priority](#medium-priority) - - [3. Logging anti-patterns](#3-logging-anti-patterns) -- [Low Priority](#low-priority) - - [4. CI updates](#4-ci-updates) -- [Completed Items](#completed-items) -- [Suggested Execution Order](#suggested-execution-order) - ---- - -## High Priority - -### 1. Broken game features (gameplay-impacting TODOs) - -These TODOs represent features that are visibly broken for players. - -| Location | Issue | Impact | Tracker | -|----------|-------|--------|---------| -| ~~`model_character.go:88,101,113`~~ | ~~`TODO: fix bookshelf data pointer` for G10-ZZ, F4-F5, and S6 versions~~ | ~~Wrong pointer corrupts character save reads for three game versions.~~ **Fixed.** Corrected offsets to 103928 (G1–Z2), 71928 (F4–F5), 23928 (S6) — validated via inter-version delta analysis and Ghidra decompilation of `snj_db_get_housedata` in the ZZ DLL. | [#164](https://github.com/Mezeporta/Erupe/issues/164) | -| ~~`handlers_achievement.go:117`~~ | ~~`TODO: Notify on rank increase` — always returns `false`~~ | ~~Achievement rank-up notifications are silently suppressed.~~ **Fixed.** RE'd `putDisplayed_achievement` in ZZ DLL: sends opcode + 1 zero byte (no achievement ID). Added `displayed_levels` column to track per-character last-displayed levels; `GetAchievement` compares current vs displayed; `DisplayedAchievement` snapshots current levels. | [#165](https://github.com/Mezeporta/Erupe/issues/165) | -| ~~`handlers_guild_info.go:443`~~ | ~~`TODO: Enable GuildAlliance applications` — hardcoded `true`~~ | ~~Guild alliance applications are always open regardless of setting.~~ **Fixed.** Added `recruiting` column to `guild_alliances`, wired `OperateJoint` actions `0x06`/`0x07`, reads from DB. | [#166](https://github.com/Mezeporta/Erupe/issues/166) | -| ~~`handlers_session.go:410`~~ | ~~`TODO(Andoryuuta): log key index off-by-one`~~ | ~~Known off-by-one in log key indexing is unresolved~~ **Documented.** RE'd from ZZ DLL: `putRecord_log`/`putTerminal_log` don't embed the key (size 0), so the off-by-one only matters in pre-ZZ clients and is benign server-side. | [#167](https://github.com/Mezeporta/Erupe/issues/167) | -| ~~`handlers_session.go:551`~~ | ~~`TODO: This case might be <=G2`~~ | ~~Uncertain version detection in switch case~~ **Documented.** RE'd ZZ per-entry parser (FUN_115868a0) confirms 40-byte padding. G2 DLL analysis inconclusive (stripped, no shared struct sizes). Kept <=G1 boundary with RE documentation. | [#167](https://github.com/Mezeporta/Erupe/issues/167) | -| ~~`handlers_session.go:714`~~ | ~~`TODO: Retail returned the number of clients in quests`~~ | ~~Player count reported to clients does not match retail behavior~~ **Fixed.** Added `QuestReserved` field to `StageSnapshot` that counts only clients in "Qs" stages, pre-collected under server lock to respect lock ordering. | [#167](https://github.com/Mezeporta/Erupe/issues/167) | -| ~~`msg_mhf_add_ud_point.go:28`~~ | ~~`TODO: Parse is a stub` — field meanings unknown~~ | ~~UD point packet fields unnamed, `Build` not implemented~~ **Fixed.** RE'd `putAdd_ud_point` from ZZ DLL: two uint32 fields are QuestPoints (sum of 11 category accumulators) and BonusPoints (kiju prayer multiplier extra). Added `diva_points` table, repo methods, handler accumulation, and migration 0009. | [#168](https://github.com/Mezeporta/Erupe/issues/168) | - -### 2. Test gaps on critical paths - -**All handler files now have test coverage.** - -~~**Repository files with no store-level test file (17 total):**~~ **Fixed.** All 20 repo source files now have corresponding `_test.go` files. The split guild files (`repo_guild_adventure.go`, `repo_guild_alliance.go`, etc.) are covered by `repo_guild_test.go`. These are mock-based unit tests; SQL-level integration tests against a live database remain a future goal. - ---- - -## Medium Priority - -### 3. Logging anti-patterns - -~~**a) `fmt.Sprintf` inside structured logger calls (6 sites):**~~ **Fixed.** All 6 sites now use `zap.Uint32`/`zap.Uint8`/`zap.String` structured fields instead of `fmt.Sprintf`. - -~~**b) 20+ silently discarded SJIS encoding errors in packet parsing:**~~ **Fixed.** All call sites now use `SJISToUTF8Lossy()` which logs decode errors at `slog.Debug` level. - ---- - -## Low Priority - -### 4. CI updates - -- ~~`codecov-action@v4` could be updated to `v5` (current stable)~~ **Removed.** Replaced with local `go tool cover` threshold check (no Codecov account needed). -- ~~No coverage threshold is enforced — coverage is uploaded but regressions aren't caught~~ **Fixed.** CI now fails if total coverage drops below 50% (current: ~58%). - ---- - -## Completed Items - -Items resolved since the original audit: - -| # | Item | Resolution | -|---|------|------------| -| ~~3~~ | **Sign server has no repository layer** | Fully refactored with `repo_interfaces.go`, `repo_user.go`, `repo_session.go`, `repo_character.go`, and mock tests. All 8 previously-discarded error paths are now handled. | -| ~~4~~ | **Split `repo_guild.go`** | Split from 1004 lines into domain-focused files: `repo_guild.go` (466 lines, core CRUD), `repo_guild_posts.go`, `repo_guild_alliance.go`, `repo_guild_adventure.go`, `repo_guild_hunt.go`, `repo_guild_cooking.go`, `repo_guild_rp.go`. | -| ~~6~~ | **Inconsistent transaction API** | All call sites now use `BeginTxx(context.Background(), nil)` with deferred rollback. | -| ~~7~~ | **`LoopDelay` config has no Viper default** | `viper.SetDefault("LoopDelay", 50)` added in `config/config.go`. | -| — | **Monthly guild item claim** (`handlers_guild.go:389`) | Now tracks per-character per-type monthly claims via `stamps` table. | -| — | **Handler test coverage (4 files)** | Tests added for `handlers_session.go`, `handlers_gacha.go`, `handlers_plate.go`, `handlers_shop.go`. | -| — | **Handler test coverage (`handlers_commands.go`)** | 62 tests covering all 12 commands, disabled-command gating, op overrides, error paths, raviente with semaphore, course enable/disable/locked, reload with players/objects. | -| — | **Handler test coverage (`handlers_data_paper.go`)** | 20 tests covering all DataType branches (0/5/6/gift/>1000/unknown), ACK payload structure, earth succeed entry counts, timetable content, serialization round-trips, and paperGiftData table integrity. | -| — | **Handler test coverage (5 files)** | Tests added for `handlers_seibattle.go` (9 tests), `handlers_kouryou.go` (7 tests), `handlers_scenario.go` (6 tests), `handlers_distitem.go` (8 tests), `handlers_guild_mission.go` (5 tests in coverage5). | -| — | **Entrance server raw SQL** | Refactored to repository interfaces (`repo_interfaces.go`, `repo_session.go`, `repo_server.go`). | -| — | **Guild daily RP rollover** (`handlers_guild_ops.go:148`) | Implemented via lazy rollover in `handlers_guild.go:110-119` using `RolloverDailyRP()`. Stale TODO removed. | -| — | **Typos** (`sys_session.go`, `handlers_session.go`) | "For Debuging" and "offical" typos already fixed in previous commits. | -| — | **`db != nil` guard** (`handlers_session.go:322`) | Investigated — this guard is intentional. Test servers run without repos; the guard protects the entire logout path from nil repo dereferences. Not a leaky abstraction. | -| ~~2~~ | **Repo test coverage (17 files)** | All 20 repo source files now have `_test.go` files with mock-based unit tests. | -| — | **Bookshelf data pointer** ([#164](https://github.com/Mezeporta/Erupe/issues/164)) | Corrected `pBookshelfData` offsets for G1–Z2 (103928), F4–F5 (71928), S6 (23928). Validated via inter-version delta analysis and Ghidra decompilation of ZZ `snj_db_get_housedata`. | -| — | **Guild nil panics** ([#171](https://github.com/Mezeporta/Erupe/issues/171)) | Three fixes merged post-RC1: nil guards for alliance guild lookups (aee5353), variable shadowing fix in scout list (8e79fe6), nil guards in cancel/answer scout handlers (8717fb9). Clan hall softlock resolved. | -| — | **ecdMagic byte order** ([#174](https://github.com/Mezeporta/Erupe/issues/174)) | Corrected constant byte order in `crypt_conn.go` (10ac803). | -| — | **Rengoku caching** | Cached `rengoku_data.bin` at startup to avoid repeated disk reads (5b631d1). | -| — | **rasta_id=0 save issue** ([#163](https://github.com/Mezeporta/Erupe/issues/163)) | `SaveMercenary` now skips rasta_id update when value is 0, preserving NULL for characters without a mercenary. | -| — | **fmt.Printf in setup wizard** | Replaced `fmt.Printf` in `server/setup/setup.go` with structured `zap` logging. | - ---- - -## Suggested Execution Order - -Based on remaining impact: - -1. ~~**Add tests for `handlers_commands.go`**~~ — **Done.** 62 tests covering all 12 commands (ban, timer, PSN, reload, key quest, rights, course, raviente, teleport, discord, playtime, help), disabled-command gating, op overrides, error paths, and `initCommands`. -2. ~~**Fix bookshelf data pointer** ([#164](https://github.com/Mezeporta/Erupe/issues/164))~~ — **Done.** Corrected offsets for G1–Z2, F4–F5, S6 via delta analysis + Ghidra RE -3. ~~**Fix achievement rank-up notifications** ([#165](https://github.com/Mezeporta/Erupe/issues/165))~~ — **Done.** RE'd `putDisplayed_achievement` from ZZ DLL; added `displayed_levels` column + service methods + migration 0008 -4. ~~**Add coverage threshold** to CI~~ — **Done.** 50% floor enforced via `go tool cover` in CI; Codecov removed. -5. ~~**Fix guild alliance toggle** ([#166](https://github.com/Mezeporta/Erupe/issues/166))~~ — **Done.** `recruiting` column + `OperateJoint` allow/deny actions + DB toggle -6. ~~**Fix session handler retail mismatches** ([#167](https://github.com/Mezeporta/Erupe/issues/167))~~ — **Documented.** RE'd from ZZ DLL; log key off-by-one is benign server-side, player count fixed via `QuestReserved`. -7. ~~**Reverse-engineer MhfAddUdPoint fields** ([#168](https://github.com/Mezeporta/Erupe/issues/168))~~ — **Done.** RE'd from ZZ DLL: QuestPoints + BonusPoints, added `diva_points` table + migration 0009