199 lines
19 KiB
Markdown
199 lines
19 KiB
Markdown
|
# 代码评审检查清单
|
|||
|
|
|
|||
|
|
> 用于评审代码的正确性、设计一致性、安全性和可维护性。
|
|||
|
|
> 依据 IEEE 1028-2008 和行业最佳实践。
|
|||
|
|
|
|||
|
|
> **维度命名空间（dimension namespace）**: 本清单中的维度代码（如 TST/BEH/GATE/REG）是**清单局部（checklist-local）**标识符，仅在本文档内唯一；其他清单可能对相同字母串绑定不同概念（如 code-review.md 的 TST=Test Quality、verification.md 的 TST=Test Suite Completeness、port.md 的 TST=Test Porting）。跨清单引用时必须带清单限定（如 code-review.md TST 5.8），不得使用裸代码。
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 使用说明
|
|||
|
|
|
|||
|
|
1. 阅读待评审的全部代码文件；
|
|||
|
|
2. 同时阅读设计文档中的相关章节作为参考基线；
|
|||
|
|
3. 逐项检查，判定为"通过"或"不通过"；
|
|||
|
|
4. 对"不通过"项注明具体文件名、行号和问题描述；
|
|||
|
|
5. 任一检查维度中若存在"不通过"项，须修订后重新评审；
|
|||
|
|
6. 所有维度均通过后方可合并或发布。
|
|||
|
|
7. 第 2 轮及以后的评审中，任何仅属外观性的 MINOR 发现（命名、注释、死代码、导入顺序、风格等不影响正确性或可读性的问题）必须满足其一：已修复并关闭，或在 synthesis 中以 `WAIVED-{id}` 条目显式豁免并注明原因。`WAIVED-{id}` 约定的规范定义见 `.octopus/skills/_shared/review-revision-prompt.md`（本清单的豁免标记形如 `WAIVED-COR-R2-001`）。
|
|||
|
|
8. **CR-COMMIT 门禁**：代码评审收敛（所有维度 PASS）后，必须将所有评审修订提交到工作分支，再进入 verify 阶段。未提交的评审修订不得通过 verify 放行。验证阶段（verify）启动前必须确认 `git status` 无未提交修改。
|
|||
|
|
9. **standalone-bugfix 模式的预先存在模式豁免**：在 standalone-bugfix 模式下，修复的范围应以最小化外科手术为原则。若 COR/STY/DOC 维度的发现指向的是**修复前已存在的模式**（并非本次变更引入），且该模式与文件中已有代码保持一致，则 `WAIVED` 或 `ACCEPTED_RISK` 是有效的处理方式。具体适用场景：
|
|||
|
|
- COR 1.9/1.10/1.13：错误处理策略在变更前已采用同等模式（如 `console.warn` 而非面向用户的错误提示、无超时策略等），本次变更未使其劣化
|
|||
|
|
- STY 6.8：文件/模块长度超过 200 行属于变更前已有的结构，拆分为独立重构任务（非本次 bugfix 范畴）
|
|||
|
|
- DOC 9.5：注释中的默认值初始化（如 `lastTriggerScrollTop = -1`）为防御性编程，在观察者建立前即被覆盖，无行为影响
|
|||
|
|
每次豁免必须在 synthesis 中以带编号的 `WAIVED-{id}` 或 `ACCEPTED_RISK` 条目记录原因。此豁免不影响其他维度的评审标准。
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 1. 正确性、错误处理与兼容性检查（COR — Correctness, Error Handling & Compatibility）
|
|||
|
|
|
|||
|
|
| # | 检查项 | 通过 | 不通过 | 备注 |
|
|||
|
|
| ---- | ------------------------------------------------------------ | ---- | ------ | ---- |
|
|||
|
|
| 1.1 | 逻辑分支完整，无遗漏的 if/else、switch case 或漏处理的枚举值 | ☐ | ☐ | |
|
|||
|
|
| 1.2 | 边界条件正确处理（空值、空集合、零值、负值、最大值/最小值） | ☐ | ☐ | |
|
|||
|
|
| 1.3 | null/undefined 在使用前已检查，无空引用风险 | ☐ | ☐ | |
|
|||
|
|
| 1.4 | 异步操作正确使用 await 或 Promise 链，无竞态条件 | ☐ | ☐ | |
|
|||
|
|
| 1.5 | 循环有正确的终止条件，无无限循环风险 | ☐ | ☐ | |
|
|||
|
|
| 1.6 | 类型转换安全（如字符串转数字、JSON 解析），有失败处理 | ☐ | ☐ | |
|
|||
|
|
| 1.7 | 无逻辑死区（unreachable code）或死代码（dead code） | ☐ | ☐ | |
|
|||
|
|
| 1.8 | 所有可能失败的操作（I/O、网络、解析、数据库）有错误处理 | ☐ | ☐ | |
|
|||
|
|
| 1.9 | 错误信息对用户友好（不暴露内部堆栈、路径或 SQL） | ☐ | ☐ | |
|
|||
|
|
| 1.10 | 外部服务调用有超时、重试和熔断策略（与设计文档一致） | ☐ | ☐ | |
|
|||
|
|
| 1.11 | 事务边界明确，异常时回滚，无部分提交 | ☐ | ☐ | |
|
|||
|
|
| 1.12 | 错误码和 HTTP 状态码语义正确（不把 500 当 400 用） | ☐ | ☐ | |
|
|||
|
|
| 1.13 | 异常被捕获且传播到合适的层级，无被吞掉的异常 | ☐ | ☐ | |
|
|||
|
|
| 1.14 | 异步错误（Promise rejection、EventEmitter error）有处理器 | ☐ | ☐ | |
|
|||
|
|
| 1.15 | 公共 API 的签名、参数和返回值未做不兼容变更（或已标注 breaking） | ☐ | ☐ | |
|
|||
|
|
| 1.16 | 配置项的新增/删除/改名有迁移路径或向后兼容处理 | ☐ | ☐ | |
|
|||
|
|
| 1.17 | 客户端（前端/移动端/SDK）与后端接口版本兼容 | ☐ | ☐ | |
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 2. 设计一致性与依赖检查（DGN — Design Compliance & Dependencies）
|
|||
|
|
|
|||
|
|
| # | 检查项 | 通过 | 不通过 | 备注 |
|
|||
|
|
| ---- | ------------------------------------------------------ | ---- | ------ | ---- |
|
|||
|
|
| 2.1 | 代码实现了设计文档中该组件的全部指定接口和方法签名 | ☐ | ☐ | |
|
|||
|
|
| 2.2 | 组件职责与设计文档中声明的职责一致，无越界逻辑 | ☐ | ☐ | |
|
|||
|
|
| 2.3 | 组件依赖关系与设计文档的依赖图一致，无反向依赖 | ☐ | ☐ | |
|
|||
|
|
| 2.4 | 数据模型（字段、类型、关系）与设计文档的数据设计一致 | ☐ | ☐ | |
|
|||
|
|
| 2.5 | 接口输入输出的 Schema 与设计文档的接口设计一致 | ☐ | ☐ | |
|
|||
|
|
| 2.6 | 代码未引入设计文档中未提及的新依赖或新服务 | ☐ | ☐ | |
|
|||
|
|
| 2.7 | 架构模式（工厂、策略、仓储等）的使用方式与设计决策一致 | ☐ | ☐ | |
|
|||
|
|
| 2.8 | 若代码偏离设计，有明确的 ADR 或注释说明理由 | ☐ | ☐ | |
|
|||
|
|
| 2.9 | 已用 `graph usages`/`impact` 核验改动符号的所有调用方与传递影响均已处理，无遗漏的破坏性变更（评审时用代码图核对，而非 grep 逐文件追踪） | ☐ | ☐ | |
|
|||
|
|
| 2.10 | 实现文件结构与迭代计划中的架构描述一致（文件数、模块划分、依赖方向）。若偏离（如单文件合并替代多文件架构），迭代计划已更新或偏离理由于设计文档中标明 | ☐ | ☐ | |
|
|||
|
|
| 2.11 | 新增依赖有明确的技术理由（不在审查时追问"为什么需要它"） | ☐ | ☐ | |
|
|||
|
|
| 2.12 | 依赖版本已锁定（package-lock.json / bun.lock / 等同文件已更新） | ☐ | ☐ | |
|
|||
|
|
| 2.13 | 无已知漏洞的依赖版本（依据 CVE 数据库或 `npm audit` 等同检查） | ☐ | ☐ | |
|
|||
|
|
| 2.14 | 无引入未使用的依赖 | ☐ | ☐ | |
|
|||
|
|
| 2.15 | 许可证兼容，无 GPL/AGPL 等强传染性许可证引入到非 GPL 项目 | ☐ | ☐ | |
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 3. 安全性检查（SEC — Security）
|
|||
|
|
|
|||
|
|
| # | 检查项 | 通过 | 不通过 | 备注 |
|
|||
|
|
| ---- | ----------------------------------------------------------------------------- | ---- | ------ | ---- |
|
|||
|
|
| 3.1 | 用户输入在执行 SQL/命令/HTML 前已参数化或转义 | ☐ | ☐ | |
|
|||
|
|
| 3.2 | 身份认证逻辑无绕过路径，会话令牌安全生成和验证 | ☐ | ☐ | |
|
|||
|
|
| 3.3 | 授权检查在关键操作前执行，无 IDOR（越权访问）风险 | ☐ | ☐ | |
|
|||
|
|
| 3.4 | 敏感数据（密码、令牌、密钥、PII）不在日志、错误消息或响应中泄露 | ☐ | ☐ | |
|
|||
|
|
| 3.5 | 加密算法为业界推荐标准（无 MD5/SHA1/DES 用于安全目的），密钥管理合规 | ☐ | ☐ | |
|
|||
|
|
| 3.6 | 输入校验在服务端执行（不依赖客户端校验） | ☐ | ☐ | |
|
|||
|
|
| 3.7 | 无硬编码的凭据、密钥、内网地址或环境特定值 | ☐ | ☐ | |
|
|||
|
|
| 3.7.1 | 暂存区/提交无符号链接，无机器本地绝对路径引用（`git diff --cached --diff-filter=T` 为空）| ☐ | ☐ | |
|
|||
|
|
| 3.8 | 安全相关配置项（CORS、CSP、速率限制）符合设计文档安全章节 | ☐ | ☐ | |
|
|||
|
|
| 3.9 | SAST 结论已核对：PR 存在时引用同 SHA 的 CI SAST 结果（`sast.yml`，无新增 HIGH/CRITICAL 发现）；无 PR 或同 SHA 无扫描结果时标注 `UNVERIFIABLE-LOCAL`，留待 verify Phase 2.5 处理。评审员为只读权限，不自行运行扫描工具 | ☐ | ☐ | |
|
|||
|
|
| 3.10 | 针对 CI SAST 报告的 HIGH 发现：True Positive 已修复或路由 Developer；False Positive 已标注排除理由（引用 `sast.yml` 产物，不重复推导） | ☐ | ☐ | |
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 4. 性能检查（PERF — Performance）
|
|||
|
|
|
|||
|
|
| # | 检查项 | 通过 | 不通过 | 备注 |
|
|||
|
|
| ---- | ------------------------------------------------------------- | ---- | ------ | ---- |
|
|||
|
|
| 4.1 | 循环内无同步 I/O 或数据库查询（N+1 问题） | ☐ | ☐ | |
|
|||
|
|
| 4.2 | 算法复杂度合理（无 O(n²) 或更高在主路径中，除非设计明确接受） | ☐ | ☐ | |
|
|||
|
|
| 4.3 | 资源（连接、文件句柄、缓冲区）在使用后释放，无泄漏风险 | ☐ | ☐ | |
|
|||
|
|
| 4.4 | 查询使用了索引，EXPLAIN 计划与设计文档索引设计一致 | ☐ | ☐ | |
|
|||
|
|
| 4.5 | 适度使用缓存和批处理，无过早优化但也无非受控的重复计算 | ☐ | ☐ | |
|
|||
|
|
| 4.6 | 异步操作非阻塞，长耗时操作用队列或后台任务处理 | ☐ | ☐ | |
|
|||
|
|
| 4.7 | 前端：列表子项有稳定 key，事件处理器引用稳定，无不必要的重渲染 | ☐ | ☐ | |
|
|||
|
|
| 4.8 | 前端：大列表使用虚拟滚动或分页，非首屏组件使用代码分割（lazy） | ☐ | ☐ | |
|
|||
|
|
| 4.9 | 前端：图片有懒加载（`loading="lazy"`）和尺寸占位，无布局偏移 | ☐ | ☐ | |
|
|||
|
|
| 4.10 | 前端：`useMemo`/`useCallback`/`computed`/`derived` 使用合理，无不必要的计算 | ☐ | ☐ | |
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 5. 测试质量检查（TST — Test Quality）
|
|||
|
|
|
|||
|
|
| # | 检查项 | 通过 | 不通过 | 备注 |
|
|||
|
|
| --- | ------------------------------------------------------ | ---- | ------ | ---- |
|
|||
|
|
| 5.1 | 新增代码有对应的测试（单元或集成），覆盖主要路径和分支 | ☐ | ☐ | |
|
|||
|
|
| 5.2 | 测试覆盖了边界条件（空值、极值、异常路径） | ☐ | ☐ | |
|
|||
|
|
| 5.3 | 测试断言验证了具体行为，非仅"不抛异常"或"返回非空" | ☐ | ☐ | |
|
|||
|
|
| 5.4 | 测试相互独立，可任意顺序运行，无共享可变状态 | ☐ | ☐ | |
|
|||
|
|
| 5.5 | Mock/Stub 的使用合理，模拟的外部行为与真实行为一致 | ☐ | ☐ | |
|
|||
|
|
| 5.6 | 测试命名清晰表达了测试场景和预期结果 | ☐ | ☐ | |
|
|||
|
|
| 5.7 | 本次变更涉及的测试已通过（`test:changed`，由 mechanical-green gate 机器判定，评审员引用 gate 结果即可）。全量套件（`test:parallel`）归 verify，不是代码评审门禁（[org-internal #2598] 检查分层） | ☐ | ☐ | |
|
|||
|
|
| 5.8 | 评审发现的"dummy fixture / harness 默认值漂移"类问题，不能仅凭 `WAIVED` 处置：若漂移值是 harness 对**生产 CLI/配置默认值**的镜像（如 harness 旧 owner 默认 vs CLI 新 owner 默认这类 token 漂移），WAIVED 会把真实漂移放行到 post-merge（[org-internal #3169] TST-F002 → 后续 [org-internal #3172] 才修）。处置前确认该值是否被生产路径读取；被读取则必须 FIX 或显式登记 TD | ☐ | ☐ | |
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 6. 风格与约定检查（STY — Style & Convention）
|
|||
|
|
|
|||
|
|
| # | 检查项 | 通过 | 不通过 | 备注 |
|
|||
|
|
| --- | ------------------------------------------------------ | ---- | ------ | ---- |
|
|||
|
|
| 6.1 | 命名遵循项目约定（函数名、变量名、文件名、目录结构） | ☐ | ☐ | |
|
|||
|
|
| 6.2 | 缩进、空格、引号、分号、行长度与项目格式化配置一致 | ☐ | ☐ | |
|
|||
|
|
| 6.3 | 函数/方法长度合理（通常 ≤ 50 行），单一职责 | ☐ | ☐ | |
|
|||
|
|
| 6.4 | 导入语句有序分组（第三方、内部、相对），无未使用的导入 | ☐ | ☐ | |
|
|||
|
|
| 6.5 | 无注释掉的代码块（应删除或用版本控制追溯） | ☐ | ☐ | |
|
|||
|
|
| 6.6 | 类型声明充分，无不必要的 `any` 或隐式类型 | ☐ | ☐ | |
|
|||
|
|
| 6.7 | Lint 零错误已由机器判定（mechanical-green gate 与 CI 的 `bun oxlint --deny-warnings`）；评审员引用 gate/CI 结果即可，只复核 lint 覆盖不到的约定项（6.1–6.6、6.8），不重复人工推导 | ☐ | ☐ | |
|
|||
|
|
| 6.8 | 文件/模块长度 ≤ 200 行（超出需拆分）。模块过长降低可维护性和 review 效率 | ☐ | ☐ | |
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 7. 数据库与数据检查（DBT — Database & Data）
|
|||
|
|
|
|||
|
|
| # | 检查项 | 通过 | 不通过 | 备注 |
|
|||
|
|
| --- | ------------------------------------------------------- | ---- | ------ | ---- |
|
|||
|
|
| 7.1 | 数据库迁移可安全回滚（down migration 正确且与 up 对称） | ☐ | ☐ | |
|
|||
|
|
| 7.2 | 查询使用参数化，无拼接 SQL 字符串 | ☐ | ☐ | |
|
|||
|
|
| 7.3 | 索引使用与设计文档的索引设计对应，EXPLAIN 计划合理 | ☐ | ☐ | |
|
|||
|
|
| 7.4 | 事务范围最小化（不在事务内执行外部调用或长计算） | ☐ | ☐ | |
|
|||
|
|
| 7.5 | 数据库连接生命周期正确，连接池配置合理 | ☐ | ☐ | |
|
|||
|
|
| 7.6 | 无大规模数据迁移导致锁表风险，大表变更方案已说明 | ☐ | ☐ | |
|
|||
|
|
| 7.7 | 数据库 Schema 变更对已有数据向后兼容（新字段允许 NULL 或有默认值） | ☐ | ☐ | |
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 8. 可访问性与浏览器兼容性检查（A11Y — Accessibility & Browser Compatibility）
|
|||
|
|
|
|||
|
|
> 适用于所有面向用户的前端代码。确保 UI 可被各类用户（包括使用辅助技术者）正常使用。
|
|||
|
|
|
|||
|
|
| # | 检查项 | 通过 | 不通过 | 备注 |
|
|||
|
|
| ---- | ----------------------------------------------------------------------- | ---- | ------ | ---- |
|
|||
|
|
| 8.1 | 交互元素使用原生语义 HTML（`<button>`/`<a>`/`<input>` 而非 `<div>` 模拟） | ☐ | ☐ | |
|
|||
|
|
| 8.2 | 图片有有意义的 `alt` 文本；纯装饰性图片使用 `alt=""` 或 CSS background | ☐ | ☐ | |
|
|||
|
|
| 8.3 | 表单控件有关联的 `<label>` 元素（非仅 placeholder） | ☐ | ☐ | |
|
|||
|
|
| 8.4 | 所有交互元素可通过键盘访问和操作（Tab 聚焦，Enter/Space 激活，方向键导航） | ☐ | ☐ | |
|
|||
|
|
| 8.5 | 模态框/弹窗：打开时焦点移入首元素，关闭时焦点回退触发按钮，ESC 可关闭 | ☐ | ☐ | |
|
|||
|
|
| 8.6 | 动态内容更新有 `aria-live` 通知（如搜索结果显示数、表单错误提示） | ☐ | ☐ | |
|
|||
|
|
| 8.7 | 颜色对比度合规：正文 ≥ 4.5:1，大文本 ≥ 3:1，非仅靠颜色传达信息 | ☐ | ☐ | |
|
|||
|
|
| 8.8 | 页面标题层级（`h1`→`h2`→`h3`）有逻辑层次，无跳级 | ☐ | ☐ | |
|
|||
|
|
| 8.9 | `aria-label`/`aria-labelledby` 为仅图标按钮、导航区提供了屏幕阅读器标签 | ☐ | ☐ | |
|
|||
|
|
| 8.10 | 焦点指示器可见（`:focus-visible`），无 `outline: none` 但未提供替代样式 | ☐ | ☐ | |
|
|||
|
|
| 8.11 | a11y 自动化扫描（`a11y.yml` / axe-core）结果已引用：PR 命中触发路径时以同 SHA CI 结论为准（无新增违规项），评审员不重复运行；语义性条目（8.1–8.10）仍由评审判断 | ☐ | ☐ | |
|
|||
|
|
| 8.12 | 前端：目标浏览器（Chrome/Firefox/Safari/Edge 最近 2 个主版本）下功能正常 | ☐ | ☐ | |
|
|||
|
|
| 8.13 | 前端：CSS 特性（Grid/Flexbox/Custom Properties）在目标浏览器均有支持或降级方案 | ☐ | ☐ | |
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 9. 文档检查（DOC — Documentation）
|
|||
|
|
|
|||
|
|
| # | 检查项 | 通过 | 不通过 | 备注 |
|
|||
|
|
| --- | ---------------------------------------------------------- | ---- | ------ | ---- |
|
|||
|
|
| 9.1 | 复杂算法、非常规优化或非直觉的逻辑有解释性注释 | ☐ | ☐ | |
|
|||
|
|
| 9.2 | 对外 API 的接口文档已同步更新（参数、返回值、错误码） | ☐ | ☐ | |
|
|||
|
|
| 9.3 | 面向用户的错误消息清晰、可操作（不应是"系统错误，请重试"） | ☐ | ☐ | |
|
|||
|
|
| 9.4 | README / runbook / 运维文档如有必要已更新（如新增配置项） | ☐ | ☐ | |
|
|||
|
|
| 9.5 | 注释与代码一致（代码已改但注释未改视为文档错误） | ☐ | ☐ | |
|
|||
|
|
| 9.6 | 废弃的 API/配置有 `@deprecated` 标记和替代方案说明 | ☐ | ☐ | |
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 10. 可追溯性完整性检查（TRC — Traceability Completeness）
|
|||
|
|
|
|||
|
|
| # | 检查项 | 通过 | 不通过 | 备注 |
|
|||
|
|
| ---- | ------------------------------------------------------------------ | ---- | ------ | ---- |
|
|||
|
|
| 10.1 | 每次代码修改的 commit/PR 描述包含对应的工作项 ID | ☐ | ☐ | |
|
|||
|
|
| 10.2 | 每个验收标准的实现均有对应的自动化测试用例 | ☐ | ☐ | |
|
|||
|
|
| 10.3 | 无验收标准遗漏测试或手动验证路径（全部覆盖） | ☐ | ☐ | |
|
|||
|
|
| 10.4 | 代码修改范围未超出工作项定义（无"顺便修"的无关变更） | ☐ | ☐ | |
|
|||
|
|
| 10.5 | 新增的需求引用（REQ-F-{NNN} / REQ-NF-{NNN}）在代码中有对应实现 | ☐ | ☐ | |
|
|||
|
|
| 10.6 | 已删除/弃用的代码有明确的移除原因和替代方案说明 | ☐ | ☐ | |
|
|||
|
|
| 10.7 | 测试报告能按工作项 ID 筛选（测试与工作项可交叉引用） | ☐ | ☐ | |
|