diff --git a/REVIEW_REPORT.md b/REVIEW_REPORT.md index 4a4b56c..f247c06 100644 --- a/REVIEW_REPORT.md +++ b/REVIEW_REPORT.md @@ -1,287 +1,165 @@ -> [!NOTE] -> Status: Historical snapshot. Current refactor results and validated baseline are tracked in `CHANGELOG.md` (updated 2026-02-16). +# Code & Documentation Review Report -# Code Review Report (Verified) +**Date:** 2026-04-07 +**Reviewer:** Forge ## 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`. +This report was updated by verifying `docs/TECHNICAL_REVIEW.md` against the current codebase and aligning conclusions to evidence. -## Findings +Primary verification inputs: +- `docs/TECHNICAL_REVIEW.md:1-152` +- `src/vlm/executor.py:110-112` +- `src/vlm/executor.py:321-377` +- `src/vlm/duplicate_resolve.py:36-48` +- `src/vlm/planner.py:108-111` +- `src/vlm/quarantine.py:116-129` +- `src/vlm/scanner.py:178-192` +- `src/vlm/io.py:225-251` +- `pyproject.toml:12-20` -### 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` +Validation baseline: +- `uv run pytest -q` → **496 passed** (as recorded in `docs/TECHNICAL_REVIEW.md:10`). -### 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`. +## Overall Score -### 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. +## **8.0 / 10** -### 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. +### Score breakdown +- **Module boundaries / pipeline:** 8.5/10 +- **Execution safety (filesystem):** 7.0/10 +- **Planning / duplicate logic:** 7.5/10 +- **Data I/O & validation:** 8.0/10 +- **Error handling consistency:** 7.5/10 +- **Test signal:** 8.5/10 +- **Dependencies:** 9.0/10 -## 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. +## Verified strengths -## 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. +1. **Pipeline and module boundaries are clean and explicit** (scan → parse → analyze → plan → execute). + - `src/vlm/commands/scan.py:14-97` + - `src/vlm/commands/parse.py:17-166` + - `src/vlm/commands/analyze.py:25-124` + - `src/vlm/commands/plan.py:14-112` + - `src/vlm/commands/execute.py:37-249` -## 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()`). +2. **Defensive safety measures exist in key areas** (destination root checks, quarantine manifest two-phase flow, JSON schema checks). + - `src/vlm/executor.py:354-377` + - `src/vlm/quarantine.py:221-320` + - `src/vlm/io.py:225-251` -## Post-Fix Validation -- Command: `uv run --with pytest --with hypothesis pytest -q` -- Result: **374 passed** +3. **Testing coverage is broad and currently green.** + - `docs/TECHNICAL_REVIEW.md:10` + - `tests/test_path_safety.py:1-122` + - `tests/test_duplicate_resolve.py:1-184` -## 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. +## Verified findings -### 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. +### F1) Move/Rename source path is not constrained to `library_root` (High) -### 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'`. +- `_perform_operation` validates destination under root, but does not enforce source under root before rename. + - `src/vlm/executor.py:355-377` -### 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. +**Impact:** A crafted/manual plan can attempt renames from paths outside managed library boundaries. -### 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. +### F2) `by_quality` silently falls back to first item on quality-data mismatch (Medium) -### 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. +- `choose_keep_index` returns index `0` if `quality_comparison` is missing/misaligned. + - `src/vlm/duplicate_resolve.py:40-43` -### 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`. +**Impact:** Behavior degrades to input-order selection without explicit operator visibility. -### Execution checklist (single pass) +--- -## Enrichment 增量评审报告(2026-02-10) +### F3) Duplicate resolution join relies on exact string path matches (Medium) -### 评审范围 -- 文件:`src/vlm/enrichment.py` -- 关注点:刷新一致性、配置可观测性、命名输出质量 +- Planner builds `path_to_index` from `str(vf.path)` and joins using exact string equality. + - `src/vlm/planner.py:108-111` -### 总体评分(修复后) -- 当前得分:**92 / 100** -- 评分依据: - - 正确性(40 分):37/40(refresh 无命中不再残留旧字段) - - 健壮性(30 分):27/30(未知 provider 已 fail-fast) - - 输出质量(20 分):18/20(display title 去重) - - 可维护性(10 分):10/10(TMDB-only,分支复杂度降低) +**Impact:** Path normalization differences (symlink/case/serialization form) can silently exclude items from duplicate handling. -### 问题明细与修复状态 +--- -#### 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`。 +### F4) Quarantine category rejection raises exception while execute loop lacks per-op guard (Medium) -#### 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。 +- Quarantine rejects unsupported categories with `raise ValueError`. + - `src/vlm/quarantine.py:116-129` +- Execute loop iterates operations without local try/except around each operation call. + - `src/vlm/executor.py:110-112` -#### 3) P2 - display_title fallback 可能重复标题(**fixed**) -- 修复位置:`src/vlm/enrichment.py:423` -- 修复内容: - - `_build_display_title` 增加同值去重逻辑;`title_zh/title_en` 回退同值时只保留一个。 -- 验证: - - `tests/test_enrichment.py:260` 断言双空回退时输出单标题而非重复标题。 +**Impact:** One invalid quarantine operation can abort the run instead of being recorded as a single failed result. -### 结果 -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. +### F5) `find` non-zero exit still allows stdout parsing (Low) + +- Scanner logs non-zero return issues but still parses emitted stdout. + - `src/vlm/scanner.py:178-192` + +**Impact:** Partial scan results may be accepted without strict failure semantics. + +--- + +### F6) Optional dependency overlap (`textual` in both `dev` and `tui`) (Low) + +- `textual` appears in both extras. + - `pyproject.toml:13-20` + +**Impact:** Minor install-surface ambiguity. + +--- + +### F7) Plan JSON validation returns dict-typed structure at boundary (Informational) + +- `validate_plan_json` validates shape but returns plain `dict`. + - `src/vlm/io.py:225-251` + +**Impact:** Validator/model drift risk over time if object construction paths diverge. + +--- + +### F8) Unknown duplicate strategy defaults to first item (Informational) + +- Unrecognized `strategy` falls through to `return 0`. + - `src/vlm/duplicate_resolve.py:48` + +**Impact:** Configuration typo can silently behave as first-seen policy. + +--- + +## Advice (priority order) + +1. **Add source-root validation for move/rename execution path** and test for crafted plan source outside root. + - `src/vlm/executor.py:335-377` + +2. **Make `by_quality` mismatch explicit** (error/metadata flag/manual fallback), rather than silent index-0 default. + - `src/vlm/duplicate_resolve.py:40-43` + +3. **Unify quarantine error contract**: return failed `OperationResult` for unsupported categories (avoid run-aborting exception path). + - `src/vlm/quarantine.py:116-129` + - `src/vlm/executor.py:110-112` + +4. **Normalize duplicate path keys consistently** across analysis emission and planning consumption. + - `src/vlm/planner.py:108-111` + +5. **Harden duplicate strategy validation in config** to reject unknown values at load/validate time. + - `src/vlm/duplicate_resolve.py:36-48` + - `src/vlm/config.py:224-340` + +6. **Clarify or gate partial scan behavior on `find` failures** (strict mode or stronger warning semantics). + - `src/vlm/scanner.py:178-192` + +--- + +## Closing + +The codebase remains strong in structure and testing discipline. The key improvements are concentrated in execution guardrails and duplicate-resolution determinism. Addressing the top three items above should materially improve operational safety and predictability. -### 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.