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

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

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,

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:

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)

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))
 	}

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

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()

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 {