fix(auth): narrow keyring exception handler + log root cause

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### Fixed
 
 - **publish.yml: bump pinned `mcp-publisher` v1.5.0 → v1.7.6 to match the new registry OIDC audience** (#79) — the v0.5.1 release ran with `mcp-publisher v1.5.0` (the pin in `.github/actions/install-mcp-publisher/action.yml`); PyPI publish succeeded but the `publish-registry` job failed at GitHub OIDC login with `invalid audience: expected https://registry.modelcontextprotocol.io, got [mcp-registry]` (HTTP 401). Root cause: the registry deployed [`modelcontextprotocol/registry#1229`](https://github.com/modelcontextprotocol/registry/pull/1229) ("auth: bind GitHub OIDC token exchange to a per-deployment audience") in `v1.7.6` on 2026-04-30 — one day before our 2026-05-01 release. v1.5.0's `login github-oidc` flow sends audience `mcp-registry`; v1.7.6's flow sends audience `https://registry.modelcontextprotocol.io`, which is what the new registry server validates against. Bumped the action's `default` from `v1.5.0` to `v1.7.6` (and added an explanatory comment so the next bump prompt has the rationale at hand). Re-running the failed `publish-registry` job on the existing v0.5.1 tag won't pick up this fix because `actions/checkout@v6` resolves to the tag's commit; the next release tag will exercise the fix end-to-end. v0.5.1 itself is on PyPI as expected and is the install path users actually hit; the missed registry entry is purely directory metadata.
+- **Keyring exception handler narrows + logs root cause** (#80) — closes #38. `core/auth.py:147-148` previously caught a bare `except Exception:` with a flat `logger.debug("Keyring not available.")`, hiding every keyring failure mode behind one generic line: locked macOS keychain (operator-actionable: unlock the keychain), `keyring.errors.NoKeyringError` on a headless host (signals a config issue), `keyring.errors.InitError`, OS-level errors on the D-Bus reach path, and genuine library bugs. Operators running `mcp-synology check -v` saw "Keyring not available." with no clue whether the keychain was locked, the backend was missing, or the library blew up. Narrowed to two typed handlers: `except KeyringError as e` (the typed-error case — covers `KeyringLocked`, `NoKeyringError`, `InitError`, `PasswordSetError`, `PasswordDeleteError`, and any other `keyring.errors.*` class) and `except OSError as e` (D-Bus reach errors, permission failures on the OS keychain DB). Both log at DEBUG with `exc_info=True` so the actual exception type, message, and traceback land in the verbose-mode output. Genuine bugs are no longer caught — they propagate up so they can be triaged. The pre-keyring D-Bus socket pre-check (`auth.py:130-131`) was bumped from DEBUG to INFO and rewritten with three concrete remediations (`mcp-synology setup` from a real desktop session, `dbus-run-session` wrapper, or `SYNOLOGY_USERNAME` / `SYNOLOGY_PASSWORD` / `SYNOLOGY_DEVICE_ID` env vars to bypass keyring entirely) — this is the most common path on Linux services launched without a desktop session, and the previous DEBUG-only log meant operators running `mcp-synology check` (without `-v`) saw a generic "no credentials" error with no breadcrumb to the root cause. Updated `tests/core/test_auth.py::_no_keyring()` fixture to raise `keyring.errors.NoKeyringError` instead of bare `Exception` so existing tests exercise the production-shaped error path. New `TestKeyringErrorHandling` (3 cases): `KeyringError` logged with `exc_info` and message text at DEBUG, `OSError` logged separately, and a defense-in-depth case proving a keyring blow-up doesn't block credential resolution from config/env. Strengthened `TestDbusSocketMissing::test_dbus_not_set_when_socket_missing_on_linux` to assert the new INFO-level remediation hint contains the socket path AND all three remediation strings. 553 unit tests pass at 96.13% coverage.
 
 ## 0.5.1 (2026-05-01)
 

src/mcp_synology/core/auth.py

@@ -14,6 +14,7 @@
 from typing import TYPE_CHECKING, Any
 
 import keyring as kr
+from keyring.errors import KeyringError
 
 from mcp_synology.core.errors import AuthenticationError, SynologyError
 
@@ -127,7 +128,17 @@ def _resolve_credentials(self) -> tuple[str, str, str | None]:
                     os.environ["DBUS_SESSION_BUS_ADDRESS"] = dbus_addr
                     logger.debug("Set DBUS_SESSION_BUS_ADDRESS=%s for keyring access", dbus_addr)
                 else:
-                    logger.debug("D-Bus socket not found at %s; keyring may not work", socket_path)
+                    # INFO not DEBUG — without D-Bus the keyring path is dead
+                    # for this session; the operator needs to know how to
+                    # recover. Closes #38 (the operator-actionable hint).
+                    logger.info(
+                        "D-Bus socket not found at %s; OS keyring is unavailable. "
+                        "Run 'mcp-synology setup' from a real desktop session "
+                        "(or under 'dbus-run-session'), or set credentials via the "
+                        "SYNOLOGY_USERNAME / SYNOLOGY_PASSWORD / SYNOLOGY_DEVICE_ID "
+                        "env vars to bypass the keyring entirely.",
+                        socket_path,
+                    )
 
             try:
                 service = f"mcp-synology/{self._config.instance_id or 'default'}"
@@ -144,8 +155,25 @@ def _resolve_credentials(self) -> tuple[str, str, str | None]:
                 if kr_device and not device_id:
                     device_id = kr_device
                     logger.debug("Device ID from keyring")
-            except Exception:
-                logger.debug("Keyring not available.")
+            except KeyringError as e:
+                # Typed keyring failure (NoKeyringError, InitError,
+                # KeyringLocked, PasswordSetError, etc.). Log at DEBUG with
+                # `exc_info` so `mcp-synology check -v` surfaces the actual
+                # cause (locked keychain on macOS is operator-actionable;
+                # NoKeyringError on a headless host signals a config issue).
+                # Closes #38 — previously caught by a bare `except Exception`
+                # with a flat "Keyring not available." line that hid all of
+                # these. Falls through to the no-credentials check below;
+                # the resolver chain still works without keyring.
+                logger.debug("Keyring access failed: %s", e, exc_info=True)
+            except OSError as e:
+                # D-Bus socket present but unreachable, permission errors on
+                # the OS keychain backend, etc. Same DEBUG-with-exc_info
+                # treatment as the typed-error case above. The pre-check at
+                # line 123-131 already emitted an INFO hint if the D-Bus
+                # socket was missing entirely; this branch covers the
+                # reach-but-fail variant.
+                logger.debug("Keyring OS-level error: %s", e, exc_info=True)
 
         if not username or not password:
             msg = (

tests/core/test_auth.py

@@ -39,9 +39,19 @@ def _make_client() -> DsmClient:
 
 
 def _no_keyring() -> MagicMock:
-    """Return a mock keyring module where get_password raises."""
+    """Return a mock keyring module where get_password raises a realistic
+    `keyring.errors.NoKeyringError`.
+
+    Pre-#38 this used bare `Exception("No keyring backend")` and the
+    production code's bare `except Exception` swallowed it. After #38 the
+    handler narrowed to `KeyringError`/`OSError`, so the realistic typed
+    error is also what the tests should mock — keeps the production-shaped
+    error path exercised everywhere this fixture is used.
+    """
+    from keyring.errors import NoKeyringError
+
     mock = MagicMock()
-    mock.get_password.side_effect = Exception("No keyring backend")
+    mock.get_password.side_effect = NoKeyringError("No keyring backend")
     return mock
 
 
@@ -609,8 +619,22 @@ def test_session_name_format(self) -> None:
 
 
 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."""
+    def test_dbus_not_set_when_socket_missing_on_linux(
+        self, caplog: pytest.LogCaptureFixture
+    ) -> None:
+        """Linux + DBUS unset + socket missing → log INFO with remediation hint
+        and do not set env var.
+
+        Closes #38 (the operator-actionable hint half). Pre-fix this branch
+        only logged at DEBUG ("D-Bus socket not found at %s; keyring may not
+        work"), so an operator running `mcp-synology check` (without -v) saw
+        a generic "no credentials" error with no clue that the missing D-Bus
+        socket was the root cause. INFO-level message names the socket path
+        AND points at three concrete remediations: run setup from a real
+        session, wrap with `dbus-run-session`, or use SYNOLOGY_* env vars.
+        """
+        import logging
+
         config = _make_config(auth={"username": "admin", "password": "secret"})
         client = _make_client()
         auth = AuthManager(config, client)
@@ -624,14 +648,136 @@ def test_dbus_not_set_when_socket_missing_on_linux(self) -> None:
             patch("sys.platform", "linux"),
             patch("pathlib.Path.exists", return_value=False),
             patch("os.getuid", return_value=1000),
+            caplog.at_level(logging.INFO, logger="mcp_synology.core.auth"),
         ):
             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"
+        # INFO record carries actionable remediation, not just a description.
+        info_messages = [r.getMessage() for r in caplog.records if r.levelname == "INFO"]
+        assert any(
+            "D-Bus socket not found" in msg
+            and "/run/user/1000/bus" in msg
+            and "mcp-synology setup" in msg
+            and "SYNOLOGY_USERNAME" in msg
+            for msg in info_messages
+        ), f"expected actionable INFO hint, got: {info_messages}"
+
+
+class TestKeyringErrorHandling:
+    """Closes #38 — narrow keyring exception handler + log root cause.
+
+    Pre-fix `core/auth.py:147-148` caught bare `except Exception` with a flat
+    `logger.debug("Keyring not available.")`, hiding the actual failure
+    (locked macOS keychain, NoKeyringError on a headless host, OSError on
+    D-Bus socket reach issues, library bugs). Now narrows to
+    `keyring.errors.KeyringError` + `OSError` and logs each at DEBUG with
+    `exc_info=True` so `mcp-synology check -v` surfaces the real cause.
+    """
+
+    def _make_keyring_that_raises(self, exc: Exception) -> MagicMock:
+        mock = MagicMock()
+        mock.get_password.side_effect = exc
+        return mock
+
+    def test_keyring_error_logged_with_exc_info_at_debug(
+        self, caplog: pytest.LogCaptureFixture
+    ) -> None:
+        """Typed `KeyringError` (e.g. macOS locked keychain) is logged with
+        the exception message and traceback at DEBUG, NOT swallowed.
+        """
+        import logging
+
+        from keyring.errors import KeyringLocked
+
+        config = _make_config(auth={"username": "admin", "password": "secret"})
+        client = _make_client()
+        auth = AuthManager(config, client)
+
+        with (
+            patch.dict(os.environ, _clean_env(), clear=True),
+            patch(
+                "mcp_synology.core.auth.kr",
+                self._make_keyring_that_raises(KeyringLocked("Keychain is locked")),
+            ),
+            caplog.at_level(logging.DEBUG, logger="mcp_synology.core.auth"),
+        ):
+            auth._resolve_credentials()
+
+        debug_records = [r for r in caplog.records if r.levelname == "DEBUG"]
+        # Failure log carries the exception text and exc_info traceback.
+        matching = [
+            r
+            for r in debug_records
+            if "Keyring access failed" in r.getMessage() and "Keychain is locked" in r.getMessage()
+        ]
+        assert matching, (
+            f"expected DEBUG log with KeyringError message, got: "
+            f"{[r.getMessage() for r in debug_records]}"
+        )
+        assert matching[0].exc_info is not None, (
+            "expected exc_info traceback on the keyring failure log"
+        )
+        # The pre-fix flat "Keyring not available." string must NOT appear.
+        assert not any(r.getMessage() == "Keyring not available." for r in caplog.records), (
+            "regression: legacy flat 'Keyring not available.' message reappeared"
+        )
+
+    def test_keyring_oserror_logged_with_exc_info_at_debug(
+        self, caplog: pytest.LogCaptureFixture
+    ) -> None:
+        """OS-level keyring failure (D-Bus reach errors, permission denied
+        on macOS keychain DB, etc.) is logged with the exception message
+        and traceback at DEBUG, separate from the typed-error branch.
+        """
+        import logging
+
+        config = _make_config(auth={"username": "admin", "password": "secret"})
+        client = _make_client()
+        auth = AuthManager(config, client)
+
+        with (
+            patch.dict(os.environ, _clean_env(), clear=True),
+            patch(
+                "mcp_synology.core.auth.kr",
+                self._make_keyring_that_raises(OSError("Connection refused: /run/user/1000/bus")),
+            ),
+            caplog.at_level(logging.DEBUG, logger="mcp_synology.core.auth"),
+        ):
+            auth._resolve_credentials()
+
+        debug_records = [r for r in caplog.records if r.levelname == "DEBUG"]
+        matching = [
+            r
+            for r in debug_records
+            if "Keyring OS-level error" in r.getMessage() and "Connection refused" in r.getMessage()
+        ]
+        assert matching, (
+            f"expected DEBUG log with OSError message, got: "
+            f"{[r.getMessage() for r in debug_records]}"
+        )
+        assert matching[0].exc_info is not None
+
+    def test_keyring_failure_does_not_block_config_credentials(self) -> None:
+        """A keyring blow-up must not prevent the resolver from returning
+        credentials sourced from the config file (or env). Defense in depth.
+        """
+        from keyring.errors import NoKeyringError
+
+        config = _make_config(auth={"username": "admin", "password": "secret"})
+        client = _make_client()
+        auth = AuthManager(config, client)
+
+        with (
+            patch.dict(os.environ, _clean_env(), clear=True),
+            patch(
+                "mcp_synology.core.auth.kr",
+                self._make_keyring_that_raises(NoKeyringError("No backend")),
+            ),
+        ):
+            username, password, _ = auth._resolve_credentials()
+        assert username == "admin"
+        assert password == "secret"
 
 
 class TestLoginErrorPaths: