diff --git a/.gitignore b/.gitignore index 3dd5b55..c5bc50f 100644 --- a/.gitignore +++ b/.gitignore @@ -75,3 +75,18 @@ uploads/avatars/* # Backup temp backup_temp/ + +# SQLite temp files +sub2api +sub2api-shm +sub2api-wal + +# Codex temp +.codex-tmp/ + +# Workbuddy memory (local AI memory, not project files) +.workbuddy/memory/ +.workbuddy/expert-history.json + +# Test coverage output +frontend/admin/coverage/ diff --git a/docs/code-review/SENIOR_DEV_REVIEW_2026-04-10.md b/docs/code-review/SENIOR_DEV_REVIEW_2026-04-10.md new file mode 100644 index 0000000..c25e4d6 --- /dev/null +++ b/docs/code-review/SENIOR_DEV_REVIEW_2026-04-10.md @@ -0,0 +1,550 @@ +# 资深工程师代码 Review 报告 + +**项目**：用户管理系统（UMS） +**Review 日期**：2026-04-10 23:45 +**分支**：`fix/status-review-sync-20260409` +**Reviewer**：资深全栈工程师 +**Review 范围**：后端 Go + 前端 React/TS，全项目维度 + +--- + +## 执行摘要 + +> 本次 Review 基于**真实工具执行结果**（go build / go test / 覆盖率数据 / 代码扫描），不依赖文档自述。 + +| 维度 | 评分 | 状态 | +|------|------|------| +| 构建稳定性 | **9/10** | ✅ 全链路编译通过 | +| 测试覆盖率 | **4/10** | 🔴 核心层极低（Service 15.2%，Handler 15.7%）| +| 代码质量 | **6.5/10** | 🟠 存在 Stub 谎报、职责混乱等问题 | +| 安全实践 | **7/10** | 🟡 基础加固到位，中级加固有缺口 | +| 架构设计 | **6/10** | 🟠 分层存在渗漏，Service 依赖具体实现 | +| 工程规范 | **6/10** | 🟠 行尾符乱、文档滞后、魔法数字残留 | +| **综合评分** | **6.4/10** | ⚠️ **不达上线标准** | + +--- + +## 一、构建与基础质量（实测数据） + +### 1.1 编译结果 + +``` +go build ./cmd/server ✅ PASS +go vet ./... ✅ PASS（无警告） +go test ./... -short ✅ PASS（所有包通过） +``` + +**结论**：基础工程卫生合格，无编译错误，无 vet 警告。 + +### 1.2 测试覆盖率——真实扫描结果 + +| 包 | 覆盖率 | 评价 | +|----|--------|------| +| `internal/api/handler` | **15.7%** | 🔴 严重不足 | +| `internal/service` | **15.2%** | 🔴 严重不足 | +| `internal/api/middleware` | **21.5%** | 🔴 严重不足 | +| `internal/auth` | **28.1%** | 🔴 不足（安全敏感） | +| `internal/repository` | **47.1%** | 🟡 中等，需提升 | +| `internal/security` | **37.9%** | 🟡 中等 | +| `internal/config` | **85.2%** | ✅ 良好 | +| `internal/auth/providers` | **80.6%** | ✅ 良好 | +| `internal/pkg/proxyurl` | **100%** | ✅ 优秀 | +| `internal/pagination` | **0.0%** | 🔴 无测试（游标分页核心模块！） | +| `internal/domain` | **2.7%** | 🔴 基本零测试 | + +**核心问题**：Handler 和 Service 是业务逻辑的关键层，覆盖率双双仅 15%，意味着 85% 的业务逻辑完全没有测试保护。这是目前最危险的质量问题。 + +--- + +## 二、代码问题清单 + +### P0 - 文档声称已实现，代码实为 Stub + +**问题位置**：`internal/api/handler/user_handler.go:337-339` + +```go +func (h *UserHandler) UploadAvatar(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"message": "avatar upload not implemented"}) +} +``` + +**严重性**：🔴 P0 +**说明**：`docs/code-review/PROJECT_REAL_COMPLETION_REVIEW_2026-04-10.md` 明确写道"Avatar Upload — 已实现且已验证"，甚至列出了测试场景（UploadAvatar_Unauthorized、UploadAvatar_NonAdminCannotUpdateOther）。但 Handler 层函数体仅返回 `"avatar upload not implemented"`，是纯 stub。Service 层也没有 `UploadAvatar` 函数。这是文档声称与真实代码**完全矛盾**的典型案例——也是团队中"Live 不等于闭环"原则被违反的直接证据。 + +**修复方向**： +1. 实现真实的 multipart 文件接收、校验（大小/类型）、存储逻辑 +2. 添加 Service 层 `UploadAvatar` 方法 +3. 对失败路径实现文件清理（cleanup on partial write） +4. 补充真实 401/403/413 响应测试 + +--- + +### P0 - AdminRoleID 硬编码魔法数字 + +**问题位置**：`internal/service/user_service.go:284` + +```go +const AdminRoleID = 1 +``` + +**严重性**：🔴 P0 +**说明**：这是典型的魔法常量设计反模式。管理员角色的 ID 完全依赖数据库插入顺序，在以下场景会直接断裂： +- 数据库迁移到新环境 +- 插入顺序变化（数据 seed 逻辑修改） +- 多租户场景 + +**修复方向**：通过角色 `code` 字段（如 `"admin"`）动态查询角色 ID，不要依赖自增 ID。 + +```go +// 正确做法：通过 code 查询 +adminRole, err := s.roleRepo.GetByCode(ctx, "admin") +``` + +--- + +### P1 - Service 层依赖具体实现而非接口 + +**问题位置**：`internal/service/user_service.go:17-24` + +```go +type UserService struct { + userRepo *repository.UserRepository // ← 具体类型 + userRoleRepo *repository.UserRoleRepository // ← 具体类型 + roleRepo *repository.RoleRepository // ← 具体类型 + passwordHistoryRepo *repository.PasswordHistoryRepository // ← 具体类型 +} +``` + +**严重性**：🟠 P1 +**说明**：Service 层直接依赖 Repository 具体结构体，而非接口。这违反了依赖倒置原则（DIP），导致： +1. 无法对 Service 层进行单元测试（需要真实数据库） +2. 无法 Mock 依赖（这是覆盖率仅 15% 的根因之一） +3. 切换数据库实现或添加缓存层时，需要修改 Service 代码 + +**这是覆盖率低的架构根因**，必须优先解决。 + +**修复方向**： +```go +// 定义接口 +type UserRepository interface { + GetByID(ctx context.Context, id int64) (*domain.User, error) + Create(ctx context.Context, user *domain.User) error + // ... +} + +// Service 依赖接口 +type UserService struct { + userRepo UserRepository + // ... +} +``` + +--- + +### P1 - AssignRoles 删旧建新非事务，存在数据竞争风险 + +**问题位置**：`internal/service/user_service.go:267-280` + +```go +// 删除用户现有角色 +if err := s.userRoleRepo.DeleteByUserID(ctx, userID); err != nil { + return err +} + +// 创建新的用户角色关联（←非原子操作，删旧成功但建新失败 → 用户无角色） +var userRoles []*domain.UserRole +for _, roleID := range roleIDs { + userRoles = append(userRoles, &domain.UserRole{...}) +} +return s.userRoleRepo.BatchCreate(ctx, userRoles) +``` + +**严重性**：🟠 P1 +**说明**：删除旧角色和创建新角色之间没有事务包装。若 BatchCreate 失败，用户角色会被清空（陷入无角色状态）。并发请求场景下窗口期内用户权限会出现短暂真空。 + +**修复方向**：用 DB 事务包装整个操作： +```go +return s.db.Transaction(func(tx *gorm.DB) error { + if err := s.userRoleRepo.WithTx(tx).DeleteByUserID(ctx, userID); err != nil { + return err + } + return s.userRoleRepo.WithTx(tx).BatchCreate(ctx, userRoles) +}) +``` + +--- + +### P1 - ListAdmins / GetUserRoles 存在 N+1 查询问题 + +**问题位置**：`internal/service/user_service.go:241-247` 和 `299-307` + +```go +// N+1 查询反模式 +for _, roleID := range roleIDs { + role, err := s.roleRepo.GetByID(ctx, roleID) // ← 每个角色一次查询 + // ... +} +``` + +同样的模式在 `ListAdmins` 中： +```go +for _, adminID := range adminUserIDs { + user, err := s.userRepo.GetByID(ctx, adminID) // ← 每个用户一次查询 +} +``` + +**严重性**：🟠 P1 +**说明**：N+1 查询在角色/管理员数量增长时会导致明显性能退化。100 个管理员 = 101 次数据库查询。 + +**修复方向**： +```go +// Repository 提供批量查询方法 +roles, err := s.roleRepo.GetByIDs(ctx, roleIDs) + +// 同样用于用户列表 +users, err := s.userRepo.GetByIDs(ctx, adminUserIDs) +``` + +--- + +### P1 - 密码修改中哈希计算重复两次 + +**问题位置**：`internal/service/user_service.go:81-104` + +```go +// 第一次哈希（用于历史记录） +newHashedPassword, hashErr := auth.HashPassword(newPassword) + +// ... goroutine 里保存历史 ... + +// 第二次哈希（用于更新用户密码）← 重复计算！ +newHashedPassword, err := auth.HashPassword(newPassword) +user.Password = newHashedPassword +``` + +**严重性**：🟠 P1 +**说明**：Argon2id（64MB 内存，5 次迭代）的哈希计算成本很高，对同一密码哈希两次是纯浪费。此外代码有逻辑问题：若历史记录分支进入 goroutine，主流程再哈希一次，两次结果是不同的哈希（因为 Argon2 包含随机盐），但这不是主要问题——主要问题是性能浪费和代码逻辑不清晰。 + +**修复方向**：哈希一次，复用结果： +```go +newHashedPassword, err := auth.HashPassword(newPassword) +if err != nil { + return errors.New("密码哈希失败") +} +// 复用 newHashedPassword 给历史记录和用户更新 +``` + +--- + +### P2 - 响应格式不统一 + +**问题位置**：`internal/api/handler/user_handler.go` + +多处响应格式不一致： +```go +// 有的接口使用 code/message/data 包装 +c.JSON(http.StatusCreated, gin.H{ + "code": 0, + "message": "success", + "data": toUserResponse(user), +}) + +// 有的接口裸返回 +c.JSON(http.StatusOK, toUserResponse(user)) // GetUser + +// 有的返回字符串 +c.JSON(http.StatusOK, gin.H{"message": "user deleted"}) // DeleteUser +``` + +**严重性**：🟡 P2 +**说明**：前端需要处理三种不同的响应结构，这是前后端联调噩梦的来源。 + +--- + +### P2 - 行尾符污染（git 警告已暴露） + +**问题位置**：15 个文件存在 LF/CRLF 混用 + +``` +warning: in the working copy of 'internal/api/handler/user_handler.go', +LF will be replaced by CRLF the next time Git touches it +``` + +**严重性**：🟡 P2 +**说明**：Windows 开发环境下 git 行尾符不一致会影响 diff 可读性、代码审查效率，以及跨平台 CI/CD。 + +**修复方向**：在 `.gitattributes` 中强制统一行尾符： +``` +* text=auto eol=lf +*.go text eol=lf +*.ts text eol=lf +*.tsx text eol=lf +``` + +--- + +### P2 - JWT 密钥缺乏启动时强制校验 + +**问题位置**：`configs/config.yaml:57` + +```yaml +jwt: + secret: "" # ⚠️ 生产环境必须通过 JWT_SECRET 环境变量设置 +``` + +**严重性**：🟡 P2 +**说明**：注释写明了"必须通过环境变量设置"，但代码是否在启动时强制检查（release 模式下 secret 为空则拒绝启动）？若没有，服务会以空密钥运行，所有 JWT 签名均可伪造。 + +需要在启动代码中验证： +```go +if cfg.Server.Mode == "release" && cfg.JWT.Secret == "" { + log.Fatal("FATAL: JWT_SECRET must be set in release mode") +} +``` + +--- + +## 三、架构评估 + +### 3.1 优点（值得肯定） + +| 方面 | 亮点 | +|------|------| +| **Argon2id** | 密码哈希使用 Argon2id，参数配置合理（64MB/5次/4并行）✅ | +| **crypto/rand** | 所有随机数使用 `crypto/rand`，无 `math/rand` ✅ | +| **游标分页** | Sprint 18 实现的 Cursor 分页设计扎实，keyset 模式正确 ✅ | +| **SQLite WAL** | WAL 模式 + PRAGMA 调优，体现了工程意识 ✅ | +| **Token 轮换** | Refresh Token 滚动轮换防无限流实现正确 ✅ | +| **非 root 容器** | Dockerfile 使用非 root 用户运行 ✅ | +| **健康检查** | Docker HEALTHCHECK 已配置 ✅ | +| **CSRF 保护** | CSRF token 机制存在且有效 ✅ | + +### 3.2 架构债务 + +``` +┌─────────────────────────────────────────────────────┐ +│ Handler 层 │ +│ ✅ 职责基本清晰，但响应格式不统一 │ +└─────────────────────────────────────────────────────┘ + │ 调用（具体类型 ↓） +┌─────────────────────────────────────────────────────┐ +│ Service 层 ⚠️ │ +│ - 依赖具体 Repository 结构体（违反 DIP） │ +│ - 存在 N+1 查询 │ +│ - AdminRoleID 硬编码 │ +│ - 无事务包装的多步操作 │ +└─────────────────────────────────────────────────────┘ + │ 调用（直接依赖 ↓） +┌─────────────────────────────────────────────────────┐ +│ Repository 层 ✅ │ +│ - GORM 使用规范 │ +│ - 游标分页实现正确 │ +│ - LIKE 注入防护已处理 │ +└─────────────────────────────────────────────────────┘ +``` + +--- + +## 四、安全评估 + +| 安全点 | 状态 | 说明 | +|--------|------|------| +| 密码哈希算法 | ✅ 优秀 | Argon2id 配置合理 | +| 随机数生成 | ✅ 优秀 | 全部 crypto/rand | +| JWT JTI | ✅ 良好 | timestamp+random 格式 | +| Token 轮换 | ✅ 良好 | 滚动轮换防重放 | +| access_token 存储 | ✅ 良好 | 内存存储，非 localStorage | +| CSRF 保护 | ✅ 良好 | 机制存在且已验证 | +| 容器安全 | ✅ 良好 | 非 root 用户 | +| JWT 密钥强制校验 | ⚠️ 缺口 | release 模式未见强制启动失败 | +| 登录响应时序 | ✅ 已修复 | 常数时间比较 | +| `GetUserRoles` 授权 | ✅ 已修复 | self/admin 验证已添加 | +| 文件上传安全 | 🔴 Stub | `UploadAvatar` 未实现，无校验逻辑 | +| gosec 扫描 | ❓ 未知 | `gosec-report.json` 存在但本次未分析 | + +--- + +## 五、工程规范评估 + +### 5.1 Git 规范 + +- ✅ 提交信息格式规范（`feat:`/`fix:`/`test:`/`docs:` 前缀） +- ✅ 功能分支隔离（`fix/status-review-sync-20260409`） +- ⚠️ **行尾符污染**：15 个文件存在 LF/CRLF 混用，git 已在每次操作时发出警告，需要通过 `.gitattributes` 根治 + +### 5.2 文档一致性 + +- 🔴 **严重文档漂移**：`PROJECT_REAL_COMPLETION_REVIEW_2026-04-10.md` 声称 "Avatar Upload — 已实现且已验证"，实际代码为纯 stub（`"avatar upload not implemented"`）。文档与代码存在**直接矛盾**。 +- ✅ 有历史 Sprint 记录的习惯，审计链路清晰 +- 🟡 多份 Review 报告（24 个文件）存在重叠和相互矛盾的结论，容易造成认知混乱 + +### 5.3 测试规范 + +| 测试类型 | 状态 | +|--------|------| +| 后端单元测试 | ⚠️ 存在但覆盖率极低（15-28%）| +| 后端集成测试 | ✅ 有 `internal/integration/` 包 | +| 前端单元测试 | ✅ 325 测试通过，无 jsdom 噪声 | +| E2E 测试 | ⚠️ 脚本存在但环境变量问题未解决 | +| 性能测试 | ✅ 有 `internal/performance/` 包 | + +--- + +## 六、前端质量评估 + +| 维度 | 状态 | 说明 | +|------|------|------| +| TypeScript 严格模式 | ✅ | tsconfig 启用 strict | +| 构建 | ✅ | Vite 构建通过 | +| Lint | ✅ | ESLint 通过，无错误 | +| 单元测试 | ✅ | 325 测试，无噪声 | +| jsdom 噪声 | ✅ | 已修复（window.alert mock）| +| 401 刷新机制 | ✅ | 单次刷新 + 并发锁 | +| Token 存储 | ✅ | access_token 内存，refresh_token HttpOnly Cookie | +| 设备信任 | ⚠️ | localStorage 持久化，但 device_id 为随机值 | +| 响应格式处理 | 🟠 | 需适配不一致的后端响应格式 | + +--- + +## 七、改进路线图 + +### 第一阶段：P0 修复（必须在下一个 PR 完成） + +**优先级**：不修复不允许声称上线就绪 + +| # | 任务 | 预估工时 | 负责人 | +|---|------|----------|--------| +| 1 | 实现真实的 `UploadAvatar` Handler（文件校验+存储+错误清理） | 3h | 后端 | +| 2 | 添加 Service 层 `UploadAvatar` 方法 | 1h | 后端 | +| 3 | 将 `AdminRoleID` 从硬编码改为动态查询 role code | 1h | 后端 | +| 4 | 更新文档，同步真实状态（删除虚假"已验证"结论） | 0.5h | 全体 | + +### 第二阶段：P1 架构修复（本周完成） + +| # | 任务 | 预估工时 | 团队收益 | +|---|------|----------|----------| +| 1 | 为 Repository 层提取接口（UserRepository/RoleRepository 等） | 4h | 解锁 Service 单元测试，覆盖率可从 15% → 60%+ | +| 2 | 用 DB 事务包装 `AssignRoles` 的删旧建新操作 | 1h | 消除数据竞争窗口 | +| 3 | 为 `GetUserRoles` / `ListAdmins` 提供批量查询方法（消除 N+1） | 2h | 性能提升 | +| 4 | 统一 Handler 响应格式（全部使用 code/message/data 结构） | 2h | 前端联调质量提升 | +| 5 | release 模式下 JWT secret 空值强制启动失败 | 0.5h | 消除安全漏洞 | + +### 第三阶段：P2 工程规范（本月完成） + +| # | 任务 | 预估工时 | +|---|------|----------| +| 1 | 添加 `.gitattributes` 统一行尾符（LF） | 0.5h | +| 2 | 将 `internal/pagination` 包覆盖率从 0% 提升至 80%+ | 2h | +| 3 | 将 Handler/Service 覆盖率目标提升至 60%（通过接口+mock 解锁） | 8h | +| 4 | 解析 `gosec-report.json`，修复 SEC 级别问题 | 2h | +| 5 | 整合多份 Review 文档，归档旧版，保留单一权威状态文档 | 1h | + +--- + +## 八、团队技术能力提升建议 + +基于本次 Review，针对团队现状提出以下系统性建议： + +### 8.1 必须立即建立的编码规范 + +**规范 1：Service 层必须面向接口编程** +```go +// ❌ 错误做法（当前状态） +type UserService struct { + userRepo *repository.UserRepository +} + +// ✅ 正确做法 +type UserRepository interface { + GetByID(ctx context.Context, id int64) (*domain.User, error) + Create(ctx context.Context, user *domain.User) error +} + +type UserService struct { + userRepo UserRepository +} +``` + +**规范 2：多步数据库操作必须用事务** +```go +// ❌ 危险做法（当前状态） +s.userRoleRepo.DeleteByUserID(ctx, userID) // 失败后下面不执行 +s.userRoleRepo.BatchCreate(ctx, userRoles) // 成功但上面失败 → 数据不一致 + +// ✅ 正确做法 +db.Transaction(func(tx *gorm.DB) error { + if err := roleRepo.WithTx(tx).DeleteByUserID(ctx, userID); err != nil { + return err // 自动回滚 + } + return roleRepo.WithTx(tx).BatchCreate(ctx, userRoles) +}) +``` + +**规范 3：文档必须与代码同步，禁止超前声称** +- 合并门禁：PR 描述中的"已实现"必须附带 grep 证据或测试截图 +- 函数体内有 `"not implemented"` 字符串的接口，不允许在文档中标注为"已实现" + +### 8.2 测试文化建设 + +当前团队测试覆盖率极低（核心层 15%）的根本原因是**架构不支持测试**——Service 依赖具体类型导致无法 Mock。 + +建立以下测试规范： + +1. **新功能必须先写测试**（TDD）：不是要求 100% 覆盖，而是核心 happy path + 主要错误路径 +2. **单元测试必须可以离线运行**：不依赖真实数据库（通过接口+mock 实现） +3. **覆盖率下限**：Service 层 ≥ 60%，Handler 层 ≥ 50%（当前目标，通过接口重构后可达） + +### 8.3 代码 Review 要求（从下一个 PR 开始执行） + +PR 描述必须包含： +1. **变更原因**（1-2 句） +2. **实际执行过的验证命令及输出**（不接受"应该通过"这种表述） +3. **影响范围说明**（后端/前端/数据库结构） +4. **Checklist**： + - [ ] `go build ./...` 通过 + - [ ] `go vet ./...` 无警告 + - [ ] `go test ./... -short` 通过 + - [ ] 新增代码有对应测试 + - [ ] 文档已同步 + +--- + +## 九、诚实状态评估 + +基于本次实测，以下是可以诚实声称的状态： + +### ✅ 可以诚实声称 + +- 后端全量测试通过（-short 模式） +- `go build` / `go vet` 零错误 +- 前端 325 单元测试通过，lint/build 绿灯 +- Argon2id 密码安全、Token 机制、CSRF 保护已到位 +- 游标分页设计正确，P99 延迟满足 SLA（<100ms） +- 非 root 容器、健康检查、WAL 模式已配置 + +### ❌ 不可以声称 + +- "Avatar Upload 已实现" — **虚假，Handler 是 stub** +- "核心业务逻辑有充分测试保护" — Handler/Service 覆盖率 15%，远不充分 +- "架构设计符合 DIP 原则" — Service 依赖具体类型，违反 DIP +- "E2E 主入口已验证" — 脚本存在环境变量问题，未完成完整验证 +- "项目达到上线标准" — P0 问题（Stub 谎报）未解决 + +--- + +## 十、附：资深工程师给团队的话 + +这个项目整体基础不差——安全加固方向是对的，游标分页的工程思维体现了对性能的重视，Sprint 制度的执行留下了清晰的审计链。这些都是值得保持的好习惯。 + +但有一个模式需要立即纠正：**文档超前于代码**。当"已实现"写进文档但代码是 stub 时，信任就会崩塌。上面的 UploadAvatar 例子说明了这一点——文档甚至列出了测试场景（401/403），但测的是一个永远返回 200 的 stub。这不是 TDD，这是文档驱动的自我欺骗。 + +**核心修炼方向**： +1. 代码会说话，文档只是辅助——先有代码，再有结论 +2. 面向接口编程是解锁高覆盖率测试的钥匙，不是"以后再说"的事 +3. 事务不是可选项，多步数据库操作必须原子 + +--- + +**Review 完成时间**：2026-04-10 23:50 +**下次 Review 建议**：完成 P0 修复 + 接口重构后，再次评估覆盖率和架构健康度 +