Mirror/Erupe
1
0
Fork 0
mirror of https://github.com/Mezeporta/Erupe.git synced 2026-03-22 07:32:32 +01:00
Code Issues Packages Projects Releases Wiki Activity

chore: fix typos, remove stale comment, and update codecov-action to v4

Browse Source 
- Remove misleading "For Debuging" comment in sys_session.go
- Fix "offical" → "official" typo in handlers_session.go
- Update codecov-action@v3 → @v4 in CI workflow
- Consolidate technical-debt.md with completed items and updated TOC
This commit is contained in:
Houmgaor
2026-02-22 18:20:09 +01:00
parent 82b967b715
commit de00e41830
4 changed files with 51 additions and 86 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
.github/workflows/go.yml vendored
View File

@@ -44,7 +44,7 @@ jobs:
run: go test -race -coverprofile=coverage.out ./... -timeout=10m run: go test -race -coverprofile=coverage.out ./... -timeout=10m
- name: Upload Coverage to Codecov - name: Upload Coverage to Codecov
uses: codecov/codecov-action@v3 uses: codecov/codecov-action@v4
with: with:
files: ./coverage.out files: ./coverage.out
flags: unittests flags: unittests

132
docs/technical-debt.md
View File

@@ -1,6 +1,6 @@
# Erupe Technical Debt & Suggested Next Steps # Erupe Technical Debt & Suggested Next Steps
> Analysis date: 2026-02-22 > Last updated: 2026-02-22
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. 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.
@@ -9,15 +9,12 @@ This document tracks actionable technical debt items discovered during a codebas
- [High Priority](#high-priority) - [High Priority](#high-priority)
- [1. Broken game features (gameplay-impacting TODOs)](#1-broken-game-features-gameplay-impacting-todos) - [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) - [2. Test gaps on critical paths](#2-test-gaps-on-critical-paths)
- [3. Sign server has no repository layer](#3-sign-server-has-no-repository-layer)
- [Medium Priority](#medium-priority) - [Medium Priority](#medium-priority)
- [4. Split repo_guild.go](#4-split-repo_guildgo) - [3. Logging anti-patterns](#3-logging-anti-patterns)
- [5. Logging anti-patterns](#5-logging-anti-patterns) - [4. Typos and stale comments](#4-typos-and-stale-comments)
- [6. Inconsistent transaction API](#6-inconsistent-transaction-api)
- [7. LoopDelay config has no Viper default](#7-loopdelay-config-has-no-viper-default)
- [Low Priority](#low-priority) - [Low Priority](#low-priority)
- [8. Typos and stale comments](#8-typos-and-stale-comments) - [5. CI updates](#5-ci-updates)
- [9. CI updates](#9-ci-updates) - [Completed Items](#completed-items)
- [Suggested Execution Order](#suggested-execution-order) - [Suggested Execution Order](#suggested-execution-order)
--- ---
@@ -31,31 +28,26 @@ These TODOs represent features that are visibly broken for players.
| Location | Issue | Impact | | Location | Issue | Impact |
|----------|-------|--------| |----------|-------|--------|
| `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 | | `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 |
| ~~`handlers_guild.go:389`~~ | ~~`TODO: Implement month-by-month tracker` — always returns `0x01` (claimed)~~ | ~~Players can never claim monthly guild items~~ **Fixed.** Now tracks per-character per-type monthly claims via `stamps` table. |
| `handlers_guild_ops.go:148` | `TODO: Move this value onto rp_yesterday and reset to 0... daily?` | Guild daily RP rollover logic is missing entirely | | `handlers_guild_ops.go:148` | `TODO: Move this value onto rp_yesterday and reset to 0... daily?` | Guild daily RP rollover logic is missing entirely |
| `handlers_achievement.go:125` | `TODO: Notify on rank increase` — always returns `false` | Achievement rank-up notifications are silently suppressed | | `handlers_achievement.go:125` | `TODO: Notify on rank increase` — always returns `false` | Achievement rank-up notifications are silently suppressed |
| `handlers_guild_info.go:443` | `TODO: Enable GuildAlliance applications` — hardcoded `true` | Guild alliance applications are always open regardless of setting | | `handlers_guild_info.go:443` | `TODO: Enable GuildAlliance applications` — hardcoded `true` | Guild alliance applications are always open regardless of setting |
| `handlers_session.go:397` | `TODO(Andoryuuta): log key index off-by-one` | Known off-by-one in log key indexing is unresolved | | `handlers_session.go:394` | `TODO(Andoryuuta): log key index off-by-one` | Known off-by-one in log key indexing is unresolved |
| `handlers_session.go:577` | `TODO: This case might be <=G2` | Uncertain version detection in switch case | | `handlers_session.go:535` | `TODO: This case might be <=G2` | Uncertain version detection in switch case |
| `handlers_session.go:777` | `TODO: Retail returned the number of clients in quests` | Player count reported to clients does not match retail behavior | | `handlers_session.go:698` | `TODO: Retail returned the number of clients in quests` | Player count reported to clients does not match retail behavior |
### 2. Test gaps on critical paths ### 2. Test gaps on critical paths
**Handler files with no test file:** **Handler files with no test file (7 remaining):**
| File | Lines | Priority | Reason | | File | Priority | Reason |
|------|-------|----------|--------| |------|----------|--------|
| `handlers_session.go` | 833 | HIGH | Login/logout, log key, character enumeration | | `handlers_commands.go` | HIGH | Admin command system |
| `handlers_gacha.go` | 411 | HIGH | Economy system with DB writes | | `handlers_data_paper.go` | MEDIUM | Daily paper data |
| `handlers_commands.go` | 421 | HIGH | Admin command system | | `handlers_seibattle.go` | MEDIUM | Sei battle system |
| `handlers_data_paper.go` | 621 | MEDIUM | Daily paper data | | `handlers_scenario.go` | LOW | Mostly complete, uses repo |
| `handlers_plate.go` | 294 | MEDIUM | Armor plate system | | `handlers_distitem.go` | LOW | Distribution items |
| `handlers_shop.go` | 291 | MEDIUM | Shopping system | | `handlers_guild_mission.go` | LOW | Guild missions |
| `handlers_seibattle.go` | 259 | MEDIUM | Sei battle system | | `handlers_kouryou.go` | LOW | Kouryou system |
| `handlers_scenario.go` | ~100 | LOW | Mostly complete, uses repo |
| `handlers_distitem.go` | small | LOW | Distribution items |
| `handlers_guild_mission.go` | small | LOW | Guild missions |
| `handlers_kouryou.go` | small | LOW | Kouryou system |
**Repository files with no store-level test file (17 total):** **Repository files with no store-level test file (17 total):**
@@ -63,83 +55,57 @@ These TODOs represent features that are visibly broken for players.
These are validated indirectly through mock-based handler tests but have no SQL-level integration tests. These are validated indirectly through mock-based handler tests but have no SQL-level integration tests.
### 3. Sign server has no repository layer
The channelserver was refactored to use repository interfaces (commits `a9cca84`, `6fbd294`, `1d5026c`), but `server/signserver/` was not included. It still does raw `db.QueryRow`/`db.Exec` with **8 silently discarded errors** on write paths:
```
server/signserver/dbutils.go:86,91,94,100,107,123,149
server/signserver/session.go
server/signserver/dsgn_resp.go
```
Examples of discarded errors (login timestamps, return-to-player expiry, rights queries):
```go
_, _ = s.db.Exec("UPDATE users SET return_expires=$1 WHERE id=$2", returnExpiry, uid)
_ = s.db.QueryRow("SELECT last_character FROM users WHERE id=$1", uid).Scan(&lastPlayed)
```
A database connectivity issue during login would be invisible.
--- ---
## Medium Priority ## Medium Priority
### 4. Split `repo_guild.go` ### 3. Logging anti-patterns
At 1004 lines with 71 functions, `repo_guild.go` mixes 6 distinct concerns:
- Guild CRUD and metadata
- Member management
- Applications/recruitment
- RP tracking
- Item box operations
- Message board posts
Suggested split: `repo_guild.go` (core CRUD), `repo_guild_members.go`, `repo_guild_items.go`, `repo_guild_board.go`.
### 5. 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`. ~~**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. ~~**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.
### ~~6. Inconsistent transaction API~~ (Fixed) ### 4. Typos and stale comments
**Status:** Fixed. All transaction call sites now use `BeginTxx(context.Background(), nil)` with deferred rollback, replacing the old `Begin()` + manual rollback pattern across `repo_guild.go`, `repo_guild_rp.go`, `repo_festa.go`, and `repo_event.go`. | Location | Issue |
|----------|-------|
### ~~7. `LoopDelay` config has no Viper default~~ (Fixed) | `sys_session.go:73` | Comment says "For Debuging" — typo ("Debugging"), and the field is used in production logging, not just debugging |
| `handlers_session.go:394` | "offical" should be "official" |
**Status:** Fixed. `viper.SetDefault("LoopDelay", 50)` added in `config/config.go`, matching the `config.example.json` value. | `handlers_session.go:322` | `if s.server.db != nil` guard wraps repo calls — leaky abstraction from the pre-repository refactor |
--- ---
## Low Priority ## Low Priority
### 8. Typos and stale comments ### 5. CI updates
| Location | Issue | - `codecov-action@v4` could be updated to `v5` (current stable)
|----------|-------|
| `sys_session.go:73` | Comment says "For Debuging" — typo, and the field is used in production logging, not just debugging |
| `handlers_session.go:397` | "offical" should be "official" |
| `handlers_session.go:324` | `if s.server.db != nil` guard wraps repo calls that are already nil-safe — refactoring artifact |
### 9. CI updates
- `codecov-action@v3` could be updated to `v4` (current stable)
- No coverage threshold is enforced — coverage is uploaded but regressions aren't caught - No coverage threshold is enforced — coverage is uploaded but regressions aren't caught
--- ---
## 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`. |
| — | **Entrance server raw SQL** | Refactored to repository interfaces (`repo_interfaces.go`, `repo_session.go`, `repo_server.go`). |
---
## Suggested Execution Order ## Suggested Execution Order
Based on impact and the momentum from recent repo-interface refactoring: Based on remaining impact:
1. **Add tests for `handlers_session.go` and `handlers_gacha.go`** — highest-risk untested code on the critical login and economy paths 1. **Add tests for `handlers_commands.go`** — highest-risk remaining untested handler (admin commands)
2. **Refactor signserver to use repository interfaces** — completes the pattern established in channelserver and surfaces 8 hidden error paths 2. **Fix bookshelf data pointer** (`model_character.go`) — corrupts saves for three game versions
3. ~~**Fix monthly guild item claim**~~ (`handlers_guild.go:389`) — **Done** 3. **Implement guild daily RP rollover** (`handlers_guild_ops.go:148`) — missing game feature
4. **Split `repo_guild.go`** — last oversized file after the recent refactoring push 4. **Fix typos** (`sys_session.go:73`, `handlers_session.go:394`) — quick cleanup
5. ~~**Fix `fmt.Sprintf` in logger calls**~~ — **Done** 5. **Update `codecov-action` to v5** and add coverage threshold — prevents regressions
6. ~~**Add `LoopDelay` Viper default**~~ — **Done**
7. ~~**Log SJIS decoding errors**~~ — **Done**
8. ~~**Standardize on `BeginTxx`**~~ — **Done**

2
server/channelserver/handlers_session.go
View File

@@ -391,7 +391,7 @@ func handleMsgSysIssueLogkey(s *Session, p mhfpacket.MHFPacket) {
return return
} }
// TODO(Andoryuuta): In the offical client, the log key index is off by one, // TODO(Andoryuuta): In the official client, the log key index is off by one,
// cutting off the last byte in _most uses_. Find and document these accordingly. // cutting off the last byte in _most uses_. Find and document these accordingly.
s.Lock() s.Lock()
s.logKey = logKey s.logKey = logKey

1
server/channelserver/sys_session.go
View File

@@ -70,7 +70,6 @@ type Session struct {
// Contains the mail list that maps accumulated indexes to mail IDs // Contains the mail list that maps accumulated indexes to mail IDs
mailList []int mailList []int
// For Debuging
Name string Name string
closed atomic.Bool closed atomic.Bool
ackStart map[uint32]time.Time ackStart map[uint32]time.Time