4 Commits (main)

Author	SHA1	Message	Date
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).	4 weeks 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.	4 weeks 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.	4 weeks 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.	4 weeks ago