diff --git a/FIX_PLAN.md b/FIX_PLAN.md new file mode 100644 index 0000000..7910e96 --- /dev/null +++ b/FIX_PLAN.md @@ -0,0 +1,118 @@ +# Fix Plan (Verified Issues Only) + +## Objective +- Repair only verified defects from `REVIEW_REPORT.md`. +- Keep behavior stable outside defect scope. +- Ensure every fix is testable and reproducible. + +## Definition of Done +- `uv run --with pytest --with hypothesis pytest -q` passes. +- No test imports use `src.vlm...`. +- Timestamp handling outputs correct UTC instants. +- Logging remains usable when file log path is unwritable. +- Default home-based paths are resolved at runtime, not import time. + +## Phase 0 - Baseline and guardrails +- Run baseline suite and keep output: + - `uv run --with pytest --with hypothesis pytest -q` +- Record current known failures for before/after comparison. +- Guardrails: + - No unrelated refactor. + - No behavior change outside listed findings. + +## Phase 1 - Fix broken test import paths (P1) +- Files: + - `tests/test_executor.py` + - `tests/test_quarantine.py` +- Changes: + - Replace `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: + - No `ModuleNotFoundError: No module named 'src'`. + +## Phase 2 - Fix timezone relabeling bug (P2) +- Files: + - `src/vlm/scanner.py` + - `src/vlm/reports.py` +- Changes: + - Use aware UTC timestamp creation for file mtimes. + - Remove naive `replace(tzinfo=timezone.utc)` relabeling in export paths. + - Enforce explicit conversion to UTC instant. +- Tests: + - Add/extend tests for naive timestamps under non-UTC local timezone assumptions. + - Assert true instant conversion (not label swap). +- 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: + - Timestamp tests pass and no relabel bug remains. + +## Phase 3 - Remove dead `error_count` logic (P3) +- File: + - `src/vlm/scanner.py` +- Preferred change: + - Remove unused `error_count` and dead summary branch. +- Alternative (if product requires count): + - Increment and propagate count from scan exception paths. +- Verification: + - `uv run --with pytest --with hypothesis pytest -q tests/test_scanner.py` +- Acceptance: + - No dead state/branch for error counting. + +## Phase 4 - Harden logging setup fallback (P3 risk) +- File: + - `src/vlm/logging_config.py` +- Changes: + - Wrap log-dir creation and file handler setup with `try/except (OSError, PermissionError)`. + - Keep console logging active when file logging cannot initialize. + - Emit one clear warning about fallback. +- Tests: + - Add/extend tests to simulate mkdir/file-handler failure. + - Verify process continues (warning-only behavior). +- 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: + - No command abort when file log path is unwritable. + +## Phase 5 - Resolve import-time home defaults (P3 new) +- Files: + - `src/vlm/logging_config.py` + - `src/vlm/cli.py` +- Changes: + - Replace import-time `Path.home()` constants with runtime helper functions. + - Resolve defaults at function/option execution time. +- Tests: + - Add/extend monkeypatch tests to prove runtime resolution. +- 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: + - Defaults follow runtime environment changes in tests and execution. + +## Documentation Updates (with code changes) +- `README.md` + - Add troubleshooting note for logging fallback to console-only. + - Clarify UTC timestamp expectation in outputs/reports. +- `REVIEW_REPORT.md` + - Update each issue status (`open` -> `fixed`) with evidence. + +## Execution Order +1. Phase 1 (restore full test collection first). +2. Phase 2 (timezone correctness). +3. Phase 3 (dead logic cleanup). +4. Phase 4 (logging resilience). +5. Phase 5 (runtime defaults). +6. Documentation + final full-suite validation. + +## Final Validation Checklist +- Run: `uv run --with pytest --with hypothesis pytest -q` +- Spot-check CLI: + - `vlm --help` +- Confirm no regression in updated modules: + - `src/vlm/scanner.py` + - `src/vlm/reports.py` + - `src/vlm/logging_config.py` + - `src/vlm/cli.py` diff --git a/README.md b/README.md index 5d5608c..f61a8ba 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,10 @@ uv pip install -e ".[dev]" vlm --help ``` +Reports and inventory exports store `modified_timestamp` values in UTC (`YYYY-MM-DDTHH:MM:SS`). + +If file logging cannot be initialized (for example, unwritable log directory), VLM falls back to console logging and continues running. + ## Development Run tests: diff --git a/REVIEW_REPORT.md b/REVIEW_REPORT.md index de0163b..415e4d5 100644 --- a/REVIEW_REPORT.md +++ b/REVIEW_REPORT.md @@ -1,49 +1,234 @@ -# Code Review Report +# Code Review Report (Verified) ## Scope - Reviewed Python sources under `src/vlm/`, tests under `tests/`, and docs (`README.md`, `AGENTS.md`). -- Executed test runs on February 9, 2026: - - `pytest -q` - - `pytest -q --ignore=tests/test_executor.py --ignore=tests/test_quarantine.py` +- 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 -- Found 4 actionable issues: 2 high-priority functional problems, 1 medium-priority data correctness issue, and 1 low-priority observability issue. -- Markdown docs are generally clear; no blocking doc defects were found. +- 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 - CLI startup fails when log path is not writable -- Files: `src/vlm/logging_config.py:63`, `src/vlm/logging_config.py:87` -- `setup_logging()` unconditionally creates the log directory and rotating file handler. -- In restricted environments, this raises `PermissionError` and aborts CLI initialization (including read-only commands like `--help`). -- Impact: broad command/test failure in CI/sandbox/service-user contexts. - -### P1 - Test imports use wrong module path +### 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 package imports `vlm...`, causing collection failure (`ModuleNotFoundError: No module named 'src'`). +- 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 - Timestamp conversion is incorrect for naive datetimes -- Files: `src/vlm/scanner.py:148`, `src/vlm/scanner.py:379`, `src/vlm/scanner.py:437` -- Naive local timestamps are later relabeled as UTC via `replace(tzinfo=timezone.utc)` instead of converted. -- Impact: exported timestamps can be shifted by local timezone offset. +### 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 - Scan error counter is dead code -- Files: `src/vlm/scanner.py:53`, `src/vlm/scanner.py:64` -- `error_count` is initialized/reported but never incremented. -- Impact: scan summary underreports error conditions. +### 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 -- `pytest -q` failed at collection due to `src.vlm` imports in two test files. -- `pytest -q --ignore=tests/test_executor.py --ignore=tests/test_quarantine.py` reported 34 failures, dominated by logging startup failure: - - `PermissionError: [Errno 1] Operation not permitted: '/Users/windy/.vlm/logs/vlm.log'` +- `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. Make logging setup fault-tolerant (fallback to console-only logging). -2. Correct test imports to `vlm...`. -3. Fix timezone handling for inventory timestamps. -4. Wire scan exception paths to increment `error_count`. +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) +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. diff --git a/src/vlm/cli.py b/src/vlm/cli.py index 0331115..58a8b2b 100644 --- a/src/vlm/cli.py +++ b/src/vlm/cli.py @@ -15,8 +15,9 @@ from vlm.config import Config, load_config, create_default_config, validate_conf from vlm.logging_config import setup_logging, get_logger -# Default configuration path -DEFAULT_CONFIG_PATH = Path.home() / ".vlm" / "config.yaml" +def default_config_path() -> Path: + """Return the default config path resolved at runtime.""" + return Path.home() / ".vlm" / "config.yaml" class CLIContext: @@ -34,7 +35,7 @@ pass_context = click.make_pass_decorator(CLIContext) @click.option( '--config', type=click.Path(path_type=Path), - default=DEFAULT_CONFIG_PATH, + default=default_config_path, help='Path to configuration file (default: ~/.vlm/config.yaml)' ) @click.option( @@ -1962,7 +1963,7 @@ def config_cmd(ctx: CLIContext): @click.option( '--path', type=click.Path(path_type=Path), - default=DEFAULT_CONFIG_PATH, + default=default_config_path, help='Path where configuration file should be created' ) def config_init(path: Path): diff --git a/src/vlm/logging_config.py b/src/vlm/logging_config.py index 7145c62..1f55fb8 100644 --- a/src/vlm/logging_config.py +++ b/src/vlm/logging_config.py @@ -13,13 +13,16 @@ from pathlib import Path from typing import Optional -# Default log directory -DEFAULT_LOG_DIR = Path.home() / ".vlm" / "logs" DEFAULT_LOG_FILE = "vlm.log" MAX_LOG_SIZE = 10 * 1024 * 1024 # 10MB in bytes BACKUP_COUNT = 5 # Keep 5 backup log files +def default_log_dir() -> Path: + """Return the default log directory resolved at runtime.""" + return Path.home() / ".vlm" / "logs" + + class OperationContextFilter(logging.Filter): """Filter to add operation context to log records.""" @@ -57,10 +60,7 @@ def setup_logging( # Use default log directory if not specified if log_dir is None: - log_dir = DEFAULT_LOG_DIR - - # Create log directory if it doesn't exist - log_dir.mkdir(parents=True, exist_ok=True) + log_dir = default_log_dir() # Get root logger logger = logging.getLogger("vlm") @@ -83,17 +83,23 @@ def setup_logging( logger.addHandler(console_handler) # File handler with rotation (DEBUG+) - log_file_path = log_dir / log_file - file_handler = logging.handlers.RotatingFileHandler( - filename=log_file_path, - maxBytes=MAX_LOG_SIZE, - backupCount=BACKUP_COUNT, - encoding='utf-8' - ) - file_handler.setLevel(logging.DEBUG) - file_handler.setFormatter(formatter) - file_handler.addFilter(OperationContextFilter()) - logger.addHandler(file_handler) + try: + log_dir.mkdir(parents=True, exist_ok=True) + log_file_path = log_dir / log_file + file_handler = logging.handlers.RotatingFileHandler( + filename=log_file_path, + maxBytes=MAX_LOG_SIZE, + backupCount=BACKUP_COUNT, + encoding='utf-8' + ) + file_handler.setLevel(logging.DEBUG) + file_handler.setFormatter(formatter) + file_handler.addFilter(OperationContextFilter()) + logger.addHandler(file_handler) + except (OSError, PermissionError) as exc: + logger.warning( + f"File logging disabled (cannot write to {log_dir}): {exc}" + ) # Prevent propagation to root logger logger.propagate = False diff --git a/src/vlm/reports.py b/src/vlm/reports.py index a263e5a..82181ab 100644 --- a/src/vlm/reports.py +++ b/src/vlm/reports.py @@ -19,6 +19,13 @@ from vlm.models import SeasonCompleteness, DuplicateGroup, VideoFile, MovieIdent logger = logging.getLogger(__name__) +def _normalize_to_utc(timestamp: datetime) -> datetime: + """Normalize a datetime to a UTC instant.""" + if timestamp.tzinfo is None: + timestamp = timestamp.astimezone() + return timestamp.astimezone(timezone.utc) + + def generate_inventory_report( files: list[VideoFile], format: str, @@ -87,11 +94,7 @@ def _generate_inventory_csv( # Write each file for video_file in files: # Format timestamp as ISO 8601 in UTC - if video_file.modified_timestamp.tzinfo is None: - # Assume local time, convert to UTC - modified_utc = video_file.modified_timestamp.replace(tzinfo=timezone.utc) - else: - modified_utc = video_file.modified_timestamp.astimezone(timezone.utc) + modified_utc = _normalize_to_utc(video_file.modified_timestamp) row = { 'path': str(video_file.path), @@ -127,11 +130,7 @@ def _generate_inventory_json( # Add each file for video_file in files: # Format timestamp as ISO 8601 in UTC - if video_file.modified_timestamp.tzinfo is None: - # Assume local time, convert to UTC - modified_utc = video_file.modified_timestamp.replace(tzinfo=timezone.utc) - else: - modified_utc = video_file.modified_timestamp.astimezone(timezone.utc) + modified_utc = _normalize_to_utc(video_file.modified_timestamp) file_data = { 'path': str(video_file.path), diff --git a/src/vlm/scanner.py b/src/vlm/scanner.py index b77dd9e..e4c85c4 100644 --- a/src/vlm/scanner.py +++ b/src/vlm/scanner.py @@ -20,6 +20,13 @@ from vlm.models import VideoFile logger = logging.getLogger(__name__) +def _normalize_to_utc(timestamp: datetime) -> datetime: + """Normalize a datetime to a UTC instant.""" + if timestamp.tzinfo is None: + timestamp = timestamp.astimezone() + return timestamp.astimezone(timezone.utc) + + def scan_library(root: Path, config: Config) -> list[VideoFile]: """Recursively scan library for video files. @@ -50,8 +57,6 @@ def scan_library(root: Path, config: Config) -> list[VideoFile]: video_files = [] file_count = 0 - error_count = 0 - # Recursively scan directory tree for video_file in _scan_directory_recursive(root, config, root): video_files.append(video_file) @@ -61,9 +66,6 @@ def scan_library(root: Path, config: Config) -> list[VideoFile]: logger.debug(f"Scanned {file_count} files so far...") logger.info(f"Scan complete. Found {file_count} video files") - if error_count > 0: - logger.warning(f"Encountered {error_count} errors during scan (see log for details)") - return video_files @@ -145,7 +147,7 @@ def _create_video_file(file_path: Path, library_root: Path) -> Optional[VideoFil # Get file stats stat = file_path.stat() size_bytes = stat.st_size - modified_timestamp = datetime.fromtimestamp(stat.st_mtime) + modified_timestamp = datetime.fromtimestamp(stat.st_mtime, tz=timezone.utc) # Categorize based on directory structure category = categorize_file(file_path, library_root) @@ -376,11 +378,7 @@ def save_inventory_csv(files: list[VideoFile], output: Path, library_root: Path) # Write each file for video_file in files: # Format timestamp as ISO 8601 in UTC - if video_file.modified_timestamp.tzinfo is None: - # Assume local time, convert to UTC - modified_utc = video_file.modified_timestamp.replace(tzinfo=timezone.utc) - else: - modified_utc = video_file.modified_timestamp.astimezone(timezone.utc) + modified_utc = _normalize_to_utc(video_file.modified_timestamp) row = { 'path': str(video_file.path), @@ -434,11 +432,7 @@ def save_inventory_json(files: list[VideoFile], output: Path, library_root: Path # Add each file for video_file in files: # Format timestamp as ISO 8601 in UTC - if video_file.modified_timestamp.tzinfo is None: - # Assume local time, convert to UTC - modified_utc = video_file.modified_timestamp.replace(tzinfo=timezone.utc) - else: - modified_utc = video_file.modified_timestamp.astimezone(timezone.utc) + modified_utc = _normalize_to_utc(video_file.modified_timestamp) file_data = { 'path': str(video_file.path), diff --git a/tests/test_cli_state.py b/tests/test_cli_state.py index cd1f7c2..c681168 100644 --- a/tests/test_cli_state.py +++ b/tests/test_cli_state.py @@ -7,7 +7,7 @@ from pathlib import Path import pytest from click.testing import CliRunner -from vlm.cli import main +from vlm.cli import main, default_config_path @pytest.fixture @@ -85,6 +85,12 @@ def test_state_set_and_show(runner, temp_state_file, temp_config): assert "checked manually" in result.output +def test_default_config_path_resolves_at_runtime(tmp_path, monkeypatch): + """Test CLI default config path follows current home directory.""" + monkeypatch.setattr(Path, 'home', lambda: tmp_path) + assert default_config_path() == tmp_path / ".vlm" / "config.yaml" + + def test_state_query(runner, temp_state_file, temp_config): """Test querying files by status.""" test_files = [ diff --git a/tests/test_executor.py b/tests/test_executor.py index 7421c2f..31c474d 100644 --- a/tests/test_executor.py +++ b/tests/test_executor.py @@ -10,8 +10,8 @@ from uuid import uuid4 import pytest -from src.vlm.executor import ExecutionEngine -from src.vlm.models import ExecutionPlan, FileOperation +from vlm.executor import ExecutionEngine +from vlm.models import ExecutionPlan, FileOperation @pytest.fixture diff --git a/tests/test_logging.py b/tests/test_logging.py index 8dadfec..dd81e7a 100644 --- a/tests/test_logging.py +++ b/tests/test_logging.py @@ -11,6 +11,7 @@ from vlm.logging_config import ( get_logger, log_operation, MAX_LOG_SIZE, + default_log_dir, ) @@ -71,6 +72,28 @@ class TestLoggingSetup: logger = setup_logging(log_level=level, log_dir=tmp_path) assert logger is not None + def test_setup_logging_falls_back_when_file_logging_unwritable(self, tmp_path, monkeypatch): + """Test setup continues with console logging if file logging cannot initialize.""" + def fail_mkdir(self, parents=False, exist_ok=False): + raise PermissionError("mock permission denied") + + monkeypatch.setattr(Path, "mkdir", fail_mkdir) + + logger = setup_logging(log_level="INFO", log_dir=tmp_path / "logs") + + logger.info("Console-only logging still works") + assert len(logger.handlers) == 1 + assert isinstance(logger.handlers[0], logging.StreamHandler) + + +class TestDefaultPathResolution: + """Test runtime default path resolution.""" + + def test_default_log_dir_resolves_at_runtime(self, tmp_path, monkeypatch): + """Test default log directory follows current Path.home() value.""" + monkeypatch.setattr(Path, "home", lambda: tmp_path) + assert default_log_dir() == tmp_path / ".vlm" / "logs" + class TestDualOutput: """Test dual output to console and file.""" diff --git a/tests/test_quarantine.py b/tests/test_quarantine.py index cd68d74..9d8c77f 100644 --- a/tests/test_quarantine.py +++ b/tests/test_quarantine.py @@ -5,9 +5,9 @@ import pytest from pathlib import Path from datetime import datetime -from src.vlm.quarantine import QuarantineManager -from src.vlm.config import Config -from src.vlm.models import QuarantineEntry, QuarantineManifest +from vlm.quarantine import QuarantineManager +from vlm.config import Config +from vlm.models import QuarantineEntry, QuarantineManifest class TestQuarantineManager: diff --git a/tests/test_reports.py b/tests/test_reports.py index 64c2182..cd04f88 100644 --- a/tests/test_reports.py +++ b/tests/test_reports.py @@ -5,10 +5,12 @@ Tests inventory reports, completeness reports, duplicate reports, and summary re import csv import json +import os +import time import pytest from io import StringIO from pathlib import Path -from datetime import datetime +from datetime import datetime, timezone from vlm.models import SeasonCompleteness, DuplicateGroup, VideoFile, MovieIdentity, SeriesIdentity from vlm.reports import ( generate_inventory_report, @@ -205,12 +207,13 @@ class TestInventoryReport: def test_timestamp_formatting(self): """Test that timestamps are formatted as ISO 8601.""" + naive_local = datetime(2023, 6, 15, 14, 30, 45) files = [ VideoFile( Path("/test.mkv"), "test.mkv", 1000, - datetime(2023, 6, 15, 14, 30, 45), + naive_local, "movie" ) ] @@ -219,7 +222,38 @@ class TestInventoryReport: report = generate_inventory_report(files, "csv", library_root) # Check timestamp format - assert "2023-06-15T14:30:45" in report + expected_utc = naive_local.astimezone(timezone.utc).strftime("%Y-%m-%dT%H:%M:%S") + assert expected_utc in report + + @pytest.mark.skipif(not hasattr(time, "tzset"), reason="tzset not available on this platform") + def test_naive_timestamp_is_converted_from_local_to_utc(self, monkeypatch): + """Test report conversion for naive timestamps uses local timezone semantics.""" + original_tz = os.environ.get("TZ") + try: + monkeypatch.setenv("TZ", "Etc/GMT-2") + time.tzset() + + naive_local = datetime(2023, 6, 15, 14, 30, 45) + expected_utc = naive_local.astimezone(timezone.utc).strftime("%Y-%m-%dT%H:%M:%S") + + files = [ + VideoFile( + Path("/test.mkv"), + "test.mkv", + 1000, + naive_local, + "movie" + ) + ] + report = generate_inventory_report(files, "json", Path("/test")) + data = json.loads(report) + assert data["files"][0]["modified_timestamp"] == expected_utc + finally: + if original_tz is None: + monkeypatch.delenv("TZ", raising=False) + else: + monkeypatch.setenv("TZ", original_tz) + time.tzset() class TestCompletenessReport: diff --git a/tests/test_scanner.py b/tests/test_scanner.py index a2ac764..4326514 100644 --- a/tests/test_scanner.py +++ b/tests/test_scanner.py @@ -2,7 +2,8 @@ import os import tempfile -from datetime import datetime +import time +from datetime import datetime, timezone from pathlib import Path from unittest.mock import patch, MagicMock import subprocess @@ -125,6 +126,7 @@ class TestScanLibrary: assert vf.path == video_file assert vf.size_bytes > 0 assert isinstance(vf.modified_timestamp, datetime) + assert vf.modified_timestamp.tzinfo == timezone.utc assert vf.category == "movie" def test_scan_categorizes_files(self, tmp_path): @@ -867,3 +869,37 @@ class TestInventoryReports: assert csv_row['category'] == json_file_data['category'] assert csv_row['resolution'] == json_file_data['resolution'] assert csv_row['codec'] == json_file_data['codec'] + + @pytest.mark.skipif(not hasattr(time, "tzset"), reason="tzset not available on this platform") + def test_save_inventory_csv_converts_naive_local_time_to_utc(self, tmp_path, monkeypatch): + """Test naive timestamps are interpreted as local time and converted to UTC.""" + original_tz = os.environ.get("TZ") + try: + monkeypatch.setenv("TZ", "Etc/GMT-2") + time.tzset() + + naive_local = datetime(2024, 1, 15, 10, 30, 0) + expected_utc = naive_local.astimezone(timezone.utc).strftime("%Y-%m-%dT%H:%M:%S") + + video_files = [ + VideoFile( + path=Path("/library/movie/test.mp4"), + filename="test.mp4", + size_bytes=1024000, + modified_timestamp=naive_local, + category="movie" + ) + ] + + output_file = tmp_path / "inventory.csv" + from vlm.scanner import save_inventory_csv + save_inventory_csv(video_files, output_file, Path("/library")) + + content = output_file.read_text(encoding="utf-8") + assert expected_utc in content + finally: + if original_tz is None: + monkeypatch.delenv("TZ", raising=False) + else: + monkeypatch.setenv("TZ", original_tz) + time.tzset()