refactor: consolidate skill docs, add anti-drift tests, and apply audit fixes
DLO-13: Restructure vlm-library-workflow skill as safety contract layer. - Rewrite SKILL.md (69 lines): safety contract, execution threshold semantics, six-step high-risk loop, decision rules, phase skeleton - Delete redundant references (cli-reference, workflow, command-recipes, dev-guide) - Add triage.md (failure mapping + preflight) and dev-map.md (module→test mapping) - Add tests/test_docs_consistency.py: 78 parametrized tests verifying documented vlm commands exist in CLI registry - Add CSV path mismatch test to test_plan_review.py (4th safety gate path) - Delete vlm-expert.skill (Gemini package, 7 months stale) and README Gemini section DLO-2 audit fixes: rate limiter injection, symmetric quarantine categories, review-plan safety gates, parser improvements, planner validation. CLI modularization: commands/ directory with one module per command group.
This commit is contained in:
windyboy
@@ -177,24 +177,24 @@ class TestQuarantineAddCommand:
|
||||
assert "Reason: duplicate file" in result.output
|
||||
assert "successfully quarantined" in result.output
|
||||
|
||||
def test_add_anime_file_rejected(self, config_file, temp_library):
|
||||
"""Test that anime files are rejected."""
|
||||
def test_add_anime_file_supported(self, config_file, temp_library):
|
||||
"""Test that anime files can be quarantined (configured category)."""
|
||||
runner = CliRunner()
|
||||
|
||||
# Create a test anime file
|
||||
anime_file = temp_library / "anime" / "Anime Show.mkv"
|
||||
anime_file.write_text("anime content")
|
||||
|
||||
# Try to quarantine (should fail)
|
||||
# Quarantine should succeed
|
||||
result = runner.invoke(main, [
|
||||
'--config', str(config_file),
|
||||
'quarantine', 'add',
|
||||
str(anime_file)
|
||||
])
|
||||
|
||||
assert result.exit_code == 1
|
||||
assert "not supported" in result.output.lower()
|
||||
assert anime_file.exists() # File should still exist
|
||||
assert result.exit_code == 0
|
||||
assert "successfully quarantined" in result.output
|
||||
assert not anime_file.exists()
|
||||
|
||||
def test_add_nonexistent_file(self, config_file, temp_library):
|
||||
"""Test adding a file that doesn't exist."""
|
||||
|
||||
@@ -0,0 +1,82 @@
|
||||
"""Verify vlm commands referenced in documentation exist in the CLI registry."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import re
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
from click.testing import CliRunner
|
||||
|
||||
from vlm.cli import main
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parent.parent
|
||||
DOC_GLOBS = [
|
||||
"README.md",
|
||||
"CLAUDE.md",
|
||||
"AGENTS.md",
|
||||
"skills/**/*.md",
|
||||
]
|
||||
|
||||
COMMAND_PATTERN = re.compile(
|
||||
r"^(?:uv run )?(?:\$ )?vlm\s+(.+)$", re.MULTILINE
|
||||
)
|
||||
|
||||
|
||||
def _extract_doc_commands() -> list[str]:
|
||||
commands: list[str] = []
|
||||
for pattern in DOC_GLOBS:
|
||||
for path in REPO_ROOT.glob(pattern):
|
||||
text = path.read_text(encoding="utf-8")
|
||||
for match in COMMAND_PATTERN.finditer(text):
|
||||
raw = match.group(1).strip()
|
||||
raw = raw.rstrip("\\").strip()
|
||||
if raw.startswith("#") or not raw:
|
||||
continue
|
||||
commands.append(raw)
|
||||
return commands
|
||||
|
||||
|
||||
def _cli_command_names() -> dict[str, list[str]]:
|
||||
result: dict[str, list[str]] = {}
|
||||
for name, cmd in main.commands.items():
|
||||
result[name] = []
|
||||
if hasattr(cmd, "commands"):
|
||||
result[name] = list(cmd.commands.keys())
|
||||
return result
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"raw_cmd",
|
||||
sorted(set(_extract_doc_commands())),
|
||||
ids=lambda c: c[:60],
|
||||
)
|
||||
def test_documented_command_exists(raw_cmd: str):
|
||||
parts = raw_cmd.split()
|
||||
if not parts:
|
||||
pytest.skip("empty command")
|
||||
|
||||
first = parts[0]
|
||||
if first in ("--help", "-h"):
|
||||
return
|
||||
|
||||
registry = _cli_command_names()
|
||||
|
||||
if first not in registry:
|
||||
pytest.fail(f"Documented command 'vlm {first}' not in CLI registry")
|
||||
|
||||
if len(parts) > 1:
|
||||
sub = parts[1]
|
||||
if sub.startswith("-"):
|
||||
return
|
||||
subcommands = registry.get(first, [])
|
||||
if subcommands and sub not in subcommands:
|
||||
pytest.fail(
|
||||
f"Documented subcommand 'vlm {first} {sub}' "
|
||||
f"not in CLI registry (available: {subcommands})"
|
||||
)
|
||||
|
||||
|
||||
def test_fake_command_is_caught():
|
||||
registry = _cli_command_names()
|
||||
assert "nonexistent-command-xyz" not in registry
|
||||
@@ -45,7 +45,7 @@ def test_enrich_incremental_cache_hit(tmp_path, monkeypatch):
|
||||
provider = DummyProvider()
|
||||
monkeypatch.setattr(
|
||||
"vlm.enrichment._build_providers",
|
||||
lambda _config, request_timeout, retries: [provider],
|
||||
lambda _config, request_timeout, retries, **_kwargs: [provider],
|
||||
)
|
||||
|
||||
identities = {
|
||||
@@ -113,7 +113,7 @@ def test_enrich_flags_low_reputation_for_review(tmp_path, monkeypatch):
|
||||
provider = LowScoreProvider()
|
||||
monkeypatch.setattr(
|
||||
"vlm.enrichment._build_providers",
|
||||
lambda _config, request_timeout, retries: [provider],
|
||||
lambda _config, request_timeout, retries, **_kwargs: [provider],
|
||||
)
|
||||
|
||||
identities = {
|
||||
@@ -151,7 +151,7 @@ def test_enrich_refresh_all_bypasses_cache(tmp_path, monkeypatch):
|
||||
provider = DummyProvider()
|
||||
monkeypatch.setattr(
|
||||
"vlm.enrichment._build_providers",
|
||||
lambda _config, request_timeout, retries: [provider],
|
||||
lambda _config, request_timeout, retries, **_kwargs: [provider],
|
||||
)
|
||||
|
||||
identities = {
|
||||
@@ -227,7 +227,7 @@ def test_refresh_all_clears_stale_enrichment_fields(tmp_path, monkeypatch):
|
||||
provider = FlakyProvider()
|
||||
monkeypatch.setattr(
|
||||
"vlm.enrichment._build_providers",
|
||||
lambda _config, request_timeout, retries: [provider],
|
||||
lambda _config, request_timeout, retries, **_kwargs: [provider],
|
||||
)
|
||||
|
||||
identities = {
|
||||
@@ -331,7 +331,7 @@ def test_enrich_auth_failure_stops_immediately(tmp_path, monkeypatch):
|
||||
|
||||
monkeypatch.setattr(
|
||||
"vlm.enrichment._build_providers",
|
||||
lambda _config, request_timeout, retries: [AuthFailProvider()],
|
||||
lambda _config, request_timeout, retries, **_kwargs: [AuthFailProvider()],
|
||||
)
|
||||
|
||||
identities = {
|
||||
@@ -387,8 +387,15 @@ def test_enrich_respects_max_concurrency_for_uncached_records(tmp_path, monkeypa
|
||||
|
||||
thread_ids: set[int] = set()
|
||||
lock = threading.Lock()
|
||||
|
||||
def _fake_enrich(record, media_type, config_obj, request_timeout, retries):
|
||||
seen_limiters: dict[str, object] = {}
|
||||
def _fake_enrich(record, media_type, config_obj, request_timeout, retries, rate_limiters=None):
|
||||
with lock:
|
||||
if rate_limiters is not None and "tmdb" in rate_limiters:
|
||||
limiter = rate_limiters["tmdb"]
|
||||
if "limiter" not in seen_limiters:
|
||||
seen_limiters["limiter"] = limiter
|
||||
else:
|
||||
assert limiter is seen_limiters["limiter"]
|
||||
time.sleep(0.01)
|
||||
with lock:
|
||||
thread_ids.add(threading.get_ident())
|
||||
@@ -420,3 +427,81 @@ def test_enrich_respects_max_concurrency_for_uncached_records(tmp_path, monkeypa
|
||||
assert stats["enriched"] == 8
|
||||
assert stats["cache_hits"] == 0
|
||||
assert len(thread_ids) > 1
|
||||
|
||||
|
||||
class _RateLimitedTMDB:
|
||||
name = "tmdb"
|
||||
|
||||
def __init__(self, api_key, *, rate_limiter=None, record_wait=None, wait_lock=None, **kwargs):
|
||||
self.rate_limiter = rate_limiter
|
||||
self.last_request_count = 0
|
||||
self._record_wait = record_wait
|
||||
self._wait_lock = wait_lock
|
||||
|
||||
def enrich(self, *, title, media_type, year=None):
|
||||
if self.rate_limiter is not None:
|
||||
self.rate_limiter.wait()
|
||||
if self._record_wait is not None and self._wait_lock is not None:
|
||||
with self._wait_lock:
|
||||
self._record_wait.append(time.monotonic())
|
||||
self.last_request_count = 1
|
||||
return ProviderResult(
|
||||
provider="tmdb",
|
||||
canonical_id=f"tmdb:{title}",
|
||||
title_zh="测试",
|
||||
title_en=title,
|
||||
translation_source="tmdb",
|
||||
)
|
||||
|
||||
|
||||
def test_enrich_concurrent_workers_share_provider_rate_limiter(tmp_path, monkeypatch):
|
||||
"""Concurrent TMDB providers must admit requests through one limiter."""
|
||||
config = Config(
|
||||
library_root=tmp_path,
|
||||
enrichment_cache_db=tmp_path / "cache.db",
|
||||
enrichment_providers=["tmdb"],
|
||||
tmdb_api_key="fake",
|
||||
translation_fallback_machine=False,
|
||||
enrichment_max_concurrency=4,
|
||||
)
|
||||
|
||||
identities = {
|
||||
"metadata": {},
|
||||
"movies": [
|
||||
{
|
||||
"path": f"/library/movie/Test.{i}.mkv",
|
||||
"filename": f"Test.{i}.mkv",
|
||||
"category": "movie",
|
||||
"title": f"Test {i}",
|
||||
"year": 2020,
|
||||
"confidence": 0.9,
|
||||
"needs_review": False,
|
||||
}
|
||||
for i in range(3)
|
||||
],
|
||||
"series": [],
|
||||
"anime": [],
|
||||
"other": [],
|
||||
}
|
||||
|
||||
shared_limiters: dict[str, object] = {}
|
||||
wait_times: list[float] = []
|
||||
wait_lock = threading.Lock()
|
||||
|
||||
def _factory(api_key, **kwargs):
|
||||
limiter = kwargs.pop("rate_limiter")
|
||||
if "limiter" not in shared_limiters:
|
||||
shared_limiters["limiter"] = limiter
|
||||
else:
|
||||
assert limiter is shared_limiters["limiter"]
|
||||
return _RateLimitedTMDB(api_key, rate_limiter=limiter, record_wait=wait_times, wait_lock=wait_lock)
|
||||
|
||||
monkeypatch.setattr("vlm.enrichment.TMDBProvider", _factory)
|
||||
|
||||
_, stats = enrich_identities_data(identities, config, refresh_mode="refresh_all")
|
||||
assert stats["enriched"] == 3
|
||||
assert len(wait_times) == 3
|
||||
# Shared limiter must serialize request starts at >= 0.25s apart
|
||||
# (allow small tolerance for thread scheduling).
|
||||
for earlier, later in zip(wait_times, wait_times[1:]):
|
||||
assert later - earlier >= 0.2
|
||||
|
||||
@@ -76,6 +76,63 @@ def test_build_identity_lookup_and_enrich(tmp_path):
|
||||
assert enriched[0]["source_name"] == "Show.S01E01.mkv"
|
||||
|
||||
|
||||
def test_enrich_review_rows_sidecars_column(tmp_path):
|
||||
op = FileOperation(
|
||||
operation_type="move",
|
||||
source_path=tmp_path / "dl/Show.S01E01.mkv",
|
||||
destination_path=tmp_path / "lib/series/Show/Season 01/S01E01.mkv",
|
||||
reason="organize",
|
||||
has_conflict=False,
|
||||
review_context={
|
||||
"title": "Show",
|
||||
"category": "series",
|
||||
"season": 1,
|
||||
"episode": 1,
|
||||
"sidecars": [
|
||||
{"name": "Show.S01E01.zh.srt", "source_path": str(tmp_path / "dl/Show.S01E01.zh.srt"), "proposed_destination_path": str(tmp_path / "lib/series/Show/Season 01/Show.S01E01.zh.srt")},
|
||||
{"name": "Show.S01E01.nfo", "source_path": str(tmp_path / "dl/Show.S01E01.nfo"), "proposed_destination_path": str(tmp_path / "lib/series/Show/Season 01/Show.S01E01.nfo")},
|
||||
],
|
||||
},
|
||||
)
|
||||
plan = _minimal_plan([op])
|
||||
rows = [
|
||||
{
|
||||
"index": "1",
|
||||
"operation_type": "move",
|
||||
"risk_flags": "",
|
||||
"source_path": str(op.source_path),
|
||||
"destination_path": str(op.destination_path),
|
||||
"reason": op.reason,
|
||||
}
|
||||
]
|
||||
enriched = enrich_review_rows(rows, plan, library_root=tmp_path / "lib")
|
||||
assert enriched[0]["sidecars"] == "Show.S01E01.zh.srt, Show.S01E01.nfo"
|
||||
|
||||
|
||||
def test_enrich_review_rows_sidecars_empty_by_default(tmp_path):
|
||||
op = FileOperation(
|
||||
operation_type="move",
|
||||
source_path=tmp_path / "dl/Show.S01E01.mkv",
|
||||
destination_path=tmp_path / "lib/series/Show/Season 01/S01E01.mkv",
|
||||
reason="organize",
|
||||
has_conflict=False,
|
||||
review_context={"title": "Show", "category": "series", "season": 1, "episode": 1},
|
||||
)
|
||||
plan = _minimal_plan([op])
|
||||
rows = [
|
||||
{
|
||||
"index": "1",
|
||||
"operation_type": "move",
|
||||
"risk_flags": "",
|
||||
"source_path": str(op.source_path),
|
||||
"destination_path": str(op.destination_path),
|
||||
"reason": op.reason,
|
||||
}
|
||||
]
|
||||
enriched = enrich_review_rows(rows, plan, library_root=tmp_path / "lib")
|
||||
assert enriched[0].get("sidecars", "") == ""
|
||||
|
||||
|
||||
def test_check_review_requirements_missing_csv(tmp_path):
|
||||
plan_path = tmp_path / "plan.json"
|
||||
op = FileOperation(
|
||||
@@ -137,6 +194,30 @@ def test_check_review_requirements_passes_after_apply_review(tmp_path):
|
||||
assert errors == []
|
||||
|
||||
|
||||
def test_check_review_requirements_detects_csv_path_mismatch(tmp_path):
|
||||
plan_path = tmp_path / "plan.json"
|
||||
csv_path = tmp_path / "review.csv"
|
||||
other_csv = tmp_path / "other.csv"
|
||||
op = FileOperation(
|
||||
operation_type="move",
|
||||
source_path=tmp_path / "a.mkv",
|
||||
destination_path=tmp_path / "lib/a.mkv",
|
||||
reason="Series needs manual review (season exceeds configured threshold)",
|
||||
has_conflict=True,
|
||||
)
|
||||
plan = _minimal_plan([op])
|
||||
save_plan(plan, plan_path)
|
||||
rows, _ = review_plan(plan)
|
||||
save_review_csv(rows, csv_path)
|
||||
save_review_csv(rows, other_csv)
|
||||
|
||||
updated = apply_review_to_plan(load_plan(plan_path), csv_path)
|
||||
save_plan(updated, plan_path)
|
||||
|
||||
errors = check_review_requirements(load_plan(plan_path), plan_path, other_csv)
|
||||
assert any("does not match" in e for e in errors)
|
||||
|
||||
|
||||
def test_planner_noop_manual_review_does_not_block_execute_gate(tmp_path):
|
||||
plan_path = tmp_path / "plan.json"
|
||||
op = FileOperation(
|
||||
|
||||
@@ -63,6 +63,92 @@ def test_generate_plan_for_movie_with_year(config):
|
||||
assert "Some Movie (2020)" in operation.reason
|
||||
|
||||
|
||||
|
||||
def test_generate_plan_attaches_sidecar_context(tmp_path):
|
||||
"""Test move operations carry review-visible sidecar associations."""
|
||||
config = Config(
|
||||
library_root=tmp_path,
|
||||
video_extensions=[".mp4", ".mkv", ".avi"],
|
||||
movie_template="movie/{title} ({year})/",
|
||||
movie_filename_template="{title} ({year}){ext}",
|
||||
)
|
||||
|
||||
src_dir = tmp_path / "downloads"
|
||||
src_dir.mkdir()
|
||||
video = src_dir / "Movie (2020).mkv"
|
||||
video.touch()
|
||||
sub = src_dir / "Movie (2020).zh.srt"
|
||||
sub.touch()
|
||||
nfo = src_dir / "Movie (2020).nfo"
|
||||
nfo.touch()
|
||||
unrelated = src_dir / "Movie (2020).1.srt"
|
||||
unrelated.touch()
|
||||
|
||||
video_file = VideoFile(
|
||||
path=video,
|
||||
filename=video.name,
|
||||
size_bytes=1000000,
|
||||
modified_timestamp=datetime.now(timezone.utc),
|
||||
category="movie",
|
||||
)
|
||||
identity = MovieIdentity(
|
||||
title="Movie",
|
||||
year=2020,
|
||||
confidence=0.9,
|
||||
needs_review=False,
|
||||
original_filename=video.name,
|
||||
)
|
||||
|
||||
plan = generate_plan([(video_file, identity)], config)
|
||||
operation = plan.operations[0]
|
||||
assert operation.operation_type == "move"
|
||||
sidecars = operation.review_context.get("sidecars")
|
||||
assert sidecars is not None
|
||||
assert [s["name"] for s in sidecars] == ["Movie (2020).nfo", "Movie (2020).zh.srt"]
|
||||
dest_dir = operation.destination_path.parent
|
||||
by_name = {s["name"]: s for s in sidecars}
|
||||
assert by_name["Movie (2020).zh.srt"]["proposed_destination_path"] == str(dest_dir / "Movie (2020).zh.srt")
|
||||
assert by_name["Movie (2020).nfo"]["proposed_destination_path"] == str(dest_dir / "Movie (2020).nfo")
|
||||
# Numeric-suffix file must not be associated
|
||||
assert all(s["name"] != "Movie (2020).1.srt" for s in sidecars)
|
||||
|
||||
|
||||
def test_generate_plan_no_sidecars_for_noop(tmp_path):
|
||||
"""Test no-op operations carry no sidecar context."""
|
||||
config = Config(
|
||||
library_root=tmp_path,
|
||||
video_extensions=[".mp4", ".mkv", ".avi"],
|
||||
movie_template="movie/{title} ({year})/",
|
||||
movie_filename_template="{title} ({year}){ext}",
|
||||
)
|
||||
|
||||
src_dir = tmp_path / "downloads"
|
||||
src_dir.mkdir()
|
||||
video = src_dir / "random_movie.mkv"
|
||||
video.touch()
|
||||
sub = src_dir / "random_movie.srt"
|
||||
sub.touch()
|
||||
|
||||
video_file = VideoFile(
|
||||
path=video,
|
||||
filename=video.name,
|
||||
size_bytes=1000000,
|
||||
modified_timestamp=datetime.now(timezone.utc),
|
||||
category="movie",
|
||||
)
|
||||
identity = MovieIdentity(
|
||||
title="Random Movie",
|
||||
year=None,
|
||||
confidence=0.3,
|
||||
needs_review=True,
|
||||
original_filename=video.name,
|
||||
)
|
||||
|
||||
plan = generate_plan([(video_file, identity)], config)
|
||||
operation = plan.operations[0]
|
||||
assert operation.operation_type == "no-op"
|
||||
assert "sidecars" not in (operation.review_context or {})
|
||||
|
||||
def test_generate_plan_for_movie_without_year(config):
|
||||
"""Test plan generation for a movie without year (needs review)."""
|
||||
video_file = VideoFile(
|
||||
|
||||
@@ -82,19 +82,26 @@ class TestQuarantineManager:
|
||||
assert expected_quarantine_path.exists()
|
||||
assert expected_quarantine_path.read_text() == "test content"
|
||||
|
||||
def test_quarantine_anime_file_rejected(self, manager, config):
|
||||
"""Test that quarantining anime files returns a failed result."""
|
||||
def test_quarantine_anime_file_round_trip(self, manager, config):
|
||||
"""Test that quarantining and restoring anime files works."""
|
||||
# Create a test anime file
|
||||
anime_file = config.library_root / "anime" / "Test Anime.mkv"
|
||||
anime_file.write_text("test content")
|
||||
|
||||
result = manager.quarantine_file(anime_file)
|
||||
|
||||
assert result.success is False
|
||||
assert "Quarantine not supported for category 'anime'" in (result.error_message or "")
|
||||
assert result.success is True
|
||||
expected_quarantine_path = config.library_root / "anime" / ".quarantine" / "Test Anime.mkv"
|
||||
assert not anime_file.exists()
|
||||
assert expected_quarantine_path.exists()
|
||||
assert expected_quarantine_path.read_text() == "test content"
|
||||
|
||||
# Verify file was not moved
|
||||
# Restore and verify round trip
|
||||
restore_result = manager.restore_from_quarantine(expected_quarantine_path)
|
||||
assert restore_result.success is True
|
||||
assert anime_file.exists()
|
||||
assert anime_file.read_text() == "test content"
|
||||
assert not expected_quarantine_path.exists()
|
||||
|
||||
def test_quarantine_other_file_rejected(self, manager, config):
|
||||
"""Test that quarantining other files returns a failed result."""
|
||||
@@ -590,7 +597,7 @@ class TestQuarantineListing:
|
||||
|
||||
def test_list_quarantined_invalid_category(self, manager, config):
|
||||
"""Test listing with invalid category returns empty list."""
|
||||
entries = manager.list_quarantined(category="anime")
|
||||
entries = manager.list_quarantined(category="unconfigured")
|
||||
assert entries == []
|
||||
|
||||
entries = manager.list_quarantined(category="other")
|
||||
@@ -906,7 +913,7 @@ class TestQuarantineRestoration:
|
||||
category = manager._determine_category_from_quarantine(outside)
|
||||
assert category is None
|
||||
|
||||
# Unsupported category
|
||||
anime = config.library_root / "anime" / ".quarantine" / "Anime.mkv"
|
||||
category = manager._determine_category_from_quarantine(anime)
|
||||
# Unsupported category (directory not in configured categories)
|
||||
other = config.library_root / "other" / ".quarantine" / "Other.mkv"
|
||||
category = manager._determine_category_from_quarantine(other)
|
||||
assert category is None
|
||||
|
||||
Reference in New Issue
Block a user