返回知识库

版本协作 · 版本协作

代码评审Code Review

他人审阅改动并给出意见的过程:逐处给通过、评论或拦截,拦截清零才能批准。

先认出钉在 + 行上的评论,再决定能不能批准

下面就是 Files changed 里的一处 hunk。默认拦截钉在拼接 SQL 上;作者没改时批准被挡住;改成参数化后,LGTM 写在同一条评论上。

第 0 步先打开:打开 PR 的 Files changed,点某一行左边的 +。下面就是钉在那一行上的评论。

server/projects.ts · L42Request changes
+ const sql = `SELECT * WHERE owner='${uid}'`;

不能批准拦截 · 字符串拼接 SQL,未登录可越权。

意见钉在这一行。权限问题必须先改,评论「看着改改」不够。

原因:这一行有注入和越权。下一步:等作者改,或先看没修就想批准会怎样。

知识点:每处 diff 给一个结论

评论条刚走过的路,这里只给命名。

  • 结论只有三种:通过、评论(风险 + 改法)、拦截(必须改)。
  • 注入、越权、密钥一律拦截,修复前不批准。
  • 评论要写清风险和建议,不说「改一下」。
  • 批准附 LGTM 或验收依据,并等 CI 重跑。

什么时候用、怎么用

改动要进他人会读的分支,就逐处给结论。

  • 信号:PR 打开,diff 里有权限、数据和边界改动。
  • 最短路径:读目标 → 逐 hunk 给结论 → 追拦截到清零 → 再批准。
  • 不适用:自己批自己、只看 CI 绿、用「认识作者」代替读 diff。

正反例:同一处 SQL,只改一条意见

只改变意见能不能执行,作者下一步完全不同。

正例拦截写清改法

未登录可拼 uid:越权 + 注入。改用参数化查询和 req.user.id,补越权用例。修复前不批。

反例「这里不太安全」

作者不知道改哪、改成什么。拦截没落地,PR 仍可能被别人点批准。

快速自测

评审一个改了登录接口的 PR,下面哪种最该拦下?

继续查证

术语的技术定义和行为以这些一手或权威资料为准。

下一步学

和本知识点经常一起出现的概念。