halops-code-review

# 代码规范 Review 本 skill 审查 **本次改动** 是否符合仓库规范，以"先出方案、确认后再改"的两阶段方式执行。 skill 本身不记录任何具体的文件路径、目录、算子名或常量。仓库的规范和约定每次都从仓库当前状态中重新发现，所以仓库结构或算子变化后，skill 不需要修改。 ## 规范来源 每次执行都按下面的顺序重新查找，找到哪些就用哪些，并在报告的"审查范围"里列出实际用到的来源： 1. **编码规范**：本目录的 `code_standards.md`，即 HALOps 的 CUDA/C++ 编码规则，每次都要读取，规则编号以该文件为准。 2. **仓库规范文档**：贡献指南、风格指南。用 glob 在仓库根目录和 `docs/` 下查找（文件名常见如 `*standard*`、`CONTRIBUTING*`、`STYLE*`、`*guide*`），不要假设固定的文件名。 3. **工具配置**：`.pre-commit-config.yaml`、`pyproject.toml` 或 `setup.cfg` 里的 lint 配置、`.clang-format`、`.editorconfig`。 4. **CI 配置**：`.github/workflows/` 或其他 CI 目录里的 lint、测试步骤，以及固定的工具版本。 5. **已有代码**：与改动最相似的现有实现，即下文的"对照基线"。规范文档没写、但已有代码一致遵守的写法，按仓库约定对待。已有代码彼此不一致时，不作为问题报，只放进"需要用户决定的事项"。 6. **用户约束**：用户在对话中说过的约束和偏好（例如"某目录先不动""检查只留最关键的"），优先于以上所有来源。 本目录的 `checklist.md` 只列审查维度和发现约定的方法，不包含具体规则。具体规则以 `code_standards.md` 和仓库文件为准。 ## 硬性约束 - 阶段一 **只读**：不修改任何仓库文件，不 commit，不 push，不 stash，不切分支。pre-commit 只在临时 worktree 里跑，用完就删。 - 未经用户在本轮对话里明确确认，**不得进入阶段二**。"看起来没问题"不算确认。 - 任何阶段都不 commit / push，除非用户明确要求。 - 不 kill 不是自己启动的进程；仓库可能有其他会话在编译或测试。 - 仓库处于 rebase、merge、cherry-pick 中途，或有未解决的冲突时，先告诉用户，不要在这种状态下跑会改文件的工具。 ## 阶段一：Review 与修改方案 ### 1. 确定审查范围 按以下优先级确定 diff，并在报告开头写明实际使用的范围和命令： 1. 用户指定了 commit、区间、分支或 PR（如 `HEAD~3..HEAD`、`origin/main...HEAD`）时，按用户指定的来。 2. 工作区有未提交改动时，审查已暂存、未暂存和未跟踪的改动： - 已暂存与未暂存：`git diff HEAD` - 未跟踪：`git ls-files --others --exclude-standard`，排除构建产物和 `.claude/reviews/` 3. 工作区干净时，审查最近一个 commit：`git show HEAD`。 先用 `--stat` 看规模，再用 `-U0` 拿到每个文件的改动行号。 - **重命名**：用 `-M` 识别。纯重命名只检查名字和引用是否已全部更新，不重新审查内容。 - **删除或改名**：用旧名字在全仓 grep，包括构建脚本、语言绑定注册、包导出、文档、测试、benchmark，结果应该为 0。 ### 2. 读取规范和上下文 - 读取上一节找到的规范来源。 - 每个改动文件都读完整的改动 hunk，并补读理解所需的上下文：所在函数和类的完整定义，调用方和被调用的 helper。 - **对照基线**：为每个新增或大改的文件找 1~2 个最相似的现有实现。找法： - 同目录的兄弟文件，或命名前缀、后缀相同的文件； - grep 改动里的关键结构（入口函数的写法、绑定注册、基类或模板名），找其他使用者。 基线用来判断入口写法、检查方式、命名风格、注释密度、测试写法是否和仓库一致。 - **跨层一致**：改动涉及对外接口时，用函数名 grep 出整条调用链（Python 封装、语言绑定、C++/CUDA 入口、测试、文档）。各层的参数顺序、默认值、语义要一致。 ### 3. 审查 逐项对照 `checklist.md` 的维度，具体规则按规范来源判断。审查原则： - **只报改动引入的问题。** 问题所在的行必须是本次新增或修改的，或者是本次改动导致的。例如删了函数但还有引用；新写的 helper 与已有的重复；改了接口但文档、测试没有同步。 - **未改动代码里的既有问题不进入修改方案。** 确实严重的（正确性问题），放进报告末尾"范围外观察"，最多 5 条，只列不改。 - **只报读代码核实过的问题。** 推断出来、但没有完全核实的，标记为"需确认"，并写明还缺什么证据。 - **引用规则要写出处。** 写成"规范文档名 + 条目编号或标题"，或者"对照基线 `文件:行号`"。 - **格式问题以 pre-commit 结果为准。** 人工审查时不逐条报 formatter 能自动处理的问题。 **改动规模较大时并行审查**：超过约 1500 行或 10 个文件时，按目录或模块拆成 2~4 组，交给并行的只读子 agent。每个子 agent 都要带上规范来源、`checklist.md`、改动行号范围和"只报改动引入的问题"这条原则。汇总前，P0 条目由你亲自复核代码，确认后才写进报告。 ### 4. pre-commit 格式检查 仓库有 `.pre-commit-config.yaml`、并且有 Python 环境时必须做这一步。没有该配置文件时跳过，在报告里写明。 **确定版本 `$PCV`** - 先在 CI 配置里找固定的版本：`grep -rn "pre-commit==" .github/ <其他 CI 目录>`。 - 找不到时，看 `.pre-commit-config.yaml` 里的 `minimum_pre_commit_version`。 - 都没有就问用户一次。不要自己选版本。 **确定解释器 `$PY`** - 用户之前指定过的解释器（对话里说过或记忆里有记录）直接沿用；没有就问一次。 - 用户表示没有 Python 环境时跳过本步，在报告里写明"未执行：无 Python 环境"。 **确认版本** - `$PY -m pre_commit --version` 的输出必须是 `$PCV`。不要直接用 PATH 里的 `pre-commit`，它的版本可能不同。 - 未安装或版本不对时，先征得用户同意，再执行 `$PY -m pip install pre-commit==$PCV`。用户不同意就跳过本步，并在报告里写明原因。 **在临时 worktree 里执行** 阶段一不能改用户的工作区，而 formatter 类 hook 会原地改文件，所以要在临时 worktree 里跑： ```bash REPO=$(git rev-parse --show-toplevel) TMPROOT=$(mktemp -d); WT="$TMPROOT/wt" # 审查工作区改动：以 HEAD 为基础；审查 commit 或区间：用区间终点，例如 git worktree add --detach "$WT" <end-ref> git -C "$REPO" worktree add --detach "$WT" HEAD # 仅在审查工作区改动时执行：把被审查文件的工作区版本（含未跟踪文件）拷进去，已删除的文件不拷 for f in $FILES; do mkdir -p "$WT/$(dirname "$f")"; cp "$REPO/$f" "$WT/$f"; done cd "$WT" && git add -A $PY -m pre_commit run --files $FILES > "$TMPROOT/pre-commit.out" 2>&1 git diff --stat; git diff # hook 自动修复产生的改动 cd "$REPO" && git worktree remove --force "$WT" && git worktree prune && rm -rf "$TMPROOT" ``` - **检查范围**：`$FILES` 只包含被审查的文件，不包括已删除的文件。不要用 `--all-files`。 - **清理**：执行前后各跑一次 `git status --porcelain` 并对比，确认用户的工作区没有变化。无论成功还是失败，都要删除临时 worktree。 - **hook 环境安装失败**：hook 环境第一次安装需要联网。装不上时在报告里列出没跑成的 hook，不要换其他版本绕过去。 **结果怎么写进报告** - **可自动修复的**（输出里写 "files were modified by this hook" 的 hook）：合并成一条 `P1-fmt`。列出涉及的文件、对应的 hook 和 `git diff --stat`。修改方案是：阶段二在原工作区对这些文件执行同一条 pre-commit 命令。如果自动修复改到了本次没有改动的行，要在报告里说明，并放进"需要用户决定的事项"。 - **不能自动修复的**（hook 失败但没有改文件，例如 lint 报错、yaml 检查、密钥检测、大文件检测）：每条单独列为问题，写明 `文件:行号` 和报错内容。"未定义名字"这类可能是真实 bug，要读代码确认，确认属实的归入 P0。 - **全部通过**：写一句"pre-commit `$PCV` 全部通过"，并列出跑过的 hook。 ### 5. 输出报告 报告写入 `.claude/reviews/review-<YYYYMMDD-HHMM>-<short-sha>.md`。 - 第一次使用时，先确认 `.claude/reviews/` 已经被 git 忽略。如果没有，把它追加到 `.git/info/exclude`（只影响本地），不要改 `.gitignore`。 - 对话里给出摘要，并附上报告路径。 报告结构： ```markdown ## 审查范围 <diff 命令、涉及文件数/行数、实际用到的规范来源、对照基线、未审查的部分及原因> ## 结论 <2~4 句：整体情况、最需要先处理的问题> ## pre-commit 检查 <解释器路径、pre-commit 版本及来源、检查的文件数；各 hook 结果；未执行时写原因> ## 问题与修改方案 ### P0 正确性 / 健壮性 | ID | 位置 | 问题（证据） | 规则出处 | 修改方案 | 状态 | ### P1 规范（规则违反、死代码、魔数、重复、命名、include、格式） ### P2 可读性 / 注释 / 文档 / 单测 ## 范围外观察（不在本次修改方案内） ## 需要用户决定的事项 <方案有多种取舍、已有代码彼此不一致、或可能与用户既定约束冲突的条目> ``` - **ID**：用 `P0-1`、`P1-3` 这样的格式，方便用户按 ID 确认或反馈。 - **位置**：写成 `文件:行号`。 - **状态**：取值为"已核实"或"需确认"。 - **修改方案**：写到能直接执行的程度，说明改哪里、怎么改、是否影响数值或性能。有多种方案时给出推荐项。 ### 6. 等待用户确认 报告发出后**停止**，询问用户：是全部按方案改还是只改部分 ID；对哪些方案有修改意见；是否自己改。 - **用户给了反馈**：在原报告上修订对应条目，并标注"已按反馈修订"，把修订后的方案再给用户确认，直到用户明确确认。 - **用户说自己改，或者说不用改了**：到此结束，不进入阶段二。 - **用户只确认了部分条目**：阶段二只改这些条目。 ## 阶段二：应用已确认的方案 只有在用户明确确认之后才执行。 ### 1. 准备 - 重新执行 `git status`。如果阶段一之后工作区有变化（比如别人提交了代码），先对比报告里涉及的位置是否还成立。不成立的条目先停下来告诉用户。 - 编译、测试和 pre-commit 都使用阶段一确定的 `$PY` 和 `$PCV`。 - **找构建和测试命令**：从规范文档、构建脚本（`build.sh`、`Makefile`、`setup.py`、`CMakeLists.txt` 等）、测试配置（`pyproject.toml`、`pytest.ini`、`conftest.py`）和 CI 配置里找，不要假设。找不到时问用户。 - **避免并发编译**：编译前检查有没有其他进程在编译同一个仓库，例如 `ps aux | grep -E "<构建脚本名>|ninja|make|nvcc"`。如果有，告诉用户，不要和它并发编译，否则构建目录里会混进不同环境编出来的产物。 ### 2. 按条目修改 - **范围**：只改用户确认过的条目，不顺手修其他问题。改的过程中新发现的问题，记下来放到最后的汇报里。 - **顺序**：先 P0，再 P1，最后 P2。 - **风格**：代码风格要和对照基线一致，注释遵循仓库的风格，不要写修改记录式的注释。 - **纯重构**：去重、拆函数、改名、具名化常量这类改动，不能改变数值行为。 - **需确认的条目**：动手前先读代码确认问题确实存在。如果确认不存在，不改，并在汇报里写明理由。 ### 3. 验证 依次执行： 1. 在原工作区执行 `$PY -m pre_commit run --files <改动文件>`。hook 自动修复了文件时，再执行一次，确认全部 Passed。如果用户确认了 `P1-fmt`，这一步同时就完成了它。 2. 改了需要编译的代码时，用上一步找到的构建命令编译。如果出现链接或 ABI 类错误（例如 undefined symbol），先确认构建环境是否一致，必要时清掉构建目录后重新编译。 3. 跑测试： - 先跑与改动直接相关的测试。用改动的函数名、模块名在测试目录里 grep，找出相关的测试文件。 - 如果改的是公共头文件、公共 helper 或构建配置，跑全量测试。 4. 有纯重构类条目时，对受影响接口在改动前后的输出做逐位比较（例如 `torch.equal`）。改动前的基准用另一个 worktree 编译原版本来得到，不要用 stash。 5. 可能影响性能的改动（kernel 内部结构、访存方式、launch 配置），如果仓库有 benchmark 工具，用对应配置做前后对比；没有就说明没有做性能对比。 验证失败时：先修。如果同一种思路失败了两次，停下来分析根因，并告诉用户，不要一直反复小修小补。 ### 4. 汇报 - 更新报告文件：每个条目的状态改成"已修改"、"未修改（原因）"或"确认非问题（理由）"。 - 在对话里给出简短结论，包括： - 改了哪些 ID； - 跳过了哪些，以及跳过的原因； - 实际跑了哪些检查和测试，结果分别是什么； - 哪些检查没跑，以及没跑的原因； - 修改过程中新发现的问题。 - 不要 commit。如果用户要求提交，提交说明按规范文档里的提交格式写。