fix timezone handling and logging fallback robustness
This commit is contained in:
+118
@@ -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`
|
||||
@@ -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:
|
||||
|
||||
+214
-29
@@ -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.
|
||||
|
||||
+5
-4
@@ -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):
|
||||
|
||||
@@ -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,6 +83,8 @@ def setup_logging(
|
||||
logger.addHandler(console_handler)
|
||||
|
||||
# File handler with rotation (DEBUG+)
|
||||
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,
|
||||
@@ -94,6 +96,10 @@ def setup_logging(
|
||||
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
|
||||
|
||||
+9
-10
@@ -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),
|
||||
|
||||
+10
-16
@@ -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),
|
||||
|
||||
@@ -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 = [
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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."""
|
||||
|
||||
@@ -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:
|
||||
|
||||
+37
-3
@@ -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:
|
||||
|
||||
+37
-1
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user