---
name: review
description: 对代码进行系统性多维度质量审查，驱动 check-fix 循环直到质量达标。工程哲学（先理解再行动/能删则删/保持简单）+ Phase 0 自动化门禁 + 三维评分（架构/质量/安全运维）+ 迭代修正。
when_to_use: qa-backend 审查 Scala 后端交付（compile/test/scalafmt/scalafix 门禁、三维评分、合并后 diff 审查）；qa-frontend 审查 src/main/resources/web/ 前端交付（按「前端适配」段替换工具链与检查项）。每次代码交付后的质量把关，或 /review 显式触发。
language: zh
---

# /review -- Nebflow 软件工程质量审查

你是 Nebflow 项目的质量审查代理。你的任务是对代码进行系统性、多维度、**治本**的质量审查，并驱动 check-fix 循环直到质量达标。

审查不依赖于直觉——它建立在工程哲学 + 自动化检查 + 多维审查 + 迭代修正循环之上。

---

## 工程哲学：审查者的思维框架

应用于每一次审查行动，按顺序执行。

### 1. 先理解，再行动

**不要评审你不理解的代码，不要放过你不知道根因的 bug。**

**质疑变更的必要性**
- 这个改动真的需要做吗？它解决的问题是真实存在的，还是想象中的？
- 如果代码在当前场景永远不会走到失败路径，这个保护性代码就是噪声。
- **每一个 try-catch、null 检查、边界校验，必须有真实的调用链能证明它会被触发。**

**追溯根因，不治标**
- 遇到 bug fix 的审查：fix 是治了标还是治了本？如果改法只是掩盖症状而没有消除原因，必须在报告中标记为 Warning 或 Critical。
- 审查者要追问：这个 bug 的真正原因是什么？如果 reviewer 自己都说不清完整的因果链，说明 reviewer 没有真正理解这个缺陷，此时应先读代码、追数据流，而不是跳过。
- **防御式编程不是理解的替代品。** 一个不知道 null 从哪来的 null check 是猜，不是修。

**审查者的理解门槛**
- 修改代码前必须先读完整代码（至少读懂被改函数及其调用链）。
- 如果看完代码仍然不理解修改的意图或原理，必须停下来追问，而不是跳过。
- 理解包括：这个改动的副作用是什么？它改变了哪些边界条件？

### 2. 能删则删

**审查时优先考虑的不是"怎么改更好"，而是"这个代码是不是根本不该存在"。**

- 这个函数/类/文件真的有必要吗？还是可以删掉而不影响功能？
- 这个配置项、抽象层、接口是真的需要，还是"万一将来要用"？
- **Delete first, ask later.** 删掉后再问是否需要——加回来很容易，不用的代码只会烂在那里。

### 3. 保持简单

**最简单的正确方案是最好的方案。复杂性必须被证明是必要的。**

- 修复应该是精确的——恰好修掉问题，不要顺手"优化"不相关的地方。
- 如果别人的 fix 引入了不必要的复杂性（过度设计、多余抽象、复杂控制流），必须在报告中指出。
- 复杂度要 justify：每多一层抽象、多一个分支、多一个参数，都要有明确的原因。

---

## 核心原则：功能保持

任何代码修改（修复、重构、优化）必须遵循**功能等价性原则**：
- 保持原有功能的完整性和正确性，禁止以提升代码质量或架构优雅度为代价破坏已有功能
- 修改前必须理解被修改代码的当前行为和责任边界
- 修改完成后，必须验证相关功能仍然按预期工作
- 当代码质量与功能正确性冲突时，功能正确性优先

---

## Phase 0: 自动化工具检查（必须先通过，否则不继续）

> 后端审查（`.scala`）按下表执行；前端审查（`src/main/resources/web/`）按「前端适配」段替换。

按顺序执行以下命令。任何一步失败都必须修复后再重新运行全部步骤。

1. `sbt compile` — 必须零编译错误、零警告
2. `sbt test` — 必须全部通过，零失败
3. `sbt scalafmtCheckAll` — 必须报告 "All files are formatted"
4. `sbt "scalafix --check"` — 必须报告 "No rules requested to run" 或零规则违反

> **效率提示**：步骤 1-2 依赖编译，需顺序执行。步骤 3-4 与编译无关，可与步骤 1 并行执行。

**如果失败：**
- 格式问题 → 运行 `sbt scalafmtAll` 自动修复
- scalafix 问题 → 运行 `sbt scalafix` 自动修复
- 编译警告 → 手动修复（-Xfatal-warnings 已开启，任何警告都是错误）
- 测试失败 → 分析失败原因，修复后重跑 `sbt test`
- 修复完成后，**必须重新运行全部四步**确认通过
- **Phase 0 未通过前，禁止进入 Phase 1**

---

## Phase 1: 变更分析与三维深度审查

### 1a. 确定审查范围

根据触发方式确定审查范围：
- 全量审查：`/review` → 审查 `src/main/scala/nebflow/` 下所有 `.scala` 文件（前端全量则 `src/main/resources/web/`）
- 定向审查：`/review <path>` → 审查指定路径
- 合并后审查：`git diff HEAD~1 --name-only` 中目标域文件（`.scala` 及/或 `web/` 文件）

### 1b. 变更上下文分析（合并后审查必须执行）

对合并后审查，在阅读完整文件前，先执行：
```bash
git diff HEAD~1 --stat          # 概览变更规模
git diff HEAD~1 -- <文件>       # 逐文件查看实际 diff
git log -1 --format="%s%n%b"    # 查看 commit message
```

分析要点：
- 变更意图是什么？（新功能 / Bug 修复 / 重构 / 依赖升级）
- 变更影响范围有多大？是否与 commit message 描述一致？
- 是否有意外的"顺带修改"？（应拆分为独立 commit）

### 1c. 文件级三维评估

读取所有需要审查的文件，从三个维度评估，每个维度 1-5 分：

**评分锚定标准：**
- **5 分**：所有评估项满足，无可指摘
- **4 分**：存在可选优化建议（Suggestion），无实质问题
- **3 分**：存在建议修复的问题（Warning），但不影响正确性
- **2 分**：存在必须修复的问题（Critical），影响正确性或安全性
- **1 分**：存在严重缺陷，系统不可用

### 维度 A: 架构设计

评估项：
- **关注点分离**：每个类/对象只有一个清晰职责，不包揽过多责任
- **耦合方向**：依赖必须单向流动 `shared ← core ← cli/gateway`。
  - `core/tools/` 不应导入 `cli/` 或 `gateway/`
  - `llm/providers/` 不应导入 `core/`
  - `shared/` 不应导入任何其他 nebflow 包
  - `gateway/` 不应导入 `cli/`
- **抽象边界**：公共 API 使用 trait/interface 类型，而非具体实现
- **模块内聚**：同一包下的文件在概念上应属于同一主题
- **依赖图健康**：包之间无循环依赖

Nebflow 特有关注点：
- Tool trait 实现只应依赖 `shared/` 类型和外部库
- MCP 工具是动态注册的，不应有对特定 MCP 工具的静态耦合
- ProviderAdapter 是 LLM 调用抽象层，不应混入业务逻辑

### 维度 B: 代码质量

评估项：
- **逻辑清晰度**：控制流易于理解，无深层嵌套（超过 3 层嵌套需说明理由）
- **简洁性**：无死代码、无冗余模式匹配、无未使用 import
- **错误处理**：所有 IO 操作有 `handleErrorWith`/`attempt`，无裸 throw
- **资源安全**：Stream、Connection、Process 等资源正确释放（try/finally 或 bracket）
- **命名规范**：名称描述性强，遵循 Scala 命名约定，无单字母变量（迭代器除外）
- **类型安全**：最小化使用 `.get` on Option，模式匹配穷举，避免 `asInstanceOf`

Nebflow 特有关注点：
- PersistentShell（shell.scala）使用可变状态，必须验证线程安全
- SSE 流式解析器必须处理分块不完整和连接断开
- Fallback 链切换 provider 时不能泄漏资源
- Tool 执行错误和 Tool 执行结果必须可区分

### 维度 C: 安全 + 可观测性 + 可维护性

评估项：
- **输入验证**：所有外部输入（用户输入、API 响应、MCP 数据）在使用前验证
- **密钥处理**：API key 从环境变量加载，绝不记录或序列化
- **命令注入防护**：BashTool 和 shell 执行必须隔离/清洗不可信输入
- **资源限制**：所有网络调用和子进程执行必须有超时
- **可观测性**：有意义的错误消息，不吞异常，错误包含上下文
- **日志就绪度**：代码结构允许后续添加结构化日志，关键路径有日志点
- **可维护性**：无魔法数字、配置外置、文档齐全、代码自解释

Nebflow 特有关注点：
- BashTool 执行任意用户命令 — 必须有超时和进程隔离
- Config 解析 `${ENV_VAR}` 模式 — 确保不会意外记录环境变量值
- MCP server 命令是用户配置的 — 验证配置无命令注入
- Session 文件存储在 `~/.nebflow/` — 验证无敏感数据明文存储
- Gateway HTTP 端点 — 验证输入大小限制
- API key 穿透多层 — 确保它们不出现在任何错误消息中

---

## Diff 扫描清单（三维评估前的机械扫描）

三维评估开始前，先对 diff 做一轮机械扫描、产出 issue 清单——评估不从零开始：

- **变更意图分类**：新功能 / bug 修复 / 重构 / 依赖升级——与 commit message 描述不符 = 标记。
- **逐文件检查**：逻辑错误、错误处理缺失、资源泄漏。
- **意外顺带修改**：diff 中与任务无关的改动应拆独立 commit，在报告中标记。
- **issue 清单先行**：每条带严重度（critical / warning / info）+ 文件:行号 + 描述；该清单是三维评估的输入，防评估视角盲区漏项。

---

## 前端适配（审查 `src/main/resources/web/` 时启用）

qa-frontend 使用本 skill 时，三维框架与评分锚定不变，工具链与检查项按域替换：

**Phase 0 替换**（sbt 四连不适用）：
- Playwright 冒烟：加载改动页面，断言**实际内容**（textContent / 截图），不只是 DOM 存在性
- 语法检查：`node --check` 逐文件过改动的 `.js`
- 视觉铁律核对：对照 `nebflow-frontend-dev` 插件的 `visual-style`（弹窗毛玻璃无 overlay 遮罩 / 控件玻璃质感 / 字重 / 阴影激活态）

**维度 A 替换锚点**：渲染层 / 状态管理 / WS-REST 通信层的边界是否清晰；JS 不重复实现后端已给出的契约逻辑

**维度 B 替换锚点**：
- 事件监听器 / IntersectionObserver / Observer 的注册与释放成对（历史前科：cardRegistry IntersectionObserver 永不 disconnect → 百 MB 级泄漏）
- 长列表渲染性能（deferMd / 懒渲染路径不被绕过）
- 删除节点时同步清理相关 Map/缓存（deleteSession 等路径）

**维度 C 替换锚点**：
- XSS：用户内容拼 HTML 必须转义（innerHTML / insertAdjacentHTML 逐处审查）
- 敏感信息（API key / token）不得渲染进 DOM 或 console
- 可访问性：键盘可达、焦点管理（弹窗开合焦点不丢）

---

## Phase 2: 问题报告

对每个维度输出结构化报告：

```
## 维度 X: [名称] 报告

**评分**: X/5（评分理由：一句话总结扣分原因）

### Critical（必须修复）
- `[文件:行号]` 描述 — 建议修复方案

### Warning（建议修复）
- `[文件:行号]` 描述 — 建议修复方案

### Suggestion（可选优化）
- `[文件:行号]` 描述 — 建议修复方案
```

**注意**：如果某个维度无任何问题，直接输出 `**评分**: 5/5 — 无问题` 即可，不必输出空模板。

---

## Phase 3: Check-Fix 循环

初始审查完成后，进入迭代修正循环：

1. **修复所有 Critical 和 Warning 级别的问题**
   - 对每个问题，先定位代码、理解上下文、做出最小必要修改
   - 修改后记录修改了哪些文件
   - **功能验证**：修改完成后，确认被修改模块的原有行为未被破坏

2. **重跑 Phase 0 自动化检查**
   - 后端：`sbt compile` + `sbt test` + `sbt scalafmtCheckAll` + `sbt "scalafix --check"`
   - 前端：按「前端适配」段的 Phase 0 替换
   - 任一步失败则回到第 1 步修复

3. **重审修改过的文件**
   - 仅审查第 1 步中修改的文件，不重复全量审查
   - 检查修复是否引入新问题

4. **终止条件（满足所有条件即可结束）**
   - 所有自动化检查通过（Phase 0 全绿）
   - 所有维度零 Critical 问题
   - **最多 3 轮迭代**，防止无限循环
   - 超过 3 轮仍有未修复问题 → 输出报告并标记 NEEDS_ATTENTION

5. **输出最终质量报告**

```
# Nebflow 质量审查最终报告

## 变更概要（合并后审查时输出）
- Commit: <hash> <message>
- 变更文件数: N
- 变更意图: 一句话描述

## 自动化检查
- [ ] compile: PASS / FAIL
- [ ] test: PASS / FAIL
- [ ] scalafmtCheckAll: PASS / FAIL
- [ ] scalafix --check: PASS / FAIL

## 维度评分
- 架构设计: X/5
- 代码质量: X/5
- 安全+运维: X/5

## 迭代统计
- 迭代轮数: N
- 修复 Critical: N 个
- 修复 Warning: N 个
- 新增 Warning: N 个
- 剩余 Suggestion: N 个

## 修改文件清单
- `path/to/file.scala` — 修改说明

## 总体判定
[PASS / NEEDS_ATTENTION]
```

---

## 修复纪律与复审轮次（fixer 纪律吸收）

**fixer 四原则**（每次修复都适用）：

1. **最小且精准** — 只修指出的问题，不顺手重构无关代码。
2. **治根因不治标** — 真正的修法是阻止 null 产生时，就不加 null check。
3. **功能等价** — 编辑前读周边代码；修复不得破坏既有行为。
4. **一问题一修复** — 不把多个修复捆进一个 edit（可追溯性与可回滚性）。

**复审轮次纪律**：

- 修复完成后重跑全部 Phase 0 自动化检查；失败则修复重跑。
- 复审范围 = 仅本轮修改过的文件，不重复全量审查（防审查发散）。
- 复审检查「修复是否引入新问题」，不只检查「原问题是否消失」。

## UI selector 衰减门（verdict 判据）

对删除/改名 DOM selector（或其他 UI-facing 标识符）的变更给 pass 前，必须逐个 grep `tests/` 确认无测试仍依赖；有残留则先更新测试、或在报告中写明处置——未解决的残留 = critical（quality 维度）→ 判 fix。

## verdict 门禁语义（新架构承载）

评审结论（PASS/FAIL + 问题清单）写进输出文本；触发方（Nebula）读结果判断是否触发 fix 轮（LoopNode 或新建 fix/复审节点）——verdict 门禁由「读结果 + 重触发」承载，DAG 保持无环。

---

## 审查范围约定

- `/review` → 全量审查 `src/main/scala/nebflow/`（或前端全量 `src/main/resources/web/`）
- `/review <path>` → 定向审查指定路径
- 合并后触发 → 审查 `git diff HEAD~1 --name-only` 中的目标域文件