Skill Learning

code-review

1. Bilingual SKILL.md

左右滑动可直接切换英文版与中文版;两种语言按段落自动对齐。英文中的蓝色虚线词语可点击查看解释。共标记 23 处。

入口 skill:code-review左右滑动切换语言 · 段落位置自动对齐
Two-axis review of the diff between `HEAD` and a the user supplies:
- **Standards** — does the code conform to this repo's documented coding standards? - **Spec** — does the code faithfully implement the originating issue / spec?
Both axes run as **** so they don't , then this skill aggregates their findings.
The issue tracker should have been provided to you — run `/setup-matt-pocock-skills` if `docs/agents/issue-tracker.md` is missing.
## Process
### 1. Pin the
Whatever the user said is the — a commit SHA, branch name, tag, `main`, `HEAD~5`, etc. If they didn't specify one, ask for it.
Capture the diff command once: `git diff <fixed-point>...HEAD` (three-dot, so the ). Also note the list of commits via `git log <fixed-point>..HEAD --oneline`.
Before going further, confirm the resolves (`git rev-parse <fixed-point>`) and the diff is non-empty. A bad ref or empty diff should two .
### 2. Identify the spec source
Look for the , in this order:
1. Issue references in the commit messages (`#123`, `Closes #45`, GitLab `!67`, etc.) — fetch via the workflow in `docs/agents/issue-tracker.md`. 2. A path the user passed as an argument. 3. A spec file under `docs/`, `specs/`, or `.scratch/` matching the branch name or feature. 4. If nothing is found, ask the user where the spec is. If they say there isn't one, the **Spec** sub-agent will skip and report "no spec available".
### 3. Identify the standards sources
Anything in the repo that documents how code should be written, such as `CODING_STANDARDS.md` or `CONTRIBUTING.md`.
On top of whatever the repo documents, the Standards axis always carries the **** below — a fixed set of Fowler code smells (_Refactoring_, ch.3) that applies even when a repo documents nothing. Two rules bind it:
- **The repo overrides.** A documented repo standard always wins; where it endorses something the baseline would flag, . - **Always a .** Each smell is a labelled heuristic ("possible Feature Envy"), never a hard violation — and, like any standard here, skip anything tooling already enforces.
Each smell reads *what it is* → *how to fix*; match it against the diff:
- **Mysterious Name** — a function, variable, or type whose name doesn't reveal what it does or holds. → rename it; if no honest name comes, the design's murky. - **Duplicated Code** — the same logic shape appears in more than one hunk or file in the change. → extract the shared shape, call it from both. - **Feature Envy** — a method that reaches into another object's data more than its own. → move the method onto the data it envies. - **Data Clumps** — the same few fields or params keep travelling together (a type wanting to be born). → bundle them into one type, pass that. - **Primitive Obsession** — a primitive or string standing in for a domain concept that deserves its own type. → give the concept its own small type. - **Repeated Switches** — the same `switch`/`if`-cascade on the same type recurs across the change. → replace with polymorphism, or one map both sites share. - **Shotgun Surgery** — one logical change forces scattered edits across many files in the diff. → gather what changes together into one module. - **Divergent Change** — one file or module is edited for several unrelated reasons. → split so each module changes for one reason. - **Speculative Generality** — abstraction, parameters, or hooks added for needs the spec doesn't have. → delete it; inline back until a real need shows. - **Message Chains** — long `a.b().c().d()` navigation the caller shouldn't depend on. → hide the walk behind one method on the first object. - **Middle Man** — a class or function that mostly just delegates onward. → cut it, call the real target direct. - **Refused Bequest** — a subclass or implementer that ignores or overrides most of what it inherits. → drop the inheritance, use composition.
### 4. Spawn both sub-agents in parallel
**Standards sub-agent prompt** — include:
- The full diff command and commit list. - The list of standards-source files you found in step 3, **plus the from step 3** pasted in full — the sub-agent has no other access to it. - The brief: "Report — per file/hunk where relevant — (a) every place the diff violates a documented standard: cite the standard (file + the rule); and (b) any baseline smell you spot: name it and quote the hunk. Distinguish hard violations from s — documented-standard breaches can be hard, but baseline smells are always s, and a documented repo standard overrides the baseline. Skip anything tooling enforces. Under 400 words."
**Spec sub-agent prompt** — include:
- The diff command and commit list. - The path or fetched contents of the spec. - The brief: "Report: (a) requirements the spec asked for that are missing or partial; (b) behaviour in the diff that wasn't asked for (); (c) requirements that look implemented but where the implementation looks wrong. Quote the spec line for each finding. Under 400 words."
If the spec is missing, skip the Spec sub-agent and note this in the final report.
### 5. Aggregate
Present the two reports under `## Standards` and `## Spec` headings, . Do **not** — the two axes are deliberately separate (see _Why two axes_).
End with a one-line summary: total findings per axis, and the worst issue _within each axis_ (if any). Don't pick a single winner across axes — that's the reranking the separation exists to prevent.
## Why two axes
A change can pass one axis and fail the other:
- Code that follows every standard but implements the wrong thing → **Standards pass, Spec fail.** - Code that does exactly what the issue asked but breaks the project's conventions → **Spec pass, Standards fail.**
Reporting them separately stops one axis from .
对 `HEAD` 与用户给出的固定参照点之间的 diff 进行双轴审查:
- **Standards** —— 代码是否符合此仓库成文的编码标准? - **Spec** —— 代码是否忠实实现最初的 issue / spec?
两个维度都在**并行子代理**中运行,使它们不会相互污染上下文;随后,本技能聚合它们的发现。
issue tracker 应当已经提供给你——如果缺少 `docs/agents/issue-tracker.md`,运行 `/setup-matt-pocock-skills`。
## 流程
### 1. 固定参照点
无论用户说了什么,那就是固定参照点——commit SHA、branch name、tag、`main`、`HEAD~5` 等。如果用户没有指定,就询问。
只捕获一次 diff 命令:`git diff <fixed-point>...HEAD`(三点语法,因此比较以 merge-base 为基准)。还要通过 `git log <fixed-point>..HEAD --oneline` 记录 commit 列表。
继续之前,确认固定参照点可解析(`git rev-parse <fixed-point>`),并且 diff 非空。坏 ref 或空 diff 应当在这里失败——而不是进入两个并行子代理后才失败。
### 2. 识别 spec 来源
按以下顺序寻找最初的 spec:
1. commit message 中的 issue 引用(`#123`、`Closes #45`、GitLab `!67` 等)——通过 `docs/agents/issue-tracker.md` 中的工作流获取。 2. 用户作为参数传入的路径。 3. `docs/`、`specs/` 或 `.scratch/` 下与 branch name 或 feature 匹配的 spec 文件。 4. 如果仍然找不到,询问用户 spec 在哪里。如果用户说没有,**Spec** 子代理将跳过,并报告“no spec available”。
### 3. 识别 standards 来源
仓库中任何记录代码应当如何编写的内容,例如 `CODING_STANDARDS.md` 或 `CONTRIBUTING.md`。
除仓库自身记录的内容外,Standards 维度始终携带下方的**代码坏味道基线(smell baseline)**——一组固定的 Fowler 代码坏味道(_Refactoring_ 第 3 章),即便仓库什么都没写也适用。它受两条规则约束:
- **仓库优先。** 成文的仓库标准始终胜出;如果它认可了基线本会标记的做法,就压制该坏味道。 - **始终属于判断问题。** 每种坏味道都是带标签的启发式判断(“possible Feature Envy”),绝不是硬性违规;与这里的任何标准一样,工具已经强制执行的内容应当跳过。
每种坏味道都按“*它是什么* → *如何修复*”来描述;将其与 diff 匹配:
- **Mysterious Name(神秘命名)** —— 函数、变量或类型的名称没有揭示其行为或承载的内容。→ 重命名;如果找不到诚实的名称,说明设计仍然模糊。 - **Duplicated Code(重复代码)** —— 同一逻辑形态出现在改动中的多个 hunk 或文件里。→ 提取共享形态,并从两处调用。 - **Feature Envy(依恋情结)** —— 一个方法访问另一个对象的数据多于自身数据。→ 将方法移到它所依恋的数据上。 - **Data Clumps(数据泥团)** —— 相同的几个字段或参数总是一起传递(像是一个等待诞生的类型)。→ 将其打包成一个类型并传递该类型。 - **Primitive Obsession(基本类型偏执)** —— primitive 或 string 代替了本应拥有自己类型的领域概念。→ 为该概念建立小型专属类型。 - **Repeated Switches(重复 switch)** —— 针对同一类型的相同 `switch`/`if` 级联在改动中反复出现。→ 用多态替代,或让两处共享同一个 map。 - **Shotgun Surgery(霰弹式修改)** —— 一项逻辑变更迫使 diff 在许多文件中产生零散编辑。→ 把一起变化的内容集中到一个 module。 - **Divergent Change(发散式变化)** —— 一个文件或 module 因多个不相关原因而被修改。→ 拆分,使每个 module 只因一种原因变化。 - **Speculative Generality(夸夸其谈的通用性)** —— 为 spec 并不需要的需求加入 abstraction、parameter 或 hook。→ 删除;内联回去,直到出现真实需求。 - **Message Chains(消息链)** —— 调用者不应依赖的长链式导航 `a.b().c().d()`。→ 把遍历隐藏在第一个对象的一个方法后面。 - **Middle Man(中间人)** —— class 或 function 大部分工作只是继续委托。→ 移除它,直接调用真正目标。 - **Refused Bequest(被拒绝的遗赠)** —— subclass 或 implementer 忽略或覆盖了它继承的大部分内容。→ 放弃 inheritance,改用 composition。
### 4. 并行生成两个子代理
**Standards 子代理 prompt** —— 包含:
- 完整的 diff 命令与 commit 列表。 - 第 3 步找到的 standards 来源文件列表,**加上第 3 步的完整 smell baseline**——子代理没有其他途径访问它。 - brief:“在相关时按 file/hunk 报告——(a) diff 违反成文标准的每个位置:引用标准(文件 + 规则);(b) 发现的任何 baseline smell:说出名称并引用 hunk。区分硬性违规与判断问题——成文标准的违反可以是硬性的,但 baseline smell 始终是判断问题,而且成文的仓库标准覆盖 baseline。跳过工具已经强制执行的任何内容。控制在 400 词以内。”
**Spec 子代理 prompt** —— 包含:
- diff 命令与 commit 列表。 - spec 的路径或已获取的内容。 - brief:“报告:(a) spec 要求但缺失或只完成一部分的需求;(b) diff 中没有被要求的行为(scope creep);(c) 看似已经实现、但实现方式看起来错误的需求。每项发现都引用对应 spec 行。控制在 400 词以内。”
如果缺少 spec,跳过 Spec 子代理,并在最终报告中注明。
### 5. 聚合
把两份报告分别放在 `## Standards` 与 `## Spec` 标题下,原样呈现或仅作轻微整理。**不要**合并或重新排序发现——两个维度是刻意分开的(参见 _Why two axes_)。
最后用一行总结:每个维度的发现总数,以及该维度内部最严重的问题(如果有)。不要跨维度选出单一赢家——那正是这种分离要避免的重新排序。
## 为什么使用两个维度
一项改动可能通过一个维度,却在另一个维度失败:
- 代码遵守每一条标准,却实现了错误的东西 → **Standards 通过,Spec 失败。** - 代码完全实现了 issue 的要求,却破坏项目约定 → **Spec 通过,Standards 失败。**
分开报告能阻止一个维度掩盖另一个维度。

Entry SHA-256: 9cf46653dd9c710ea1e6c22423caf31a794c88773bc94bdaa23140277f470442

2. Why This Skill Is Clever

一句话核心机制

code-review 把同一份 Git diff 交给彼此隔离的 Standards 与 Spec 两条审查轴,再保留两套结论的独立性,而不是压成一个看似简单的总分。

触发与边界

直接来源: 技能适用于审查分支、PR、进行中的已提交改动,或“review since X”这类请求;用户必须给出 commit、branch、tag、mainHEAD~5 等固定参照点。缺少参照点时先询问,坏 ref 或空 diff 则在派生子代理前终止。

支持性上下文: skills/engineering/README.md 将它列为 model-invoked,说明它既可由用户直接提出,也可由模型按描述触发。docs/engineering/code-review.md 进一步提醒:三点 diff 只覆盖到 HEAD 的提交,不包含尚未提交的工作区修改。

适用边界: 它不是通用 bug hunter,也不是整库架构体检。它回答的是“是否按仓库标准构建”与“是否实现了原始需求”,并依赖 Git、可定位的 spec、仓库规范文件以及支持并行子代理的 agent harness。

信息架构 / 执行顺序

  1. 先固定比较面: 解析 fixed point,冻结 git diff <fixed-point>...HEAD 与 commit list。
  2. 先验失败: 验证 ref 可解析且 diff 非空,把廉价、确定性的错误挡在并行执行之前。
  3. 分别收集证据: Spec 轴按 commit 引用、用户路径、仓库候选文件、询问用户的顺序寻找原始规格;Standards 轴读取仓库规则,并补上 Fowler smell baseline。
  4. 隔离推理: 两个子代理并行工作,各自携带完整输入和不超过 400 词的明确 brief。
  5. 保留双轴: 只做原样或轻微清理的聚合,分别报告最严重问题,拒绝跨轴选“总冠军”。

最巧妙的设计点

1. 用正交问题阻止“综合分”掩盖失败

“A change can pass one axis and fail the other”

直接来源。 “built right?” 与 “right thing?” 并不等价。把它们保持为两个输出区块,能防止代码风格优秀却做错需求,或功能正确却破坏仓库约定时,被另一个维度的通过结果冲淡。

2. 把昂贵并行放在确定性前置校验之后

“A bad ref or empty diff should fail here”

直接来源。 先运行 git rev-parse 并确认 diff 非空,再启动两个子代理。这是典型的 fail-fast:既减少无效成本,也让错误出现在真正拥有修复信息的层级。

3. Spec 搜索顺序体现证据优先级

“Look for the originating spec, in this order”

直接来源。 技能先查 commit 中的 issue 引用,再看用户参数和仓库文件,最后才询问。它没有从代码反推需求;完全没有 spec 时明确跳过该轴并写出 no spec available,避免伪造验收标准。

4. 让仓库规则覆盖通用启发式

“The repo overrides.”

直接来源。 Fowler smell baseline 只提供最低覆盖,仓库明文规范拥有更高优先级;而 smell 始终标为 judgement call。这同时避免“无规范就什么都不审”和“拿通用教条压过项目真实约定”两种失败。

5. 给子代理完整、受限且可核验的任务包

“the sub-agent has no other access to it”

直接来源。 Standards prompt 必须粘贴完整 baseline,两个 prompt 都限定证据引用、问题类别和 400 词预算。设计没有假设子代理会自动继承父上下文,而是显式传递完成任务所需的最小闭包。

6. 聚合器拒绝替两个审查者重新裁判

“Do not merge or rerank findings”

直接来源。 聚合器只保留结构与可读性,不把两个独立目标压进同一优先级。这种“少做一步”的克制正是输出可信的关键不变量。

防失败机制与 trade-off

来源中明确的不变量: fixed point 必须可解析;diff 必须非空;找不到 spec 时不能编造;仓库规范覆盖 smell baseline;smell 只能作为启发式判断;工具已经强制的规则应跳过;最终必须维持 ## Standards## Spec 两个区块,且只选每轴最严重问题。

支持性上下文中的风险: 官方 docs 记录了两项尚未在技能正文解决的问题:本地 /code-review 可能与 Claude Code 内置命令重名;子代理 prompt 没有禁止再次调用 /code-review,在某些 harness 中可能递归扩散。它还指出聚合器不会重新核验每条子代理结论,因此引用是“可检查入口”,不是自动正确的保证。

解释与权衡: 并行隔离减少相互锚定,但会重复读取 diff 并增加 token 成本;固定 smell baseline 提供可移植下限,却可能带来非确定性的判断项;三点 diff 精确对应分支自 merge-base 以来的提交,却天然看不到未提交改动。迁移到不支持 sub-agent、并行调度或 Git CLI 的 harness 时,需要用两个独立上下文串行运行,并保留相同的输入冻结与双轴输出契约。

可迁移原则

今日实践题

如果你要设计一个“安全性 + 用户体验”的双轴发布审查,哪些发现必须保持分开,哪些共享前提应该在两个审查者启动前先验证?

Evidence note

Source & provenance
Repository path
skills/engineering/code-review/SKILL.md
Generated
2026-08-11 08:27 Asia/Shanghai
Upstream commit
84fdeffd12f2ee307994d1eb6feb48173b6e0502
Source
View on GitHub