Phase 3 + Phase 4 of #14: server.py + auth.py + 95% floor enforcement (93% → 96%)

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Added
 
+- **Test coverage Phase 3 + Phase 4 of #14** (#19) — closes #14. Total coverage 93% → 96%, with Phase 4's `--cov-fail-under=95` guardrail enforced in `pyproject.toml` so future regressions fail CI. Three more files at 100% (`server.py` 57% → 99% — one defensive `if self._client is None` branch unreachable; `core/auth.py` 90% → 100%; `modules/__init__.py` 96% → 100%; `core/formatting.py` 97% → 99%). Test count 457 → 487 (+30 cases). New `TestSharedClientManagerLifecycle` (15 cases) directly tests the lazy `get_client` init, `with_update_notice` clearing logic, signal handler installation including SIGTERM closure invocation, `_cleanup_session` with both running-loop and no-loop paths, and `_bg_update_check` with newer-version, no-update, and error-swallowing scenarios. New `TestPlatformLabel`, `TestCreateServerInstructionPaths` cover the `_platform_label` Darwin/Linux/Windows branches and the `instructions_file` / `custom_instructions` template paths. New `TestDbusSocketMissing`, `TestLoginErrorPaths`, `TestLogout` close the remaining gaps in `core/auth.py` (D-Bus socket-not-found branch, non-2FA SynologyError propagation, "no sid" AuthenticationError, and the three logout paths). No production code touched.
 - **Test coverage Phase 2 of #14** (#17) — total coverage 85% → 93%. `cli/version.py` 27% → 100% and `cli/setup.py` 63% → 100%, the two largest gaps remaining after Phase 1. Test count 392 → 457 (+65 cases) across two new test files: `tests/core/test_cli_version.py` (40 cases covering `_get_current_version`/`_get_latest_pypi_version`/`_version_tuple`/`_detect_installer`/`_load_global_state`/`_save_global_state`/`_check_for_update`/`_do_auto_upgrade`/`_do_revert`, with `urlopen` and `subprocess.run` mocked at the boundary), and `tests/core/test_cli_setup.py` (25 cases covering the async helpers `_attempt_login`/`_connect_and_login`/`_setup_login` including the 2FA bootstrap path with device-token storage, plus `_setup_credential_flow` error paths, the `setup` command's discovered-config valid-and-invalid branches, the `_setup_interactive` validation-failure exit, and the `_emit_claude_desktop_snippet` Linux DBUS fallback). No production code touched.
 - **Test coverage Phase 1 of #14** (#16) — total coverage 81% → 85%. Five files brought to 100%: `cli/check.py` (51%), `cli/main.py` (56%), `cli/logging_.py` (78%), `modules/system/__init__.py` (23%), `modules/filestation/__init__.py` (70%). Test count 336 → 392 (+56 cases). New test classes in `tests/core/test_cli.py` cover the `_check_login` async path, every top-level option in the `main` group (`--check-update`, `--auto-upgrade`, `--revert`, version-change tracking, auto-upgrade trigger), and the early/configured logging setup. Two new test files (`tests/modules/{system,filestation}/test_register.py`) exercise module registration closure bodies via `server._tool_manager._tools[name].fn` extraction with sentinel `AsyncMock` return values, walking the tool body lines that the prior `assert server is not None` style left uncovered. No production code touched.
 - **`CLAUDE.md` documents the per-PR CHANGELOG convention** (#16) — adds an "Adding a CHANGELOG entry on every PR" section under "Common Tasks" specifying that every PR updates `## Unreleased` in `CHANGELOG.md` using strict Keep a Changelog categories (`### Added`, `### Changed`, `### Fixed`). Updates the "Bumping the version for a release" steps to rename `## Unreleased` to `## <version> (<date>)` and add a fresh empty `## Unreleased` section, plus notes that the `publish.yml` `github-release` awk extractor (`## <version>( |\()`) walks past `## Unreleased` harmlessly during tag-push releases.

pyproject.toml

@@ -96,4 +96,4 @@ markers = [
     "integration: requires a real Synology NAS (not run in CI)",
     "vdsm: requires virtual-dsm Docker container with KVM (not run in CI by default)",
 ]
-addopts = "-m 'not integration and not vdsm'"
+addopts = "-m 'not integration and not vdsm' --cov=mcp_synology --cov-fail-under=95"

tests/core/test_auth.py

@@ -400,3 +400,102 @@ def test_session_name_format(self) -> None:
         assert auth._session_name.startswith("MCPSynology_test-nas_")
         uuid_part = auth._session_name.split("_")[-1]
         assert len(uuid_part) == 8
+
+
+class TestDbusSocketMissing:
+    def test_dbus_not_set_when_socket_missing_on_linux(self) -> None:
+        """Linux + DBUS unset + socket missing → log debug, do not set env var."""
+        config = _make_config(auth={"username": "admin", "password": "secret"})
+        client = _make_client()
+        auth = AuthManager(config, client)
+
+        clean = _clean_env()
+        clean.pop("DBUS_SESSION_BUS_ADDRESS", None)
+
+        with (
+            patch.dict(os.environ, clean, clear=True),
+            patch("mcp_synology.core.auth.kr", _no_keyring()),
+            patch("sys.platform", "linux"),
+            patch("pathlib.Path.exists", return_value=False),
+            patch("os.getuid", return_value=1000),
+        ):
+            username, _, _ = auth._resolve_credentials()
+            # Falls back to config-file credentials; the missing-socket branch
+            # logs at debug and does NOT set the env var (assert inside the
+            # patch.dict block so the assertion sees the patched env, not the
+            # restored one)
+            assert "DBUS_SESSION_BUS_ADDRESS" not in os.environ
+        assert username == "admin"
+
+
+class TestLoginErrorPaths:
+    @respx.mock
+    async def test_login_non_2fa_synology_error_propagates(self) -> None:
+        """A non-403 SynologyError on login is re-raised, not wrapped."""
+        from mcp_synology.core.errors import SynologyError
+
+        respx.get(f"{BASE_URL}/webapi/entry.cgi").respond(
+            json={"success": False, "error": {"code": 400}}
+        )
+
+        config = _make_config(auth={"username": "admin", "password": "secret"})
+        async with _make_client() as client:
+            auth = AuthManager(config, client)
+            with (
+                patch.dict(os.environ, _clean_env(), clear=True),
+                patch("mcp_synology.core.auth.kr", _no_keyring()),
+                pytest.raises(SynologyError),
+            ):
+                await auth.login()
+
+    @respx.mock
+    async def test_login_succeeds_but_no_sid_raises(self) -> None:
+        """DSM returns success=True but no sid in data → AuthenticationError."""
+        respx.get(f"{BASE_URL}/webapi/entry.cgi").respond(
+            json={"success": True, "data": {}}  # no sid
+        )
+
+        config = _make_config(auth={"username": "admin", "password": "secret"})
+        async with _make_client() as client:
+            auth = AuthManager(config, client)
+            with (
+                patch.dict(os.environ, _clean_env(), clear=True),
+                patch("mcp_synology.core.auth.kr", _no_keyring()),
+                pytest.raises(AuthenticationError, match="no session ID"),
+            ):
+                await auth.login()
+
+
+class TestLogout:
+    async def test_logout_no_sid_is_noop(self) -> None:
+        config = _make_config(auth={"username": "admin", "password": "secret"})
+        async with _make_client() as client:
+            auth = AuthManager(config, client)
+            client.sid = None
+            # Should not raise; should not call request
+            await auth.logout()
+
+    @respx.mock
+    async def test_logout_success_clears_sid(self) -> None:
+        respx.get(f"{BASE_URL}/webapi/entry.cgi").respond(json={"success": True})
+
+        config = _make_config(auth={"username": "admin", "password": "secret"})
+        async with _make_client() as client:
+            auth = AuthManager(config, client)
+            client.sid = "active-sid"
+            await auth.logout()
+            assert client.sid is None
+
+    @respx.mock
+    async def test_logout_synology_error_still_clears_sid(self) -> None:
+        """Logout failure (e.g., already-expired session) is swallowed; sid cleared."""
+        respx.get(f"{BASE_URL}/webapi/entry.cgi").respond(
+            json={"success": False, "error": {"code": 105}}
+        )
+
+        config = _make_config(auth={"username": "admin", "password": "secret"})
+        async with _make_client() as client:
+            auth = AuthManager(config, client)
+            client.sid = "expired-sid"
+            await auth.logout()  # must not raise
+            assert client.sid is None

tests/core/test_formatting.py

@@ -72,6 +72,14 @@ def test_empty_pairs(self) -> None:
         result = format_key_value([])
         assert "No data" in result
 
+    def test_empty_pairs_with_title(self) -> None:
+        """Empty pairs + title → title appears with underline before "No data"."""
+        result = format_key_value([], title="Stats")
+        lines = result.split("\n")
+        assert lines[0] == "Stats"
+        assert lines[1] == "=====" == "=" * len("Stats")
+        assert "No data" in lines[2]
+
 
 class TestFormatStatus:
     def test_success(self) -> None: