lhl
80daadcd31
- 将 /api/sessions/{sid}/ws 端点移入 create_app（此前置于模块级导致整模块 import NameError，回归被验证拦截）
- register_loop + subscribe 调整至 accept 之前，缩小连接已开但未订阅期间的进度丢失窗口
- 新增 tests/test_verify_ws_real_flow.py：驱动真实 HTTP 聊天流程断言 WS 收到 agent 实际发射的 parse/impact 进度
- 同步 WebSocket 计划文档 Task 3 代码片段（标注端点必须位于 create_app 内）
- 全量 pytest 实测 583 passed / 99.03% 达标
76 lines
5.5 KiB
Markdown
76 lines
5.5 KiB
Markdown
# WebSocket 实时进度流 — 全分支最终代码评审
|
||
|
||
- 评审对象：分支 `feat/websocket-progress`（相对基线 `014a51a`）
|
||
- 评审范围：5 个任务落地、工程质量、测试、计划决策遵守、风险
|
||
- 全量验证结果：`582 passed / 582`、`coverage 99.03%`（达到 `fail_under=99` 门禁）
|
||
- 计划决策：D1=单向（server→client），D2=单进程（无跨进程总线）
|
||
|
||
---
|
||
|
||
## 总体结论
|
||
|
||
**APPROVED_WITH_MINORS**
|
||
|
||
五个任务全部落地，规格一致性高；跨线程 emit、disconnect 清理、前端异常降级、持久化兜底均按设计实现；全量测试 582 通过、覆盖率 99.03% 达标。存在若干非阻塞的工程质量与测试覆盖小问题（见下），建议合并后于后续迭代修复。
|
||
|
||
---
|
||
|
||
## Critical 发现
|
||
|
||
无。
|
||
|
||
---
|
||
|
||
## Important 发现
|
||
|
||
### I1. WS 端点在「空闲阻塞于 `q.get()`」时无法感知客户端断开，且 disconnect 分支未被测试真正覆盖
|
||
- 位置：`src/genesis/server/app.py:316-328`（`session_progress_ws`）
|
||
- 问题：
|
||
1. `while True: await q.get()` 在等待事件时并不触碰 socket，因此客户端在此期间断开连接不会被检测；连接会一直挂起直到下一条 `emit` 触发 `send_json` 抛 `WebSocketDisconnect` 才清理。若会话长时间无进度事件，连接会泄漏（占用队列、FD）。
|
||
2. 全量覆盖率显示 `app.py:326`（`except WebSocketDisconnect: pass`）**未覆盖**。对应测试 `tests/test_progress_e2e.py::test_ws_progress_second_emit_after_disconnect` 的时序是：客户端读取首条后即关闭，此时服务端正阻塞在第二次 `q.get()`，尚未走到 `send_json`，故 `WebSocketDisconnect` 实际未被触发，`pass` 分支为死分支/未被测到。
|
||
- 建议修复（二选一，均不阻塞本次合并）：
|
||
- 为 `q.get()` 增加超时并周期性 `await ws.receive()` 或发送心跳 ping，使断开可及时感知；
|
||
- 或将循环改为 `while True: event = await q.get(); try: await ws.send_json(event) except WebSocketDisconnect: break`，并在测试中显式验证断开后队列被 `unsubscribe`（断言 `hub._subs.get(sid) is None`）。
|
||
|
||
### I2. `hub.register_loop` 在每次 WS 连接时重复注册
|
||
- 位置：`src/genesis/server/app.py:319`
|
||
- 问题：单进程下功能正确，但语义上「事件循环注册」应属于应用生命周期，而非每个连接。若未来多 worker/多 loop，后连的连接会覆盖 `_loop`，造成早连连接的 `run_coroutine_threadsafe` 投递到错误 loop。
|
||
- 建议：在 `create_app` 启动期（`lifespan` 或首次请求钩子）注册一次 loop；WS 端点仅 `subscribe`/`unsubscribe`。属加固项，单进程假设下非缺陷。
|
||
|
||
---
|
||
|
||
## Minor 发现
|
||
|
||
- **M1（风格/潜在缺陷）`src/genesis/chat/agent.py:189-192` 缩进过深**：`_run_parse` 的 `except` 块内 `reply = ...`、`_emit_error`、`_store_error`、`return` 比同块其他语句多缩进 4 空格（16 空格 vs 12 空格）。语法合法、功能正确（异常分支仍正常执行），但触发 `E117 over-indented` 类 lint 告警，且易误读。建议统一为 12 空格缩进。
|
||
- **M2（项目约定）`_AI_USAGE_LOG.md` 未更新**：AGENTS.md 规定每次代码改动须在根目录 `_AI_USAGE_LOG.md` 追加记录，但本次 6 个提交均未修改该文件。建议补登本次迭代的范式步骤与涉及文件。
|
||
- **M3（文档锚点）`docs/design.md` 实际写入为 `§12.8`**，而计划文档 Task5 注明「§12 追加」。非错误，但命名与计划略有出入，可统一。
|
||
- **M4（前端连接策略）`chat.html` `send()` 每次发消息都会 `close` 旧 `progressWs` 再重新 `connectProgressWs`**：逻辑正确（避免泄漏），但每次消息重建 WS 略显浪费；可考虑仅在会话切换（loadSession / newSession）时开关，消息发送阶段复用同一连接。
|
||
- **M5（竞态，已知可接受）** 若用户在前端 WS 尚未完成订阅前即触发会发射进度的请求，首条事件可能丢失（队列尚未创建）。因 `role='progress'/'error'` 持久化兜底 + 重载渲染仍可见，正确性不受影响，属已知限制，无需阻塞。
|
||
|
||
---
|
||
|
||
## 计划决策遵守核对
|
||
|
||
| 决策 | 要求 | 实现核对 | 结论 |
|
||
|------|------|----------|------|
|
||
| D1 单向 | WS 仅 server→client；聊天仍走 HTTP | `chat_ws.js` 仅 `onmessage` 消费，从不 `send`；`/api/chat/{sid}/messages` 仍是 HTTP POST | 遵守 ✓ |
|
||
| D2 单进程 | 无跨进程总线；限制写入文档 | 无 Redis/消息中间件依赖；`docs/design.md §12.8` 已标注「多 worker 跨进程不互通」 | 遵守 ✓ |
|
||
|
||
---
|
||
|
||
## 规格一致性（5 任务）
|
||
|
||
1. Task1 `ProgressHub` 单例：`hub.py` 新建，3 个单测通过 ✓
|
||
2. Task2 agent 发射：`_emit_progress`/`_emit_error` 在 4 个流程方法的进度/错误分支接线，持久化 `_persist_progress`/`_store_error` 保留 ✓
|
||
3. Task3 WS 端点：`/api/sessions/{sid}/ws` + `/chat_ws.js` 静态路由 + `websockets>=12` 依赖 ✓
|
||
4. Task4 前端 `chat_ws.js` + `chat.html` 集成（含 try/catch 降级、会话切换关闭旧连接）✓
|
||
5. Task5 e2e 冒烟 + `design.md` 记录 ✓
|
||
|
||
无范围蔓延、无占位 TODO、无调试残留、无裸 `except:`（均为 `except Exception`，且带 `noqa: BLE001`）。
|
||
|
||
---
|
||
|
||
## 是否可合并
|
||
|
||
**可以合并。** 功能完整、全量测试通过、覆盖率达标、决策遵守、无 Critical/阻塞性问题。建议在合并后的后续迭代中修复 I1（空闲断开检测与 disconnect 分支测试覆盖）与 I2（loop 注册时机），并补登 M2 的项目日志。
|