代码审查是一门手艺
代码审查是一门手艺
目标读者:希望提升软件开发能力的开发者。这篇文章最初是写给初级开发者的,但其中有些内容对技术负责人等同样适用。所以如果读起来感觉有些杂乱,敬请见谅,借用我内心那位 Pascal 的话:"抱歉,我实在没时间把它拆成两篇。"
在开发者社区里,关于代码审查的讨论一直很多,尤其是 2025-2026 年这段时间。例如下面这些说法(不一定都来自同一些人):
- "代码审查是瓶颈"
- "强制合并前代码审查适用于低信任环境;你应该直接推送到 main 分支"
- "代码审查查不出 bug"
- "代码审查不是用来做 X 的,而是用来做 Y 的"
- "LLM 在代码审查方面比人类更强"
- "LLM 代码审查在发现边界情况和 bug 方面远胜人类"
- "你不要再看代码了;你应该做 XYZ"
等等,诸如此类。
在这些讨论中,单独把"代码审查不是用来做 X 的,而是用来做 Y 的"拿出来看,相关研究给出的观点如下:
通过对访谈数据进行编码,我们识别出 Google 开发者对代码审查的四个核心期待:教育、维持规范、把关和预防事故。教育指在代码审查中教或学,这与最初引入代码审查的初衷一致;规范指组织对某种自由选择的偏好(例如格式或 API 使用模式);把关涉及围绕源代码、设计选择或其他产物建立和维护边界;事故则指引入 bug、缺陷或其他质量问题。
同样,《现代代码评审的期望、成果与挑战》(2013)一文指出:
我们的研究表明,虽然发现缺陷仍是代码评审的主要动机,但评审中关于缺陷的内容比预期要少,反而提供了诸多额外收益,例如知识传递、增进团队对项目的了解、以及针对问题提出替代方案。此外,我们发现理解代码和变更才是代码评审的关键所在,而开发人员会运用多种手段来满足理解需求,其中大部分需求是现有工具所无法满足的。
所以至少我们可以达成共识:代码评审有诸多目的。
其他要点我稍后会谈到。
不过在此之前,我想阐述一个我在其他地方很少见到的观点:代码评审是一项技能。具体而言,我主张:
- 代码评审的能力是可以提升的。所谓"更好",是指在上述所有维度上都得到改进:发现 bug、发现设计问题、增进对项目进展的了解,以及深入理解代码。
- 可以通过教导帮助他人提升代码评审能力。
- 由于这是一项相对现代的技能,我们尚不清楚人类在这方面的能力天花板在哪里(例如,速度与质量之间的帕累托前沿在哪里)。
- 如果你是一名软件开发者,并且相信在可预见的未来人类仍将持续参与程序的开发与维护,那么提升代码评审能力就是有价值的。
首先,我打算举三个近几周工作中在代码评审时发现 bug 的小例子。我特意选择 bug 来讨论,因为它们相对而言不存在歧义。
接下来,我会先讲讲自己和代码审查相关的背景,并给出一些支持核心论点的理由。
然后,我会讨论一些关于如何实践和改进代码审查的想法。
最后,我会讨论前面提到的那些被反复传播的关于代码审查的梗,看看它们在核心论点下是否经得起推敲。
我们开始吧。
三个差点引入的 Bug
为了便于理解,下面的例子省略了不少细节。 如果读到一半冒出诸如"这看起来就是个代码异味,怪不得差点出 bug" 或者"用 XXX 就能避免"这样的想法,不妨留意一下。
三个案例中有两个,写 PR 的人对相关代码是熟悉的。
另外值得一提的是,下面所有 PR 都用目前(2026 年 6 月左右)较为先进的几款编码模型跑过 LLM 审查。 它们都没有发现我发现的问题。
参考 Lorin Hochstein 这篇《传统视角与韧性工程视角》里的表格可能会很有帮助:这篇文章很短,值得一读。下面的表格是原文表格的精简版。
| 传统视角关注 | 韧性工程视角关注 |
|---|---|
| 目标 | 生产压力 |
| 降低复杂度 | 驾驭复杂度 |
| 根本原因 | 多因素交互 |
| 把人当隐患 | 把人当资源 |
阅读下面的案例时,一个思路是试着同时从表格的两侧去思考。
写一段 git 配置
我们在 $WORK 使用自己的开发机用于软件开发的临时虚拟机。
,这些机器跑在 EC2 实例上。
启动流程里有两条子进程:
一个后台进程,负责初始化不需要立刻就绪的状态。 到用户开始使用开发机的时候,这个进程可能跑完了,也可能还没跑完。
一个前台进程,需要从用户笔记本获取一些额外数据,并在完成前阻塞用户。只有这个进程结束后,用户才能开始使用 devbox。
为了降低延迟,我们一直在努力把更多操作搬到后台进程。本着这个思路,我的一位同事提交了一个 PR,把对全局 ~/.gitconfig 的部分修改
为了性能和一致性,我们想统一管理用户 Git 配置的某些方面。是的,我知道 Nix 这个东西,我自己的一台服务器就在用。但我们在工作中不用 Nix。你能管住自己跑偏的念头吗?
从前台进程移到了后台进程。
这样,当 git config 命令需要修改 ~/.gitconfig 时,会先在 ~/.gitconfig.lock 上获取一个独占的文件锁。这可以防止来自其他(协作中的)进程的并发修改,比如其他 git config 调用。
看到那个 PR 的时候,我想起 devbox 启动过程中我们曾遇到过非确定性问题——Git 在获取锁失败时直接快速失败的策略,恰好被类似的并发写入问题触发,导致启动时不稳定。
如果只是在上面套一层重试,仍然会引入非确定性,所以同事利用现有的机制加了一条跨进程的依赖边,让写入操作只在后台进程完成之后才发生。
随后我指出,我们其实很早就试过这个方案,但几乎立刻就放弃了,因为端到端延迟变长了(前台进程的某个子步骤必须等后台进程整个跑完)。
最后,由于还有一些文件修改没法直接走 git config,
因为需要协调 # DO NOT EDIT 这类区块。
我们最终采用了单独的 flock 方案(配一个独立的 .lock 文件)。这样做的好处是:(1)可以用退避策略进行重试;(2)在同一个 flock 下完成多项修改,期间不会有其他写入介入;(3)直接写入时不必担心并发写入的问题。
要不要显示进度
有一个定期运行的 CI 任务,会做些处理然后把 tar 包上传到 AWS S3 桶。
结果发现 aws 命令行工具默认会显示进度。
大概是为了方便调试问题,以及在终端里直接使用 CLI 时让人对进度心里有数。
同事把上传逻辑从 1 个桶改成了 4 个桶,想加速从其他区域的下载。AWS 桶是绑定到特定区域的。
改完之后 CI 任务就开始失败了,因为日志文件超过了 10MB 的限制。
原来 aws CLI 每上传 256 KB 就会输出一行日志。
当上传量达到 10GB 以上时,这意味着几万行日志。
再乘以 4 个桶,日志量就直接冲破了 10MB 的上限。
同事提了个 PR,把调用改成加 --no-progress 参数来修 CI 任务。
当时我第一反应是:「嗯,这是唯一的选择吗?
能不能少打点进度信息?
要是上传中途失败了,有进度信息能让人更清楚地知道在哪一步挂掉的。」
于是我去查了 aws CLI 的文档,看到有个 --progress-seconds <INT> 参数可以调整进度输出的频率。
我问 PR 作者能不能改用这个。
我隐约记得之前出过一个小事故:有个常用脚本引入了一个 aws CLI 参数,但脚本运行的部分环境里 CLI 版本太旧,不支持那个参数,结果脚本就崩了一堆人。
既然 PR 作者也经历过那次事故,
我以为他会做好功课,检查一下 CI 任务里跑的是什么 CLI 版本,确认一下那个版本支不支持 --progress-seconds。
PR 作者很快回复了我,把 PR 改成了去掉 --no-progress,换成 --progress-seconds。
这个速度让我有点意外,于是我意识到他们可能没考虑到同样的风险。我先试着去翻 AWS CLI 的更新日志,看看有没有说明这个参数是从哪个版本引入的(因为参数文档里没写)。结果没查到。然后我让大模型帮我追查 CI 任务用的 CLI 版本,以及这个参数对应的提交和首次发布的版本号。这才发现,如果用了 --progress-seconds,任务肯定会挂,因为任务里装的 CLI 版本太老了。
我对几个提交和版本做了下抽样验证,然后在 PR 上留了条评论,简短地道了个歉——没早点说清楚我的假设——请他们核实一下结论,或者另找方案。
最后这个 PR 的修法是装一个新一点的 aws CLI 版本,刚好仓库里另一个地方已经在用现成的包。
差点引发故障的多余 SHA
还记得前面那个 CI 任务吗?我最近一直在做的事,是把这组任务的发布流程和整个 CI 发布流程解耦。这个 CI 系统相当复杂,不同的任务可以走各自的发布节奏。
有一天,我把自己的 WIP 改动 rebase 到 master 上,撞上了合并冲突。原来有人给现有的任务加了几个新变体。
这组任务负责上传一些 tarball。有位工程师需要再上传几个稍微不同处理方式的 tarball,于是单独开了个新任务来做,而没有去改这组任务里已有的那一个。
我第一反应是:"嗯?为什么单开一个任务,不就是在某个列表里多加一项的事吗?"
后来才知道,那个加任务的 PR 的 reviewer 当时就建议新旧任务拆开做,是为了降低影响原有任务性能和稳定性的风险。另外,那几个所谓"小"区别里,有一条是新任务会顺带上传带校验和的 sidecar 文件。读到这里你可能在想:"S3 不是本身就支持校验和吗?"——我也是这事全部收尾之后才知道,原 PR 作者估计当时也不知道。
看到那行代码时我有点困惑。"既然这个任务需要校验和,那其他现有任务是不是也需要?如果是的话,为什么不把校验和逻辑统一应用到所有任务上?我们是否需要更新现有读取器来校验旧 tarball 的完整性?这些新的 tarball 是否跨越了某种信任边界,而旧的则没有?"
多想了一会儿之后,我意识到这里有个 bug。大约一年前,我看过 Hannes Mühleisen 教授的一个演讲,主题是 DuckLake —— 面向普通用户的 SQL 湖仓格式。演讲里讲到了 Apache Iceberg 以及它对文件的使用方式,听起来非常复杂。
当时我的理解是,这种复杂性的部分原因在于本质上没有办法做多对象事务。因此 DuckLake 的一个关键设计决策就是用一个支持 ACID 事务的 SQL 数据库来维护元数据。
回到那个 CI 任务上,它把 tarball 上传到了一个固定 bucket 下的固定对象名。校验和是在上传之前算的。所以,虽然 S3 对单次对象写入能保证 all-or-nothing 语义,但如果任务在上传校验和之前就被取消或崩溃了,那么读取器在以 fail-closed 的方式用旧校验和校验 tarball 完整性时校验和的常规用法。
就会开始失败,导致该功能出现近似宕机的状态,直到 tarball 和校验和重新同步为止。
我把这个故障场景指给了创建 PR 的人,以及引入读取路径的那个 PR 的作者。最终,sidecar 文件的读取路径校验并没有被引入。
为什么代码审查是一项技能
让我们把时钟倒回到更早一些的时候。2018 年。
我那时候还是个物理学研究生,做的是各种模拟相关的研究。我们大量使用 Jupyter 笔记本。第一天模拟还能跑,第二天就崩了,第三天把第二天的改动撤销了,它还是继续崩。
我当时完全不知道该怎么管理代码,怎么提升效率,怎么避免错误,也不知道怎么用版本控制。后来我开始接触到自动化测试和代码审查,顿时觉得打开了新世界的大门:"等等,什么?原来大家真的会逐行审查代码,而不只是看输出结果?!当软件工程师一定很幸福吧。"
慢慢地,我对如何更好地写软件越来越感兴趣,对原本要研究的物理反而越来越提不起劲。2019 年我退出了博士项目,转行做起了软件工程师。
从那至今,职业生涯中已经不止一次有人跟我说,他们觉得我的代码审查比平均水平更有帮助。
举一个数据点:在 2026 年之前,我审查 PR 时,评论密度大约在每 30-100 行代码一条评论的范围内,具体取决于作者是否熟悉我的审查风格、代码背景以及之前的讨论等等。这些评论里大部分并不是在挑 bug,更多是在问一些相当"平淡"的问题,比如澄清疑问、命名、代码分层之类的。
所以我为什么要跟你说这些呢?我想表达的是,代码审查这项技能我也是慢慢学会的,并不是天生就会,也不是天生就能在审查中发现 bug。
要说个人特质上有没有什么特别之处,确实可能有。我能想到的几点:
- 当发现是自己之前引入的 bug 时,我往往会比大多数开发者更"较真"。
- 我习惯用不变量和小证明的思路来思考程序。
- 我喜欢读技术博客、听技术演讲,尤其喜欢那些讲调试故事和性能排查的内容。
这几点里,我认为第二点尤其可以通过学习来掌握。
退一步看,我觉得关于代码评审,我们其实有很多不了解的地方。翻翻相关文献,内容通常相当贫乏。
除了开展更结构化的研究——这在典型的企业环境中很难获得支持——我认为在代码评审相关的新实践上还有很大的探索空间,包括小规模试验,让评审对作者、评审者以及整个团队都更有价值。
戴上疯狂科学家的帽子
在这一节,我想提出一些改进代码评审实践(或者至少尝试其变体)的潜在想法。写下来的目的有两个:
- 给你一些启发,激发你自己的创意。
- 让你感受到现有实践在哪些方面有所欠缺。
重点不是“你应该去试试这些”(也许你不该)或“我确信这些会奏效”(我不确定,因为我还没机会实践过)。
下面的想法假设已有坚实的心理安全感基础,因为如果没有这个基础,你几乎肯定应该先解决这个问题,再去尝试更奇怪的东西。
随机化的过程导向式苏格拉底对话
假设你有一位初级工程师,正在向一位对相关代码有专业知识的资深工程师请求评审。
当 PR 提交并请求评审时,假设一个机器人根据可调频率随机决定创建一个会议。
在会议中,资深人士不会直接指出不合理之处或可改进的地方,而是先询问初级成员的观点,了解他们为何采取某种做法,或基于哪些假设。关键在于避免提出反事实问题,比如“为什么没有用另一种方式做”或“为什么没想到这一点”。这听起来简单,但实际操作时你会发现很难坚持!(编辑:如果你好奇“为何要避免反事实问题”,可以参考我之前的文章《如何从我的错误中学习?》以及Lorin Hochstein的《反事实问题的困境》。)
在这个过程中,思维敏捷的初级成员会意识到自己理解上的不足,并可能发现可以改进的地方。
重点应放在不同子技能的思考过程上,比如撰写清晰的PR描述或稳健的错误处理,以PR为切入点来引导讨论。
另见:有效反馈的六项原则,第1节:有效反馈基于过程而非结果。
轻量级近失事件复盘
如今,对事故进行复盘越来越普遍。同样,将工作组织为冲刺并在冲刺结束时召开团队回顾会议也很常见。这也是一项可以学习、提升的技能!
如果在冲刺期间,代码审查中每次发现错误时,PR作者都被要求录制一段短视频,解释背景和发现的问题,并说明该问题是否与过去的问题类似,以及过去问题的影响,会怎样?这里的措辞是经过深思熟虑的。我特意不提“如果问题漏掉可能产生什么潜在影响”,因为这属于推测或预测的范畴。
在团队回顾会议上,团队可以一起观看这些视频,提出问题,增进共同知识。这样,团队中的每个人也能接触到更多差点漏掉的问题。
为了避免频率上的差异,你可以限制每人每次会议最多一个片段。
另见:你错过了那些险些发生的事。
防火墙式建模
在不运行甚至不看代码就发现bug(视频)(StrangeLoop 2019)中,Jay Parlar描述了一个例子:他试图为某个访问控制系统在Alloy中建立模型——该系统跨越多个代码库,而他并未查看代码——在建模过程中,他发现了bug和设计问题。
以我有限的经验来看,轻量级形式化方法在推理流程语义(涉及取消、可能无限期的等待、文件锁定协议)以及访问控制时似乎很有用。
如果作为基线,当你开始处理一个系统时,尤其是那些与多个外部系统交互或承载高安全或正确性风险的系统,由两人分别扮演建模者和程序员角色会怎样?建模者在不看代码的情况下开发模型,程序员则专注于代码。他们在中间会合,共同创建测试用例。
这样,模型就能作为一个更小的参考,用于审查代码的完整性,并测试各种不寻常的情况,否则在项目早期阶段需要大量测试脚手架才能实现。
研究专长
(好吧,这个不太符合“疯狂科学家”的主题,但这是我的博客。)
在许多领域,关于专长的文献越来越多。但在代码(尤其是代码审查)领域,这种研究并不以同样的方式存在。
如果你分析团队中发生的代码审查,并试图找出在洞察性审查评论方面存在异常(领域,人员)组合的情况会怎样?或者,你也可以通过访谈来获取这些信息。
一旦你找到了这些“异类”,可以尝试一种易于学习的方法,比如应用认知任务分析,来提取他们身上的隐性知识。相较于许多其他任务,在代码审查中这样做更为简便,因为无需模拟难以重现的情境(如救火现场),你只需让这个人现场审查PR即可。关于代码审查的普遍看法
希望至此,我已在一定程度上说服你:代码审查是一项技能,且我们仍有大量空间去尝试不同的实践方式。例如,在培训体系方面,我们远未达到像团队运动(如足球)和双人博弈(如国际象棋、围棋)那样成熟的研究水平。
此刻,你或许(合理地)仍在想:“那又怎样?LLM在这些方面进步的速度远超人力的提升,试图在代码审查上与LLM竞争毫无意义。”另一个相关的质疑可能是,尽管你在意代码审查,但你的管理层并不在意。抱歉,关于如何说服管理层,我并无良策。
如果你还记得我之前的表述:
如果你是一名软件开发者,并且相信在可预见的未来,人类仍将参与程序的开发与维护,那么提升代码审查能力就是有价值的。
这句话的后半部分以前提为条件。因此,我们可以对前提本身(“如果你相信……在可预见的未来”)成立的可能性持不同看法,但这与结论(“那么”)是否成立是两回事。
将这一推论反过来,大致会是:
如果你是一名软件开发者,即使相信在可预见的未来,人类仍将参与程序的开发与维护,提升代码审查能力也是净负面的。
例如,你可能因为认为还有其他技能能带来显著更高的杠杆效应而持此观点。我常见到“系统设计”、“产品思维”或“培养品味”被推崇为更值得发展的方向。
对此,我有以下几点回应。
首先,如果你能比同行更深入地理解底层抽象,这几乎总是一种优势,因为你能解决他们无法解决的更广泛问题。例如,如今,如果你能更轻松地理解SQL查询计划,或者了解内存分配和汇编等知识,你在设计更健壮、高性能的代码时,就会比那些不懂这些主题的工程师更有优势。
其次,鉴于软件开发与其他众多领域相比仍处于起步阶段,我们很可能远未触及人类在各个方面技能和表现的天花板,代码审查也不例外。因此,你周围所见到的平均水平很可能远未达到上限。
最后,我建议你以经验报告、案例研究和你自己的观察为基础来构建世界观,而不是依赖社交媒体上的“观点”。Cedric Chin 在《如何理解AI》和《给担忧AI的年轻人的一封信》中对此的阐述比我所能表达的更为透彻。
归根结底,如果你要对自己发布的代码负责,并且这些代码对真实用户产生实际影响,那么思考如何提升自己的技能就至关重要。为此,我相信投资于提高代码审查能力是作为一名软件开发人员能做的最有价值的事情之一。
夜叉:什么比风更快?
尤帝士提尔:心念。
夜叉:什么比草叶更繁多?
尤帝士提尔:心中的思绪。
夜叉:什么是最值得称赞的?
尤帝士提尔:技艺。
夜叉:什么是最宝贵的财富?
尤帝士提尔:知识。