long-agent/user-system
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: complete production readiness improvements

Browse Source 
- Fix DIP violations in service layer (device, stats, auth middleware)
- Add ReplaceUserRoles interface method for transaction safety
- Implement Magic Bytes validation for avatar uploads
- Standardize OAuth error handling with ErrOAuthProviderNotSupported
- Use crypto/rand for JWT secret generation instead of weak fixed key
- Apply code formatting with gofumpt and goimports
- Fix staticcheck issues (S1024, S1008, ST1005)
- Add comprehensive quality and functional test reports
- Achieve 36.3% test coverage (up from 16.3%)
- All E2E, integration, and business logic tests passing
This commit is contained in:
long-agent
2026-04-12 16:15:32 +08:00
parent 861736cf4d
commit 09beb173cc
22 changed files with 3122 additions and 414 deletions
Download Patch File Download Diff File Expand all files Collapse all files

550
docs/code-review/CODE_REVIEW_PROCESS.md
View File

@@ -1,360 +1,354 @@
# 代码审查流程规范
# 代码审查流程规范 v2.0
**文档版本**: v1.0
**生成日期**: 2026-04-08
**适用范围**: User Management System (UMS) 项目
**文档版本**: v2.0
**更新日期**: 2026-04-12
**适用范围**: User Management System (UMS) 项目
**配套标准**: `CODE_REVIEW_STANDARD_V4.md`
**配套 Checklist**: `REVIEW_EXECUTION_CHECKLIST.md`
---
## 一、审查角色与职责
## 一、核心原则
### 1.1 角色定义
### 1.1 零信任文档原则
| 角色 | 职责 | 要求 |
|------|------|------|
| **作者 (Author)** | 自审、修复问题、响应反馈 | 熟悉代码逻辑 |
| **审查者 (Reviewer)** | 全面审查、标注问题、给出建议 | 了解业务和安全要求 |
| **仲裁者 (Arbiter)** | 解决争议、最终决策 | 资深开发者/架构师 |
> **任何"已完成"的声明，必须附带可重现的命令和输出，否则视为未完成。**
### 1.2 职责边界
历史教训：
- v2.0 时期因依赖文档自述，评分虚高至 9.7/10
- 2026-04-11 发现前端构建实际失败，但文档标注 "PASS"
- v4.0 要求：工具证据先于文档断言
**作者职责**：
1. 提交前完成自审检查清单
2. 确保代码可编译、可测试
3. 及时响应审查反馈
4. 修复问题时主动沟通
### 1.2 教学优先原则
**审查者职责**：
1. 按时完成审查（常规 4h 内）
2. 提供具体、可操作的反馈
3. 公平、一致地执行标准
4. 记录审查结果
审查的目的是让代码更好、让开发者成长，不是门卫把关：
- 每个问题说明"为什么是问题"，而非只说"改掉"
- 赞扬好的实践，具体表扬有教学价值
**仲裁者职责**：
1. 解决审查争议
2. 判定标准模糊地带
3. 优化审查流程
### 1.3 优先级纪律
| 级别 | 处理规则 | 不遵守的后果 |
|------|----------|-------------|
| 🔴 P0 | 禁止合并，4h 内修复 | 永久 Block |
| 🟠 P1 | 禁止合并，当天修复 | 永久 Block |
| 🟡 P2 | 附计划后可合并，本周修复 | 跟踪 Issue |
| 🔵 P3 | 可合并，本 Sprint 修复 | 技术债台账 |
| 💭 P4 | 可忽略 | - |
---
## 二、审查触发条件
## 二、角色与职责
### 2.1 必须审查
| 角色 | 职责 | SLA |
|------|------|-----|
| **作者** | 自审 → 提 PR → 修复问题 → 更新文档 | 当日响应 |
| **审查者** | 执行 Checklist → 标注问题 → 给出建议 → Approve | P0:1h / P1:4h / P2:8h |
| **Tech Lead** | SLA 超时升级，争议仲裁，流程优化 | 1个工作日 |
| 条件 | 说明 |
|------|------|
| 所有 PR 到 main | 任何合入 main 的代码必须审查 |
| 安全相关变更 | 认证、授权、加密相关 |
| 基础设施变更 | 配置、部署、CI/CD |
| 数据库 schema 变更 | 迁移文件 |
### 2.1 作者自审清单（提 PR 前必须执行）
### 2.2 简化审查（可选）
| 条件 | 说明 |
|------|------|
| 文档更新 | *.md 文件 |
| 测试用例补充 | 仅新增测试 |
| 依赖更新 | 无代码变更 |
| 配置调整 | 明确无风险 |
---
## 三、审查执行流程
### 3.1 阶段一：准备工作
```
审查者接收 PR 后：
1. 阅读 PR 描述，理解变更目的
2. 查看关联的 Issue/Ticket
3. 确认影响范围
4. 准备审查清单
```powershell
# 最小自审（2分钟）
cd d:\usersystem
go build ./cmd/server # 必须通过
go vet ./... # 必须通过
go test ./... -short -count=1 # 必须通过
cd frontend\admin
npm.cmd run lint # 必须通过
npm.cmd run build # 必须通过 ← 重点，历史有谎报
```
### 3.2 阶段二：自动化检查
```bash
# 后端检查
go vet ./...
go build ./cmd/server
go test ./... -count=1
gosec ./... # 安全扫描
# 前端检查
npm run lint
npm run build
npm test
npm audit
# 覆盖率检查
go test -coverprofile=coverage.out
go tool cover -func=coverage.out | tail -1
```
### 3.3 阶段三：代码审查
#### 审查顺序（建议）
1. **接口/API 层** - 先看暴露的接口是否合理
2. **业务逻辑层** - 核心逻辑实现
3. **数据访问层** - 数据库操作
4. **基础设施** - 错误处理、日志
5. **测试** - 覆盖率、有效性
#### 审查要点
**文件维度**：
- [ ] 新增文件是否必要
- [ ] 删除文件是否安全
- [ ] 修改文件是否最小化
**安全维度**：
- [ ] 输入验证
- [ ] 权限检查
- [ ] 敏感数据处理
- [ ] 加密实现
**正确性维度**：
- [ ] 逻辑正确
- [ ] 边界处理
- [ ] 错误处理
- [ ] 并发安全
**性能维度**：
- [ ] 数据库查询
- [ ] 缓存使用
- [ ] 资源释放
### 3.4 阶段四：反馈与修复
#### 评论格式
### 2.2 作者 PR 描述模板
```markdown
🔴 **[级别] 问题标题**
位置: `file.go:42`
## 变更目的
[1-2句说明：解决什么问题，为什么这样解决]
## 影响范围
- [ ] 后端（Go）
- [ ] 前端（React/TypeScript）
- [ ] 数据库（schema变更）
- [ ] 部署配置
- [ ] 文档
## 验证命令与结果
```bash
$ go build ./cmd/server
# 输出: [粘贴实际输出]
$ go test ./... -short
# 输出: ok ... [粘贴实际输出]
$ npm.cmd run build
# 输出: [粘贴实际输出]
```
## 是否需要 E2E 测试？
- [ ] 是 → 已执行 `npm.cmd run e2e:full:win`（粘贴结果）
- [ ] 否 → 理由：[说明为何不需要]
## 剩余已知问题（P2及以下）
- [问题1] #issue-link
```
---
## 三、审查执行流程（SOP）
```
┌─────────────────────────────────────────────────────────────────┐
│ 作者自审 + 提 PR │
│ □ go build / go vet / go test -short 全通过 │
│ □ npm lint / npm build 全通过（无 TS 错误！） │
│ □ PR 描述包含验证命令输出 │
└─────────────────────────┬───────────────────────────────────────┘
▼
┌─────────────────────────────────────────────────────────────────┐
│ 阶段 1：自动化门禁（CI，约5分钟） │
│ □ go build + go vet + go test -race │
│ □ 覆盖率 ≥ 60% │
│ □ govulncheck（无已知CVE） │
│ □ npm lint + npm build + npm test │
│ □ npm audit（high漏洞=0） │
│ │
│ ⚠️ 任一失败 → PR 自动 Block，作者修复后重新触发 │
└─────────────────────────┬───────────────────────────────────────┘
▼（CI 全通过）
┌─────────────────────────────────────────────────────────────────┐
│ 阶段 2：审查者人工审查（10-20分钟） │
│ │
│ 按优先级审查顺序： │
│ 1. 安全维度（P0 优先）—— 新 API 权限？文件上传？SQL 注入？ │
│ 2. API 契约 —— 响应格式统一？HTTP状态码正确？ │
│ 3. 前后端集成 —— 路径/字段名/类型一致？ │
│ 4. 业务逻辑 —— 功能正确？边界处理？ │
│ 5. 测试质量 —— 测试是真实的？非虚假断言？ │
│ 6. 运维影响 —— 配置变更？Runbook 需要更新？ │
│ │
│ 使用 REVIEW_EXECUTION_CHECKLIST.md 逐项执行 │
└─────────────────────────┬───────────────────────────────────────┘
▼
┌─────────────────────────────────────────────────────────────────┐
│ 阶段 3：问题标注 │
│ 使用标准格式（见第四节） │
│ P0/P1 → 逐项说明问题+原因+建议修复 │
│ P2/P3 → 可集中列表 │
│ 亮点 → 至少指出 1 个好的做法 │
└─────────────────────────┬───────────────────────────────────────┘
▼
┌─────────────────────────────────────────────────────────────────┐
│ 阶段 4：作者修复 │
│ P0/P1 → 修复后回复每条评论，附命令输出证明 │
│ P2 → 修复或创建 Issue 跟踪，评论 Issue 链接 │
│ P3 → 修复或在 PR 评论说明原因 │
└─────────────────────────┬───────────────────────────────────────┘
▼
┌─────────────────────────────────────────────────────────────────┐
│ 阶段 5：E2E 检查（条件触发） │
│ 触发条件（满足任一）： │
│ - 认证相关变更 │
│ - 路由守卫变更 │
│ - 导航组件变更 │
│ - Token 管理变更 │
│ 命令：cd frontend/admin && npm.cmd run e2e:full:win │
└─────────────────────────┬───────────────────────────────────────┘
▼
┌─────────────────────────────────────────────────────────────────┐
│ 阶段 6：Approve + 合并 │
│ □ 所有 🔴🟠 问题已修复（有验证命令证明） │
│ □ P2 有 Issue 跟踪计划 │
│ □ 覆盖率未下降 > 5% │
│ □ 文档已同步（API 变更 → Swagger，配置变更 → .env.example） │
│ □ Approve │
└─────────────────────────────────────────────────────────────────┘
```
---
## 四、审查评论格式规范
### 问题标注格式
```markdown
🔴 **[P0 - 安全] 文件上传缺少 Magic Bytes 校验**
📍 位置：`internal/api/handler/avatar_handler.go:95`
**问题描述**：
[清晰描述问题]
当前仅校验文件扩展名，攻击者可将 PHP Shell 命名为 `.jpg` 绕过检查。
**为什么这是个问题**：
[解释风险或影响]
**风险**：
恶意文件可能被服务端执行，导致 RCE（远程代码执行）。
**建议修复**：
```code
// 建议的代码
```go
src, _ := file.Open()
buf := make([]byte, 512)
n, _ := src.Read(buf)
contentType := http.DetectContentType(buf[:n])
allowedMIME := map[string]bool{
"image/jpeg": true, "image/png": true,
}
if !allowedMIME[contentType] {
c.JSON(400, gin.H{"message": "invalid file content"})
return
}
src.Seek(0, io.SeekStart)
```
---
🟠 **[级别] 问题标题**
...
---
🟡 **[级别] 问题标题**
...
🟡 **[P2 - 可维护性] context.Background() 在请求链路中截断追踪**
📍 位置：`internal/api/middleware/auth.go:131`
**问题描述**：缓存查询使用 `context.Background()` 而非请求 context，导致 Trace ID 无法传播。
**建议**：将函数签名改为接收 `ctx context.Context`，传递调用者的 context。
---
💭 **[挑剔] 可选优化**
...
---
✅ **做得好的地方**
[具体表扬]
✅ **做得好：Argon2id 密码哈希配置优秀**
`internal/auth/password.go` 中 64MB 内存、5次迭代的 Argon2id 配置超越行业基准，
有效防御 GPU 暴力破解。
```
#### 修复确认
| 问题级别 | 修复要求 | 确认方式 |
|----------|----------|----------|
| 🔴 | 必须修复 | 重新审查 |
| 🟠 | 必须修复 | 截图确认或重新审查 |
| 🟡 | 建议修复 | 修复后标注或提供理由 |
| 💭 | 可选 | 可忽略，提供理由即可 |
### 3.5 阶段五：完成审查
#### Approve 条件
```
□ 所有 🔴🟠 问题已修复
□ 🟡 问题 ≤ 3 个或有明确修复计划
□ 覆盖率不下降 > 5%
□ 审查者确认理解变更
```
#### 评论模板
### Approve 评论格式
```markdown
## 审查结论
✅ **可以合并**
✅ **批准合并**
**评分**: X.X/10
**综合评分**：X.X/10
**亮点**:
- [1]
- [2]
**亮点**：
- Argon2id 配置超越行业基准
- 游标分页 P99=53ms，性能优秀
**遗留问题**:
- [1] (P1, @负责人)
- [2] (P2, @负责人)
**遗留 P2（已有 Issue 跟踪）**：
- #123 OpenAPI 注释完善
- #124 pagination 包测试
**后续关注**:
- [建议后续优化项]
**合并后 24h 内请确认**：
- 生产监控无异常告警
- 关键业务指标（登录成功率）正常
LGTM 🚀
```
---
## 四、审查时效管理
## 五、审查时效 SLA
### 4.1 SLA 要求
| PR 优先级 | 首次审查 | 修复后复核 | 最大总周期 |
|-----------|----------|------------|-----------|
| P0 安全紧急 | **30 分钟** | 15 分钟 | 2 小时 |
| P1 重要修复 | **1 小时** | 30 分钟 | 4 小时 |
| P2 常规功能 | **4 小时** | 2 小时 | 24 小时 |
| P3 重构/文档 | **8 小时** | 4 小时 | 48 小时 |
| PR 优先级 | 首次审查 | 修复后复核 | 最大周期 |
|-----------|----------|------------|----------|
| P0 (安全/紧急) | 1 小时 | 30 分钟 | 4 小时 |
| P1 (重要) | 4 小时 | 1 小时 | 24 小时 |
| P2 (常规) | 8 小时 | 2 小时 | 48 小时 |
| P3 (优化) | 24 小时 | 4 小时 | 72 小时 |
### 4.2 超时处理
### 超时处理
```
1. 超过 SLA 50% → 提醒（@审查者）
2. 超过 SLA 100% → 升级（@Tech Lead）
3. 超过 3 天无响应 → 仲裁者介入
超 SLA 50% → 作者 @审查者 催促
超 SLA 100% → 作者 @Tech Lead 升级
超 3 个工作日无响应 → Tech Lead 仲裁
```
---
## 五、争议解决
## 六、特殊场景处理
### 5.1 常见争议场景
| 场景 | 解决方式 |
|------|----------|
| 问题级别判定分歧 | 参照分级标准，模糊取高 |
| 是否必须修复 | 审查者决定，仲裁者终裁 |
| 代码风格偏好 | 参考规范，无标准则接受 |
| 性能优化必要性 | 量化数据支持 |
### 5.2 仲裁流程
### 6.1 大型 PR（>500 行）
```
1. 作者提出仲裁请求
2. 审查者陈述理由
3. 仲裁者审查双方观点
4. 仲裁者做出最终决定
5. 记录仲裁结果（供后续参考）
优先请求作者拆分，按以下维度拆：
- 后端/前端 分开
- 功能/测试 分开
- 重构/新功能 分开
如必须整体审查：
1. 分批审查（核心安全逻辑优先）
2. 明确标记哪些部分已审查
3. 剩余部分安排跟进审查
```
### 6.2 生产紧急修复（Hotfix）
```
流程：
1. Tech Lead 批准先合并（P0 安全问题）
2. 24 小时内完成完整审查
3. 发现问题立即 Hotfix v2
4. 72 小时内完成事后复盘
条件：
- 只允许 P0 安全/稳定性问题
- 必须在 Hotfix 分支（hotfix/XXX）
- 合并后必须同步更新所有文档
```
### 6.3 安全相关变更（额外严格）
```
触发条件（满足任一）：
- 认证/授权逻辑
- 密码/Token 处理
- 文件上传
- 外部服务调用（OAuth/SMS/Email）
- 数据库 schema（含敏感字段）
额外要求：
- Tech Lead 必须参与审查
- 发布前必须运行完整安全扫描（gosec + govulncheck）
- 需要额外的攻击场景测试
```
---
## 六、审查质量保证
## 七、争议解决
### 6.1 审查者自我检查
```
审查前：
□ 我理解这次变更的目的吗？
□ 我知道如何验证这些变更吗？
审查中：
□ 我是否检查了所有相关文件？
□ 我的反馈是否具体且可操作？
□ 我的反馈是否公平、一致？
审查后：
□ 我的评分是否合理？
□ 我的反馈是否有教育价值？
```
### 6.2 审查质量指标
| 指标 | 定义 | 目标 |
|------|------|------|
| 审查一致性 | 同类问题的判定一致率 | > 90% |
| 反馈质量 | 作者满意度评分 | > 4.0/5 |
| 审查效率 | 平均审查时间 | < 4h |
| 缺陷逃逸率 | 合并后发现的问题数 | < 2/版本 |
---
## 七、特殊场景处理
### 7.1 大型 PR
```
当 PR > 500 行变更时：
1. 请求作者拆分为多个 PR
2. 或分批审查（核心逻辑优先）
3. 明确标记哪些部分已审查
4. 剩余部分安排后续审查
```
### 7.2 紧急修复
```
当生产环境需要紧急修复时：
1. 允许先合并后审查（需要 Tech Lead 批准）
2. 24 小时内完成审查
3. 发现问题立即发版修复
4. 事后复盘，总结经验
```
### 7.3 外部贡献
```
当接收外部 PR 时：
1. 所有审查标准相同
2. 增加许可证检查
3. 增加贡献协议确认
4. 必要时要求补充签名
```
| 争议类型 | 解决方式 |
|----------|----------|
| 问题级别分歧 | 参照标准，模糊取高；Tech Lead 终裁 |
| 是否必须修复 | 审查者决定，作者提仲裁请求 |
| 技术方案选择 | 量化数据支持（性能/复杂度），Tech Lead 仲裁 |
| 代码风格偏好 | 参考项目规范，无标准则接受 |
---
## 八、审查记录归档
### 8.1 归档内容
| 内容 | 位置 | 保存期限 |
|------|------|----------|
| PR 审查评论 | GitHub PR | 永久 |
| 审查报告 | `docs/code-review/` | 永久 |
| 争议解决记录 | `docs/team/disputes.md` | 永久 |
| 审查指标汇总 | `docs/team/metrics/` | 1 年 |
### 8.2 报告生成
每次全面审查后生成报告：
```
docs/code-review/CODE_REVIEW_REPORT_YYYY-MM-DD.md
```
报告模板见 `CODE_REVIEW_STANDARD_V2.md` 第 7 节。
| 全面审查报告 | `docs/code-review/COMPREHENSIVE_REVIEW_YYYY-MM-DD.md` | 永久 |
| 专项审查报告 | `docs/code-review/[主题]_REVIEW_YYYY-MM-DD.md` | 永久 |
| 问题跟踪 | Gitea Issues | 永久 |
| 工具扫描结果 | `docs/evidence/` | 90天 |
---
## 九、持续改进
### 9.1 流程回顾
### 9.1 回顾周期
| 周期 | 内容 | 负责人 |
|------|------|--------|
| 每月 | 审查效率分析 | Tech Lead |
| 每季度 | 流程优化讨论 | Team |
| 每半年 | 规范更新 | 代码审查专家 |
| 每次 Sprint 结束 | 审查效率/质量小结 | Tech Lead |
| 每月 | 流程优化讨论（缺陷逃逸率、审查一致性）| Team |
| 每季度 | 标准文档更新（CODE_REVIEW_STANDARD_VX.md）| 代码审查专家 |
### 9.2 改进建议
### 9.2 关键质量指标（目标）
团队成员可以通过以下方式提出改进建议：
1. 在 `docs/team/improvements/` 创建提案
2. 在 Team Meeting 中讨论
3. PR 到本文档
| 指标 | 当前 | 目标 |
|------|------|------|
| 缺陷逃逸率 | 未量化 | < 2个/Sprint |
| P0/P1 修复时效 | 未量化 | 100% 在 SLA 内 |
| 审查覆盖率 | 100% | 保持 |
| 虚假完成率 | 历史有案例 | 0 |
---
*本文档由代码审查专家 Agent 制定，版本: v1.0*
*最后更新: 2026-04-08*
*文档版本: v2.0*
*更新时间: 2026-04-12*
*主要变更: 新增零信任文档原则 + SOP 流程图 + E2E 触发条件 + 各维度专项检查要求*