Back to Details

code-review-and-quality

Compare original and translation side by side

🇺🇸

Original

English
🇨🇳

Translation

Chinese

Code Review and Quality

代码审查与质量

Overview

概述

Multi-dimensional code review with quality gates. Every change gets reviewed before merge — no exceptions. Review covers five axes: correctness, readability, architecture, security, and performance. The standard is: "Would a staff engineer approve this diff and the verification story?"
基于质量门禁的多维度代码审查。所有变更在合并前都必须经过审查——无一例外。审查涵盖五个维度：正确性、可读性、架构、安全性和性能。评判标准是：「资深工程师会批准这个diff和验证说明吗？」

When to Use

使用场景

  • Before merging any PR or change
  • After completing a feature implementation
  • When another agent or model produced code you need to evaluate
  • When refactoring existing code
  • After any bug fix (review both the fix and the regression test)
  • 在合并任何PR或变更前
  • 完成功能实现后
  • 当需要评估其他Agent或模型生成的代码时
  • 重构现有代码时
  • 修复任何漏洞后（同时审查修复方案和回归测试）

The Five-Axis Review

五维度审查

Every review evaluates code across these dimensions:
每一次审查都会从以下维度评估代码：

1. Correctness

1. 正确性

Does the code do what it claims to do?
  • Does it match the spec or task requirements?
  • Are edge cases handled (null, empty, boundary values)?
  • Are error paths handled (not just the happy path)?
  • Does it pass all tests? Are the tests actually testing the right things?
  • Are there off-by-one errors, race conditions, or state inconsistencies?
代码是否实现了其宣称的功能？
  • 是否符合规格或任务要求？
  • 是否处理了边缘情况（空值、空集合、边界值）？
  • 是否处理了异常路径（而非仅处理正常流程）？
  • 是否通过了所有测试？测试是否真正覆盖了正确的内容？
  • 是否存在差一错误、竞态条件或状态不一致的问题？

2. Readability & Simplicity

2. 可读性与简洁性

Can another engineer (or agent) understand this code without the author explaining it?
  • Are names descriptive and consistent with project conventions? (No 
    temp
    , 
    data
    , 
    result
    without context)
  • Is the control flow straightforward (avoid nested ternaries, deep callbacks)?
  • Is the code organized logically (related code grouped, clear module boundaries)?
  • Are there any "clever" tricks that should be simplified?
  • Could this be done in fewer lines? (1000 lines where 100 suffice is a failure)
  • Are abstractions earning their complexity? (Don't generalize until the third use case)
  • Would comments help clarify non-obvious intent? (But don't comment obvious code.)
  • Are there dead code artifacts: no-op variables (
    _unused
    ), backwards-compat shims, or 
    // removed
    comments?
其他工程师（或Agent）无需作者解释就能理解这段代码吗？
  • 命名是否具有描述性且符合项目规范？（不要使用无上下文的
    temp
    、
    data
    、
    result
    ）
  • 控制流是否直观（避免嵌套三元表达式、深层回调）？
  • 代码组织是否逻辑清晰（相关代码分组，模块边界明确）？
  • 是否存在需要简化的「巧妙」技巧？
  • 能否用更少的代码实现？（用1000行代码完成100行就能实现的功能是不合格的）
  • 抽象是否匹配其复杂度？（不要过早泛化，直到出现第三个使用场景再考虑）
  • 是否需要注释来澄清非显而易见的意图？（但不要对明显的代码添加注释）
  • 是否存在死代码残留：未使用变量（
    _unused
    ）、向后兼容垫片或
    // removed
    注释？

3. Architecture

3. 架构

Does the change fit the system's design?
  • Does it follow existing patterns or introduce a new one? If new, is it justified?
  • Does it maintain clean module boundaries?
  • Is there code duplication that should be shared?
  • Are dependencies flowing in the right direction (no circular dependencies)?
  • Is the abstraction level appropriate (not over-engineered, not too coupled)?
变更是否符合系统设计？
  • 是否遵循现有模式或引入了新模式？如果是新模式，是否有充分理由？
  • 是否保持了清晰的模块边界？
  • 是否存在可以复用的重复代码？
  • 依赖流向是否正确（无循环依赖）？
  • 抽象级别是否合适（不过度设计，也不过度耦合）？

4. Security

4. 安全性

Does the change introduce vulnerabilities?
  • Is user input validated and sanitized?
  • Are secrets kept out of code, logs, and version control?
  • Is authentication/authorization checked where needed?
  • Are SQL queries parameterized (no string concatenation)?
  • Are outputs encoded to prevent XSS?
  • Are dependencies from trusted sources with no known vulnerabilities?
变更是否引入了漏洞？
  • 用户输入是否经过验证和清理？
  • 机密信息是否未出现在代码、日志和版本控制中？
  • 是否在需要的地方进行了身份验证/授权检查？
  • SQL查询是否使用参数化（无字符串拼接）？
  • 输出是否经过编码以防止XSS攻击？
  • 依赖是否来自可信来源且无已知漏洞？

5. Performance

5. 性能

Does the change introduce performance problems?
  • Any N+1 query patterns?
  • Any unbounded loops or unconstrained data fetching?
  • Any synchronous operations that should be async?
  • Any unnecessary re-renders in UI components?
  • Any missing pagination on list endpoints?
  • Any large objects created in hot paths?
变更是否引入了性能问题？
  • 是否存在N+1查询模式？
  • 是否存在无界循环或无限制的数据获取？
  • 是否存在应异步执行的同步操作？
  • UI组件是否存在不必要的重渲染？
  • 列表接口是否缺少分页？
  • 热点路径中是否创建了大型对象？

Review Process

审查流程

Step 1: Understand the Context

步骤1：了解上下文

Before looking at code, understand the intent:
- 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

步骤2：先审查测试

Tests reveal intent and coverage:
- Do tests exist for the change?
- Do they test behavior (not implementation details)?
- Are edge cases covered?
- Do tests have descriptive names?
- Would the tests catch a regression if the code changed?
测试能体现意图和覆盖范围：
- 变更是否有对应的测试？
- 测试是否针对行为（而非实现细节）？
- 是否覆盖了边缘情况？
- 测试是否有描述性名称？
- 如果代码变更，测试能否捕获回归问题？

Step 3: Review the Implementation

步骤3：审查实现

Walk through the code with the five axes in mind:
For each file changed:
1. Correctness: Does this code do what the test says it should?
2. Readability: Can I understand this without help?
3. Architecture: Does this fit the system?
4. Security: Any vulnerabilities?
5. Performance: Any bottlenecks?
结合五维度逐一检查代码：
对于每个变更的文件：
1. 正确性：此代码是否实现了测试所描述的功能？
2. 可读性：我无需帮助就能理解这段代码吗？
3. 架构：此变更是否符合系统设计？
4. 安全性：是否存在漏洞？
5. 性能：是否存在瓶颈？

Step 4: Categorize Findings

步骤4：分类审查发现

CategoryActionExample
CriticalMust fix before mergeSecurity vulnerability, data loss risk, broken functionality
ImportantShould fix before mergeMissing test, wrong abstraction, poor error handling
SuggestionConsider for improvementNaming improvement, code style preference, optional optimization
NitpickTake it or leave itFormatting, comment wording (skip these in AI reviews)
分类操作示例
严重问题合并前必须修复安全漏洞、数据丢失风险、功能损坏
重要问题合并前应修复缺少测试、错误的抽象、异常处理不足
建议考虑优化命名优化、代码风格偏好、可选性能优化
细节优化可自行决定格式调整、注释措辞（AI审查中可跳过此类）

Step 5: Verify the Verification

步骤5：验证验证说明

Check the author's verification story:
- What tests were run?
- Did the build pass?
- Was the change tested manually?
- Are there screenshots for UI changes?
- Is there a before/after comparison?
检查作者的验证说明：
- 运行了哪些测试？
- 构建是否通过？
- 是否进行了手动测试？
- UI变更是否有截图？
- 是否有前后对比？

Multi-Model Review Pattern

多模型审查模式

Use different models for different review perspectives:
Model A writes the code
    │
    ▼
Model B reviews for correctness and architecture
    │
    ▼
Model A addresses the feedback
    │
    ▼
Human makes the final call
This catches issues that a single model might miss — different models have different blind spots.
Example prompt for a review agent:
Review this code change for correctness, security, and adherence to
our project conventions. The spec says [X]. The change should [Y].
Flag any issues as Critical, Important, or Suggestion.
针对不同审查视角使用不同模型：
模型A编写代码
    │
    ▼
模型B审查正确性和架构
    │
    ▼
模型A处理反馈
    │
    ▼
人类做出最终决定
这种模式能发现单一模型可能遗漏的问题——不同模型有不同的盲区。
审查Agent示例提示词：
审查此代码变更的正确性、安全性以及是否符合项目规范。规格要求[X]。变更应实现[Y]。将问题标记为严重、重要或建议。

Dead Code Hygiene

死代码清理

After any refactoring or implementation change, check for orphaned code:
  1. Identify code that is now unreachable or unused
  2. List it explicitly
  3. Ask before deleting: "Should I remove these now-unused elements: [list]?"
Don't leave dead code lying around — it confuses future readers and agents. But don't silently delete things you're not sure about. When in doubt, ask.
DEAD CODE IDENTIFIED:
- formatLegacyDate() in src/utils/date.ts — replaced by formatDate()
- OldTaskCard component in src/components/ — replaced by TaskCard
- LEGACY_API_URL constant in src/config.ts — no remaining references
→ Safe to remove these?
在任何重构或实现变更后，检查孤立代码：
  1. 识别当前不可达或未使用的代码
  2. 明确列出这些代码
  3. 删除前确认：「我是否应该移除这些不再使用的元素：[列表]？」
不要遗留死代码——这会混淆后续的阅读者和Agent。但不要擅自删除不确定的内容。如有疑问，先询问。
识别到的死代码：
- src/utils/date.ts中的formatLegacyDate() —— 已被formatDate()替代
- src/components/中的OldTaskCard组件 —— 已被TaskCard替代
- src/config.ts中的LEGACY_API_URL常量 —— 无剩余引用
→ 是否可以安全移除？

Honesty in Review

审查的诚实性

When reviewing code — whether written by you, another agent, or a human:
  • Don't rubber-stamp. "LGTM" without evidence of review helps no one.
  • Don't soften real issues. "This might be a minor concern" when it's a bug that will hit production is dishonest.
  • Quantify problems when possible. "This N+1 query will add ~50ms per item in the list" is better than "this could be slow."
  • Push back on approaches with clear problems. Sycophancy is a failure mode in reviews. If the implementation has issues, say so directly and propose alternatives.
  • Accept override gracefully. If the author has full context and disagrees, defer to their judgment.
在审查代码时——无论是你自己、其他Agent还是人类编写的代码：
  • 不要敷衍批准。 没有审查依据的「LGTM」对任何人都没有帮助。
  • 不要淡化真实问题。 当代码存在会影响生产环境的漏洞时，不要说「这可能是个小问题」。
  • 尽可能量化问题。 「此N+1查询会为列表中的每个条目增加约50ms延迟」比「这可能很慢」更准确。
  • 对有明显问题的方案提出反对。 阿谀奉承是审查的失败模式。如果实现存在问题，直接指出并提出替代方案。
  • 优雅接受否决。 如果作者有完整的上下文并不同意你的意见，尊重他们的判断。

Dependency Discipline

依赖管理规范

Part of code review is dependency review:
Before adding any dependency:
  1. Does the existing stack solve this? (Often it does.)
  2. How large is the dependency? (Check bundle impact.)
  3. Is it actively maintained? (Check last commit, open issues.)
  4. Does it have known vulnerabilities? (
    npm audit
    )
  5. What's the license? (Must be compatible with the project.)
Rule: Prefer standard library and existing utilities over new dependencies. Every dependency is a liability.
代码审查的一部分是依赖审查：
添加任何依赖前：
  1. 现有技术栈是否能解决这个问题？（通常是可以的）
  2. 依赖的体积有多大？（检查打包影响）
  3. 是否处于活跃维护状态？（检查最后一次提交、未解决问题）
  4. 是否存在已知漏洞？（使用
    npm audit
    检查）
  5. 许可证是什么？（必须与项目兼容）
规则： 优先使用标准库和现有工具，而非新增依赖。每个依赖都是一个潜在风险。

The Review Checklist

审查检查清单

markdown
undefined
markdown
undefined

Review: [PR/Change title]

审查：[PR/变更标题]

Context

上下文

  • I understand what this change does and why
  • 我理解此变更的目的和原因

Correctness

正确性

  • Change matches spec/task requirements
  • Edge cases handled
  • Error paths handled
  • Tests cover the change adequately
  • 变更符合规格/任务要求
  • 处理了边缘情况
  • 处理了异常路径
  • 测试充分覆盖了变更内容

Readability

可读性

  • Names are clear and consistent
  • Logic is straightforward
  • No unnecessary complexity
  • 命名清晰且一致
  • 逻辑直观
  • 无不必要的复杂度

Architecture

架构

  • Follows existing patterns
  • No unnecessary coupling or dependencies
  • Appropriate abstraction level
  • 遵循现有模式
  • 无不必要的耦合或依赖
  • 抽象级别合适

Security

安全性

  • No secrets in code
  • Input validated at boundaries
  • No injection vulnerabilities
  • Auth checks in place
  • 代码中无机密信息
  • 边界处的输入已验证
  • 无注入漏洞
  • 已添加权限检查

Performance

性能

  • No N+1 patterns
  • No unbounded operations
  • Pagination on list endpoints
  • 无N+1查询模式
  • 无无界操作
  • 列表接口已分页

Verification

验证

  • Tests pass
  • Build succeeds
  • Manual verification done (if applicable)
  • 测试通过
  • 构建成功
  • 已完成手动验证（如适用）

Verdict

结论

  • Approve — Ready to merge
  • Request changes — Issues must be addressed
undefined
  • 批准 —— 可合并
  • 要求修改 —— 必须解决问题
undefined

Common Rationalizations

常见自我合理化借口

RationalizationReality
"It works, that's good enough"Working code that's unreadable, insecure, or architecturally wrong creates debt that compounds.
"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. The review is the quality gate — use it.
"AI-generated code is probably fine"AI code needs more scrutiny, not less. It's confident and plausible, even when wrong.
"The tests pass, so it's good"Tests are necessary but not sufficient. They don't catch architecture problems, security issues, or readability concerns.
借口现实
「能运行就够了」可运行但不可读、不安全或架构错误的代码会产生不断累积的技术债务。
「我写的代码，我知道它是正确的」作者会对自己的假设视而不见。任何变更都能从另一视角受益。
「我们以后再清理」以后永远不会到来。审查就是质量门禁——充分利用它。
「AI生成的代码可能没问题」AI代码需要更严格的审查，而非更少。它即使错误也会表现得自信且合理。
「测试通过了，所以没问题」测试是必要但不充分的条件。它们无法捕获架构问题、安全隐患或可读性问题。

Red Flags

危险信号

  • PRs merged without any review
  • Review that only checks if tests pass (ignoring other axes)
  • "LGTM" without evidence of actual review
  • Security-sensitive changes without security-focused review
  • Large PRs that are "too big to review properly"
  • No regression tests with bug fix PRs
  • PR未经任何审查就被合并
  • 仅检查测试是否通过的审查（忽略其他维度）
  • 无实际审查依据的「LGTM」
  • 安全敏感变更未经过安全专项审查
  • 因「太大而无法充分审查」的大型PR
  • 漏洞修复PR未包含回归测试

Verification

最终验证

After review is complete:
  • All Critical issues are resolved
  • All Important issues are resolved or explicitly deferred with justification
  • Tests pass
  • Build succeeds
  • The verification story is documented (what changed, how it was verified)
审查完成后：
  • 所有严重问题已解决
  • 所有重要问题已解决或有明确理由推迟处理
  • 测试通过
  • 构建成功
  • 验证说明已记录（变更内容、验证方式）