# Code Review Report (Verified)

## Scope
- Reviewed Python sources under `src/vlm/`, tests under `tests/`, and docs (`README.md`, `AGENTS.md`).
- Verified behavior on **February 9, 2026** with:
  - `uv run --with pytest --with hypothesis pytest -q`
  - `uv run --with pytest --with hypothesis pytest -q --ignore=tests/test_executor.py --ignore=tests/test_quarantine.py`
  - Targeted runtime checks for logging init and `vlm --help` behavior.

## Summary
- Confirmed **3 concrete code issues** and **1 conditional robustness risk**.
- The previous claim that ignored-suite tests had dozens of logging-related failures is **not reproducible** in current repo state.
- Found one additional missed issue family: timezone relabeling logic also exists in `src/vlm/reports.py`.

## Findings

### P1 - Test imports use wrong module path (confirmed)
- Files:
  - `tests/test_executor.py:13`
  - `tests/test_executor.py:14`
  - `tests/test_quarantine.py:8`
  - `tests/test_quarantine.py:9`
  - `tests/test_quarantine.py:10`
- Tests import `src.vlm...` instead of installed package path `vlm...`.
- Impact: test collection stops with `ModuleNotFoundError: No module named 'src'`.
- Recommended fix:
  - Replace imports in affected tests:
    - `from src.vlm.executor import ExecutionEngine` -> `from vlm.executor import ExecutionEngine`
    - `from src.vlm.models import ...` -> `from vlm.models import ...`
    - `from src.vlm.quarantine import QuarantineManager` -> `from vlm.quarantine import QuarantineManager`
    - `from src.vlm.config import Config` -> `from vlm.config import Config`
  - Re-run: `uv run --with pytest --with hypothesis pytest -q`

### P2 - Naive datetime is relabeled as UTC instead of converted (confirmed)
- Files:
  - `src/vlm/scanner.py:148`
  - `src/vlm/scanner.py:381`
  - `src/vlm/scanner.py:439`
  - `src/vlm/reports.py:92`
  - `src/vlm/reports.py:132`
- Pattern uses `replace(tzinfo=timezone.utc)` for naive values, which does not perform timezone conversion.
- Impact: exported timestamps can be offset by local timezone difference.
- Recommended fix:
  - Normalize file mtime as timezone-aware at source:
    - In `src/vlm/scanner.py:148`, prefer `datetime.fromtimestamp(stat.st_mtime, tz=timezone.utc)`.
  - In exporters (`scanner.py`, `reports.py`), avoid relabeling:
    - If timestamp is naive, interpret it as local time then convert:
      - `dt_local = video_file.modified_timestamp.astimezone()` (local tz attach)
      - `modified_utc = dt_local.astimezone(timezone.utc)`
    - Or enforce invariant that `VideoFile.modified_timestamp` is always aware UTC and simplify output logic.
  - Add regression tests for non-UTC local timezone scenarios.

### P3 - `error_count` in scanner is dead code (confirmed)
- Files:
  - `src/vlm/scanner.py:53`
  - `src/vlm/scanner.py:64`
- `error_count` is initialized and conditionally logged, but never incremented.
- Impact: scan summary can underreport encountered scan errors.
- Recommended fix:
  - Preferred: remove `error_count` and rely on structured per-error logs.
  - Alternative: return `(video_file, had_error)` from recursive scanner or propagate error counters upward and increment on exception branches at `src/vlm/scanner.py:107` and `src/vlm/scanner.py:112`.

### P3 - Logging setup is not fault-tolerant on unwritable log paths (conditional risk)
- Files:
  - `src/vlm/logging_config.py:63`
  - `src/vlm/logging_config.py:87`
- `setup_logging()` unconditionally creates log directory and `RotatingFileHandler`; unwritable paths raise (`PermissionError`/`OSError`).
- Important boundary:
  - Verified: `vlm --help` does **not** trigger this path (Click help exits before command callback body).
  - Still true: regular command initialization can fail if effective log path is unwritable.
- Recommended fix:
  - Wrap file-handler setup in `try/except (OSError, PermissionError)` and keep console handler active.
  - Emit a warning to stderr/log once: file logging disabled due to permission/path issue.
  - Optionally allow explicit `log_dir` in config/CLI to avoid hardcoded home-path dependency in restricted runtimes.

### P3 - Default home-based paths are resolved at import time (new)
- Files:
  - `src/vlm/logging_config.py:17`
  - `src/vlm/cli.py:19`
- `DEFAULT_LOG_DIR` and `DEFAULT_CONFIG_PATH` use `Path.home()` during module import.
- Impact:
  - Runtime monkeypatching/tests cannot redirect defaults reliably.
  - In embedded or dynamically switched user contexts, defaults may become stale.
- Recommended fix:
  - Replace import-time constants with runtime helpers:
    - `def default_log_dir() -> Path: return Path.home() / ".vlm" / "logs"`
    - `def default_config_path() -> Path: return Path.home() / ".vlm" / "config.yaml"`
  - Resolve defaults inside `setup_logging()` / Click option callback initialization.

## Test Evidence
- `uv run --with pytest --with hypothesis pytest -q`:
  - fails at collection with 2 errors (`tests/test_executor.py`, `tests/test_quarantine.py`) due to `src.vlm` imports.
- `uv run --with pytest --with hypothesis pytest -q --ignore=tests/test_executor.py --ignore=tests/test_quarantine.py`:
  - **293 passed**, no observed logging-related mass failures.
- Direct runtime check:
  - `setup_logging(log_dir=Path('/sys/...'))` raises `PermissionError`, confirming the conditional logging robustness risk.

## Recommended Fix Order
1. Fix test imports from `src.vlm...` to `vlm...` so full suite can run.
2. Correct timezone handling in both `scanner.py` and `reports.py`.
3. Either remove `error_count` or increment it on scan exceptions.
4. Make logging initialization resilient (fallback to console-only logging when file logging setup fails).
5. Move home-based defaults from import-time constants to runtime-resolved helpers.

## Clarifications to Keep in Future Reports
- Distinguish **confirmed current failures** from **environment-dependent risks**.
- Include exact commands and outputs used for evidence.
- Avoid claiming `--help` initialization failures unless explicitly reproduced for Click group callbacks.

## Remediation Status (Implemented on February 9, 2026)
- P1 test import path issue: **fixed** (`tests/test_executor.py`, `tests/test_quarantine.py` now import from `vlm...`).
- P2 timezone relabeling issue: **fixed** in `src/vlm/scanner.py` and `src/vlm/reports.py` via local-to-UTC conversion instead of relabeling.
- P3 scanner `error_count` dead code: **fixed** by removing unused counter logic.
- P3 logging unwritable path risk: **fixed** by console-only fallback in `setup_logging()` when file logging cannot initialize.
- P3 import-time home defaults: **fixed** using runtime helpers (`default_log_dir()`, `default_config_path()`).

## Post-Fix Validation
- Command: `uv run --with pytest --with hypothesis pytest -q`
- Result: **374 passed**

## Detailed Code & Documentation Remediation Plan

### Goal and success criteria
- Goal: fix only verified issues above, keep behavior stable, and make evidence reproducible.
- Done when:
  - `uv run --with pytest --with hypothesis pytest -q` passes.
  - No `src.vlm` imports remain in tests.
  - UTC timestamp output logic is deterministic and covered by tests.
  - Logging keeps CLI usable even when file log path is unwritable.
  - Docs reflect the new behavior and troubleshooting paths.

### Phase 0 - Baseline snapshot (before edits)
- Run and save baseline:
  - `uv run --with pytest --with hypothesis pytest -q`
- Capture current known failing stack traces (import errors) for before/after comparison.
- Keep this scope boundary:
  - No unrelated refactors.
  - No behavior changes outside listed findings.

### Phase 1 - Fix incorrect test import paths (P1)
- Code changes:
  - `tests/test_executor.py`: replace all `from src.vlm...` with `from vlm...`.
  - `tests/test_quarantine.py`: replace all `from src.vlm...` with `from vlm...`.
- Verification:
  - `uv run --with pytest --with hypothesis pytest -q tests/test_executor.py tests/test_quarantine.py`
  - `uv run --with pytest --with hypothesis pytest -q`
- Acceptance:
  - Collection succeeds for full suite.
  - No `ModuleNotFoundError: No module named 'src'`.

### Phase 2 - Correct timezone handling in scanner/reports (P2)
- Code changes:
  - `src/vlm/scanner.py`:
    - Ensure file mtime uses aware UTC at creation (`datetime.fromtimestamp(..., tz=timezone.utc)`).
    - Remove/replace any naive `replace(tzinfo=timezone.utc)` relabeling.
  - `src/vlm/reports.py`:
    - Remove/replace naive `replace(tzinfo=timezone.utc)` relabeling.
    - Keep output conversions explicit and consistent (UTC output contract).
- Tests to add/update:
  - Add scanner/report tests that simulate non-UTC local timezone interpretation for naive datetimes.
  - Assert resulting exported timestamps are correct UTC instants (not relabeled local clock time).
- Verification:
  - `uv run --with pytest --with hypothesis pytest -q tests/test_scanner.py tests/test_reports.py`
  - `uv run --with pytest --with hypothesis pytest -q`
- Acceptance:
  - All timezone-related tests pass.
  - No remaining `replace(tzinfo=timezone.utc)` on naive datetimes in scanner/report export paths.

### Phase 3 - Remove or correctly implement scanner `error_count` (P3)
- Preferred implementation:
  - Remove unused `error_count` state and its summary branch if not needed by product behavior.
- Alternative (if summary count is required by UX):
  - Increment count in scan exception branches and propagate to final summary.
- Verification:
  - Update/add scanner tests to verify chosen behavior.
  - `uv run --with pytest --with hypothesis pytest -q tests/test_scanner.py`
- Acceptance:
  - No dead summary logic remains.
  - Error reporting behavior is explicit and test-covered.

### Phase 4 - Make logging initialization fault-tolerant (P3 risk)
- Code changes:
  - `src/vlm/logging_config.py`:
    - Wrap file-log directory creation and `RotatingFileHandler` setup with `try/except (OSError, PermissionError)`.
    - Always keep console handler active.
    - Emit one clear warning when file logging is disabled.
- Tests to add/update:
  - Add a test for unwritable log dir scenario (mocking mkdir/handler failure) to confirm no hard failure.
  - Keep check that CLI help path remains unaffected.
- Verification:
  - `uv run --with pytest --with hypothesis pytest -q tests/test_logging.py tests/test_cli.py`
  - `uv run --with pytest --with hypothesis pytest -q`
- Acceptance:
  - Commands still run with console logs when file logging setup fails.
  - Failure mode is warning-only, not process abort.

### Phase 5 - Resolve import-time home defaults (P3 new)
- Code changes:
  - `src/vlm/logging_config.py`: replace `DEFAULT_LOG_DIR = Path.home()...` with runtime helper.
  - `src/vlm/cli.py`: replace `DEFAULT_CONFIG_PATH = Path.home()...` with runtime helper.
  - Resolve defaults when functions/options execute, not during module import.
- Tests to add/update:
  - Add tests that monkeypatch `Path.home()`/environment and assert defaults resolve at runtime.
- Verification:
  - `uv run --with pytest --with hypothesis pytest -q tests/test_cli*.py tests/test_logging.py`
  - `uv run --with pytest --with hypothesis pytest -q`
- Acceptance:
  - Default paths reflect runtime environment in tests and execution.
  - No import-time home path freeze remains for these defaults.

### Documentation updates (must ship with code)
- `README.md`:
  - Add troubleshooting note: file logging may fall back to console-only if log dir is unwritable.
  - Clarify timestamp output expectation (UTC).
- `REVIEW_REPORT.md`:
  - Mark each finding status: `open -> fixed`, and record PR/commit reference when available.
- Optional (if present in repo docs):
  - Add developer note for test import convention: always import from package root (`vlm`), not `src.vlm`.

### Execution checklist (single pass)

## Enrichment 增量评审报告（2026-02-10）

### 评审范围
- 文件：`src/vlm/enrichment.py`
- 关注点：刷新一致性、配置可观测性、命名输出质量

### 总体评分（修复后）
- 当前得分：**92 / 100**
- 评分依据：
  - 正确性（40 分）：37/40（refresh 无命中不再残留旧字段）
  - 健壮性（30 分）：27/30（未知 provider 已 fail-fast）
  - 输出质量（20 分）：18/20（display title 去重）
  - 可维护性（10 分）：10/10（TMDB-only，分支复杂度降低）

### 问题明细与修复状态

#### 1) P1 - 刷新失败时未清理陈旧富化字段（**fixed**）
- 修复位置：`src/vlm/enrichment.py:378`
- 修复内容：
  - `_apply_payload` 从“仅写入非 None”改为“payload 含 key 即覆盖”，支持将字段显式清空为 `None`。
- 验证：
  - `tests/test_enrichment.py:187` 覆盖 refresh 后无匹配场景，断言旧 `canonical_id/title_zh/title_en/reputation_*` 被清空，`enrichment_confidence == 0.0`。

#### 2) P2 - 未识别 provider 被静默忽略（**fixed**）
- 修复位置：
  - `src/vlm/enrichment.py:157`
  - `src/vlm/config.py:271`
- 修复内容：
  - `_build_providers` 对未知 provider 直接抛 `ValueError`（fail-fast）。
  - 配置校验新增白名单，仅允许 `tmdb`。
  - 同步重构为 TMDB-only，移除 enrich 主流程中的 Douban 分支。
- 验证：
  - `tests/test_enrichment.py:176` 覆盖未知 provider 报错。
  - `tests/test_config.py:447` 覆盖配置校验拒绝不支持 provider。

#### 3) P2 - display_title fallback 可能重复标题（**fixed**）
- 修复位置：`src/vlm/enrichment.py:423`
- 修复内容：
  - `_build_display_title` 增加同值去重逻辑；`title_zh/title_en` 回退同值时只保留一个。
- 验证：
  - `tests/test_enrichment.py:260` 断言双空回退时输出单标题而非重复标题。

### 结果
1. 三项问题均已修复并有测试覆盖。
2. enrich 已切换为 TMDB-only，配置和文档已同步。
3. 全量测试通过：`uv run --with pytest --with hypothesis pytest -q` -> `411 passed`。

### 修复后目标分
- 达成：**92 / 100**。
1. Apply Phase 1, run focused + full tests.
2. Apply Phase 2, add timezone tests, run focused + full tests.
3. Apply Phase 3, run scanner tests + full tests.
4. Apply Phase 4, run logging/CLI tests + full tests.
5. Apply Phase 5, run CLI/logging tests + full tests.
6. Update docs and close report statuses.

### Risks and mitigations
- Risk: timezone tests become platform-dependent.
  - Mitigation: use explicit tz-aware fixtures and deterministic conversion assertions.
- Risk: logging fallback emits duplicate warnings.
  - Mitigation: guard warning emission in setup path or test for idempotent configuration.
- Risk: changing default path resolution affects existing tests.
  - Mitigation: update tests to assert runtime-resolved behavior explicitly.