代码审查Skill

审查目标

$ARGUMENTS

参数解析：

• 文件路径（如src/foo.cpp）→ 单文件审查
• git range（如HEAD~3..HEAD）→ git diff <range>获取变更
• PR/MR URL → 通过平台工具获取diff
• 无参数 → 询问用户要审查什么

当前环境

• 仓库： !git remote -v 2>/dev/null | head -1 || echo "NOT_A_GIT_REPO"
• 工作目录： !pwd

参考标准

根据上方仓库信息判断仓库类型，按下表加载标准文件。

始终加载

按仓库加载

领域知识

审查hccl/hcomm仓库时，若~/repo/cann/docs/hccl/目录存在，可按需查阅其中的文档辅助理解业务语义：

使用方式：当审查中遇到不确定的API语义、通信流程、环境变量用途时，用Grep搜索相关关键词。不要主动全量加载这些文件。

按需查阅

1. 理解变更上下文

此步骤是AI审查的核心优势——充分利用对整个代码库的访问能力。

PR/MR审查：

1. 通读所有diff，理解MR整体意图（新特性？bug修复？重构？）
2. 对每个变更的函数/类，主动读取：调用者（grep函数名）、所属类头文件、基类/派生类、同模块文件
3. 识别遗漏：基于MR意图，检查是否有应改而未改的地方

单文件审查：读取完整文件+关键依赖文件+grep可疑函数的其他用法。

2. 工具验证（质量门槛）

硬性规则：未经工具验证的发现只能标注"待确认"，且仅严重级别时才报出。

以下场景必须使用工具确认，不要仅凭diff猜测：

• 指针赋值/传递：读函数签名，确认值传递vs引用（值传递形参地址返回后失效）
• 算术运算：读变量声明，确认类型和值域（uint32_t*1000可能溢出）
• 结构体成员增删：grep成员名，检查所有引用点是否同步修改
• sizeof()：确认操作数类型（容器对象≠数据大小）
• 函数返回指针：读实现，确认返回null的路径
• 跨文件遗漏：grep被修改/删除的标识符，检查其他文件中的引用

通过工具验证可将"待确认"升级为"较确定"。不要因节省工具调用而跳过验证。

3. 逐条过筛必检规则

C++必检规则

对diff中每个格式化调用、每个指针操作、每个数组访问、每个算术运算逐条检查。命中即为【严重】，不可降级：

1. 格式字符串参数匹配：%说明符的个数和类型是否与实参一一对应？（规则3.1.3）
2. 空指针解引用：指针解引用前是否有非空检查？（红线1.5）

• 排除：已在调用链上游保证非空的参数透传（需工具验证调用链）

3. 数组越界：索引是否有边界保护？（红线1.2）
4. 除零保护：除数是否可能为零？（红线1.1）
5. 整数溢出/翻转：加减乘结果是否可能溢出？（红线1.3）

• 排除：值域已证明不可能溢出的场景（需工具验证值域约束）

6. 变量未初始化：所有分支上是否先初始化再使用？（红线1.4）
7. 资源泄漏：所有路径（含异常）上是否正确释放？（红线1.6）
8. 禁用函数：是否用了memcpy/sprintf/strcpy等而非_s安全替代？（规则2.18.1）
9. 并发安全：共享数据是否有data race？（红线1.7）

Python必检规则

对diff中每个函数、每个资源操作、每个外部数据处理逐条检查。命中即为【严重】，不可降级：

1. 类型混淆：None返回值是否被当作有效对象使用？Optional类型是否先检查再访问？
2. 异常吞没：except Exception/except:是否隐藏了关键错误？异常处理是否过于宽泛？
3. 资源泄漏：文件/连接/锁是否使用with语句或try/finally确保释放？
4. 可变默认参数：函数默认参数是否使用了list/dict/set等可变对象？
5. 注入风险：是否用eval()/exec()/os.system()处理外部输入？SQL是否用参数化查询？
6. 路径遍历：外部输入的文件路径是否做了规范化和白名单校验？
7. 并发安全：多线程共享数据是否有竞争条件？GIL不保护跨语句的原子性
8. 除零保护：除数是否可能为零？
9. 索引越界：列表/元组索引是否有边界保护？字典key是否可能不存在？

4. 分层检查其余规则

C++

【严重】 — 必检规则命中 + 内存分配后未判空(2.16.1)+ 安全函数返回值未检(2.18.6)+ 外部数据入exec/dlopen(2.18.7-8)+ 路径未规范化(2.17.1)+ 内存安全：UAF/悬垂指针/泄漏(3.1.2)+ 敏感信息未清零(2.15.12)+ format受外部控制(3.5.5)

【一般】 — 命名违规(1.1.x)+ typedef→using(2.3.2)+ C头文件(2.2.1)+ C风格转换(2.7.1)+ 只读形参缺const(2.10.6)+ explicit缺失(2.15.5)+ 裸new/delete(2.10.4)+ 持有c_str()指针(2.10.1)+ 缺大括号(1.2.4-5)+ catch(...)(2.1.4.1)+ switch缺default(2.8.1)+ 虚函数缺override(2.13.3)+ lambda按引用捕获局部变量(2.14.2)+ 拷贝/移动未成对(2.15.6)+ delete/move/swap缺noexcept(3.2.1)

【建议】 — 注释风格(1.3.x)+ TODO/FIXME(1.3.3)+ 魔鬼数字(2.4.2)+ 冗余代码(2.1.3)

Python

【严重】 — 必检规则命中 + 敏感信息硬编码（密码/密钥/token写在代码中）+ pickle/yaml.load反序列化不可信数据 + subprocess.shell=True拼接外部输入

【一般】 — 裸except:未re-raise + 未使用logging而用print做日志 + 全局可变状态 + 函数超过50行 + 嵌套超过4层 + 未使用类型注解（公共API）+ import * + 循环导入

【建议】 — PEP 8风格违规 + 魔鬼数字/字符串 + 冗余代码 + TODO/FIXME + 缺少docstring（公共API）

4.5约束反证

对每个非"确定"置信度的发现，输出前执行反证检验。

核心：不要只确认"这像是个bug"，要反过来问——在什么条件下这个问题不成立？然后用工具验证这些条件是否成立。

步骤：

1. 列出反证条件。按bug类别，典型的"若成立则为误报"条件：

• 空指针解引用：调用链上游已判空（grep调用者）；构造函数/工厂保证非空；值域约束排除null
• 整数溢出：操作数值域受业务约束（枚举值、小常量、循环计数器）；类型提升后安全
• 资源泄漏：RAII/智能指针/自定义Guard管理；析构函数自动释放；调用者契约负责释放
• 数组越界：索引值域受循环条件/前置校验约束；容器有边界保护
• 并发问题：变量实际仅单线程访问（grep所有引用点）；已有锁/原子操作/屏障保护
• 逻辑缺陷：隐含前置条件使问题路径不可达；防御性编程已覆盖该场景
• 参考references/false-positives.md中的已确认误报模式

2. 工具验证。对每个反证条件，用Read/Grep/git show尝试验证，不要仅凭推测。

3. 判定：

• 反证条件经工具验证成立 → 抑制该发现
• 部分成立或无法判断 → 保持"待确认"，在报告中标注未能否证的原因
• 反证条件不成立 → 升级为"较确定"，附验证依据（如"已确认CreateSocket()失败时返回nullptr，见socket.cpp:78，调用链无判空"）

注意：严重级别的发现即使反证不彻底也应报出（标"待确认"），宁多勿漏。此步骤目的是减少误报，不是放过bug。

5. 输出格式

禁止使用表格展示发现。逐条块状输出，按严重级别降序，---分隔。"待确认"集中放末尾。无发现直接输出"未发现问题"，不凑数。

## 变更概述

本MR为[模块名]实现了[功能/修复]，主要变更：
- file_a.cpp: [变更要点]
涉及N个文件，M处新增/修改。

## 审查发现

共发现N个问题（严重x / 一般y / 建议z）

---

### #1 [严重] 使用了禁用函数memcpy
- 位置：`src/platform/transport/tcp_transport.cpp:142`
- 规则：2.18.1
- 置信度：确定

问题代码：
```cpp
memcpy(dst, src, len);
```

修复建议：
```cpp
errno_t ret = memcpy_s(dst, dstMax, src, len);
CHK_SAFETY_FUNC_RET(ret);
```

---

### #2 [严重] Destroy持锁期间执行阻塞同步操作
- 位置：`src/channel/hcomm_channel.cpp:245-250`
- 规则：CON-04
- 置信度：较确定
- 反证：已检查是否有异步替代路径或锁外执行的可能。确认`hcclStreamSynchronize`必须在`channelMutex_`持锁期间调用（因为stream句柄在锁保护范围内），且无超时参数，阻塞时间取决于设备负载（实测可达60秒）。反证条件不成立。

问题代码：
```cpp
std::lock_guard<std::mutex> lock(channelMutex_);
// ... 持锁期间
hcclStreamSynchronize(stream);  // 可阻塞60s+
```

修复建议：
将stream同步移到锁外，或使用带超时的同步API。

---

## 总结

[1-2句总体评价]
建议优先处理x个严重问题，其中N个确定，M个待确认。

输出规则：

• 每个发现必须含：位置（file:line或file:start-end）、规则编号、置信度、问题代码（只含问题行）、修复建议。"较确定"/"待确认"的发现还必须含- 反证：字段（见4.5节和输出格式示例）。连续多行问题用范围格式file:199-201，不连续用逗号file:199, 210
• 代码片段用围栏代码块（C++用```cpp，Python用```python，支持语法高亮）
• 报告即终止：报告以"## 总结"段落结尾。之后不要追加任何客套话、后续提示或对话性文字（如"如果需要更深入分析请告诉我"）。报告本身就是最终输出。

5.5输出自检清单

输出报告前逐项检查，不合格则修正后再输出：

1. 发现标题是否为 ### #N [等级] 描述 格式？（禁止 ### 发现 N: 等变体）
2. 等级标签是否为 [严重]/[一般]/[建议] 之一？
3. 置信度是否为"确定"/"较确定"/"待确认"之一？（禁止"高"/"中"/"低"）
4. 规则字段是否引用了具体规范编号（如2.18.1、红线1.5）而非自由描述（如"类型安全"）？
5. 属性表格是否包含"发现"汇总行（| 发现 | 严重x / 一般y / 建议z |）？
6. "## 审查发现"下方是否有 共发现N个问题（严重x / 一般y / 建议z） 汇总行？
7. 行号是否经过验证？（见下方行号自检）
8. 每个"较确定"/"待确认"发现是否经过约束反证（4.5节）？若未执行反证，回退执行后再输出。

5.6行号自检

输出报告前，对每个发现的行号执行自检：

1. 对照diff的@@ +行号 @@ hunk头验证行号正确性
2. hunk起始行号是第一个上下文行，不是第一个+行
3. 禁止猜测行号——如无法确认，用git show或Read工具重新定位

置信度

每个发现必须标注置信度。只允许以下三个词，禁止使用"高/中/低"等替代表述：

• 确定：机械匹配可判定（禁用函数、命名违规、缺大括号、未检查返回值）
• 较确定：已通过工具验证且经过约束反证（4.5节）。在输出中用- 反证：字段标注尝试否证的过程和结论，如："已检查调用链上游无判空保护（grep CreateSocket调用点共3处，均未检查返回值），反证条件不成立"
• 待确认：已尝试约束反证但无法判定。用- 反证：字段标注未能否证的原因和需人工确认的具体问题

过滤规则：不报告置信度低于"待确认"的发现。优先用工具提升置信度。宁多勿漏：宁可多报"待确认"的严重/一般问题让人工判断，也不漏掉真实缺陷。建议级别低置信度可省略。

C++命名规范速查

C++禁用函数速查

memcpy/bcopy→memcpy_s, memmove→memmove_s, memset→memset_s, strcpy/strncpy→strcpy_s/strncpy_s, strcat/strncat→strcat_s/strncat_s, sprintf/snprintf→sprintf_s/snprintf_s, vsprintf/vsnprintf→vsprintf_s/vsnprintf_s, scanf/sscanf/fscanf→scanf_s/sscanf_s/fscanf_s, gets→gets_s

Python PEP 8速查

Python常见反模式速查：

• except:/except Exception: → 指定具体异常类型
• def f(x=[]): → def f(x=None): x = x or []
• eval(user_input) → 使用ast.literal_eval或白名单解析
• os.system(cmd) → subprocess.run([...], shell=False)
• open(f)无close → with open(f) as fh:
• == None → is None
• type(x) == int → isinstance(x, int)