code-review.md 24 KB
代码审查
输入要求
- 代码片段（必填）：需要审查的代码
- 编程语言（必填）：代码使用的语言
- 上下文说明（可选）：这段代码的功能、所在的模块
- 审查重点（可选）：如"重点关注安全性"、"检查内存泄漏"
输出示例
## 代码审查报告
**审查文件**：e/admin/ecmsmod.php
**编程语言**：PHP
**代码行数**：412 行
**问题总数**：3 个（致命 0 / 严重 1 / 警告 1 / 建议 1）
## 严重问题（建议修复）
### 问题1：ehash 校验缺失导致 CSRF 风险
**位置**：DoEditMod 函数 / 第 88-102 行
**原代码**：
```php
if($_POST['enews']=='EditMod'){ ... } // 未校验 hCheckEcmsRHash()
```
**问题描述**：后台写操作未做 rhash 校验，可被跨站请求伪造触发非预期修改。
**影响**：攻击者可构造页面诱导管理员点击，静默修改模型配置。
**修复建议**：
```php
hCheckEcmsRHash();
if($_POST['enews']=='EditMod'){ ... }
```
## 警告与建议
- 第 130 行 `fetch1()` 未做 false 守卫，建议 `?: []`。
## 审查总结
- **整体评分**：82/100
- **优先修复**：问题1（上线前必须修）
执行流程
第一步：Spec一致性检查
审查前先检查代码是否符合spec：
- 如有spec：逐条检查spec中的scenario是否被正确实现
- 发现不一致：标注spec与实现的偏差，建议修正spec或代码
无spec：提示建议在审查后使用
spec-driven-development补全spec✓ Spec一致性检查: - Scenario 1: [通过/失败] — [说明] - Scenario 2: [通过/失败] — [说明]
第二步：项目规范检查
检查代码是否符合项目记忆中的规范：
- 命名约定（snake_case / camelCase / PascalCase）
- 错误处理模式（异常 vs 返回码）
- 导入排序、文件组织等风格规范
- 发现新规范：提示记录到项目记忆
第三步：代码理解
- 识别编程语言和框架
- 理解代码的功能和逻辑
- 识别代码的上下文（函数、类、模块）
第四步：多维度审查
| 维度 | 检查内容 | 权重 |
|---|---|---|
| Bug风险 | 空指针、数组越界、资源未释放、并发问题 | 30% |
| 安全性 | SQL注入、XSS、敏感信息硬编码、权限绕过 | 25% |
| 性能 | 时间复杂度、内存泄漏、N+1查询、循环内IO | 20% |
| 可维护性 | 命名规范、函数长度、圈复杂度、重复代码 | 15% |
| 最佳实践 | 语言特性利用、设计模式、错误处理 | 10% |
| Karpathy维度 | 简洁优先、精准修改、编码前思考 | 附加 |
小片段快速审查模式：
当用户只提供单个函数、短代码片段或明确要求“快速看看”时，先按以下四象限完成轻量审查；发现高风险问题后再展开完整分级报告：
| 快速象限 | 检查重点 | 输出要求 |
|---|---|---|
| 代码规范与可读性 | 命名、缩进、函数长度、嵌套层级、注释有效性 | 只指出影响理解或维护的具体问题 |
| 潜在 Bug | 边界条件、空值处理、数组索引、资源释放、并发 | 必须说明触发条件和可能后果 |
| 性能分析 | 算法复杂度、重复计算、循环内 IO/SQL/网络请求 | 能量化则量化，不能量化则标注原因 |
| 安全性考量 | 注入、XSS、敏感信息、权限校验、反序列化风险 | 命中即提升为严重或致命候选 |
快速模式不得降低审查标准；只是压缩输出篇幅，不省略 Bug、安全和性能判断。
第五步：Karpathy维度审查
在常规审查之外，按Karpathy编码哲学进行专项审查：
简洁优先（Simplicity First）：
- 是否包含超出需求的功能？
- 是否存在单次使用的抽象（Strategy模式做简单计算）？
- 是否可以用更少的代码实现（200行能否变50行）？
- 是否包含推测性功能（缓存、验证、通知等未被要求的）？
精准修改（Surgical Changes）（针对修改的代码）：
- 是否只修改了必要的代码？
- 是否"顺手"改进了相邻代码、注释或格式？
- 是否保持了原有代码风格？
- 每一行修改是否能追溯到需求？
编码前思考（Think Before Coding）：
- 代码中是否有未陈述的假设？
- 是否有硬编码的"魔法值"没有说明原因？
- 函数/类设计是否考虑了多种使用场景？
Karpathy维度问题不纳入 severity 分级，单独标注为 「Karpathy建议」。
第六步：问题分级
| 级别 | 说明 | 必须修复 |
|---|---|---|
| 致命（Critical） | 会导致系统崩溃、数据丢失、安全漏洞 | 是 |
| 严重（Major） | 明显Bug、性能严重下降、安全隐患 | 是 |
| 警告（Warning） | 潜在风险、代码异味、不符合规范 | 建议 |
| 建议（Suggestion） | 可优化点、风格问题、最佳实践 | 可选 |
第七步：生成修复建议
对每个问题：
- 指出具体代码位置（行号/函数名）
- 说明问题原因和影响
- 提供修复后的代码示例
- 说明修复后的收益
Karpathy建议的修复优先级：
- 过度设计 → 先简化再审查
- 顺手重构 → 回滚无关修改
- 隐藏假设 → 添加参数校验或文档
第八步：记录到项目记忆
审查完成后，将发现的问题和规范沉淀到项目记忆（参见 project-memory-management.md）：
- 记录反复出现的坏味道模式（Convention Capture 模式）：提炼为团队编码规范避免复发
- 记录安全/性能问题的修复模式（Decision Record 模式）：形成可复用的检查清单
- 若审查中发现 spec 与实现偏差，反馈到 spec 同步更新
- 更新已知问题清单，标注高风险模块供后续审查重点关注
实战请求示例
示例一：小片段快速审查
帮我快速审查这段 PHP 代码。
重点：SQL 注入、XSS、PHP 8.2 兼容、是否有明显逻辑 Bug。
范围：只看我贴出的函数，不扩展到其他文件。
输出：按阻塞/严重/一般/建议分级；如果没有明显问题，请明确写“未发现阻塞问题”。
验证：说明是否需要 php -l 或单元测试。
优先加载：code-review.md；如果出现数据库访问，再协同 mysql-database.md。
示例二：功能合并前审查
帮我审查这个订单导出功能的改动。
范围：Controller、Service、导出任务、前端轮询接口。
重点：权限、CSRF、Init-Step-Poll、防重复提交、超时恢复、错误提示。
要求：先列阻塞问题，再列可优化项；不要直接重构，除非影响主线可用性。
优先加载：code-review.md、api-design.md、test-generation.md。
输出格式
## 代码审查报告
**审查文件**：[文件名]
**编程语言**：[语言]
**代码行数**：[X行]
**问题总数**：[X]个（致命[X] / 严重[X] / 警告[X] / 建议[X]）
## 致命问题（必须修复）
### 问题1：[问题标题]
**位置**：[函数名/行号范围]
**原代码**：
```[代码片段]```
**问题描述**：[具体说明]
**影响**：[可能导致什么后果]
**修复建议**：
```[修复后的代码]```
## 严重问题（建议修复）
### 问题N：[问题标题]
...
## 警告与建议
...
## 审查总结
- **整体评分**：[X]/100
- **主要风险**：[一句话总结]
- **优先修复**：[Top 3问题]
- **正向评价**：[代码中的亮点]
无明显问题输出规范：
如果完成审查后未发现有效问题，不得强行制造建议，必须明确输出：
## 代码审查报告
**审查结论**：未发现明显问题
**已覆盖维度**：可读性 / Bug风险 / 性能 / 安全性 / 项目规范
**残余风险**：[缺少运行环境、依赖上下文、测试结果或业务规则时说明；无则写"无"]
**建议验证**：[可选的测试、lint、静态分析或人工验收步骤]
质量标准
- 每个问题必须指出具体代码位置，不能只说"这段代码"
- 修复建议必须是可运行的代码，不能是伪代码或描述
- 致命问题必须说明可能导致的具体后果（如"可能导致SQL注入，攻击者可获取全表数据"）
- 不得误报（把正确代码标记为问题），不确定时标注"疑似"
- 安全审查必须覆盖OWASP Top 10相关风险
- 性能审查必须量化影响
- 涉及 MySQL/SQL 时必须检查参数绑定、索引策略、EXPLAIN 证据、事务边界、DDL 风险和回滚方案
- 证伪门禁：每个结论交付前，先构造一个反例尝试推翻它；构造不出反例才允许交付。跑通 ≠ 正确。环检测三次没写对，根因不是没清单，而是每次写完都没人攻击它；本门禁把"跑通就交付"逼成"证伪后交付"
性能反模式审查清单
当用户要求代码优化、性能优化、架构优化、慢接口排查，或审查中发现性能嫌疑点时，追加以下检查：
| 反模式 | 审查信号 | 风险判定 | 修复方向 |
|---|---|---|---|
| N+1 查询 | 循环体内出现数据库查询、ORM find/select/where、外部 API 请求 |
数据量增长时调用次数线性放大，严重时可导致超时或连接池耗尽 | 批量查询、预加载、JOIN、聚合、一次性查询后内存分组 |
| 同步阻塞 I/O | 高并发接口中出现同步文件读写、同步网络调用、长 CPU 任务 | 请求线程被阻塞，吞吐下降，尾延迟升高 | 异步化、后台任务、队列、流式处理、连接池 |
| 无界缓存/内存泄漏 | 全局 Map/dict/Object/static 集合持续写入，缺少 TTL、容量、清理 | 长时间运行后内存持续增长，可能 OOM | TTL、容量上限、LRU、生命周期清理、弱引用或托管缓存 |
| 无上限数据拉取 | 列表接口无分页、.all() / findAll() / SELECT * 拉全量 |
大数据量下内存、网络和数据库压力失控 | 强制分页、游标、最大 page size、按需字段 |
| 索引失效 | DATE(column)、LIKE '%xxx'、隐式类型转换、函数包裹索引列 |
查询退化为全表扫描或大范围扫描 | 改写查询条件、调整索引、提供 EXPLAIN 验证 |
性能问题只允许在有证据时定性为结论；静态扫描命中但缺少 benchmark、profile、EXPLAIN 或日志时，必须标注为"疑似"并给出最小验证动作。
长任务反模式审查清单
当用户要求实现或审查长任务（批量导入/导出/生成静态页/重建索引/采集同步/清理缓存/>3 秒或 >100 条处理）时，追加以下检查：
| 反模式 | 审查信号 | 风险判定 | 修复方向 |
|---|---|---|---|
| 单请求同步跑长任务 | 批量操作在单个 HTTP 请求内同步执行，无分批、无进度返回 | PHP 超时/内存耗尽、用户无进度感知、无法取消 | 改 Init → Step → Poll：Init 建任务、Step 分批、Poll 查状态 |
| Step 非幂等 | Step 重试会重复处理同一批数据 | 数据重复、状态错乱 | 用 last_id/游标/offset 保证每批唯一，重复调用结果一致 |
| 进度仅存内存 | 任务进度存 PHP 变量/session 而非 DB/缓存/任务文件 | 进程重启/多实例下进度丢失，Poll 永远 running |
进度持久化到数据库/缓存/任务文件，不能只依赖 PHP 内存 |
| 无取消入口 | 前端有取消按钮但后端无 cancel 端点或忽略取消标记 | 用户无法中止错误任务，资源空耗 | 提供 cancel 端点，Step 检测取消标记后优雅退出 |
| 无超时/僵尸保护 | 无 updated_at 阈值检测，running 任务卡死无兜底 |
僵尸任务永远 running，前端轮询死循环 |
Poll 检测 running 超阈值→标记 failed（僵尸）或允许 resume |
| 并发不安全 | 多 Step 并发/多用户操作同任务无锁 | 数据竞争、覆盖 | 任务级锁或串行 Step |
长任务问题只允许在有实现证据时定性为结论；静态扫描命中但缺少接口契约、进度持久化证据或取消实现时，必须标注为"疑似"并给出最小验证动作。
MySQL 索引审查清单
| 检查项 | 判定标准 |
|---|---|
| 查询路径 | 索引必须对应 WHERE/JOIN/ORDER BY/GROUP BY/分页路径 |
| 联合索引顺序 | 必须说明等值、区分度、范围、排序、覆盖字段顺序 |
| EXPLAIN | 高频/慢查询必须提供 key/type/rows/Extra 证据 |
| 覆盖索引 | 只允许覆盖高频小字段列表，禁止塞入大字段 |
| 冗余索引 | 检查 (a) 与 (a,b)、唯一索引与普通索引重复 |
| 写入成本 | 新增索引必须评估 INSERT/UPDATE/DELETE 成本 |
| 删除索引 | 必须有依赖 SQL、风险说明和回滚 SQL |
CMS 安全审查清单
当审查对象为 PHP+MySQL CMS 代码时，在「多维度审查」之外追加以下专项检查：
| 检查项 | 检查方法 | 判定标准 |
|---|---|---|
| SQL 注入（CMS 上下文） | 检查是否使用 CMS 官方数据访问层，是否存在 $_GET/$_POST 直拼 SQL |
必须使用参数绑定或 CMS API |
| XSS（CMS 模板） | 检查模板输出是否默认转义，PHP 直接输出是否 htmlspecialchars |
所有用户输入输出前必须转义 |
| CSRF | 检查表单提交和状态变更操作是否有 Token 验证 | POST/PUT/DELETE 必须验证 CSRF Token |
| 文件上传 | 检查是否有 MIME 校验 + 扩展名白名单 + 重命名 + 非执行目录 | 四项缺一不可 |
| 反序列化 | 检查是否有 unserialize() 对不可信数据的调用 |
禁止，改用 JSON |
| 密码存储 | 检查密码是否用 password_hash()/password_verify() |
禁止 MD5/SHA1 |
| Include 注入 | 检查是否有 include $user_input |
路径必须白名单或固定 |
| 数组键引号 | 检查 $arr[key] 无引号访问 |
必须加引号 $arr['key']，PHP 8 致命错误 |
| 类型比较 | 检查是否使用 == 而非 === |
统一 === |
详见 cms-development.md 安全红线章节。
Java 专项审查清单
当审查对象为 Java/Spring 代码时，在「多维度审查」之外追加以下专项检查：
| 检查项 | 检查方法 | 判定标准 |
|---|---|---|
| 正确性 | 检查边界条件、空值、集合访问、Optional 使用、异常路径 | 不得依赖隐式非空假设，异常路径必须可解释 |
| 安全性 | 检查 SQL 拼接、XSS、反序列化、敏感信息、鉴权注解 | MyBatis 禁止 ${} 拼接外部输入，接口必须有权限边界 |
| 性能 | 检查 N+1 查询、循环内 IO/SQL、重复对象创建、分页缺失 | 高频路径必须说明复杂度和数据量影响 |
| 健壮性 | 检查异常处理、日志上下文、事务边界、连接池/超时 | 禁止空 catch；事务边界必须在业务方法层清晰表达 |
| 并发安全 | 检查共享可变状态、线程池配置、锁粒度、CompletableFuture Executor | 共享状态必须有线程安全策略，线程池不得使用隐式默认配置 |
| Spring 分层 | 检查 Controller/Service/Repository/DTO/Entity 职责 | Controller 不写业务细节，Entity 不直接作为长期 API 响应 |
| JVM 风险 | 检查大对象、缓存无界、线程泄漏、GC 压力和资源关闭 | 性能判断必须有耗时、GC、线程、堆或连接池证据 |
详见 java-development.md 的源码分析、Spring Boot 结构、并发与 JVM 章节。
算法正确性审查清单
当用户要求审查图遍历、环检测、递归、状态机、拓扑排序、依赖解析，或发现递归/DFS 代码时，在「多维度审查」之外追加以下专项检查：
| 检查项 | 检查方法 | 判定标准 |
|---|---|---|
| 不变量（invariant） | 检查是否显式声明了「不变量命题 + 终止度量 + 守恒关系」三件套（推导方法见 karpathy-coding-guidelines.md 不变量推导三步法），且实现能断言它们 |
关键算法必须交出三件套并过自检（可判定/初始成立/归纳保持/充分性），不能只靠"流程跑通"或"写了个不变量" |
| DFS 回边判定 | 检查回边识别是否基于"在递归栈中"而非"已访问"，避免把横叉边/前向边误判为环 | 回边必须只在当前 DFS 路径中闭合才计为环 |
| 方向语义解耦 | 检查遍历方向（down/up/both）与边有向性是否被编码耦合 | 遍历方向必须独立于边的有向性，反方向遍历结果应一致 |
| 结构边界覆盖 | 检查测试是否覆盖 0/1 节点、自环、互环、三环、菱形 DAG、平行边 | 见 test-generation.md 算法边界用例清单 |
| 死代码检测 | 检查是否存在永不被调用的函数/分支、恒真/恒假条件、不可达代码 | 标记死代码，要求删除或说明保留理由 |
图与递归类缺陷的典型漏网形态是"流程跑通但结果错"，审查必须断言具体结果（环数、环成员、是否标记、是否重复），不得仅以"能运行"判定正确。
关联 reference
- code-generation（代码生成）— 审查来自
code-generation产出的代码时，重点验证生成是否偏离 Spec 约束和编码规范 - karpathy-coding-guidelines（Karpathy编码规范）— 用
karpathy-coding-guidelines深入评估代码是否符合编码哲学 - frontend-design（前端设计）— 前端页面审查时参考
frontend-design的交互状态、响应式、可访问性和浏览器验证清单 - bug-diagnosis（Bug诊断）— 审查发现的具体Bug可用
bug-diagnosis深入分析根因 - refactoring（重构建议）— 审查发现的代码异味可用
refactoring获取详细重构方案 - test-generation（测试用例生成）— 修复后可用
test-generation补充回归测试 - cms-development（CMS二次开发）— PHP+MySQL CMS 场景参考
cms-development.md的安全红线和兼容性检查 - java-development（Java专项参考）— Java/Spring 场景参考
java-development.md的分层结构、异常处理、并发和 JVM 检查 - mysql-database（MySQL数据库）— 审查 SQL 注入、慢查询、索引滥用、事务边界和迁移风险时参考
mysql-database.md - project-memory-management（项目记忆管理）— 记录反复出现的坏味道、安全/性能问题修复模式和已知问题清单
失败回退机制
| 步骤 | 失败条件 | 回退目标 | 最大重试 | 不可恢复时升级路径 |
|---|---|---|---|---|
| 第一步：Spec一致性检查 | Spec文档不可用或无关联 | 跳过一致性检查，从代码反推预期行为 | 0 | 标注"无Spec对照"，仅审查内部一致性和通用规范 |
| 第二步：项目规范检查 | 项目规范文档缺失或代码文件不可读 | 跳过规范加载，直接分析代码结构，标注"规范缺失" | 2 | 基于通用最佳实践审查，建议补充项目规范 |
| 第三步：代码理解 | 代码量过大超出分析能力 | 分批审查，逐文件输出，最后汇总 | 0（分批） | 告知用户分批结果，建议缩小审查范围 |
| 第四步：多维度审查 | 某维度缺少足够上下文 | 标注"待验证"，跳过该维度 | 1 | 输出已覆盖维度的审查结果，缺项标注为"建议补充后重新审查" |
| 第五步：Karpathy维度审查 | 简洁性判断主观争议 | 标注为"建议评估"，附正反观点 | 0 | 交由用户/团队讨论决策 |
| 第六步：问题分级 | 严重性边界模糊（中/高难判） | 默认升级一级（保守策略），标注"存疑" | 1 | 所有存疑项统一标记为"需确认" |
| 第七步：生成修复建议 | 修复方案可能引入新问题 | 标注风险提示，建议回归测试范围 | 2 | 输出修复建议+单元测试覆盖要求，由开发者自行实施 |
| 第八步：记录到项目记忆 | 项目记忆系统不可用 | 输出审查报告和规范沉淀到本地文件 | 1 | 标注"审查结果未沉淀"，提示用户手动保存 |