From 3be9fb73173e477d153d79fcd3e278bb093c350c Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Tue, 2 Jun 2026 17:55:22 -0500 Subject: [PATCH] fix(desktop): address Copilot review on xAI loopback flow - web_server: join the callback-server thread in the start error path so a failed discovery/URL build doesn't leave a daemon thread running - web_server: loopback worker now bails if the session was cancelled while waiting for the callback or exchanging the code, instead of persisting tokens the user no longer wants (+ regression test) - onboarding: fall back to window.open when the desktop bridge's openExternal is unavailable, so the flow never silently stalls --- apps/desktop/src/store/onboarding.ts | 14 ++++++- hermes_cli/web_server.py | 20 +++++++++- tests/hermes_cli/test_web_oauth_dispatch.py | 44 +++++++++++++++++++++ 3 files changed, 76 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src/store/onboarding.ts b/apps/desktop/src/store/onboarding.ts index b1dfa0a13..1e8b2325b 100644 --- a/apps/desktop/src/store/onboarding.ts +++ b/apps/desktop/src/store/onboarding.ts @@ -411,6 +411,18 @@ export async function refreshOnboarding(ctx: OnboardingContext) { return false } +// Open a sign-in URL via the desktop bridge, falling back to window.open +// when the bridge isn't present (e.g. the web dashboard / dev preview) so +// the flow never silently stalls in a waiting state. Mirrors the pattern in +// apps/desktop/src/app/artifacts/index.tsx. +async function openSignInUrl(url: string) { + if (window.hermesDesktop?.openExternal) { + await window.hermesDesktop.openExternal(url) + } else { + window.open(url, '_blank', 'noopener,noreferrer') + } +} + export async function startProviderOAuth(provider: OAuthProvider, ctx: OnboardingContext) { clearPoll() @@ -425,7 +437,7 @@ export async function startProviderOAuth(provider: OAuthProvider, ctx: Onboardin try { const start = await startOAuthLogin(provider.id) const browserUrl = start.flow === 'device_code' ? start.verification_url : start.auth_url - await window.hermesDesktop?.openExternal(browserUrl) + await openSignInUrl(browserUrl) if (start.flow === 'pkce') { setFlow({ status: 'awaiting_user', provider, start, code: '' }) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index bb29783bf..fc740e6c5 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -3540,12 +3540,17 @@ def _start_xai_loopback_flow() -> Dict[str, Any]: ) except Exception: # Binding succeeded but URL construction failed — release the socket - # so we don't leak a listener on the loopback port. + # and join the serving thread so we don't leak a listener (or a + # lingering daemon thread) on the loopback port. try: server.shutdown() server.server_close() except Exception: pass + try: + thread.join(timeout=1.0) + except Exception: + pass raise sid, sess = _new_oauth_session("xai-oauth", "loopback") @@ -3589,6 +3594,14 @@ def _xai_loopback_worker(session_id: str) -> None: s["status"] = "error" s["error_message"] = message + def _cancelled() -> bool: + # The session is removed from the registry when the user cancels + # (DELETE /sessions/{id}). If that happened while we were blocked on + # the callback or token exchange, abort instead of persisting tokens + # the user no longer wants. + with _oauth_sessions_lock: + return session_id not in _oauth_sessions + try: callback = hauth._xai_wait_for_callback( sess["server"], @@ -3600,6 +3613,9 @@ def _xai_loopback_worker(session_id: str) -> None: _fail(f"xAI authorization timed out: {exc}") return + if _cancelled(): + return + if callback.get("error"): detail = callback.get("error_description") or callback["error"] _fail(f"xAI authorization failed: {detail}") @@ -3638,6 +3654,8 @@ def _xai_loopback_worker(session_id: str) -> None: "expires_in": payload.get("expires_in"), "token_type": str(payload.get("token_type") or "Bearer").strip() or "Bearer", } + if _cancelled(): + return hauth._save_xai_oauth_tokens( tokens, discovery=sess.get("discovery"), diff --git a/tests/hermes_cli/test_web_oauth_dispatch.py b/tests/hermes_cli/test_web_oauth_dispatch.py index d014a62aa..9b73cfa00 100644 --- a/tests/hermes_cli/test_web_oauth_dispatch.py +++ b/tests/hermes_cli/test_web_oauth_dispatch.py @@ -489,6 +489,50 @@ def test_xai_loopback_worker_fails_on_state_mismatch(monkeypatch): ws._oauth_sessions.pop(session_id, None) +def test_xai_loopback_worker_skips_persist_when_cancelled(monkeypatch): + """If the session is cancelled while waiting, the worker must not persist.""" + from hermes_cli import auth as auth_mod + from hermes_cli import web_server as ws + + session_id = "xai-loopback-cancel-test" + ws._oauth_sessions[session_id] = { + "session_id": session_id, + "provider": "xai-oauth", + "flow": "loopback", + "created_at": time.time(), + "status": "pending", + "error_message": None, + "server": object(), + "thread": object(), + "callback_result": {}, + "redirect_uri": "http://127.0.0.1:56121/callback", + "verifier": "verifier", + "challenge": "challenge", + "state": "st", + "token_endpoint": "https://auth.x.ai/oauth2/token", + "discovery": {}, + } + + def _wait_then_cancel(*args, **kwargs): + # Simulate the user cancelling (DELETE /sessions/{id}) while we were + # blocked on the callback: the session vanishes, then a valid code + # arrives. The worker must notice and bail before persisting. + ws._oauth_sessions.pop(session_id, None) + return {"code": "auth-code", "state": "st"} + + monkeypatch.setattr(auth_mod, "_xai_wait_for_callback", _wait_then_cancel) + + def _must_not_persist(*args, **kwargs): + raise AssertionError("tokens must not be persisted for a cancelled session") + + monkeypatch.setattr(auth_mod, "_save_xai_oauth_tokens", _must_not_persist) + monkeypatch.setattr(ws, "_add_xai_oauth_pool_entry", _must_not_persist) + + # Should return cleanly without raising and without persisting. + ws._xai_loopback_worker(session_id) + assert session_id not in ws._oauth_sessions + + def test_unknown_pkce_provider_rejected_cleanly(): """A future PKCE provider without an explicit branch must NOT silently route to Anthropic.