docs: 新增代码审查报告 CODE_REVIEW.md（P0/P1/P2 findings + 修复计划）
This commit is contained in:
Yaojia Wang
133
CODE_REVIEW.md
Normal file
133
CODE_REVIEW.md
Normal file
@@ -0,0 +1,133 @@
|
||||
# 代码审查报告 — writer-work-flow
|
||||
|
||||
> 审查日期：2026-06-21 · 分支：`develop`
|
||||
> 方法：7 个并行专项 Agent（架构 / FastAPI / Python 质量 / 安全 / 数据库 / React / TypeScript）逐文件审读真实代码 + 主控量化扫描。
|
||||
> 门禁现状（审查中复跑验证）：后端 `ruff` clean · `mypy` clean（98–107 files）· `pytest` **281 passed** · 前端 `pnpm typecheck`/`next lint` clean。
|
||||
|
||||
---
|
||||
|
||||
## 1. 总体结论
|
||||
|
||||
**架构是合理的，代码整体质量高。** CLAUDE.md 中 9 条架构不变量经逐一对照真实代码 **全部成立**（非仅文档声明）：DB 为唯一真源、agent 仅经 DB 通信、agent 只声明 `tier` 不绑模型/SDK、四审只读 + 验收事务为唯一写入门、digest 从 final_text 抽取、checkpoint 仅存控制流、确定性记忆选择（无向量）、snake_case 契约、Prompt 缓存稳定前缀。Repository/Protocol 解耦贯彻彻底（15 个 Protocol 仓储），SDK 局限在网关内，前端 100% 消费 OpenAPI 生成的类型。
|
||||
|
||||
**没有发现 CRITICAL 安全漏洞**：Fernet 静态加密、响应/日志一致脱敏、SQL 全参数化、无硬编码密钥、无 XSS（零 `dangerouslySetInnerHTML`）、无 SSRF（base_url 服务端固定）。前端 TS 异常干净：`strict: true`，零 `as any`/`@ts-ignore`/非空断言。
|
||||
|
||||
**主要问题集中在「运维硬化 + 边界健壮性 + 一致性」**，而非架构缺陷。最关键的几项被多个 Agent 独立交叉确认（见下方 ⊕ 标记），可信度高。
|
||||
|
||||
| 维度 | 评级 |
|
||||
|---|---|
|
||||
| 架构合理性 | ✅ 优秀（9/9 不变量成立） |
|
||||
| 后端代码质量 | ✅ 良好（ruff/mypy/pytest 全绿） |
|
||||
| 安全 | 🟡 良好，有 4 项 HIGH 运维硬化缺口 |
|
||||
| 数据库 | 🟡 设计扎实，时区/索引/事务竞态需修 |
|
||||
| 前端质量 | ✅ 良好，有 stale-closure + a11y 缺口 |
|
||||
| 类型安全 | 🟡 优秀，边界处有 `as` 强转削弱保证 |
|
||||
|
||||
---
|
||||
|
||||
## 2. 按严重度汇总（⊕ = 多 Agent 交叉确认）
|
||||
|
||||
### 🔴 P0 — 上生产前必修（正确性 / 安全 / 数据完整性）
|
||||
|
||||
| # | 问题 | 证据 | 修复 |
|
||||
|---|---|---|---|
|
||||
| P0-1 ⊕ | **仓储内部自提交，破坏事务原子性。** `SqlCredentialStore` 的 upsert/delete/routing 各自 `commit()`，`PUT /settings/providers` 多凭据写入非原子——第 3 条成功、路由失败则 DB 处于半更新且无回滚。`SqlChapterRepo.save_draft` 同样自 `commit()`，违反全仓「仓储只 flush、调用方提交」契约。 | `services/credentials.py:159,194,210,236`；`domain/chapter_repo.py:130,135` | 全部改为 `flush()`，由 `upsert_providers`/端点统一 `commit()` 一次。 |
|
||||
| P0-2 ⊕ | **加密密钥空默认 + 异常未兜底。** `credential_enc_key` 默认 `""`，进程照常启动接流量；首次凭据读写才抛 `CredentialKeyError`（`RuntimeError`），而 `main.py` 仅注册了 `AppError` 处理器 → 裸 500、无 envelope、无 request_id。`.env.example` 也留空，强化误配路径。 | `config/settings.py:18`；`security/credentials.py`；`routers/settings_providers.py:82`；`main.py` | 在 `_lifespan` 启动校验 `_fernet(enc_key)` **快速失败**；并加 catch-all `Exception` handler 统一 envelope。 |
|
||||
| P0-3 | **`job.error` 直接回传 `str(exc)` 到前端。** 后台任务把原始异常字符串写库并经 `GET /jobs/{id}` 原样返回、前端直接渲染 → 信息泄露（解密失败提示、内部路径等）。 | `services/job_runner.py:79`；`routers/jobs.py:32`；`lib/jobs/job.ts:102` | `run_job` 按类型分流：`AppError` 存 code+message；其他存通用「任务执行失败」，绝不存 `str(exc)`。 |
|
||||
| P0-4 | **SSE 流内异常无 error 事件 / 尾部 commit 无兜底。** 评审流 `graph.ainvoke` 直接 await，抛错则流被截断、客户端收不到 `error` 事件；两处生成器尾部 `session.commit()` 失败会静默丢失用量账本记录。 | `routers/projects.py:407`、`:279`、`:413` | `graph.ainvoke` 包 try/except 先 yield `error_event` 再 return；尾部 commit 包 try/except 至少 `log.error`。 |
|
||||
|
||||
### 🟠 P1 — 应修（健壮性 / 性能 / 真实 bug）
|
||||
|
||||
| # | 问题 | 证据 | 修复 |
|
||||
|---|---|---|---|
|
||||
| P1-1 | **时间戳全部 `TIMESTAMP WITHOUT TIME ZONE`。** 16 张表 `created_at/updated_at` 用 `func.now()` 无 `timezone=True`，存服务器本地时；`ChapterReview.created_at` 是「取最新评审」的排序键，跨时区/改时区即静默错乱。 | `db/base.py:23,27-28`；初始迁移全部时间列 | `DateTime(timezone=True)` → TIMESTAMPTZ；加 `ALTER ... AT TIME ZONE 'UTC'` 迁移。 |
|
||||
| P1-2 | **熔断器对非可重试错误不计数。** `Gateway.run` 命中不可重试错误（401/400/内容策略）时直接 `raise`，从不 `record_failure` → 错误 API key 永不触发熔断，每次仍打同一坏 provider。 | `llm_gateway/gateway.py:211-216` | 重试前对持续性 4xx（至少 401/403）也 `record_failure`。 |
|
||||
| P1-3 | **验收热路径缺复合索引。** `list_for_chapter` 按 `(project_id, chapter_no) ORDER BY created_at DESC` 查询，仅有单列 `project_id` 索引；`chapter_digests.recent()` 同样。`projects.owner_id`（主访问谓词）、`usage_ledger`、`jobs.status`、`rules.project_id` 均无索引。 | 初始迁移；`review_repo.py:116`、`project_repo.py:91` | 新迁移加复合/部分索引（见 §4 SQL）。 |
|
||||
| P1-4 | **`max_version` TOCTOU 竞态。** `promote_to_accepted` 读 `max(version)` 再插入未加锁，并发验收同一章会撞唯一约束 → 未捕获 `IntegrityError` 抛 500。`save_draft` 的 read-then-insert 同类竞态。 | `domain/chapter_repo.py:160-172`、`104-137` | `max_version` 查询加 `.with_for_update()`，或 `INSERT ... ON CONFLICT`。 |
|
||||
| P1-5 | **`scan_overdue` 全表扫 + 逐行更新。** 拉全项目伏笔行到内存再 Python 过滤/逐条 UPDATE。 | `domain/foreshadow_repo.py:201-218` | 谓词下推 SQL `WHERE`，批量 `UPDATE ... RETURNING`。 |
|
||||
| P1-6 ⊕ | **两处 `useEffect` 抑制 exhaustive-deps，隐藏 stale-closure。** `useStyleLearn` 的 `projectId`、`useKimiOauth` 的 `refreshStatus` 被排除依赖；切项目/回调变化时会用到旧值。 | `lib/style/useStyleLearn.ts:64`；`lib/settings/useKimiOauth.ts:78` | 把稳定回调/值纳入依赖（均已 `useCallback`，零成本）或用 ref 显式追踪。 |
|
||||
| P1-7 | **两处模态框无 focus trap。** `CommandPalette`、`Drawer` 声明 `aria-modal` 但 Tab 可逃逸到背景，违反 WCAG 2.1.2。 | `components/command/CommandPalette.tsx:99`；`components/Drawer.tsx:26` | 实现焦点陷阱（`focus-trap-react` 或自写 Tab 循环）。 |
|
||||
| P1-8 | **边界处 `as` 强转削弱类型保证。** `parseSseBlock`/`parseReviewBlock` 末尾 `as SseEvent`、`server.ts` 的 `(await res.json()) as T` 在「流→状态」「服务端→组件」两个最关键信任边界绕过结构校验。 | `lib/stream/sse.ts:42`；`lib/review/sse.ts:142`；`lib/api/server.ts:25,35` | 改逐事件类型守卫或 Zod 解析后返回，去掉强转。 |
|
||||
| P1-9 | **`assert` 作类型收窄出现在生产适配器路径。** `-O` 运行时被剥离，`output_schema` 可能漏成 `None`。 | `adapters/anthropic.py:147`；`adapters/openai_compat.py:129` | 换成显式 `if ... is None: raise ValueError(...)`。 |
|
||||
| P1-10 | **OAuth `user_code` 写入结构化日志。** 授权窗口内日志可见者可冒用用户授权意图。 | `routers/kimi_oauth.py:150-155` | 移除 `user_code`，仅留 `request_id`/`job_id` 关联。 |
|
||||
|
||||
### 🟡 P2 — 打磨（一致性 / DRY / 类型收紧 / 次要硬化）
|
||||
|
||||
- **DRY：`GatewayRun` Protocol 4 处重复**（`orchestrator/{review,generation,outline,style_extract}_node.py`）→ 抽到 `_protocols.py` 单点定义。⊕（Python agent；gotcha 注明的「故意分散」理由不成立，因仅依赖 `ww_llm_gateway.types` 无环）
|
||||
- **DRY：`_is_transient`/`_TRANSIENT_NAMES` 3 处适配器重复** → 抽到 `adapters/base.py`。
|
||||
- **codegen 质量：弱类型响应字段。** `GET /jobs/{id}` 返回裸 `dict`、`StyleFingerprintResponse.dimensions/evidence: dict[str, Any]`、`ReviewHistoryItem` 的 JSONB 裸 dict、`selling_points: list[Any]` → 定义具体 Pydantic 子模型，提升 TS 客户端类型。
|
||||
- **路由无 OpenAPI 错误响应声明**（`responses={404: ErrorEnvelope, ...}`）→ TS 端无类型化错误。
|
||||
- **缺速率限制**（所有触发 LLM 的端点 + `/oauth/start` + `/settings/providers/test`）→ 至少对 LLM 端点加轻量限流；若接受延后，记入 `memory/decisions.md`。
|
||||
- **读端点缺 project 存在性校验**：`list_characters`/`list_world_entities`/`list_foreshadow` 不存在的 project 返回空 200 而非 404。
|
||||
- **`httpx.AsyncClient()` 无 timeout**（Kimi token refresh，`project_deps.py:308`）→ 加 `timeout=30.0`。
|
||||
- **架构层轻微泄漏（M1）：** `provider_deps.py:13` 直接 `from openai import AsyncOpenAI` 并在 `:31-41` 重复 `_PROVIDER_BASE_URLS`（网关 `factory.py` 已拥有）→ 改调网关 `build_adapter(...)`，base_url 单点归网关。⊕（架构 agent + 主控 grep）
|
||||
- **`accept_service.py:91` `session: object` + `type: ignore`** → 换 1 方法 `Protocol`（`Committable`）。
|
||||
- **冗余/可删强转**：`useRefine.ts:68`、`useCharacterGen.ts:111`、`useStyleLearn.ts:114`、`useForeshadow.ts:63`、`useAutosave.ts:41`（`data` 已由生成客户端定型，直接用即可）。
|
||||
- **`tsconfig` 缺 `noUncheckedIndexedAccess`** → 开启以捕获数组/Record 索引的隐性 `undefined`。
|
||||
- **CORS：`allow_credentials=True` + `allow_methods/headers=["*"]`** → 收窄白名单，并加启动断言 origins 不含 `"*"`。
|
||||
- **`credential_enc_key` 用裸 `str`** → 改 `SecretStr` 防误序列化。
|
||||
- **`request_id` 头未校验**（`middleware.py:22`）→ 正则白名单，否则生成新 UUID。
|
||||
- **测试用 `"x"*44` 充当 Fernet key**（非法 key，`setdefault` 有误用真 key 风险）→ conftest 用 `Fernet.generate_key()`。
|
||||
- **前端小项**：`Toast` 定时器未在卸载清理 + key 用 `Date.now()+Math.random()`；`key={index}` 用在可重排冲突列表（`AnnotatedText.tsx:29`、`ConflictAdjudication.tsx:35`）；`ReviewErrorNote` 裸 `<a target=_blank>` 缺 `rel="noopener"`（`ReviewReport.tsx:410`，兄弟组件已用 `<Link>`）；`ReviewReport` 在 render 内 `segmentText` 大文本 split 未 `useMemo`；`ConflictCard` `fieldset/legend` 误用于 toggle 按钮；`useInjection.save` 并发无锁。
|
||||
- **大文件提示**：`specs.py` 403、`generation_node.py` 307（均在 800 硬线内，可后续按域拆分）。
|
||||
- **日志未接管 stdlib root**（SQLAlchemy/httpx/uvicorn 日志绕过 structlog JSON）→ `logging.basicConfig` + `ProcessorFormatter`。
|
||||
|
||||
---
|
||||
|
||||
## 3. 修复计划（分阶段）
|
||||
|
||||
### 阶段 A — 安全 & 事务正确性（P0，建议先做，半天内可完成）
|
||||
1. **P0-1** 移除 `SqlCredentialStore` 4 处 + `save_draft` 2 处 `commit()` → `flush()`；调用方统一 commit。补一条「多凭据一请求」集成测试覆盖部分提交失败回滚。@backend + @db
|
||||
2. **P0-2** `_lifespan` 启动校验 `_fernet(enc_key)`；注册 catch-all `Exception` handler → `ErrorCode.INTERNAL` envelope。`.env.example` 注明必填。@backend
|
||||
3. **P0-3** `run_job` 异常分类落库（`AppError` → code+message；其他 → 通用文案）。@backend
|
||||
4. **P0-4** 两处 SSE 生成器：`graph.ainvoke` 包 try/except yield error_event；尾部 commit 包 try/except 记日志。@backend
|
||||
|
||||
### 阶段 B — 数据层正确性 & 性能（P1，1 次迁移搞定大部分）
|
||||
5. **P1-1** 时间列改 TIMESTAMPTZ（`base.py` + 数据迁移 `AT TIME ZONE 'UTC'`）。@db
|
||||
6. **P1-3** 新迁移加索引（见 §4）。@db
|
||||
7. **P1-4** `max_version` 加 `with_for_update()`（或 `save_draft`/`promote` 改 upsert）。@db
|
||||
8. **P1-5** `scan_overdue` 谓词下推 + 批量 UPDATE。@db
|
||||
|
||||
### 阶段 C — 网关 & 适配器健壮性（P1/P2）
|
||||
9. **P1-2** 熔断器对非可重试 4xx 计数。@llm
|
||||
10. **P1-9** `assert` → 显式 `raise ValueError`。@llm
|
||||
11. **P1-10** 移除 OAuth `user_code` 日志。@backend
|
||||
12. **P2** 抽 `GatewayRun` 单点 Protocol；抽 `_is_transient` 共享工具；`httpx` timeout；Gemini Protocol 改 `async def`。@llm
|
||||
|
||||
### 阶段 D — 前端健壮性 & a11y（P1/P2）
|
||||
13. **P1-6** 修两处 stale-closure 依赖。@frontend
|
||||
14. **P1-7** `CommandPalette`/`Drawer` 加 focus trap。@frontend
|
||||
15. **P1-8** SSE 解析与 `server.ts` 去 `as` 强转，改类型守卫/校验。@frontend
|
||||
16. **P2** 删冗余强转、开 `noUncheckedIndexedAccess`、Toast 清理、`key` 稳定化、`rel="noopener"`、`useMemo` 大文本、`useInjection` 加锁。@frontend
|
||||
|
||||
### 阶段 E — codegen & 运维硬化（P2）
|
||||
17. 弱类型响应字段建模（`JobResponse`/`DimensionEntry`/`ReviewConflictView`/`selling_points: list[str]`）+ 路由 `responses=` 错误声明 → 重生成 TS 客户端。@backend → @frontend
|
||||
18. 速率限制（或记 decision 延后）、CORS 收窄、`SecretStr`、`request_id` 校验、stdlib 日志接管、测试 Fernet key、`provider_deps` 调网关 factory、读端点 404 校验。@backend/@devops
|
||||
|
||||
---
|
||||
|
||||
## 4. 建议的索引迁移（P1-3）
|
||||
|
||||
```sql
|
||||
-- 验收/评审热路径
|
||||
CREATE INDEX ix_chapter_reviews_project_chapter
|
||||
ON chapter_reviews (project_id, chapter_no, created_at DESC);
|
||||
CREATE INDEX ix_chapter_digests_project_chapter
|
||||
ON chapter_digests (project_id, chapter_no DESC);
|
||||
-- 主访问谓词
|
||||
CREATE INDEX ix_projects_owner_id ON projects (owner_id);
|
||||
CREATE INDEX ix_usage_ledger_owner_id ON usage_ledger (owner_id);
|
||||
CREATE INDEX ix_usage_ledger_project ON usage_ledger (project_id);
|
||||
CREATE INDEX ix_rules_project_id ON rules (project_id);
|
||||
CREATE INDEX ix_jobs_status_running ON jobs (status) WHERE status = 'running';
|
||||
-- 可选：全局 rules 部分索引（配合 all_for_project 的 OR project_id IS NULL）
|
||||
CREATE INDEX ix_rules_global ON rules (id) WHERE project_id IS NULL;
|
||||
```
|
||||
|
||||
并对 `Chapter.status` / `Foreshadow.status` 加 CHECK 约束（P2，DB 边界兜底状态机）。
|
||||
|
||||
---
|
||||
|
||||
## 5. 已确认安全的事项（非问题）
|
||||
|
||||
无硬编码密钥 · SQL 全参数化（无 `text()` 拼接）· LLM 调用日志只记 `input_chars` 长度不记正文/key · OAuth token Fernet 加密落库且不出现在日志/响应/job 结果 · 无 `dangerouslySetInnerHTML` · base_url 服务端固定无 SSRF · 仅 `NEXT_PUBLIC_API_BASE` 暴露前端（非密钥）· 验收事务严格按 §5.5 顺序：冲突门(`projects.py:478`)→事务外从 final_text 抽 digest(`:491`)→原子事务(`:499`)。
|
||||
Reference in New Issue
Block a user