From 8fead0b1f3e53caca7fa23b722adfafb7d72e12a Mon Sep 17 00:00:00 2001 From: Houmgaor Date: Tue, 24 Feb 2026 13:55:49 +0100 Subject: [PATCH] fix(handlers): add error handling for swallowed repo/service errors Several handler files discarded errors from repository and service calls, creating nil-dereference risks and silent data corruption: - guild_adventure: 3 GetByCharID calls could panic on nil guild - gacha: GetGachaPoints silently returned zero balances on DB error - house: HasApplication called before nil check on guild; GetHouseContents error discarded with 7 return values - distitem: 3 distRepo calls had no error logging - guild_ops: Disband/Leave service errors were invisible - shop: gacha type/weight/fpoint lookups had no error logging - discord: bcrypt error could result in nil password being set --- docs/improvements.md | 134 ++++++++++++++++++ server/channelserver/handlers_discord.go | 14 +- server/channelserver/handlers_distitem.go | 15 +- server/channelserver/handlers_gacha.go | 12 +- .../channelserver/handlers_guild_adventure.go | 21 ++- server/channelserver/handlers_guild_ops.go | 10 +- server/channelserver/handlers_house.go | 19 ++- server/channelserver/handlers_shop.go | 15 +- 8 files changed, 221 insertions(+), 19 deletions(-) create mode 100644 docs/improvements.md diff --git a/docs/improvements.md b/docs/improvements.md new file mode 100644 index 000000000..a33beb535 --- /dev/null +++ b/docs/improvements.md @@ -0,0 +1,134 @@ +# Erupe Improvement Plan + +> Analysis date: 2026-02-24 + +Actionable improvements identified during a codebase audit. Items are ordered by priority and designed to be tackled sequentially. Complements `anti-patterns.md` and `technical-debt.md`. + +## Table of Contents + +- [1. Fix swallowed errors with nil-dereference risk](#1-fix-swallowed-errors-with-nil-dereference-risk) +- [2. Fix bookshelf data pointer for three game versions](#2-fix-bookshelf-data-pointer-for-three-game-versions) +- [3. Add error feedback to parseChatCommand](#3-add-error-feedback-to-parsechatcommand) +- [4. Reconcile service layer docs vs reality](#4-reconcile-service-layer-docs-vs-reality) +- [5. Consolidate GuildRepo mocks](#5-consolidate-guildrepo-mocks) +- [6. Add mocks for 8 unmocked repo interfaces](#6-add-mocks-for-8-unmocked-repo-interfaces) +- [7. Extract inline data tables from handler functions](#7-extract-inline-data-tables-from-handler-functions) + +--- + +## 1. Fix swallowed errors with nil-dereference risk + +**Priority:** High — latent panics triggered by any DB hiccup. + +~30 sites use `_, _` to discard repo/service errors. Three are dangerous because the returned value is used without a nil guard: + +| Location | Risk | +|----------|------| +| `handlers_guild_adventure.go:24,48,73` | `guild, _ := guildRepo.GetByCharID(...)` — no nil guard, will panic on DB error | +| `handlers_gacha.go:56` | `fp, gp, gt, _ := userRepo.GetGachaPoints(...)` — balance silently becomes 0, enabling invalid transactions | +| `handlers_house.go:167` | 7 return values from `GetHouseContents`, error discarded entirely | + +Additional sites that don't panic but produce silently wrong data: + +| Location | Issue | +|----------|-------| +| `handlers_distitem.go:35,111,129` | `distRepo.List()`/`GetItems()` errors become empty results, no logging | +| `handlers_guild_ops.go:30,49` | `guildService.Disband()`/`Leave()` errors swallowed (nil-safe due to `result != nil` guard, but invisible failures) | +| `handlers_shop.go:125,131` | Gacha type/weight lookups discarded | +| `handlers_discord.go:34` | `bcrypt.GenerateFromPassword` error swallowed (only fails on OOM) | + +**Fix:** Add error checks with logging and appropriate fail ACKs. For the three high-risk sites, add nil guards at minimum. + +**Status:** **Done.** All swallowed errors fixed across 7 files: + +- `handlers_guild_adventure.go` — 3 `GetByCharID` calls now check error and nil, early-return with ACK +- `handlers_gacha.go` — `GetGachaPoints` now checks error and returns zeroed response; `GetStepupStatus` logs error +- `handlers_house.go` — `GetHouseContents` now checks error and sends fail ACK; `HasApplication` moved inside nil guard to prevent nil dereference on `ownGuild`; `GetMission` and `GetWarehouseNames` now log errors +- `handlers_distitem.go` — 3 `distRepo` calls now log errors +- `handlers_guild_ops.go` — `Disband` and `Leave` service errors now logged +- `handlers_shop.go` — `GetShopType`, `GetWeightDivisor`, `GetFpointExchangeList` now log errors +- `handlers_discord.go` — `bcrypt.GenerateFromPassword` error now returns early with user-facing message + +--- + +## 2. Fix bookshelf data pointer for three game versions + +**Priority:** High — corrupts character save reads. + +From `technical-debt.md`: `model_character.go:88,101,113` has `TODO: fix bookshelf data pointer` for G10-ZZ, F4-F5, and S6 versions. All three offsets are off by exactly 14810 vs the consistent delta pattern of other fields. Needs validation against actual save data. + +**Fix:** Analyze save data from affected game versions to determine correct offsets. Apply fix and add regression test. + +**Status:** Pending. + +--- + +## 3. Add error feedback to parseChatCommand + +**Priority:** Medium — improves operator experience with low effort. + +`handlers_commands.go:71` is a 351-line switch statement dispatching 12 chat commands. Argument parsing errors (`strconv`, `hex.DecodeString`) are silently swallowed at lines 240, 256, 368, 369. Malformed commands silently use zero values instead of giving the operator feedback. + +**Fix:** On parse error, send a chat message back to the player explaining the expected format, then return early. Each command's branch already has access to the session for sending messages. + +**Status:** Pending. + +--- + +## 4. Reconcile service layer docs vs reality + +**Priority:** Medium — documentation mismatch causes confusion for contributors. + +The CLAUDE.md architecture section shows a clean `handlers → svc_*.go → repo_*.go` layering, but in practice: + +- **GuildService** has 7 methods. **GuildRepo** has 68. Handlers call `guildRepo` directly ~60+ times across 7 guild handler files. +- The 4 services (`GuildService`, `MailService`, `AchievementService`, `GachaService`) were extracted for operations requiring cross-repo coordination (e.g., disband triggers mail), but the majority of handler logic goes directly to repos. + +This isn't necessarily wrong — the services exist for multi-repo coordination, not as a mandatory pass-through. + +**Fix:** Update the architecture diagram in `CLAUDE.md` to reflect the actual pattern: services are used for cross-repo coordination, handlers call repos directly for simple CRUD. Remove the implication that all handlers go through services. Alternatively, expand service coverage to match the documented architecture, but that is a much larger effort with diminishing returns. + +**Status:** Pending. + +--- + +## 5. Consolidate GuildRepo mocks + +**Priority:** Low — reduces friction for guild test authoring. + +`repo_mocks_test.go` (1004 lines) has two separate GuildRepo mock types: + +- `mockGuildRepoForMail` (67 methods, 104 lines) — used by mail tests +- `mockGuildRepoOps` (38 methods, 266 lines) — used by ops/scout tests, with configurable behavior via struct fields + +The `GuildRepo` interface has 68 methods. Neither mock implements the full interface. Adding any new `GuildRepo` method requires updating both mocks or compilation fails. + +**Fix:** Merge into a single `mockGuildRepo` with all 68 methods as no-op defaults. Use struct fields (as `mockGuildRepoOps` already does for ~15 methods) for configurable returns in tests that need specific behavior. + +**Status:** Pending. + +--- + +## 6. Add mocks for 8 unmocked repo interfaces + +**Priority:** Low — enables isolated handler tests for more subsystems. + +8 of the 21 repo interfaces have no mock implementation: `TowerRepo`, `FestaRepo`, `RengokuRepo`, `DivaRepo`, `EventRepo`, `MiscRepo`, `MercenaryRepo`, `CafeRepo`. + +Tests for those handlers either use stub handlers that skip repos or rely on integration tests. This limits the ability to write isolated unit tests. + +**Fix:** Add no-op mock implementations for each, following the pattern established by existing mocks. + +**Status:** Pending. + +--- + +## 7. Extract inline data tables from handler functions + +**Priority:** Low — improves readability. + +`handlers_items.go:18` — `handleMsgMhfEnumeratePrice` (164 lines) embeds two large `var` data blocks inline in the function body. These are static data tables, not logic. + +**Fix:** Extract to package-level `var` declarations or a dedicated data file (following the pattern of `handlers_data_paper_tables.go`). + +**Status:** Pending. diff --git a/server/channelserver/handlers_discord.go b/server/channelserver/handlers_discord.go index 06b7dcf99..c282554c4 100644 --- a/server/channelserver/handlers_discord.go +++ b/server/channelserver/handlers_discord.go @@ -31,8 +31,18 @@ func (s *Server) onInteraction(ds *discordgo.Session, i *discordgo.InteractionCr }) } case "password": - password, _ := bcrypt.GenerateFromPassword([]byte(i.ApplicationCommandData().Options[0].StringValue()), 10) - err := s.userRepo.SetPasswordByDiscordID(i.Member.User.ID, password) + password, err := bcrypt.GenerateFromPassword([]byte(i.ApplicationCommandData().Options[0].StringValue()), 10) + if err != nil { + _ = ds.InteractionRespond(i.Interaction, &discordgo.InteractionResponse{ + Type: discordgo.InteractionResponseChannelMessageWithSource, + Data: &discordgo.InteractionResponseData{ + Content: "Failed to hash password.", + Flags: discordgo.MessageFlagsEphemeral, + }, + }) + return + } + err = s.userRepo.SetPasswordByDiscordID(i.Member.User.ID, password) if err == nil { _ = ds.InteractionRespond(i.Interaction, &discordgo.InteractionResponse{ Type: discordgo.InteractionResponseChannelMessageWithSource, diff --git a/server/channelserver/handlers_distitem.go b/server/channelserver/handlers_distitem.go index e626784d9..8d3c8fb76 100644 --- a/server/channelserver/handlers_distitem.go +++ b/server/channelserver/handlers_distitem.go @@ -32,7 +32,10 @@ func handleMsgMhfEnumerateDistItem(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfEnumerateDistItem) bf := byteframe.NewByteFrame() - itemDists, _ := s.server.distRepo.List(s.charID, pkt.DistType) + itemDists, err := s.server.distRepo.List(s.charID, pkt.DistType) + if err != nil { + s.logger.Error("Failed to list item distributions", zap.Error(err)) + } bf.WriteUint16(uint16(len(itemDists))) for _, dist := range itemDists { @@ -108,7 +111,10 @@ func handleMsgMhfApplyDistItem(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfApplyDistItem) bf := byteframe.NewByteFrame() bf.WriteUint32(pkt.DistributionID) - distItems, _ := s.server.distRepo.GetItems(pkt.DistributionID) + distItems, err := s.server.distRepo.GetItems(pkt.DistributionID) + if err != nil { + s.logger.Error("Failed to get distribution items", zap.Error(err)) + } bf.WriteUint16(uint16(len(distItems))) for _, item := range distItems { bf.WriteUint8(item.ItemType) @@ -126,7 +132,10 @@ func handleMsgMhfAcquireDistItem(s *Session, p mhfpacket.MHFPacket) { if pkt.DistributionID > 0 { err := s.server.distRepo.RecordAccepted(pkt.DistributionID, s.charID) if err == nil { - distItems, _ := s.server.distRepo.GetItems(pkt.DistributionID) + distItems, err := s.server.distRepo.GetItems(pkt.DistributionID) + if err != nil { + s.logger.Error("Failed to get distribution items for acquisition", zap.Error(err)) + } for _, item := range distItems { switch item.ItemType { case 17: diff --git a/server/channelserver/handlers_gacha.go b/server/channelserver/handlers_gacha.go index 348f446db..24f2448ac 100644 --- a/server/channelserver/handlers_gacha.go +++ b/server/channelserver/handlers_gacha.go @@ -53,7 +53,12 @@ func handleMsgMhfGetGachaPlayHistory(s *Session, p mhfpacket.MHFPacket) { func handleMsgMhfGetGachaPoint(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfGetGachaPoint) - fp, gp, gt, _ := s.server.userRepo.GetGachaPoints(s.userID) + fp, gp, gt, err := s.server.userRepo.GetGachaPoints(s.userID) + if err != nil { + s.logger.Error("Failed to get gacha points", zap.Error(err)) + doAckBufSucceed(s, pkt.AckHandle, make([]byte, 12)) + return + } resp := byteframe.NewByteFrame() resp.WriteUint32(gp) resp.WriteUint32(gt) @@ -159,7 +164,10 @@ func handleMsgMhfPlayStepupGacha(s *Session, p mhfpacket.MHFPacket) { func handleMsgMhfGetStepupStatus(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfGetStepupStatus) - status, _ := s.server.gachaService.GetStepupStatus(pkt.GachaID, s.charID, TimeAdjusted()) + status, err := s.server.gachaService.GetStepupStatus(pkt.GachaID, s.charID, TimeAdjusted()) + if err != nil { + s.logger.Error("Failed to get stepup status", zap.Error(err)) + } bf := byteframe.NewByteFrame() bf.WriteUint8(status.Step) diff --git a/server/channelserver/handlers_guild_adventure.go b/server/channelserver/handlers_guild_adventure.go index 316726516..a911c350d 100644 --- a/server/channelserver/handlers_guild_adventure.go +++ b/server/channelserver/handlers_guild_adventure.go @@ -21,7 +21,12 @@ type GuildAdventure struct { func handleMsgMhfLoadGuildAdventure(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfLoadGuildAdventure) - guild, _ := s.server.guildRepo.GetByCharID(s.charID) + guild, err := s.server.guildRepo.GetByCharID(s.charID) + if err != nil || guild == nil { + s.logger.Error("Failed to get guild for character", zap.Error(err)) + doAckBufSucceed(s, pkt.AckHandle, make([]byte, 1)) + return + } adventures, err := s.server.guildRepo.ListAdventures(guild.ID) if err != nil { s.logger.Error("Failed to get guild adventures from db", zap.Error(err)) @@ -45,7 +50,12 @@ func handleMsgMhfLoadGuildAdventure(s *Session, p mhfpacket.MHFPacket) { func handleMsgMhfRegistGuildAdventure(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfRegistGuildAdventure) - guild, _ := s.server.guildRepo.GetByCharID(s.charID) + guild, err := s.server.guildRepo.GetByCharID(s.charID) + if err != nil || guild == nil { + s.logger.Error("Failed to get guild for character", zap.Error(err)) + doAckSimpleSucceed(s, pkt.AckHandle, make([]byte, 4)) + return + } if err := s.server.guildRepo.CreateAdventure(guild.ID, pkt.Destination, TimeAdjusted().Unix(), TimeAdjusted().Add(6*time.Hour).Unix()); err != nil { s.logger.Error("Failed to register guild adventure", zap.Error(err)) } @@ -70,7 +80,12 @@ func handleMsgMhfChargeGuildAdventure(s *Session, p mhfpacket.MHFPacket) { func handleMsgMhfRegistGuildAdventureDiva(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfRegistGuildAdventureDiva) - guild, _ := s.server.guildRepo.GetByCharID(s.charID) + guild, err := s.server.guildRepo.GetByCharID(s.charID) + if err != nil || guild == nil { + s.logger.Error("Failed to get guild for character", zap.Error(err)) + doAckSimpleSucceed(s, pkt.AckHandle, make([]byte, 4)) + return + } if err := s.server.guildRepo.CreateAdventureWithCharge(guild.ID, pkt.Destination, pkt.Charge, TimeAdjusted().Unix(), TimeAdjusted().Add(1*time.Hour).Unix()); err != nil { s.logger.Error("Failed to register guild adventure", zap.Error(err)) } diff --git a/server/channelserver/handlers_guild_ops.go b/server/channelserver/handlers_guild_ops.go index af6d71c53..89c9a6c0d 100644 --- a/server/channelserver/handlers_guild_ops.go +++ b/server/channelserver/handlers_guild_ops.go @@ -27,7 +27,10 @@ func handleMsgMhfOperateGuild(s *Session, p mhfpacket.MHFPacket) { switch pkt.Action { case mhfpacket.OperateGuildDisband: - result, _ := s.server.guildService.Disband(s.charID, guild.ID) + result, err := s.server.guildService.Disband(s.charID, guild.ID) + if err != nil { + s.logger.Error("Failed to disband guild", zap.Error(err)) + } response := 0 if result != nil && result.Success { response = 1 @@ -46,7 +49,10 @@ func handleMsgMhfOperateGuild(s *Session, p mhfpacket.MHFPacket) { bf.WriteUint32(0) } case mhfpacket.OperateGuildLeave: - result, _ := s.server.guildService.Leave(s.charID, guild.ID, characterGuildInfo.IsApplicant, guild.Name) + result, err := s.server.guildService.Leave(s.charID, guild.ID, characterGuildInfo.IsApplicant, guild.Name) + if err != nil { + s.logger.Error("Failed to leave guild", zap.Error(err)) + } response := 0 if result != nil && result.Success { response = 1 diff --git a/server/channelserver/handlers_house.go b/server/channelserver/handlers_house.go index 35298335d..4df8bce40 100644 --- a/server/channelserver/handlers_house.go +++ b/server/channelserver/handlers_house.go @@ -147,8 +147,8 @@ func handleMsgMhfLoadHouse(s *Session, p mhfpacket.MHFPacket) { // Guild verification if state > 3 { ownGuild, err := s.server.guildRepo.GetByCharID(s.charID) - isApplicant, _ := s.server.guildRepo.HasApplication(ownGuild.ID, s.charID) if err == nil && ownGuild != nil { + isApplicant, _ := s.server.guildRepo.HasApplication(ownGuild.ID, s.charID) othersGuild, err := s.server.guildRepo.GetByCharID(pkt.CharID) if err == nil && othersGuild != nil { if othersGuild.ID == ownGuild.ID && !isApplicant { @@ -164,7 +164,12 @@ func handleMsgMhfLoadHouse(s *Session, p mhfpacket.MHFPacket) { } } - houseTier, houseData, houseFurniture, bookshelf, gallery, tore, garden, _ := s.server.houseRepo.GetHouseContents(pkt.CharID) + houseTier, houseData, houseFurniture, bookshelf, gallery, tore, garden, err := s.server.houseRepo.GetHouseContents(pkt.CharID) + if err != nil { + s.logger.Error("Failed to get house contents", zap.Error(err), zap.Uint32("charID", pkt.CharID)) + doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) + return + } if houseFurniture == nil { houseFurniture = make([]byte, 20) } @@ -201,7 +206,10 @@ func handleMsgMhfLoadHouse(s *Session, p mhfpacket.MHFPacket) { func handleMsgMhfGetMyhouseInfo(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfGetMyhouseInfo) - data, _ := s.server.houseRepo.GetMission(s.charID) + data, err := s.server.houseRepo.GetMission(s.charID) + if err != nil { + s.logger.Error("Failed to get myhouse mission", zap.Error(err)) + } if len(data) > 0 { doAckBufSucceed(s, pkt.AckHandle, data) } else { @@ -343,7 +351,10 @@ func handleMsgMhfOperateWarehouse(s *Session, p mhfpacket.MHFPacket) { switch pkt.Operation { case 0: var count uint8 - itemNames, equipNames, _ := s.server.houseRepo.GetWarehouseNames(s.charID) + itemNames, equipNames, err := s.server.houseRepo.GetWarehouseNames(s.charID) + if err != nil { + s.logger.Error("Failed to get warehouse names", zap.Error(err)) + } bf.WriteUint32(0) bf.WriteUint16(10000) // Usages temp := byteframe.NewByteFrame() diff --git a/server/channelserver/handlers_shop.go b/server/channelserver/handlers_shop.go index 9c33f8fa7..c958e2287 100644 --- a/server/channelserver/handlers_shop.go +++ b/server/channelserver/handlers_shop.go @@ -122,13 +122,19 @@ func handleMsgMhfEnumerateShop(s *Session, p mhfpacket.MHFPacket) { case 2: // Actual gacha bf := byteframe.NewByteFrame() bf.WriteUint32(pkt.ShopID) - gachaType, _ := s.server.gachaRepo.GetShopType(pkt.ShopID) + gachaType, err := s.server.gachaRepo.GetShopType(pkt.ShopID) + if err != nil { + s.logger.Error("Failed to get gacha shop type", zap.Error(err)) + } entries, err := s.server.gachaRepo.GetAllEntries(pkt.ShopID) if err != nil { doAckBufSucceed(s, pkt.AckHandle, make([]byte, 4)) return } - divisor, _ := s.server.gachaRepo.GetWeightDivisor(pkt.ShopID) + divisor, err := s.server.gachaRepo.GetWeightDivisor(pkt.ShopID) + if err != nil { + s.logger.Error("Failed to get gacha weight divisor", zap.Error(err)) + } bf.WriteUint16(uint16(len(entries))) for _, ge := range entries { var items []GachaItem @@ -262,7 +268,10 @@ func handleMsgMhfGetFpointExchangeList(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfGetFpointExchangeList) bf := byteframe.NewByteFrame() - exchanges, _ := s.server.shopRepo.GetFpointExchangeList() + exchanges, err := s.server.shopRepo.GetFpointExchangeList() + if err != nil { + s.logger.Error("Failed to get fpoint exchange list", zap.Error(err)) + } var buyables uint16 for _, e := range exchanges { if e.Buyable {