diff --git a/.claude/skills/issue-verification-workflow/SKILL.md b/.claude/skills/issue-verification-workflow/SKILL.md new file mode 100644 index 00000000..77ac5edf --- /dev/null +++ b/.claude/skills/issue-verification-workflow/SKILL.md @@ -0,0 +1,427 @@ +# Skill: issue-verification-workflow + +# Issue 驱动的全流程验证与修复流水线 + +端到端的工作流，从 GitHub Issue/PR 抓取开始，经过定位、复现验证、修复、回归验证， +最终输出结构化验证报告并提交代码。 + +适用场景： +- 用户说 "验证我的 PR #253" +- 用户说 "帮我修 issue #238" +- 用户说 "查一下有哪些待修的 bug，逐个处理" +- 用户提供一批 issue/commit 需要系统性验证 + +--- + +## Phase 0 — 环境感知 + +### 0.1 识别运行模式 + +判断当前是**本地开发**还是**远程服务器验证**： + +| 信号 | 本地模式 | 远程模式 | +|------|---------|---------| +| SSH 地址提供 | 无 | 有 (如 `ssh root@115.120.47.8`) | +| 容器配置提供 | 无 | 有 (如 `docker exec lmcache-p2p-pd-csy`) | +| NPU 设备可用 | 本地有 NPU | 远程有 NPU | + +**远程模式**下，所有测试命令通过 `ssh {host} "docker exec {container} bash -c '...'"` 执行。 +代码修改优先在本地进行，然后 `rsync` 到远程。 + +### 0.2 收集环境信息 + +在远程模式下，首先收集： + +```bash +# 硬件 +ssh {host} "npu-smi info" # NPU 型号/数量 +ssh {host} "docker exec {container} python --version" # Python 版本 + +# 软件栈 +ssh {host} "docker exec {container} pip list | grep -iE 'lmcache|vllm|torch'" + +# 代码状态 +ssh {host} "cd /path/to/LMCache-Ascend && git log --oneline -10" +``` + +### 0.3 确定目标范围 + +从用户输入中提取： +- **Issues**: 用 `webfetch` 获取详情 +- **PR**: 查看包含哪些 commits → 提取每个 commit 对应的 issue +- **Commits**: 直接被给定的 sha 列表 + +--- + +## Phase 1 — 抓取与理解 Issue + +### 1.1 获取 Issue 内容 + +```bash +# 通过 GitHub API 获取 issue/PR +webfetch https://github.com/LMCache/LMCache-Ascend/pull/{num} +webfetch https://github.com/LMCache/LMCache-Ascend/issues/{num} +``` + +### 1.2 提取关键信息 + +对每个 issue 提取： + +| 字段 | 说明 | +|------|------| +| **标题** | 问题的一句话描述 | +| **根因** | 为什么出错 | +| **影响文件** | 哪些 .py 文件需要改 | +| **修复方式** | 代码具体改了什么 | +| **影响类型** | 正确性 / 性能 / 日志 / 清理 | + +### 1.3 获取实际 Diff + +```bash +git log --oneline HEAD~10 # 找到相关 commit +git show {commit} --stat # 看改了哪些文件 +git diff {base}..{commit} -- {file} # 看具体 diff +``` + +### 1.4 分类 Issue + +| 类型 | 验证策略 | +|------|---------| +| **性能** (host blocking, timing, throughput) | 微基准测试 + 运行时指标抓取 | +| **正确性** (索引 bug, 逻辑错误, dead state) | 源代码格式审计 + 断言 | +| **日志** (格式修正, 语义澄清) | 源码格式审计 + 运行时 grep | +| **清理** (dead code removal) | grep 确认不存在 | +| **兼容性** (API signature, version) | import 测试 + 运行时检查 | + +--- + +## Phase 2 — 设计验证方案 + +### 2.1 为每个 issue 创建验证条目 + +每个 issue 需要以下维度的验证（至少选择一种）： + +``` +Issue #N: +├── 源码级 (Source Audit) +│ ├── grep 确认目标代码存在/不存在 +│ └── 格式/结构检查 +├── 单元级 (Unit Test) +│ ├── import + 基础调用 +│ └── 断言关键属性 +├── 性能级 (Benchmark) +│ ├── 微基准：循环计时，对比 before/after +│ └── 集成指标：抓取运行时 log 中的 timing 字段 +├── 集成级 (Integration) +│ ├── 启动服务 → 确认无 ERROR +│ ├── 发送请求 → 确认响应正确 +│ └── 检查日志 → 确认关键 log 出现 +└── 回归级 (Regression) + └── 确认修复未引入新问题 +``` + +### 2.2 性能 Issue 专项验证模板 + +```python +import torch, time, inspect + +# 1. 源码审计 +with open("target_file.py") as f: + src = f.read() +assert "expected_pattern" in src, "FAIL: pattern not found" + +# 2. 微基准（跑在真实 NPU 上） +n_iters = 2000 +# Old behavior simulation +torch.npu.synchronize() +t0 = time.perf_counter() +for _ in range(n_iters): + old_behavior() +t_old = (time.perf_counter() - t0) / n_iters * 1e6 + +# New behavior simulation +torch.npu.synchronize() +t0 = time.perf_counter() +for _ in range(n_iters): + new_behavior() +t_new = (time.perf_counter() - t0) / n_iters * 1e6 + +print(f"Old: {t_old:.1f} us/op New: {t_new:.1f} us/op Speedup: {t_old/t_new:.0f}x") +assert t_old / max(t_new, 0.001) > threshold, "FAIL: speedup below threshold" +``` + +### 2.3 正确性/日志 Issue 验证模板 + +```python +with open("target_file.py") as f: + src = f.read() + +# 确认旧错误模式已移除 +assert "bad_pattern" not in src, "FAIL: bad pattern still present" + +# 确认新正确模式已加入 +assert "good_pattern" in src, "FAIL: correct pattern not found" + +# 检查注释/说明 +assert "NOTE(#N)" in src, "FAIL: explanatory comment missing" +``` + +--- + +## Phase 3 — 执行验证 + +### 3.1 同步代码到目标环境 + +```bash +# 远程模式：rsync 本地代码到服务器 +ssh {host} "rsync -a --delete --exclude='build' --exclude='__pycache__' \ + /path/to/local/LMCache-Ascend/ /path/to/remote/LMCache-Ascend/" +``` + +### 3.2 依次执行验证条目 + +按优先级：源码级 → 单元级 → 性能级 → 集成级 + +每个验证条目明确输出 `PASS` 或 `FAIL` 及具体数据。 + +### 3.3 集成测试（如需要） + +当 issue 涉及运行时行为时，需要启动完整服务： + +```bash +# 1. 创建配置文件 +cat > config.yaml << EOF +chunk_size: 256 +local_cpu: True +max_local_cpu_size: 50 +internal_api_server_enabled: True +internal_api_server_host: "0.0.0.0" +internal_api_server_port_start: 5999 +EOF + +# 2. 启动服务 +docker exec -d {container} bash -c " + export LMCACHE_CONFIG_FILE=/path/to/config.yaml + export ASCEND_RT_VISIBLE_DEVICES=4,5,6,7 + export VLLM_ENABLE_V1_MULTIPROCESSING=1 + python -m vllm.entrypoints.openai.api_server \ + --port 7100 --model /path/to/model \ + --tensor-parallel-size 4 --trust-remote-code \ + --kv-transfer-config '{\"kv_connector\":\"{connector}\",...}' \ + > /path/to/logs/vllm.log 2>&1 & +" + +# 3. 等待就绪（轮询 /health，最多等待 60 秒） +TIMEOUT=60 +INTERVAL=5 +ELAPSED=0 +while ! curl -s -o /dev/null -w '%{http_code}' http://localhost:7100/health | grep -q 200; do + if [ $ELAPSED -ge $TIMEOUT ]; then + echo "ERROR: Server failed to start within $TIMEOUT seconds" + exit 1 + fi + sleep $INTERVAL + ELAPSED=$((ELAPSED + INTERVAL)) +done + +# 4. 发送测试请求 +curl -s http://localhost:7100/v1/chat/completions -H "Content-Type: application/json" \ + -d '{"model":"...","messages":[{"role":"user","content":"..."}],"max_tokens":10}' + +# 5. 检查日志 +grep -E "LMCache|offload_total_time|ERROR|Traceback" /path/to/logs/vllm.log +``` + +### 3.4 监控关键指标 + +| 指标 | 命令 | 含义 | +|------|------|------| +| `/health` 返回 | `curl -s -o /dev/null -w '%{http_code}'` | 服务是否存活 | +| HBM 使用率 | `npu-smi info -t usages -i {id}` | 模型是否加载 | +| AICore 使用率 | 同上 | 是否有推理负载 | +| ERROR 计数 | `grep -c ERROR {log}` | 应用级错误 | +| Traceback 计数 | `grep -c Traceback {log}` | Python 异常 | + +--- + +## Phase 4 — 生成验证报告 + +### 4.1 报告输出路径 + +所有报告输出到当前工作目录下的 `output/` 目录： + +``` +{workspace}/output/{PR|ISSUE}_{num}_verification_report.md +{workspace}/output/{PR|ISSUE}_{num}_manual_verification_guide.md +``` + +### 4.2 报告结构模板 + +#### 验证报告 (`verification_report.md`) + +```markdown +# {PR/Issue} #{num} 测试验证报告 + +> PR: {url} +> 标题: {title} +> 测试日期: {date} +> 测试环境: {hardware + software details} + +## 一、摘要 +{1-2 paragraph summary of all findings} + +## 二、测试环境 +{Table: HW, NPU, CANN, Container, Python, vLLM, LMCache, Model, TP} + +## 三、变更清单与验证结果 +{Table: commit | file | change | verification method | result} + +## {For performance issues:} 性能专项验证 +{Per-issue breakdown: source audit + benchmark data} + +## 四、vLLM 集成测试 +{Startup timeline, inference results, KV cache stats} + +## 五、健康检查 +{ERROR/Traceback counts, WARNING audit, NPU status} + +## 六、结论 +{Overall result + recommendations} +``` + +#### 手动验证指南 (`manual_verification_guide.md`) + +```markdown +# {PR/Issue} #{num} 手动验证操作指南 + +## 环境 +{Connection info, container, commit} + +## 步骤 1: 确认代码版本 +{bash commands + expected output} + +## 步骤 N: 每个验证条目 +{command block + expected output + pass condition} + +## 验证清单 +{Checklist table: #, step, item, pass condition} +``` + +### 4.3 数据要求 + +报告中所有数据必须来自**真实环境测试**，不得编造： + +- ✅ 性能数据：显示具体 iteration 数、元素数、耗时 +- ✅ 日志摘录：引用实际 `grep` 输出 +- ✅ 推理结果：引用实际 `curl` 响应的 `content` 字段 +- ✅ NPU 状态：引用 `npu-smi` 输出 + +--- + +## Phase 5 — 清理与提交 + +### 5.1 停止测试服务 + +```bash +ssh {host} "docker exec {container} pkill -9 -f vllm.entrypoints" +``` + +### 5.2 确认无残留 + +```bash +ssh {host} "docker exec {container} ps aux | grep python | grep -v grep" +``` + +### 5.3 提交代码（如需要） + +```bash +git add {changed_files} +git commit -m "{type}({scope}): {description} (#{issue_number})" +git push origin {branch} +``` + +Commit message 格式遵循项目规范： +- `fix({scope}): {description} (#{issue})` +- `feat({scope}): {description}` +- `chore({scope}): {description}` + +### 5.4 清理临时文件 + +```bash +rm -f /tmp/verify_*.py # 本地临时脚本 +rm -rf {workspace}/output/ # 报告（可选保留） +``` + +--- + +## Phase 6 — 流程总结输出 + +验证完成后，用简洁的表格向用户汇报： + +| 阶段 | 内容 | 状态 | +|------|------|------| +| Phase 1 | Issue 抓取与分析 | ✅ | +| Phase 2 | 验证方案设计 | ✅ | +| Phase 3 | 执行验证 | ✅ N/N PASS | +| Phase 4 | 生成报告 | ✅ output/xxx.md | +| Phase 5 | 清理 | ✅ | + +--- + +## 快速参考卡片 + +### 常用命令速查 + +```bash +# Issue 查询 +webfetch https://github.com/LMCache/LMCache-Ascend/issues/{num} +webfetch https://github.com/LMCache/LMCache-Ascend/pull/{num} + +# Diff 查询 +git log --oneline -10 +git diff {base}..{head} -- {file} +git show {commit} --stat + +# 远程执行 +ssh {host} "docker exec {container} bash -c '...'" +ssh {host} "docker exec {container} python3 -c '...'" + +# 文件传输 +scp /tmp/script.py {host}:/tmp/ +ssh {host} "docker cp /tmp/script.py {container}:/tmp/" +rsync -a --exclude='build' local/ remote/ + +# 日志检查 +grep -c "ERROR" {log} +grep -c "Traceback" {log} +grep "LMCache" {log} | grep -v WARNING +``` + +### Issue 类型（不局限于以下几类） → 验证方法映射 + +| Issue 类型 | 最小验证 | 推荐验证 | 可选验证 | +|-----------|---------|---------|---------| +| pin_memory 添加 | 源码 grep | 微基准 host blocking time | — | +| 索引 bug 修复 | 源码 grep 旧模式移除 | import + 静态检查逻辑 | 端到端推理 | +| 日志格式修正 | 源码 grep 新旧格式 | — | 运行时 grep log | +| dead code 移除 | 源码 grep 确认不存在 | — | — | +| stream sync 修复 | 源码 grep 新模式 | import 测试 | — | +| timing 修正 | 源码 grep + 注释检查 | 运行时 grep timing 值 | 基准对比 | + +--- + +--- + + + +### Templates + +| File | Purpose | +|------|---------| +| `templates/verification_report_template.md` | 验证报告标准模板 (6 章节) | +| `templates/manual_verification_guide_template.md` | 手动验证操作指南模板 | + +--- + +Base directory for this skill: file:///Users/yhl/Project/LMCache-Ascend/.claude/skills/issue-verification-workflow +Relative paths in this skill (e.g., scripts/, reference/) are relative to this base directory. diff --git a/.claude/skills/issue-verification-workflow/scripts/verify_issue.py b/.claude/skills/issue-verification-workflow/scripts/verify_issue.py new file mode 100644 index 00000000..0f99381a --- /dev/null +++ b/.claude/skills/issue-verification-workflow/scripts/verify_issue.py @@ -0,0 +1,267 @@ +#!/usr/bin/env python3 +""" +LMCache-Ascend Issue Verification Runner + +Utility that automates source-level verification of issue fixes. +Run on the target environment (local or container). + +Usage: + python3 verify_issue.py --issue-type pin_memory --file lmcache_ascend/v1/npu_connector/npu_connectors.py + python3 verify_issue.py --issue-type offload_time --file lmcache_ascend/v1/cache_engine.py + python3 verify_issue.py --issue-type dead_code --file lmcache_ascend/v1/storage_backend/p2p_backend.py --pattern local_lookup_cache + python3 verify_issue.py --issue-type index_fix --file lmcache_ascend/v1/blend/blender.py --bad-pattern "recomp_ratios[0]" +""" + +import argparse +import sys +import time +from pathlib import Path + +# Try to import torch for performance tests (optional) +try: + import torch + import torch_npu + HAS_TORCH = True +except ImportError: + HAS_TORCH = False + +# Base path for LMCache-Ascend source +DEFAULT_BASE = "/mnt/sdb/csy/p2p/LMCache-Ascend" + + +def read_file(filepath: str, base: str = DEFAULT_BASE) -> str: + """Read source file, returning contents.""" + path = Path(base) / filepath + if not path.exists(): + print(f"FAIL: File not found: {path}") + sys.exit(1) + return path.read_text() + + +# --------------------------------------------------------------------------- +# Issue-specific verification functions +# --------------------------------------------------------------------------- + +def verify_pin_memory(filepath: str, base: str = DEFAULT_BASE) -> dict: + """Verify #238: pin_memory() + non_blocking=True in npu_connectors.""" + src = read_file(filepath, base) + results = {} + + # Source audit + results["pin_memory_found"] = ".pin_memory()" in src + results["non_blocking_found"] = "non_blocking=True" in src + + # Count occurrences in _initialize_pointers via line analysis + pin_count = src.count(".pin_memory()") + nb_count = src.count("non_blocking=True") + results["pin_memory_count"] = pin_count + results["non_blocking_count"] = nb_count + results["pin_memory_branches"] = "all 4" if pin_count >= 4 else f"{pin_count}/4" + + # Micro-benchmark (if torch available) + if HAS_TORCH: + try: + n_iters = 2000 + n_elem = 96 + pinned = torch.empty(n_elem, dtype=torch.int64, device="cpu").pin_memory() + unpinned = torch.empty(n_elem, dtype=torch.int64, device="cpu") + npu_p = torch.empty(n_elem, dtype=torch.int64, device="npu") + npu_u = torch.empty(n_elem, dtype=torch.int64, device="npu") + + torch.npu.synchronize() + t0 = time.perf_counter() + for _ in range(n_iters): + npu_u.copy_(unpinned) + t_old = (time.perf_counter() - t0) / n_iters * 1e6 + + torch.npu.synchronize() + t0 = time.perf_counter() + for _ in range(n_iters): + npu_p.copy_(pinned, non_blocking=True) + t_new = (time.perf_counter() - t0) / n_iters * 1e6 + + results["benchmark_old_us"] = round(t_old, 1) + results["benchmark_new_us"] = round(t_new, 1) + results["benchmark_speedup"] = round(t_old / max(t_new, 0.001), 0) + results["is_pinned"] = pinned.is_pinned() + except Exception as e: + results["benchmark_error"] = str(e) + + results["passed"] = ( + results.get("pin_memory_found") and + results.get("non_blocking_found") and + results.get("pin_memory_count", 0) >= 4 and + results.get("non_blocking_count", 0) >= 1 + ) + return results + + +def verify_offload_time(filepath: str, base: str = DEFAULT_BASE) -> dict: + """Verify #233: offload_time log format correction.""" + src = read_file(filepath, base) + results = {} + + results["old_label_removed"] = "offload_time: %.4f ms, put_time" not in src + results["new_label_present"] = "offload_total_time" in src + results["breakdown_present"] = "process_tokens: %.4f ms, from_gpu" in src + results["comment_present"] = "NOTE(#233)" in src and "wait_for_forward" in src + + results["passed"] = ( + results["old_label_removed"] and + results["new_label_present"] and + results["breakdown_present"] and + results["comment_present"] + ) + return results + + +def verify_dead_code(filepath: str, bad_pattern: str, base: str = DEFAULT_BASE) -> dict: + """Verify dead code removal.""" + src = read_file(filepath, base) + results = {} + + results["pattern_absent"] = bad_pattern not in src + + results["passed"] = results["pattern_absent"] + return results + + +def verify_index_fix(filepath: str, bad_pattern: str, base: str = DEFAULT_BASE) -> dict: + """Verify hardcoded index fix.""" + src = read_file(filepath, base) + results = {} + + results["bad_pattern_removed"] = bad_pattern not in src + + results["passed"] = results["bad_pattern_removed"] + return results + + +def verify_stream_sync(filepath: str, expected_pattern: str, base: str = DEFAULT_BASE) -> dict: + """Verify stream synchronization fix (e.g., store_stream wrapping).""" + src = read_file(filepath, base) + results = {} + + results["pattern_found"] = expected_pattern in src + + results["passed"] = results["pattern_found"] + return results + + +# --------------------------------------------------------------------------- +# Display helpers +# --------------------------------------------------------------------------- + +def print_results(issue_type: str, results: dict): + """Pretty-print verification results.""" + print(f"\n{'='*55}") + print(f"Verification: {issue_type}") + print(f"{'='*55}") + + for key, value in results.items(): + if key == "passed": + continue + if key.startswith("benchmark_"): + if key == "benchmark_error": + print(f" [WARN] Benchmark failed: {value}") + else: + print(f" {key}: {value}") + else: + status = "✓" if value else "✗" + print(f" [{status}] {key}: {value}") + + passed = results.get("passed", False) + print(f"\n Overall: {'PASSED' if passed else 'FAILED'}") + print(f"{'='*55}") + return passed + + +# --------------------------------------------------------------------------- +# CLI +# --------------------------------------------------------------------------- + +def main(): + parser = argparse.ArgumentParser( + description="LMCache-Ascend Issue Verification Runner" + ) + parser.add_argument( + "--issue-type", required=True, + choices=["pin_memory", "offload_time", "dead_code", "index_fix", "stream_sync"], + help="Type of issue to verify" + ) + parser.add_argument( + "--file", required=True, + help="Relative path of target file (from lmcache-ascend root)" + ) + parser.add_argument( + "--pattern", default=None, + help="Pattern to check for dead_code or stream_sync" + ) + parser.add_argument( + "--bad-pattern", default=None, + help="Bad pattern to check against for index_fix" + ) + parser.add_argument( + "--base", default=DEFAULT_BASE, + help="Base directory of lmcache-ascend source" + ) + parser.add_argument( + "--all", action="store_true", + help="Run all checks for PR #253" + ) + + args = parser.parse_args() + + if args.all: + # Run all PR #253 checks + checks = { + "pin_memory": lambda: verify_pin_memory("lmcache_ascend/v1/npu_connector/npu_connectors.py", args.base), + "offload_time": lambda: verify_offload_time("lmcache_ascend/v1/cache_engine.py", args.base), + "dead_code_p2p": lambda: verify_dead_code( + "lmcache_ascend/v1/storage_backend/p2p_backend.py", "local_lookup_cache", args.base), + "index_fix_blend": lambda: verify_index_fix( + "lmcache_ascend/v1/blend/blender.py", "recomp_ratios[0]", args.base), + "stream_sync_ms": lambda: verify_stream_sync( + "lmcache_ascend/mindspore/v1/npu_connector.py", "store_stream", args.base), + } + + all_passed = True + for name, check_fn in checks.items(): + results = check_fn() + if not print_results(name, results): + all_passed = False + + if all_passed: + print("\n✓ All checks PASSED") + else: + print("\n✗ Some checks FAILED") + sys.exit(1) + return + + # Single check + if args.issue_type == "pin_memory": + results = verify_pin_memory(args.file, args.base) + elif args.issue_type == "offload_time": + results = verify_offload_time(args.file, args.base) + elif args.issue_type == "dead_code": + if not args.pattern: + print("ERROR: --pattern required for dead_code check") + sys.exit(1) + results = verify_dead_code(args.file, args.pattern, args.base) + elif args.issue_type == "index_fix": + if not args.bad_pattern: + print("ERROR: --bad-pattern required for index_fix check") + sys.exit(1) + results = verify_index_fix(args.file, args.bad_pattern, args.base) + elif args.issue_type == "stream_sync": + if not args.pattern: + print("ERROR: --pattern required for stream_sync check") + sys.exit(1) + results = verify_stream_sync(args.file, args.pattern, args.base) + + if not print_results(args.issue_type, results): + sys.exit(1) + + +if __name__ == "__main__": + main() diff --git a/.claude/skills/issue-verification-workflow/templates/manual_verification_guide_template.md b/.claude/skills/issue-verification-workflow/templates/manual_verification_guide_template.md new file mode 100644 index 00000000..edc2747d --- /dev/null +++ b/.claude/skills/issue-verification-workflow/templates/manual_verification_guide_template.md @@ -0,0 +1,57 @@ +# Manual Verification Guide Template + +# PR/Issue #{{NUM}} 手动验证操作指南 + +## 环境 + +| 项目 | 值 | +|------|-----| +| 服务器 | `ssh {{SSH_HOST}}` | +| 工作路径 | `{{WORK_PATH}}` | +| 目标容器 | `{{CONTAINER}}` | +| 待验证 commit | `{{COMMIT}}` | + +--- + +## 步骤 1：确认代码版本 + +```bash +{{VERSION_CHECK_CMD}} +``` + +**预期输出**: +``` +{{VERSION_CHECK_OUTPUT}} +``` + +--- + +## 步骤 2：同步代码到容器 + +```bash +{{SYNC_CMD}} +``` + +**预期结果**: 容器内 git log 与宿主一致，HEAD = `{{COMMIT}}`。 + +--- + +{{STEPS}} + +--- + +## 步骤 N+1：清理 + +```bash +{{CLEANUP_CMD}} +``` + +**预期输出**: `{{CLEANUP_OUTPUT}}` + +--- + +## 验证清单 + +| # | 步骤 | 验证项 | 通过条件 | +|---|------|--------|---------| +{{CHECKLIST_TABLE}} diff --git a/.claude/skills/issue-verification-workflow/templates/verification_report_template.md b/.claude/skills/issue-verification-workflow/templates/verification_report_template.md new file mode 100644 index 00000000..9fbec559 --- /dev/null +++ b/.claude/skills/issue-verification-workflow/templates/verification_report_template.md @@ -0,0 +1,120 @@ +# Verification Report Template + +> **PR/Issue**: #{{NUM}} +> **URL**: {{URL}} +> **标题**: {{TITLE}} +> **作者**: {{AUTHOR}} +> **测试日期**: {{DATE}} +> **测试人员**: 自动化验证 (真实 Ascend NPU 环境) + +--- + +## 一、摘要 + +{{1-2 paragraph summary of what was tested and the overall result}} + +--- + +## 二、测试环境 + +| 项目 | 详情 | +|------|------| +| **服务器** | `{{SSH_HOST}}` | +| **硬件平台** | {{HW_PLATFORM}} | +| **NPU** | {{NPU_SPEC}} | +| **容器** | `{{CONTAINER}}` ({{IMAGE}}) | +| **CANN** | {{CANN_VERSION}} | +| **Python** | {{PYTHON_VERSION}} | +| **vLLM** | {{VLLM_VERSION}} | +| **LMCache (上游)** | {{LMCACHE_UPSTREAM_VERSION}} | +| **LMCache-Ascend** | {{LMCACHE_ASCEND_VERSION}}, commit `{{COMMIT}}` | +| **模型** | {{MODEL}} | +| **TP 配置** | {{TP_CONFIG}} | + +### LMCache 运行配置 + +```yaml +{{LMCACHE_CONFIG_YAML}} +``` + +--- + +## 三、变更清单与验证结果 + +| # | Commit | 变更文件 | 变更说明 | 验证方式 | 结果 | +|---|--------|---------|---------|---------|------| +{{CHANGES_TABLE}} + +--- + +## {For Performance Issues} 三-B、性能相关 Issue 专项验证 + +### #{{ISSUE_NUM}} — {{ISSUE_TITLE}} + +**问题**: {{PROBLEM_DESCRIPTION}} + +**修复**: {{FIX_DESCRIPTION}} + +**验证方法**: {{METHOD}} + +**验证结果**: + +| 指标 | 修复前 | 修复后 | 改进 | +|------|--------|--------|------| +{{METRICS_TABLE}} + +--- + +## 四、集成测试 + +### 4.1 启动时间线 + +``` +{{STARTUP_TIMELINE}} +``` + +### 4.2 推理测试结果 + +| # | 请求 | 响应 | prompt_tokens | completion_tokens | HTTP | +|---|------|------|---------------|-------------------|------| +{{INFERENCE_TABLE}} + +### 4.3 LMCache 指标 + +| 请求 | Total Tokens | Engine Computed | LMCache Hit | Need Load | +|------|-------------|-----------------|-------------|-----------| +{{LMCACHE_METRICS}} + +--- + +## 五、健康检查 + +### 5.1 错误/异常 + +| 类别 | 数量 | 说明 | +|------|------|------| +| **ERROR** | {{ERROR_COUNT}} | {{ERROR_DESC}} | +| **Traceback** | {{TRACEBACK_COUNT}} | {{TRACEBACK_DESC}} | +| **WARNING** | {{WARNING_COUNT}} | {{WARNING_DESC}} | + +### 5.2 NPU 状态 + +``` +{{NPU_STATUS}} +``` + +--- + +## 六、结论 + +### 6.1 总体评价 + +{{OVERALL_ASSESSMENT}} + +### 6.2 建议 + +{{RECOMMENDATIONS}} + +--- + +*报告由 LMCache-Ascend issue-verification-workflow 自动生成。测试时间: {{TEST_TIME}}* diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md new file mode 100644 index 00000000..431131af --- /dev/null +++ b/.claude/skills/pr-review/SKILL.md @@ -0,0 +1,513 @@ +--- +name: pr-review +description: Pull Request 审查工作流，从 PR 上下文采集、分层审查（规范/正确性/安全/架构/测试），到输出结构化审查报告。覆盖 Go 并发、文件权限、错误处理、i18n、核心层解耦等高频风险领域。 +license: MIT +compatibility: opencode +metadata: + audience: code reviewers, maintainers, contributors + output: markdown review report (file-scoped findings + verdict) + language: Go / TypeScript (cc-connect primary stack) +--- + +# Skill: pr-review + +# Pull Request 审查流水线 + +端到端的 PR 审查工作流，从采集 PR 上下文（diff / 描述 / 关联 issue / CI） +开始，经过**分层审查**（规范、正确性、安全、架构、测试、文档）， +最终输出一份可执行的结构化审查报告，并给出 `Approve / Request changes / Comment` +结论与逐条 finding。 + +本 skill 的审查准则提炼自 cc-connect 项目的真实 PR 实践（参考 PR #1436 并发修复、 +#1433 文件权限修复、#1425 错误处理、#1384 飞书卡片、#1349 会话隔离 等）。 + +适用场景： +- 用户说 "审查我的 PR #1436" +- 用户说 "review this pull request" +- 用户说 "帮我 review 一下这个 diff" +- 用户给出 PR URL 或本地分支名，需要系统性审查 + +--- + +## Phase 0 — 环境感知与目标确认 + +### 0.1 确定审查目标 + +从用户输入中识别以下任一形态： + +| 输入形态 | 解析方式 | +|---------|---------| +| PR URL | `webfetch` 抓取 conversation / commits / files | +| PR 编号 `#1436` | 组装 URL 后 `webfetch` | +| 本地分支名 | `git log main..HEAD` + `git diff main...HEAD` | +| 已 checkout 的工作区 | 直接 `git status` + `git diff` | +| 一组 commit sha | `git show {sha}` 逐个分析 | + +### 0.2 识别目标仓库的约定 + +在动手审查前，先确认项目约定（优先级从高到低）： + +1. `AGENTS.md` / `CONTRIBUTING.md` / `REVIEW.md` — 项目自述的审查规范 +2. `.github/CODEOWNERS` — 谁是必选审查人 +3. `.github/pull_request_template.md` — PR 描述必填项 +4. 近期已合并 PR 的 commit message 风格、描述章节 +5. `.golangci.yml` / `biome.json` / `.eslintrc` — 静态检查规则 + +> 若目标仓是 cc-connect，约定清单见 `templates/review_rubric.md` 的「cc-connect 项目专项约定」。 + +### 0.3 确认审查深度 + +向用户确认（或按默认）审查深度： + +| 深度 | 范围 | 适用 | +|------|------|------| +| **Quick** | diff 规范 + 显眼风险 | 小修小补、文档、chore | +| **Standard**（默认） | 全部六层，按风险加权 | 绝大多数功能/修复 PR | +| **Deep** | Standard + 并发/安全逐行 + 回归用例审计 | 涉及 core/、并发、鉴权、文件权限 | + +--- + +## Phase 1 — PR 上下文采集 + +目标：在审查前把「这个 PR 到底改了什么、为什么改、怎么验证的」一次性吃透。 + +### 1.1 抓取 PR 元信息 + +```bash +# 远程 PR（通过 GitHub） +webfetch https://github.com/{org}/{repo}/pull/{num} # conversation + 描述 +webfetch https://github.com/{org}/{repo}/pull/{num}/files # 改动文件清单 +webfetch https://github.com/{org}/{repo}/pull/{num}/checks # CI 状态 + +# 本地分支 +git log --oneline {base}..HEAD +git diff --stat {base}...HEAD +git diff {base}...HEAD +``` + +可使用 `scripts/collect_pr_context.sh` 自动汇总以上信息。 + +### 1.2 提取审查关键字段 + +对每个 PR 建立审查卡片： + +| 字段 | 说明 | 举例 | +|------|------|------| +| **类型** | feat / fix / refactor / chore / docs / perf | `fix(core)` | +| **Scope** | 改动所属模块 | core / claudecode / feishu / slack | +| **根因** | 为什么改（bug PR 必填） | goroutine 读 agentSession 未持锁 | +| **修复方式** | 代码层面具体怎么改 | 持锁捕获局部变量，nil 时返回 error | +| **In scope** | 本次改了哪些 | `writeTempAppendPromptFile` | +| **Out of scope** | 显式声明不改什么 | `ensureSharedSystemPromptFile`、daemon 路径 | +| **关联 issue** | Fixes / Refs / Related | `Fixes #1429`、Related #1072 | +| **测试** | 新增/回归测试名 + 全套耗时 | `TestNew_WorkDirDoesNotExist`；`./core/` 47.7s PASS | +| **风险声明** | 作者自评的安全/兼容影响 | chmod 0644 扩大可读范围但内容非密 | + +> 任何字段缺失都不是阻塞，但要在报告中以 `⚠ 描述缺章节` 形式标注， +> 提示作者补全 —— 这是 cc-connect 维护者审查时的常见反馈点。 + +### 1.3 构建改动地图 + +把 diff 按「文件 → hunk → 性质」组织，便于分层审查： + +``` +PR #1436 fix(core): goroutine race +├── core/session.go +│ ├── hunk 1 [正确性] Send() 内 agentSession 改为局部捕获 +│ └── hunk 2 [正确性] nil 检查 + 返回 error +├── core/session_test.go +│ └── hunk 1 [测试] 新增 race 场景用例 +└── go.mod (无变更) +``` + +性质标签取自下一节的六层模型。 + +--- + +## Phase 2 — 分层审查（核心） + +按以下六层逐层过 diff。**每一层都要有结论**：✅ 通过 / ⚠ 建议改进 / ❌ 必须修复 / ➖ 不适用。 +详细的逐项检查清单见 `templates/review_rubric.md`，本节给出每层的审查要点与高频反模式。 + +### 第 1 层：规范与元信息（Meta & Conventions） + +**目标**：PR 描述、commit message、文件归属是否符合项目约定。 + +检查项： +- [ ] Commit message 遵循 Conventional Commits：`type(scope): description` +- [ ] bug 修复 PR 的 commit body 说明根因，而非只说「修复了」 +- [ ] PR 描述含必要章节：Summary / Scope(In/Out) / Tests / Risk / Related +- [ ] `Fixes #N` / `Closes #N` 正确关联 issue +- [ ] 标题不超过 ~72 字符，使用英文祈使句或中文动宾结构 +- [ ] 作者 requested 正确的 CODEOWNERS 审查人 +- [ ] PR 不夹带无关改动（一个 PR 一个主题） + +**反模式**： +- `update code` / `fix bug` 这类无信息 commit message +- PR 描述只有一行「如题」 +- `Fixes #N` 写在标题但正文没展开根因 +- 一个 PR 同时做 feat + refactor + 升级依赖 + +### 第 2 层：正确性与逻辑（Correctness） + +**目标**：改动是否真的解决了问题、有没有引入新 bug。这是审查的**重心**。 + +#### 2.1 通用正确性 +- [ ] 边界条件：空值、零值、空切片、off-by-one、首次/末次迭代 +- [ ] 错误处理：error 是否被检查、是否被吞、是否 wrap（`fmt.Errorf("%w", err)`） +- [ ] 资源管理：`defer` 关闭、goroutine 泄漏、文件句柄 +- [ ] 类型断言 / 类型转换有无保护 +- [ ] 字符串拼接在循环中是否用 `strings.Builder` +- [ ] map 并发读写（Go） +- [ ] 时间比较用 `time.Time.Before/After`，不要比 int64 + +#### 2.2 Go 并发专项（cc-connect 高频风险） + +cc-connect 大量使用 goroutine 处理消息流，并发 bug 是 P1 高发区（参考 #1436）： + +- [ ] **共享变量在 goroutine 中被访问时是否持锁** —— 最高频反模式 + - 反例：`go func() { state.agentSession.Send(...) }()` 而 `cleanupInteractiveState` 会置 nil + - 正例：持锁捕获到局部变量，goroutine 内只用局部 +- [ ] `sync.Mutex` / `sync.RWMutex` 保护范围是否覆盖所有读写路径 +- [ ] 是否需要 `-race` 重跑（CI 默认开 `-race`？） +- [ ] channel 关闭方与发送方是否唯一 +- [ ] `context.Context` 是否正确传递与取消 +- [ ] `sync.WaitGroup` 的 `Add/Done/Wait` 是否配对 +- [ ] select 的 default 分支会不会忙等 + +```go +// ❌ 反模式（#1436 修复前） +go func() { + state.agentSession.Send(ctx, msg) // state.agentSession 可能已被 cleanup 置 nil +}() + +// ✅ 正确 +state.mu.Lock() +sess := state.agentSession +state.mu.Unlock() +if sess == nil { + return errors.New("agent session already cleaned up") +} +go func() { + sess.Send(ctx, msg) +}() +``` + +#### 2.3 错误处理专项（参考 #1425） + +- [ ] 不要返回 OS 级模糊错误（如 `fork/exec ... no such file`）让用户猜根因 +- [ ] 在能判断根因的位置加 preflight 检查，返回业务级清晰错误 +- [ ] 错误信息包含：是什么 + 在哪 + 怎么修（如 `claudecode: work_dir "/x" does not exist`） +- [ ] error chain 用 `%w` 保留原始错误，用 `%s`/`%v` 仅在需要脱敏时 +- [ ] 不要 `panic` 在库代码里，除非真的是不可恢复的编程错误 + +### 第 3 层：安全（Security） + +**目标**：改动是否引入新的攻击面或权限/数据泄露。 + +#### 3.1 文件与权限（参考 #1433） +- [ ] 新建文件/目录的 mode 是否最小化 + - 临时文件默认 `os.CreateTemp` 是 `0600`，跨用户读取需显式 `Chmod(0o644)` + - 配置/密钥类必须 `0600` +- [ ] `run_as_user` / `setuid` 场景下，跨用户读写权限是否对齐 +- [ ] 路径拼接是否防目录穿越（`filepath.Clean` / 拒绝 `..`） +- [ ] 文件内容是否含敏感信息（token、私钥、`.env`） + +#### 3.2 输入与输出 +- [ ] 外部输入（用户消息、webhook、配置文件）是否做校验与长度限制 +- [ ] SQL / Shell / HTML / URL 是否用参数化或转义 +- [ ] 反序列化是否限制大小、类型 +- [ ] 日志是否泄露敏感字段（token、password、PII） + +#### 3.3 鉴权与密钥 +- [ ] 新增 endpoint 是否走鉴权中间件 +- [ ] token / webhook secret 是否从环境变量或 secret manager 读，不硬编码 +- [ ] diff 中是否误提交 `.env`、`config.local.toml`、`*.key` +- [ ] 依赖更新是否引入已知 CVE（配合 `govulncheck` / `npm audit`） + +> 安全类 finding **默认定级 ≥ Major**，不允许「先合后修」。 + +### 第 4 层：架构与可维护性（Architecture） + +**目标**：改动是否破坏模块边界、是否增加未来维护成本。 + +#### 4.1 分层与耦合（cc-connect 专项） +- [ ] `core/` 不得硬编码具体平台名（feishu/slack/...）或 agent 名（claudecode/...） + - 必须通过 adapter 注册表 / 接口注入 + - 参见 #1384、#1424 等 adapter PR 的做法 +- [ ] 新增用户可见文案必须走 i18n，且**所有支持语言一并补齐** +- [ ] 配置项加到 `config.toml` 时同步更新 schema、默认值、文档 +- [ ] 公共 API 变更是否需要 deprecation 周期 + +#### 4.2 可维护性 +- [ ] 单函数行数 / 圈复杂度是否失控（> ~80 行 / > ~15 分支需警示） +- [ ] 是否重复造轮子（项目内已有 `writeFileAtomic` 就别再写一遍） +- [ ] 命名是否达意（`data` / `info` / `tmp` 这类无信息名要改） +- [ ] 注释是否解释 **为什么**，而非复述代码 +- [ ] TODO/FIXME 是否带 issue 号（`TODO(#1430): ...`） + +#### 4.3 性能 +- [ ] 热路径上的分配（box/unbox、string↔[]byte、map 预分配） +- [ ] O(n²) 循环、N+1 查询、全表扫描 +- [ ] 锁粒度是否过粗阻塞热路径 +- [ ] 是否引入不必要的同步 I/O 在请求路径 + +> 性能优化类 PR 要求附带 before/after 基准数据，否则标 `⚠ 缺基准`。 + +### 第 5 层：测试（Tests） + +**目标**：改动是否有充分的测试覆盖，且测试本身可信。 + +- [ ] bug 修复 PR 必须带**回归测试**（先复现再修，参考 #1425 的 `TestNew_WorkDirDoesNotExist`） +- [ ] 回归测试名能说明被测行为，不用 `TestFunc1` / `TestCase2` +- [ ] 测试是否真正断言了行为，而非只检查「没 panic」 +- [ ] 并发相关改动是否用 `t.Parallel()` + `-race` 验证 +- [ ] 测试不依赖外部环境（用 `t.TempDir()` 而非硬编码 `/tmp/xxx`，参考 #1425 修复的 7 个旧用例） +- [ ] mock/stub 是否泄漏到非测试代码 +- [ ] PR 描述是否给出全套测试耗时（cc-connect 惯例：`./core/ 47.7s PASS`） +- [ ] 是否删除/绕过了既有测试来「让 CI 过」 + +**反模式**： +```go +// ❌ 假测试：只确认没崩 +func TestSend(t *testing.T) { + s := New() + s.Send(msg) // 无断言 +} + +// ❌ 硬编码路径，换机器就挂 +dir := "/tmp/claudecode-test" + +// ✅ 真测试 +func TestSend_AfterCleanupReturnsError(t *testing.T) { + s := newState() + s.cleanup() + err := s.Send(ctx, msg) + require.Error(t, err) + require.Contains(t, err.Error(), "already cleaned up") +} +``` + +### 第 6 层：文档与变更日志（Docs & Changelog） + +- [ ] 用户可见行为变更是否更新 README / docs / CHANGELOG +- [ ] 新增配置项是否在 `config.example.toml` 体现 +- [ ] 新增命令/flag 是否更新 `--help` 文本与文档 +- [ ] 截图/GIF（如 UI 改动）是否附在 PR 描述 +- [ ] 破坏性变更是否在 PR 描述显式标注 `BREAKING CHANGE` + +--- + +## Phase 3 — 风险评估与定级 + +对每个 finding 定级，决定是否阻塞合并： + +| 级别 | 含义 | 是否阻塞 | 处理 | +|------|------|---------|------| +| 🟥 **Blocker** | 会导致数据损坏/安全事故/核心功能不可用 | 是 | 必须 Request changes | +| 🟧 **Major** | 安全类、正确性 bug、破坏既有功能 | 是 | 必须 Request changes | +| 🟨 **Minor** | 边界未覆盖、测试不足、命名不佳 | 视情况 | 建议 Request changes 或 Approve + comment | +| 🟩 **Nit** | 风格、注释、微优化 | 否 | Approve + 顺手提建议 | +| ℹ️ **Info** | 表扬、提问、知识点分享 | 否 | Approve + comment | + +**定级原则**： +1. 安全类默认 ≥ Major，不妥协 +2. 正确性类按「触发概率 × 影响面」定级 +3. 测试缺失：bug 修复缺回归 = Major；新功能缺测试 = Minor +4. 文档/规范类一般 Minor 及以下 +5. 主分支保护范围内（如 `core/`）的架构问题从严 + +--- + +## Phase 4 — 输出审查报告 + +### 4.1 报告路径 + +``` +{workspace}/output/pr_{num}_review.md +``` + +### 4.2 报告结构 + +使用 `templates/pr_review_report_template.md`，核心结构： + +```markdown +# PR #{num} 审查报告 + +> 标题 / 作者 / 仓库 / 审查日期 / 审查深度 + +## 一、结论 +{Approve | Request changes | Comment} —— 一句话理由 + +## 二、改动概览 +{文件数 / 增删行 / 类型 / scope / 关联 issue} + +## 三、分层审查结论 +| 层 | 结论 | 主要发现 | +|----|------|---------| +| 规范 | ✅ | ... | +| 正确性 | ⚠ | ... | +| ... | ... | ... | + +## 四、Findings（逐条） +{每条：定位 file:line / 级别 / 问题 / 建议 / 可选 patch} + +## 五、亮点（必有，至少 1 条） +{肯定作者做对的地方} + +## 六、合并前必须解决 / 建议改进 / 可选 +``` + +### 4.3 Finding 写作规范 + +每条 finding 必须可执行、可定位、可验证： + +```markdown +### F1 — Send goroutine 未持锁读 agentSession 🟥 Blocker +**位置**：core/session.go:142 +**问题**：goroutine 内直接读 state.agentSession，与 cleanupInteractiveState 置 nil 存在竞态。 +**影响**：agent 退出时高概率 nil panic，P1。 +**建议**：持锁捕获到局部变量后传入 goroutine，nil 时返回 error。 +**参考**：cc-connect #1436 的同型修复。 +```patch +- go func() { state.agentSession.Send(ctx, msg) }() ++ state.mu.Lock() ++ sess := state.agentSession ++ state.mu.Unlock() ++ if sess == nil { return errSessionGone } ++ go func() { sess.Send(ctx, msg) }() +``` +``` + +写作要点： +1. **先定位**：`file:line` 必须精确 +2. **先说是什么，再说为什么坏**：避免上来就评判 +3. **给可执行建议**：最好附带 patch，而非「请优化」 +4. **引用先例**：项目内类似修复的 PR 号、issue 号 +5. **区分事实与推测**：推测要标「我推测…，请确认」 + +### 4.4 评论语气 + +- **对事不对人**：「这段循环是 O(n²)，n=10k 时会卡」，而非「你写错了」 +- **先肯定后建议**：至少留 1 条亮点；大改动先肯定工作量 +- **疑问优先于否定**：不确定时用「这里是否考虑过 X？」 +- **尊重作者判断**：Minor 以下可加「如果你同意我再改，不同意可忽略」 + +--- + +## Phase 5 — 提交审查结果 + +### 5.1 回填到 GitHub（如用户授权） + +```bash +# 通过 gh CLI 提交 review（需要用户确认） +gh pr review {num} \ + --request-changes \ + --body-file output/pr_{num}_review.md + +# 或仅提交单条 inline comment +gh api repos/{org}/{repo}/pulls/{num}/comments \ + -f body="..." -f commit_id=... -f path=... -F line=... +``` + +> ⚠ **未经用户明确同意，不要执行任何 `gh pr review` / 评论 / 合并命令。** +> 默认只生成报告文件，由用户自行决定如何提交。 + +### 5.2 汇报给用户 + +审查完成后，用简洁表格向用户汇报： + +| 阶段 | 内容 | 状态 | +|------|------|------| +| Phase 1 | 上下文采集 | ✅ N 文件 / M 增删 | +| Phase 2 | 分层审查 | ✅ 6 层完成 | +| Phase 3 | 风险评估 | ⚠ 2 Major / 1 Minor | +| Phase 4 | 报告生成 | ✅ output/pr_1436_review.md | +| Phase 5 | 提交 | ⏸ 待用户确认 | + +--- + +## 快速参考卡片 + +### 常用命令 + +```bash +# PR 上下文 +webfetch https://github.com/{org}/{repo}/pull/{num} +git diff {base}...HEAD --stat +git log --oneline {base}..HEAD + +# 本地一键采集（本 skill 提供） +bash scripts/collect_pr_context.sh {base} + +# Go 专项 +go build ./... +go test -race ./... +go vet ./... +gofmt -l . +govulncheck ./... + +# 提交审查（需用户确认） +gh pr review {num} --approve --body-file output/pr_{num}_review.md +``` + +### 改动类型 → 重点审查层映射 + +| PR 类型 | 必查层 | 重点 | +|---------|-------|------| +| `fix(core)` 并发 | 正确性(2.2) + 测试 | 锁、goroutine、回归用例 | +| `fix` 文件/权限 | 安全(3.1) | mode、跨用户、路径穿越 | +| `fix` 错误处理 | 正确性(2.3) | 错误信息清晰度、preflight | +| `feat` 新 adapter | 架构(4.1) | core 不硬编码、i18n | +| `feat` 新 endpoint | 安全(3.3) + 测试 | 鉴权、输入校验 | +| `refactor` | 正确性 + 架构 + 测试 | 行为不变、测试不动 | +| `chore` 依赖升级 | 安全(3.3) | CVE、breaking | +| `docs` | 规范 + 文档 | 仅元信息层 | +| `perf` | 性能(4.3) + 测试 | 必须带基准数据 | + +### 阻塞 vs 非阻塞 决策树 + +``` +finding 涉及安全？ ─是─→ Blocker/Major，必须 Request changes + │否 + ├─ 涉及正确性？ ─是─→ 按概率×影响定级，通常 Major + │ 缺回归测试 → Major + │否 + ├─ 涉及 core/ 主干？ ─是─→ 从严，Minor 也倾向 Request changes + │否 + ├─ bug 修复 PR 缺测试？ ─是─→ Major + │否 + └─ 规范/风格/文档 → Nit/Minor，Approve + comment +``` + +--- + +## Bundled Resources + +### Templates + +| File | Purpose | +|------|---------| +| `templates/pr_review_report_template.md` | 结构化审查报告标准模板（含分层结论 + Findings 表） | +| `templates/review_rubric.md` | 六层审查的完整检查清单 + cc-connect 项目专项约定 | + +### Scripts + +| File | Purpose | +|------|---------| +| `scripts/collect_pr_context.sh` | 一键汇总本地 PR 的 diff/stat/log/CI 信息，供审查消费 | + +--- + +## When to Use This Skill + +- 用户给你一个 PR（URL / 编号 / 分支名 / diff）并要求 review +- 用户说「帮我审一下这个改动」/「这个 PR 有什么问题」 +- 你作为 maintainer 需要对 contributor 的 PR 给出结构化反馈 +- 团队希望统一 PR 审查的维度、定级、报告格式 + +## When NOT to Use + +- 只是本地写代码时想 lint（用 lint 工具直接跑） +- 用户只是想理解某段代码做什么（直接讲解即可，不必走完整流水线） +- PR 仅含自动生成的文件变更（lockfile、vendor 等），走 Quick 之外的深度无意义 diff --git a/.claude/skills/pr-review/scripts/collect_pr_context.sh b/.claude/skills/pr-review/scripts/collect_pr_context.sh new file mode 100755 index 00000000..84e45b45 --- /dev/null +++ b/.claude/skills/pr-review/scripts/collect_pr_context.sh @@ -0,0 +1,148 @@ +#!/usr/bin/env bash +# collect_pr_context.sh — 一键汇总本地 PR 的 diff/stat/log/CI 信息，供 pr-review skill 消费。 +# +# 用法： +# bash collect_pr_context.sh [BASE_REF] [HEAD_REF] [OUTPUT_DIR] +# +# 默认： +# BASE_REF = origin/main +# HEAD_REF = HEAD +# OUTPUT_DIR = ./output/pr_context_ +# +# 产出： +# output/pr_context_/ +# ├── 00_summary.txt # 一句话概览（commit 数、增删行、文件数） +# ├── 01_commits.txt # BASE..HEAD 的 commit 列表 +# ├── 02_stat.txt # 每文件增删行 +# ├── 03_files.txt # 变更文件清单 +# ├── 04_full_diff.txt # 完整 diff（审查主输入） +# ├── 05_messages.txt # 各 commit 的完整 message +# └── 06_meta.txt # 仓库元信息 / branch / 是否 dirty +# +# 远程 PR 采集请用： +# gh pr view {num} --json title,body,files,commits,baseRefName,headRefName +# gh pr diff {num} +# gh pr checks {num} +# +# 退出码： +# 0 成功 +# 1 参数错误 / 不在 git 仓库 +# 2 base ref 不存在 + +set -euo pipefail + +BASE_REF="${1:-origin/main}" +HEAD_REF="${2:-HEAD}" +OUTPUT_DIR="${3:-./output/pr_context_$(date +%Y%m%d_%H%M%S)}" + +log() { printf '[collect] %s\n' "$*" >&2; } +die() { printf '[collect][ERROR] %s\n' "$*" >&2; exit "${2:-1}"; } + +# --- 前置检查 --------------------------------------------------------------- +command -v git >/dev/null || die "git not found" +git rev-parse --is-inside-work-tree >/dev/null 2>&1 || die "not in a git repo" + +# 检查 ref 是否存在；origin/main 可能需要 fetch +if ! git rev-parse --verify --quiet "${BASE_REF}^{commit}" >/dev/null; then + log "base ref '${BASE_REF}' 未找到，尝试 git fetch origin ..." + git fetch --quiet origin 2>/dev/null || true + git rev-parse --verify --quiet "${BASE_REF}^{commit}" >/dev/null \ + || die "base ref '${BASE_REF}' 不存在，请先 git fetch 或指定正确的 base" 2 +fi +git rev-parse --verify --quiet "${HEAD_REF}^{commit}" >/dev/null \ + || die "head ref '${HEAD_REF}' 不存在" + +mkdir -p "${OUTPUT_DIR}" +log "output → ${OUTPUT_DIR}" + +MERGE_BASE="$(git merge-base "${BASE_REF}" "${HEAD_REF}")" +log "merge-base = ${MERGE_BASE}" + +# --- 00 summary ------------------------------------------------------------- +{ + echo "repo: $(git rev-parse --show-toplevel 2>/dev/null || echo '?')" + echo "base ref: ${BASE_REF} ($(git rev-parse --short "${BASE_REF}"))" + echo "head ref: ${HEAD_REF} ($(git rev-parse --short "${HEAD_REF}"))" + echo "merge-base: $(git rev-parse --short "${MERGE_BASE}")" + echo "commits: $(git rev-list --count "${MERGE_BASE}..${HEAD_REF}")" + echo "files: $(git diff --name-only "${MERGE_BASE}..${HEAD_REF}" | wc -l | tr -d ' ')" + git diff --shortstat "${MERGE_BASE}..${HEAD_REF}" | sed 's/^/diff: /' + echo "dirty: $(if [ -n "$(git status --porcelain)" ]; then echo yes; else echo no; fi)" + echo "generated: $(date -u +%Y-%m-%dT%H:%M:%SZ)" +} > "${OUTPUT_DIR}/00_summary.txt" + +# --- 01 commits ------------------------------------------------------------- +git log --oneline --no-decorate "${MERGE_BASE}..${HEAD_REF}" \ + > "${OUTPUT_DIR}/01_commits.txt" + +# --- 02 stat ---------------------------------------------------------------- +git diff --stat "${MERGE_BASE}..${HEAD_REF}" \ + > "${OUTPUT_DIR}/02_stat.txt" + +# --- 03 files --------------------------------------------------------------- +git diff --name-status "${MERGE_BASE}..${HEAD_REF}" \ + > "${OUTPUT_DIR}/03_files.txt" + +# --- 04 full diff ----------------------------------------------------------- +# 约束行宽，便于审查工具读取 +git diff --find-renames --find-copies "${MERGE_BASE}..${HEAD_REF}" \ + > "${OUTPUT_DIR}/04_full_diff.txt" + +# --- 05 commit messages ----------------------------------------------------- +{ + while IFS= read -r sha; do + [ -z "${sha}" ] && continue + echo "============================================================" + git show --no-patch --format='commit %H%nAuthor: %an <%ae>%nDate: %ad%n%n%B' "${sha}" + done < <(git rev-list --reverse "${MERGE_BASE}..${HEAD_REF}") +} > "${OUTPUT_DIR}/05_messages.txt" + +# --- 06 meta ---------------------------------------------------------------- +{ + echo "## branch info" + echo "current branch: $(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo '?')" + echo + echo "## remote" + git remote -v + echo + echo "## top 10 recently touched files in diff (churn signal)" + git log --name-only --pretty=format: "${MERGE_BASE}..${HEAD_REF}" \ + | grep -v '^$' | sort | uniq -c | sort -rn | head -n 10 + echo + echo "## files by extension" + git diff --name-only "${MERGE_BASE}..${HEAD_REF}" \ + | awk -F. 'NF>1{print $NF} NF==1{print "(no-ext)"}' \ + | sort | uniq -c | sort -rn +} > "${OUTPUT_DIR}/06_meta.txt" + +log "done. files:" +ls -1 "${OUTPUT_DIR}" | sed 's/^/ /' >&2 + +# --- 可选：远程 PR 增强（需要 gh 且在 GitHub 仓） -------------------------- +if command -v gh >/dev/null 2>&1; then + REMOTE_URL="$(git config --get remote.origin.url 2>/dev/null || true)" + case "${REMOTE_URL}" in + *github.com*) + log "检测到 GitHub remote，尝试用 gh 拉取 PR 列表（若已 push）" + CURRENT_BRANCH="$(git rev-parse --abbrev-ref HEAD 2>/dev/null || true)" + if [ -n "${CURRENT_BRANCH}" ] && [ "${CURRENT_BRANCH}" != "HEAD" ]; then + # 找到当前分支对应的 open PR（若有） + PR_NUM="$(gh pr list --head "${CURRENT_BRANCH}" --state open \ + --json number --jq '.[0].number' 2>/dev/null || true)" + if [ -n "${PR_NUM}" ]; then + log "发现 PR #${PR_NUM}，补充远程视图" + { + echo "## PR #${PR_NUM} (auto-detected from branch ${CURRENT_BRANCH})" + gh pr view "${PR_NUM}" --json title,author,body,baseRefName,headRefName,labels \ + 2>/dev/null || echo "(gh pr view failed)" + echo + echo "## CI checks" + gh pr checks "${PR_NUM}" 2>/dev/null || echo "(no checks)" + } > "${OUTPUT_DIR}/07_github_pr.txt" + fi + fi + ;; + esac +fi + +log "all done." diff --git a/.claude/skills/pr-review/templates/pr_review_report_template.md b/.claude/skills/pr-review/templates/pr_review_report_template.md new file mode 100644 index 00000000..90f441dc --- /dev/null +++ b/.claude/skills/pr-review/templates/pr_review_report_template.md @@ -0,0 +1,125 @@ +# PR #{NUM} 审查报告 + +> **标题**: {PR 标题} +> **作者**: @{author} +> **仓库**: {org}/{repo} +> **审查日期**: {YYYY-MM-DD} +> **审查深度**: Quick / Standard / Deep +> **PR 链接**: {url} + +--- + +## 一、结论 + +| 维度 | 结论 | +|------|------| +| **总体裁决** | {Approve / Request changes / Comment} | +| **一句话理由** | {例如：核心并发修复正确且带回归测试，仅需补一处锁粒度} | +| **阻塞项数** | 🟥 Blocker: {n}　🟧 Major: {n}　🟨 Minor: {n}　🟩 Nit: {n} | + +--- + +## 二、改动概览 + +| 项 | 值 | +|----|----| +| 类型 / Scope | {例如 fix(core)} | +| 文件变更 | {n} files (+{a} / -{d}) | +| 关联 Issue | {Fixes #N / Related #M} | +| CI 状态 | {✅ all green / ❌ X failing} | +| 根因（bug PR） | {一句话根因} | +| 修复方式 | {一句话怎么改的} | + +**改动地图**： + +``` +{file1} +├── hunk 1 [正确性] ... +└── hunk 2 [测试] ... +{file2} +└── hunk 1 [安全] ... +``` + +--- + +## 三、分层审查结论 + +| # | 层 | 结论 | 关键发现摘要 | +|---|----|------|-------------| +| 1 | 规范与元信息 | {✅/⚠/❌/➖} | {...} | +| 2 | 正确性与逻辑 | {✅/⚠/❌/➖} | {...} | +| 3 | 安全 | {✅/⚠/❌/➖} | {...} | +| 4 | 架构与可维护性 | {✅/⚠/❌/➖} | {...} | +| 5 | 测试 | {✅/⚠/❌/➖} | {...} | +| 6 | 文档与变更日志 | {✅/⚠/❌/➖} | {...} | + +图例：✅ 通过　⚠ 建议改进　❌ 必须修复　➖ 不适用 + +--- + +## 四、Findings（逐条） + +> 每条 finding 遵循：定位 → 问题 → 影响 → 建议 → 可选 patch。 +> 按级别从高到低排列。 + +### F1 — {问题一句话标题} {级别图标} {Blocker/Major/Minor/Nit/Info} + +- **位置**: `{file}:{line}` +- **类型**: {正确性 / 安全 / 并发 / 性能 / 测试 / 规范 / 文档} +- **问题**: {客观描述这段代码做了什么、与预期有何偏差} +- **影响**: {触发条件 × 后果；若是性能问题给数据} +- **建议**: {可执行的修复方向} +- **参考**: {项目内同类修复 PR/issue 号，或业界做法} + +```diff +- {原代码} ++ {建议代码} +``` + +- [ ] 作者回应：（待填写） + +--- + + + +--- + +## 五、亮点 + +> 必填至少 1 条。肯定作者做对的地方，建立正向反馈。 + +- 👍 {例如：持锁捕获局部变量的修复方式与 #1436 一致，是处理这类竞态的正解} +- 👍 {例如：回归测试 TestXxx 先复现后修复，且断言清晰} +- 👍 {例如：PR 描述的 In/Out scope 非常明确，审查时省去大量判断成本} + +--- + +## 六、合并清单 + +### 合并前必须解决（Blocker / Major） +- [ ] {对应 F 编号 + 一句话} +- [ ] {...} + +### 建议改进（Minor，不阻塞但强烈推荐） +- [ ] {...} + +### 可选（Nit，作者可忽略） +- [ ] {...} + +### 已通过的检查（确认项） +- [x] commit message 符合 Conventional Commits +- [x] 无 secrets / credentials +- [x] {其它本 PR 已做对的项} + +--- + +## 七、附：审查元数据 + +| 项 | 值 | +|----|----| +| 审查所依据的准则 | review_rubric.md v1 | +| 项目专项约定 | {cc-connect / 其它} | +| 本次审查覆盖的 commit | {sha 列表} | +| 工具辅助 | {go vet / govulncheck / 手工} | + + diff --git a/.claude/skills/pr-review/templates/review_rubric.md b/.claude/skills/pr-review/templates/review_rubric.md new file mode 100644 index 00000000..58ece092 --- /dev/null +++ b/.claude/skills/pr-review/templates/review_rubric.md @@ -0,0 +1,242 @@ +# Review Rubric — PR 审查完整检查清单 + +> 本文档是 `SKILL.md` Phase 2「分层审查」的配套细则。 +> 审查时按层逐项打勾；任何 ❌ 即对应一份 finding。 +> 准则版本：v1（提炼自 cc-connect PR #1436/#1433/#1425/#1384/#1349 等真实实践）。 + +--- + +## 定级速查 + +| 级别 | 图标 | 阻塞 | 典型场景 | +|------|------|------|---------| +| Blocker | 🟥 | 是 | 数据损坏、安全漏洞、核心功能不可用 | +| Major | 🟧 | 是 | 安全类、正确性 bug、破坏既有功能、bug 修复缺回归 | +| Minor | 🟨 | 视情况 | 边界未覆盖、测试不足、命名不佳、文档缺失 | +| Nit | 🟩 | 否 | 风格、注释、微优化 | +| Info | ℹ️ | 否 | 表扬、提问、知识点 | + +--- + +## 第 1 层：规范与元信息 + +### 1.1 Commit Message +- [ ] 遵循 Conventional Commits：`type(scope): description` + - type ∈ {feat, fix, refactor, perf, chore, docs, test, style, ci, build} + - scope 与模块名一致（core / claudecode / feishu / slack / web / api ...） +- [ ] 标题 ≤ ~72 字符，英文祈使句或中文动宾结构 +- [ ] bug 修复的 commit body 说明**根因**，而非只说「修复了」 +- [ ] `Fixes #N` / `Closes #N` 关联正确 issue +- [ ] 无 `WIP` / `tmp` / `final-final` 残留 +- [ ] Co-Authored-By 标注真实（如 AI 协作） + +### 1.2 PR 描述 +- [ ] 含 Summary（这个 PR 做了什么、为什么） +- [ ] bug 修复含 Root Cause（根因分析） +- [ ] 含 Scope：显式列出 In scope / Out of scope（cc-connect 强约定） +- [ ] 含 Tests：新增/回归测试名 + 全套耗时（如 `./core/ 47.7s PASS`） +- [ ] 含 Risk：作者自评的安全/兼容/性能影响 +- [ ] 含 Related：交叉引用相关 PR/issue +- [ ] 破坏性变更显式标注 `BREAKING CHANGE` +- [ ] UI 改动附截图/GIF + +### 1.3 PR 卫生 +- [ ] 一个 PR 一个主题（不夹带无关改动） +- [ ] requested 正确的 CODEOWNERS 审查人 +- [ ] CI 全绿（或失败项与本 PR 无关并已说明） +- [ ] 无冲突，已 rebase 到最新 base +- [ ] 无调试代码（`fmt.Println` / `console.log` / `dump()`） + +--- + +## 第 2 层：正确性与逻辑 + +### 2.1 通用正确性 +- [ ] 边界：空值 / 零值 / 空切片 / nil map 写入 / off-by-one +- [ ] 首次与末次迭代正确（for i:=0; i 安全类 finding 默认 ≥ Major，不允许「先合后修」。 + +--- + +## 第 4 层：架构与可维护性 + +### 4.1 分层与解耦（cc-connect 专项强约束） +- [ ] **`core/` 不硬编码平台名**（feishu/slack/discord/...） +- [ ] **`core/` 不硬编码 agent 名**（claudecode/codex/...） +- [ ] 新平台/agent 通过 adapter 注册表或接口注入（#1384、#1424 范式） +- [ ] 新增用户可见文案走 i18n，且**所有支持语言一并补齐** +- [ ] 配置项同步更新：`config.toml` schema + 默认值 + `config.example.toml` + 文档 +- [ ] 公共 API 变更评估 deprecation 周期 +- [ ] 不跨层调用（adapter 不直接被 core 反向依赖） + +### 4.2 可维护性 +- [ ] 单函数 ≤ ~80 行、圈复杂度 ≤ ~15（超出标 ⚠） +- [ ] 不重复造轮子（项目已有 `writeFileAtomic` 就别再写） +- [ ] 命名达意（`data` / `info` / `tmp` / `handler2` 要改） +- [ ] 注释解释**为什么**，不复述代码 +- [ ] TODO/FIXME 带 issue 号：`TODO(#1430): ...` +- [ ] 死代码、注释掉的大段代码移除 + +### 4.3 性能 +- [ ] 热路径避免不必要的分配（box/unbox、string↔[]byte） +- [ ] 循环内不重复计算可外提的值 +- [ ] map/slice 预分配（`make(map, n)` / `make([]T, 0, n)`） +- [ ] 锁粒度：不在持锁时做 I/O 或长计算 +- [ ] 请求路径无同步阻塞 I/O（应 async 或队列） +- [ ] perf 类 PR 必须带 before/after 基准数据 + +--- + +## 第 5 层：测试 + +### 5.1 覆盖与回归 +- [ ] bug 修复 PR 必须带**回归测试**（先复现再修，#1425 范式） +- [ ] 新功能有对应单测/集测 +- [ ] 回归测试名能说明被测行为（`TestSend_AfterCleanupReturnsError`） +- [ ] 测试真正断言行为，非仅检查「没 panic」 +- [ ] 并发改动用 `t.Parallel()` + `-race` +- [ ] 边界用例：空输入、单元素、超长输入、非法输入 + +### 5.2 测试质量 +- [ ] 不依赖外部环境（用 `t.TempDir()`，不硬编码 `/tmp/xxx`，#1425 修了 7 个此类） +- [ ] mock/stub 不泄漏到非测试代码 +- [ ] 测试可独立运行、可重复（无测试间隐式依赖） +- [ ] 不为「让 CI 过」删除/绕过既有测试 +- [ ] 表驱动测试的 case 命名达意 +- [ ] PR 描述给出全套测试耗时（cc-connect 惯例） + +### 5.3 测试反模式 +```go +// ❌ 假测试 +func TestX(t *testing.T) { + X() // 无断言 +} + +// ❌ 硬编码路径 +dir := "/tmp/claudecode-test" + +// ❌ 忽略错误 +_, _ = riskyCall() + +// ✅ 真测试 +func TestSend_AfterCleanupReturnsError(t *testing.T) { + s := newState(); s.cleanup() + err := s.Send(ctx, msg) + require.Error(t, err) + require.Contains(t, err.Error(), "already cleaned up") +} +``` + +--- + +## 第 6 层：文档与变更日志 + +- [ ] 用户可见行为变更 → 更新 README / docs +- [ ] 新增配置项 → 更新 `config.example.toml` + 文档 +- [ ] 新增命令/flag → 更新 `--help` 文本与文档 +- [ ] 破坏性变更 → CHANGELOG / MIGRATION 笔记 +- [ ] 新增公开 API → godoc / tsdoc 注释完整 +- [ ] UI 改动 → 截图/GIF 附 PR 描述 +- [ ] 删除/重命名公共 API → deprecation 提示 + +--- + +## cc-connect 项目专项约定 + +> 当目标仓为 chenhg5/cc-connect 时，以下为强约定，违反视同 Major。 + +### 提交与描述 +- Commit message 用 Conventional Commits，scope 准确（core/claudecode/feishu/slack/pi/web/api/...） +- bug 修复 PR 描述必含章节：**What / Scope(In/Out) / Why / Tests / Risk / Related** +- 参考范例：#1433（描述章节最完整）、#1425（含 CUJ 影响标注） + +### 代码约定 +- Go 代码须通过 `go build ./...`、`go test ./...`（并发类加 `-race`）、`go vet` +- 改动触及 `core/` 时禁止硬编码任何平台名/agent 名 +- 新增用户可见文案必须 i18n 并补齐所有语言 +- 临时文件跨用户读取需显式 `Chmod(0o644)`（#1433 范式） +- 错误信息须业务级清晰，不暴露 OS 级 fork/exec 类消息（#1425 范式） + +### 并发 +- goroutine 内访问共享状态必须持锁或先捕获局部（#1436 范式） +- 清理路径置 nil 后，所有读取方必须 nil-safe + +### 审查流程 +- 维护者用 CODEOWNERS 自动指派；PM 不跟踪 PR 状态，由 QA lane 负责合并 +- AGENTS.md 的 Pre-Commit Checklist 须满足 + +### 安全红线 +- 不得在源码硬编码 token / webhook secret / 用户凭据 +- `run_as_user` 路径下，文件权限须与跨用户读取契约一致 +- diff 不得包含 `.env` / `*.key` / 私钥 + +--- + +## 审查人自检（提交报告前） + +- [ ] 每条 finding 都有 `file:line` 精确定位 +- [ ] 每条 finding 都给可执行建议（最好附 patch） +- [ ] 报告含至少 1 条亮点 +- [ ] 区分「必须解决 / 建议 / 可选」三档 +- [ ] 没有把推测写成事实（推测要标「请确认」） +- [ ] 安全/正确性类已定级 ≥ Major +- [ ] 语气对事不对人，先肯定后建议 diff --git a/.gitignore b/.gitignore index 656bc1d6..bee27411 100644 --- a/.gitignore +++ b/.gitignore @@ -23,4 +23,4 @@ schedule.log # claude CLAUDE.md -.claude/ +!.claude/ \ No newline at end of file