153 lines
7.7 KiB
Markdown
153 lines
7.7 KiB
Markdown
# Technical review: video-library-manager
|
||
|
||
## Metadata
|
||
|
||
| Field | Value |
|
||
|--------|--------|
|
||
| Repository path | `dl-organizer` (package `vlm`) |
|
||
| Verification date (UTC) | 2026-04-06 |
|
||
| Git revision verified | `ea21e15` |
|
||
| Test run | `uv run pytest -q` → **496 passed** |
|
||
|
||
## Scope
|
||
|
||
- Source: `src/vlm/**/*.py`, `tests/`
|
||
- Config: `pyproject.toml`
|
||
- Assumption: single-user local library; artifacts semi-trusted; no formal threat model in repo.
|
||
|
||
## Executive summary
|
||
|
||
Architecture and tests are solid. Verified gaps: **no `library_root` check on move/rename source** in executor; **`by_quality` falls back to index 0 without surfacing reason**; **quarantine raises `ValueError` for unsupported category** while `execute_plan` has no per-operation try/except, so one bad op can abort the run; **duplicate join uses exact string path keys**; **`find` non-zero exit still consumes stdout**.
|
||
|
||
Weighted score (0–10): **8.0**.
|
||
|
||
## Score rubric
|
||
|
||
| Criterion | Score | Notes |
|
||
|-----------|-------|--------|
|
||
| Module boundaries / pipeline | 8.5 | Commands → domain modules; scan→parse→analyze→plan→execute is explicit. |
|
||
| Execution safety (filesystem) | 7.0 | Destination under root checked; **source not checked** for move/rename. |
|
||
| Planning / duplicate logic | 7.5 | Multiple strategies; string path identity; silent `by_quality` fallback; unknown `strategy` → keep index 0 (`duplicate_resolve.py:48`). |
|
||
| Data I/O & validation | 8.0 | `io.py` validates identities/analysis/plan shapes; validated objects remain dict-typed at boundaries (`type: ignore` in places). |
|
||
| Error handling consistency | 7.5 | Quarantine: `raise` vs `OperationResult` mismatch on category rejection. |
|
||
| Test signal | 8.5 | Broad `tests/`; path safety and resolver covered (`test_path_safety.py`, `test_duplicate_resolve.py`, etc.). |
|
||
| Dependencies | 9.0 | Runtime: `click`, `pyyaml` only (`pyproject.toml:7–9`). |
|
||
|
||
## Findings
|
||
|
||
### F1 [High] Move/rename: source not constrained to `library_root`
|
||
|
||
**Evidence:** `ExecutionEngine._perform_operation` checks destination with `is_within_root(..., self.config.library_root)` then calls `operation.source_path.rename(...)` without checking the source (`executor.py:354–377`).
|
||
|
||
**Impact:** Malformed or hand-edited `plan.json` can rename **from any path the process can access** into the library tree. `QuarantineManager.quarantine_file` instead requires `file_path.relative_to(self.config.library_root)` (`quarantine.py:133–137`).
|
||
|
||
**Recommendation:** For `move` / `rename`, require `is_within_root(operation.source_path, self.config.library_root)` (after `Path` resolution policy is defined). Fail with `OperationResult(success=False)`. Optional config gate for deliberate “import from outside,” default off. Add test alongside `tests/test_path_safety.py:76–103`.
|
||
|
||
---
|
||
|
||
### F2 [Medium] `by_quality`: silent fallback to first item
|
||
|
||
**Evidence:** `choose_keep_index` — `if quality_comparison is None or len(quality_comparison) != len(items): return 0` (`duplicate_resolve.py:40–42`).
|
||
|
||
**Impact:** Behavior equals **input order**, not quality, with no structured flag in plan metadata from this function alone.
|
||
|
||
**Recommendation:** Return `None` and skip auto-quarantine for that group, or require CLI/plan failure when `duplicate_keep == by_quality` and rows misaligned, or write explicit `metadata` / summary line when fallback occurs.
|
||
|
||
---
|
||
|
||
### F3 [Medium] Duplicate group ↔ identities join: exact path strings
|
||
|
||
**Evidence:** `path_to_index = {str(vf.path): i for ...}` and `indices = [path_to_index[p] for p in paths if p in path_to_index]` (`planner.py:108–111`). Analysis `files` must match `str(VideoFile.path)` byte-for-byte.
|
||
|
||
**Impact:** Symlinks, differing normalization between analysis writer and identities loader, or OS case rules can **drop** files from duplicate resolution.
|
||
|
||
**Recommendation:** Normalize keys (e.g. `utils.canonical_path_str`) at both analysis emission and plan consumption; document contract; regression test if symlinks are in scope.
|
||
|
||
---
|
||
|
||
### F4 [Low–Medium] Quarantine: `ValueError` vs `OperationResult`
|
||
|
||
**Evidence:** Unsupported category path does `raise ValueError(error_msg)` after logging (`quarantine.py:116–129`). Other failure modes return `OperationResult`.
|
||
|
||
**Evidence:** `execute_plan` loop calls `execute_operation` with no try/except (`executor.py:110–112`).
|
||
|
||
**Impact:** One invalid quarantine operation (e.g. manual plan) can **abort** the whole execute pass instead of recording a single failed result.
|
||
|
||
**Recommendation:** Return failed `OperationResult` for unsupported category; reserve exceptions for invariant violations only.
|
||
|
||
---
|
||
|
||
### F5 [Low] `find` non-zero exit: stdout still parsed
|
||
|
||
**Evidence:** On `process.returncode != 0`, code logs stderr if present, then always parses `stdout` into paths (`scanner.py:178–192`).
|
||
|
||
**Impact:** Partial or stale listing possible without hard failure.
|
||
|
||
**Recommendation:** Document behavior; optionally fail if `returncode != 0` and empty result, or add `--strict-scan`.
|
||
|
||
---
|
||
|
||
### F6 [Low] Optional dependencies: `textual` in both `dev` and `tui`
|
||
|
||
**Evidence:** `pyproject.toml:13–16` (`dev`), `18–19` (`tui`).
|
||
|
||
**Impact:** Install surface ambiguity only.
|
||
|
||
**Recommendation:** Document that `dev` includes Textual for TUI-related tests, or slim `dev` if CI wants fewer deps.
|
||
|
||
---
|
||
|
||
### F7 [Informational] Validated JSON remains dict-typed at I/O edge
|
||
|
||
**Evidence:** `validate_plan_json` returns `dict` (`io.py:224–251`); constructors may still bridge dict ↔ dataclass elsewhere.
|
||
|
||
**Impact:** Field drift between validator and `models.py` possible over time.
|
||
|
||
**Recommendation:** Single factory path: validated dict → domain objects for hot paths.
|
||
|
||
---
|
||
|
||
### F8 [Informational] Unknown `duplicate_keep` strategy string
|
||
|
||
**Evidence:** After strategy checks, `choose_keep_index` falls through to `return 0` (`duplicate_resolve.py:48`).
|
||
|
||
**Impact:** Config typo **keeps first duplicate** like `first_seen` without failing load.
|
||
|
||
**Recommendation:** Validate `duplicate_keep` in `config.py` / `validate_config` against an allowlist; reject unknown values.
|
||
|
||
## Verified strengths
|
||
|
||
| Claim | Evidence |
|
||
|--------|----------|
|
||
| `find` invoked without shell string | argv list: `subprocess.Popen(command, ...)` (`scanner.py:171–172`, `164–169`). |
|
||
| Path component sanitization | `sanitize_path_component` strips controls and separators (`utils.py:65–75`); planner uses it before templates (`planner.py:301`, `419`). |
|
||
| Destination under library in planner | `is_within_root(destination, config.library_root)` (`planner.py:321`, `440`). |
|
||
| Quarantine manifest two-phase | Pending manifest write before `rename` (`quarantine.py:221–268` region). |
|
||
| JSON artifact validation | `_validate_parsed_identities_json`, `_validate_analysis_json`, `validate_plan_json` (`io.py:157–251`). |
|
||
| Parallel ffprobe | `ThreadPoolExecutor` when `include_video_metadata` and more than one path (`scanner.py:85–101`). |
|
||
|
||
## Priority order
|
||
|
||
1. F1
|
||
2. F2
|
||
3. F4
|
||
4. F3
|
||
5. F8, F5, F6, F7
|
||
|
||
## Verification log (document vs codebase)
|
||
|
||
| Statement in this doc | Checked against |
|
||
|------------------------|-----------------|
|
||
| F1 source not under root guard | `executor.py:321–377` |
|
||
| F2 by_quality fallback | `duplicate_resolve.py:40–43` |
|
||
| F3 path_to_index | `planner.py:108–111` |
|
||
| F4 raise + no try in loop | `quarantine.py:116–129`, `executor.py:110–112` |
|
||
| F5 find returncode | `scanner.py:178–192` |
|
||
| F6 pyproject optional | `pyproject.toml:12–20` |
|
||
| F7 validate_plan_json returns dict | `io.py:224–251` |
|
||
| F8 unknown strategy | `duplicate_resolve.py:36–48` |
|
||
| Strengths table | `scanner.py`, `utils.py`, `planner.py`, `quarantine.py`, `io.py` as cited |
|
||
| 496 tests passed | `uv run pytest -q` on 2026-04-06 |
|
||
|
||
This document is evidence-based against the paths above; behavior not re-listed here is **not** claimed verified.
|