From b5e205160d48febe08813ed4ba4393194c8887f0 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Fri, 31 Jul 2026 00:00:43 -0700 Subject: [PATCH] [eric] settings: disconnect a subscription provider from the UI, verified against 9router not assumed --- backend/apps/agents/agents.py | 48 +------ .../apps/agents/disconnect_subscription.py | 111 +++++++++++++++ .../apps/nine_router/subscription_health.py | 8 ++ backend/tests/test_subscription_disconnect.py | 127 ++++++++++++++++++ .../subscription/SubscriptionCard.tsx | 66 ++++++--- .../subscription/SubscriptionCards.tsx | 54 ++++++-- 6 files changed, 344 insertions(+), 70 deletions(-) create mode 100644 backend/apps/agents/disconnect_subscription.py create mode 100644 backend/tests/test_subscription_disconnect.py diff --git a/backend/apps/agents/agents.py b/backend/apps/agents/agents.py index 8c40003f..da9604c2 100644 --- a/backend/apps/agents/agents.py +++ b/backend/apps/agents/agents.py @@ -438,10 +438,11 @@ async def subscriptions_connect(body: dict): raise HTTPException(status_code=503, detail="9Router not available. Please install Node.js.") # Reconnecting gemini-cli must wipe antigravity; registry prefers AG and a stale AG token would 400 after gemini-cli refreshes. - cascade = P_PROVIDER_CASCADE_REMOVES.get(provider, []) + from backend.apps.agents.disconnect_subscription import PROVIDER_CASCADE_REMOVES, delete_provider_connections + cascade = PROVIDER_CASCADE_REMOVES.get(provider, []) if cascade: try: - await p_delete_provider_connections(cascade) + await delete_provider_connections(cascade) except Exception: pass @@ -892,47 +893,12 @@ async def list_models(): return {"models": result, "notes": notes} -# gemini-cli and antigravity are two Google OAuth lanes; registry prefers AG, so we cascade-wipe AG when reconnecting gemini-cli to avoid stale-AG 400s. One-directional: AG operations MUST NOT cascade back. -P_PROVIDER_CASCADE_REMOVES: dict[str, list[str]] = { - "gemini-cli": ["antigravity"], -} - - -async def p_delete_provider_connections(providers: list[str]) -> int: - """Delete 9Router connections in `providers`; returns count removed, silent on 9Router unreachable.""" - import httpx - from backend.apps.nine_router import NINE_ROUTER_API, get_providers - try: - connections = await get_providers() - except Exception: - return 0 - targets = [c for c in connections if c.get("provider") in providers and c.get("id")] - removed = 0 - async with httpx.AsyncClient(timeout=10.0) as client: - for c in targets: - try: - await client.delete(f"{NINE_ROUTER_API}/providers/{c['id']}") - removed += 1 - except Exception: - pass - return removed - - @agents.router.post("/subscriptions/disconnect") async def subscriptions_disconnect(body: dict): - """Disconnect a subscription provider via 9Router; cascades-wipe Google's paired lanes.""" + """Disconnect a subscription provider's 9Router lane; reports ok only once the lane is verifiably gone.""" + from backend.apps.agents.disconnect_subscription import disconnect_subscription provider = body.get("provider", "") if not provider: raise HTTPException(status_code=400, detail="provider required") - - try: - to_remove = [provider, *P_PROVIDER_CASCADE_REMOVES.get(provider, [])] - removed = await p_delete_provider_connections(to_remove) - if removed: - from backend.apps.service.client import sync as p_sync - from backend.apps.settings.settings import load_settings - p_sync(load_settings().model_dump()) - return {"ok": True} - return {"ok": False, "error": "Connection not found"} - except Exception as e: - raise HTTPException(status_code=500, detail=str(e)) + result = await disconnect_subscription(provider) + return result.model_dump() diff --git a/backend/apps/agents/disconnect_subscription.py b/backend/apps/agents/disconnect_subscription.py new file mode 100644 index 00000000..91ab3e92 --- /dev/null +++ b/backend/apps/agents/disconnect_subscription.py @@ -0,0 +1,111 @@ +"""Drop a subscription provider's 9Router lane and report only what actually happened. + +Deleting is the easy half; honesty is the hard half. 9Router answers a bad id with a 404 and a JSON +error body, so a caller that ignores status codes reports "disconnected" for a lane that is still +live, and the user reconnects on top of a stale row. So success here is verified, never inferred: +`ok` means the provider has no connection row left, confirmed by re-reading 9Router. +""" + +import logging +from typing import Dict, List, Optional + +import httpx +from pydantic import BaseModel, ConfigDict +from typeguard import typechecked + +from backend.apps.nine_router.process import NINE_ROUTER_API, cli_auth_headers, is_running +from backend.apps.nine_router.subscription_health import invalidate_health_cache + +logger = logging.getLogger(__name__) + +# gemini-cli and antigravity are two Google OAuth lanes; the registry prefers AG, so dropping gemini-cli must drop the stale AG row too or it 400s. One-directional: AG operations MUST NOT cascade back. +PROVIDER_CASCADE_REMOVES: Dict[str, List[str]] = { + "gemini-cli": ["antigravity"], +} + +P_TIMEOUT_S = 10.0 + + +class SubscriptionDisconnectResult(BaseModel): + """What the user may be told: `ok` only once the lane is verifiably empty.""" + + model_config = ConfigDict(validate_assignment=True) + ok: bool + removed: int = 0 + error: str = "" + + +@typechecked +def sync_settings_state() -> None: + """Push the settings snapshot to the cloud state sync, exactly as connecting does. Imported late: service.client reaches back into this package.""" + from backend.apps.service.client import sync + from backend.apps.settings.settings import load_settings + sync(load_settings().model_dump()) + + +@typechecked +async def p_list_connections() -> Optional[List[Dict]]: + """Every 9Router connection row, or None when the router can't be read. + + Deliberately not `nine_router.get_providers`, which folds an unreachable router into an empty + list: here "I couldn't look" and "nothing is connected" must not be the same answer. + """ + try: + async with httpx.AsyncClient(timeout=P_TIMEOUT_S, headers=cli_auth_headers()) as client: + r = await client.get(f"{NINE_ROUTER_API}/providers") + if r.status_code != 200: + return None + data = r.json() + if not isinstance(data, dict): + return None + return data.get("connections") or [] + except Exception as e: + logger.debug(f"9Router provider list failed: {e}") + return None + + +@typechecked +async def delete_provider_connections(providers: List[str]) -> int: + """Delete every 9Router connection belonging to `providers`; returns how many really went.""" + connections = await p_list_connections() + if connections is None: + return 0 + removed = 0 + async with httpx.AsyncClient(timeout=P_TIMEOUT_S, headers=cli_auth_headers()) as client: + for c in connections: + if c.get("provider") not in providers or not c.get("id"): + continue + try: + r = await client.delete(f"{NINE_ROUTER_API}/providers/{c['id']}") + if r.status_code < 400: + removed += 1 + else: + logger.warning(f"9Router refused to drop the {c.get('provider')} connection: HTTP {r.status_code}") + except Exception as e: + logger.warning(f"9Router delete failed for {c.get('provider')}: {e}") + return removed + + +@typechecked +async def disconnect_subscription(provider: str) -> SubscriptionDisconnectResult: + """Clear `provider` (and its cascade lanes) from 9Router, confirming the lane is gone.""" + if not is_running(): + return SubscriptionDisconnectResult(ok=False, error="The subscription service isn't running.") + + targets = [provider, *PROVIDER_CASCADE_REMOVES.get(provider, [])] + removed = await delete_provider_connections(targets) + + remaining = await p_list_connections() + if remaining is None: + return SubscriptionDisconnectResult( + ok=False, removed=removed, error="Couldn't confirm the disconnect. Please try again.", + ) + if any(c.get("provider") in targets for c in remaining): + return SubscriptionDisconnectResult( + ok=False, removed=removed, error="The subscription service kept the connection. Please try again.", + ) + + sync_settings_state() + # A deliberate disconnect makes the cached boot verdict a lie: it would nag "reconnect" about the lane the user just dropped. + invalidate_health_cache() + return SubscriptionDisconnectResult(ok=True, removed=removed) diff --git a/backend/apps/nine_router/subscription_health.py b/backend/apps/nine_router/subscription_health.py index a805b7cf..ac01ef86 100644 --- a/backend/apps/nine_router/subscription_health.py +++ b/backend/apps/nine_router/subscription_health.py @@ -43,6 +43,14 @@ def health_probe_enabled() -> bool: return os.environ.get("OPENSWARM_BOOT_HEALTH", "1") != "0" +@typechecked +def invalidate_health_cache() -> None: + """Drop the cached verdict after a deliberate connect/disconnect, which makes it stale by definition.""" + global p_cached_result, p_cached_at + p_cached_result = None + p_cached_at = 0.0 + + @typechecked def classify_auth_dead(status_code: int, body_text: str) -> bool: """Dead ONLY on a definitive auth failure; anything ambiguous reads healthy (silence beats a false reconnect prompt).""" diff --git a/backend/tests/test_subscription_disconnect.py b/backend/tests/test_subscription_disconnect.py new file mode 100644 index 00000000..2e29ab2f --- /dev/null +++ b/backend/tests/test_subscription_disconnect.py @@ -0,0 +1,127 @@ +"""Disconnecting a subscription reports the VERIFIED end state, never the attempt. + +The shipped bug this pins: 9Router answers a bad id with a 404 and a JSON error body, and the old +code counted every DELETE it managed to send as a removal, so the UI said "disconnected" for a lane +that was still live and the next connect stacked on a stale row. +""" + +import asyncio +from typing import Dict, List, Optional + +import backend.apps.agents.disconnect_subscription as ds + + +class FakeResponse: + def __init__(self, status_code: int, payload: Optional[Dict] = None): + self.status_code = status_code + self.p_payload = payload or {} + + def json(self) -> Dict: + return self.p_payload + + +class FakeRouter: + """A 9Router whose rows only disappear when `deletable` allows the delete to succeed.""" + + def __init__(self, rows: List[Dict], deletable: bool = True, readable: bool = True): + self.rows = list(rows) + self.deletable = deletable + self.readable = readable + self.deletes: List[str] = [] + + def client(self, **kwargs) -> "FakeClient": + return FakeClient(self) + + +class FakeClient: + def __init__(self, router: FakeRouter): + self.router = router + + async def __aenter__(self) -> "FakeClient": + return self + + async def __aexit__(self, *exc) -> None: + return None + + async def get(self, url: str, **kw) -> FakeResponse: + if not self.router.readable: + return FakeResponse(503, {"error": "router down"}) + return FakeResponse(200, {"connections": self.router.rows}) + + async def delete(self, url: str, **kw) -> FakeResponse: + conn_id = url.rsplit("/", 1)[-1] + self.router.deletes.append(conn_id) + if not self.router.deletable: + return FakeResponse(404, {"error": "Connection not found"}) + self.router.rows = [r for r in self.router.rows if r["id"] != conn_id] + return FakeResponse(200, {"message": "Connection deleted successfully"}) + + +CLAUDE_AND_CODEX = [ + {"id": "c1", "provider": "claude", "name": "Account 1"}, + {"id": "x1", "provider": "codex", "name": "Account 1"}, + {"id": "x2", "provider": "codex", "name": "Account 2"}, +] + + +def p_install(monkeypatch, router: FakeRouter) -> None: + monkeypatch.setattr(ds, "is_running", lambda: True) + monkeypatch.setattr(ds.httpx, "AsyncClient", lambda **kw: router.client(**kw)) + monkeypatch.setattr(ds, "invalidate_health_cache", lambda: None) + monkeypatch.setattr(ds, "sync_settings_state", lambda: None) + + +def test_disconnect_clears_every_row_of_the_lane(monkeypatch): + router = FakeRouter(CLAUDE_AND_CODEX) + p_install(monkeypatch, router) + result = asyncio.run(ds.disconnect_subscription("codex")) + assert result.ok and result.removed == 2 and result.error == "" + assert [r["provider"] for r in router.rows] == ["claude"] + + +def test_a_refused_delete_is_never_reported_as_success(monkeypatch): + router = FakeRouter(CLAUDE_AND_CODEX, deletable=False) + p_install(monkeypatch, router) + result = asyncio.run(ds.disconnect_subscription("claude")) + assert not result.ok + assert result.removed == 0 + assert result.error + assert len(router.rows) == 3 # the lane really is still there + + +def test_an_unreadable_router_cannot_confirm(monkeypatch): + router = FakeRouter(CLAUDE_AND_CODEX, readable=False) + p_install(monkeypatch, router) + result = asyncio.run(ds.disconnect_subscription("claude")) + assert not result.ok and result.error + + +def test_disconnect_is_idempotent_when_the_lane_is_already_clear(monkeypatch): + router = FakeRouter([{"id": "c1", "provider": "claude"}]) + p_install(monkeypatch, router) + result = asyncio.run(ds.disconnect_subscription("codex")) + assert result.ok and result.removed == 0 + assert router.deletes == [] + + +def test_google_lanes_cascade_together(monkeypatch): + router = FakeRouter([ + {"id": "g1", "provider": "gemini-cli"}, + {"id": "a1", "provider": "antigravity"}, + {"id": "c1", "provider": "claude"}, + ]) + p_install(monkeypatch, router) + result = asyncio.run(ds.disconnect_subscription("gemini-cli")) + assert result.ok and result.removed == 2 + assert [r["provider"] for r in router.rows] == ["claude"] + # One-directional: dropping antigravity must NOT take gemini-cli with it. + assert ds.PROVIDER_CASCADE_REMOVES.get("antigravity") is None + + +def test_router_down_fails_closed(monkeypatch): + router = FakeRouter(CLAUDE_AND_CODEX) + p_install(monkeypatch, router) + monkeypatch.setattr(ds, "is_running", lambda: False) + result = asyncio.run(ds.disconnect_subscription("claude")) + assert not result.ok and result.error + assert router.deletes == [] diff --git a/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCard.tsx b/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCard.tsx index 5aaba333..9ee724c3 100644 --- a/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCard.tsx +++ b/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCard.tsx @@ -6,25 +6,42 @@ import CircularProgress from '@mui/material/CircularProgress'; import { useClaudeTokens } from '@/shared/styles/ThemeContext'; import type { SubscriptionProvider } from './subscriptionProviders'; -const SubscriptionCard: React.FC<{ provider: SubscriptionProvider; connected: boolean; onConnect: () => void; onDisconnect: () => void; connecting: boolean; userCode?: string; disconnecting?: boolean }> = ({ provider, connected, onConnect, onDisconnect, connecting, userCode, disconnecting }) => { +interface Props { + provider: SubscriptionProvider; + connected: boolean; + connecting: boolean; + confirmingDisconnect: boolean; + disconnecting: boolean; + error?: string; + userCode?: string; + onConnect: () => void; + onRequestDisconnect: () => void; + onCancelDisconnect: () => void; + onDisconnect: () => void; +} + +const SubscriptionCard: React.FC = ({ + provider, connected, connecting, confirmingDisconnect, disconnecting, error, userCode, + onConnect, onRequestDisconnect, onCancelDisconnect, onDisconnect, +}) => { const c = useClaudeTokens(); - const isPreview = (provider as any).preview; const dotColor = connected ? c.status.success : connecting ? c.accent.primary : c.border.medium; + const linkButton = { + border: 'none', background: 'transparent', p: 0, cursor: 'pointer', + fontFamily: 'inherit', fontSize: '0.6875rem', transition: 'color 0.15s ease', + } as const; return ( @@ -44,21 +61,32 @@ const SubscriptionCard: React.FC<{ provider: SubscriptionProvider; connected: bo - {isPreview ? ( + {provider.preview ? ( Coming soon ) : connected ? ( disconnecting ? ( - - ) : ( - - - Connected - - + + + Disconnecting... + + ) : confirmingDisconnect ? ( + + Disconnect? + + Cancel + + Disconnect - + + + ) : ( + + Connected + + Disconnect + ) ) : connecting && userCode ? ( @@ -77,6 +105,12 @@ const SubscriptionCard: React.FC<{ provider: SubscriptionProvider; connected: bo )} + + {error && ( + + {error} + + )} ); }; diff --git a/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCards.tsx b/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCards.tsx index 8e5e1976..d5a6d76d 100644 --- a/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCards.tsx +++ b/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCards.tsx @@ -11,12 +11,24 @@ import { setSubscriptionStatus, markSubscriptionConnected, selectSubscriptionConnections, + type SubscriptionConnection, } from '@/shared/state/subscriptionsSlice'; import { API_BASE } from '@/shared/config'; import { SUBSCRIPTION_PROVIDERS } from './subscriptionProviders'; import SubscriptionCard from './SubscriptionCard'; import { runConnectFlow } from './subscriptionConnect'; +/** What POST /agents/subscriptions/disconnect answers; `ok` is the backend's verified end state, never a guess. */ +interface DisconnectResponse { + ok?: boolean; + removed?: number; + error?: string; +} + +function isProviderActive(connections: SubscriptionConnection[], providerId: string): boolean { + return connections.some((p) => p.provider === providerId && (p.isActive || p.testStatus === 'active')); +} + function friendlyConnectError(detail: string): string { const d = (detail || '').trim(); const lower = d.toLowerCase(); @@ -36,6 +48,8 @@ const SubscriptionCards: React.FC = () => { const connections = useAppSelector(selectSubscriptionConnections); const [connecting, setConnecting] = useState(null); const [disconnecting, setDisconnecting] = useState(null); + const [confirmingDisconnect, setConfirmingDisconnect] = useState(null); + const [disconnectError, setDisconnectError] = useState<{ provider: string; message: string } | null>(null); const [userCode, setUserCode] = useState(''); const [pollTimer, setPollTimer] = useState(null); const [connectError, setConnectError] = useState(null); @@ -70,15 +84,18 @@ const SubscriptionCards: React.FC = () => { return () => { cancelled = true; clearInterval(interval); }; }, [fetchStatus]); - const isConnected = (providerId: string) => - connections.some( - (p: any) => - p.provider === providerId && (p.isActive || p.testStatus === 'active'), - ); + const isConnected = (providerId: string) => isProviderActive(connections, providerId); + + // A pending confirm on a lane that died some other way (reconnect, cascade wipe) would pop back on reconnect. + useEffect(() => { + if (confirmingDisconnect && !isProviderActive(connections, confirmingDisconnect)) setConfirmingDisconnect(null); + }, [confirmingDisconnect, connections]); const handleConnect = async (providerId: string) => { if (pollTimer) { clearInterval(pollTimer); setPollTimer(null); } setConnectError(null); + setConfirmingDisconnect(null); + setDisconnectError(null); setConnecting(providerId); setUserCode(''); @@ -105,20 +122,27 @@ const SubscriptionCards: React.FC = () => { }; const handleDisconnect = async (providerId: string) => { + if (disconnecting) return; + setConfirmingDisconnect(null); + setDisconnectError(null); setDisconnecting(providerId); + // No settle delay: the backend only answers ok after re-reading 9Router, so the lane is already gone. try { - await fetch(`${API_BASE}/agents/subscriptions/disconnect`, { + const r = await fetch(`${API_BASE}/agents/subscriptions/disconnect`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ provider: providerId }), }); - } catch {} - // Wait briefly for 9Router to process, then refresh subscription status + model picker. - setTimeout(() => { - fetchStatus(); - refreshPickerModels(); - setDisconnecting(null); - }, 500); + const data = (await r.json().catch(() => ({}))) as DisconnectResponse; + if (!r.ok || !data.ok) { + setDisconnectError({ provider: providerId, message: data.error || 'Could not disconnect. Please try again.' }); + } + } catch { + setDisconnectError({ provider: providerId, message: 'Could not reach OpenSwarm. Please try again.' }); + } + await fetchStatus(); + refreshPickerModels(); + setDisconnecting(null); }; // 4s safety-net poller while connecting; clears Connecting state whenever 9Router reports the provider isActive (handles Windows postMessage failures). @@ -206,9 +230,13 @@ const SubscriptionCards: React.FC = () => { provider={p} connected={isConnected(p.id)} onConnect={() => handleConnect(p.id)} + onRequestDisconnect={() => { setDisconnectError(null); setConfirmingDisconnect(p.id); }} + onCancelDisconnect={() => setConfirmingDisconnect(null)} onDisconnect={() => handleDisconnect(p.id)} connecting={connecting === p.id} + confirmingDisconnect={confirmingDisconnect === p.id} disconnecting={disconnecting === p.id} + error={disconnectError?.provider === p.id ? disconnectError.message : undefined} userCode={connecting === p.id ? userCode : undefined} /> ))}