code-review

代码审查

根据以下原则审查差异或指定文件。

1. 名称必须反映当前实际情况

• 变量和函数的名称应描述它们现在是什么，而不是过去是什么。
• 如果底层机制发生了变化（例如 FUSE → NFS），所有相关名称都必须更新。
• 请思考：新读者会不会被这个名称弄糊涂？

2. 不留过时引用

• 重构后，搜索旧方案的相关引用——失效的检测逻辑、废弃的功能标志、提及已删除代码的注释。
• 如果某种方案尝试后又被撤销，就删除所有痕迹。代码库应呈现出当前方案从一开始就是既定计划的样子。

3. 简化检测和保护逻辑

• 判断“某功能能否运行”的条件应只检查真正关键的那一项。
• 如果一项检查就能涵盖所需条件，就不要串联备用检测（存在二进制文件 OR 存在源代码 OR 存在工具链）。

4. 注释说明原因，而非发生过什么

• **保留：**不明显的技术发现、平台特性、“如果删除此项，X 会因 Y 而出问题”。
  • 好：// com.apple.provenance causes SIGKILL when spawned as child process
  • 好：// umount while server is alive panics the macOS NFS client
  • 好：// NFS client uses cookie verifier to decide if cached readdir is valid
• **删除：**叙述尝试过什么、放弃了什么、重命名了什么。
  • 不好：// We changed this from cp to cat to work around the provenance issue
  • 不好：// Wrapper added because FUSE-T was zero-padding (see commit abc123)
  • 不好：// Previously this was called fuseAvailable but we renamed it
• **删除：**只是复述代码所做操作、没有补充理由的注释。
• 很多时候无需任何注释。

5. 分离安装时和运行时的职责

• 安装脚本负责设置前置条件（工具链、系统依赖）。
• 运行时命令负责处理编译、缓存和二进制文件管理。
• 不要混为一谈。如果安装脚本编译的二进制文件也由运行时编译，那么其中一处就有问题。

6. 不留中间产物

• 在尝试多种方案后，检查是否有只在过渡阶段存在的逻辑。
• 如果为方案 A 添加了变通方案，后来又改用方案 B，就应删除该变通方案，即使它没有造成危害。
• 代码应呈现出从一开始就知道正确答案的人从头编写的样子。

7. 变更后收紧保证

• 当某项变更使某个条件成为保证（例如变量现在总会被赋值、文件总会被创建、函数总会返回）时，检查下游代码中仍在防范旧“可能”状态的逻辑。
• 对不再可能为空的值进行多余的 null 检查、备用默认值和 if (x) 判断会造成误导——它们暗示了一种并不存在的可能性。
• 不再需要重新赋值时，优先使用 const 而不是 let。
• **绝不要压制信号——要修复根本原因。**未使用参数前加 _ 前缀、// @ts-ignore、eslint-disable、as any——这些都会掩盖实际问题。如果参数未被使用，就删除它，并在所有调用处一并移除。如果类型不匹配，就修正类型。运行检查器，修复所有错误，然后重复此过程。跨多个文件进行机械式修改正是 Agent 擅长的事——调用处再多也不是问题。

8. 变更纠缠时，从头开始

• 如果一个文件经历了 3 轮或更多轮相互冲突的编辑，使用 git restore 还原，再重新应用所需的更改。
• 不要试图逐处修补一团乱麻——从头开始更快，也更不容易出错。

9. 测试必须验证实际值，而不只是集合大小

• 测试去重、规范化或幂等操作时，应断言存储的值，而不只是使用 toHaveLength(1)。
• 仅检查长度无法证明逻辑正确——有缺陷的规范化器可能会悄悄丢弃一个输入，也可能存储两个不同的规范化形式，但它们碰巧匹配。
• 模式：以格式 A 添加值，再以格式 B 重新添加，然后断言数量为 1，且存储的值等于预期的规范化形式。
• 不好：expect(data.phones).toHaveLength(1)——即使规范化有问题，也会通过
• 好：expect(data.phones).toHaveLength(1); expect(data.phones[0]).toBe('+12125551234')——这能证明规范化识别了两种格式

10. 不要创建薄封装或仅用于重新导出的模块

• 如果一个模块只转发另一个包的函数或类型，就删除该模块，直接导入原始包。
• 不要为脚手架代码或尚未发布的分支工作保留本地兼容垫片。直接替换脚手架。
• 重新导出汇总模块并无用处，除非它们定义了具有项目特定语义的真实公共边界。避免创建唯一作用是 export * from ... 的文件。
• 优先直接使用上游 API 名称，而不是另造 discoverAll() 这样的本地别名来替代 loadSkills()。

11. 只在真正的职责边界上拆分

• 不要只为减少行数而拆分文件。新文件应负责一项连贯的职责，读者能清楚说出它是什么。
• 当模块可以集中管理相关策略、状态转换、资源处理或特定领域行为时，再提取模块。
• 如果调用方仍需了解所有细节才能使用，就不要把一次性辅助函数移到新文件中。
• 好的拆分通常会让依赖随其服务的职责一起迁移，从而减少原文件的导入负担。
• 如果提取后只是增加了整体间接层级，却没有厘清职责归属，就把代码留在原处。

12. 让测试与实现解耦——端到端驱动系统

最有价值的测试套件，是与其覆盖的实现解耦程度最高的套件。解耦到这样的程度：通过驱动前端来测试后端，通过用户使用的同一个入口来测试模块。探测内部实现的测试会将内部实现固定下来；驱动公共接口的测试则让你可以自由重构底层的一切。

• 优先选择能提供快速、确定性反馈的最外层入口。 CLI 二进制文件 > 顶层导出函数 > 内部辅助函数 > 私有方法。如果用户调用的是 CLI，就针对 CLI 编写测试。如果用户看到的是 TUI，就通过真实按键所经过的同一输入管道来驱动它，并断言渲染出的画面，而不是中间状态。
• 测试行为，而不是结构。 断言可观察到的结果——退出代码、标准输出、渲染结果、磁盘上的文件、HTTP 响应、持久化的行。不要断言调用了哪些函数、调用顺序如何、中间数据的形状是什么。这些都属于应该可以自由变更的实现细节。
• 测试通过，就应该意味着真实用户能得到正确结果。 如果生产路径出问题时测试仍能通过，说明测试接线方式有误。常见问题包括：模拟被测对象、绕过路由器／分发器／解析器、手动构造生产环境本应从输入生成的内部事件。
• 测试框架必须按生产环境的方式接线系统。 实际使用中始终会设置的可选输入也是契约的一部分——在测试框架中传入等效输入（例如，已完成状态流、模态配置、相同的环境变量）。如果只有在真实环境中运行才能发现 bug，说明测试框架漏掉了生产环境会设置的某项内容；修复测试框架，这样下次出现同类回归时，就能由测试套件捕获，而不是让用户发现。
• 耦合的测试会增加重构成本。 重命名内部函数或移动模块时，如果几十个测试都会失败，但用户可见行为并未改变，那么测试套件测错了层级。将这些测试改为针对外层接口编写，并删除脆弱的测试。
• 默认采用端到端或系统级回归测试。 仅在特定的复杂业务逻辑确实需要快速反馈来显著加快开发时，才使用单元测试。

13. 检查失败可能是工具链缺陷，而不是代码缺陷

在手动编写类型、添加强制类型转换、固定值或重构代码以通过检查器（类型检查器、编译器、代码检查工具、测试运行器、构建工具）之前，先确认失败是代码缺陷，而不是工具链／环境造成的问题。修改代码来满足有缺陷或不匹配的工具，只是在用变通办法掩盖真正的问题——这正是「绝不压制信号」原则在更高一层的体现。

• 使用报告失败的确切工具链进行复现——采用 CI／部署使用的版本和配置，而不只是本地版本。如果运行的是不同版本，本地检查通过并不能证明什么：固定的预发布版、预览构建，或你的机器与 CI 之间的版本漂移，都可能导致相同源码在本地通过、在 CI 失败（反之亦然）。
• 如果相同源码在实际使用／固定的工具下通过，代码就是正确的——修复应针对工具链（固定版本、修正配置），而不是代码。如果输出含糊不清，就直接探查工具——强制它打印实际计算出的值、类型或错误，而不是猜测原因。
• 确保本地与 CI 一致。 确认负责阻止合并／部署的检查使用与部署相同的工具链；版本漂移会让每个绿色检查都变得可疑，而「本地通过了」也不再能证明部署会成功。

14. 身份来自身份验证，绝不来自调用方

• 从经过身份验证的请求（API 密钥、会话或经过验证的内部密钥）中解析执行操作的身份——组织／租户／工作区 ID、用户 ID、操作者，绝不要从调用方提供的参数中获取。若公开变更操作从参数中读取 actorId／orgId，这就构成了身份伪造漏洞，而不是一项功能。
• 唯一例外是受信任的内部调用：仅当存在有效的内部密钥时才接受调用方提供的操作者；否则从身份验证信息中推导操作者，并拒绝调用方提供的 ID。
• 若要将范围限定到已验证身份所属范围内的子对象（特定频道、分享或应用），请获取_子对象_ ID，并验证它属于从身份验证信息中推导出的父对象——不要信任请求正文中与之并列的父对象 ID。

15. 公开 API 操作应精简为身份验证 + 分发，而不是内联业务逻辑

• 公开路由负责身份验证、校验、推导身份，并分发到负责实际工作的模型函数或内部操作。保持这个包装层精简——它是身份验证／运行时边界。
• 不要因为“只有 3 行”，就把第三方 SDK／分析／邮件逻辑内联到公开边界中。将其放在分发之后。
• 特别是在 Convex 中：不要为了满足某一个操作，就给包含许多操作的文件添加 'use node'——这会强制该文件中的每个操作都运行在 Node 运行时。应拆分为：身份验证 + 分发（默认运行时）→ services/<thing>.ts（'use node'，负责调用 SDK）。

16. 公开端点应以最小范围发布

• 每个路由每次请求处理一项。不要在第二个实际调用方出现之前，就预先添加 /batch 变体、分页列表或筛选参数。
• 在路由中内联 schema；如果没有任何复用场景，就不要为了“复用”把一个 4 行的 schema 提取到共享的 schemas.ts 中。除非请求正文确实没有大小上限，否则不要设置正文大小上限。

17. 复用现有类型——从已有类型派生，不要重新声明

• 如果上游已存在某个类型（schema 校验器、协议包、生成的客户端、SDK），就直接使用，不要内联重新声明其结构，哪怕只声明一部分。重复声明的结构会逐渐偏离。
• 对于变体，应通过以下方式派生：Pick／Omit／Partial／Parameters／ReturnType／索引访问／Extract／Exclude；Convex／Zod 的对应方式为 Infer<typeof V>、FunctionArgs<typeof api.x.y>、z.input/z.output。
• 错误示例：在已有 User 类型旁定义 type UserSummary = { id: string; name: string; email: string }。正确示例：Pick<User, 'id' | 'name' | 'email'>。

18. 在索引处筛选，绝不要先 take() 再后置筛选

• 当列表查询需要返回表中某个子分组的行（活跃与已归档、status === X）时，索引必须负责筛选。不要先对未筛选的查询执行 .take(N)，再在 JS 中对目标分组执行 .filter()。
• 这种做法会出问题：.take(N) 会返回索引排序中最靠前的 N 行。如果最前面的行全属于_另一个_分组，那么即使目标分组中有数百条较早的行，后置筛选也会返回零条。take(N*2) 只会延后出错。
• 推荐模式：将区分字段放入索引，并使用 .withIndex(..., q => q.eq(...))；对于“存在”分组，使用 q.gt(field, 0)（Convex 会将 undefined 排在已定义值之前）。.first()／.unique() 以及“是否存在任何 X”的探测也同样适用。

19. 绝不使用 .collect()——用 .take(N) 限制读取范围

• Convex 的 .collect() 会读取所有匹配的行，不设上限。对于会随时间按租户累积数据的内容（会话、事件、账本行、审计行），第一天看起来正常的查询，最终会在一次响应式更新中拉取数千行，并悄无声息地降低整个客户端的响应式性能。
• 将 .collect() 替换为 .take(N)，其中 N 是明显高于当前工作集的安全上限（100、1000）——这是针对异常数据的上限，而不是 UX 分页器。.withIndex(...).filter(...).collect() 也一样。
• 如果确实需要读取每一行（迁移、管理员导出、一次性维护），请用注释说明，并通过可取消的变更操作／操作运行，绝不要在响应式 query 中运行。

20. 不要只为重命名或设置默认值而重新映射行

• 把所有字段原样复制，只重命名两个字段或将 undefined → false 的对象字面量 .map() 都是冗余代码，而且会悄无声息地丢弃新列，直到有人更新映射。如果使用方需要所有字段，就返回整行；如果只需要一部分字段，就使用 Pick/Omit 或解构并保留其余字段。只有在新名称能实质性提升清晰度时才重命名；只有在下游确实无法处理缺失值时才设置默认值。
• 使用解构并保留其余字段的方式，一次性剥离仅用于存储的字段（_id、_creationTime、内部 ID）。示例：return rows.map(({ _id, _creationTime, ...row }) => row)。

21. 内部 API 不需要版本或兼容性机制

• 当生产者和使用方都由我们负责时（我们自己的应用、服务和函数），不要添加 protocolVersion 字段、版本协商、能力标记，或“以防对方版本较旧”的分支。部署就是版本。
• 唯一真正的兼容性维度是字段，而不是版本——这是不对称的，不是一个可调选项：严格解析请求（拒绝未知字段），宽容解析响应/事件（忽略未知字段）。这样生产者就能添加字段，而不需要使用方同步发布；这涵盖了我们实际遇到的唯一版本差异（组件按各自节奏更新）。
• 反模式：在没有任何代码读取的线上 Schema 中写入版本字面量；if (payload.v >= 2) 分支的唯一调用方是我们自己部署的代码；对响应使用严格解析器，这会让每次服务器新增字段都成为破坏性变更。
• 例外：真正的外部 API（使用方无法由我们重新部署）应在路由层进行版本控制（/v1/），绝不按字段分别控制。

22. 必须保持一致的内容需要有唯一负责人

• 当两个声明必须保持一致，却又能各自独立变更时，它们就会逐渐偏离。应为共同决策指定唯一位置，并让各处基于该决策组合实现——平行枚举、选择列表或重复声明其他位置已有集合的 switch 是最常见的情况（由源数据派生结构是规则 17；本规则适用于所有必须一致的值）。
• 更难的一点是：如果相近的内容确实存在合理差异，就应在代码中说明原因。未记录的差异若出现在其他方面完全相同的内容之间，就和漂移 Bug 无法区分——读者无法判断这是有意为之还是疏忽。

23. 不要把只使用一次的值提取为命名 const，除非它确实值得

• 在模块作用域提取一个只被引用一次的 const，会增加间接层，却没有带来好处：读者必须跳转到声明处才能知道值是什么，而命名本身不过是在复述内联值和简短注释已经表达的内容。应在唯一的调用处内联，并用注释说明 WHY。
• 满足以下至少一项时，单次使用的命名 const 才有合理性：
  • 它是经常变化的配置，或是预期由运维人员/读者调整的值（归为一组的超时/限制参数、功能阈值、会被调整的重试预算）。
  • 它与相关 const 并列，并从该组中获得含义（*_TIMEOUT_MS 代码块、限制表、同级枚举成员）——分组本身就是说明。
  • 单独声明在结构上有意义：它作为模块公开接口的一部分导出、被类型引用，或与它所参数化的数据/文件放在一起，以便将来新增第二个调用方时能找到它。
• 其他情况都应内联。评审标准：如果某个名称仅用于标记一个只使用一次的字面值，而且它既不常变，也没有与同类项放在一起，那就是冗余——将其合并到调用处，并用注释说明该值的原因。
• 反例：单独声明 const STOP_SESSION_CONNECT_TIMEOUT_MS = 5_000，且只在一个 runAction({ connectTimeoutMs: STOP_SESSION_CONNECT_TIMEOUT_MS }) 中使用。
• 正例：在调用处写 connectTimeoutMs: 5_000, // 5s：冷启动的沙盒不能因默认 60s 的连接超时而一直等待。

24. 批处理循环应隔离单项故障——一项出错也绝不能拖累其他项

• 对独立工作项进行循环处理时（扫描、cron 批处理、队列清空、集群遍历、扇出），必须逐项捕获异常并继续处理——将失败项收集到结果中的 failed 列表，或记录日志。未捕获的单项异常会中止整轮处理；由于运行器（cron、调度器、同步轨道）会在下个周期重新选择相同的有序集合，持续稳定失败的项就会永久阻塞队头：其后的所有项都会永远重新入队，而指标却显示任务正在“重试”。
• 在事务上下文中（例如处理页面并推进游标的数据库变更），后果更严重：未捕获的异常还会回滚游标／进度写入，因此同一页面会永远重复处理。捕获错误可以让事务继续，但失败项在抛出异常前自身的写入仍会保留，因此应在单项边界处捕获错误，并确保该处的写入幂等或可安全重试。
• 只有在能够证明故障影响整个批次（共享的下游服务无法访问）时，才可以因失败提前退出；单项错误绝不应导致提前退出。如果整轮处理的调用方需要获知失败情况，就应捕获、记录并继续处理，然后在整轮结束后重新抛出第一个错误。
• 即使做了隔离，也要留意排序陷阱：如果成功项仍留在候选集合中（例如每次重试时都重新快照阻塞项之前的所有内容），扫描逻辑还必须在选择时排除已完成的工作。
• 可接受的写法：逐项使用 try/catch，并在结果中包含 failed；按项键控的 Promise.allSettled；捕获、记录并继续处理，然后在整轮结束后重新抛出。
• 错误写法：在每小时运行的 cron 中使用 for (const ws of candidates) { await archive(ws); await mark(ws) }——一台无法访问的 VM 会永远冻结其后的所有候选项。
• 正确写法：在同一个循环中将每项的整个处理逻辑放入 try/catch，把 { workspaceId, error } 推入 failed 数组，并将该数组返回给调用方。

25. 重命名用户看到的内容时，也要全面检查测试依赖的内容

• 面向用户的文案、可访问名称、路由和 data-* 属性，对于未纳入门禁流程的测试层至关重要，例如线上预发布环境测试框架、端到端旅程测试套件和外部监控。即便重命名后所有本地门禁都通过，也可能在几小时后悄然破坏他人会话中的测试。
• 如果变更重命名了可见文案或重构了界面，就应在整个仓库中 grep 旧字符串／选择器——明确包括测试框架和 e2e 软件包——并在同一轮修改中更新所有锚点。重命名的同时就要全面检查。
• 审查时可以这样验证：从差异中挑选一个已重命名的字符串，并在整个仓库中 grep。差异之外任何仍然存在的匹配项，都可能在下一次线上运行时导致故障。
• 结构优于文案：旅程测试需要锚点时，应优先在界面上使用自有的 data-* 属性，而不是显示文本——这样产品文案就可以自由变更，无需触碰测试。如果旅程测试仍以文案作为锚点，而界面上已有结构化属性，就应指出这一问题。

26. 让代码变得比你接手时更好——整洁的差异不值得以仓库凌乱为代价

• 绝不要为了让差异聚焦而撤销顺带带来的改进。 如果格式化工具、代码检查工具或代码转换工具也清理了你的改动未涉及的文件，也要把这些改动提交（如果改动较多，可以单独提交）。撤销这些改动，只是为了让差异在审查时看起来更整洁，却会让之后每次处理都付出代价，而且同样的问题还会被再次撤销。仓库的健康比差异的整洁更重要。
• 发现死代码，就在这次处理时清理。 无用的列、参数、函数、导出项，或唯一调用者是测试的测试——都应立即删除，包括任何为其辩护的注释。不要把它记下来；备注只是推迟处理，下一位读者还得重新确认它已经无用。
• 发现错误注释，就是发现了缺陷。 应立即修正。错误注释会比本可修正它的差异存续更久，还会积极误导他人——比没有注释更糟。
• 修复改动暴露出的潜在错误。 如果某项工作让一个很少运行的路径变成每次都会运行，就会暴露顺序错误、无法执行的清理逻辑，以及从未触发过的断言。这就是你的错误：它因你的改动而变得可触达。
• 改进之前先验证。 删除或重写之前，先确认目标确实无用、错误或损坏——搜索调用者，运行测试。误删关键内容的后果，远比你正在修复的凌乱严重。
• 限制条件：不要把无关的行为变更悄悄塞进功能提交，也不要因为不喜欢相邻模块的结构就重写它。本规则涵盖机械性整理、死代码、错误注释，以及你让其变得可触达的错误——不包括趁机重新设计。
• 审查者的判断标准：这次改动合并后，仓库里是否有任何文件比改动前更糟，或包含已知错误的陈述？如果有，这次处理就还没完成。

你的任务

审查：$ARGUMENTS

如果未提供参数，则审查 git diff --staged 或 git diff（未暂存的改动）。

对于发现的每个问题，请注明文件和行号。按类别分组。最后给出干净／不干净的结论。