代码审查

审查的目标不是"把代码看一遍"，而是在合并前抓住代价最高的缺陷，并用最少的噪声说清楚。一次好的审查有三个特征：每条意见都能落到具体 path:line、每条意见都能说清触发条件与影响、审查者如实说明自己没看到什么。

0. 先定审查范围

• 用户给了 base 就用用户的；只说"我的分支"时先看 git branch --show-current 和 git remote show origin 推断默认分支（main/master），并把实际使用的 base 写进报告。推断不出来就问一句，不要猜。
• 仓库不在当前工作目录时传 directory=<repo 路径>；工具返回 {"error","hint"} 时按 hint 处理（例如换 analyze_file 或直接读文件），不要静默继续。
• 报告开头必须写明范围，例如：范围：main...HEAD（5 文件，+120/−30）。
• diff 为空就直接说没有变更，不要编造审查意见。
• 如果 code-review-mcp 不可用：退化为 git diff <base>...HEAD 加直接读文件，并在报告里说明工具不可用。

1. 用工具拿事实，而不是拿结论

review_changes 返回的是线索（findings / signals / function_deltas / focus），不是审查结论。它看不到的东西才是审查的主体：改动意图、语义、跨文件契约、运行时行为。

读 packet 之前，先弄清改动意图：git log --oneline <base>..HEAD、PR 描述、关联 issue。不知道它想做什么，就判断不了它做完了没有。

读 packet 的顺序：

1. focus：先看它点名的位置——风险密度最高；
2. files[*].priority == "high"：逐文件深审；
3. signals：依赖、迁移、CI、接口契约、测试变更、安全命中（secret_added / security_finding），往往比单行问题更重要；
4. limits：记住哪些文件被跳过、哪些被截断、子目录范围说明——这是最后"覆盖范围"的素材。

工具结果的用法：

• 每条 finding 都必须回到源码确认（见 §3），禁止把工具输出直接粘进报告。
• 工具的 severity（error/warning/info）是规则默认严重级，不是报告级别；只有你自己验证并定级后才能写进"阻塞"。confidence 为 medium/low 的命中更要先确认，多数应按"待确认"处理。
• error 级命中（密钥、注入类）优先确认，但也要排除误报（测试夹具里的假密钥很常见）。
• 工具附带的分数/等级（quality）只是健康度参考，不要写进审查结论或用来替代判断。
• analyze_file 只在需要看整文件既有问题时用；不算本次引入的问题要单独标注。
• check_project 只用于"这个仓库长什么样"的摸底，不作为审查结论。
• list_rules 用于向用户解释某条规则的 ID 与含义。
• review_diff 已弃用（v0.3.0 起），行为与旧版一致但只做浅层 diff 检查；不要使用，统一用 review_changes。
• 需要看调用方、类型定义、测试时，直接读文件——必须读，这一步不能省。

2. 审查维度（逐项过，不要跳）

对每个 high / medium 文件，带着这些问题读改动及其上下文：

A. 正确性 — 改动在边界上还对吗？ 空/单元素/None/0/负数/超长输入；off-by-one；条件取反；早返回是否跳过清理；默认值是否悄悄改变语义；状态机是否漏分支；浮点、时区、编码、排序稳定性。

B. 错误与失败路径 — 出错时会发生什么？ 新增的 IO/网络/解析/序列化是否处理失败；except 是否过宽或吞掉错误；失败是否被静默成空值或默认值；部分写入是否非原子；重试是否有上限；错误信息是否泄漏内部细节。

C. 安全 — 输入是否被当成代码/命令/路径/HTML？ 注入（SQL、命令、模板、路径穿越）、反序列化、认证与授权检查是否被新分支绕过、密钥与凭据、敏感日志、CSP/CORS/跳转、新依赖是否可信。

D. 并发与资源 — 并发下还成立吗？ 共享可变状态、锁的顺序与粒度、async 里的阻塞调用、连接/文件/任务是否释放、无界队列或无界内存、竞态与 TOCTOU。

E. 数据与兼容 — 上线和回滚会怎样？ schema/迁移是否可逆、旧数据能否读、序列化格式变化、API 字段删除或类型收窄、默认值变化对老客户端的影响、缓存键与失效、幂等性。

F. 性能 — 规模上来会怎样？ 循环内查询（N+1）、缺失索引、重复计算、无界分页、大 payload 一次性读入。

G. 测试 — 这些改动被证明过吗？ 新行为是否有测试；测试是否断言真实行为而不是只"跑通"；被删除、被 skip、被放宽的断言；mock 是否把被测逻辑也 mock 掉了。

H. 可观测性 — 出问题能查吗？ 关键路径的日志、指标、追踪；配置默认值是否安全；feature flag 与回滚开关。

I. 依赖与构建 — 值得引入吗？ 新依赖的必要性、版本是否钉住、许可证、CI 变更是否削弱了检查。

J. 风格 — 只在有据可依时提。 仓库有 formatter/linter 配置就只说"跑一下 formatter"，不逐条列；没有标准时才提，并且合并成一条。

K. 意图与完整性 — 它声称做的事做完了吗？ 对照 commit/PR 描述检查：是否漏改调用点、配置、迁移、文档、类型定义；是否只改了一半的入口（新代码走新逻辑、旧入口没切）；是否夹带了无关重构或格式化（scope creep，应拆 PR）。

3. 验证门槛（最重要的一节）

报出去之前，每条候选问题都要过这一关：

1. 我能说出具体的输入、状态或调用顺序吗？（说不出来 → 那不是缺陷，是感觉）
2. 我会看到什么错误结果？（崩溃、错数据、越权、静默失败、性能退化）
3. 源码里确实没有别的地方拦住它吗？（找过校验层、类型约束、上游保证、测试了吗？）
4. 这是本次改动引入或放大的，还是本来就存在？（看 base 版本或 git log -L）

四问答不全的，二选一：降级成 "待确认"（写清怀疑什么、为什么不确定、需要什么信息），或者直接丢掉。误报的代价高于漏报一条 minor——宁可少说，不说错。

反向检查，防止漏报：改动删掉了什么？删掉的校验、异常处理、测试、await、return 往往是缺陷来源。改动新增了分支、参数、配置项吗？它们的组合是否都被覆盖。

4. 定级与取舍

• 默认最多 8 条发现，按阻塞 → 重要 → 次要排序；超出就合并同类项，或只保留阻塞与重要。
• 次要问题超过 3 条时合并成一条，不要刷屏。
• 与本次改动无关的问题，单列"顺带发现（非本次引入）"，最多 2 条，并明确写"不阻塞本次合并"。

5. 输出

## 结论
Request changes — <一句话：为什么>

范围：main...HEAD（5 文件，+120/−30）

## 阻塞
1. `src/auth/session.py:42` 会话校验在异常分支被跳过
   - 触发：token 解析抛 ValueError 时直接 return True
   - 影响：任意畸形 token 都能通过鉴权
   - 修复：except 分支 return False；补一条"畸形 token 被拒"的测试

## 重要
1. ...

## 待确认
- `src/db.py:88` 没找到分页上限的调用方约束，怀疑无 limit 时全表扫描；需要看调用方或压测确认。

## 已检查、未发现问题
正确性边界 / 错误路径 / 安全（注入、鉴权、密钥）/ 并发 / 数据迁移 / 测试。

## 覆盖范围
深审 3 个 high 文件（a.py、b.py、c.py）；扫过 5 个 medium；跳过 2 个生成文件与 1 个二进制；truncated=true，frontend/ 未审查。

规则：

• 每条发现必须有 path:line、触发条件、影响；修复建议能贴代码就贴。
• 不重复 diff、不夸奖、不总结代码做了什么。
• 结论三选一：Request changes / Approve with comments / Approve，后面接一句理由。
• 用用户的语言回答；标识符、命令、路径保持原样。
• 没有发现问题也要给出"已检查清单 + 覆盖范围"，让用户知道审查质量，而不是感觉被敷衍。

6. 大改动怎么办

• 超过 40 个文件或 1000 增加行：按 priority 分层，只深审 high，medium 快扫，low 只看调用关系；报告里写明未深审的文件。
• 用户要求"全审"：分批审查，每批给结论，最后汇总；不为了覆盖而降低每条意见的质量。
• 单文件超 5MB、二进制、生成代码：跳过并说明。

反模式（出现即错）

• 把 linter 输出当审查结论；或反过来完全不用工具、凭空猜。
• 报"这里可能有问题"却给不出触发条件。
• 把 pre-existing 问题当成这次改动引入的。
• 在代码没变的地方要求重构（scope creep）。
• 用"建议添加注释/测试"凑数——要么指出缺哪个具体测试、覆盖哪个分支，要么不提。
• 只挑风格问题，漏掉真正的逻辑缺陷。
• 把工具的 severity=error 直接写成"阻塞"，或用 0-100 分 / A-F 等级代替审查结论。
• 因为改动大就笼统说"整体不错，建议多测试"。
• 修改代码：本技能只审不改；用户要求修复时，先出审查结论，再单独执行修改。