235 lines
12 KiB
Markdown
235 lines
12 KiB
Markdown
# Code Review Report (Verified)
|
|
|
|
## Scope
|
|
- Reviewed Python sources under `src/vlm/`, tests under `tests/`, and docs (`README.md`, `AGENTS.md`).
|
|
- Verified behavior on **February 9, 2026** with:
|
|
- `uv run --with pytest --with hypothesis pytest -q`
|
|
- `uv run --with pytest --with hypothesis pytest -q --ignore=tests/test_executor.py --ignore=tests/test_quarantine.py`
|
|
- Targeted runtime checks for logging init and `vlm --help` behavior.
|
|
|
|
## Summary
|
|
- Confirmed **3 concrete code issues** and **1 conditional robustness risk**.
|
|
- The previous claim that ignored-suite tests had dozens of logging-related failures is **not reproducible** in current repo state.
|
|
- Found one additional missed issue family: timezone relabeling logic also exists in `src/vlm/reports.py`.
|
|
|
|
## Findings
|
|
|
|
### P1 - Test imports use wrong module path (confirmed)
|
|
- Files:
|
|
- `tests/test_executor.py:13`
|
|
- `tests/test_executor.py:14`
|
|
- `tests/test_quarantine.py:8`
|
|
- `tests/test_quarantine.py:9`
|
|
- `tests/test_quarantine.py:10`
|
|
- Tests import `src.vlm...` instead of installed package path `vlm...`.
|
|
- Impact: test collection stops with `ModuleNotFoundError: No module named 'src'`.
|
|
- Recommended fix:
|
|
- Replace imports in affected tests:
|
|
- `from src.vlm.executor import ExecutionEngine` -> `from vlm.executor import ExecutionEngine`
|
|
- `from src.vlm.models import ...` -> `from vlm.models import ...`
|
|
- `from src.vlm.quarantine import QuarantineManager` -> `from vlm.quarantine import QuarantineManager`
|
|
- `from src.vlm.config import Config` -> `from vlm.config import Config`
|
|
- Re-run: `uv run --with pytest --with hypothesis pytest -q`
|
|
|
|
### P2 - Naive datetime is relabeled as UTC instead of converted (confirmed)
|
|
- Files:
|
|
- `src/vlm/scanner.py:148`
|
|
- `src/vlm/scanner.py:381`
|
|
- `src/vlm/scanner.py:439`
|
|
- `src/vlm/reports.py:92`
|
|
- `src/vlm/reports.py:132`
|
|
- Pattern uses `replace(tzinfo=timezone.utc)` for naive values, which does not perform timezone conversion.
|
|
- Impact: exported timestamps can be offset by local timezone difference.
|
|
- Recommended fix:
|
|
- Normalize file mtime as timezone-aware at source:
|
|
- In `src/vlm/scanner.py:148`, prefer `datetime.fromtimestamp(stat.st_mtime, tz=timezone.utc)`.
|
|
- In exporters (`scanner.py`, `reports.py`), avoid relabeling:
|
|
- If timestamp is naive, interpret it as local time then convert:
|
|
- `dt_local = video_file.modified_timestamp.astimezone()` (local tz attach)
|
|
- `modified_utc = dt_local.astimezone(timezone.utc)`
|
|
- Or enforce invariant that `VideoFile.modified_timestamp` is always aware UTC and simplify output logic.
|
|
- Add regression tests for non-UTC local timezone scenarios.
|
|
|
|
### P3 - `error_count` in scanner is dead code (confirmed)
|
|
- Files:
|
|
- `src/vlm/scanner.py:53`
|
|
- `src/vlm/scanner.py:64`
|
|
- `error_count` is initialized and conditionally logged, but never incremented.
|
|
- Impact: scan summary can underreport encountered scan errors.
|
|
- Recommended fix:
|
|
- Preferred: remove `error_count` and rely on structured per-error logs.
|
|
- Alternative: return `(video_file, had_error)` from recursive scanner or propagate error counters upward and increment on exception branches at `src/vlm/scanner.py:107` and `src/vlm/scanner.py:112`.
|
|
|
|
### 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
|
|
- `uv run --with pytest --with hypothesis pytest -q`:
|
|
- fails at collection with 2 errors (`tests/test_executor.py`, `tests/test_quarantine.py`) due to `src.vlm` imports.
|
|
- `uv run --with pytest --with hypothesis pytest -q --ignore=tests/test_executor.py --ignore=tests/test_quarantine.py`:
|
|
- **293 passed**, no observed logging-related mass failures.
|
|
- Direct runtime check:
|
|
- `setup_logging(log_dir=Path('/sys/...'))` raises `PermissionError`, confirming the conditional logging robustness risk.
|
|
|
|
## Recommended Fix Order
|
|
1. Fix test imports from `src.vlm...` to `vlm...` so full suite can run.
|
|
2. Correct timezone handling in both `scanner.py` and `reports.py`.
|
|
3. Either remove `error_count` or increment it on scan exceptions.
|
|
4. Make logging initialization resilient (fallback to console-only logging when file logging setup fails).
|
|
5. Move home-based defaults from import-time constants to runtime-resolved helpers.
|
|
|
|
## 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.
|