1. 先搞清楚代码审查到底在解决什么问题很多团队把代码审查当成“走形式”提完 MR 之后找个同事点一下 Approve然后合入、上线、完事。一旦线上出问题大家又开始互相问“当时谁 Review 的”。我做过好几个项目的 code review 落地最深的感触是open-code-review 这个方向真正解决的不是“谁看代码”而是“我们怎么保证交付质量的下限”。它是一套从开源社区沉淀出来的、可以被任意团队复用的代码审查方法论也可以落地成一套自动化辅助工具链。它适合任何用 Git 做协作、有代码合入流程、希望在问题进入主干之前就被发现和拦截的团队。代码审查为什么值得认真对待我用一个类比来解释写代码就像在高速公路上开车编码是踩油门审查是方向盘和刹车。油门踩得快不快决定效率但方向盘和刹车决定你能不能安全到达目的地。没有审查的代码合入就像一台没有刹车的车短期看起来跑得很欢长期一定出事。这里需要明确一点open-code-review 不是指某一家公司开发的商业工具而是指在开源协作模式下代码审查应该具备的开放心态、标准化流程和配套工具链。你可以把它理解为一套开放的审查实践——凡是参与代码贡献的人无论是核心维护者还是偶尔提 PR 的开发者都能用同一套规则来保证代码质量。这才是“open”的含义。1.1 代码审查的核心价值代码审查最直接的价值是发现 Bug。但如果你以为它只是为了抓 Bug格局就小了。根据我自己的实际经验代码审查的价值至少有三个层次。第一层是即时质量保障。一个改动进入主干之前先让另外一个人过一遍可以从思路上发现逻辑错误、边界条件遗漏、安全隐患、性能问题。很多 Bug 在写代码的人眼里是“看不到”的因为我们对自己写的代码有思维惯性或者说是一种“盲区”。自己写的代码大脑会自动补全逻辑中缺失的部分。别人看的时候没有这种补全反而能一眼看出问题。第二层是知识传递。代码审查是最好的团队学习场景之一。新人通过看老人的审查意见能学会团队的编码规范、架构约定、业务逻辑。老人通过审查新人的代码也能了解到新的写法和思路。我在实际工作中发现一个团队如果坚持做高质量的 code review成员之间的代码风格会自然趋同新人的成长速度也会明显加快。第三层是架构守护。没有代码审查的团队系统会一点点腐烂。今天这里加一个临时判断明天那里硬编码一个字符串后天又在业务代码里塞了一段与业务无关的逻辑。每个改动单独看都不致命但积累半年之后系统会变得让人看不懂、不敢改。代码审查是防止架构腐化的最有效手段因为它把关的是每一次合入的“增量”。1.2 小型团队和开源项目怎么选择方案说到方案选择很多小团队会陷入一个误区看到大厂搞了一整套复杂的代码审查流水线觉得自己也要照搬。以我的经验看这完全是本末倒置。代码审查的核心是“人的判断”工具只是辅助。小团队最应该做的是先把流程跑通再逐步加自动化。如果是 1 到 5 人的小团队或者个人开源项目我认为最合理的方案是用 GitHub 或 GitLab 自带的 MR 审查功能加上一些轻量级的自动化检查比如 pre-commit、ESLint、格式检查不需要单独部署额外的系统。等团队规模到了中等水平再引入代码覆盖率统计、SonarQube 这类静态扫描工具。如果你所在的组织已经有了比较成熟的研发管理平台那更省事只需要约定好审查规则、分支保护策略然后把 open-code-review 的实践方式固化到团队规范里。这里我想强调一个原则方案越简单越容易坚持。一个每天要花 20 分钟处理工具的流程团队一定坚持不了三个月。工具是为人服务的不要让工具变成负担。2. 从零搭一套轻量级审查流程2.1 最小闭环MR 驱动的审查链路我这几年帮多个团队搭建过 code review 流程最稳定、最不容易被绕过的方案是“MR 驱动的审查链路”。这里的 MR 既可以指 GitHub 的 Pull Request也可以指 GitLab 的 Merge Request。核心思想是所有的代码变更必须以合并请求的形式提出必须经过至少一个审查人的明确同意才能合入主干分支。一个最小闭环包含五个环节。第一步创建分支。开发者从主干分支切出特性分支例如feature/xxx或者fix/xxx。分支命名规范值得认真约定因为三个月之后你要从几十个分支里找到某次改动对应的分支如果名字乱写会浪费不少时间。第二步本地开发和自测。在提交 MR 之前开发者要自己先把代码过一遍至少完成单元测试、格式化检查、lint 检查。我见过太多人把 MR 一提就当甩手掌柜结果 CI 一下跑出十几个编译错误。这不是代码审查的问题是责任心的问题。第三步创建 MR。提交 MR 时要写清楚改了什么、为什么改、怎么测试验证。模板这东西非常有用我建议每个团队都维护一个 MR 描述模板让开发者在创建 MR 时按模板填写。这样审查者不用额外花时间问“你这个改动的背景是什么”。第四步指定审查者。审查者通常由两部分组成一个是熟悉这块代码的资深开发者负责技术把关一个是与本改动相关的业务负责人负责业务逻辑和影响范围确认。第五步审查通过后合入。合入方式建议选择 squash merge 或者 rebase merge保持主干历史的整洁。不要选择直接 merge否则主干分支会留一堆 merge commit历史记录一塌糊涂。注意分支保护策略一定要开启。主干分支应该设置为受保护分支不允许任何人直接 push只允许通过 MR 合入。这是整个链条的地基如果这层没守住前面的流程全都白费。2.2 自动化检查先顶上再放人进来人工审查的价值很高但成本也高。一个人的时间只有那么多如果每次都把简单的格式问题、明显的语法问题丢给人来看那审查者很快就会疲劳真正该花精力去思考的架构问题反而没人认真看了。所以我的建议是能用自动化检查解决的绝不让它浪费人工。自动化检查可以分三层。第一层是本地钩子。推荐使用 husky 配合 lint-staged在开发者本地提交代码的时候就自动做 lint 和格式化检查。这样问题在最早期的环节就被拦截开发者自己就能修复根本不需要进入审查流程。实测下来这一层能拦截掉至少三成的低级问题。第二层是 CI 流水线。MR 创建之后自动触发静态检查、单元测试、构建验证、覆盖率统计。如果这些检查不通过MR 根本都不应该进入人工审查环节。很多团队用的是 GitHub Actions 或者 GitLab CI配置起来都不复杂。我在多个项目里用过的组合是ESLintJavaScript/TypeScript 静态检查 Jest单测 SonarQube代码质量和安全隐患扫描效果很稳定。第三层是自动合并检查。比如要求 MR 必须关联到对应的需求或者 Bug 单检查分支是否有冲突检查是否满足“至少 N 个审查者批准”的条件。GitHub 的 branch protection rules 和 GitLab 的 approval rules 都支持这类配置。我要特别提醒的是自动化检查不能替代人工审查它只是替人工做了那些重复性的、确定性的、可枚举的检查项。真正关于逻辑、设计、业务场景的判断还是需要人来完成。工具和人不是替代关系是分工关系。3. 核心环节怎么看一份可落地的审查清单3.1 第一遍快速浏览理解改动的目标正式开始审查别人的代码时最容易犯的错误是上来就看 diff一行一行地过看到哪算哪。这种方式的效率极低而且很容易错过核心问题。我做审查的习惯是分三遍走。第一遍先不看代码只看 MR 的描述、关联的 issue、相关设计文档。搞清楚这个 MR 的目标是什么预期解决什么问题影响的范围包括哪些模块风险的评估如何。这一步的核心目标是建立“上下文”。比如我最近审查一个支付模块的改动MR 描述里写“修复当余额不足时支付失败后没有回滚的问题”。我先理解目标再去 diff 里找“余额判断”“失败分支”“回滚逻辑”这几处关键点而不是漫无目的地看整份 diff。这样审查效率非常高而且能很快对代码是否符合目标做出判断。3.2 第二遍细看逻辑把 Bug 拦截在合入之前第二遍才是一行一行看代码但已经不是无目的地看了而是带着问题去看。这一遍我会重点留意几类问题第一类是边界条件。传参的边界值、空值、null、超长字符串、并发场景下的临界状态。这些都是 Bug 的高发区尤其是 if 分支的判断条件我会反复推演各种输入下的走向。第二类是异常处理。网络请求失败怎么办、依赖服务超时怎么办、数据库操作中途失败怎么办、消息队列消费失败怎么重试。很多同学只写了正常路径对异常路径不管不顾这种代码上线之后迟早会捅娄子。第三类是安全问题。权限校验是否齐全、用户输入是否有校验、敏感数据是否有脱敏、SQL 拼接是否可能被注入。安全问题是代码审查里最不能手软的一类一旦发现必须修改之后才能合入。第四类是性能隐患。是否有在循环里做数据库查询、是否加载了过多不必要的数据、慢 SQL 是否加了索引、是否有明显的死锁风险。第五类是逻辑一致性。改动的代码是否与现有代码的约定一致比如是同步实现还是异步实现是返回 boolean 还是返回错误码是手动事务还是声明式事务这些都会影响到后续维护者的心智。第二遍看下来大部分问题会浮现出来。做记录的时候有一个技巧把发现的问题按照严重程度分级。T0 级是必须修复的比如安全问题、致命的逻辑错误T1 级是应该修复的比如明显的异常未处理、性能隐患T2 级是可以优化的比如命名不规范、代码风格不一致。分级之后审查意见会更有说服力也方便开发者按优先级处理。3.3 第三遍换位验证审查可维护性与扩展性第三遍是站在未来维护者的角度去审视。我会问自己几个问题三个月之后如果我自己来维护这段代码我能看懂吗如果需求发生变化这个代码结构是否容易扩展如果出了线上问题日志里能否快速定位到问题所在这个环节主要是看代码的结构设计、命名、封装、注释的质量。比如一个函数是不是太长了一个类是不是承担了过多职责常量是否写成魔法数字重复代码是否可以抽取复用依赖是否合理模块边界是否清晰。我提供一个审查清单的模板你可以直接复制到团队规范里使用。审查维度检查要点严重级别需求一致性代码实现是否与需求描述一致T0边界条件空值、越界、并发、超时等场景是否覆盖T0异常处理失败分支是否有兜底逻辑T0安全权限、注入、敏感数据是否处理正确T0性能是否存在循环查询、慢 SQL、大对象加载T1可读性命名清晰、函数不超长、逻辑容易理解T1可测试性核心逻辑是否可单测是否有测试覆盖T1一致性是否遵守团队代码规范和架构约定T2冗余代码是否有无用依赖、死代码、重复逻辑T2文档注释复杂逻辑是否有必要注释说明T2提示这个清单不是用来“打钩作业”的而是用来帮助审查者建立系统的思考框架。熟练之后这些检查项会内化为你的直觉。4. 实操过程记录一次完整的 open-code-review4.1 场景设定与前置准备为了把上面的方法论讲透我模拟一次真实的审查过程。假设我们的项目是一个电商系统的订单服务本次 MR 的内容是“新增订单取消接口支持用户取消未支付的订单”。MR 描述写得很规范背景用户在下单之后如果一直不支付订单会占用库存需要提供取消入口改动范围新增OrderController.cancelOrder接口修改订单状态流转逻辑新增库存释放逻辑测试验证本地联调通过单元测试覆盖了正常取消、重复取消、订单不存在三种场景关联需求ORD-1024代码 diff 的核心片段如下我先简化一下PostMapping(/order/cancel) public Result cancelOrder(RequestParam Long orderId) { Order order orderMapper.selectById(orderId); if (order null) { return Result.fail(订单不存在); } if (order.getStatus() ! OrderStatus.CREATED) { return Result.fail(订单状态不允许取消); } order.setStatus(OrderStatus.CANCELLED); orderMapper.updateById(order); stockService.releaseStock(order.getId(), order.getProductId(), order.getQuantity()); return Result.success(); }第一眼看这段代码逻辑很清晰查询订单、校验状态、更新状态、释放库存。如果没有带着审查的清单去看很容易直接点 Approve。但如果你按照我前面说的“三遍法”漏洞就会浮出来。4.2 逐段 Review 的记录先看第一遍的上下文。这个 MR 要解决的是“用户取消未支付订单”的问题。从业务角度看取消订单涉及到两个核心状态“未支付订单被取消”和“库存被释放”。好带着这个上下文看代码。看到lock了吗没看到。这个接口现在完全没有并发保护。如果用户点了两次取消按钮两个请求同时进来两个线程都查到订单状态是 CREATED都通过了校验然后一次更新成 CANCELLED另一次也更新成 CANCELLED同时释放两次库存。库存释放重复执行在真实的电商系统里这是重大事故。我必须标记为 T0 级问题。再看异常处理。stockService.releaseStock如果抛异常订单状态已经更新成 CANCELLED 了库存却没有释放成功数据和库存就会不一致。这里存在的就是事务一致性问题。orderMapper.updateById和stockService.releaseStock必须放在同一个事务里要么都成功要么都失败。这也是 T0 级问题。再考虑一个问题cancelOrder只判断了订单状态是 CREATED但是不同来源的订单是否都可取消比如营销活动订单、秒杀订单、预售订单它们的取消规则是否一致这个问题我不用自己猜直接在 MR 评论里提出来让开发者确认。这属于 T1 级问题因为它可能是业务规则上的遗漏。安全相关的问题也看一下接口是否做了登录校验是否有权限校验是否校验了“当前用户只能取消自己的订单”如果用户传别人的订单号是不是也能取消这不是本次 diff 里能完全看到的内容因为可能依赖拦截器但我需要在审查意见里提醒开发者确认。这个如果是缺失那就是 T0。代码风格方面order.getStatus() ! OrderStatus.CREATED直接与枚举比较建议用枚举自身提供的方法或封装常量增加可读性。这是 T2 级问题。4.3 根据审查意见优化后的代码综合以上分析我会在 MR 评论里给出意见开发者修改之后的版本大概是这样的PostMapping(/order/cancel) Transactional(rollbackFor Exception.class) public Result cancelOrder(RequestParam Long orderId) { Order order orderMapper.selectByIdForUpdate(orderId); if (order null) { return Result.fail(订单不存在); } if (!order.ownedBy(loginUserId())) { return Result.fail(只能取消自己的订单); } if (!order.canCancel()) { return Result.fail(订单状态不允许取消); } order.cancel(); orderMapper.updateById(order); stockService.releaseStock(order.getId(), order.getProductId(), order.getQuantity()); return Result.success(); }这里几个关键的改动点是selectByIdForUpdate加行锁防止并发重复取消Transactional保证订单状态更新和库存释放的原子性ownedBy加了当前用户的归属校验canCancel把取消状态的判断封装到领域对象里。这样的代码明显更稳经得起推敲。这一套“发现问题、提出意见、修改代码、结果验证”的闭环就是 open-code-review 的完整形态。5. 常见问题与排查技巧实录5.1 PR 堆积没人审查怎么办这个现象在小团队里太常见了。开发者把 MR 提出来之后指派了审查人然后左等右等一整天都没人理。我见过很多团队因为审查延迟导致开发阻塞最后大家索性不按流程走了直接 push 到主干。从根子上说审查堆积的原因通常有两个一是没有约定响应时间二是审查者没有把审查当成自己工作的一部分。解决办法也很直接在团队规范里写明“MR 必须在 4 个工作小时内被首次回复”。这里的首次回复可以是审查意见、可以是确认稍后处理但不能不理会。另外从流程上建议在 MR 合并条件中开启“必须授权才能合入”强制推动审查动作落地。我自己还实践过一个方法每天固定两个时间段处理审查比如上午十点半和下午四点半。到了时间就把待审查的 MR 拉出来集中处理效率比随时被打断高很多。5.2 Review 意见掰扯不清最后变成吵架代码审查讨论到最后变成意气之争的情况我相信每个团队都遇到过。意见本身没有问题问题往往出在表达方式和判断标准上。先说表达方式。审查者提意见的时候建议用“三明治”句式先肯定代码的合理之处再指出具体问题和改进建议最后收尾表达整体认可或整体保留意见。这种表达方式不是为了让谁舒服而是让讨论聚焦在“问题本身”而不是“谁对谁错”。再说判断标准。代码审查中最浪费时间的讨论是“这个写法好还是那个写法好”这类没有标准答案的问题。要避免这种情况团队应该先把编码规范定好。比如复杂逻辑必须写清楚注释函数尽量控制在 50 行以内不允许重复的魔法数字禁止在循环里调用远程服务规范越明确审查意见越容易达成一致。遇到规范没有覆盖的问题审查者需要表明立场这是一个“建议”而不是“必须”让开发者根据实际情况决定。注意代码审查中最没价值的意见是纯粹个人偏好型的“如果是我会这么写”。除非有明确的理由说明这样写更好否则这类意见只会增加沟通成本不会提升代码质量。5.3 自动化跑过了人还要看什么有了 CI、静态检查、覆盖率之后有一种新的坑开发者觉得“CI 都过了审查也就是走个流程”。这是一个非常大的误解我强调过很多次。自动化能检查的是“确定性的规则”。比如代码格式、未使用的变量、安全漏洞的已知模式、测试覆盖率是否达标。但自动化无法判断的是“这个实现方案是否正确”“这个交互设计是否合理”“这个分支的取舍是否符合业务预期”“这段逻辑将来是否容易被误改”。换句话说自动化检查是“下游拦截”人工审查才是“上游控制”。我在实践中最深的一个体会是真正挽救过我的项目的往往不是“自动检查发现了一个空指针”而是“同事在审查的时候问我如果这个接口被频繁调用你这段逻辑成不成立”。这种从业务场景出发的质疑自动化永远做不了。所以我建议每个团队把下面这个事情做成硬性约束即使所有 CI 检查都通过每次 MR 依然需要至少一个人做完整的人工审查并在 MR 里留下评论说明“核心逻辑已经理解同意合入”。5.4 一份可以直接用的团队落地速查表最后把我认为最关键的实践要点整理成一份速查表可以直接作为团队文档的基础。主题推荐做法不推荐的做法MR 创建使用模板说明背景、改动、测试方式随手丢一段代码上来自动化检查本地钩子 CI 检查双层拦截只靠人工看格式分支保护开启禁止直推、必须 MR 合入谁都能改主干审查响应4 小时内首次回复拖到第二天甚至更久意见分级T0/T1/T2 分级反馈所有问题不分轻重混在一起审核人选择模块资深者 业务负责人随便找个人点通过合入方式合并需求后使用 squash merge保留大量无意义的 merge commit这套流程刚跑起来的一到两周团队会不太适应总觉得额外花了时间。但坚持一个月左右大家就会感受到明显的区别主干分支的质量明显提升上线后的问题明显减少新人也通过审查学到了不少东西。这也是我做 open-code-review 相关实践以来最大的成就感来源。
1. 先搞清楚代码审查到底在解决什么问题很多团队把代码审查当成“走形式”提完 MR 之后找个同事点一下 Approve然后合入、上线、完事。一旦线上出问题大家又开始互相问“当时谁 Review 的”。我做过好几个项目的 code review 落地最深的感触是open-code-review 这个方向真正解决的不是“谁看代码”而是“我们怎么保证交付质量的下限”。它是一套从开源社区沉淀出来的、可以被任意团队复用的代码审查方法论也可以落地成一套自动化辅助工具链。它适合任何用 Git 做协作、有代码合入流程、希望在问题进入主干之前就被发现和拦截的团队。代码审查为什么值得认真对待我用一个类比来解释写代码就像在高速公路上开车编码是踩油门审查是方向盘和刹车。油门踩得快不快决定效率但方向盘和刹车决定你能不能安全到达目的地。没有审查的代码合入就像一台没有刹车的车短期看起来跑得很欢长期一定出事。这里需要明确一点open-code-review 不是指某一家公司开发的商业工具而是指在开源协作模式下代码审查应该具备的开放心态、标准化流程和配套工具链。你可以把它理解为一套开放的审查实践——凡是参与代码贡献的人无论是核心维护者还是偶尔提 PR 的开发者都能用同一套规则来保证代码质量。这才是“open”的含义。1.1 代码审查的核心价值代码审查最直接的价值是发现 Bug。但如果你以为它只是为了抓 Bug格局就小了。根据我自己的实际经验代码审查的价值至少有三个层次。第一层是即时质量保障。一个改动进入主干之前先让另外一个人过一遍可以从思路上发现逻辑错误、边界条件遗漏、安全隐患、性能问题。很多 Bug 在写代码的人眼里是“看不到”的因为我们对自己写的代码有思维惯性或者说是一种“盲区”。自己写的代码大脑会自动补全逻辑中缺失的部分。别人看的时候没有这种补全反而能一眼看出问题。第二层是知识传递。代码审查是最好的团队学习场景之一。新人通过看老人的审查意见能学会团队的编码规范、架构约定、业务逻辑。老人通过审查新人的代码也能了解到新的写法和思路。我在实际工作中发现一个团队如果坚持做高质量的 code review成员之间的代码风格会自然趋同新人的成长速度也会明显加快。第三层是架构守护。没有代码审查的团队系统会一点点腐烂。今天这里加一个临时判断明天那里硬编码一个字符串后天又在业务代码里塞了一段与业务无关的逻辑。每个改动单独看都不致命但积累半年之后系统会变得让人看不懂、不敢改。代码审查是防止架构腐化的最有效手段因为它把关的是每一次合入的“增量”。1.2 小型团队和开源项目怎么选择方案说到方案选择很多小团队会陷入一个误区看到大厂搞了一整套复杂的代码审查流水线觉得自己也要照搬。以我的经验看这完全是本末倒置。代码审查的核心是“人的判断”工具只是辅助。小团队最应该做的是先把流程跑通再逐步加自动化。如果是 1 到 5 人的小团队或者个人开源项目我认为最合理的方案是用 GitHub 或 GitLab 自带的 MR 审查功能加上一些轻量级的自动化检查比如 pre-commit、ESLint、格式检查不需要单独部署额外的系统。等团队规模到了中等水平再引入代码覆盖率统计、SonarQube 这类静态扫描工具。如果你所在的组织已经有了比较成熟的研发管理平台那更省事只需要约定好审查规则、分支保护策略然后把 open-code-review 的实践方式固化到团队规范里。这里我想强调一个原则方案越简单越容易坚持。一个每天要花 20 分钟处理工具的流程团队一定坚持不了三个月。工具是为人服务的不要让工具变成负担。2. 从零搭一套轻量级审查流程2.1 最小闭环MR 驱动的审查链路我这几年帮多个团队搭建过 code review 流程最稳定、最不容易被绕过的方案是“MR 驱动的审查链路”。这里的 MR 既可以指 GitHub 的 Pull Request也可以指 GitLab 的 Merge Request。核心思想是所有的代码变更必须以合并请求的形式提出必须经过至少一个审查人的明确同意才能合入主干分支。一个最小闭环包含五个环节。第一步创建分支。开发者从主干分支切出特性分支例如feature/xxx或者fix/xxx。分支命名规范值得认真约定因为三个月之后你要从几十个分支里找到某次改动对应的分支如果名字乱写会浪费不少时间。第二步本地开发和自测。在提交 MR 之前开发者要自己先把代码过一遍至少完成单元测试、格式化检查、lint 检查。我见过太多人把 MR 一提就当甩手掌柜结果 CI 一下跑出十几个编译错误。这不是代码审查的问题是责任心的问题。第三步创建 MR。提交 MR 时要写清楚改了什么、为什么改、怎么测试验证。模板这东西非常有用我建议每个团队都维护一个 MR 描述模板让开发者在创建 MR 时按模板填写。这样审查者不用额外花时间问“你这个改动的背景是什么”。第四步指定审查者。审查者通常由两部分组成一个是熟悉这块代码的资深开发者负责技术把关一个是与本改动相关的业务负责人负责业务逻辑和影响范围确认。第五步审查通过后合入。合入方式建议选择 squash merge 或者 rebase merge保持主干历史的整洁。不要选择直接 merge否则主干分支会留一堆 merge commit历史记录一塌糊涂。注意分支保护策略一定要开启。主干分支应该设置为受保护分支不允许任何人直接 push只允许通过 MR 合入。这是整个链条的地基如果这层没守住前面的流程全都白费。2.2 自动化检查先顶上再放人进来人工审查的价值很高但成本也高。一个人的时间只有那么多如果每次都把简单的格式问题、明显的语法问题丢给人来看那审查者很快就会疲劳真正该花精力去思考的架构问题反而没人认真看了。所以我的建议是能用自动化检查解决的绝不让它浪费人工。自动化检查可以分三层。第一层是本地钩子。推荐使用 husky 配合 lint-staged在开发者本地提交代码的时候就自动做 lint 和格式化检查。这样问题在最早期的环节就被拦截开发者自己就能修复根本不需要进入审查流程。实测下来这一层能拦截掉至少三成的低级问题。第二层是 CI 流水线。MR 创建之后自动触发静态检查、单元测试、构建验证、覆盖率统计。如果这些检查不通过MR 根本都不应该进入人工审查环节。很多团队用的是 GitHub Actions 或者 GitLab CI配置起来都不复杂。我在多个项目里用过的组合是ESLintJavaScript/TypeScript 静态检查 Jest单测 SonarQube代码质量和安全隐患扫描效果很稳定。第三层是自动合并检查。比如要求 MR 必须关联到对应的需求或者 Bug 单检查分支是否有冲突检查是否满足“至少 N 个审查者批准”的条件。GitHub 的 branch protection rules 和 GitLab 的 approval rules 都支持这类配置。我要特别提醒的是自动化检查不能替代人工审查它只是替人工做了那些重复性的、确定性的、可枚举的检查项。真正关于逻辑、设计、业务场景的判断还是需要人来完成。工具和人不是替代关系是分工关系。3. 核心环节怎么看一份可落地的审查清单3.1 第一遍快速浏览理解改动的目标正式开始审查别人的代码时最容易犯的错误是上来就看 diff一行一行地过看到哪算哪。这种方式的效率极低而且很容易错过核心问题。我做审查的习惯是分三遍走。第一遍先不看代码只看 MR 的描述、关联的 issue、相关设计文档。搞清楚这个 MR 的目标是什么预期解决什么问题影响的范围包括哪些模块风险的评估如何。这一步的核心目标是建立“上下文”。比如我最近审查一个支付模块的改动MR 描述里写“修复当余额不足时支付失败后没有回滚的问题”。我先理解目标再去 diff 里找“余额判断”“失败分支”“回滚逻辑”这几处关键点而不是漫无目的地看整份 diff。这样审查效率非常高而且能很快对代码是否符合目标做出判断。3.2 第二遍细看逻辑把 Bug 拦截在合入之前第二遍才是一行一行看代码但已经不是无目的地看了而是带着问题去看。这一遍我会重点留意几类问题第一类是边界条件。传参的边界值、空值、null、超长字符串、并发场景下的临界状态。这些都是 Bug 的高发区尤其是 if 分支的判断条件我会反复推演各种输入下的走向。第二类是异常处理。网络请求失败怎么办、依赖服务超时怎么办、数据库操作中途失败怎么办、消息队列消费失败怎么重试。很多同学只写了正常路径对异常路径不管不顾这种代码上线之后迟早会捅娄子。第三类是安全问题。权限校验是否齐全、用户输入是否有校验、敏感数据是否有脱敏、SQL 拼接是否可能被注入。安全问题是代码审查里最不能手软的一类一旦发现必须修改之后才能合入。第四类是性能隐患。是否有在循环里做数据库查询、是否加载了过多不必要的数据、慢 SQL 是否加了索引、是否有明显的死锁风险。第五类是逻辑一致性。改动的代码是否与现有代码的约定一致比如是同步实现还是异步实现是返回 boolean 还是返回错误码是手动事务还是声明式事务这些都会影响到后续维护者的心智。第二遍看下来大部分问题会浮现出来。做记录的时候有一个技巧把发现的问题按照严重程度分级。T0 级是必须修复的比如安全问题、致命的逻辑错误T1 级是应该修复的比如明显的异常未处理、性能隐患T2 级是可以优化的比如命名不规范、代码风格不一致。分级之后审查意见会更有说服力也方便开发者按优先级处理。3.3 第三遍换位验证审查可维护性与扩展性第三遍是站在未来维护者的角度去审视。我会问自己几个问题三个月之后如果我自己来维护这段代码我能看懂吗如果需求发生变化这个代码结构是否容易扩展如果出了线上问题日志里能否快速定位到问题所在这个环节主要是看代码的结构设计、命名、封装、注释的质量。比如一个函数是不是太长了一个类是不是承担了过多职责常量是否写成魔法数字重复代码是否可以抽取复用依赖是否合理模块边界是否清晰。我提供一个审查清单的模板你可以直接复制到团队规范里使用。审查维度检查要点严重级别需求一致性代码实现是否与需求描述一致T0边界条件空值、越界、并发、超时等场景是否覆盖T0异常处理失败分支是否有兜底逻辑T0安全权限、注入、敏感数据是否处理正确T0性能是否存在循环查询、慢 SQL、大对象加载T1可读性命名清晰、函数不超长、逻辑容易理解T1可测试性核心逻辑是否可单测是否有测试覆盖T1一致性是否遵守团队代码规范和架构约定T2冗余代码是否有无用依赖、死代码、重复逻辑T2文档注释复杂逻辑是否有必要注释说明T2提示这个清单不是用来“打钩作业”的而是用来帮助审查者建立系统的思考框架。熟练之后这些检查项会内化为你的直觉。4. 实操过程记录一次完整的 open-code-review4.1 场景设定与前置准备为了把上面的方法论讲透我模拟一次真实的审查过程。假设我们的项目是一个电商系统的订单服务本次 MR 的内容是“新增订单取消接口支持用户取消未支付的订单”。MR 描述写得很规范背景用户在下单之后如果一直不支付订单会占用库存需要提供取消入口改动范围新增OrderController.cancelOrder接口修改订单状态流转逻辑新增库存释放逻辑测试验证本地联调通过单元测试覆盖了正常取消、重复取消、订单不存在三种场景关联需求ORD-1024代码 diff 的核心片段如下我先简化一下PostMapping(/order/cancel) public Result cancelOrder(RequestParam Long orderId) { Order order orderMapper.selectById(orderId); if (order null) { return Result.fail(订单不存在); } if (order.getStatus() ! OrderStatus.CREATED) { return Result.fail(订单状态不允许取消); } order.setStatus(OrderStatus.CANCELLED); orderMapper.updateById(order); stockService.releaseStock(order.getId(), order.getProductId(), order.getQuantity()); return Result.success(); }第一眼看这段代码逻辑很清晰查询订单、校验状态、更新状态、释放库存。如果没有带着审查的清单去看很容易直接点 Approve。但如果你按照我前面说的“三遍法”漏洞就会浮出来。4.2 逐段 Review 的记录先看第一遍的上下文。这个 MR 要解决的是“用户取消未支付订单”的问题。从业务角度看取消订单涉及到两个核心状态“未支付订单被取消”和“库存被释放”。好带着这个上下文看代码。看到lock了吗没看到。这个接口现在完全没有并发保护。如果用户点了两次取消按钮两个请求同时进来两个线程都查到订单状态是 CREATED都通过了校验然后一次更新成 CANCELLED另一次也更新成 CANCELLED同时释放两次库存。库存释放重复执行在真实的电商系统里这是重大事故。我必须标记为 T0 级问题。再看异常处理。stockService.releaseStock如果抛异常订单状态已经更新成 CANCELLED 了库存却没有释放成功数据和库存就会不一致。这里存在的就是事务一致性问题。orderMapper.updateById和stockService.releaseStock必须放在同一个事务里要么都成功要么都失败。这也是 T0 级问题。再考虑一个问题cancelOrder只判断了订单状态是 CREATED但是不同来源的订单是否都可取消比如营销活动订单、秒杀订单、预售订单它们的取消规则是否一致这个问题我不用自己猜直接在 MR 评论里提出来让开发者确认。这属于 T1 级问题因为它可能是业务规则上的遗漏。安全相关的问题也看一下接口是否做了登录校验是否有权限校验是否校验了“当前用户只能取消自己的订单”如果用户传别人的订单号是不是也能取消这不是本次 diff 里能完全看到的内容因为可能依赖拦截器但我需要在审查意见里提醒开发者确认。这个如果是缺失那就是 T0。代码风格方面order.getStatus() ! OrderStatus.CREATED直接与枚举比较建议用枚举自身提供的方法或封装常量增加可读性。这是 T2 级问题。4.3 根据审查意见优化后的代码综合以上分析我会在 MR 评论里给出意见开发者修改之后的版本大概是这样的PostMapping(/order/cancel) Transactional(rollbackFor Exception.class) public Result cancelOrder(RequestParam Long orderId) { Order order orderMapper.selectByIdForUpdate(orderId); if (order null) { return Result.fail(订单不存在); } if (!order.ownedBy(loginUserId())) { return Result.fail(只能取消自己的订单); } if (!order.canCancel()) { return Result.fail(订单状态不允许取消); } order.cancel(); orderMapper.updateById(order); stockService.releaseStock(order.getId(), order.getProductId(), order.getQuantity()); return Result.success(); }这里几个关键的改动点是selectByIdForUpdate加行锁防止并发重复取消Transactional保证订单状态更新和库存释放的原子性ownedBy加了当前用户的归属校验canCancel把取消状态的判断封装到领域对象里。这样的代码明显更稳经得起推敲。这一套“发现问题、提出意见、修改代码、结果验证”的闭环就是 open-code-review 的完整形态。5. 常见问题与排查技巧实录5.1 PR 堆积没人审查怎么办这个现象在小团队里太常见了。开发者把 MR 提出来之后指派了审查人然后左等右等一整天都没人理。我见过很多团队因为审查延迟导致开发阻塞最后大家索性不按流程走了直接 push 到主干。从根子上说审查堆积的原因通常有两个一是没有约定响应时间二是审查者没有把审查当成自己工作的一部分。解决办法也很直接在团队规范里写明“MR 必须在 4 个工作小时内被首次回复”。这里的首次回复可以是审查意见、可以是确认稍后处理但不能不理会。另外从流程上建议在 MR 合并条件中开启“必须授权才能合入”强制推动审查动作落地。我自己还实践过一个方法每天固定两个时间段处理审查比如上午十点半和下午四点半。到了时间就把待审查的 MR 拉出来集中处理效率比随时被打断高很多。5.2 Review 意见掰扯不清最后变成吵架代码审查讨论到最后变成意气之争的情况我相信每个团队都遇到过。意见本身没有问题问题往往出在表达方式和判断标准上。先说表达方式。审查者提意见的时候建议用“三明治”句式先肯定代码的合理之处再指出具体问题和改进建议最后收尾表达整体认可或整体保留意见。这种表达方式不是为了让谁舒服而是让讨论聚焦在“问题本身”而不是“谁对谁错”。再说判断标准。代码审查中最浪费时间的讨论是“这个写法好还是那个写法好”这类没有标准答案的问题。要避免这种情况团队应该先把编码规范定好。比如复杂逻辑必须写清楚注释函数尽量控制在 50 行以内不允许重复的魔法数字禁止在循环里调用远程服务规范越明确审查意见越容易达成一致。遇到规范没有覆盖的问题审查者需要表明立场这是一个“建议”而不是“必须”让开发者根据实际情况决定。注意代码审查中最没价值的意见是纯粹个人偏好型的“如果是我会这么写”。除非有明确的理由说明这样写更好否则这类意见只会增加沟通成本不会提升代码质量。5.3 自动化跑过了人还要看什么有了 CI、静态检查、覆盖率之后有一种新的坑开发者觉得“CI 都过了审查也就是走个流程”。这是一个非常大的误解我强调过很多次。自动化能检查的是“确定性的规则”。比如代码格式、未使用的变量、安全漏洞的已知模式、测试覆盖率是否达标。但自动化无法判断的是“这个实现方案是否正确”“这个交互设计是否合理”“这个分支的取舍是否符合业务预期”“这段逻辑将来是否容易被误改”。换句话说自动化检查是“下游拦截”人工审查才是“上游控制”。我在实践中最深的一个体会是真正挽救过我的项目的往往不是“自动检查发现了一个空指针”而是“同事在审查的时候问我如果这个接口被频繁调用你这段逻辑成不成立”。这种从业务场景出发的质疑自动化永远做不了。所以我建议每个团队把下面这个事情做成硬性约束即使所有 CI 检查都通过每次 MR 依然需要至少一个人做完整的人工审查并在 MR 里留下评论说明“核心逻辑已经理解同意合入”。5.4 一份可以直接用的团队落地速查表最后把我认为最关键的实践要点整理成一份速查表可以直接作为团队文档的基础。主题推荐做法不推荐的做法MR 创建使用模板说明背景、改动、测试方式随手丢一段代码上来自动化检查本地钩子 CI 检查双层拦截只靠人工看格式分支保护开启禁止直推、必须 MR 合入谁都能改主干审查响应4 小时内首次回复拖到第二天甚至更久意见分级T0/T1/T2 分级反馈所有问题不分轻重混在一起审核人选择模块资深者 业务负责人随便找个人点通过合入方式合并需求后使用 squash merge保留大量无意义的 merge commit这套流程刚跑起来的一到两周团队会不太适应总觉得额外花了时间。但坚持一个月左右大家就会感受到明显的区别主干分支的质量明显提升上线后的问题明显减少新人也通过审查学到了不少东西。这也是我做 open-code-review 相关实践以来最大的成就感来源。
