ai: apply changes for #932 (3 review threads)

Addresses:
  - #3858601138 at src/databricks/sql/backend/kernel/auth_bridge.py:468
  - #3858601153 at src/databricks/sql/session.py:193
  - #3858601164 at src/databricks/sql/client.py:234

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

Author: peco-engineer-bot[bot]

src/databricks/sql/backend/kernel/auth_bridge.py

@@ -462,10 +462,14 @@ def kernel_auth_kwargs(
             "oauth_scopes": scopes if scopes is not None else list(PYSQL_OAUTH_SCOPES),
             # OAuth U2M token-cache enable/disable: when present in auth_options,
             # forward to the kernel as token_cache_enabled on the U2M branch.
-            # Default disabled (bool(None) = False) for backward compatibility when
-            # moving token persistence control to the kernel. This ensures callers
-            # must opt-in to on-disk persistence rather than silently enabling it.
-            "token_cache_enabled": bool(opts.get("oauth_token_cache_enabled")),
+            # Default disabled for backward compatibility when moving token
+            # persistence control to the kernel. This ensures callers must
+            # opt-in to on-disk persistence rather than silently enabling it.
+            # Coerced via _coerce_bool so a string DSN/env value like "False"
+            # is not treated as truthy (bool("False") is True).
+            "token_cache_enabled": _coerce_bool(
+                opts.get("oauth_token_cache_enabled")
+            ),
         }
         if federation_client_id:
             kwargs["identity_federation_client_id"] = federation_client_id
@@ -519,6 +523,28 @@ def _coerce_redirect_port(redirect_port: Any) -> int:
         )
 
 
+def _coerce_bool(value: Any) -> bool:
+    """Coerce an opt-in boolean flag (e.g. ``oauth_token_cache_enabled``,
+    which may arrive as a string from a DSN/env) to a ``bool``.
+
+    A plain ``bool(value)`` is wrong for string inputs: ``bool("False")`` is
+    ``True``, which would silently enable on-disk token persistence whenever
+    the flag arrived as the string ``"False"``. Only genuinely truthy values
+    enable the flag: real booleans, and the usual textual/numeric truthy
+    spellings ("true"/"1"/"yes"/"on"). ``None`` (unset) and anything else
+    disable it (opt-in default)."""
+    if isinstance(value, bool):
+        return value
+    if value is None:
+        return False
+    if isinstance(value, str):
+        return value.strip().lower() in ("true", "1", "yes", "on")
+    if isinstance(value, (int, float)):
+        return value != 0
+    # Unknown types default to disabled rather than truthy-by-accident.
+    return False
+
+
 def _normalize_scopes(scopes: Any) -> Optional[list]:
     """Normalise an ``oauth_scopes`` value to a list of strings, or
     ``None`` to let the kernel apply its defaults.

src/databricks/sql/client.py

@@ -228,9 +228,13 @@ def read(self) -> Optional[OAuthToken]:
             :param oauth_token_cache_enabled: `bool | None`, optional (default is None)
                 **Kernel-only, U2M-only.** Controls whether the kernel persists OAuth U2M
                 refresh tokens to disk (AES-256 encrypted, at `~/.config/databricks-sql-kernel/oauth/`).
-                When unset (None, the default), the kernel's own default behavior applies.
-                When True, enables persistent on-disk token cache; when False, tokens are
-                held in memory only and the user must re-authenticate when the process restarts.
+                When unset (None, the default), the connector treats this as False and
+                forwards `token_cache_enabled=False` to the kernel, so on-disk caching is
+                disabled by default — matching the Thrift posture and avoiding silently
+                writing tokens to disk. Callers must opt in explicitly to enable persistence.
+                When True, enables persistent on-disk token cache; when False (or unset),
+                tokens are held in memory only and the user must re-authenticate when the
+                process restarts.
                 Has no effect on Thrift or SEA backends, which maintain their own token
                 lifecycle via `experimental_oauth_persistence`. This parameter is distinct
                 from the Thrift-only `experimental_oauth_persistence` — this controls the

src/databricks/sql/session.py

@@ -188,9 +188,12 @@ def _create_backend(
                 ),
                 # OAuth U2M token-cache enable/disable: controls whether the kernel
                 # persists U2M refresh tokens to disk (encrypted, at ~/.config/databricks-sql-kernel/oauth/).
-                # Omitted ⇒ kernel default (enabled); False ⇒ in-memory only.
+                # Coerced via _coerce_bool on the oauth-u2m branch, so omitted/None
+                # ⇒ token_cache_enabled=False (disabled, in-memory only) — the
+                # opt-in default that preserves backward compat when token
+                # persistence moves to the kernel path; True ⇒ on-disk persistence.
                 # This is forwarded to the kernel's pyo3 Session as token_cache_enabled
-                # on the oauth-u2m auth branch only, ensuring backward compat when moved to the kernel path.
+                # on the oauth-u2m auth branch only.
                 "oauth_token_cache_enabled": kwargs.get("oauth_token_cache_enabled"),
                 # Azure Entra SP credentials for the azure-sp-m2m path. The
                 # kernel owns Azure resolution (endpoint/scope/tenant discovery),

tests/unit/test_kernel_auth_bridge.py

@@ -620,6 +620,35 @@ def test_u2m_token_cache_enabled_true_forwarded(self):
         )
         assert kwargs["token_cache_enabled"] is True
 
+    @pytest.mark.parametrize(
+        "raw_value",
+        ["False", "false", "0", "no", "off", "", "  ", "nope"],
+    )
+    def test_u2m_token_cache_enabled_falsey_string_stays_false(self, raw_value):
+        # A string DSN/env value that reads as falsey (e.g. "False") must NOT
+        # enable on-disk persistence: bool("False") is True, so the flag is
+        # coerced via _coerce_bool rather than bool().
+        kwargs = kernel_auth_kwargs(
+            _FakeOAuthProvider(),
+            {
+                "auth_type": "databricks-oauth",
+                "oauth_token_cache_enabled": raw_value,
+            },
+        )
+        assert kwargs["token_cache_enabled"] is False
+
+    @pytest.mark.parametrize("raw_value", ["True", "true", "1", "yes", "on"])
+    def test_u2m_token_cache_enabled_truthy_string_enables(self, raw_value):
+        # An explicit truthy string DSN/env value enables persistence.
+        kwargs = kernel_auth_kwargs(
+            _FakeOAuthProvider(),
+            {
+                "auth_type": "databricks-oauth",
+                "oauth_token_cache_enabled": raw_value,
+            },
+        )
+        assert kwargs["token_cache_enabled"] is True
+
     @pytest.mark.parametrize("u2m_auth_type", ["databricks-oauth", "azure-oauth"])
     def test_u2m_token_cache_enabled_both_auth_types(self, u2m_auth_type):
         # token_cache_enabled applies to both databricks-oauth and azure-oauth U2M types.