尧图网站设计 尧图网站设计YAOTU DESIGN
ARTICLE DETAIL

资讯详情

深耕网站设计与一线实操的经验洞察。

open-code-review:从流程到工具的代码评审最佳实践

open-code-review:从流程到工具的代码评审最佳实践 我在两年前把团队内部的代码评审机制重新整理了一遍仓库名就叫open-code-review。这个名字起得很直白目标是想让代码评审从“两个人关起门来看一眼”变成“所有人都能看见、都能评论、事后还能复盘”的开放过程。当时团队正处在从八个人扩张到三十个人的阶段PR 越来越多光靠组长逐个 review 已经明显跟不上了。试行open-code-review之后评审平均耗时从 3.2 天降到了 1.1 天更重要的是新人通过看别人 MR 里的争论学习效率比读文档高了不少。如果你也在带团队或者你正在维护一个多人协作的开源代码仓库只是想知道“什么样的评审流程才不算形式主义”这篇文章值得往下看。它不依赖某个特定工具GitLab、GitHub、Gitea 都能落地核心是一套可执行、可度量的规则。1. 从我踩过的坑说起为什么需要 open-code-review1.1 封闭式评审的问题并不在“人”在于流程先说说之前的评审长什么样开发者提 MR在群里 两个人“帮忙看下。”被 的两个人点开 MR看到一大坨代码不知道从哪看起索性先标记为已读。如果真的看了也很少在代码行上评论更多是私聊作者“这里好像有点问题。”合并之后评审记录消失在聊天记录里。这种模式把信任建立在工作记忆上问题就出在这里。团队十个人的时候每个人写什么模块组长心里有数团队三十个人的时候光靠记忆根本转录不过来。私聊里的一句“这里好像有问题”没有沉淀在 MR 上就会造成两个麻烦作者修完以后别人不知道他为什么改后来人看这段代码也看不到当初的取舍。还有更隐蔽的问题只有被 的人有资格评论其他人想说话会被默认成“多管闲事”。这会让一些真正了解公共模块的老同事选择闭嘴。一次我印象特别深一个后端同事私下跟我讲他在某次评审里发现了一个数据竞争问题但碍于“不是被指定的人”一直没好意思开口直到测试环境出了故障才暴露出来。这种事情反复出现之后我开始认真思考问题可能不在人的积极性而在评审机制本身就是封闭的。1.2 open-code-review 的四个核心词我理解的open-code-review不是简单把权限放开、谁都能评论而是四个关键词组成的一套规则公开、结构化、可度量、有闭环。公开仓库内所有成员都能查看 MR、都能在代码行上评论。评论不需要被分配也不需要经过组长同意。只有公开的讨论才能形成知识沉淀私聊里的评审意见本质上是一次性消耗品。结构化每条评论必须能归到某类问题逻辑错误、安全问题、性能隐患、代码风格、测试遗漏。没有结构的评审最后会变成“我觉得这里的代码不太优雅”这种模糊对话。可度量评审时长、第一次评论时间、阻塞性评论数量、参与评审人数这些都应该有数据记录。没有数据团队就无法判断评审到底卡在哪一环。有闭环每一条评论都要有明确的结果修改、确认忽略、转移到待办。所有阻塞性问题都解决了代码才能合并。不能一边在评论里吵着一边有人手动点了“合并”。这四个词合在一起才是open-code-review的核心。它解决的不是“谁来看”而是“看的过程有没有留下可追溯的、对团队有用的信息”。2. 把 open-code-review 落地成一套流程2.1 从提交到合入open-code-review 的六步流程不要一上来就整很复杂的流程我推荐从最基础的六步开始。以下表格是我在团队里实际使用的流程版本你可以直接复制过去改。阶段关键动作时限负责人提交前本地跑 lint 和单测写清楚 MR 描述无作者发布创建草稿 MR标注改动模块和风险点无作者分配机器人或维护者至少指派 1 名评审人发布后 2 小时内维护者评审评审人在行级评论必须标注评论级别24 小时内首次评论评审人修改作者解决所有 Blocking 评论并回复48 小时内作者合入CI 全绿、至少 1 个 Approve、无未解决 Blocking无维护者每个环节背后都有一个设计意图不是随便定的。提交前跑 lint 和单测是为了把“代码能不能跑”这件事从评审中彻底拿掉。评审人的精力应该花在“这么做对不对”“以后好不好维护”上而不是纠正缩进和拼写错误。创建草稿 MR 的意义在于让评审人知道什么时候可以开始看。很多团队的问题是代码刚 push 上来还在改评审人就被 到了结果他评论完之后作者又推了一版完全不同的。草稿 MR 是一个很好的信号这里还没准备好你可以先围观但请不要正式评审。分配评审人这一步尤其重要。我见过太多团队用“在群里喊一嗓子”来代替指派最后没人觉得自己有责任看。必须有一个确定的负责人谁没参与一眼就能看出来。2.2 角色定义谁必须评、谁可以评、谁来兜底open-code-review 强调开放但开放不等于没有责任人。我在流程里定义了四种角色作者负责提交代码、回复评论、在 MR 描述里写清楚背景。评审人被正式指派的人有验收责任。他的approve是合入的硬性条件之一。维护者拥有合并权限的人通常是技术负责人或模块 owner。他负责兜底处理评审人之间的分歧。围观者任何能看到仓库的人都可以评论但评论不进入验收计数。这里有个细节值得注意围观者的评论是否要强制解决我的答案是不需要。如果强制要求作者必须解决所有围观者的意见那评论区会变得越来越冷清因为大家怕给作者添麻烦。正确的做法是把围观者的评论当成“提问”作者可以选择回答“这不会发生原因是 XXX”并关闭没有人会因为关闭了评论就被记一笔。这个设计的优点是既保住了开放的评论入口又不会让作者陷入无休止的争论。我记得有一次一个来自 QA 的同事在某个前端 MR 里提了一个关于边界状态的疑问虽然不是评审人但他看得非常准。作者解释了自己的方案顺便把边界情况加了一个测试。如果我们的流程不允许围观者评论这个测试大概率就不会出现。2.3 评审检查单不要只凭感觉给意见很多评审人看完一个几百行的 MR 之后只会写“LGTM”Looks Good To Me不是因为他真的觉得没问题而是他不知道自己该看什么。解决这个问题最好的办法就是给评审人一张明确的检查单。以下是我整理的一份精简版每条都尽量是“可验证”的而不是“请检查代码质量”这种废话。维度检查项判断方法正确性边界条件有没有处理看空值、越界、初始状态、并发场景安全性外部输入是否校验看用户输入、文件上传、权限判断性能有没有明显 N1 查询或循环内调用看数据库查询和数据量变化可测试性新逻辑有没有对应测试搜索关联 test 文件可维护性命名是否能直接看出意图不看实现只看函数名和变量名可扩展性后续需求变更时这段代码需要推倒重来吗只做合理推测这里不要过度设计检查单不需要太长十二到十五条顶天了。太长会让评审人产生“完成检查单”的心理反而把真正的问题漏掉。我在团队里把这份检查单放在 MR 模板中并在 CI 里生成一个“评审回执”让评审人在批复 approve 之前勾一遍。老实说一开始很多人嫌麻烦但坚持一个月之后大家对于“什么叫完成了评审”这件事有了共同的预期。后来把检查单本身也开源到了团队公共仓库里形成了open-code-review项目的一部分。3. 用工具把评审规则固化成“硬约束”3.1 提交前用 Git 钩子和 commit 规范挡掉低级问题流程是软的工具才是硬的。如果你想在没有代码托管平台插件的情况下尽量把流程固定下来Git 钩子是最低成本的方案。我们在仓库根目录维护了一份.git/hooks/commit-msg脚本用来统一 commit message 格式。这个脚本很简单核心逻辑就是正则匹配#!/bin/bash # .git/hooks/commit-msg msg_file$1 if ! grep -qE ^(feat|fix|refactor|docs|test|chore)(\(.\))?: $msg_file; then echo commit message 不符合规范示例fix(user): 修复登录后闪退问题 2 exit 1 fi有人会觉得这很形式化但我实际体验下来commit message 统一之后很多自动化工具才能跑起来。比如我们之后的版本发布脚本就是靠git log --grep^feat来生成 changelog。如果每个人写的格式都不一样这个脚本就完全废了。除了 commit-msg我还会在pre-push钩子里跑一次快速 lint#!/bin/bash # .git/hooks/pre-push echo pre-push: 运行代码检查... npm run lint if [ $? -ne 0 ]; then echo lint 未通过禁止推送 2 exit 1 fi这里的关键点是钩子必须放在本地并且靠约定来同步。因为 Git 钩子不会随仓库自动克隆到开发者本地所以我会在package.json里加一个prepare脚本让开发者在npm install时自动安装 hooks。如果你用 Python也可以在pyproject.toml里配置类似动作。这样做的好处是即使团队里有人不主动跑测试提交时也会被迫过一遍检查大大减少了评审人需要处理的“低级问题”。我们的切身体会是引入这个机制之后评审压力减小了很多。3.2 评审中把 MR 描述和评论模板化如果你不想自己开发工具最简单的办法是在代码托管平台里配置 MR 描述模板。我们用的 GitLab在项目根目录创建.gitlab/merge_request_templates/default.md## 为什么改 !-- 写清楚这次需求或 bug 的背景以及为什么采用当前做法 -- ## 改动点 !-- 列出主要改动的文件和模块不要写 git diff 里能看到的东西 -- ## 自测结果 !-- 本地测试、单元测试、手动验证的结论 -- ## 需要评审者重点看什么 !-- 如果有不确定的设计决策请在这里详细说明 --模板的价值不是逼着作者填表而是迫使作者在提交代码之前把上下文梳理一遍。很多评审问题其实是上下文缺失导致的。评审人看到一段没头没尾的 diff本能反应就是“感觉不对”但说不清哪里不对。模板让作者先把自己的思路摆出来评审人就容易给出有针对性的反馈。评论模板同样重要。我们在团队里统一了评论前缀的规范所有行级评论都必须带下面三种标记之一[Blocking]必须修复才能合入。通常用于逻辑错误、安全漏洞、导致线上事故的问题。[Nit]非阻塞的小建议。可以忽略但建议作者看一眼。[Question]作者需要给出解释不一定改代码。这套机制看起来简单但它做了一件事把“作者和评审人之间的地位博弈”从评审中剥离了。以前大家不好意思指出问题是怕态度太强硬伤了和气现在有了前缀[Blocking]不是说“你代码写得差”而是“这里如果不改会有实际风险”。评论的对抗性大幅下降。我也要求作者在回复时用Acknowledged作为确认词。对于[Nit]类评论作者只要回复Acknowledged表示收到即可不需要额外声明“我不改”。这样可以避免每个 nit 都来回拉扯两轮。3.3 数据化用 API 统计评审指标周一例会不用拍脑袋规则有了工具也有了但还不够。我相信一句话没有数据支撑的流程改进都是靠感觉。所以open-code-review里还有一个核心模块是用代码托管平台的 API 拉取评审数据生成每周报告。下面是一个简化版的 Python 脚本用来统计一个 GitLab 项目中已合并 MR 的平均评审耗时和评论数。实际项目中我会加上多项目汇总、Excel 导出等功能但核心就是这样import requests from datetime import datetime, timezone TOKEN glpat-xxx BASE https://gitlab.example.com/api/v4 PROJECT_ID 42 headers {PRIVATE-TOKEN: TOKEN} mrs requests.get( f{BASE}/projects/{PROJECT_ID}/merge_requests, headersheaders, params{state: merged, per_page: 100}, ).json() total_duration 0 total_comments 0 count 0 for mr in mrs: created datetime.fromisoformat(mr[created_at].replace(Z, 00:00)) merged datetime.fromisoformat(mr[merged_at].replace(Z, 00:00)) # 评论数通过另一个接口获取这里简化成列表 notes requests.get( f{BASE}/projects/{PROJECT_ID}/merge_requests/{mr[iid]}/notes, headersheaders, ).json() duration_hours (merged - created).total_seconds() / 3600 total_duration duration_hours total_comments len(notes) count 1 if count 0: print(f本周合并 MR 数: {count}) print(f平均评审耗时: {total_duration / count:.1f} 小时) print(f平均评论数: {total_comments / count:.1f})这类数据在我们团队里主要用来发现流程瓶颈。比如平均评审耗时异常高可能是某个模块缺 owner或者某个人长期被指派却从来不参与。平均评论数如果低于 1说明大部分人只是在走审批流程根本没真的看代码。这里有个容易踩的坑API token 权限不要搞太大只需要read_api权限就够了。而且脚本不要用团队的公共 token最好用某个定时任务的专用机器人账号。我也见过有人把 token 直接 commit 到仓库这个风险极大强烈不推荐。另一个要注意的是单纯的“耗时”指标会被大 MR 拉高。所以我会额外看一条评审效率 总改动行数 / 评审时长虽然很粗糙但能帮团队发现超大 MR 对整个流程的拖累。后来我们把 MR 拆小限制单次改动尽量不超过 400 行评审效率和评论质量明显上升。4. 常见问题排查实录评审推不下去怎么办4.1 没人评论或者评论永远是 LGTM推行open-code-review初期最典型的问题是MR 挂了两天除了机器人没有任何人说话。后来有人终于点了 approve评论只有一个单词LGTM。我复盘时发现问题不在于同事敷衍而在于被指派的评审人不知道“该怎么评”。尤其是刚毕业的工程师让他 review 一个资深开发的 MR他压力很大既怕说错又怕说太多得罪人。最后只能回一个 LGTM 保平安。解决办法是拆成两步第一步给评审人非常具体的任务。在 MR 描述里作者必须写清楚“需要评审者重点看什么”。比如“重点看登录态过期后并发请求的处理是否正确”评审人就有了支点。第二步把“评论质量”放进周会。每周挑一条最有价值的[Blocking]评论展示它如何发现了一个潜在线上问题。这样大家会逐渐明白指出问题不是找茬而是在帮助团队避免事故。我们坚持了几个星期之后LGTM 的比例从 70% 降到了 20% 左右。4.2 评审变成“找茬现场”人情味没了开放评审到一定程度会遇到另一个反面问题评论区变成了辩论赛。两个人在一个缩进问题上来回争论十几次最后还要拉维护者来裁判。本质上是因为评论没有权重所有意见看起来都同等重要。解决思路是强化[Blocking]和[Nit]的分级。我明确规定只有[Blocking]才能阻止合入[Nit]和Question类评论作者可以选择不处理。这样一来风格、命名、行数这类主观问题都归入 nit根本吵不起来。如果有人非要用[Blocking]来提风格建议维护者会介入告诉他这不是 blocking 级别的问题。明确规则之后大概一个月评论区的语气就恢复正常了。大家会把精力放在真实性问题上而不是个人偏好上。4.3 跨时区异步评审上下文丢失怎么办团队分散在不同城市之后我遇到最大的问题不是没人评而是“看了跟没看一样”。评审人打开 MR看到 300 行 diff却不知道这是什么功能只能从代码里硬猜评论的质量自然上不去。这个问题的根源是上下文没有传达到位。异步评审不像线下黑板上可以指着说它必须靠作者把上下文写清楚。我们用的措施有两招。第一招要求作者把 MR 描述里的“为什么改”写到不少于三句话并且包括一个完整场景例子。比如共享组件库的 MR 中我会要求写出“现在登录后调用 getUserInfo如果 token 过期会抛 401本次改动让请求队列自动重试”这样一来评审人不需要先读完整业务代码也能理解改动意图。第二招把大 MR 拆小。这是我反复强调的超过 400 行的 MR评审人很难在异步对话中保持完整的上下文。拆成多个小 MR 之后每个 MR 的问题域聚焦评审人只需要理解一小块逻辑。异步评审的最大敌人是“上下文爆炸”而不是时差。4.4 合入门禁被绕过流程设计得再完善也会有人绕过。最常见的是维护者觉得“这个改动太简单了”或者“客户在等这个修复”于是手动点了合并跳过了门禁。一两次可以理解但形成习惯后评审流程就会变成废纸。我在 GitLab 里把主分支设置成“保护分支”在这个模式下不是所有人都能直接 push 和 merge。合入必须满足三个条件CI pipeline 通过至少 1 个指定评审人 approve所有[Blocking]评论已 resolve这三条是平台的硬约束不是靠自觉。如果有人真的需要绕过那必须经过维护者和技术负责人双重同意并且在 MR 里记录绕过原因。我个人经验是一旦把流程固化成平台规则成员反而不会觉得麻烦因为“规则不是我定的是系统强制要求的”这是心理上很微妙的一层。5. 把 open-code-review 变成团队知识沉淀5.1 将高频评审问题转化为自动化规则评审每个月都会产生几十条评论如果这些评论只停留在 MR 里价值就被浪费了。我习惯在每个迭代结束后把评审评论导出来做一次关键词聚合看看哪些问题反复出现。常见的重复问题有空指针没有判空、时间边界差了 1 秒、日志里没有打印关键请求 ID、异常被吞掉导致排障困难。这些问题每条都很小单独看不会影响产品但频繁出现会消耗大量评审精力。解决方式是把高频问题变成自动化检查。比如日志规范用一个简单的eslint规则或者grep脚本就能排查错误处理问题可以在 CI 里对指定目录做静态检查。当我们把open-code-review跑了一个季度之后线下的评审评论数量反而下降了因为一半问题都在 CI 阶段被挡掉了。评审终于可以把精力放在真正的架构问题上。5.2 用评审数据反哺个人绩效与技术分享评审数据还有一个容易被忽视的作用就是反哺团队建设。每隔两周我会拉一次评审统计数据但目的不是考核而是发现哪类技术讨论值得全员分享。举一个实际例子有几次评审里几个同事都在争论 Python 的可变默认参数问题然后各自用不同方式绕开了。这件事说明团队在这个知识点上存在系统性的认知缺口。我把典型 MR 里的讨论整理成一次 30 分钟的技术分享讲了为什么def foo(items[])会踩坑以及怎么在代码评审中快速识别这类问题。分享之后同类评论明显减少。open-code-review的价值也在这里它产生的不只是“代码能不能合并”的结论更是一份团队能力地图。通过分析评审中反复出现的问题你总能找到下一步要补的短板。5.3 一个值得抄作业的最小启动包如果让我给一个还完全没有评审流程的团队列最小启动包我会写这三行每个 MR 至少指派 1 名评审人评审人必须在一个工作日内留下第一条评论。MR 描述必须包含“为什么改”和“需要评审者重点看什么”模板直接套用 3.2 节。合入前必须完成所有[Blocking]评论这个用保护分支强制。这三条已经能覆盖 80% 的收益。我自己踩过最大的坑就是一上来把流程做得太大又是指标又是机器人结果团队成员一星期就放弃了一半。后来老老实实从最小闭环开始跑了三个月再把统计脚本和自动化规则加进去反而稳妥得多。如果你正在整理自己团队的评审流程建议把仓库名直接命名为open-code-review把模板、脚本、流程图都放进去。后续你甚至可以把它做成一个可复用项目让其他团队也能通过复制仓库快速启动。代码评审这件事本质上不是约束人的是用规则让信息流动起来让每个人都能看到、能提问、能改进。从最小的指派和模板开始比什么都重要。
返回列表