diff --git a/skills/chinese-code-review/CONTRIBUTORS b/skills/chinese-code-review/CONTRIBUTORS new file mode 100644 index 0000000..d8649da --- /dev/null +++ b/skills/chinese-code-review/CONTRIBUTORS @@ -0,0 +1 @@ +root diff --git a/skills/chinese-code-review/SKILL.md b/skills/chinese-code-review/SKILL.md new file mode 100644 index 0000000..9534d9e --- /dev/null +++ b/skills/chinese-code-review/SKILL.md @@ -0,0 +1,282 @@ +--- +name: chinese-code-review +description: 中文 review 沟通参考——话术模板、分级标注(必须修复/建议修改/仅供参考)、国内团队常见反模式应对。仅在用户显式 /chinese-code-review 时调用,不要根据上下文自动触发。 +version: "1.0.0" +license: MIT +metadata: + hermes: + tags: [code-review, chinese] +--- + +# 中文代码审查规范 + +## 概述 + +国内团队做 Code Review 常遇到两个极端:要么过度客气导致关键问题被放过,要么照搬西方直白风格让同事下不来台。本技能帮你找到平衡点——**既不回避问题,又让人愿意接受反馈**。 + +**核心原则:** 用"建议"代替"命令",用"提问"代替"否定",但绝不因为面子而放过 bug。 + +## 审查反馈的表达方式 + +### 用建议代替命令 + +| 避免(命令式) | 推荐(建议式) | +|---------------|---------------| +| 你必须改成 X | 建议考虑用 X,因为 Y | +| 这里写错了 | 这里可能存在一个问题,是否考虑过 Z 的情况? | +| 不要用这个方法 | 这个方法在 A 场景下可能有性能问题,可以看看 B 方案 | +| 这段代码不行 | 这段逻辑我理解得对吗?如果输入为空的话会怎样? | + +### 用提问代替否定 + +当你不确定对方意图时,先问再评: + +``` +# 好的方式 +这里用 sync 方式读文件是出于什么考虑?如果并发量上来,可能会阻塞事件循环。 + +# 不好的方式 +这里不应该用 sync 方式读文件。 +``` + +### 分级标注 + +统一使用优先级标记,让作者快速判断轻重缓急: + +- **[必须修复]** — 安全漏洞、数据丢失风险、逻辑错误(不修不能合) +- **[建议修改]** — 性能问题、可维护性、缺少校验(本次或下次迭代修复) +- **[仅供参考]** — 命名优化、风格建议、替代方案(不改也行) +- **[问题]** — 不确定的地方,需要作者解释意图 + +### 审查评论模板 + +``` +[必须修复] SQL 注入风险 + +第 42 行:用户输入直接拼接到 SQL 语句中。 + +原因:攻击者可以通过 name 参数注入 `'; DROP TABLE users; --`。 + +建议:使用参数化查询: + db.query('SELECT * FROM users WHERE name = $1', [name]) + +参考:https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html +``` + +## 中英混排代码注释规范 + +### 何时用中文 + +- **业务逻辑说明** — 用中文解释业务背景和需求来源 +- **复杂算法注释** — 用中文写思路,确保团队成员都能理解 +- **TODO / FIXME** — 用中文描述待办事项,方便搜索和追踪 +- **文档注释(内部项目)** — JSDoc / Javadoc 中的描述文字用中文 + +```typescript +/** + * 计算用户的会员等级折扣 + * + * 业务规则: + * - 普通会员 9.5 折 + * - 银卡会员 9 折 + * - 金卡会员 8.5 折 + * - 钻石会员 8 折 + * + * @param level - 会员等级(MemberLevel enum) + * @param amount - 原始金额(单位:分) + * @returns 折后金额(单位:分) + */ +function calculateDiscount(level: MemberLevel, amount: number): number { + // ... +} +``` + +### 何时用英文 + +- **变量名、函数名、类名** — 始终用英文命名,遵循团队命名规范 +- **Git commit message** — 参考下方 commit 规范 +- **开源项目注释** — 面向国际社区的项目,注释统一用英文 +- **错误信息和日志** — 生产环境的 error message 用英文(避免编码问题) +- **API 接口文档** — 对外暴露的 API 用英文 + +### 混排格式要求 + +```typescript +// 好:中英文之间加空格 +// 使用 Redis 缓存来减少 MySQL 的查询压力 + +// 坏:中英文之间没有空格 +// 使用Redis缓存来减少MySQL的查询压力 + +// 好:技术术语保留英文 +// 这里用 debounce 防抖处理,避免频繁触发 API 请求 + +// 坏:强行翻译技术术语 +// 这里用防抖动处理,避免频繁触发应用程序接口请求 +``` + +## Commit Message 中英双语格式 + +### 推荐格式 + +团队内部项目使用中文 commit message,采用约定式提交(Conventional Commits)的中文版: + +``` +<类型>(<范围>): <简要描述> + +<详细说明(可选)> + +<关联信息(可选)> +``` + +### 类型对照表 + +| 类型 | 含义 | 示例 | +|------|------|------| +| feat | 新功能 | feat(用户): 新增手机号登录功能 | +| fix | 修复 Bug | fix(支付): 修复微信支付回调重复处理的问题 | +| docs | 文档变更 | docs: 更新 API 接口文档 | +| style | 代码格式 | style: 统一缩进为 2 个空格 | +| refactor | 重构 | refactor(订单): 拆分订单服务,提取公共逻辑 | +| perf | 性能优化 | perf(列表): 虚拟滚动优化长列表渲染性能 | +| test | 测试 | test(auth): 补充登录模块单元测试 | +| chore | 构建/工具 | chore: 升级 Node.js 至 v20 | + +### 示例 + +``` +fix(支付): 修复支付宝异步回调签名校验失败的问题 + +原因:升级 SDK 后签名算法从 RSA 变为 RSA2,但回调校验仍使用旧算法。 +方案:回调处理中同时兼容 RSA 和 RSA2 签名校验。 + +Closes #1234 +``` + +### 面向国际社区的项目 + +如果项目面向国际社区或有外籍成员,commit message 用英文,PR 描述中可附加中文说明: + +``` +fix(payment): fix Alipay async callback signature verification failure + +The SDK upgrade changed the signature algorithm from RSA to RSA2, +but the callback handler still used the old algorithm. + +Closes #1234 +``` + +## 常见反模式与对策 + +### 反模式一:过度客气 + +**表现:** 所有评论都是"我觉得可能也许大概好像这里有个小问题"。 + +**后果:** 关键 bug 被隐藏在一堆委婉语里,作者根本不知道哪些必须改。 + +**对策:** 使用分级标注。[必须修复] 就是必须修复,语气可以温和,但级别必须准确。 + +``` +# 坏:过度客气 +不知道我理解得对不对,这里好像可能有一点点并发问题,不过也许我看错了... + +# 好:温和但清晰 +[必须修复] 并发安全问题 + +这里的 map 在多个 goroutine 中同时读写,会触发 panic。 +建议加 sync.RWMutex,或者换成 sync.Map。 + +复现方式:加 -race flag 跑测试就能看到。 +``` + +### 反模式二:不敢给高级开发者提意见 + +**表现:** 高级开发者或 Leader 的代码直接 Approve,不仔细看。 + +**后果:** 代码质量双标,团队对 Code Review 失去信任。 + +**对策:** Code Review 对事不对人。可以换个表达方式: + +``` +# 提问式(适合给资深同事的反馈) +想请教一下,这里选择用递归而不是迭代,是出于什么考虑? +我在想如果递归深度超过 1000 层会不会有栈溢出的风险? + +# 学习式 +学到了一个新写法!不过有个小疑问——这里的类型断言在运行时不会做检查, +如果上游数据结构变了,这里会静默通过。是否考虑加个 runtime validation? +``` + +### 反模式三:审查变成风格之争 + +**表现:** 大量评论纠结于缩进、空格、花括号位置。 + +**后果:** 浪费时间,忽略真正的问题。 + +**对策:** 风格问题交给 ESLint / Prettier / gofmt 等工具自动处理。Code Review 聚焦逻辑、安全、性能。 + +### 反模式四:只写"LGTM" + +**表现:** 随手一个 LGTM 就 Approve,没有实质性审查。 + +**后果:** Code Review 形同虚设,出了问题没人兜底。 + +**对策:** 即使代码质量很好,也要写出你关注了哪些方面: + +``` +LGTM + +审查了以下方面: +- 并发安全:锁的粒度合理 +- 错误处理:所有外部调用都有 error handling +- 向下兼容:新增字段都有默认值,不影响老版本 + +一个小建议 [仅供参考]:第 78 行的变量名 `d` 可以改成 `duration`,更易读。 +``` + +## 审查流程建议 + +### 开始审查前 + +1. **先看 PR 描述**,理解改动的背景和目的 +2. **看关联的 Issue 或需求文档** +3. **先整体浏览**,再逐文件细看 + +### 审查顺序 + +1. **架构层面** — 方案是否合理?有没有更好的方式? +2. **正确性** — 逻辑对不对?边界条件处理了吗? +3. **安全性** — 有没有注入、越权、信息泄露? +4. **性能** — 有没有 N+1 查询、内存泄漏、不必要的循环? +5. **可维护性** — 半年后能看懂吗?测试覆盖了吗? +6. **风格** — 只关注工具无法自动处理的部分 + +### 给出总结 + +审查结束后,给一段总结,包括: +- 整体评价(一句话) +- 值得学习的地方(先扬后抑) +- 主要问题列表(按优先级) +- 建议的修改方向 + +``` +总结:整体实现思路清晰,支付回调的幂等处理很到位。 + +主要问题: +1. [必须修复] 并发写 map 的问题(2 处) +2. [建议修改] 缺少对空值的校验(3 处) +3. [仅供参考] 几个变量命名可以更语义化 + +建议先修复并发问题,校验的部分可以本次一起改或者拆到下个迭代。 +``` + +## 检查清单 + +在提交审查意见前,确认: + +- [ ] 每条评论都标注了优先级 +- [ ] [必须修复] 的问题都给出了具体的修复建议 +- [ ] 没有因为面子而跳过关键问题 +- [ ] 没有纠结于工具能自动处理的风格问题 +- [ ] 对好的代码给予了肯定 +- [ ] 给出了整体总结