7 Commits (2fcdfd3145d415355b9202c406d8985fda8eb003)

Author	SHA1	Message	Date
Broque Thomas	e27ecb84f4	Final review-pass nits — class docstring, dead branch, dead imports, boot resilience ... Going line-by-line through the engine package + boot wiring. Five small things worth fixing before Cin reads it: (1) MediaServerEngine class docstring still claimed to be a "single entry point for cross-server library operations" — but the prior honesty pass cut all the cross-server dispatch wrappers because they had no callers. Class is really lookup + small accessors now. Docstring rewritten to match. (2) configured_clients() had a dead `not hasattr(client, 'is_connected')` branch. is_connected is in REQUIRED_METHODS so every client the registry yields here implements it. Branch removed; comment notes the reasoning. (3) types.py imported `datetime` and `Dict` but used neither — dead imports dropped. (4) types.py docstring claimed "all four servers" defined an XTrackInfo dataclass. Actually only Plex / Jellyfin / Navidrome did; SoulSync uses richer per-track wrappers. Fixed. (5) web_server.py boot: - media_server_engine added to the chained `= None` declaration so it's always defined before the try/except, defending against the rare path where engine init AND fallback both raise. - Outer engine init failure logger now uses exc_info=True for full traceback (boot-time issues are rare but worth diagnosing). - Nested fallback failure now logs explicitly instead of silently leaving media_server_engine as None. Tests: 2121 still pass.	2 months ago
Broque Thomas	6489244bcc	MS Cin/JohnBaumb honesty pass — drop dead wrappers, sync contract to reality ... Pre-review audit found premature abstraction + lying docstrings. Cut what isn't used, made the rest match what's actually shipped. (1) Engine: dropped 7 cross-server dispatch wrappers that had ZERO production callers (ensure_connection / get_all_artists / get_all_album_ids / search_tracks / trigger_library_scan / is_library_scanning / get_library_stats / get_recently_added_albums). Every consumer reaches the active client directly via sync_service._get_active_media_client() or engine.client(name). Engine surface shrinks to client(name) / active_client() / active_server / is_connected() (the one wrapper that has callers — 4 dashboard status sites) / configured_clients() / reload_config(). ~150 lines deleted, 5 dead-method tests removed. (2) Contract Protocol body trimmed to match REQUIRED_METHODS exactly (is_connected, ensure_connection, get_all_artists, get_all_album_ids). The other 5 methods that were declared in the Protocol "required" section weren't actually required — Plex doesn't implement get_recently_added_albums, Jellyfin doesn't implement search_tracks, SoulSync doesn't implement most of them. Static contract now matches runtime conformance test. Optional methods moved to a KNOWN_PER_SERVER_METHODS data-only listing with audited per-server coverage notes — discoverability without false promises. (3) Engine module docstring + __init__.py docstring no longer overclaim "33+ chains collapsed" — only 4 uniform-shape chains were collapsed; ~18 server-specific chains stay explicit per the "lift what's truly shared" standard. Phrasing now matches reality. (4) types.py docstring claimed TrackInfo.from_jellyfin_dict and TrackInfo.from_navidrome_dict exist as classmethods. They don't — only from_plex_track / from_plex_playlist do. Jellyfin and Navidrome construct TrackInfo inline at their call sites today. Docstring now honest about that + flags the lift as a clean followup. (5) Engine line 95 comment "backward-compat for source-specific reaches" was misleading — there is no legacy alternative being preserved; engine.client(name) IS the canonical access pattern. Section header rewritten. Tests: 2121 pass (was 2126; -5 dead-method pin tests).	2 months ago
Broque Thomas	99f527ecd1	MS pre-review polish followup — log levels + docstring honesty ... Three nits I missed in the prior pass: (1) Two more exception swallows still at logger.debug — the get_recently_added_albums wrapper and the configured_clients inner loop. My earlier sed only matched single-line patterns. Both now log at warning level so a broken Plex / Jellyfin surfaces in the boot log instead of silently returning []. (2) registry.py module docstring still claimed it replaced "33 hand-maintained dispatch sites" — same overclaim I fixed on the engine docstring. Server-specific chains stay explicit in web_server.py per the "lift what's truly shared" standard; the registry just owns name → client lookup. Docstring rewritten to match reality.	2 months ago
Broque Thomas	860f9a0a8c	MS pre-review polish — encapsulation + visibility + tests ... Five tightening passes anticipating Cin / JohnBaumb's review nits: (1) Engine no longer reaches into ``registry._instances`` private attr. New public ``MediaServerRegistry.set_instance(name, client)`` method — engine constructor calls it for the ``clients=`` pre-built case so internal storage stays encapsulated. (2) Engine module docstring no longer overclaims. Originally said it "Replaces the historic 33+ if/elif chains" — but only the four uniform-shape ``is_connected`` chains were collapsed. The 19 chains that do server-specific work (Plex raw API vs Jellyfin / Navidrome client methods returning different shapes) stay explicit per the "lift what's truly shared" standard. Docstring rewritten to say exactly that. (3) Per-method exception swallows upgraded from ``logger.debug`` to ``logger.warning``. Returning safe defaults stays the right behavior for a read-side engine (Plex offline shouldn't crash the app), but silent debug-level swallowing made debugging hard — JohnBaumb pushed the download engine to surface real errors. Same treatment here: default still safe, but the warning tells you Plex is down. (4) ``_safe_init_media_client`` in web_server.py now logs the exception type + traceback. Broad ``except Exception`` is still intentional (any failure means that one server can't be used; the others stay up) but the boot log now distinguishes config errors (ConnectionError, AuthenticationError) from import / dependency failures. (5) Two new tests pin the encapsulation + fallback contracts: - ``test_engine_with_empty_clients_dict_is_safe_to_use`` — empty engine returns safe defaults on every method, doesn't raise. - ``test_engine_uses_registry_set_instance_not_private_attr`` — spy on registry.set_instance verifies engine uses the public method.	2 months ago
Broque Thomas	49f7679eef	MS Cin-1 + Cin-2: Explicit contract inheritance + generic accessors ... Apply the Cin-1 / Cin-2 pattern from the download refactor PR to the media server engine PR before review. Cin-1 — explicit inheritance: - PlexClient, JellyfinClient, NavidromeClient, SoulSyncClient now explicitly inherit MediaServerClient instead of relying on structural typing alone. Pre-change a reader of plex_client.py had no way to know the class was supposed to satisfy the contract. - Removed the engine + registry re-exports from core/media_server/__init__.py to break the circular import that the inheritance change introduced (importing the package now triggered a chain that loaded clients before their base class resolved). Submodules import directly: from core.media_server.engine import MediaServerEngine, etc. - Conformance test now also asserts isinstance() / issubclass() against MediaServerClient — drift in any class fails at the test boundary instead of at runtime. Cin-2 — generic accessors + singleton: - engine.configured_clients() — replaces the legacy per-server `if X and X.is_connected(): clients[name] = X` chains in web_server.py. - engine.reload_config(name=None) — generic dispatch, so callers pass the server name instead of reaching for plex_client.reload_config() directly. - get_media_server_engine() / set_media_server_engine() singleton factory matching the get_metadata_engine() / get_download_orchestrator() shape. web_server.py boots via set_media_server_engine(...) so factory + global handle share state. - 7 new tests pin the accessors + singleton behaviour.	2 months ago
Broque Thomas	971d683ebd	C1: Migrate status-check dispatch sites to engine ... Two sites in web_server.py replaced: - /status route's media-server connectivity check (4-way if/elif for plex/jellyfin/navidrome/soulsync) → engine.is_connected() - /api/playlists endpoint's server_connected check (3-way if/elif) → engine.is_connected() Engine reads active_server config + dispatches to the right client with internal connection caching preserved (the underlying clients all cache is_connected() calls). Engine constructor now accepts a pre-built clients={...} dict so web_server.py wires the same instances as its existing per-client globals — no double-init. Suite still green. Per-server clients still accessible via engine.client(name) for source-specific reaches.	2 months ago
Broque Thomas	6b54ca6598	Phase B: Add MediaServerEngine skeleton ... `MediaServerEngine` reads the active server from config + dispatches to the corresponding registered client. Per-server reaches still work through `engine.client(name)`. Required-method dispatch (is_connected, ensure_connection, get_all_artists, get_all_album_ids) returns safe defaults when the active client failed to initialize OR when the method raises. Optional-method dispatch (search_tracks, trigger_library_scan, is_library_scanning, get_library_stats, get_recently_added_albums) checks hasattr first — SoulSync standalone has no trigger_library_scan or get_library_stats, engine no-ops with appropriate defaults instead of forcing every client to declare stub methods. 10 new engine tests pin: active-server resolution, required dispatch routing, exception safety, missing-optional-method fallback shape. Suite still green (1951 passed). Engine isn't on any production code path yet — Phase C migrates the 33 web_server.py dispatch sites to call engine.method() instead of hand-branching by active_server name.	2 months ago