mcpField
C
code-review

code-review

github.com/ouyangjiahong26Testing & quality579 installsUpdated 1d ago
Repository

About code-review

A read-only code review skill for examining changes against repository standards and the original task requirements. It defines how to select a comparison point, verify findings, and report the two review types separately.

Key features

  1. 01Select a fixed diff comparison point
  2. 02Locate task requirements and coding standards
  3. 03Review standards and task scope separately
  4. 04Verify findings against source changes
  5. 05Exclude specified files and categories
  6. 06Report concise, separate findings

Use cases

  1. 01Review a branch against its base
  2. 02Check changes against an issue or request
  3. 03Assess repository conventions and security concerns

Install

Copy the command and run it in a terminal.

$npx skills add https://github.com/ouyangjiahong26/skills --skill code-review

After installing, type/code-reviewin Claude Code to confirm it loaded.

Details

From the project README

对 HEAD 与固定点之间的 diff 分别做标准审查和任务要求审查:

  • 标准:代码是否遵守本仓库已记录的编码标准?
  • 任务要求:代码是否忠实实现了原始 issue 的要求?

审查是只读的:不编辑文件、不提交、不跑构建或测试。发现的问题报出来,修复交给调用方。

流程

1. 确定审查目标

按这个顺序从参数认定固定点:

  1. PR/issue 引用(#473、GitLab !67):按 docs/agents/issue-tracker.md 的流程拉取。PR 描述留给步骤 2 当任务要求,固定点取 PR 的 base 分支(origin/<base>)。
  2. 明确的 git 引用:commit SHA、分支名、tag、main、HEAD~5、区间(main...my-feature)。
  3. 什么都没给:用 @{upstream}(上游跟踪分支)。
  4. 认不出,或没有上游跟踪:问用户。

一次把 diff 命令都记下来:git diff <fixed-point>...HEAD(三点,比较的是 merge-base),再加 git diff HEAD,覆盖已跟踪文件的未提交改动,再用 git status --short 找未跟踪文件(它们整体都是新改动,直接读全文)。未提交的改动同样在审查范围内,除非用户明确只审已提交内容。同时记下 commit 列表:git log <fixed-point>..HEAD --oneline。

继续之前,先跑一次上面的 diff 命令(坏引用会直接以 unknown revision 报错),确认固定点能解析、审查范围内有改动(提交区间、工作区、未跟踪文件至少一处非空)。坏引用、空范围、PR 场景的分支不匹配,按边界情况表处理,在这里就停下,不要拖到审查中途。

2. 找任务要求来源

按这个顺序找原始任务要求:

  1. 步骤 1 拉到的 PR 描述:参数里给了 PR 引用时,它就是首选任务要求。
  2. commit message 里的 issue 引用(#123、Closes #45、GitLab !67 等):按 docs/agents/issue-tracker.md 的流程拉取。
  3. 用户作为参数传入的路径。
  4. 本会话中用户的原始请求:这批提交出自当前会话时,用户当初的指令就是任务要求。
  5. docs/、specs/、.scratch/ 下与分支名或功能匹配的任务要求文件。
  6. 都没找到,问用户任务要求在哪。用户说没有任务要求,则跳过任务要求审查,报告无任务要求可用。
3. 找标准来源

标准来源是仓库里所有规定代码该怎么写的文件,比如 CODING_STANDARDS.md 或 CONTRIBUTING.md。

在仓库声明之上,标准审查始终再加两条基线:异味基线与安全基线。即使仓库什么都没声明,它们也适用。两条规则决定这两组基线怎么用:

  • 仓库优先:已记录的仓库标准始终优先。仓库在某处明确认可了基线会标红的东西,就压住这条异味。
  • 永远是判断题:每条异味是一个带标签的启发式判断,不是硬性违反。

每条异味先弄清是什么,再看怎么修,然后逐条对照 diff:

  • 神秘命名:函数、变量或类型的名字没说清它做什么、装什么。修法:改名。想不出诚实的名字,是设计浑浊的信号。
  • 重复代码:同样的逻辑形状出现在改动的多个 hunk 或文件里。修法:把共同的形状抽出来,各处都调用它。
  • 依恋情结:一个方法更多地伸手到别的对象的数据里,而不是自己的。修法:把方法搬到它羡慕的那份数据上。
  • 数据泥团:同样的几个字段或参数总是黏在一起出现(一个呼之欲出的类型)。修法:合并成一个类型,传这个类型。
  • 基本类型偏执:用基本类型或字符串代替本该有自己类型的领域概念。修法:给概念一个自己的小类型。
  • 重复分支:对同一类型的 switch / if 阶梯在改动里反复出现。修法:用多态替换,或两边共享一张映射表。
  • 霰弹手术:一个逻辑改动逼你在 diff 的许多文件里分散地改。修法:把一起变的东西收进一个模块。
  • 发散式变化:一个文件或模块因几种互不相关的原因被改。修法:拆开,让每个模块只为一种原因变。
  • 投机式泛化:为任务要求没提的需求加了抽象、参数或钩子。修法:删掉,内联回去,直到真实需求出现。
  • 消息链:一长串 a.b().c().d() 导航,调用方本不该依赖。修法:把这段导航藏进第一个对象的一个方法里。
  • 中间人:一个类或函数主要只是转发。修法:删掉,直接调真正的目标。
  • 被拒的遗赠:子类或实现者把继承来的大部分东西忽略或重写。修法:放弃继承,改用组合。

安全基线按下面的检查点逐项对照 diff:

  • 注入:用户输入不经过参数化,直接拼进 SQL、命令或路径。
  • 跨站脚本:未转义的用户输入进入 HTML 或 JS 上下文。
  • 认证与授权:新增端点缺权限检查,或信任客户端传来的身份字段。
  • 敏感信息:密钥、令牌、个人信息进入日志、错误信息或测试夹具。
  • 危险求值:eval、反序列化、动态加载作用于不可信输入。
4. 依次做标准审查和任务要求审查

先做标准审查,再做任务要求审查。每一项审查都先审后验,两步走完才算完成该项审查的报告。标准审查和任务要求审查的发现互不合并、互不重排。

分发:改动跨多个文件或模块时,按同一目录或同一模块分组,派子代理并行审,每组一个子代理。子代理的指令里直接贴该组的 diff(别让每个子代理重跑 git diff),并附上该组的审查任务说明和下面的丢弃标准。小改动自己审。子代理只负责读和报,不做修改。

审:按任务说明过一遍 diff,记下草稿发现。先划掉不审的部分(生成代码、*.lock、内嵌第三方代码、纯测试替身,以及仓库声明跳过或工具已经在管的类别:lint、格式、类型检查),在报告里记一行说明这些部分被排除,不对它们产生发现。

  • 标准审查的任务:对照标准源文件(步骤 3)和上文的两条基线,按文件或 hunk 记录。(a) diff 违反了哪条已记录的标准,引用标准(文件 + 那条规则)。(b) 基线异味或安全检查点的命中,点名并引用对应 hunk。已记录标准的违反可以是硬性的,两条基线按步骤 3 的两条规则处理。
  • 任务要求审查的任务:对照任务要求(路径或拉取到的内容)记录。(a) 任务要求提出、但在 diff 中缺失或只做了一半的需求。(b) diff 里有、但任务要求没提的行为(范围蔓延)。(c) 看似已实现、但实现方式看起来不对的需求。每条配任务要求的对应行。

diff 引入的类型、枚举成员、事件、消息、命令、队列项这类跨函数或模块接口传递的值,逐个读接收端的分发点(switch、路由、过滤器、处理器注册表、循环体),确认它由显式分支或已有的兜底分支正确接收。静默丢弃、无操作、return 了事都算发现。分发点常常在 diff 之外,读完接收端之前,不要断定生产端没问题。

验:对着 diff 逐条复核草稿发现:

  • 把引用的事实核对回去:hunk 里真有那行,标准文件真有那条规则,任务要求真有那句要求。
  • 站在被审代码的立场反驳一次,命中任一条就丢弃:影响不可证(说不出触发条件,也指不出受影响的调用方)。本次改动之前就存在。看着是作者有意的设计选择。需要本仓库别处都不具备的严格度才成立。只是改进式的模糊建议,没有离散的修法。
  • 反驳不动、引用坐实的,保留原判,不降级成可能。

丢弃的发现不进报告,也不要提曾经发现又丢弃过。风格、命名、文档偏好本身不产生发现,仓库明文规定要审的除外。验证后的标准报告和任务要求报告各 400 字以内。

5. 汇总

把标准报告和任务要求报告分别放在 ## 标准 和 ## 任务要求 标题下,逐字或略加清理。不要合并或重排发现项,两个报告故意分开(原因见“为什么分开报告”)。

末尾一行总结:标准和任务要求各几条发现、各自最坏的问题(如有)是什么。不要在标准和任务要求之间选出单一的最坏,合并排序正是分开报告要避免的。

范围里有未提交或未跟踪改动时,在报告开头单列一行写明哪些文件不在提交区间内。

边界情况

情况处理方式
固定点认不出,或既没有参数也没有 @{upstream}向用户询问固定点,拿到后从步骤 1 继续
审查范围内没有任何改动(提交区间、工作区、未跟踪文件都为空)停下,告诉用户无改动可审,不做标准和任务要求审查
参数是 PR 引用,但当前分支不是该 PR 的 head提示先检出 PR 分支(如 gh pr checkout <N>)再重跑。用户坚持就地审,改按该 PR 的 merge commit 对比并说明局限
PR/issue 拉取失败报出失败原因,问用户:贴任务要求原文,还是跳过 tracker 拉取、从步骤 2 第 3 条继续找
docs/agents/issue-tracker.md 不存在让用户跑 /setup-ouyangjiahong-skills。用户不想配,跳过所有 tracker 拉取,从步骤 2 第 3 条继续找
仓库没有任何标准声明文件标准审查照常进行,只用两条基线
改动全落在排除范围里(全是生成代码 / 锁文件 / 内嵌第三方代码)不做标准和任务要求审查,告诉用户改动的全部文件都被排除,并列出是哪些
子代理无法派出或超时自己审全部。报告里写明哪些部分没覆盖到,不要默认声称覆盖完整

为什么分开报告

一个改动可以在一项审查里通过,在另一项里失败:

  • 代码遵守每条标准,但实现的内容是错的:标准审查通过,任务要求审查失败。
  • 代码恰好做了 issue 要求的事,但破坏了项目约定:任务要求审查通过,标准审查失败。

分开报告,防止一项审查的结论遮住另一项。

Comments

Markdown supported