Files

T

windyboy 2dd329cba9 refactor CLI command modules and synchronize docs

2026-02-16 12:31:26 +08:00

47 KiB

Raw Blame History

Note

Status: Historical snapshot. Current refactor results and validated baseline are tracked in CHANGELOG.md (updated 2026-02-16).

Video Library Manager - Architecture Review

Review Date: 2026-02-13 Reviewer: Claude (Sonnet 4.5) Project Version: Commit 1f55eab Review Scope: Full architecture, workflow design, implementation quality

Executive Summary

Video Library Manager (VLM) is a well-architected Python CLI tool with a safety-first design philosophy. The multi-stage pipeline architecture (scan → parse → enrich → analyze → plan → execute) effectively separates concerns and provides human-in-the-loop checkpoints before file modifications.

Overall Assessment: ⭐⭐⭐⭐ (4/5)

Key Strengths

✅ Clear separation of concerns with single-responsibility modules
✅ Safety-first design with dry-run, rollback, and quarantine mechanisms
✅ Graceful degradation when optional dependencies (ffprobe) are unavailable
✅ Incremental caching for expensive operations (metadata, enrichment)

Critical Issues Identified

❌ Data loss during workflow: Metadata (resolution/codec) lost in analyze stage
❌ No unified I/O layer: Duplicated CSV/JSON loading, no schema versioning
❌ Path validation missing: Absolute paths can become stale between stages
❌ Non-atomic operations: Multi-step operations can fail partially

Priority Recommendations

Immediate: Fix identity reconstruction to preserve video metadata
Short-term: Consolidate I/O operations, add schema versioning
Long-term: Implement explicit state machine for workflow progression

1. Workflow Design Analysis

1.1 Current Architecture

┌─────────┐     ┌───────┐     ┌─────────┐     ┌─────────┐     ┌──────┐     ┌─────────┐
│  Scan   │────▶│ Parse │────▶│ Enrich  │────▶│ Analyze │────▶│ Plan │────▶│ Execute │
└─────────┘     └───────┘     └─────────┘     └─────────┘     └──────┘     └─────────┘
     │               │              │               │              │              │
inventory.csv  identities.json  (in-place)   analysis.json   plan.json   rollback.json

Design Pattern: Linear pipeline with explicit file I/O between stages

1.2 Strengths

✅ Clear Stage Separation

Each stage has a well-defined input/output contract:

Scan: library_root → inventory.csv (VideoFile list)
Parse: inventory.csv → identities.json (Identity objects)
Enrich: identities.json → identities.json (in-place update)
Analyze: identities.json → analysis.json (gaps + duplicates)
Plan: identities.json + analysis.json → plan.json (operations)
Execute: plan.json → file system changes + rollback.json

This separation allows:

Running stages independently
Manual review of intermediate artifacts
Debugging at each stage boundary

✅ Read-First Principle

Most stages are read-only until final execution:

Scan: read-only file system traversal
Parse: read CSV, write JSON (no file moves)
Enrich: update JSON in-place (no file moves)
Analyze: read-only analysis
Plan: read-only planning

Only Execute stage modifies the file system, minimizing risk.

✅ Human-in-the-Loop Checkpoints

Three review points before file modifications:

After analyze: Review gaps and duplicates in analysis.json
After plan: Review proposed operations in plan.json
Before execute: Dry-run preview with vlm execute (no --confirm)

1.3 Weaknesses

❌ No Transaction Semantics

Issue: Each stage is independent with no rollback for failed intermediate stages.

Example Scenario:

vlm enrich  # Crashes after enriching 5,000 of 10,000 records
# Result: identities.json partially updated, no way to resume
# User must re-run --refresh-all (re-processes all 10,000)

Impact:

Wasted API calls for already-enriched records
Potential data inconsistency if crash during file write
No incremental resume capability

Recommendation:

Add checkpoint files (e.g., .enrich_checkpoint) with last processed index
Detect checkpoint on startup, resume from last position
Or: Use atomic file operations (write to temp, rename)

❌ Implicit State Progression

Issue: Workflow progression is tracked only by file existence, not explicit state.

Current State Tracking:

# state.py exists but is NOT integrated into main workflow
# Main workflow relies on:
if os.path.exists("inventory.csv"):  # Scan completed
if os.path.exists("identities.json"):  # Parse completed
if os.path.exists("analysis.json"):  # Analyze completed

Consequences:

Can't detect if file was manually edited (invalidating workflow state)
No way to know if scan completed successfully or crashed mid-way
State management (vlm state commands) is disconnected from main pipeline

Recommendation:

Add .vlm/workflow_state.json tracking stage completion and file hashes
Validate file integrity before each stage (check hash against expected)
Integrate StateManager into main workflow, auto-mark files as "processed"

❌ Lossy Data Reconstruction

Critical Issue: Video metadata lost during analyze stage.

Root Cause:

# analysis.py lines 70-123: detect_duplicates()
# Loads identities.json and reconstructs VideoFile objects
# But io.py reconstruction loses resolution/codec/bitrate

# io.py:48-60 _create_video_file_from_identity()
vf = VideoFile(
    path=Path(record["filename"]),  # Only basic fields
    filename=record["filename"],
    size_bytes=0,  # ⚠️ Defaulted to 0!
    modified_timestamp=datetime.now(timezone.utc),
    category=record.get("category", "other"),
    resolution=None,  # ⚠️ Lost!
    codec=None,      # ⚠️ Lost!
    duration_seconds=None,
    bitrate_kbps=None,
)

Impact on Duplicate Resolution:

by_quality strategy fails because resolution/codec are None
Quality comparison (analysis.py:115) returns incomplete metadata
Users relying on duplicate_keep: by_quality get incorrect results

Evidence:

# duplicate_resolve.py:67-85 _quality_score()
resolution_rank = {
    "4K": 4, "2160p": 4,
    "1080p": 3,
    "720p": 2,
    "480p": 1,
}.get(vf.resolution, 0)  # Always returns 0 if resolution=None!

Recommendation:

Option 1: Store full VideoFile in identities.json (not just identity)

{
  "movies": [
    {
      "identity": {...},
      "video_file": {
        "resolution": "1080p",
        "codec": "x265",
        ...
      }
    }
  ]
}

Option 2: Load metadata from inventory.csv during analyze stage

# Load both inventory.csv and identities.json
# Join on path to get full metadata

2. Data Flow Analysis

2.1 Current Data Paths

Stage 1: Scan
  VideoFile(path, filename, size_bytes, modified_timestamp, category,
            resolution, codec, duration_seconds, bitrate_kbps)
  ↓ serialize to CSV
  inventory.csv (9 columns)

Stage 2: Parse
  Load inventory.csv → List[VideoFile]
  ↓ parse filenames → extract identity
  MovieIdentity(title, year, confidence, needs_review)
  SeriesIdentity(title, season, episodes, confidence, needs_review)
  ↓ serialize to JSON
  identities.json
  {
    "movies": [
      {"filename": "...", "title": "Inception", "year": 2010, ...}
    ],
    "series": [...]
  }

Stage 3: Enrich (in-place update)
  Load identities.json
  ↓ call TMDB API
  Add: title_zh, title_en, reputation_score, canonical_id, ...
  ↓ serialize back to JSON
  identities.json (updated in-place)

Stage 4: Analyze
  Load identities.json
  ↓ reconstruct VideoFile (⚠️ loses resolution/codec)
  Detect gaps and duplicates
  ↓ serialize
  analysis.json
  {
    "completeness": [...],
    "duplicates": [
      {
        "files": [
          {"filename": "...", "size_bytes": 0, "resolution": null}
        ]
      }
    ]
  }

Stage 5: Plan
  Load identities.json + analysis.json
  ↓ generate FileOperation list
  ExecutionPlan(operations, summary, human_summary)
  ↓ serialize
  plan.json

Stage 6: Execute
  Load plan.json
  ↓ execute file operations
  File system changes + rollback.json

2.2 Data Loss Points

Stage Transition	Data Preserved	Data Lost	Impact
Scan → inventory.csv	All VideoFile fields	None	✅ Good
inventory.csv → Parse	VideoFile fully loaded	None	✅ Good
Parse → identities.json	Identity fields	VideoFile metadata (resolution/codec)	⚠️ Minor - still in inventory.csv
identities.json → Analyze	Identity fields	VideoFile metadata	❌ Critical - duplicate quality comparison broken
Analyze → analysis.json	Duplicate groups, gaps	Enrichment fields (title_zh)	⚠️ Minor - not needed for plan
analysis.json → Plan	Operations, conflicts	Original identity context	✅ OK - plan references paths

2.3 Recommendations

Fix 1: Preserve Metadata in Identities

Add video_file object to identities.json:

{
  "movies": [
    {
      "filename": "/library/movie/Inception (2010).mkv",
      "title": "Inception",
      "year": 2010,
      "confidence": 0.95,
      "video_file": {
        "resolution": "1080p",
        "codec": "x265",
        "size_bytes": 2147483648,
        "duration_seconds": 8880
      }
    }
  ]
}

Migration Path:

Update io.save_identities_json() to include video_file field
Update io.load_identities_json() to reconstruct VideoFile from saved data
Add schema version: {"version": "1.1", "movies": [...]} for compatibility check

Fix 2: Use Inventory as Source of Truth

Keep inventory.csv as canonical metadata store, join with identities:

# In analyze stage
inventory = io.load_inventory_csv("inventory.csv")
identities = io.load_identities_json("identities.json")

# Build lookup: path → VideoFile
vf_by_path = {vf.path: vf for vf in inventory}

# Join: identity + full VideoFile
for identity in identities:
    vf = vf_by_path.get(Path(identity["filename"]))
    if vf:
        # Use vf.resolution, vf.codec for quality comparison

Trade-off: Requires inventory.csv to be present during analyze stage (dependency)

3. Module Coupling Analysis

3.1 Coupling Matrix

Module	High Coupling	Medium Coupling	Low Coupling
cli.py	commands/*, config	io, models	utils
scanner.py	config, models	io	utils
parser.py	models	-	utils
enrichment.py	cache, providers, models	config	utils
analysis.py	models, parser	io	-
planner.py	duplicate_resolve, models, config	io	utils
executor.py	quarantine, models, config	io	-
io.py	scanner, models	-	-

3.2 Problematic Coupling

Issue 1: CLI Bypasses I/O Module

Problem: CLI commands duplicate CSV/JSON loading logic instead of using io.py.

Evidence:

# cli.py parse command (lines 185-336)
with open(input_path) as f:
    reader = csv.DictReader(f)
    files = []
    for row in reader:
        # Manual CSV parsing instead of io.load_inventory_csv()

Impact:

Changes to CSV schema require updates in multiple places
Inconsistent error handling between cli.py and io.py
Maintenance burden

Fix:

# cli.py parse command
from vlm.io import load_inventory_csv, save_identities_json

files = load_inventory_csv(input_path)
# ... parse identities ...
save_identities_json(identities, output_path)

Issue 2: Circular Import Risk

Potential Issue: io.py imports scanner.py for type hints, cli.py imports both.

# io.py:4
from vlm.scanner import scan_library  # Direct import

# cli.py:10
from vlm import scanner
from vlm import io

Current Status: No circular import (yet), but fragile.

Recommendation:

Use TYPE_CHECKING for type-only imports:

from typing import TYPE_CHECKING
if TYPE_CHECKING:
    from vlm.scanner import VideoFile

3.3 Missing Abstractions

Missing: Unified File I/O Interface

Current State: Each module handles file I/O differently.

Examples:

scanner.py: Uses csv.writer() directly
enrichment.py: Uses json.dump() directly
cli.py: Uses csv.DictReader() directly
io.py: Provides helpers but not used consistently

Recommendation: Create FileRepository abstraction:

class FileRepository:
    def load_inventory(self, path: Path) -> List[VideoFile]: ...
    def save_inventory(self, files: List[VideoFile], path: Path): ...
    def load_identities(self, path: Path) -> dict: ...
    def save_identities(self, identities: dict, path: Path): ...
    def load_analysis(self, path: Path) -> Analysis: ...
    def save_analysis(self, analysis: Analysis, path: Path): ...
    def load_plan(self, path: Path) -> ExecutionPlan: ...
    def save_plan(self, plan: ExecutionPlan, path: Path): ...

Benefits:

Single source of truth for file formats
Easy to add schema versioning
Testable in isolation (mock file system)
Consistent error handling

4. Error Handling Review

4.1 Current Error Strategy

Positive Patterns

✅ Graceful Degradation in Scanner:

# scanner.py:401-487
try:
    metadata = _extract_metadata_ffprobe(file_path, logger)
except Exception:
    logger.debug(f"ffprobe failed for {file_path}, skipping metadata")
    # Continue with None values

Assessment: Good - missing ffprobe doesn't crash, just degrades functionality.

✅ Explicit Error States:

# models.py FileOperation
has_conflict: bool = False  # Explicit flag for destination conflicts
needs_review: bool = False  # Explicit flag for parsing uncertainty

Assessment: Good - errors are data, not exceptions.

✅ Nested Exception Handling:

# cli.py execute command
try:
    plan = load_plan_json(plan_path)
    try:
        results = executor.execute_plan(plan, mode="execute", confirmed=confirmed)
    except ExecutionError as e:
        logger.error(f"Execution failed: {e}")
except PlanLoadError as e:
    logger.error(f"Plan load failed: {e}")

Assessment: Good - staged error handling with context.

Negative Patterns

❌ Generic Exception Catching:

# enrichment.py:97-120
try:
    payload, api_calls, failures, skip_reason = _enrich_record(...)
except Exception as e:
    logger.error(f"Enrichment failed: {e}")
    stats["failed"] += 1
    # ⚠️ Can't distinguish network errors from data errors

Problem: All errors treated equally, no retry logic for transient failures.

❌ Silent Data Reconstruction Failures:

# io.py:63-82 _movie_identity_from_record()
identity = MovieIdentity(
    title=record["title"],  # ⚠️ KeyError if missing, crashes silently
    year=record["year"],
    ...
)

Problem: No validation that required fields exist, crashes with unhelpful KeyError.

❌ No Exit Code Semantics:

# cli.py main()
except Exception as e:
    logger.error(f"Error: {e}")
    sys.exit(1)  # ⚠️ All errors exit with same code

Problem: Calling scripts can't distinguish error types.

4.2 Recommended Error Hierarchy

class VLMError(Exception):
    """Base exception for all VLM errors."""
    exit_code = 1

class VLMConfigError(VLMError):
    """Configuration errors (missing config, invalid YAML)."""
    exit_code = 2

class VLMDataError(VLMError):
    """Data format errors (malformed JSON, missing fields)."""
    exit_code = 3

class VLMSystemError(VLMError):
    """System errors (permission denied, disk full)."""
    exit_code = 4

class VLMAPIError(VLMError):
    """External API errors (TMDB timeout, auth failure)."""
    exit_code = 5
    is_transient: bool = False  # Retry hint

Usage Example:

try:
    config = load_config()
except FileNotFoundError:
    raise VLMConfigError("Config file not found at ~/.vlm/config.yaml")

try:
    identities = load_identities_json(path)
except json.JSONDecodeError as e:
    raise VLMDataError(f"Invalid JSON in {path}: {e}")

try:
    result = tmdb_provider.enrich(identity)
except requests.Timeout:
    raise VLMAPIError("TMDB API timeout", is_transient=True)

Benefits:

Calling code can handle errors by type
Exit codes inform shell scripts of error category
is_transient flag enables retry logic

5. State Management Review

5.1 Current State Architecture

# state.py
class FileState:
    file_path: str
    status: str  # "reviewed", "ignored", "planned", "executed", "quarantined"
    reason: Optional[str]
    updated_at: str  # ISO 8601 timestamp

class StateManager:
    def __init__(self, state_file: Path):
        self.state_store = StateStore()  # In-memory dict
        self.state_file = state_file
        self.load()  # Load entire file into memory

    def set_file_state(self, file_path: Path, status: str, reason: str):
        # Update in-memory dict
        self.state_store.set_file_state(...)
        self.save()  # Write entire dict to file

5.2 Critical Issues

Issue 1: State Not Integrated with Main Workflow

Problem: State tracking is optional and disconnected from scan/parse/execute stages.

Evidence:

vlm execute --confirm does NOT auto-mark files as "executed"
Users must manually run vlm state set <file> --status executed
No validation that files in execution plan are in correct state

Expected Workflow:

vlm scan       # Should mark files "discovered"
vlm parse      # Should mark files "parsed"
vlm enrich     # Should mark files "enriched"
vlm execute    # Should mark files "executed"

Actual Workflow:

vlm scan       # No state change
vlm parse      # No state change
vlm execute    # No state change
# State only changes via explicit vlm state set commands

Recommendation:

# executor.py execute_plan()
for operation in plan.operations:
    result = self.execute_operation(operation, mode=mode)
    if result.success and mode == "execute":
        # Auto-update state
        state_manager.set_file_state(
            operation.source_path,
            status="executed",
            reason=operation.operation_type,
        )

Issue 2: Non-Atomic State Persistence

Problem: StateManager.save() writes entire file without atomic guarantees.

Current Implementation:

# state.py:166-174
def save(self):
    with open(self.state_file, "w") as f:
        json.dump(self.state_store.to_dict(), f, indent=2)
    # ⚠️ If interrupted mid-write, file is corrupted

Fix with Atomic Write:

import tempfile
import os

def save(self):
    # Write to temp file first
    tmp_fd, tmp_path = tempfile.mkstemp(dir=self.state_file.parent)
    try:
        with os.fdopen(tmp_fd, "w") as f:
            json.dump(self.state_store.to_dict(), f, indent=2)
        # Atomic rename (POSIX guarantee)
        os.replace(tmp_path, self.state_file)
    except:
        os.unlink(tmp_path)
        raise

Issue 3: Path String vs Path Object Mismatch

Problem: State keys are strings, but API uses Path objects, leading to symlink issues.

Example:

# File accessed via symlink
vlm state set /library/symlink/movie.mkv --status reviewed

# File accessed via real path
vlm state show /library/real/movie.mkv
# Returns: No state found (different string keys!)

Fix:

# state.py StateStore.set_file_state()
def set_file_state(self, file_path: Path, status: str, reason: str):
    # Canonicalize path before using as key
    canonical_path = file_path.resolve()
    key = str(canonical_path)
    self.states[key] = FileState(...)

5.3 Recommended State Machine

┌──────────────┐
│  Discovered  │  (after scan)
└──────┬───────┘
       │
       ▼
┌──────────────┐
│    Parsed    │  (after parse, identity extracted)
└──────┬───────┘
       │
       ▼
┌──────────────┐
│   Enriched   │  (after enrich, TMDB data added)
└──────┬───────┘
       │
       ▼
┌──────────────┐
│   Analyzed   │  (after analyze, included in gap/dup detection)
└──────┬───────┘
       │
       ├─────────────────┬─────────────────┐
       ▼                 ▼                 ▼
┌──────────────┐  ┌─────────────┐  ┌──────────────┐
│   Planned    │  │   Ignored   │  │   Reviewed   │
│ (in plan.json)│  │ (user skip) │  │ (user manual)│
└──────┬───────┘  └─────────────┘  └──────────────┘
       │
       ▼
┌──────────────┐
│   Executed   │  (file moved/renamed)
└──────┬───────┘
       │
       ├─────────────────┐
       ▼                 ▼
┌──────────────┐  ┌─────────────┐
│ Quarantined  │  │ Rolled Back │
└──────────────┘  └─────────────┘

Implementation:

class WorkflowState(Enum):
    DISCOVERED = "discovered"
    PARSED = "parsed"
    ENRICHED = "enriched"
    ANALYZED = "analyzed"
    PLANNED = "planned"
    IGNORED = "ignored"
    REVIEWED = "reviewed"
    EXECUTED = "executed"
    QUARANTINED = "quarantined"
    ROLLED_BACK = "rolled_back"

VALID_TRANSITIONS = {
    WorkflowState.DISCOVERED: [WorkflowState.PARSED],
    WorkflowState.PARSED: [WorkflowState.ENRICHED, WorkflowState.IGNORED],
    WorkflowState.ENRICHED: [WorkflowState.ANALYZED, WorkflowState.IGNORED],
    # ...
}

def validate_transition(from_state: WorkflowState, to_state: WorkflowState):
    if to_state not in VALID_TRANSITIONS.get(from_state, []):
        raise VLMStateError(f"Invalid transition: {from_state} → {to_state}")

6. Caching Strategy Review

6.1 Scanner Metadata Cache

Implementation (scanner.py:219-255):

if cached_file is not None and \
   cached_file.size_bytes == size_bytes and \
   int(_normalize_to_utc(cached_file.modified_timestamp).timestamp()) == file_mtime_seconds:
    # Reuse cached metadata (resolution, codec, duration, bitrate)
    return cached_file

Cache Key: (path, size_bytes, modified_timestamp)

Strengths:

✅ Simple and effective for unchanged files
✅ Avoids expensive ffprobe calls
✅ Works across scan invocations

Weaknesses:

❌ No invalidation: If file content changes but size/mtime stay same (rare), stale cache
❌ In-memory only: Limited by RAM for large libraries
❌ Path-based key: Symlinks and relative paths won't cache-hit

Recommendations:

Add content hash to cache key (optional, expensive):

# Only for small files or on explicit --verify flag
if size_bytes < 10MB or verify_cache:
    content_hash = hashlib.md5(file.read_bytes()).hexdigest()
    cache_key = (path, size_bytes, mtime, content_hash)

Persist cache to disk (SQLite):

# ~/.vlm/metadata_cache.db
CREATE TABLE metadata_cache (
    path TEXT PRIMARY KEY,
    size_bytes INTEGER,
    mtime_seconds INTEGER,
    resolution TEXT,
    codec TEXT,
    duration_seconds INTEGER,
    bitrate_kbps INTEGER
)

6.2 Enrichment Cache

Implementation (cache.py):

CREATE TABLE identity_enrichment (
    identity_key TEXT PRIMARY KEY,
    fingerprint TEXT NOT NULL,
    payload_json TEXT NOT NULL,
    cached_at TEXT NOT NULL
)

Cache Key: identity_key (e.g., "movie:Inception:2010") Invalidation: fingerprint (hash of title/year/season/episodes)

Strengths:

✅ Persistent SQLite storage
✅ Fingerprint-based invalidation detects identity changes
✅ Survives across runs

Weaknesses:

Issue 1: Fingerprint Collision Risk

Problem: Hash collision not handled.

Example:

# enrichment.py:216-227 _fingerprint()
def _fingerprint(record: dict, media_type: str) -> str:
    payload = json.dumps({
        "media_type": media_type,
        "title": record.get("title"),
        "year": record.get("year"),
        # ...
    }, sort_keys=True)
    return hashlib.sha256(payload.encode()).hexdigest()

Collision Scenario (theoretical):

Movie "Title A (2020)" hashes to abc123...
Movie "Title B (2020)" also hashes to abc123... (collision)
Both share same cache entry (wrong data returned)

Likelihood: Extremely low (SHA-256 has 2^256 space), but not zero.

Fix: Use composite key instead of hash:

CREATE TABLE identity_enrichment (
    media_type TEXT NOT NULL,
    title TEXT NOT NULL,
    year INTEGER,
    season INTEGER,
    fingerprint TEXT NOT NULL,
    payload_json TEXT NOT NULL,
    PRIMARY KEY (media_type, title, year, season)
)

Issue 2: No Cache TTL

Problem: Cached enrichment never expires, even if TMDB data updates.

Example:

Cache stores reputation_score=7.5 from TMDB on 2025-01-01
TMDB rating updates to 8.2 on 2025-06-01
VLM continues using stale 7.5 score indefinitely

Recommendation:

# Add cache expiry
CACHE_TTL_DAYS = 90  # Configurable

CREATE TABLE identity_enrichment (
    ...
    cached_at TEXT NOT NULL,
    expires_at TEXT NOT NULL  -- cached_at + TTL
)

# Check expiry on load
def get_identity(self, identity_key, fingerprint):
    row = self.conn.execute(
        "SELECT payload_json, expires_at FROM identity_enrichment WHERE ...",
        (identity_key, fingerprint)
    ).fetchone()
    if row:
        if datetime.fromisoformat(row[1]) > datetime.now(timezone.utc):
            return json.loads(row[0])  # Cache hit
    return None  # Expired or not found

Issue 3: Payload Validation Missing

Problem: No schema validation when loading cached payload.

Risk:

# cache.py:62
payload = json.loads(row[0])
# ⚠️ If payload_json is corrupted or missing required fields,
# downstream code crashes with KeyError

Fix: Validate payload structure:

from vlm.models import EnrichedIdentity  # Hypothetical

def get_identity(self, identity_key, fingerprint):
    row = self.conn.execute(...).fetchone()
    if row:
        payload = json.loads(row[0])
        try:
            # Validate required fields
            EnrichedIdentity.from_dict(payload)
            return payload
        except (KeyError, ValueError) as e:
            logger.warning(f"Invalid cached payload for {identity_key}: {e}")
            # Invalidate corrupted cache entry
            self.invalidate(identity_key)
            return None

7. Safety Mechanisms Review

7.1 Dry-Run Implementation

Design (executor.py:45-85):

def execute_plan(self, plan, mode="dry-run", confirmed=False):
    if mode == "execute" and not confirmed:
        raise ValueError("Execute mode requires explicit confirmation")

    for op in plan.operations:
        result = self.execute_operation(op, mode=mode)
        # Same code path for dry-run and execute
        # Mode flag controls whether file ops actually happen

Strength:

✅ Dry-run uses same logic as execute (what you see is what you get)
✅ Explicit confirmation required via --confirm flag

Weakness:

❌ No separation of dry-run vs execute logic
- Both modes call same execute_operation() method
- If bug exists in execute branch, dry-run can't detect it
- Dry-run validation is weak (only checks file existence)

Recommendation: Separate validation logic

def validate_operation(self, op: FileOperation) -> ValidationResult:
    """Dry-run validation (no file system changes)."""
    errors = []

    # Check source exists
    if not op.source_path.exists():
        errors.append(f"Source does not exist: {op.source_path}")

    # Check destination not occupied
    if op.destination_path and op.destination_path.exists():
        errors.append(f"Destination conflict: {op.destination_path}")

    # Check parent directory writable
    if not os.access(op.destination_path.parent, os.W_OK):
        errors.append(f"Destination not writable: {op.destination_path.parent}")

    return ValidationResult(valid=len(errors) == 0, errors=errors)

def execute_plan(self, plan, mode="dry-run"):
    if mode == "dry-run":
        return [self.validate_operation(op) for op in plan.operations]
    else:
        return [self.execute_operation(op) for op in plan.operations]

7.2 Quarantine Safety

Implementation (quarantine.py:45-90):

def quarantine_file(self, file_path: Path, reason: str) -> OperationResult:
    # 1. Determine quarantine path
    quarantine_path = self._get_quarantine_path(file_path)

    # 2. Create quarantine directory
    quarantine_path.parent.mkdir(parents=True, exist_ok=True)

    # 3. Move file
    shutil.move(str(file_path), str(quarantine_path))

    # 4. Update manifest
    self.manifest.add_entry(QuarantineEntry(...))
    self._save_manifest()

Critical Issue: Non-Atomic Operation

Failure Scenario:

File moved to quarantine (step 3 succeeds)
Manifest update fails (step 4 fails: disk full, permission denied)
Result: File is quarantined but not tracked in manifest
User runs vlm quarantine list → file not shown
File is effectively lost (orphaned in quarantine directory)

Fix: Atomic transaction pattern

def quarantine_file(self, file_path: Path, reason: str) -> OperationResult:
    quarantine_path = self._get_quarantine_path(file_path)
    entry = QuarantineEntry(
        original_path=str(file_path),
        quarantine_path=str(quarantine_path),
        reason=reason,
        timestamp=utcnow(),
    )

    # Step 1: Add to manifest FIRST (before moving file)
    self.manifest.add_entry(entry)
    self._save_manifest()

    try:
        # Step 2: Move file
        quarantine_path.parent.mkdir(parents=True, exist_ok=True)
        shutil.move(str(file_path), str(quarantine_path))
    except Exception as e:
        # Rollback: Remove from manifest if file move failed
        self.manifest.remove_entry(entry)
        self._save_manifest()
        raise QuarantineError(f"File move failed: {e}")

    return OperationResult(success=True, ...)

Alternative: Use write-ahead log (WAL)

# Write intent to WAL before action
wal.append({"action": "quarantine", "file": file_path, "dest": quarantine_path})
# Perform action
shutil.move(...)
# Commit WAL entry
wal.commit()

7.3 Rollback Implementation

Design (executor.py:212-258):

def rollback(self, rollback_log_path: Path) -> List[OperationResult]:
    log = self._load_rollback_log(rollback_log_path)
    results = []

    # LIFO: Reverse order of operations
    for op in reversed(log.operations):
        result = self._rollback_operation(op)
        results.append(result)

    return results

Strengths:

✅ LIFO order (undo in reverse)
✅ Best-effort semantics (continues on failure)
✅ Comprehensive logging

Weaknesses:

Issue 1: No Validation Before Rollback

Problem: No check that files are still at destination before rolling back.

Failure Scenario:

vlm execute --confirm  # Moves A.mkv → B.mkv
# User manually moves B.mkv → C.mkv
vlm rollback           # Tries to move B.mkv → A.mkv
# FAILS: B.mkv no longer exists

Fix: Validate file state before rollback

def _rollback_operation(self, op: FileOperation) -> OperationResult:
    # Validate current state matches expected state
    if op.operation_type == "move":
        if not op.destination_path.exists():
            return OperationResult(
                success=False,
                error=f"Cannot rollback: file not at destination {op.destination_path}"
            )
        if op.source_path.exists():
            return OperationResult(
                success=False,
                error=f"Cannot rollback: source path occupied {op.source_path}"
            )

    # Perform rollback
    shutil.move(str(op.destination_path), str(op.source_path))

Issue 2: No Idempotency

Problem: Can't safely re-run rollback if it failed partway.

Scenario:

vlm rollback  # Fails after rolling back 50 of 100 operations
vlm rollback  # Re-run: tries to rollback already-rolled-back files
# CHAOS: Files in inconsistent state

Fix: Track rollback progress

class RollbackLog:
    operations: List[FileOperation]
    rollback_status: Dict[int, bool] = {}  # index → rolled_back

def rollback(self, rollback_log_path: Path):
    log = self._load_rollback_log(rollback_log_path)

    for i, op in enumerate(reversed(log.operations)):
        # Skip already rolled back
        if log.rollback_status.get(i, False):
            continue

        result = self._rollback_operation(op)
        if result.success:
            log.rollback_status[i] = True
            self._save_rollback_log(log, rollback_log_path)

8. Scalability & Performance

8.1 Known Bottlenecks

Bottleneck 1: Serial ffprobe Execution

Location: scanner.py:401-487 _extract_metadata_ffprobe()

Issue:

# Called once per video file
def _create_video_file(...):
    if use_metadata:
        metadata = _extract_metadata_ffprobe(file_path, logger)
    # ⚠️ Blocking subprocess.run() call per file

Impact:

10,000 files × 0.5s per ffprobe = 5,000 seconds = 83 minutes
Single-threaded, CPU idle during I/O wait

Recommendation: Parallel ffprobe with process pool

from concurrent.futures import ProcessPoolExecutor

def scan_library_parallel(library_root, config, max_workers=8):
    files = _discover_files(library_root)

    with ProcessPoolExecutor(max_workers=max_workers) as executor:
        # Submit all ffprobe jobs
        futures = {
            executor.submit(_extract_metadata_ffprobe, f, logger): f
            for f in files
        }

        # Collect results as they complete
        for future in as_completed(futures):
            file = futures[future]
            try:
                metadata = future.result(timeout=10)
                yield VideoFile(metadata=metadata, ...)
            except TimeoutError:
                logger.warning(f"ffprobe timeout for {file}")
                yield VideoFile(metadata=None, ...)

Expected Speedup: 8x on 8-core CPU (83 min → 10 min)

Bottleneck 2: Serial API Calls in Enrichment

Location: enrichment.py:67-120 enrich_identities_data()

Issue:

for record in records:
    # Blocking HTTP request per record
    payload = _enrich_record(record, media_type, providers, logger)
    # ⚠️ No concurrency despite max_concurrency=6 config

Impact:

5,000 uncached records × 1s per API call = 5,000 seconds = 83 minutes
Config option enrichment.max_concurrency exists but is NOT IMPLEMENTED

Recommendation: Async HTTP with concurrency limit

import asyncio
import aiohttp

async def enrich_identities_async(identities, config):
    semaphore = asyncio.Semaphore(config.enrichment_max_concurrency)

    async def enrich_one(record):
        async with semaphore:
            async with aiohttp.ClientSession() as session:
                return await _enrich_record_async(record, session, config)

    tasks = [enrich_one(r) for r in records]
    results = await asyncio.gather(*tasks, return_exceptions=True)
    return results

Expected Speedup: 6x with max_concurrency=6 (83 min → 14 min)

Alternative: Use threading for I/O-bound tasks

from concurrent.futures import ThreadPoolExecutor

def enrich_identities_threaded(identities, config):
    with ThreadPoolExecutor(max_workers=config.enrichment_max_concurrency) as executor:
        futures = [
            executor.submit(_enrich_record, record, providers, logger)
            for record in records
        ]
        for future in as_completed(futures):
            result = future.result()
            # Process result

8.2 Memory Concerns

Issue 1: In-Memory VideoFile List

Location: scanner.py:36 scan_library() return type

Problem:

def scan_library(...) -> List[VideoFile]:
    files = []
    for entry in _discover_files(...):
        vf = _create_video_file(entry, ...)
        files.append(vf)  # ⚠️ Accumulates in memory
    return files  # ⚠️ Entire list returned at once

Impact:

100,000 files × 500 bytes per VideoFile = 50 MB
Plus metadata strings (resolution, codec): +50 MB
Peak memory: ~100 MB for large libraries

Severity: Low for most users (modern systems have GB of RAM), but problematic for:

Very large libraries (1M+ files)
Constrained environments (embedded systems, containers with memory limits)

Recommendation: Generator pattern for streaming

def scan_library(...) -> Iterator[VideoFile]:
    for entry in _discover_files(...):
        vf = _create_video_file(entry, ...)
        yield vf  # ⚠️ Stream one at a time

# Usage
for vf in scan_library(...):
    writer.writerow(vf.to_csv_row())  # Process and discard

Trade-off: Generator requires changes to all consumers of scan_library()

Issue 2: Identities JSON in Memory

Location: enrichment.py:22 enrich_identities_data(identities_data: dict)

Problem:

# Entire identities.json loaded into memory
with open("identities.json") as f:
    identities = json.load(f)  # ⚠️ Full parse to dict

enrich_identities_data(identities, ...)  # ⚠️ Mutates dict in place

Impact:

50,000 identities × 1 KB each = 50 MB
After enrichment (adds title_zh, reputation, etc.): +50 MB
Peak: ~100 MB during enrich

Recommendation: Stream JSON processing

import ijson  # Iterative JSON parser

def enrich_identities_streaming(input_path, output_path, config):
    with open(input_path, "rb") as fin, open(output_path, "w") as fout:
        fout.write('{"movies": [')
        first = True

        # Stream parse movies array
        for record in ijson.items(fin, "movies.item"):
            enriched = _enrich_record(record, providers, config)
            if not first:
                fout.write(',')
            json.dump(enriched, fout)
            first = False

        fout.write('], "series": [...]}')

Trade-off: More complex code, harder to add summary stats

9. Missing Features & Architectural Gaps

9.1 Schema Versioning

Problem: CSV and JSON files have no version field.

Impact:

Can't detect format changes when evolving codebase
If field added/removed, old files crash on load
No migration path for legacy data

Example Breaking Change:

# v1.0: identities.json
{"movies": [{"title": "Inception", "year": 2010}]}

# v1.1: Add confidence field (BREAKING)
{"movies": [{"title": "Inception", "year": 2010, "confidence": 0.95}]}

# Loading v1.0 file with v1.1 code
identity = record["confidence"]  # KeyError!

Recommendation: Add version to all data files

{
  "version": "1.1",
  "movies": [...]
}

Migration Handler:

def load_identities_json(path: Path) -> dict:
    with open(path) as f:
        data = json.load(f)

    version = data.get("version", "1.0")  # Default to 1.0 if missing

    # Migrate old formats
    if version == "1.0":
        data = _migrate_1_0_to_1_1(data)
    elif version == "1.1":
        pass  # Current version
    else:
        raise VLMDataError(f"Unsupported identities version: {version}")

    return data

def _migrate_1_0_to_1_1(data: dict) -> dict:
    # Add default confidence to old records
    for movie in data.get("movies", []):
        if "confidence" not in movie:
            movie["confidence"] = 0.5  # Unknown confidence
    data["version"] = "1.1"
    return data

9.2 Path Canonicalization

Problem: Paths not canonicalized, symlinks cause duplicates.

Example:

/library/movie/real/Inception.mkv          # Real file
/library/movie/symlink/Inception.mkv -> ../real/Inception.mkv  # Symlink

vlm scan
# Both appear as separate files in inventory.csv!

Impact:

Duplicate detection fails (same file counted twice)
State tracking inconsistent (same file has two state entries)
Execution plan may target same file via different paths

Recommendation: Canonicalize all paths

from pathlib import Path

def canonicalize_path(path: Path) -> Path:
    """Resolve symlinks and relative components."""
    return path.resolve()

# Usage in scanner
def _create_video_file(entry, ...):
    file_path = canonicalize_path(Path(entry.path))
    # Now symlinks resolve to same canonical path

Trade-off: Performance cost (resolve() requires filesystem access)

9.3 Unified I/O Layer

Problem: CSV/JSON loading duplicated across modules.

Evidence:

cli.py parse: Manual CSV parsing (lines 200-214)
commands/analyze.py: Manual JSON loading
io.py: Provides helpers but not consistently used

Impact:

Schema changes require updates in multiple places
Inconsistent error handling
Hard to add features (e.g., compression, encryption)

Recommendation: FileRepository abstraction (see Section 3.3)

9.4 Configuration Validation

Problem: Config validated only in cli.py main(), not in Config class.

Impact:

Commands can load invalid config and fail late
Hard to test config validation in isolation

Current:

# cli.py:150-180
config = Config.load_from_yaml(config_path)
# ⚠️ No validation at load time

# Later in command
if not config.library_root:
    raise VLMConfigError("library_root not set")

Recommendation: Validate at construction

class Config:
    @classmethod
    def load_from_yaml(cls, path: Path) -> "Config":
        with open(path) as f:
            data = yaml.safe_load(f)

        config = cls(data)
        config.validate()  # ⚠️ Validate immediately
        return config

    def validate(self):
        errors = []

        if not self.library_root:
            errors.append("library_root is required")
        elif not Path(self.library_root).exists():
            errors.append(f"library_root does not exist: {self.library_root}")

        if self.video_extensions and not isinstance(self.video_extensions, list):
            errors.append("video_extensions must be a list")

        if errors:
            raise VLMConfigError("\n".join(errors))

10. Recommendations Summary

Priority 1: Critical Fixes (Do Immediately)

Issue	Impact	Effort	Fix
Identity reconstruction loses metadata	❌ Duplicate resolution by quality broken	Medium	Store full VideoFile in identities.json or join with inventory.csv
Quarantine non-atomic	❌ File loss risk if manifest update fails	Low	Update manifest before moving file
Path canonicalization missing	❌ Symlinks cause duplicates	Low	Use `Path.resolve()` everywhere
No schema versioning	❌ Breaking changes crash on old files	Low	Add "version" field to JSON/CSV

Priority 2: Important Improvements (Do Soon)

Issue	Impact	Effort	Fix
Duplicated I/O logic	⚠️ Maintenance burden, inconsistency	Medium	Consolidate to io.py, enforce usage
No exception hierarchy	⚠️ Poor error handling, no retry logic	Low	Create VLMError base class
State not integrated	⚠️ Manual state tracking, no automation	Medium	Auto-update state in execute stage
Config validation late	⚠️ Commands fail after user invocation	Low	Validate in Config.init

Priority 3: Performance Optimizations (Nice to Have)

Issue	Impact	Effort	Fix
Serial ffprobe calls	⚠️ Slow scan (83 min for 10K files)	High	Parallel execution with ProcessPoolExecutor
Serial API calls	⚠️ Slow enrichment (83 min for 5K records)	High	Async HTTP with aiohttp or ThreadPoolExecutor
In-memory file lists	⚠️ Memory usage (100MB for 100K files)	Medium	Generator pattern for streaming

Priority 4: Architectural Enhancements (Future)

Enhancement	Benefit	Effort	Description
Transaction semantics	Resume failed stages without re-processing	High	Checkpoint files for each stage
Explicit state machine	Track workflow progression explicitly	Medium	WorkflowState enum with valid transitions
Repository pattern	Testable data access, easy to mock	Medium	FileRepository abstraction layer
Cache TTL	Fresh TMDB data without manual refresh	Low	Add expires_at to enrichment cache

11. Conclusion

Overall Design Quality: ⭐⭐⭐⭐ (4/5)

VLM demonstrates strong architectural fundamentals:

Clear separation of concerns
Safety-first workflow with human checkpoints
Graceful degradation and comprehensive error logging
Well-structured modules with mostly reasonable coupling

Critical Gaps Requiring Attention:

Data Loss in Workflow: Metadata lost during identity reconstruction breaks duplicate resolution
Non-Atomic Operations: File operations can fail partway, leaving inconsistent state
Missing Abstractions: No unified I/O layer, no exception hierarchy, no schema versioning
Performance Bottlenecks: Serial execution of expensive operations (ffprobe, API calls)

Recommended Next Steps:

Week 1: Fix critical data loss bug

Preserve VideoFile metadata in identities.json
Add integration test for duplicate resolution by quality

Week 2: Add safety and consistency

Implement atomic quarantine operations
Add path canonicalization
Add schema versioning to all data files

Week 3: Consolidate I/O layer

Move all CSV/JSON operations to io.py
Enforce usage via code review
Add validation for all loaded data

Month 2: Performance optimization

Parallelize ffprobe calls
Implement async API calls for enrichment
Benchmark and measure improvements

Final Assessment

VLM is a well-designed, production-ready tool with a few critical bugs and several opportunities for improvement. The codebase is maintainable, testable, and follows Python best practices. With the recommended fixes, particularly addressing the metadata loss issue, VLM will be a robust and scalable solution for video library management.

Confidence in Current Design: High (85%) Confidence After Priority 1+2 Fixes: Very High (95%)

End of Review

47 KiB Raw Blame History Unescape Escape

Video Library Manager - Architecture Review

Executive Summary

Key Strengths

Critical Issues Identified

Priority Recommendations

1. Workflow Design Analysis

1.1 Current Architecture

1.2 Strengths

✅ Clear Stage Separation

✅ Read-First Principle

✅ Human-in-the-Loop Checkpoints

1.3 Weaknesses

❌ No Transaction Semantics

❌ Implicit State Progression

❌ Lossy Data Reconstruction

2. Data Flow Analysis

2.1 Current Data Paths

2.2 Data Loss Points

2.3 Recommendations

Fix 1: Preserve Metadata in Identities

Fix 2: Use Inventory as Source of Truth

3. Module Coupling Analysis

3.1 Coupling Matrix

3.2 Problematic Coupling

Issue 1: CLI Bypasses I/O Module

Issue 2: Circular Import Risk

3.3 Missing Abstractions

Missing: Unified File I/O Interface

4. Error Handling Review

4.1 Current Error Strategy

Positive Patterns

Negative Patterns

4.2 Recommended Error Hierarchy

5. State Management Review

5.1 Current State Architecture

5.2 Critical Issues

Issue 1: State Not Integrated with Main Workflow

Issue 2: Non-Atomic State Persistence

Issue 3: Path String vs Path Object Mismatch

5.3 Recommended State Machine

6. Caching Strategy Review

6.1 Scanner Metadata Cache

6.2 Enrichment Cache

Issue 1: Fingerprint Collision Risk

Issue 2: No Cache TTL

Issue 3: Payload Validation Missing

7. Safety Mechanisms Review

7.1 Dry-Run Implementation

7.2 Quarantine Safety

7.3 Rollback Implementation

Issue 1: No Validation Before Rollback

Issue 2: No Idempotency

8. Scalability & Performance

8.1 Known Bottlenecks

Bottleneck 1: Serial ffprobe Execution

Bottleneck 2: Serial API Calls in Enrichment

8.2 Memory Concerns

Issue 1: In-Memory VideoFile List

Issue 2: Identities JSON in Memory

9. Missing Features & Architectural Gaps

9.1 Schema Versioning

9.2 Path Canonicalization

9.3 Unified I/O Layer

9.4 Configuration Validation

10. Recommendations Summary

Priority 1: Critical Fixes (Do Immediately)

Priority 2: Important Improvements (Do Soon)

Priority 3: Performance Optimizations (Nice to Have)

Priority 4: Architectural Enhancements (Future)

11. Conclusion

Overall Design Quality: ⭐⭐⭐⭐ (4/5)

Critical Gaps Requiring Attention:

Recommended Next Steps:

Final Assessment

47 KiB

Raw Blame History