349 lines
15 KiB
Markdown
349 lines
15 KiB
Markdown
> [!NOTE]
|
||
> Status: Historical snapshot. Current refactor results and validated baseline are tracked in `CHANGELOG.md` (updated 2026-02-16).
|
||
|
||
# Codex 架构复核报告
|
||
|
||
评审日期: 2026-02-13
|
||
评审范围: `ARCHITECTURE_REVIEW.md` 逐条核验 + 全项目补充审查
|
||
评审基线: 当前工作区代码（含 `src/vlm/`、`tests/`）
|
||
|
||
## 1. 结论摘要
|
||
|
||
- `ARCHITECTURE_REVIEW.md` 中有一部分问题判断准确，但存在明显“已被代码修复却仍判定为问题”的过期结论。
|
||
- 当前项目确实仍有若干高优先级问题，且有至少 2 个关键问题未被该报告指出。
|
||
- 全量测试当前通过（整改后为 `460 passed`），且已补齐异常路径与安全边界回归用例。
|
||
|
||
## 2. 对 ARCHITECTURE_REVIEW.md 的逐项核验
|
||
|
||
### 2.1 判定为“真实存在”的问题
|
||
|
||
1. 无统一 I/O 入口（部分命令仍在 CLI 内重复读写/解析 CSV/JSON）
|
||
- 证据: `src/vlm/cli.py:220`, `src/vlm/cli.py:1628`, `src/vlm/cli.py:1496`
|
||
- 影响: 解析逻辑重复，行为漂移风险，维护成本高
|
||
|
||
2. State 未接入主流程（scan/parse/enrich/analyze/plan/execute 未自动更新状态）
|
||
- 证据: `StateManager` 仅用于 `vlm state *`
|
||
- 代码位置: `src/vlm/cli.py:1731`, `src/vlm/cli.py:1797`, `src/vlm/cli.py:1849`
|
||
|
||
3. State 持久化非原子写
|
||
- 证据: 直接 `open(..., "w")` 覆盖写
|
||
- 代码位置: `src/vlm/state.py:79`
|
||
- 风险: 崩溃/断电时 state 文件损坏
|
||
|
||
4. State 路径键未 canonicalize
|
||
- 证据: `str(file_path)` 直接作为 key，未 `resolve()`
|
||
- 代码位置: `src/vlm/state.py:117`, `src/vlm/state.py:139`
|
||
- 影响: 软链接/相对路径导致同一文件多状态
|
||
|
||
5. enrichment 串行处理，配置中的并发参数未生效
|
||
- 证据: 双层 for 串行处理 records；`enrichment_max_concurrency` 未用于调度
|
||
- 代码位置: `src/vlm/enrichment.py:67`, `src/vlm/config.py:35`
|
||
|
||
6. scan 的 ffprobe 仍串行
|
||
- 证据: 文件逐个 `_create_video_file`，其中 `extract_metadata` 为阻塞调用
|
||
- 代码位置: `src/vlm/scanner.py:272`
|
||
|
||
### 2.2 判定为“部分成立”的问题
|
||
|
||
1. “Analyze 阶段丢失 metadata，导致 by_quality 失效”
|
||
- 现状: 已支持 v2 `video_metadata`，且 analyze 支持合并 inventory
|
||
- 证据: `src/vlm/io.py:64`, `src/vlm/io.py:179`, `src/vlm/commands/analyze.py:31`
|
||
- 结论: 对 v1 输入仍可能缺质量字段；对 v2/带 inventory 路径已修复
|
||
|
||
2. “路径验证缺失，路径会陈旧”
|
||
- 现状: execute 时会检查 source 是否存在
|
||
- 证据: `src/vlm/executor.py:251`
|
||
- 结论: “陈旧路径无保护”不准确；但仍有更深层路径安全问题（见第 3 节）
|
||
|
||
3. “回滚前无验证 / 非幂等”
|
||
- 现状: rollback 为 best-effort，且会判断目标是否存在再回滚
|
||
- 证据: `src/vlm/executor.py:523`
|
||
- 结论: 作为增强建议成立，但不构成当前实现缺陷性 bug
|
||
|
||
### 2.3 判定为“不成立或已过时”的问题
|
||
|
||
1. “Quarantine 非原子，可能产生孤儿文件”
|
||
- 现状: 已实现两阶段提交（pending -> move -> committed）并含恢复逻辑
|
||
- 证据: `src/vlm/quarantine.py:190`, `src/vlm/quarantine.py:286`, `src/vlm/quarantine.py:490`
|
||
- 结论: 该条为过时结论
|
||
|
||
2. “缺少 schema versioning”
|
||
- 现状: 多个核心产物已有版本字段
|
||
- 证据:
|
||
- inventory 注释版本: `src/vlm/scanner.py:519`
|
||
- identities 版本: `src/vlm/cli.py:351`
|
||
- analysis 版本: `src/vlm/commands/analyze.py:100`
|
||
- plan 版本: `src/vlm/planner.py:430`
|
||
- rollback 版本: `src/vlm/executor.py:366`
|
||
- 结论: 不成立
|
||
|
||
3. “配置校验时机过晚”
|
||
- 现状: CLI 初始化时已执行 `validate_config`
|
||
- 证据: `src/vlm/cli.py:79`
|
||
- 结论: 不成立
|
||
|
||
4. “io.py 引入 scanner 导致循环依赖风险（高）”
|
||
- 现状: 当前依赖是单向的，未形成循环
|
||
- 证据: `src/vlm/io.py:13`
|
||
- 结论: 现阶段仅属架构偏好建议，不是实际问题
|
||
|
||
## 3. 报告遗漏但实际存在的问题（重点）
|
||
|
||
### 3.1 高优先级: `analyze` / `plan` 的 JSON 错误分支存在 NameError
|
||
|
||
- 问题: `except json.JSONDecodeError` 使用了未导入的 `json`
|
||
- 位置: `src/vlm/cli.py:628`, `src/vlm/cli.py:678`
|
||
- 影响: 输入非法 JSON 时，用户得到 `NameError` 而不是清晰报错，故障定位困难
|
||
- 复现: 通过 `CliRunner` 对无效 JSON 调用 `vlm analyze` / `vlm plan`，异常为 `name 'json' is not defined`
|
||
|
||
### 3.2 高优先级: 目标路径缺少边界约束，存在路径穿越风险
|
||
|
||
- 问题链路:
|
||
- `display_title` 可由外部 enrichment 数据参与构造，未过滤路径分隔符
|
||
- planner 直接模板拼接 destination
|
||
- 未校验 destination 必须位于 `library_root` 内
|
||
- 位置:
|
||
- `src/vlm/enrichment.py:509`
|
||
- `src/vlm/io.py:81`, `src/vlm/io.py:103`
|
||
- `src/vlm/planner.py:212`, `src/vlm/planner.py:228`
|
||
- 风险: 特殊标题/模板可把文件移动到库目录外
|
||
|
||
### 3.3 中优先级: 执行汇总统计口径重叠
|
||
|
||
- 问题: conflict 既计入 `failed` 又计入 `skipped`
|
||
- 位置: `src/vlm/executor.py:318`
|
||
- 影响: 汇总数据解释歧义，可能误导用户决策
|
||
|
||
### 3.4 中优先级: duplicate report 的排序信息被削弱
|
||
|
||
- 问题: `report duplicates` 重建 `VideoFile` 时将 `size_bytes` 固定为 0；而文本报告按 `group.files` 最大 size 排序
|
||
- 位置: `src/vlm/cli.py:1536`, `src/vlm/reports.py:338`
|
||
- 影响: 重复组排序质量下降（与真实文件体积不一致）
|
||
|
||
## 4. 设计层观察（非缺陷但建议）
|
||
|
||
1. 当前“读多写少 + 人工确认”主流程设计合理，安全性基线较好。
|
||
2. `Quarantine` 两阶段提交是亮点，但 manifest 写入仍非原子文件替换，可继续增强。
|
||
3. `io.py` 已初步形成统一入口，但 CLI 层未完全收敛，建议继续去重。
|
||
|
||
## 5. 建议修复优先级
|
||
|
||
### P0（立即）
|
||
|
||
1. 修复 `cli.py` 中 `analyze` / `plan` 的 `json` 未导入问题（避免异常路径误报）
|
||
2. 增加 destination 边界校验（`destination.resolve().is_relative_to(library_root.resolve())` 等）
|
||
3. 对标题/模板输入做路径片段净化（移除 `/`, `\\`, `..` 等）
|
||
|
||
### P1（近期）
|
||
|
||
1. State 写入改为原子落盘（临时文件 + `os.replace`）
|
||
2. State key 统一 canonical path
|
||
3. 收敛 CLI 里的重复 CSV/JSON 解析到 `io.py`
|
||
|
||
### P2（中期）
|
||
|
||
1. 修正执行汇总口径（`failed/skipped` 去重）
|
||
2. 修正 duplicate report 的 size 数据来源
|
||
3. 为 enrichment / ffprobe 引入受控并发，真正使用 `enrichment_max_concurrency`
|
||
|
||
## 6. 测试与证据
|
||
|
||
- 全量测试: `uv run pytest -q` -> `460 passed`
|
||
- 定向回归: `tests/test_cli_json_errors.py`, `tests/test_path_safety.py`, `tests/test_state.py`, `tests/test_enrichment.py`, `tests/test_scanner.py`, `tests/test_executor.py`, `tests/test_reports.py` -> 全部通过
|
||
- 额外手工复现:
|
||
- 无效 JSON 调用 `vlm analyze` / `vlm plan`，触发 `NameError`（实锤缺陷）
|
||
|
||
## 7. 最终判断
|
||
|
||
- `ARCHITECTURE_REVIEW.md` 的总体方向是有价值的，但包含多个已失效结论。
|
||
- 当前项目最需要优先处理的，不是报告里强调的旧问题，而是：
|
||
1. CLI 异常路径 NameError
|
||
2. 路径安全边界与标题净化
|
||
3. State 原子写与 canonical key
|
||
|
||
## 8. 详细整改计划（按阶段推进）
|
||
|
||
### 8.1 目标与范围
|
||
|
||
- 目标: 在不破坏现有 CLI 行为的前提下，完成高风险缺陷修复、核心一致性改造与性能增强。
|
||
- 范围: `src/vlm/cli.py`, `src/vlm/planner.py`, `src/vlm/io.py`, `src/vlm/enrichment.py`, `src/vlm/state.py`, `src/vlm/executor.py`, `src/vlm/reports.py`, `tests/` 对应模块。
|
||
- 非目标: 本轮不做大规模架构重写（例如全面重构命令框架、替换数据模型体系）。
|
||
|
||
### 8.2 里程碑
|
||
|
||
1. M1（P0，1-2 天）: 修复线上风险最高的功能正确性与安全边界问题。
|
||
2. M2（P1，3-5 天）: 完成 state 持久化可靠性与 I/O 收敛改造。
|
||
3. M3（P2，5-8 天）: 修复统计口径、报告准确性与可控并发能力。
|
||
4. M4（回归与发布，1-2 天）: 全量回归、文档更新、发布前检查。
|
||
|
||
### 8.3 任务清单（任务-交付物-验收标准）
|
||
|
||
#### M1 / P0（必须先做）
|
||
|
||
1. 任务 T1: 修复 `analyze/plan` 的 JSON 异常分支 NameError
|
||
- 修改点: `src/vlm/cli.py`
|
||
- 实施:
|
||
- 在模块级引入 `json`，或将异常类型改为已存在解析函数抛出的统一异常类型
|
||
- 保证错误信息仍是用户可读文案（非 traceback）
|
||
- 交付物: 代码修复 + 新增/更新 CLI 测试
|
||
- 验收标准:
|
||
- 输入非法 JSON 时返回非 0 退出码
|
||
- stderr 包含明确的 JSON 解析错误提示
|
||
- 不出现 `NameError`
|
||
|
||
2. 任务 T2: 增加 destination 路径边界校验
|
||
- 修改点: `src/vlm/planner.py`（必要时补充 `src/vlm/executor.py` 二次防线）
|
||
- 实施:
|
||
- 对最终 `destination` 与 `library_root` 做 canonical 关系校验
|
||
- 不在库内时直接拒绝生成/执行计划
|
||
- 交付物: 边界校验逻辑 + 错误提示 + 测试
|
||
- 验收标准:
|
||
- 包含 `../`、绝对路径逃逸等输入均被拦截
|
||
- 合法路径不受影响
|
||
|
||
3. 任务 T3: 标题与模板片段净化（防路径注入）
|
||
- 修改点: `src/vlm/enrichment.py`, `src/vlm/io.py`, `src/vlm/planner.py`
|
||
- 实施:
|
||
- 增加统一 `sanitize_path_component()`（禁止 `/`, `\\`, `..`, 控制字符）
|
||
- 所有参与目标路径拼接的字段都经过净化
|
||
- 交付物: 公共净化函数 + 调用点替换 + 回归测试
|
||
- 验收标准:
|
||
- 恶意标题不会影响目录层级
|
||
- 原有正常标题生成路径稳定
|
||
|
||
#### M2 / P1（稳定性改造）
|
||
|
||
1. 任务 T4: State 持久化改为原子写
|
||
- 修改点: `src/vlm/state.py`
|
||
- 实施:
|
||
- 使用临时文件写入 + `os.replace`
|
||
- 明确 fsync 策略（至少文件级）
|
||
- 交付物: 原子写实现 + 崩溃场景单测（可用 mock）
|
||
- 验收标准:
|
||
- 任意写入中断不会留下半截 JSON
|
||
- 反复读写结果一致
|
||
|
||
2. 任务 T5: State key 统一 canonical path
|
||
- 修改点: `src/vlm/state.py`
|
||
- 实施:
|
||
- key 写入前统一 `resolve()`（并处理不存在路径的兼容逻辑）
|
||
- 读取时保留向后兼容迁移（旧 key 可识别）
|
||
- 交付物: key 规范化 + 兼容迁移逻辑 + 测试
|
||
- 验收标准:
|
||
- 相对路径/软链接指向同一文件时状态一致
|
||
- 旧 state 文件可被平滑读取
|
||
|
||
3. 任务 T6: 收敛 CLI 里重复 CSV/JSON 读写逻辑
|
||
- 修改点: `src/vlm/cli.py`, `src/vlm/io.py`
|
||
- 实施:
|
||
- 提炼并复用 `io.py` 的统一加载/保存接口
|
||
- CLI 仅负责参数与错误展示
|
||
- 交付物: 去重后的调用链 + 回归测试
|
||
- 验收标准:
|
||
- CLI 不再手写重复解析分支
|
||
- 解析行为在各命令一致
|
||
|
||
#### M3 / P2（准确性与性能）
|
||
|
||
1. 任务 T7: 修正执行汇总统计口径重叠
|
||
- 修改点: `src/vlm/executor.py`
|
||
- 实施:
|
||
- 明确 `failed` 与 `skipped` 互斥定义
|
||
- 调整 summary 计数逻辑与输出文案
|
||
- 交付物: 计数逻辑修复 + 测试
|
||
- 验收标准:
|
||
- 相同动作不会同时计入 `failed` 与 `skipped`
|
||
- 总数与明细可对账
|
||
|
||
2. 任务 T8: 修正 duplicate report 的 size 失真
|
||
- 修改点: `src/vlm/cli.py`, `src/vlm/reports.py`
|
||
- 实施:
|
||
- `report duplicates` 重建模型时保留真实 `size_bytes`
|
||
- 报告排序按真实值生效
|
||
- 交付物: 修复 + 测试
|
||
- 验收标准:
|
||
- 重复组排序与真实文件大小一致
|
||
- 旧输入格式仍可正常输出报告
|
||
|
||
3. 任务 T9: 引入受控并发（enrichment + ffprobe）
|
||
- 修改点: `src/vlm/enrichment.py`, `src/vlm/scanner.py`, `src/vlm/config.py`
|
||
- 实施:
|
||
- 让 `enrichment_max_concurrency` 真正参与调度
|
||
- 为 ffprobe 引入线程池/进程池并控制并发上限
|
||
- 维持输出稳定顺序（必要时按输入索引回排）
|
||
- 交付物: 并发实现 + 性能对比数据 + 回归测试
|
||
- 验收标准:
|
||
- 在中等规模样本上性能显著提升
|
||
- 结果内容与串行版本一致
|
||
|
||
### 8.4 测试任务映射
|
||
|
||
1. T1 对应测试
|
||
- `tests/test_cli_parse.py` 新增: 非法 JSON 输入到 `analyze/plan` 的错误断言
|
||
|
||
2. T2/T3 对应测试
|
||
- 新增 `tests/test_path_safety.py`:
|
||
- 标题含 `../`、`/`、`\\`、控制字符
|
||
- 模板路径逃逸
|
||
- `destination` 越界拒绝
|
||
|
||
3. T4/T5 对应测试
|
||
- `tests/test_cli_state.py` 或新增 `tests/test_state_atomic.py`:
|
||
- 原子写完整性
|
||
- canonical key 去重
|
||
- 旧格式兼容
|
||
|
||
4. T6 对应测试
|
||
- `tests/test_cli_*.py` 补充接口一致性断言，验证不同命令对同类输入行为一致
|
||
|
||
5. T7/T8 对应测试
|
||
- `tests/test_executor.py`: summary 口径互斥
|
||
- `tests/test_duplicate_quality_metadata.py` 或新增 `tests/test_reports_duplicates.py`: size 排序正确性
|
||
|
||
6. T9 对应测试
|
||
- 新增 `tests/test_enrichment_concurrency.py`, `tests/test_scanner_concurrency.py`（含顺序稳定性与结果一致性）
|
||
|
||
### 8.5 执行顺序与依赖关系
|
||
|
||
1. 先做 T1/T2/T3（阻断高风险故障）。
|
||
2. 再做 T4/T5（保证持久化可靠性），T6 可与 T5 并行。
|
||
3. 最后执行 T7/T8/T9（不影响主流程安全前提下优化准确性和性能）。
|
||
4. 每个里程碑结束后运行一次全量 `uv run pytest -q`，M4 前补充一次端到端 CLI 手工回归。
|
||
|
||
### 8.6 Definition of Done（完成标准）
|
||
|
||
1. 所有 P0/P1 任务完成并合并，P2 至少完成 T7/T8。
|
||
2. 新增测试全部通过，且全量测试保持通过。
|
||
3. 风险点有对应文档说明（错误码、限制行为、兼容策略）。
|
||
4. `README.md` 或对应命令帮助文档完成必要更新。
|
||
|
||
## 9. 实施结果（本轮已完成）
|
||
|
||
### 9.1 任务完成状态
|
||
|
||
1. T1 完成: 修复 `analyze/plan` JSON 异常路径 `NameError`（模块级引入 `json`，并新增 CLI 回归测试）。
|
||
2. T2 完成: planner 增加 destination 边界校验；executor 增加二次防线。
|
||
3. T3 完成: 新增统一路径片段净化函数并用于标题生成与路径模板链路。
|
||
4. T4 完成: state 存储改为临时文件 + `fsync` + `os.replace` 原子落盘。
|
||
5. T5 完成: state key 与 `FileState.file_path` 统一 canonical path。
|
||
6. T6 完成: 收敛 CLI 重复 I/O 逻辑，`parse/report` 相关命令复用 `io.py`。
|
||
7. T7 完成: 执行汇总统计口径修正，`failed/skipped` 去重。
|
||
8. T8 完成: duplicate report 重建 `VideoFile` 时保留真实 size，并修复排序依据。
|
||
9. T9 完成: enrichment 与 scan(ffprobe) 引入受控并发，并保持结果顺序稳定。
|
||
|
||
### 9.2 新增/更新测试
|
||
|
||
1. 新增 `tests/test_cli_json_errors.py`（覆盖 analyze/plan 非法 JSON）。
|
||
2. 新增 `tests/test_path_safety.py`（路径净化、越界拦截、执行二次防线）。
|
||
3. 新增 `tests/test_config_concurrency.py`（并发参数校验）。
|
||
4. 更新 `tests/test_state.py`（原子写与 canonical key）。
|
||
5. 更新 `tests/test_scanner.py`（扫描并发行为）。
|
||
6. 更新 `tests/test_enrichment.py`（enrichment 并发行为）。
|
||
7. 更新 `tests/test_executor.py`（汇总口径变化断言）。
|
||
8. 更新 `tests/test_reports.py`（quality size 排序回归）。
|
||
|
||
### 9.3 文档更新
|
||
|
||
1. `README.md` 已补充路径安全、执行防线、统计口径、并发行为说明。
|
||
2. 本文档（`codex_review.md`）已补充实施结果与验证证据。
|