Code Review and Quality / 代码审查与质量
> 原始版本：@user_13e46a0f/codereview（原作者 Xaction）。本版为 skillhub+ 改造版，保留原作方法论，补齐其缺失的触发、输出格式与步骤闭环。
本技能包含
| 环节 | 说明 |
|---|---|
| The approval standard … | 完整可用 |
| The Five-Axis Review /… | 完整可用 |
| Structural Remedies / … | 完整可用 |
| Change Sizing / 变更规模 | 完整可用 |
| Severity labels / 严重程度… | 完整可用 |
| Review Process / 审查流程 | 完整可用 |
| Output: Review Report … | 完整可用 |
| Review: [PR/Change tit… | 完整可用 |
| Common Rationalization… | 完整可用 |
| Red Flags / 红旗 | 完整可用 |
| 其余 5 个环节 | 完整可用 |
完整交付：全部 15 个环节 + 结果文件，无内容阉割。
Code Review and Quality / 代码审查与质量
Multi-dimensional review for any change before merge. Every change gets reviewed. No exceptions. / 合并前对任何变更做多维度审查。每个变更都要被审查，没有例外。
The approval standard / 通过标准
Approve a change when it definitely improves overall code health, even if it isn't perfect. Perfect code doesn't exist. Don't block a change because it isn't exactly how you would have written it. If it improves the codebase and follows project conventions, approve it. / 当变更确实提升了整体代码健康度时通过，即使它不完美。完美代码不存在。不要因为不是你的写法就阻塞。如果变更提升了代码库并遵循项目约定，通过它。
When to use / 何时使用
- Before merging any PR or change / 合并任何 PR 之前
- After completing a feature implementation / 完成功能实现后
- When another agent or model produced code you need to evaluate / 评估其他 Agent/模型生成的代码
- When refactoring existing code / 重构现有代码
- After any bug fix (review both fix and regression test) / 修任何 Bug 后（同时审查修复和回归测试）
- User says "review this", "code review", "审查代码", "审查这个 PR" / 用户说"review this"、"code review"、"审查代码"、"审查这个 PR"
The Five-Axis Review / 五维审查
Every review evaluates code across these dimensions: / 每个审查都跨这些维度评估代码：
1. Correctness / 正确性
Does the code do what it claims to do? / 代码是否做了它声称要做的事？
- Matches the spec or task requirements? / 符合规范或任务要求？
- Edge cases handled (null, empty, boundary values)? / 边界情况处理了吗（null、空、边界值）？
- Error paths handled, not just happy path? / 错误路径处理了吗，不只是 happy path？
- Tests pass? Do tests actually test the right thing? / 测试通过吗？测试是否测了对的东西？
- Off-by-one errors, race conditions, state inconsistencies? / 差一错误、竞态条件、状态不一致？
2. Readability & Simplicity / 可读性与简洁性
Can another engineer understand this without the author explaining? / 另一个工程师能否不需要作者解释就看懂？
- Names descriptive and consistent with project conventions?(No
temp,data,resultwithout context) / 命名有意义、与项目约定一致？- Control flow straightforward (avoid nested ternaries, deep callbacks)?/ 控制流直接（避免嵌套三元、深度回调）？- Related code grouped, clear module boundaries?/ 相关代码分组、模块边界清晰？- "Clever" tricks that should be simplified?/ 「巧妙」技巧是否应该简化？- Could this be done in fewer lines?(1000 lines where 100 suffice is failure) / 能用更少行吗？
（100 行能搞定的写了 1000 行就是失败）
- Are abstractions earning their complexity?Don't generalize until third use case. / 抽象是否值得复杂度？不要在第三个用例之前泛化。- New conditional bolted onto unrelated flow?That's a design smell — push logic into its own helper. / 新条件分支硬挂到无关流程？是设计气味——把逻辑提到自己的 helper。- Repeated conditionals on same shape signal missing model or dispatcher. / 重复形状的条件分支意味着缺少模型或分发器。- Dead code artifacts: no-op variables, backwards-compat shims, // removed comments. / 死代码：
无用变量、向后兼容壳、// removed 注释。
3. Architecture / 架构
Does the change fit the system's design? / 变更是否符合系统设计？
- Follows existing patterns or introduces a justified new one?/ 遵循现有模式，或引入合理的新模式？- Clean module boundaries?/ 模块边界清晰？- Code duplication that should be shared?/ 应该共享的代码重复？- Dependencies flowing in right direction (no circular)?/ 依赖流向正确（无循环）？- Abstraction level appropriate (not over-engineered, not too coupled)?/ 抽象层级合适（不过度设计、不过度耦合）？- Does refactor reduce complexity or just relocate it?
Count concepts a reader must hold. If "cleaner" version leaves count unchanged, it isn't cleaner — prefer the restructuring that makes whole branches, modes, or layers disappear. / 重构是降低复杂度还是搬位置？数读者需要 hold 的概念。如果「更干净」的版本数量没变，那就没更干净——优先选那种让整个分支/模式/层消失的重构。- Feature-specific logic leaking into shared/general module?Keep logic in owning layer. / 特性专属逻辑是否泄漏到共享/通用模块？逻辑放在拥有的层。- Type boundaries explicit?
Question gratuitous any/unknown/optional/casts and silent fallbacks. / 类型边界是否显式？质疑无端 any/unknown/optional/cast 和静默回退。
4. Security / 安全
Does the change introduce vulnerabilities? / 变更是否引入漏洞？
-
User input validated and sanitized?/ 用户输入验证和清理了吗？- Secrets out of code, logs, version control?/ 密钥不在代码、日志、版本控制里？- Authentication/authorization checked where needed?/ 需要的地方做了鉴权/授权？- SQL queries parameterized (no string concatenation)?/ SQL 参数化（无字符串拼接）？- Outputs encoded to prevent XSS?/ 输出编码防止 XSS？- Dependencies from trusted sources, no known vulnerabilities?/ 依赖来自可信源，无已知漏洞？- External data (APIs, logs, user content, config) treated as untrusted?/ 外部数据（API、日志、用户内容、配置）视为不可信？
-
External data validated at system boundaries before use?/ 外部数据在使用前在系统边界验证了吗？
Quick security scan (run if available): / 快速安全扫描（可用就跑）：
# dependency audit / 依赖审计
npm audit # Node
pip-audit # Python
go mod tidy && go list -m -u all # Go
# secret scan / 密钥扫描
grep -rE "(api[_-]?key|secret|password|token)\s*[:=]" --include="*.{js,ts,py,go,java,rb}" .
5. Performance / 性能
Does the change introduce performance problems? / 变更是否引入性能问题？
- N+1 query patterns? / N+1 查询？
- Unbounded loops or unconstrained data fetching? / 无限循环或无约束的数据获取？
- Synchronous operations that should be async? / 应该异步的同步操作？
- Unnecessary re-renders in UI components? / UI 组件不必要的重渲染？
- Missing pagination on list endpoints? / 列表端点缺分页？
- Large objects created in hot paths? / 热路径创建大对象？
Structural Remedies / 结构修复
When you flag a structural problem, propose the move — not just the problem. A review that only says "this is complex" leaves the author guessing. / 当你标记结构问题时，给出修复动作——不要只说问题。只说「这复杂」的审查让作者瞎猜。
Reach for a named restructuring: / 使用命名的重构动作：
- Replace a chain of conditionals with a typed model or explicit dispatcher / 用类型化模型或显式分发器替换条件链
- Collapse duplicate branches into a single clearer flow / 折叠重复分支为单一清晰流
- Separate orchestration from business logic so each reads on its own / 分离编排与业务逻辑让各自可读
- Move feature-specific logic out of shared module into owning package / 把特性专属逻辑从共享模块移到拥有的包
- Reuse the canonical helper instead of a bespoke near-duplicate / 复用规范 helper而不是定制近似副本
- Make a type boundary explicit so downstream branching disappears / 让类型边界显式让下游分支消失
- Delete a pass-through wrapper that adds indirection without clarifying API / 删除传递包装只增间接不澄清 API
- Extract a helper, or split a large file into focused modules / 提取 helper，或拆分大文件为聚焦模块
Prefer the remedy that removes moving pieces over one that spreads the same complexity around. / 优先选消除移动部件的修复，而不是把同样复杂度摊开。
Change Sizing / 变更规模
Small focused changes are easier to review, faster to merge, safer to deploy. / 小而聚焦的变更更易审查、更快合并、更安全部署。
~100 lines changed → Good. Reviewable in one sitting.
好。一次性能审完。
~300 lines changed → Acceptable if single logical change.
可接受（如果是一个逻辑变更）。
~1000 lines changed → Too large. Split it.
太大。拆开。
Watch file size, not just diff size. Around 1000 total lines in a single file is a common inspection signal. When a change grows an already-large file, ask whether to extract helpers, subcomponents, or modules first. / 看文件大小，不只看 diff 大小。单文件 ~1000 行总行数是常见检查信号。变更让已大文件更大时，先问要不要先拆。
Splitting strategies / 拆分策略:
| Strategy | How | When |
|---|---|---|
| Stack 堆叠 | Submit small change, start next based on it / 提交小变更，下一个基于它 | Sequential dependencies / 顺序依赖 |
| By file group 按文件分组 | Separate changes for groups needing different reviewers / 分组给不同审查者 | Cross-cutting concerns / 横切关注点 |
| Horizontal 横向 | Create shared code/stubs first, then consumers / 先建共享代码/桩，再用 | Layered architecture / 分层架构 |
| Vertical 纵向 | Break into smaller full-stack slices / 拆为更小的全栈切片 | Feature work / 功能工作 |
Separate refactoring from feature work. Refactor + new behavior = two changes. Submit separately. / 重构与功能变更分离提交。
Severity labels / 严重程度标签
Label every comment so the author knows what's required vs optional. / 给每个评论打标，让作者知道哪些必改哪些可选。
| Prefix | Meaning | Author Action |
|---|---|---|
| (no prefix) / 无前缀 | Required change / 必改 | Must address before merge / 合并前必须处理 |
| Critical: / 严重 | Blocks merge / 阻塞合并 | Security vulnerability, data loss, broken functionality / 安全漏洞、数据丢失、功能损坏 |
| Required: / 必改 | Must fix before merge / 合并前必改 | Logic bug, missed requirement, broken test / 逻辑 Bug、漏需求、测试坏 |
| Optional: / 建议 | Suggestion / 建议 | Worth considering but not required / 值得考虑但不强制 |
| Nit: / 细节 | Minor, optional / 小问题可选 | Formatting, naming preferences / 格式、命名偏好 |
| FYI / 备忘 | Informational only / 仅作备忘 | Context for future reference / 留作日后参考 |
Lead with what matters. Order findings by leverage: correctness + security first, then structural regressions + missed simplifications, then everything else. Don't bury a real issue under cosmetic nits. A few high-conviction comments beat a long list. / 把重要的放前面。按影响力排序：正确性+安全优先，然后结构性回归+可简化点，然后其他。不要把真问题埋在细节下。
Review Process / 审查流程
Step 1: Understand context / 第一步：理解上下文
- What is this change trying to accomplish? / 变更要达成什么？
- What spec or task does it implement? / 它实现哪个规范/任务？
- What is the expected behavior change? / 期望的行为变更是什么？
Step 2: Review the tests first / 第二步：先看测试
Tests reveal intent and coverage: / 测试揭示意图和覆盖度：
- Do tests exist? / 有测试吗？
- Do they test behavior (not implementation details)? / 测的是行为（不是实现细节）？
- Are edge cases covered? / 边界覆盖了吗？
- Do tests have descriptive names? / 测试名有意义吗？
- Would tests catch a regression if code changed? / 代码改了测试能抓到吗？
Step 3: Review the implementation / 第三步：审查实现
Walk through with five axes in mind. See the Five-Axis Review above. / 用五维走查。见上。
Step 4: Categorize findings / 第四步：分类发现
Apply severity labels. Lead with what matters. / 打严重程度标签。把重要的放前面。
Step 5: Verify the verification / 第五步：核实验证
Check the author's verification story: / 检查作者的验证故事：
- What tests were run? / 跑了哪些测试？
- Did the build pass? / 构建通过吗？
- Was the change tested manually? / 手动测了吗？
- Screenshots for UI changes? / UI 变更截图了吗？
- Before/after comparison? / 前后对比了吗？
Output: Review Report / 输出：审查报告
Output format (use this template): / 输出格式（用这个模板）：
## Review: [PR/Change title]
### Context
- [ ] I understand what this change does and why / 我理解变更做什么、为什么
### Correctness / 正确性
- [ ] Change matches spec / 符合规范
- [ ] Edge cases handled / 边界处理
- [ ] Error paths handled / 错误路径处理
- [ ] Tests adequate / 测试充分
### Readability / 可读性
- [ ] Names clear / 命名清晰
- [ ] Logic straightforward / 逻辑直接
- [ ] No unnecessary complexity / 无不必要复杂度
### Architecture / 架构
- [ ] Follows existing patterns / 遵循模式
- [ ] No unnecessary coupling / 无多余耦合
- [ ] Appropriate abstraction / 抽象合适
- [ ] Refactors reduce complexity / 重构降复杂度
### Security / 安全
- [ ] No secrets in code / 代码无密钥
- [ ] Input validated / 输入验证
- [ ] No injection vulnerabilities / 无注入漏洞
- [ ] Auth checks / 鉴权检查
- [ ] External data treated as untrusted / 外部数据不可信
### Performance / 性能
- [ ] No N+1 / 无 N+1
- [ ] No unbounded operations / 无无限操作
- [ ] Pagination on lists / 列表分页
### Verification / 验证
- [ ] Tests pass / 测试过
- [ ] Build succeeds / 构建成
- [ ] Manual verification / 手动验证
### Findings / 发现
**Critical:** [list] / 严重：[列出]
**Required:** [list] / 必改：[列出]
**Optional:** [list] / 建议：[列出]
**Nit:** [list] / 细节：[列出]
**FYI:** [list] / 备忘：[列出]
### Verdict / 结论
- [ ] **Approve** — Ready to merge / 通过——可合并
- [ ] **Request changes** — Issues must be addressed / 请求变更——必须处理
Common Rationalizations / 常见合理化借口
| Rationalization / 借口 | Reality / 真相 |
|---|---|
| "It works, that's good enough" / 「能跑就行」 | Working code that's unreadable, insecure, or architecturally wrong creates compounding debt. / 能跑但难读、不安全、架构错的代码会产生复合债务。 |
| "I wrote it, so I know it's correct" / 「我写的，我肯定对」 | Authors are blind to their own assumptions. Every change benefits from another set of eyes. / 作者对自己的假设盲。 |
| "We'll clean it up later" / 「之后清理」 | Later never comes. Require cleanup before merge, not after. / 之后永远不会来。合并前要求清理，不是合并后。 |
| "AI-generated code is probably fine" / 「AI 生成的应该没问题」 | AI code needs MORE scrutiny, not less. It's confident and plausible, even when wrong. / AI 代码需要更多审查，不是更少。 |
| "The tests pass, so it's good" / 「测试过就行」 | Tests are necessary but not sufficient. They don't catch architecture, security, readability issues. / 测试必要但不充分。 |
| "The refactor makes it cleaner" / 「重构更干净」 | Relocating complexity isn't reducing it. Count concepts reader holds. / 搬位置不是降复杂度。 |
| "It's only a small addition to this file" / 「只是给这个文件小加一点」 | Small diffs still push files past healthy size. Judge resulting structure, not diff size. / 小 diff 也让文件超出健康大小。看结果结构，不看 diff 大小。 |
| "It's just a version bump" / 「只是版本号 bump」 | A bump is a behavior change you didn't write. Read the changelog. / Bump 是你没写的行为变更。读 changelog。 |
Red Flags / 红旗
- PRs merged without any review / 没审查就合并的 PR
- Review that only checks if tests pass / 只查测试通过的审查
- "LGTM" without evidence of actual review / 没证据就 LGTM
- Security-sensitive changes without security-focused review / 安全敏感变更没做安全审查
- Large PRs "too big to review properly" / 太大 PR「没法好好审」
- No regression tests with bug fix PRs / 修 Bug PR 没回归测试
- Review comments without severity labels / 审查评论无严重程度标签
- Accepting "I'll fix it later" / 接受「之后修」
- Refactor that moves code around without reducing concepts / 搬代码不降概念数的重构
- Change growing already-large file without decomposition / 让已大文件更大的变更不拆分
- New conditionals scattered into unrelated paths / 新条件分支散到无关路径
- Bespoke helper duplicating canonical one / 定制 helper 复制了规范的那个
- Bulk "bump dependencies" with no changelog review / 批量「bump 依赖」没看 changelog
Dead Code Hygiene / 死代码卫生
After any refactoring or implementation change, check for orphaned code: / 任何重构或实现变更后，检查孤儿代码：
- Identify code that's now unreachable or unused / 识别现在不可达或不用的代码
- List it explicitly / 明确列出
- Ask before deleting / 删之前先问："Should I remove these now-unused elements: [list]?"
Don't silently delete things you're not sure about. When in doubt, ask. / 没把握不要默默删。不确定就问。
Honesty in Review / 审查中的诚实
-
Don't rubber-stamp. "LGTM" without evidence helps no one. / 不要走过场。- Don't soften real issues. "This might be a minor concern" when it's a production bug is dishonest. / 不要软化真问题。- Quantify when possible. "This N+1 will add ~50ms per item" beats "this could be slow." / 能量化就量化。- Push back on approaches with clear problems. Sycophancy is a failure mode. / 对明确有问题的方案要顶回去。
-
Accept override gracefully. If author has full context and disagrees, defer. Comment on code, not people. / 优雅接受推翻。
Verification before completion / 完成前验证
- [ ] All Critical issues resolved / 所有严重问题解决
- [ ] All Required (no-prefix) changes resolved or explicitly deferred with justification / 所有必改解决或明确推迟
- [ ] Tests pass / 测试过
- [ ] Build succeeds / 构建成
- [ ] Verification story documented / 验证故事记录
See Also / 参见
- references/security-checklist.md — Full OWASP Top 10 checklist / 完整 OWASP Top 10 检查表
- references/performance-checklist.md — Performance review deep dive / 性能审查深入
Compatibility notes / 兼容性说明
- Works with any diff source: git diff, GitHub PR, GitLab MR, Bitbucket PR, or pasted code. / 适用于任何 diff 来源：git diff、GitHub PR、GitLab MR、Bitbucket PR 或粘贴的代码。- Severity vocabulary unified with industry-standard (Critical/Required/Optional/Nit) so reviews map cleanly across teams. / 严重程度词汇统一为业界标准（Critical/Required/Optional/Nit），跨团队审查映射清晰。- No tool dependency: review itself needs no special tool. Security scan commands above are best-effort. / 无工具依赖：审查本身不需要特殊工具。
上面安全扫描命令是尽力而为。
配套资源
references/security-checklist.mdreferences/performance-checklist.md
