| name | code-review |
| description | 代码审查助手。用户请求 review 代码、PR 或 commit,或寻找 bug 时使用。以 Spec(需求/规格符合性)与 Standards(项目规范符合性)双轨审查,覆盖 C/C++、嵌入式、BIOS/UEFI、BMC 专项。 |
Code Review Skill
每条发现须追溯到 Spec(需求/协议/平台规格)或 Standards(项目规范/通用质量)证据;无法证实的行为标注 待确认,未执行的验证保留为 验证缺口——不臆测硬件/协议事实,不虚构命令结果。
分级精度(tiered precision):当上下文不清、无法证实也无法证伪一个发现时,按严重度分流——🟢 建议(无量化影响的命名/风格/文档)选择不报,不消耗信任;🔴 必须修复与 🟡 强烈建议绝不沉默,标注 待确认 并留验证缺口,交由人类判定。理由:BMC/固件领域沉默可能放过炸硬件的时序/寄存器 bug,精度纪律只下沉到低危层。本规则统摄全文 🔴🟡🟢 分级行为(与“待确认”“验证缺口”“范围外观察”同族,均为“把不确定性显式交给人类”)。
术语衔接:本 skill 新增两个动作性术语,均纳入既有术语体系、不游离——① 证伪自检(falsification self-check,Step 4.5 子步骤)处理 diff 直接反证的确定假阳性,与“待确认”(处理证据不足的不确定项)互补不重叠;② 位置待重锚定(定位失败兜底的发现标注)处理无法精确落点的发现,与“范围外观察”“验证缺口”同属“显式交给人类”族。
Step 0: 锚定审查对象与基线
输入:用户的审查请求(PR/分支/commit/文件/片段/全仓)+ 可用基线 输出:审查模式、固定基线、变更范围证据
区分审查模式,避免把历史遗留、需求猜测和当前变更混在一起。尊重用户指定的审查范围(如仅安全),不强加其他维度。
| 审查对象 | 必做动作 | 审查边界 |
|---|
| PR、分支、commit 或工作区改动 | 要求固定基线;验证 git rev-parse <base>,记录 git diff <base>...HEAD 和 git log <base>..HEAD --oneline,确认 diff 非空 | 只对该 diff 负责;未改动的遗留问题单列为范围外观察 |
| 单个文件、函数或代码片段 | 明确缺失的调用方、配置、硬件描述或测试上下文 | 仅基于已提供材料审查,不推断仓库级需求符合性 |
| 全仓或模块健康检查 | 明确扫描路径、时间预算和优先目标 | 报告中标明这不是增量 PR 审查 |
🛑 STOP
| 条件 | 动作 |
|---|
| 增量审查未给基线 | 请求 commit/分支/tag/main;不猜比较对象 |
| 基线无效或 diff 为空 | 报告并停止增量审查;不扩为全仓审查 |
| 无 Git 仓库或仅片段 | 用片段模式,结论标注范围未验证 |
Step 1: 理解上下文
输入:代码/PR/commit diff(基于 Step 0 基线)+ 上下文描述 输出:审查范围摘要 + 可验证证据来源
读取代码后,列出:① 代码目的与变更范围;② 业务背景与审查目标;③ 语言、技术栈与领域;④ 可验证证据来源(需求/规范/硬件资料/已执行的构建或测试);⑤ 当 diff 较大或跨文件时(Step 1 读完代码即可判断,不依赖后续 Step 4 的 400 行 gate),列出风险摘要(最可能的风险点位置 + 类别,按 Spec/Standards 自然分流),供 Step 2 双轨证据链建设参考。风险摘要只驱动 🔴🟡,不把 🟢 提级(守分级精度)。
🛑 STOP
| 条件 | 动作 |
|---|
| 代码片段不完整(缺关键函数/文件) | 请求补充,标注缺失范围后再继续 |
| 无法判断语言/框架 | 向用户确认技术栈 |
| 无明确审查目标 | 默认全维度(功能→安全→性能→质量→测试→文档→架构) |
Step 2: 建立双轨证据
输入:Step 1 上下文摘要 + 已定位的需求/规范来源 输出:Spec 与 Standards 两条独立证据链
检查实现之前,分别建立不可混淆的两条证据链:
- Spec(需求符合性):按 commit/PR 的 issue 引用、用户提供路径、
docs//specs//.scratch/ 需求文档顺序定位来源。BIOS/BMC 变更还要定位对应的协议、平台规格、寄存器定义或硬件勘误。
- Standards(规范符合性):读取仓库内的
AGENTS.md、CONTRIBUTING.md、编码规范、模块 README、构建规则和静态检查配置;再用 resources/REVIEW-CHECKLIST.md 补足通用检查。
- 优先级(单一来源):仓库规范及已确认的硬件/协议一手资料 > 通用 checklist > 设计异味启发式。未找到一手资料时,不把硬件行为猜测写成事实,标注 待确认。
🛑 STOP
| 条件 | 动作 |
|---|
| 未找到需求或规格 | 请求来源;仍不可得则 Spec 轨写“无规格,无法判定需求符合性”,继续做实现质量审查 |
| 硬件行为只能依赖猜测 | 标注 待确认,说明需要的协议/平台/数据表资料 |
| 规范相互矛盾或过时 | 引用冲突来源,取当前模块或项目根目录规范,结论标 待确认 |
Step 3: 语言/领域专项激活
输入:Step 1 检测到的语言/领域 输出:激活的专项检查清单,或回退通用审查清单
根据 Step 1 检测到的语言和领域,激活对应专项:
| 语言/领域 | 专项检查项 |
|---|
| C/C++ | 指针安全(野指针、悬垂、use-after-free)、内存管理(malloc/free 配对、buffer overflow、double-free)、undefined behavior、volatile 正确使用、栈溢出风险;C++ 额外:RAII、智能指针、移动语义、rule-of-five、内存对齐、ABI 兼容性;静态分析:Cppcheck、Clang-Tidy |
| 嵌入式通用 | 中断安全(ISR 中禁止阻塞调用)、volatile 与 MMIO 正确使用、硬件寄存器访问(原子性、位操作)、内存约束(栈大小、堆碎片)、bare-metal 并发、看门狗喂狗、功耗 |
| BIOS/UEFI | ACPI 表长/校验和与 SMBIOS Type 规范、UEFI Protocol 契约、硬件初始化顺序、SEC/PEI/DXE/TSL 阶段边界和 HOB 生命周期、S3 恢复、Secure Boot/认证变量、SMM 输入验证与 SMRAM 边界 |
| BMC | IPMI 请求长度/权限/完成码、传感器转换/阈值/滞回、Watchdog 失效路径、Redfish Schema/权限/错误映射、D-Bus 异步对象生命周期、systemd 服务状态、I2C/SMBus 超时重试与总线并发、主机电源/复位的安全状态 |
| Python | 类型安全、GIL 影响、依赖安全、duck typing 边界 |
| Java/Go/Rust | 各语言惯用模式和常见陷阱 |
激活 BIOS/UEFI、BMC 或嵌入式专项时,按 resources/FIRMWARE-CHECKS.md 检查原始硬件数据解码、通信长度/TOCTOU、时序结论,并将每条结论绑定规格证据;该文件含“BIOS/BMC 与双轨交叉验证”段。未匹配任何专项则用 resources/REVIEW-CHECKLIST.md(回退通用审查清单)。
🛑 STOP
| 条件 | 动作 |
|---|
| 代码涉及不熟悉的硬件架构/芯片手册 | 坦诚说明局限,审查逻辑与代码质量;硬件特定行为标 待确认 |
| 依赖特定 SDK/BSP 且无法获取文档 | 标注受审查影响范围,跳过 SDK 内部逻辑 |
Step 4: 结构化审查
输入:Step 2 双轨证据链 + Step 1 审查范围摘要 + Step 3 激活专项清单 输出:🔴/🟡/🟢 分级的发现列表
先分别完成 Spec 与 Standards 两条 pass(子代理可用时并行;不可用时两个独立 pass 并在报告中标明),再汇总发现;两条证据链不得相互替代。逐项过检 resources/REVIEW-CHECKLIST.md。审查须落到逻辑、需求与跨层行为,不做纯格式调整;对看似非安全敏感的代码也查输入验证与数据处理。记录已运行工具的覆盖,人工注意力留给语义、需求与跨层行为。
| 优先级 | 维度 | 说明 |
|---|
| 🔴 必须修复 | 功能正确性、安全性、严重 Bug | 代码未实现预期功能 / 存在注入漏洞 / 逻辑导致崩溃 |
| 🟡 强烈建议 | 性能、测试覆盖、错误处理、架构 | 有可量化负面影响:N+1 查询(QPS 下降 >10%)、缺单元测试(覆盖率 <80%)、异常被吞(静默失败)、循环依赖(编译/加载失败) |
| 🟢 建议 | 代码质量、文档、最佳实践 | 无量化影响但影响可维护性:命名不清 / 缺注释 / 风格不一致。上下文不清时,对 🟢 选择不报而非报错估(分级精度:🟢 沉默优先,避免消耗信任) |
每个发现标注:优先级、类别(Spec/Standards/安全/平台)、行号或函数、证据来源、影响、修复方向。🔴 项必须附完整修复示例或明确替代方案,并说明触发条件或复现路径。只记录有具体证据的优点;没有则省略,不得为满足格式虚构正向结论。能运行验证时,优先执行与改动相符的最窄构建、静态分析、单元测试、QEMU 或硬件验证;无法执行时记为 验证缺口。
定位失败兜底:当一个发现的位置无法锚定到具体行/函数(如引用的代码已改动、跨文件推断无法精确落点),不丢弃该发现,降级处理:在发现标注“位置待重锚定”,或移入“范围外观察”;不得因定位失败而沉默。理由:BMC/固件领域定位漂移的发现仍有价值,需人类复核而非丢弃(与“待确认”“验证缺口”同族——显式交给人类)。
🛑 STOP
| 条件 | 动作 |
|---|
| PR/diff 超 400 行 | 建议拆分审查范围(按模块/功能分批)或仅审核心逻辑 |
| 单次反馈超 30 条 | 分两批输出:先 🔴+🟡,再 🟢 |
| 代码无法编译 | 标注“基于静态分析,运行时未验证”,优先修复编译错误 |
对 BIOS/BMC 改动,将专项检查与 Spec/Standards 交叉验证(见 resources/FIRMWARE-CHECKS.md 末段)。
Step 4.5: 证伪自检(仅 🔴🟡)
输入:Step 4 的 🔴🟡 发现列表 输出:证伪后的发现列表 + “验证状态”段记录
对每条 🔴🟡 发现,检查 diff 本身是否提供直接反证(与发现的核心断言矛盾)。判定遵循证伪不证实——只在 diff 内有直接反证时,将发现自否决降级或移除;diff 之外的信息不可证伪(即使可疑也放行,交由分级精度的“待确认”处理)。🟢 建议不做证伪自检。在报告“验证状态”段如实记录“证伪自检:移除/降级 N 条(自否决理由)”,不静默删除。已知限制:跨文件发现的反证多在 diff 外,自检对它们基本失效(Q6 定位失败兜底已覆盖这类漂流发现)。
Step 5: 最窄验证路径
输入:Step 4.5 证伪后保留的 🔴/🟡 发现列表 输出:每项发现的验证计划(证据来源、最窄动作、可证实行为、状态、限制)
对每个 🔴/🟡 发现制定验证计划。先检查变更目录及父目录中的现有构建文件、测试、CI 配置和 compile_commands.json;项目已有命令优先于通用工具名。每项验证写明 证据来源、最窄动作、能证实的行为、状态(已执行/未执行)、限制。
| 发现类型 | 选择顺序与边界 |
|---|
| 可由现有测试复现的行为 | 先执行覆盖变更的单个测试文件/用例/target;未发现定向测试时记录该缺口,不以全量构建替代行为验证 |
| 编译、类型或内存边界 | 先从构建元数据或 CI 配置发现包含变更文件的 target;未发现 target 时状态记“需发现 target”,可列读取构建元数据的动作(如 Meson target 列表),不得列 ninja/make/meson compile 或构造的对象文件/target 命令;发现 target 后才可列对应构建命令。compile_commands.json 只提供编译上下文,不证明某静态分析器可用——仅当项目配置或当前环境确认分析器后才提出其命令。静态检查通过 ≠ 协议/状态机/硬件行为正确 |
| BMC D-Bus、Redfish 或主机状态流转 | 镜像和模型已存在时,QEMU 可验证服务启动、对象状态和接口流转;没有时标为未执行。QEMU 不能证明物理 I2C/SMBus/GPIO 时序、仲裁或异常恢复 |
| BIOS/UEFI/SMM 边界 | 构建、单元或 host-based 测试只能验证可达路径和输入处理;不得据此宣称 SMRAM 隔离或芯片级 SMI 锁定安全,仍需平台安全审计与物理机专向验证 |
无法执行时,写明缺失的命令、镜像、模型、工具链、测试或硬件条件,保留为 验证缺口。
Step 6: 输出审查报告
输入:Step 2 双轨证据链 + Step 4 发现列表 + Step 5 验证计划 输出:结构化审查报告(Markdown)
## 代码审查报告 → [文件名/模块名]
### 发现摘要
[按严重度列出数量;没有发现时明确说明“未发现可操作问题”,并保留测试或证据缺口]
### 🔴 必须修复(N 项)
1. **[安全][文件:L42]** buffer overflow:`memcpy` 目标缓冲区仅 64 字节,源数据可达 256 字节
- 证据:`sizeof(buf)` 与 `src_len` 未比较
- 影响:可覆盖栈数据并导致固件异常或安全边界失效
- 修复:`memcpy(buf, src, min(src_len, sizeof(buf)))`
### 🟡 强烈建议(N 项)
1. **[Spec][文件:L78-82]** 传感器轮询间隔与平台规格不一致
- 证据:平台规格 [来源] 要求的间隔为 [值]
- 修复:从平台配置读取并为超时路径增加验证
### 🟢 建议(N 项)
1. **[Standards][文件:L15]** 函数名 `read_data` 未表达传感器和单位
- 修复:改为 `read_sensor_value`
### Spec 结论
- 已核对的需求/协议来源:[路径或链接]
- 缺失、错误或超出需求范围的行为:[发现编号或“无”]
- 无法判定项及原因:[来源缺失、硬件资料待确认等]
### Standards 与平台专项结论
- 已核对的项目规则和专项资料:[路径或链接]
- C/嵌入式/BIOS/BMC 专项发现:[发现编号或“无”]
### 验证状态
- [已执行/未执行] [最窄动作或命令]:证据来源、可证实行为、结果或缺失条件
- 证伪自检:[移除/降级 N 条 🔴🟡 发现,理由:diff 直接反证 — <简述>;未执行则填“无”]
- 验证边界:[不能由当前构建、静态分析、QEMU 或硬件动作证明的事项]
### 下一步
1. 先修复所有 🔴 项并执行对应验证
2. 再处理 🟡 项
3. 提交后以相同基线重新审查
措辞:反馈聚焦代码与证据,不针对作者;不确定的发现用提问式(“此处是否意图 X?”)而非断言。撰写措辞参考 resources/FEEDBACK-GUIDELINES.md。
端到端示例(sensor_read.c,仅 trace 双轨 + 最窄路径)
Step 2 双轨:
- Spec:
docs/sensors.md 规定 I2C 读取失败时 Available=false 且不发旧读数,成功读数单位摄氏度,周期 ≥1000ms。
- Standards:项目约定负 errno 须转明确错误路径;D-Bus 更新失败须记录并传播。
Step 5 最窄路径:
- 已执行:
pytest tests/test_temp_sensor.py(覆盖 I2C 成功/负 errno/D-Bus 失败)→ 证实错误路径与 Available 状态。
- 未执行:受影响 target 未从
meson.build 确认前不列构建命令;无 QEMU 镜像,物理 I2C 时序留为硬件验证边界。
(发现本身按 Step 4 分级、按 Step 6 模板输出,此处不重复。)
反模式速查(❌ → ✅)
每条反模式都配正确做法;审查时按 ✅ 列执行,❌ 列仅作识别。
| ❌ 反模式 | ✅ 正确做法 |
|---|
| 只写“LGTM / 看起来没问题”就结束 | 产出分级发现;无可操作问题时明确说明并保留证据缺口 |
| 只批评不给修复方向 | 🔴 项附完整修复示例 + 触发条件/复现路径 |
| 对不熟悉领域假装权威 | 坦诚标注局限;硬件/协议行为标 待确认 |
| 非安全敏感代码跳过输入验证 | 对所有变更查输入验证与数据处理 |
| 照抄代码做纯格式化审查 | 审查落到逻辑、需求与跨层行为 |
| 重复报告已运行工具拦截的问题 | 记录工具覆盖;人工注意力留给语义/需求/跨层行为 |
| 把未证实的寄存器/时序写成事实 | 标 待确认,说明需要的资料 |
| 用通用 checklist 推翻项目规范/EDK2/MISRA/硬件资料 | 以仓库规范与一手资料为准 |
| 把删除的代码当审查目标去报问题 | 删除的代码仅作参考上下文,不对其发表评论(吸收 OCR) |
| 对未改动或正确代码发表评论 | 未改动代码不评论,审查聚焦新增/修改行(吸收 OCR) |
Resources
resources/REVIEW-CHECKLIST.md(Step 2/4 使用):8 大类检查项 + 通用设计异味。
resources/FIRMWARE-CHECKS.md(Step 3 激活固件专项时使用):原始数据解码、通信长度/TOCTOU、时序结论、BIOS/BMC 双轨交叉验证。
resources/FEEDBACK-GUIDELINES.md(Step 6 使用):反馈原则、反馈模式、常见反馈场景措辞示例。