fix: harden request-path OIDC refresh per PR review (ZD-682)

Address PR #548 review (jamadriz):

1. Sync AI-generation endpoints (generate_qualitative_text,
   generate_test_result_description) called requests.post with
   _get_api_headers() but skipped the refresh hook, so a session hitting
   only those after token expiry kept the old failure. Add
   _ensure_fresh_oidc_token() + the clear 401 error to both. Every
   auth-bearing request path (_get/_post/_ping + both AI-gen) now refreshes.
2. Align refresh-failure with init(): on a failed refresh (e.g. revoked
   token / invalid_grant) drop the cached entry so it isn't re-attempted on
   every request. Recovery is re-running vm.init() (per the clear 401
   error); the request path can't do interactive device flow, so it stops.
3. Add wiring/integration tests asserting _get/_post/_ping invoke the
   refresh hook and that the 401 retry fires end-to-end, plus a
   drop-cache-on-refresh-failure test.
4. Extract shared _refresh_oidc_from_cache() used by both _obtain_oidc_tokens
   (init) and _ensure_fresh_oidc_token (request path) so the refresh +
   cache-persist + drop-on-failure logic can't drift.

Also clear the in-memory token (_access_token/_oidc_expires_at) on hard
refresh failure so the next request fails fast at header build with the
re-auth message instead of sending a doomed request (review nit).

37 tests pass; flake8/black/isort clean.

Author: ValidMind Support Agent

tests/test_api_client.py

@@ -24,7 +24,6 @@
 from validmind.utils import md_to_html
 from validmind.vm_models.figure import Figure
 
-
 loop = asyncio.new_event_loop()
 
 
@@ -392,6 +391,50 @@ def test_log_text_rejects_invalid_context(self):
                 context={"content_ids": ["valid", ""]},
             )
 
+    # -- request-path refresh wiring (ZD-682) -----------------------------
+    # These assert the request wrappers actually invoke the refresh hook and
+    # that the 401 retry fires end-to-end, complementing the unit tests that
+    # exercise _ensure_fresh_oidc_token in isolation.
+
+    @patch("validmind.api_client._ensure_fresh_oidc_token")
+    @patch("aiohttp.ClientSession.get")
+    def test_get_refreshes_before_request_and_retries_on_401(
+        self, mock_get, mock_ensure
+    ):
+        mock_ensure.return_value = True  # forced refresh reports success -> retry
+        mock_get.side_effect = [
+            MockAsyncResponse(401, text="unauthorized"),
+            MockAsyncResponse(200, json={"ok": True}),
+        ]
+        result = self.run_async(api_client._get, "endpoint")
+        self.assertEqual(result, {"ok": True})
+        self.assertEqual(mock_get.call_count, 2)
+        self.assertEqual(mock_ensure.call_count, 2)
+        mock_ensure.assert_any_call(force=True)
+
+    @patch("validmind.api_client._ensure_fresh_oidc_token")
+    @patch("aiohttp.ClientSession.post")
+    def test_post_refreshes_before_request(self, mock_post, mock_ensure):
+        mock_post.return_value = MockAsyncResponse(200, json={"ok": True})
+        result = self.run_async(api_client._post, "endpoint", data={"a": "b"})
+        self.assertEqual(result, {"ok": True})
+        mock_ensure.assert_called_once_with()
+
+    @patch("validmind.api_client._ensure_fresh_oidc_token")
+    @patch("requests.get")
+    def test_ping_refreshes_before_request_and_retries_on_401(
+        self, mock_get, mock_ensure
+    ):
+        mock_ensure.return_value = True
+        mock_get.side_effect = [
+            MockResponse(401, text="unauthorized"),
+            MockResponse(200, json={"model": {"name": "n", "cuid": "c"}}),
+        ]
+        api_client._ping()
+        self.assertEqual(mock_get.call_count, 2)
+        self.assertEqual(mock_ensure.call_count, 2)
+        mock_ensure.assert_any_call(force=True)
+
 
 class TestAPIClientOIDC(unittest.TestCase):
     """OIDC device-flow authentication via vm.init()."""
@@ -820,6 +863,33 @@ def test_locked_caller_adopts_concurrently_refreshed_token(self):
         # Adopted the token another caller already refreshed, no second fetch.
         self.assertEqual(api_client._access_token, "new-tok")
 
+    def test_ensure_fresh_oidc_token_drops_cache_on_refresh_failure(self):
+        # On a failed refresh (e.g. revoked refresh token / invalid_grant), the
+        # cached entry is deleted so it isn't re-attempted on every request —
+        # matching init()'s _obtain_oidc_tokens.
+        self._init_oidc(expires_at="2000-01-01T00:00:00+00:00")
+        expired = {
+            "issuer": "https://issuer.example.com/",
+            "client_id": "cid",
+            "access_token": "old-tok",
+            "expires_at": "2000-01-01T00:00:00+00:00",
+            "refresh_token": "refresh-token",
+        }
+        with (
+            patch("validmind.credentials_store.get_cached_entry", return_value=expired),
+            patch("validmind.credentials_store.delete_cached_entry") as mock_delete,
+            patch(
+                "validmind.oidc_device.try_refresh_cached_tokens",
+                side_effect=ValidMindAuthError("invalid_grant"),
+            ),
+        ):
+            self.assertFalse(api_client._ensure_fresh_oidc_token())
+            mock_delete.assert_called_once()
+        # In-memory token is cleared too, so the next request fails fast at header
+        # build with the re-auth message rather than sending a doomed request.
+        self.assertIsNone(api_client._access_token)
+        self.assertIsNone(api_client._oidc_expires_at)
+
 
 if __name__ == "__main__":
     unittest.main()

validmind/api_client.py

@@ -248,6 +248,36 @@ def _ping() -> Dict[str, Any]:
         )
 
 
+def _refresh_oidc_from_cache(
+    issuer: str,
+    client_id: str,
+    refresh_token: str,
+    scope: Optional[str],
+    audience: Optional[str] = None,
+) -> Optional[Dict[str, Any]]:
+    """Exchange a cached refresh token for new tokens, persisting the result.
+
+    Shared by the ``init()`` path (``_obtain_oidc_tokens``) and the request path
+    (``_ensure_fresh_oidc_token``) so their refresh behaviour can't drift. On
+    success the refreshed entry is cached and returned. On a failed refresh (e.g.
+    revoked token / ``invalid_grant``) the stale entry is deleted and None is
+    returned, so a dead refresh token isn't re-attempted; the caller decides how
+    to recover (``init()`` falls back to device flow, the request path stops).
+    """
+    from .credentials_store import delete_cached_entry, upsert_cached_entry
+    from .oidc_device import try_refresh_cached_tokens
+
+    try:
+        new_tokens = try_refresh_cached_tokens(
+            issuer, client_id, refresh_token, scope, audience=audience
+        )
+    except ValidMindAuthError:
+        delete_cached_entry(issuer, client_id, audience=audience)
+        return None
+    upsert_cached_entry(issuer, client_id, new_tokens, audience=audience)
+    return new_tokens
+
+
 def _obtain_oidc_tokens(
     issuer: str,
     client_id: str,
@@ -256,35 +286,25 @@ def _obtain_oidc_tokens(
 ) -> Dict[str, Any]:
     """Return a credentials entry dict with access_token, expires_at, refresh_token, etc."""
     from .credentials_store import (
-        delete_cached_entry,
         get_cached_entry,
         is_expired,
         normalize_client_id,
         normalize_issuer,
         upsert_cached_entry,
     )
-    from .oidc_device import run_device_flow, try_refresh_cached_tokens
+    from .oidc_device import run_device_flow
 
     norm_issuer = normalize_issuer(issuer)
     norm_client_id = normalize_client_id(client_id)
     cached = get_cached_entry(norm_issuer, norm_client_id, audience=audience)
     if cached and not is_expired(cached):
         return cached
     if cached and cached.get("refresh_token"):
-        try:
-            new_tokens = try_refresh_cached_tokens(
-                norm_issuer,
-                norm_client_id,
-                cached["refresh_token"],
-                scope,
-                audience=audience,
-            )
-            upsert_cached_entry(
-                norm_issuer, norm_client_id, new_tokens, audience=audience
-            )
+        new_tokens = _refresh_oidc_from_cache(
+            norm_issuer, norm_client_id, cached["refresh_token"], scope, audience
+        )
+        if new_tokens is not None:
             return new_tokens
-        except ValidMindAuthError:
-            delete_cached_entry(norm_issuer, norm_client_id, audience=audience)
     tokens = run_device_flow(norm_issuer, norm_client_id, scope, audience=audience)
     upsert_cached_entry(norm_issuer, norm_client_id, tokens, audience=audience)
     return tokens
@@ -312,6 +332,19 @@ def _set_oidc_access_token(entry: Dict[str, Any]) -> None:
     _invalidate_async_session()
 
 
+def _clear_oidc_access_token() -> None:
+    """Drop the in-memory OIDC token and pooled session after an unrecoverable refresh.
+
+    Pairs with deleting the cached entry: with no usable token in memory, the next
+    request fails fast at header build with the clear "run vm.init()" error instead
+    of sending a doomed request just to get a 401 back.
+    """
+    global _access_token, _oidc_expires_at
+    _access_token = None
+    _oidc_expires_at = None
+    _invalidate_async_session()
+
+
 def _oidc_token_is_stale() -> bool:
     """Cheap in-memory expiry check (no file I/O) using the 120s skew in is_expired."""
     from .credentials_store import is_expired
@@ -327,16 +360,16 @@ def _ensure_fresh_oidc_token(force: bool = False) -> bool:
     starts failing on every call until ``init()`` is re-run. This uses the cached
     refresh token (requires ``offline_access``, the default scope) to obtain a new
     access token in place. Returns True when a usable token is in place, False when
-    no refresh was possible (non-OIDC mode, or no cached refresh token).
+    no refresh was possible (non-OIDC mode, no cached refresh token, or a failed
+    refresh — in which case the cached entry is dropped, matching init()).
     """
     if _auth_mode != "oidc" or _oidc_login_context is None:
         return False
     # Hot path: skip file I/O and the lock while the current token is still valid.
     if not force and not _oidc_token_is_stale():
         return True
 
-    from .credentials_store import get_cached_entry, is_expired, upsert_cached_entry
-    from .oidc_device import try_refresh_cached_tokens
+    from .credentials_store import get_cached_entry, is_expired
 
     ctx = _oidc_login_context
     issuer = ctx["issuer"]
@@ -355,13 +388,18 @@ def _ensure_fresh_oidc_token(force: bool = False) -> bool:
         refresh_token = entry.get("refresh_token")
         if not refresh_token:
             return False
-        try:
-            new_tokens = try_refresh_cached_tokens(
-                issuer, client_id, refresh_token, scope, audience=audience
-            )
-        except ValidMindAuthError:
+        # Shared with init(): on failure the cached entry is dropped so a
+        # bad/revoked refresh token isn't re-attempted on every request. Recovery
+        # is re-running vm.init() (surfaced by the clear 401 auth error); unlike
+        # init() the request path can't fall back to interactive device flow.
+        new_tokens = _refresh_oidc_from_cache(
+            issuer, client_id, refresh_token, scope, audience
+        )
+        if new_tokens is None:
+            # Hard failure: the cache entry was dropped; clear the in-memory token
+            # too so the next request fails fast with the clear re-auth message.
+            _clear_oidc_access_token()
             return False
-        upsert_cached_entry(issuer, client_id, new_tokens, audience=audience)
         _set_oidc_access_token(new_tokens)
         return True
 
@@ -757,14 +795,15 @@ def _validate_log_text_context(
 
 def generate_qualitative_text(text_generation_data: Dict[str, Any]) -> Dict[str, Any]:
     """Generate qualitative text using the ValidMind AI API."""
+    _ensure_fresh_oidc_token()
     r = requests.post(
         url=_get_url("ai/generate/qualitative_text_generation"),
         headers=_get_api_headers(),
         json=text_generation_data,
     )
 
     if r.status_code != 200:
-        raise_api_error(r.text)
+        _raise_for_api_error(r.status_code, r.text)
 
     return r.json()
 
@@ -1002,13 +1041,14 @@ def log_metric(
 
 
 def generate_test_result_description(test_result_data: Dict[str, Any]) -> str:
+    _ensure_fresh_oidc_token()
     r = requests.post(
         url=_get_url("ai/generate/test_result_description"),
         headers=_get_api_headers(),
         json=test_result_data,
     )
 
     if r.status_code != 200:
-        raise_api_error(r.text)
+        _raise_for_api_error(r.status_code, r.text)
 
     return r.json()