feat: store connection passwords in OS keychain via keyring

- Add keyring>=25.0 to runtime dependencies
- ConnectionManager stores passwords in OS keychain (macOS Keychain,
  GNOME Keyring, Windows Credential Manager) instead of plaintext YAML
- save_connection: strips password from connections.yaml, sets _keyring
  flag, falls back to plaintext with warning if keychain unavailable
- get_connection: fetches password from keychain when _keyring flag set
- remove_connection: also deletes the keychain entry on removal
- Add mock_keyring autouse fixture in conftest.py to redirect keyring
  calls to an in-memory dict for all unit tests

Author: Adoit

pyproject.toml

@@ -9,6 +9,7 @@ dependencies = [
     "pyyaml>=6.0",
     "psycopg[binary]>=3.1",
     "pymysql>=1.1",
+    "keyring>=25.0",
 ]
 
 [project.scripts]

src/open_data_agent/db/connection.py

@@ -1,4 +1,9 @@
-"""ConnectionManager — named DB connections stored in ~/.config/open-data-agent/connections.yaml."""
+"""ConnectionManager — named DB connections stored in ~/.config/open-data-agent/connections.yaml.
+
+Passwords are stored in the OS keychain via the `keyring` library (macOS Keychain, GNOME Keyring,
+Windows Credential Manager). On headless environments where no keyring backend is available,
+passwords fall back to plaintext in connections.yaml with a warning.
+"""
 
 from __future__ import annotations
 
@@ -17,6 +22,8 @@
 
 logger = logging.getLogger("open_data_agent.db.connection")
 
+_KEYRING_SERVICE = "open-data-agent"
+
 _CONNECTIONS_FILE = "connections.yaml"
 _ACTIVE_CONNECTION_FILE = "active-connection"
 
@@ -60,6 +67,37 @@ def _save_connections(self, connections: dict[str, dict[str, Any]]) -> None:
         with os.fdopen(fd, "w") as f:
             yaml.dump(connections, f, default_flow_style=False)
 
+    def _keyring_set(self, name: str, password: str) -> bool:
+        """Store password in OS keychain. Returns True on success, False if unavailable."""
+        try:
+            import keyring
+            import keyring.errors
+
+            keyring.set_password(_KEYRING_SERVICE, name, password)
+            return True
+        except Exception as exc:  # noqa: BLE001
+            logger.warning("keyring unavailable — storing password in plaintext: %s", exc)
+            return False
+
+    def _keyring_get(self, name: str) -> str | None:
+        """Retrieve password from OS keychain, or None if unavailable/not found."""
+        try:
+            import keyring
+
+            return keyring.get_password(_KEYRING_SERVICE, name)
+        except Exception:  # noqa: BLE001
+            return None
+
+    def _keyring_delete(self, name: str) -> None:
+        """Delete keychain entry; silently no-ops if not found or unavailable."""
+        try:
+            import keyring
+            import keyring.errors
+
+            keyring.delete_password(_KEYRING_SERVICE, name)
+        except Exception:  # noqa: BLE001
+            pass
+
     def save_connection(self, name: str, params: dict[str, Any]) -> None:
         """Save a named connection. Raises ConfigError on missing/invalid fields."""
         missing = _REQUIRED_FIELDS - set(params.keys())
@@ -72,8 +110,17 @@ def save_connection(self, name: str, params: dict[str, Any]) -> None:
                 f"Must be one of: {', '.join(sorted(_VALID_DB_TYPES))}"
             )
 
+        stored = dict(params)
+        password = str(stored.pop("password", ""))
+
+        # Try keyring first; fall back to plaintext if unavailable.
+        if self._keyring_set(name, password):
+            stored["_keyring"] = True
+        else:
+            stored["password"] = password
+
         connections = self._load_connections()
-        connections[name] = dict(params)
+        connections[name] = stored
         self._save_connections(connections)
         logger.info("Saved connection '%s' (db_type=%s)", name, params["db_type"])
 
@@ -88,19 +135,29 @@ def list_connections(self) -> list[dict[str, Any]]:
         return result
 
     def get_connection(self, name: str) -> dict[str, Any]:
-        """Return the full connection dict (including password). Raises ConfigError if not found."""
+        """Return full connection dict including password. Raises ConfigError if not found."""
         connections = self._load_connections()
         if name not in connections:
             raise ConfigError(f"Connection '{name}' not found in connections.yaml")
-        return dict(connections[name])
+        entry = dict(connections[name])
+        if entry.pop("_keyring", False):
+            password = self._keyring_get(name)
+            if password is None:
+                raise ConfigError(
+                    f"Password for '{name}' not found in keychain. "
+                    "Re-add the connection with 'oda connections add'."
+                )
+            entry["password"] = password
+        return entry
 
     def remove_connection(self, name: str) -> None:
-        """Remove a named connection. Raises ConfigError if not found."""
+        """Remove a named connection and its keychain entry. Raises ConfigError if not found."""
         connections = self._load_connections()
         if name not in connections:
             raise ConfigError(f"Connection '{name}' not found")
         del connections[name]
         self._save_connections(connections)
+        self._keyring_delete(name)
         logger.info("Removed connection '%s'", name)
 
     def set_active_connection(self, name: str) -> None:

tests/conftest.py

@@ -1,19 +1,62 @@
-"""Shared pytest fixtures for all tests.
-
-Key fixtures:
-    sqlite_db: session-scoped in-memory SQLite connection with standard fixture schema.
-"""
+"""Shared pytest fixtures for all tests."""
 
 from __future__ import annotations
 
 import sqlite3
 from collections.abc import Generator
 from pathlib import Path
+from unittest.mock import patch
 
 import pytest
 
 _FIXTURES_DIR = Path(__file__).parent / "fixtures"
 
+# ---------------------------------------------------------------------------
+# Keyring: replace OS keychain with an in-memory dict for all unit tests.
+# Without this, tests would either require a real keychain or fail on CI.
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture(autouse=True)
+def mock_keyring() -> Generator[dict[str, str], None, None]:
+    """Autouse fixture: redirect keyring to an in-memory store for all tests."""
+    store: dict[str, str] = {}
+
+    def _set(name: str, password: str) -> bool:
+        store[name] = password
+        return True
+
+    def _get(name: str) -> str | None:
+        return store.get(name)
+
+    def _delete(name: str) -> None:
+        store.pop(name, None)
+
+    with (
+        patch.object(
+            __import__(
+                "open_data_agent.db.connection", fromlist=["ConnectionManager"]
+            ).ConnectionManager,
+            "_keyring_set",
+            lambda self, n, p: _set(n, p),
+        ),
+        patch.object(
+            __import__(
+                "open_data_agent.db.connection", fromlist=["ConnectionManager"]
+            ).ConnectionManager,
+            "_keyring_get",
+            lambda self, n: _get(n),
+        ),
+        patch.object(
+            __import__(
+                "open_data_agent.db.connection", fromlist=["ConnectionManager"]
+            ).ConnectionManager,
+            "_keyring_delete",
+            lambda self, n: _delete(n),
+        ),
+    ):
+        yield store
+
 
 @pytest.fixture(scope="session")
 def sqlite_db() -> Generator[sqlite3.Connection, None, None]: