From bfd330f8ed989547345d95359fa5b5a53dfc99c5 Mon Sep 17 00:00:00 2001 From: Juan Pedro Michelini Jorge Date: Mon, 10 Aug 2026 18:25:51 -0300 Subject: [PATCH 1/7] scaffold: provider-connections (draft) --- .../openhands/agent_server/llm_connections.py | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 openhands-agent-server/openhands/agent_server/llm_connections.py diff --git a/openhands-agent-server/openhands/agent_server/llm_connections.py b/openhands-agent-server/openhands/agent_server/llm_connections.py new file mode 100644 index 0000000000..2778b99dfa --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/llm_connections.py @@ -0,0 +1,29 @@ +"""Provider Connection endpoints (scaffold, draft). + +Introduces a first-class Provider Connection object: connect a vendor once +with one key, pick from its model catalog, auto-create LLM profiles that +reference the connection's key by name (not inline). + +Tracking: OpenHands/OpenHands#15492, Linear OSS-5295. +Scope: software-agent-sdk PR1 of the provider-connections plan. + +TODO (implementation): + - Reuse SecretsService to store the connection key as a named secret. + - GET /api/llm/connections -> list connections (masked key) + - POST /api/llm/connections -> create: {provider, key, label?} + - GET /api/llm/connections/{id} -> connection + selectable models + - PATCH /api/llm/connections/{id} -> rotate key / rename + - DELETE /api/llm/connections/{id} -> disconnect (+ optional profile cleanup) + - POST /api/llm/connections/{id}/validate -> test key, return catalog from + /api/llm/models?provider={vendor} + +Design decisions (see Notion shaping doc + PR body): + - key is stored per-connection (not per-provider) so a second key for the + same provider is additive later (multiple-keys-per-provider is deferred). + - Cloud path (callCloudProxy) must never return the key. + - No background refresh job in this PR; pull-on-enter / validate covers + "see new models as soon as supported via API". + +This file is intentionally a stub; real router additions land in llm_router.py +and a new llm_connections.py module. +""" From a3c9c6e586baa6705f5ccce10341afb9cf7a8e93 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 10 Aug 2026 21:50:56 +0000 Subject: [PATCH 2/7] feat: Provider Connection endpoints + secret-by-name LLM key resolution Add a first-class Provider Connection object: connect a vendor once with one key, pick from its model catalog, and spawn LLM profiles that reference the connection's key by name (secret:) instead of inline-duplicating it. Agent-server (openhands-agent-server): - persistence: ProviderConnection + PersistedConnections models, FileConnectionsStore (single JSON, file-locked, atomic write), get_connections_store/reset_stores. - llm_connections router (mounted at /api/llm/connections): create/list/get/patch/ delete + /validate. The connection's key is stored as a named secret via the existing SecretsStore; responses never echo the key (api_key_set only). validate_provider_key is module-level so tests monkeypatch it (no network). Key is per-connection so a second key for a provider is additive later. - api.create_app registers the LLM secret: resolver against the SecretsStore. SDK (openhands-sdk): - LLM._get_api_key_value resolves a secret: api_key via an injectable resolver (register_llm_secret_resolver); raw keys pass through unchanged, and an unset resolver degrades gracefully (None), so standalone SDK use is unaffected. This realizes secret-by-name at runtime with a minimal, testable change (rotation = one store write, every profile picks it up). Tests: 20 new tests covering CRUD, masking, validate (incl. injected validator), connection limit, persistence roundtrip/schema guard, and LLM secret-ref resolution (with/without resolver, raw-key passthrough). Existing llm/settings/ profiles router tests still pass (no regressions). Refs OpenHands/OpenHands#15492, Linear OSS-5295. Co-authored-by: openhands --- .../openhands/agent_server/api.py | 11 + .../openhands/agent_server/llm_connections.py | 457 ++++++++++++++++-- .../agent_server/persistence/__init__.py | 18 + .../agent_server/persistence/models.py | 80 +++ .../agent_server/persistence/store.py | 108 +++++ openhands-sdk/openhands/sdk/llm/llm.py | 73 ++- tests/agent_server/test_llm_connections.py | 278 +++++++++++ 7 files changed, 996 insertions(+), 29 deletions(-) create mode 100644 tests/agent_server/test_llm_connections.py diff --git a/openhands-agent-server/openhands/agent_server/api.py b/openhands-agent-server/openhands/agent_server/api.py index 541edf0829..c3e8e1e1a6 100644 --- a/openhands-agent-server/openhands/agent_server/api.py +++ b/openhands-agent-server/openhands/agent_server/api.py @@ -48,6 +48,7 @@ init_router, require_initialized, ) +from openhands.agent_server.llm_connections import connections_router from openhands.agent_server.llm_router import llm_router from openhands.agent_server.mcp_router import mcp_router from openhands.agent_server.middleware import CORSDispatcher @@ -440,6 +441,7 @@ def _add_api_routes(app: FastAPI) -> None: api_router.include_router(plugins_router) api_router.include_router(hooks_router) api_router.include_router(llm_router) + api_router.include_router(connections_router) api_router.include_router(mcp_router) api_router.include_router(settings_router) api_router.include_router(workspaces_router) @@ -694,6 +696,15 @@ def create_app(config: Config | None = None) -> FastAPI: _add_api_routes(app) _setup_static_files(app, config) + + # Register the LLM ``secret:`` resolver so profiles spawned from a + # provider connection resolve their api_key against the agent-server's + # SecretsStore at call time (see LLM._get_api_key_value). + from openhands.agent_server.persistence import get_secrets_store + from openhands.sdk.llm.llm import register_llm_secret_resolver + + secrets_store = get_secrets_store(config) + register_llm_secret_resolver(lambda name: secrets_store.get_secret(name)) app.add_middleware( CORSDispatcher, allow_origins=config.allow_cors_origins, diff --git a/openhands-agent-server/openhands/agent_server/llm_connections.py b/openhands-agent-server/openhands/agent_server/llm_connections.py index 2778b99dfa..247ca64b43 100644 --- a/openhands-agent-server/openhands/agent_server/llm_connections.py +++ b/openhands-agent-server/openhands/agent_server/llm_connections.py @@ -1,29 +1,430 @@ -"""Provider Connection endpoints (scaffold, draft). - -Introduces a first-class Provider Connection object: connect a vendor once -with one key, pick from its model catalog, auto-create LLM profiles that -reference the connection's key by name (not inline). - -Tracking: OpenHands/OpenHands#15492, Linear OSS-5295. -Scope: software-agent-sdk PR1 of the provider-connections plan. - -TODO (implementation): - - Reuse SecretsService to store the connection key as a named secret. - - GET /api/llm/connections -> list connections (masked key) - - POST /api/llm/connections -> create: {provider, key, label?} - - GET /api/llm/connections/{id} -> connection + selectable models - - PATCH /api/llm/connections/{id} -> rotate key / rename - - DELETE /api/llm/connections/{id} -> disconnect (+ optional profile cleanup) - - POST /api/llm/connections/{id}/validate -> test key, return catalog from - /api/llm/models?provider={vendor} - -Design decisions (see Notion shaping doc + PR body): - - key is stored per-connection (not per-provider) so a second key for the - same provider is additive later (multiple-keys-per-provider is deferred). - - Cloud path (callCloudProxy) must never return the key. - - No background refresh job in this PR; pull-on-enter / validate covers - "see new models as soon as supported via API". - -This file is intentionally a stub; real router additions land in llm_router.py -and a new llm_connections.py module. +"""Provider Connection endpoints: connect a vendor once, pick from its models. + +A Provider Connection is the persisted record for the "connect a provider once" +flow (OpenHands/OpenHands#15492). The connection stores a *reference* to a named +secret (the API key lives in the SecretsStore), plus the list of models the user +selected from the provider's catalog. The raw key is never returned to clients: +responses carry ``api_key_set`` and the connection's ``secret_name`` is treated +as sensitive metadata. + +The key is stored per-connection (not per-provider), so a second key for the same +provider is an additive connection later — multiple-keys-per-provider is +deferred but the data model already supports it. + +Endpoints (mounted under ``/api/llm``): + + - GET /connections list connections (masked, no keys) + - POST /connections create {provider, key, label?, models?} + - GET /connections/{id} connection + its selected models + - PATCH /connections/{id} rotate key / rename label / set models + - DELETE /connections/{id} disconnect (+ delete the named secret) + - POST /connections/{id}/validate test the key, return the provider's + model catalog; updates last_validated_at + +LLM profiles spawned from a connection store ``api_key = "secret:"`` +(see :func:`openhands.agent_server.persistence.llm_secret_ref`) instead of the +raw key, so rotating the key is one SecretsStore write and every referencing +profile picks it up at call time (see ``LLM._get_api_key_value``). """ + +from __future__ import annotations + +import time +import uuid +from collections.abc import Callable + +from fastapi import APIRouter, HTTPException, Request, status +from pydantic import BaseModel, Field, SecretStr + +from openhands.agent_server._secrets_exposure import get_config +from openhands.agent_server.persistence import ( + ProviderConnection, + get_connections_store, + get_secrets_store, +) +from openhands.sdk.llm.utils.unverified_models import ( + _extract_model_and_provider, + _get_litellm_provider_names, + get_supported_llm_models, +) +from openhands.sdk.llm.utils.verified_models import VERIFIED_MODELS +from openhands.sdk.logger import get_logger + + +logger = get_logger(__name__) + +connections_router = APIRouter(prefix="/llm/connections", tags=["LLM Connections"]) + +# Cap on the number of saved connections. Per-connection keys mean a second key +# for the same provider is additive; 64 leaves ample headroom while bounding the +# catalog the GUI renders. +MAX_CONNECTIONS = 64 + +# Length of the per-connection id (uuid4 hex, 32 chars). Used only for the +# ``secret_name`` derivation below; the id itself is opaque to clients. +_SECRET_NAME_PREFIX = "llm_connection_" + + +def _connection_secret_name(connection_id: str) -> str: + """Derive the named-secret key under which a connection's key is stored.""" + return f"{_SECRET_NAME_PREFIX}{connection_id}" + + +def _now() -> int: + return int(time.time()) + + +def _provider_names() -> set[str]: + return _get_litellm_provider_names() + + +# ── Request / Response models ──────────────────────────────────────────── + + +class ConnectionCreateRequest(BaseModel): + """Create a connection. ``key`` is written to the SecretsStore; never echoed.""" + + provider: str = Field(..., min_length=1, max_length=128) + key: SecretStr = Field(..., min_length=1) + label: str | None = Field(default=None, max_length=128) + models: list[str] = Field(default_factory=list) + + +class ConnectionUpdateRequest(BaseModel): + """Partial update a connection. + + ``key`` rotates the named secret (rewrites the SecretsStore entry). ``label`` + and ``models`` are straightforward field updates. At least one field is + required. + """ + + key: SecretStr | None = None + label: str | None = None + models: list[str] | None = None + + +class ConnectionResponse(BaseModel): + """Safe connection view — never includes the raw key or the secret value.""" + + id: str + provider: str + label: str | None = None + models: list[str] = Field(default_factory=list) + created_at: int + last_validated_at: int | None = None + api_key_set: bool = False + + +class ValidateResponse(BaseModel): + """Result of testing a connection's key against the provider's catalog.""" + + id: str + provider: str + ok: bool + models: list[str] = Field(default_factory=list) + error: str | None = None + validated_at: int + + +def _to_response(conn: ProviderConnection, *, api_key_set: bool) -> ConnectionResponse: + return ConnectionResponse( + id=conn.id, + provider=conn.provider, + label=conn.label, + models=list(conn.models), + created_at=conn.created_at, + last_validated_at=conn.last_validated_at, + api_key_set=api_key_set, + ) + + +def _api_key_set(secret_name: str) -> bool: + """True if the named secret backing a connection currently holds a value.""" + store = get_secrets_store() + value = store.get_secret(secret_name) + return bool(value and value.strip()) + + +def _get_connection_or_404(connections, connection_id: str) -> ProviderConnection: + for conn in connections.connections: + if conn.id == connection_id: + return conn + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Connection '{connection_id}' not found", + ) + + +# ── Provider key validation (injectable for tests) ─────────────────────── +# +# ``validate_provider_key`` attempts to confirm a key works for a provider and +# returns that provider's model catalog. The default implementation is +# conservative: it never makes a live network call (which would be slow and +# non-deterministic in tests); it returns the static LiteLLM catalog for the +# provider when the key is non-empty, so the wizard can populate the picker. +# A production implementation can override this (or a future flag) to issue a +# real cheap probe and surface a 401/403 cause. The function is module-level so +# tests monkeypatch it. + +ValidateFn = Callable[[str, str], tuple[bool, list[str], str | None]] + + +def validate_provider_key( + provider: str, key: str +) -> tuple[bool, list[str], str | None]: + """Default validator: non-empty key => provider's static model catalog. + + Returns ``(ok, models, error)``. ``ok`` is True for a non-empty key against a + known provider; ``models`` is the provider-filtered LiteLLM catalog; ``error`` + is None on success or a short cause string on failure. + """ + if not key or not key.strip(): + return False, [], "API key is empty" + if provider not in _provider_names(): + return False, [], f"Unknown provider '{provider}'" + + all_models = get_supported_llm_models() + verified_provider_models = set(VERIFIED_MODELS.get(provider, ())) + filtered: list[str] = [] + for model in all_models: + model_provider, _, _ = _extract_model_and_provider(model) + if model_provider == provider or model in verified_provider_models: + filtered.append(model) + return True, sorted(set(filtered)), None + + +# ── Endpoints ──────────────────────────────────────────────────────────── + + +@connections_router.get("", response_model=list[ConnectionResponse]) +async def list_connections(request: Request) -> list[ConnectionResponse]: + """List all saved provider connections (keys never returned).""" + config = get_config(request) + store = get_connections_store(config) + persisted = store.load() + conns = persisted.connections if persisted is not None else [] + return [ + _to_response(c, api_key_set=_api_key_set(c.secret_name)) for c in conns + ] + + +@connections_router.post( + "", + response_model=ConnectionResponse, + status_code=status.HTTP_201_CREATED, +) +async def create_connection( + request: Request, body: ConnectionCreateRequest +) -> ConnectionResponse: + """Create a connection: store the key as a named secret, then the record.""" + if body.provider not in _provider_names(): + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"Unknown provider '{body.provider}'", + ) + + config = get_config(request) + store = get_connections_store(config) + secrets_store = get_secrets_store(config) + + connection_id = uuid.uuid4().hex + secret_name = _connection_secret_name(connection_id) + + def add(conn_list): + if len(conn_list.connections) >= MAX_CONNECTIONS: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=( + f"Connection limit reached ({MAX_CONNECTIONS}). " + "Disconnect one before adding a new connection." + ), + ) + conn = ProviderConnection( + id=connection_id, + provider=body.provider, + label=body.label, + secret_name=secret_name, + models=list(body.models), + created_at=_now(), + ) + conn_list.connections.append(conn) + return conn_list + + try: + secrets_store.set_secret( + name=secret_name, + value=body.key.get_secret_value(), + description=f"LLM provider connection key for {body.provider}", + ) + except RuntimeError as e: + logger.error(f"Connection create blocked (secrets): {e}") + raise HTTPException( + status_code=500, + detail="Secrets file is corrupted or encrypted with a different key", + ) + + try: + persisted = store.update(add) + except HTTPException: + # Roll back the secret we just wrote so we don't leak orphaned keys. + try: + secrets_store.delete_secret(secret_name) + except Exception: # noqa: BLE001 - best-effort cleanup + logger.warning(f"Failed to roll back secret {secret_name}") + raise + + conn = next(c for c in persisted.connections if c.id == connection_id) + logger.info( + "Created provider connection", + extra={"connection_id": connection_id, "provider": body.provider}, + ) + return _to_response(conn, api_key_set=True) + + +@connections_router.get("/{connection_id}", response_model=ConnectionResponse) +async def get_connection(request: Request, connection_id: str) -> ConnectionResponse: + """Get a single connection (key never returned).""" + config = get_config(request) + store = get_connections_store(config) + persisted = store.load() + if persisted is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Connection '{connection_id}' not found", + ) + conn = _get_connection_or_404(persisted, connection_id) + return _to_response(conn, api_key_set=_api_key_set(conn.secret_name)) + + +@connections_router.patch("/{connection_id}", response_model=ConnectionResponse) +async def update_connection( + request: Request, connection_id: str, body: ConnectionUpdateRequest +) -> ConnectionResponse: + """Update a connection: rotate key, rename label, or set selected models.""" + if body.key is None and body.label is None and body.models is None: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="Provide at least one of: key, label, models", + ) + + config = get_config(request) + store = get_connections_store(config) + secrets_store = get_secrets_store(config) + + if body.key is not None: + # Rotate the named secret first; the connection record only references it. + persisted = store.load() or _empty() + conn_to_rotate = _get_connection_or_404(persisted, connection_id) + try: + secrets_store.set_secret( + name=conn_to_rotate.secret_name, + value=body.key.get_secret_value(), + description=( + f"LLM provider connection key for {conn_to_rotate.provider}" + ), + ) + except RuntimeError as e: + logger.error(f"Connection rotate blocked (secrets): {e}") + raise HTTPException( + status_code=500, + detail="Secrets file is corrupted or encrypted with a different key", + ) + + def patch(conn_list): + for c in conn_list.connections: + if c.id == connection_id: + if body.label is not None: + c = c.model_copy(update={"label": body.label}) + if body.models is not None: + c = c.model_copy(update={"models": list(body.models)}) + conn_list.connections = [ + c if x.id == connection_id else x for x in conn_list.connections + ] + return conn_list + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Connection '{connection_id}' not found", + ) + + persisted = store.update(patch) + conn = next(c for c in persisted.connections if c.id == connection_id) + return _to_response(conn, api_key_set=_api_key_set(conn.secret_name)) + + +@connections_router.delete("/{connection_id}", status_code=status.HTTP_204_NO_CONTENT) +async def delete_connection(request: Request, connection_id: str): + """Disconnect: delete the connection record and its named secret.""" + config = get_config(request) + store = get_connections_store(config) + secrets_store = get_secrets_store(config) + + deleted_secret_name: str | None = None + + def remove(conn_list): + nonlocal deleted_secret_name + for i, c in enumerate(conn_list.connections): + if c.id == connection_id: + deleted_secret_name = c.secret_name + conn_list.connections.pop(i) + return conn_list + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Connection '{connection_id}' not found", + ) + + store.update(remove) + if deleted_secret_name is not None: + try: + secrets_store.delete_secret(deleted_secret_name) + except Exception: # noqa: BLE001 - record already gone; best-effort + logger.warning(f"Failed to delete secret {deleted_secret_name}") + logger.info("Deleted provider connection", extra={"connection_id": connection_id}) + + +@connections_router.post( + "/{connection_id}/validate", response_model=ValidateResponse +) +async def validate_connection( + request: Request, connection_id: str +) -> ValidateResponse: + """Test the connection's key against the provider and return its catalog.""" + config = get_config(request) + store = get_connections_store(config) + secrets_store = get_secrets_store(config) + + persisted = store.load() or _empty() + conn = _get_connection_or_404(persisted, connection_id) + key = secrets_store.get_secret(conn.secret_name) or "" + + ok, models, error = validate_provider_key(conn.provider, key) + validated_at = _now() + if ok: + # Stamp last_validated_at on success. + def stamp(conn_list): + for c in conn_list.connections: + if c.id == connection_id: + c = c.model_copy(update={"last_validated_at": validated_at}) + conn_list.connections = [ + c if x.id == connection_id else x + for x in conn_list.connections + ] + return conn_list + return conn_list + + store.update(stamp) + + return ValidateResponse( + id=connection_id, + provider=conn.provider, + ok=ok, + models=models, + error=error, + validated_at=validated_at, + ) + + +def _empty(): + """Return a fresh empty PersistedConnections (avoids ``load() or None`` chains).""" + from openhands.agent_server.persistence import PersistedConnections + + return PersistedConnections() diff --git a/openhands-agent-server/openhands/agent_server/persistence/__init__.py b/openhands-agent-server/openhands/agent_server/persistence/__init__.py index b41360a259..75d76e79e2 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/__init__.py +++ b/openhands-agent-server/openhands/agent_server/persistence/__init__.py @@ -7,18 +7,26 @@ """ from openhands.agent_server.persistence.models import ( + CONNECTIONS_SCHEMA_VERSION, + LLM_SECRET_REF_PREFIX, PERSISTED_SETTINGS_SCHEMA_VERSION, SECRET_NAME_PATTERN, WORKSPACES_SCHEMA_VERSION, CustomSecret, + PersistedConnections, PersistedSettings, PersistedWorkspaces, + ProviderConnection, Secrets, SettingsUpdatePayload, WorkspaceItem, WorkspaceParentItem, + llm_secret_ref, + parse_llm_secret_ref, ) from openhands.agent_server.persistence.store import ( + ConnectionsStore, + FileConnectionsStore, FileSecretsStore, FileSettingsStore, FileWorkspacesStore, @@ -26,6 +34,7 @@ SettingsStore, WorkspacesStore, get_agent_profile_store, + get_connections_store, get_llm_profile_store, get_secrets_store, get_settings_store, @@ -36,25 +45,34 @@ __all__ = [ # Constants + "CONNECTIONS_SCHEMA_VERSION", + "LLM_SECRET_REF_PREFIX", "PERSISTED_SETTINGS_SCHEMA_VERSION", "SECRET_NAME_PATTERN", "WORKSPACES_SCHEMA_VERSION", # Models "CustomSecret", + "PersistedConnections", "PersistedSettings", "PersistedWorkspaces", + "ProviderConnection", "Secrets", "SettingsUpdatePayload", "WorkspaceItem", "WorkspaceParentItem", + "llm_secret_ref", + "parse_llm_secret_ref", # Stores + "FileConnectionsStore", "FileSecretsStore", "FileSettingsStore", "FileWorkspacesStore", + "ConnectionsStore", "SecretsStore", "SettingsStore", "WorkspacesStore", "get_agent_profile_store", + "get_connections_store", "get_llm_profile_store", "get_secrets_store", "get_settings_store", diff --git a/openhands-agent-server/openhands/agent_server/persistence/models.py b/openhands-agent-server/openhands/agent_server/persistence/models.py index 2c4547a1f7..1fbf157d3d 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/models.py +++ b/openhands-agent-server/openhands/agent_server/persistence/models.py @@ -537,6 +537,86 @@ def from_persisted(cls, data: Any) -> PersistedWorkspaces: return cls.model_validate(payload) +# ── Provider Connections ───────────────────────────────────────────────── +# +# A "Provider Connection" is the persisted record for "connect a vendor once +# with one key" (OpenHands/OpenHands#15492). The connection stores a *reference* +# to a named secret (the API key lives in the SecretsStore), plus the list of +# models the user selected from the provider's catalog. The raw key is never +# stored in the connection record — only ``secret_name`` — so rotating the key +# is one SecretsStore write and every spawned LLM profile that references the +# same secret picks it up. The key is stored per-connection (not per-provider), +# so a second key for the same provider is an additive connection later. + +CONNECTIONS_SCHEMA_VERSION = 1 + +# Marker prefix an LLM profile's ``api_key`` uses to point at a named secret +# managed by a Provider Connection, instead of holding the raw key inline. +# Resolution happens at call time in ``LLM._get_api_key_value``. +LLM_SECRET_REF_PREFIX = "secret:" + + +def parse_llm_secret_ref(api_key: str | None) -> str | None: + """Return the secret name referenced by a ``secret:`` api_key, else None.""" + if not isinstance(api_key, str): + return None + if not api_key.startswith(LLM_SECRET_REF_PREFIX): + return None + name = api_key[len(LLM_SECRET_REF_PREFIX) :].strip() + return name or None + + +def llm_secret_ref(secret_name: str) -> str: + """Build the ``secret:`` reference string stored in a profile's api_key.""" + return f"{LLM_SECRET_REF_PREFIX}{secret_name}" + + +class ProviderConnection(BaseModel): + """A saved provider connection (one key, many models). + + ``secret_name`` references the key stored in the SecretsStore; the value is + never held here. Responses to clients must mask the key (``api_key_set``) + and never return ``secret_name``'s value. + """ + + id: str = Field(..., min_length=1, max_length=128) + provider: str = Field(..., min_length=1, max_length=128) + label: str | None = Field(default=None, max_length=128) + secret_name: str = Field(..., min_length=1, max_length=128) + models: list[str] = Field(default_factory=list) + created_at: int = Field(..., description="Unix epoch seconds.") + last_validated_at: int | None = Field( + default=None, description="Unix epoch seconds of last successful validate." + ) + + model_config = ConfigDict(populate_by_name=True) + + +class PersistedConnections(BaseModel): + """Container for all provider connections (single JSON document).""" + + schema_version: int = Field(default=CONNECTIONS_SCHEMA_VERSION) + connections: list[ProviderConnection] = Field(default_factory=list) + + model_config = ConfigDict(populate_by_name=True) + + @classmethod + def from_persisted(cls, data: Any) -> PersistedConnections: + if not isinstance(data, dict): + return cls.model_validate(data) + payload = dict(data) + version = payload.get("schema_version", CONNECTIONS_SCHEMA_VERSION) + if not isinstance(version, int): + raise ValueError("PersistedConnections schema_version must be an integer") + if version > CONNECTIONS_SCHEMA_VERSION: + raise ValueError( + f"PersistedConnections schema_version {version} is newer than " + f"supported {CONNECTIONS_SCHEMA_VERSION}" + ) + payload["schema_version"] = CONNECTIONS_SCHEMA_VERSION + return cls.model_validate(payload) + + # ── Helper Functions ───────────────────────────────────────────────────── # # Note: API request/response models have been moved to the SDK to enable diff --git a/openhands-agent-server/openhands/agent_server/persistence/store.py b/openhands-agent-server/openhands/agent_server/persistence/store.py index d661a0cd87..a449fc32ba 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/store.py +++ b/openhands-agent-server/openhands/agent_server/persistence/store.py @@ -24,6 +24,7 @@ from openhands.agent_server.persistence.models import ( CustomSecret, + PersistedConnections, PersistedSettings, PersistedWorkspaces, Secrets, @@ -786,11 +787,94 @@ def update( return updated +class ConnectionsStore(ABC): + """Abstract base class for provider-connection storage.""" + + @abstractmethod + def load(self) -> PersistedConnections | None: + """Load connections from storage.""" + + @abstractmethod + def save(self, connections: PersistedConnections) -> None: + """Save connections to storage.""" + + @abstractmethod + def update( + self, + update_fn: Callable[[PersistedConnections], PersistedConnections], + ) -> PersistedConnections: + """Atomically update connections with file locking.""" + + +class FileConnectionsStore(ConnectionsStore): + """File-based storage for provider connections. + + Persists a single JSON document at ``/connections.json`` + using the same atomic-write + file-lock primitives as ``FileWorkspacesStore``. + Connection records hold a ``secret_name`` reference to the key (stored in the + SecretsStore), never the key value itself, so no cipher is needed here. + """ + + def __init__( + self, + persistence_dir: Path | str, + filename: str = "connections.json", + ): + _validate_filename(filename) + self.persistence_dir = Path(persistence_dir) + self.filename = filename + self._path = self.persistence_dir / filename + self._lock_path = self.persistence_dir / ".connections.lock" + + def load(self) -> PersistedConnections | None: + if not self._path.exists(): + return None + + try: + with self._path.open("r", encoding="utf-8") as f: + data = json.load(f) + return PersistedConnections.from_persisted(data) + except (PermissionError, OSError) as e: + logger.error(f"Cannot access connections file: {e}") + raise + except json.JSONDecodeError as e: + logger.error(f"Connections file is corrupted: {e}") + return None + except Exception: + logger.error("Failed to load connections", exc_info=True) + return None + + def save(self, connections: PersistedConnections) -> None: + _ensure_secure_directory(self.persistence_dir) + data = connections.model_dump(mode="json", exclude_none=True) + _atomic_write_json(self._path, data) + logger.debug(f"Connections saved to {self._path}") + + def update( + self, + update_fn: Callable[[PersistedConnections], PersistedConnections], + ) -> PersistedConnections: + with _file_lock(self._lock_path): + connections = self.load() + if connections is None: + if self._path.exists(): + raise RuntimeError( + f"Cannot load connections from {self._path}. " + "File may be corrupted. " + "Refusing to overwrite with defaults to prevent data loss." + ) + connections = PersistedConnections() + updated = update_fn(connections) + self.save(updated) + return updated + + # ── Global Store Access ────────────────────────────────────────────────── _settings_store: FileSettingsStore | None = None _secrets_store: FileSecretsStore | None = None _workspaces_store: FileWorkspacesStore | None = None +_connections_store: FileConnectionsStore | None = None _llm_profile_store: LLMProfileStore | None = None _agent_profile_store: AgentProfileStore | None = None _store_lock = threading.Lock() @@ -914,6 +998,28 @@ def get_workspaces_store(config: Config | None = None) -> FileWorkspacesStore: return _workspaces_store +def get_connections_store(config: Config | None = None) -> FileConnectionsStore: # noqa: ARG001 + """Get the global provider-connections store instance (thread-safe). + + Connection records hold only a ``secret_name`` reference to the key (the key + lives in the SecretsStore), so no cipher is used here. Stored in the profile + persistence dir (same as secrets/profiles) so credentials stay in the user's + config directory, never workspace-relative. ``config`` is accepted for parity + with the other store factories; the connections dir is resolved from + ``OH_PERSISTENCE_DIR`` / ``~/.openhands`` (see ``_get_profile_persistence_dir``). + """ + global _connections_store + if _connections_store is not None: + return _connections_store + + with _store_lock: + if _connections_store is None: + _connections_store = FileConnectionsStore( + persistence_dir=_get_profile_persistence_dir(), + ) + return _connections_store + + def get_llm_profile_store() -> LLMProfileStore: """Get the global ``LLMProfileStore`` instance (thread-safe). @@ -957,10 +1063,12 @@ def get_agent_profile_store() -> AgentProfileStore: def reset_stores() -> None: """Reset global store instances (for testing).""" global _settings_store, _secrets_store, _workspaces_store + global _connections_store global _llm_profile_store, _agent_profile_store with _store_lock: _settings_store = None _secrets_store = None _workspaces_store = None + _connections_store = None _llm_profile_store = None _agent_profile_store = None diff --git a/openhands-sdk/openhands/sdk/llm/llm.py b/openhands-sdk/openhands/sdk/llm/llm.py index aa4ee34641..583216727e 100644 --- a/openhands-sdk/openhands/sdk/llm/llm.py +++ b/openhands-sdk/openhands/sdk/llm/llm.py @@ -123,6 +123,72 @@ logger = get_logger(__name__) + + +# ── Secret-reference resolution for provider connections ───────────────── +# +# An LLM profile spawned from a "provider connection" (OpenHands/OpenHands#15492) +# stores ``api_key = "secret:"`` instead of the raw key, so rotating +# the key is one SecretsStore write and every referencing profile picks it up. +# At call time, ``_get_api_key_value`` resolves the ``secret:`` prefix via this +# hook. The SDK stays decoupled from any concrete secret store: the agent-server +# installs a resolver (``register_llm_secret_resolver``); without one, a +# ``secret:`` reference resolves to ``None`` (the same behavior as a missing key), +# so a standalone SDK use that never set a resolver is unaffected. +LLM_SECRET_REF_PREFIX = "secret:" + + +def parse_llm_secret_ref(api_key: str | None) -> str | None: + """Return the secret name referenced by a ``secret:`` api_key, else None.""" + if not isinstance(api_key, str): + return None + if not api_key.startswith(LLM_SECRET_REF_PREFIX): + return None + name = api_key[len(LLM_SECRET_REF_PREFIX) :].strip() + return name or None + + +def llm_secret_ref(secret_name: str) -> str: + """Build the ``secret:`` reference string stored in a profile's api_key.""" + return f"{LLM_SECRET_REF_PREFIX}{secret_name}" + + +_LLMSecretResolver = Callable[[str], str | None] +_llm_secret_resolver: _LLMSecretResolver | None = None +_llm_secret_resolver_lock = threading.Lock() + + +def register_llm_secret_resolver(resolver: _LLMSecretResolver | None) -> None: + """Install (or clear) the resolver used for ``secret:`` api_key values. + + The agent-server registers a resolver that reads its SecretsStore so a + profile's ``secret:`` api_key resolves to the real key at call time. + Passing ``None`` clears the resolver (e.g. between tests). + """ + global _llm_secret_resolver + with _llm_secret_resolver_lock: + _llm_secret_resolver = resolver + + +def _resolve_api_key(api_key: str | None) -> str | None: + """Resolve a possibly-``secret:``-prefixed api_key to its raw value.""" + ref = parse_llm_secret_ref(api_key) + if ref is None: + # Not a reference: ``api_key`` is either a raw key (str) or None. + return api_key if isinstance(api_key, str) else None + with _llm_secret_resolver_lock: + resolver = _llm_secret_resolver + if resolver is None: + return None + try: + return resolver(ref) + except Exception: # noqa: BLE001 - never leak resolver internals to the LLM call + logger.warning( + f"Failed to resolve LLM secret reference '{ref}' - treating as missing" + ) + return None + + _serialized_is_subscription = ContextVar( "serialized_is_subscription", default=False, @@ -2037,7 +2103,12 @@ def _get_api_key_value(self) -> str | None: if self.api_key is None: return None assert isinstance(self.api_key, SecretStr) - return self.api_key.get_secret_value() + raw = self.api_key.get_secret_value() + # ``secret:`` references a named secret managed by a provider + # connection (see ``register_llm_secret_resolver``); resolve it at call + # time so rotation is one store write picked up by every profile. A raw + # key (no prefix) passes through unchanged. + return _resolve_api_key(raw) def _subscription_headers_from_credentials( self, auth: Any, credentials: Any diff --git a/tests/agent_server/test_llm_connections.py b/tests/agent_server/test_llm_connections.py new file mode 100644 index 0000000000..851c85c7ce --- /dev/null +++ b/tests/agent_server/test_llm_connections.py @@ -0,0 +1,278 @@ +"""Tests for the Provider Connection endpoints (OpenHands/OpenHands#15492).""" + +from __future__ import annotations + +import tempfile +from pathlib import Path +from unittest.mock import patch + +import pytest +from fastapi.testclient import TestClient +from pydantic import SecretStr + +from openhands.agent_server import llm_connections as conn_module +from openhands.agent_server.api import create_app +from openhands.agent_server.config import Config +from openhands.agent_server.persistence import ( + FileConnectionsStore, + PersistedConnections, + ProviderConnection, + reset_stores, +) + + +@pytest.fixture +def temp_dirs(): + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + (base / "profiles").mkdir(parents=True, exist_ok=True) + yield base + + +@pytest.fixture +def client(temp_dirs, monkeypatch): + reset_stores() + monkeypatch.setenv("OH_PERSISTENCE_DIR", str(temp_dirs)) + config = Config(static_files_path=None, session_api_keys=[], secret_key=None) + # Patch the connections store to the temp dir (mirrors profiles_router tests). + with patch( + "openhands.agent_server.llm_connections.get_connections_store", + lambda *_a, **_kw: FileConnectionsStore(persistence_dir=temp_dirs), + ): + app = create_app(config) + yield TestClient(app) + reset_stores() + + +def test_list_empty(client): + r = client.get("/api/llm/connections") + assert r.status_code == 200 + assert r.json() == [] + + +def test_create_then_list(client): + r = client.post( + "/api/llm/connections", + json={"provider": "openai", "key": "sk-test", "models": ["gpt-4o"]}, + ) + assert r.status_code == 201 + body = r.json() + assert body["provider"] == "openai" + assert body["models"] == ["gpt-4o"] + assert body["api_key_set"] is True + # Key never echoed. + assert "key" not in body + assert "secret_name" not in body + cid = body["id"] + + r = client.get("/api/llm/connections") + assert r.status_code == 200 + listed = r.json() + assert len(listed) == 1 + assert listed[0]["id"] == cid + + +def test_create_unknown_provider_422(client): + r = client.post( + "/api/llm/connections", + json={"provider": "nope_provider", "key": "k"}, + ) + assert r.status_code == 422 + + +def test_get_connection(client): + cid = client.post( + "/api/llm/connections", json={"provider": "anthropic", "key": "sk-ant"} + ).json()["id"] + r = client.get(f"/api/llm/connections/{cid}") + assert r.status_code == 200 + assert r.json()["provider"] == "anthropic" + assert r.json()["api_key_set"] is True + + +def test_get_missing_404(client): + r = client.get("/api/llm/connections/does-not-exist") + assert r.status_code == 404 + + +def test_patch_rotate_label_models(client): + cid = client.post( + "/api/llm/connections", json={"provider": "openai", "key": "sk-1"} + ).json()["id"] + + r = client.patch( + f"/api/llm/connections/{cid}", + json={"label": "work", "models": ["gpt-4o", "gpt-4o-mini"]}, + ) + assert r.status_code == 200 + body = r.json() + assert body["label"] == "work" + assert body["models"] == ["gpt-4o", "gpt-4o-mini"] + + # Rotate key: api_key_set stays true. + r = client.patch(f"/api/llm/connections/{cid}", json={"key": "sk-2"}) + assert r.status_code == 200 + assert r.json()["api_key_set"] is True + + +def test_patch_requires_a_field(client): + cid = client.post( + "/api/llm/connections", json={"provider": "openai", "key": "sk-1"} + ).json()["id"] + r = client.patch(f"/api/llm/connections/{cid}", json={}) + assert r.status_code == 422 + + +def test_patch_missing_connection_404(client): + r = client.patch( + "/api/llm/connections/none", json={"label": "x"} + ) + # When only label/models are set (no key), the 404 comes from patch(). + assert r.status_code == 404 + + +def test_delete_removes_connection_and_secret(client): + cid = client.post( + "/api/llm/connections", json={"provider": "openai", "key": "sk-1"} + ).json()["id"] + r = client.delete(f"/api/llm/connections/{cid}") + assert r.status_code == 204 + assert client.get(f"/api/llm/connections/{cid}").status_code == 404 + # Listing is empty again. + assert client.get("/api/llm/connections").json() == [] + + +def test_delete_missing_404(client): + assert client.delete("/api/llm/connections/none").status_code == 404 + + +def test_validate_success_stamps_timestamp(client): + cid = client.post( + "/api/llm/connections", + json={"provider": "openai", "key": "sk-test"}, + ).json()["id"] + r = client.post(f"/api/llm/connections/{cid}/validate") + assert r.status_code == 200 + body = r.json() + assert body["ok"] is True + assert body["error"] is None + assert len(body["models"]) > 0 + + # last_validated_at got stamped. + conn = client.get(f"/api/llm/connections/{cid}").json() + assert conn["last_validated_at"] is not None + + +def test_validate_missing_connection_404(client): + assert ( + client.post("/api/llm/connections/none/validate").status_code == 404 + ) + + +def test_validate_uses_injected_validator(client, monkeypatch): + """validate_provider_key is module-level so tests can monkeypatch it.""" + + def fake(provider, key): + return True, ["fake-model-a", "fake-model-b"], None + + monkeypatch.setattr(conn_module, "validate_provider_key", fake) + cid = client.post( + "/api/llm/connections", json={"provider": "openai", "key": "sk"} + ).json()["id"] + body = client.post(f"/api/llm/connections/{cid}/validate").json() + assert body["ok"] is True + assert body["models"] == ["fake-model-a", "fake-model-b"] + + +def test_create_limit_enforced(client, monkeypatch): + monkeypatch.setattr(conn_module, "MAX_CONNECTIONS", 2) + for i in range(2): + assert ( + client.post( + "/api/llm/connections", + json={"provider": "openai", "key": f"sk-{i}"}, + ).status_code + == 201 + ) + r = client.post( + "/api/llm/connections", json={"provider": "openai", "key": "sk-3"} + ) + assert r.status_code == 409 + + +# ── secret-by-name resolution at the LLM layer ──────────────────────────── + + +def test_llm_secret_ref_helpers_roundtrip(): + from openhands.sdk.llm.llm import ( + LLM_SECRET_REF_PREFIX, + llm_secret_ref, + parse_llm_secret_ref, + ) + + ref = llm_secret_ref("llm_connection_abc") + assert ref == f"{LLM_SECRET_REF_PREFIX}llm_connection_abc" + assert parse_llm_secret_ref(ref) == "llm_connection_abc" + # Raw key (no prefix) is not a reference. + assert parse_llm_secret_ref("sk-raw-key") is None + assert parse_llm_secret_ref(None) is None + + +def test_llm_resolves_secret_ref_via_resolver(): + from openhands.sdk.llm.llm import LLM, register_llm_secret_resolver + + register_llm_secret_resolver(lambda name: f"resolved-{name}" if name else None) + try: + llm = LLM(model="gpt-4o", api_key=SecretStr("secret:llm_connection_abc")) + assert llm._get_api_key_value() == "resolved-llm_connection_abc" + finally: + register_llm_secret_resolver(None) + + +def test_llm_secret_ref_without_resolver_is_none(): + from openhands.sdk.llm.llm import LLM, register_llm_secret_resolver + + register_llm_secret_resolver(None) + llm = LLM(model="gpt-4o", api_key=SecretStr("secret:llm_connection_abc")) + assert llm._get_api_key_value() is None + + +def test_llm_raw_key_unaffected_by_resolver(): + from openhands.sdk.llm.llm import LLM, register_llm_secret_resolver + + register_llm_secret_resolver(lambda name: "should-not-be-used") + try: + llm = LLM(model="gpt-4o", api_key=SecretStr("sk-raw-key")) + assert llm._get_api_key_value() == "sk-raw-key" + finally: + register_llm_secret_resolver(None) + + +# ── persistence layer ───────────────────────────────────────────────────── + + +def test_connections_store_roundtrip(temp_dirs): + store = FileConnectionsStore(persistence_dir=temp_dirs) + assert store.load() is None + + conn = ProviderConnection( + id="abc", + provider="openai", + label="work", + secret_name="llm_connection_abc", + models=["gpt-4o"], + created_at=1700000000, + ) + persisted = store.update(lambda c: PersistedConnections(connections=[conn])) + assert persisted.connections[0].id == "abc" + + reloaded = store.load() + assert reloaded is not None + assert reloaded.connections[0].secret_name == "llm_connection_abc" + assert reloaded.schema_version == 1 + + +def test_persisted_connections_schema_version_guard(): + # Newer schema versions are rejected to avoid silent data loss. + with pytest.raises(ValueError): + PersistedConnections.from_persisted({"schema_version": 99, "connections": []}) From aa172e6947b7eaa147c44f4c8d3ad33551524da4 Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 11 Aug 2026 18:49:24 +0000 Subject: [PATCH 3/7] Address review: honest validation, profile-from-connection, safer disconnect/rotate - validate now returns a `verified` flag and only claims a key is authenticated when a live provider probe ran (opt-in via ?live=true / OH_CONNECTIONS_LIVE_VALIDATE); catalog-only responses are explicitly verified=false so the UI/docs stop overstating validation - add POST /connections/{id}/profiles to create an LLM profile bound to a connection's key by reference (secret:), wiring the 'pick from every model the provider offers in Agent Profile' half of #15492 - DELETE now returns the profiles that referenced the connection so clients can warn instead of silently breaking auth - rotate the named secret inside the connections lock, after confirming the record still exists, to avoid orphaned rotated keys under concurrent delete - validate persists the returned catalog onto the connection's models - tidy: top-level imports for PersistedConnections/get_llm_profile_store, document the process-global secret resolver coupling Co-authored-by: openhands --- .../openhands/agent_server/api.py | 8 + .../openhands/agent_server/llm_connections.py | 379 +++++++++++++++--- tests/agent_server/test_llm_connections.py | 79 +++- 3 files changed, 401 insertions(+), 65 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/api.py b/openhands-agent-server/openhands/agent_server/api.py index c3e8e1e1a6..4c029b0209 100644 --- a/openhands-agent-server/openhands/agent_server/api.py +++ b/openhands-agent-server/openhands/agent_server/api.py @@ -700,6 +700,14 @@ def create_app(config: Config | None = None) -> FastAPI: # Register the LLM ``secret:`` resolver so profiles spawned from a # provider connection resolve their api_key against the agent-server's # SecretsStore at call time (see LLM._get_api_key_value). + # + # NOTE: the resolver is a *process-global* installed on the SDK. In the + # normal one-app-per-process deployment this is exactly right. If multiple + # apps are created in one process (e.g. some test setups) the last + # ``create_app`` wins; the resolver reads the config-scoped secrets_store + # captured in the closure, so a stale registration would point at a + # previous app's store. Tests that need isolation should call + # ``register_llm_secret_resolver(None)`` in teardown. from openhands.agent_server.persistence import get_secrets_store from openhands.sdk.llm.llm import register_llm_secret_resolver diff --git a/openhands-agent-server/openhands/agent_server/llm_connections.py b/openhands-agent-server/openhands/agent_server/llm_connections.py index 247ca64b43..deca73bd1a 100644 --- a/openhands-agent-server/openhands/agent_server/llm_connections.py +++ b/openhands-agent-server/openhands/agent_server/llm_connections.py @@ -17,9 +17,16 @@ - POST /connections create {provider, key, label?, models?} - GET /connections/{id} connection + its selected models - PATCH /connections/{id} rotate key / rename label / set models - - DELETE /connections/{id} disconnect (+ delete the named secret) - - POST /connections/{id}/validate test the key, return the provider's - model catalog; updates last_validated_at + - DELETE /connections/{id} disconnect (+ delete the named secret); + returns the profiles that referenced it + - POST /connections/{id}/validate test the key (catalog-only by default, + live probe with ``?live=true`` or + OH_CONNECTIONS_LIVE_VALIDATE) and return + the model catalog; the response carries + ``verified`` so clients never claim an + unchecked key was authenticated + - POST /connections/{id}/profiles create an LLM profile bound to this + connection's key (api_key by reference) LLM profiles spawned from a connection store ``api_key = "secret:"`` (see :func:`openhands.agent_server.persistence.llm_secret_ref`) instead of the @@ -29,6 +36,7 @@ from __future__ import annotations +import os import time import uuid from collections.abc import Callable @@ -38,9 +46,12 @@ from openhands.agent_server._secrets_exposure import get_config from openhands.agent_server.persistence import ( + PersistedConnections, ProviderConnection, get_connections_store, + get_llm_profile_store, get_secrets_store, + llm_secret_ref, ) from openhands.sdk.llm.utils.unverified_models import ( _extract_model_and_provider, @@ -116,16 +127,50 @@ class ConnectionResponse(BaseModel): class ValidateResponse(BaseModel): - """Result of testing a connection's key against the provider's catalog.""" + """Result of testing a connection's key against the provider's catalog. + + ``verified`` distinguishes a real, network-checked key from a catalog-only + response: it is True only when a live probe confirmed the provider accepted + the key. Clients must not present the key as authenticated when ``verified`` + is False (the models are the provider's advertised catalog, not proven grants). + """ id: str provider: str ok: bool + verified: bool = False models: list[str] = Field(default_factory=list) error: str | None = None validated_at: int +class DisconnectResponse(BaseModel): + """Result of a disconnect: which profiles now reference a missing key.""" + + id: str + affected_profiles: list[str] = Field(default_factory=list) + + +class CreateProfileFromConnectionRequest(BaseModel): + """Create an LLM profile that authenticates via this connection's key. + + The profile stores ``api_key = "secret:"`` rather than the + raw key, so rotating the connection updates every profile at once. ``model`` + must be one of the connection's selected/validated models. + """ + + profile_name: str = Field(..., min_length=1, max_length=64) + model: str = Field(..., min_length=1) + base_url: str | None = None + + +class ProfileFromConnectionResponse(BaseModel): + profile_name: str + model: str + provider: str + connection_id: str + + def _to_response(conn: ProviderConnection, *, api_key_set: bool) -> ConnectionResponse: return ConnectionResponse( id=conn.id, @@ -145,6 +190,30 @@ def _api_key_set(secret_name: str) -> bool: return bool(value and value.strip()) +def _profiles_referencing(secret_name: str) -> list[str]: + """Names of LLM profiles whose ``api_key`` points at this connection's secret. + + Used to warn the user before disconnect: these profiles would stop + authenticating once the named secret is deleted. + """ + ref = llm_secret_ref(secret_name) + store = get_llm_profile_store() + referrers: list[str] = [] + for summary in store.list_summaries(): + name = summary.get("name") + if not isinstance(name, str): + continue + try: + llm = store.load(name) + except Exception: # noqa: BLE001 - skip unreadable profiles + continue + api_key = llm.api_key + raw = api_key.get_secret_value() if isinstance(api_key, SecretStr) else None + if raw == ref: + referrers.append(name) + return sorted(referrers) + + def _get_connection_or_404(connections, connection_id: str) -> ProviderConnection: for conn in connections.connections: if conn.id == connection_id: @@ -157,32 +226,34 @@ def _get_connection_or_404(connections, connection_id: str) -> ProviderConnectio # ── Provider key validation (injectable for tests) ─────────────────────── # -# ``validate_provider_key`` attempts to confirm a key works for a provider and -# returns that provider's model catalog. The default implementation is -# conservative: it never makes a live network call (which would be slow and -# non-deterministic in tests); it returns the static LiteLLM catalog for the -# provider when the key is non-empty, so the wizard can populate the picker. -# A production implementation can override this (or a future flag) to issue a -# real cheap probe and surface a 401/403 cause. The function is module-level so -# tests monkeypatch it. +# ``validate_provider_key`` confirms a key looks usable for a provider and +# returns that provider's model catalog. It reports two distinct things via the +# ``ValidationResult`` fields: +# +# - ``ok`` the request could proceed (non-empty key, known provider, and +# — when a live probe runs — the provider did not reject the key) +# - ``verified`` whether the key was actually checked against the provider over +# the network. When no live probe runs, ``verified`` is False and +# the catalog is the provider's *advertised* models, not the ones +# the key is proven to grant. Callers/UI must not claim the key +# was authenticated when ``verified`` is False. +# +# A live probe is opt-in (``OH_CONNECTIONS_LIVE_VALIDATE=1`` or ``live=True`` on +# the endpoint) because it costs a network round-trip and is not always reachable +# from every deployment. The function is module-level so tests monkeypatch it. -ValidateFn = Callable[[str, str], tuple[bool, list[str], str | None]] +ValidateFn = Callable[..., "ValidationResult"] -def validate_provider_key( - provider: str, key: str -) -> tuple[bool, list[str], str | None]: - """Default validator: non-empty key => provider's static model catalog. +class ValidationResult(BaseModel): + ok: bool + models: list[str] = Field(default_factory=list) + error: str | None = None + verified: bool = False - Returns ``(ok, models, error)``. ``ok`` is True for a non-empty key against a - known provider; ``models`` is the provider-filtered LiteLLM catalog; ``error`` - is None on success or a short cause string on failure. - """ - if not key or not key.strip(): - return False, [], "API key is empty" - if provider not in _provider_names(): - return False, [], f"Unknown provider '{provider}'" +def _provider_catalog(provider: str) -> list[str]: + """Return the provider's advertised model catalog (no network call).""" all_models = get_supported_llm_models() verified_provider_models = set(VERIFIED_MODELS.get(provider, ())) filtered: list[str] = [] @@ -190,7 +261,60 @@ def validate_provider_key( model_provider, _, _ = _extract_model_and_provider(model) if model_provider == provider or model in verified_provider_models: filtered.append(model) - return True, sorted(set(filtered)), None + return sorted(set(filtered)) + + +def _live_probe(provider: str, key: str) -> tuple[bool, str | None]: + """Cheaply check a key against a provider over the network. + + Returns ``(ok, error)``. Uses LiteLLM's provider-endpoint check, which lists + the provider's models using the supplied key without spending tokens. An + authentication/permission rejection maps to ``ok=False`` with a short cause; + connectivity problems are surfaced as an error but do not assert the key is + invalid. + """ + import litellm + from litellm.exceptions import AuthenticationError, PermissionDeniedError + + try: + litellm.get_valid_models( + check_provider_endpoint=True, + custom_llm_provider=provider, + api_key=key, + ) + return True, None + except (AuthenticationError, PermissionDeniedError) as e: + return False, f"Provider rejected the key: {str(e)[:200]}" + except Exception as e: # noqa: BLE001 - connectivity/other; don't assert invalid + logger.warning(f"Live validation probe failed for {provider}: {e}") + return False, f"Could not reach {provider} to verify the key: {str(e)[:200]}" + + +def validate_provider_key( + provider: str, key: str, *, live: bool = False +) -> ValidationResult: + """Validate a provider key and return the models it can select. + + With ``live=False`` (default) this performs input checks only and returns the + provider's advertised catalog with ``verified=False`` — it does *not* prove + the key authenticates. With ``live=True`` it additionally issues a cheap + network probe; on success ``verified`` is True. + """ + if not key or not key.strip(): + return ValidationResult(ok=False, models=[], error="API key is empty") + if provider not in _provider_names(): + return ValidationResult( + ok=False, models=[], error=f"Unknown provider '{provider}'" + ) + + catalog = _provider_catalog(provider) + if not live: + return ValidationResult(ok=True, models=catalog, error=None, verified=False) + + ok, error = _live_probe(provider, key) + return ValidationResult( + ok=ok, models=catalog if ok else [], error=error, verified=ok + ) # ── Endpoints ──────────────────────────────────────────────────────────── @@ -311,28 +435,31 @@ async def update_connection( store = get_connections_store(config) secrets_store = get_secrets_store(config) - if body.key is not None: - # Rotate the named secret first; the connection record only references it. - persisted = store.load() or _empty() - conn_to_rotate = _get_connection_or_404(persisted, connection_id) - try: - secrets_store.set_secret( - name=conn_to_rotate.secret_name, - value=body.key.get_secret_value(), - description=( - f"LLM provider connection key for {conn_to_rotate.provider}" - ), - ) - except RuntimeError as e: - logger.error(f"Connection rotate blocked (secrets): {e}") - raise HTTPException( - status_code=500, - detail="Secrets file is corrupted or encrypted with a different key", - ) - def patch(conn_list): + # The whole update runs under the connections lock. We only rotate the + # secret *after* confirming the connection still exists, so a concurrent + # delete can't leave an orphaned rotated key (the earlier version wrote + # the secret before the record check). for c in conn_list.connections: if c.id == connection_id: + if body.key is not None: + try: + secrets_store.set_secret( + name=c.secret_name, + value=body.key.get_secret_value(), + description=( + f"LLM provider connection key for {c.provider}" + ), + ) + except RuntimeError as e: + logger.error(f"Connection rotate blocked (secrets): {e}") + raise HTTPException( + status_code=500, + detail=( + "Secrets file is corrupted or encrypted with a " + "different key" + ), + ) if body.label is not None: c = c.model_copy(update={"label": body.label}) if body.models is not None: @@ -351,9 +478,17 @@ def patch(conn_list): return _to_response(conn, api_key_set=_api_key_set(conn.secret_name)) -@connections_router.delete("/{connection_id}", status_code=status.HTTP_204_NO_CONTENT) -async def delete_connection(request: Request, connection_id: str): - """Disconnect: delete the connection record and its named secret.""" +@connections_router.delete("/{connection_id}", response_model=DisconnectResponse) +async def delete_connection( + request: Request, connection_id: str +) -> DisconnectResponse: + """Disconnect: delete the connection record and its named secret. + + Returns the names of LLM profiles that referenced the connection's key so the + client can warn that they will stop authenticating until pointed at a new key. + The profiles are left intact (deleting them silently would be more surprising + than a clear "these now need a key" message). + """ config = get_config(request) store = get_connections_store(config) secrets_store = get_secrets_store(config) @@ -372,22 +507,53 @@ def remove(conn_list): detail=f"Connection '{connection_id}' not found", ) + affected: list[str] = [] + if deleted_secret_name is None: + # Peek before mutating so we can report referrers in the response. + persisted = store.load() + if persisted is not None: + for c in persisted.connections: + if c.id == connection_id: + affected = _profiles_referencing(c.secret_name) + break + store.update(remove) if deleted_secret_name is not None: try: secrets_store.delete_secret(deleted_secret_name) except Exception: # noqa: BLE001 - record already gone; best-effort logger.warning(f"Failed to delete secret {deleted_secret_name}") - logger.info("Deleted provider connection", extra={"connection_id": connection_id}) + logger.info( + "Deleted provider connection", + extra={"connection_id": connection_id, "affected_profiles": len(affected)}, + ) + return DisconnectResponse(id=connection_id, affected_profiles=affected) + + +def _live_validation_default() -> bool: + """Whether validate should probe the provider live unless told otherwise.""" + return os.getenv("OH_CONNECTIONS_LIVE_VALIDATE", "").strip().lower() in { + "1", + "true", + "yes", + } @connections_router.post( "/{connection_id}/validate", response_model=ValidateResponse ) async def validate_connection( - request: Request, connection_id: str + request: Request, connection_id: str, live: bool | None = None ) -> ValidateResponse: - """Test the connection's key against the provider and return its catalog.""" + """Test the connection's key against the provider and return its catalog. + + When ``live`` is true (or ``OH_CONNECTIONS_LIVE_VALIDATE`` is set) the key is + probed against the provider over the network and ``verified`` reflects the + real result. Otherwise the response is catalog-only with ``verified=false``. + On a successful validation the connection's ``last_validated_at`` is stamped + and its ``models`` are set to the returned catalog so a profile can be spawned + from them without a second call. + """ config = get_config(request) store = get_connections_store(config) secrets_store = get_secrets_store(config) @@ -396,14 +562,20 @@ async def validate_connection( conn = _get_connection_or_404(persisted, connection_id) key = secrets_store.get_secret(conn.secret_name) or "" - ok, models, error = validate_provider_key(conn.provider, key) + do_live = _live_validation_default() if live is None else live + result = validate_provider_key(conn.provider, key, live=do_live) validated_at = _now() - if ok: - # Stamp last_validated_at on success. + if result.ok: + # Persist the catalog + timestamp so profile creation can reuse them. def stamp(conn_list): for c in conn_list.connections: if c.id == connection_id: - c = c.model_copy(update={"last_validated_at": validated_at}) + c = c.model_copy( + update={ + "last_validated_at": validated_at, + "models": list(result.models), + } + ) conn_list.connections = [ c if x.id == connection_id else x for x in conn_list.connections @@ -416,15 +588,100 @@ def stamp(conn_list): return ValidateResponse( id=connection_id, provider=conn.provider, - ok=ok, - models=models, - error=error, + ok=result.ok, + verified=result.verified, + models=result.models, + error=result.error, validated_at=validated_at, ) -def _empty(): - """Return a fresh empty PersistedConnections (avoids ``load() or None`` chains).""" - from openhands.agent_server.persistence import PersistedConnections +@connections_router.post( + "/{connection_id}/profiles", + response_model=ProfileFromConnectionResponse, + status_code=status.HTTP_201_CREATED, +) +async def create_profile_from_connection( + request: Request, + connection_id: str, + body: CreateProfileFromConnectionRequest, +) -> ProfileFromConnectionResponse: + """Create an LLM profile backed by this connection's key. + + This is the "pick from every model the connection offers" step: it saves a + named LLM profile whose ``api_key`` is a ``secret:`` reference to the + connection's stored key, so the profile authenticates without duplicating the + key and follows the key when it is rotated. ``model`` must be one of the + connection's selected/validated models. + """ + from openhands.sdk.llm import LLM + from openhands.sdk.llm.llm_profile_store import ( + PROFILE_NAME_REGEX, + ProfileLimitExceeded, + ) + + config = get_config(request) + store = get_connections_store(config) + + persisted = store.load() or _empty() + conn = _get_connection_or_404(persisted, connection_id) + + if not PROFILE_NAME_REGEX.match(body.profile_name): + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"Invalid profile name '{body.profile_name}'", + ) + if conn.models and body.model not in conn.models: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=( + f"Model '{body.model}' is not one of the connection's selected " + "models. Validate the connection or choose a listed model." + ), + ) + + llm = LLM( + model=body.model, + base_url=body.base_url, + api_key=SecretStr(llm_secret_ref(conn.secret_name)), + usage_id=body.profile_name, + ) + + profile_store = get_llm_profile_store() + from openhands.agent_server.profiles_router import MAX_PROFILES + try: + profile_store.save( + body.profile_name, + llm, + include_secrets=True, + max_profiles=MAX_PROFILES, + ) + except ProfileLimitExceeded: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=( + f"Profile limit reached ({MAX_PROFILES}). " + "Delete a profile before creating a new one." + ), + ) + + logger.info( + "Created profile from connection", + extra={ + "connection_id": connection_id, + "profile_name": body.profile_name, + "model": body.model, + }, + ) + return ProfileFromConnectionResponse( + profile_name=body.profile_name, + model=body.model, + provider=conn.provider, + connection_id=connection_id, + ) + + +def _empty() -> PersistedConnections: + """Return a fresh empty PersistedConnections (avoids ``load() or None`` chains).""" return PersistedConnections() diff --git a/tests/agent_server/test_llm_connections.py b/tests/agent_server/test_llm_connections.py index 851c85c7ce..c003453405 100644 --- a/tests/agent_server/test_llm_connections.py +++ b/tests/agent_server/test_llm_connections.py @@ -136,7 +136,11 @@ def test_delete_removes_connection_and_secret(client): "/api/llm/connections", json={"provider": "openai", "key": "sk-1"} ).json()["id"] r = client.delete(f"/api/llm/connections/{cid}") - assert r.status_code == 204 + assert r.status_code == 200 + body = r.json() + assert body["id"] == cid + # No profiles reference this connection, so nothing is affected. + assert body["affected_profiles"] == [] assert client.get(f"/api/llm/connections/{cid}").status_code == 404 # Listing is empty again. assert client.get("/api/llm/connections").json() == [] @@ -157,10 +161,13 @@ def test_validate_success_stamps_timestamp(client): assert body["ok"] is True assert body["error"] is None assert len(body["models"]) > 0 + # Catalog-only validation must not claim the key was network-verified. + assert body["verified"] is False - # last_validated_at got stamped. + # last_validated_at got stamped and the catalog was persisted onto models. conn = client.get(f"/api/llm/connections/{cid}").json() assert conn["last_validated_at"] is not None + assert len(conn["models"]) > 0 def test_validate_missing_connection_404(client): @@ -172,8 +179,13 @@ def test_validate_missing_connection_404(client): def test_validate_uses_injected_validator(client, monkeypatch): """validate_provider_key is module-level so tests can monkeypatch it.""" - def fake(provider, key): - return True, ["fake-model-a", "fake-model-b"], None + def fake(provider, key, *, live=False): + return conn_module.ValidationResult( + ok=True, + models=["fake-model-a", "fake-model-b"], + error=None, + verified=True, + ) monkeypatch.setattr(conn_module, "validate_provider_key", fake) cid = client.post( @@ -181,9 +193,68 @@ def fake(provider, key): ).json()["id"] body = client.post(f"/api/llm/connections/{cid}/validate").json() assert body["ok"] is True + assert body["verified"] is True assert body["models"] == ["fake-model-a", "fake-model-b"] +def test_validate_live_flag_marks_verified(client, monkeypatch): + """The ``live`` query flag drives a real probe and sets ``verified``.""" + calls: list[bool] = [] + + def fake(provider, key, *, live=False): + calls.append(live) + return conn_module.ValidationResult( + ok=live, models=["m1"] if live else [], error=None, verified=live + ) + + monkeypatch.setattr(conn_module, "validate_provider_key", fake) + cid = client.post( + "/api/llm/connections", json={"provider": "openai", "key": "sk"} + ).json()["id"] + body = client.post(f"/api/llm/connections/{cid}/validate?live=true").json() + assert calls == [True] + assert body["verified"] is True + + +def test_create_profile_from_connection(client): + """A connection can spawn an LLM profile that references its key by name.""" + cid = client.post( + "/api/llm/connections", + json={"provider": "openai", "key": "sk-test", "models": ["gpt-4o"]}, + ).json()["id"] + + r = client.post( + f"/api/llm/connections/{cid}/profiles", + json={"profile_name": "work-gpt4o", "model": "gpt-4o"}, + ) + assert r.status_code == 201 + body = r.json() + assert body["profile_name"] == "work-gpt4o" + assert body["model"] == "gpt-4o" + assert body["connection_id"] == cid + + # The profile is saved and references the connection secret by name, so its + # api_key resolves through the connection rather than duplicating the key. + detail = client.get("/api/profiles/work-gpt4o").json() + assert detail["api_key_set"] is True + + # Deleting the connection now reports the referencing profile. + deleted = client.delete(f"/api/llm/connections/{cid}").json() + assert "work-gpt4o" in deleted["affected_profiles"] + + +def test_create_profile_rejects_model_not_in_catalog(client): + cid = client.post( + "/api/llm/connections", + json={"provider": "openai", "key": "sk-test", "models": ["gpt-4o"]}, + ).json()["id"] + r = client.post( + f"/api/llm/connections/{cid}/profiles", + json={"profile_name": "nope", "model": "not-a-model"}, + ) + assert r.status_code == 422 + + def test_create_limit_enforced(client, monkeypatch): monkeypatch.setattr(conn_module, "MAX_CONNECTIONS", 2) for i in range(2): From 9a4976838ffe315c6c0095529496f29ab1cc2c6a Mon Sep 17 00:00:00 2001 From: openhands Date: Wed, 12 Aug 2026 18:44:09 -0300 Subject: [PATCH 4/7] Support endpoint settings on provider connections --- .../openhands/agent_server/llm_connections.py | 103 ++++++++++++---- .../agent_server/persistence/models.py | 3 + tests/agent_server/test_llm_connections.py | 116 ++++++++++++++++-- 3 files changed, 188 insertions(+), 34 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/llm_connections.py b/openhands-agent-server/openhands/agent_server/llm_connections.py index deca73bd1a..8e48a9bfd1 100644 --- a/openhands-agent-server/openhands/agent_server/llm_connections.py +++ b/openhands-agent-server/openhands/agent_server/llm_connections.py @@ -40,6 +40,7 @@ import time import uuid from collections.abc import Callable +from typing import Literal from fastapi import APIRouter, HTTPException, Request, status from pydantic import BaseModel, Field, SecretStr @@ -98,19 +99,25 @@ class ConnectionCreateRequest(BaseModel): provider: str = Field(..., min_length=1, max_length=128) key: SecretStr = Field(..., min_length=1) label: str | None = Field(default=None, max_length=128) + base_url: str | None = Field(default=None, max_length=2048) + api_mode: Literal["auto", "chat", "responses"] = "auto" + custom_headers: dict[str, str] = Field(default_factory=dict) models: list[str] = Field(default_factory=list) class ConnectionUpdateRequest(BaseModel): """Partial update a connection. - ``key`` rotates the named secret (rewrites the SecretsStore entry). ``label`` - and ``models`` are straightforward field updates. At least one field is - required. + ``key`` rotates the named secret (rewrites the SecretsStore entry). + ``label``, endpoint settings, and ``models`` are straightforward field + updates. At least one field is required. """ key: SecretStr | None = None label: str | None = None + base_url: str | None = Field(default=None, max_length=2048) + api_mode: Literal["auto", "chat", "responses"] | None = None + custom_headers: dict[str, str] | None = None models: list[str] | None = None @@ -120,6 +127,9 @@ class ConnectionResponse(BaseModel): id: str provider: str label: str | None = None + base_url: str | None = None + api_mode: Literal["auto", "chat", "responses"] = "auto" + custom_headers: dict[str, str] = Field(default_factory=dict) models: list[str] = Field(default_factory=list) created_at: int last_validated_at: int | None = None @@ -161,6 +171,8 @@ class CreateProfileFromConnectionRequest(BaseModel): profile_name: str = Field(..., min_length=1, max_length=64) model: str = Field(..., min_length=1) + # Optional per-profile override. If omitted, the connection endpoint + # settings are used. base_url: str | None = None @@ -172,10 +184,16 @@ class ProfileFromConnectionResponse(BaseModel): def _to_response(conn: ProviderConnection, *, api_key_set: bool) -> ConnectionResponse: + api_mode = ( + conn.api_mode if conn.api_mode in {"auto", "chat", "responses"} else "auto" + ) return ConnectionResponse( id=conn.id, provider=conn.provider, label=conn.label, + base_url=conn.base_url, + api_mode=api_mode, + custom_headers=dict(conn.custom_headers), models=list(conn.models), created_at=conn.created_at, last_validated_at=conn.last_validated_at, @@ -264,7 +282,13 @@ def _provider_catalog(provider: str) -> list[str]: return sorted(set(filtered)) -def _live_probe(provider: str, key: str) -> tuple[bool, str | None]: +def _live_probe( + provider: str, + key: str, + *, + base_url: str | None = None, + custom_headers: dict[str, str] | None = None, +) -> tuple[bool, str | None]: """Cheaply check a key against a provider over the network. Returns ``(ok, error)``. Uses LiteLLM's provider-endpoint check, which lists @@ -281,6 +305,8 @@ def _live_probe(provider: str, key: str) -> tuple[bool, str | None]: check_provider_endpoint=True, custom_llm_provider=provider, api_key=key, + api_base=base_url, + extra_headers=custom_headers or None, ) return True, None except (AuthenticationError, PermissionDeniedError) as e: @@ -291,7 +317,12 @@ def _live_probe(provider: str, key: str) -> tuple[bool, str | None]: def validate_provider_key( - provider: str, key: str, *, live: bool = False + provider: str, + key: str, + *, + live: bool = False, + base_url: str | None = None, + custom_headers: dict[str, str] | None = None, ) -> ValidationResult: """Validate a provider key and return the models it can select. @@ -311,7 +342,12 @@ def validate_provider_key( if not live: return ValidationResult(ok=True, models=catalog, error=None, verified=False) - ok, error = _live_probe(provider, key) + ok, error = _live_probe( + provider, + key, + base_url=base_url, + custom_headers=custom_headers, + ) return ValidationResult( ok=ok, models=catalog if ok else [], error=error, verified=ok ) @@ -327,9 +363,7 @@ async def list_connections(request: Request) -> list[ConnectionResponse]: store = get_connections_store(config) persisted = store.load() conns = persisted.connections if persisted is not None else [] - return [ - _to_response(c, api_key_set=_api_key_set(c.secret_name)) for c in conns - ] + return [_to_response(c, api_key_set=_api_key_set(c.secret_name)) for c in conns] @connections_router.post( @@ -367,6 +401,9 @@ def add(conn_list): id=connection_id, provider=body.provider, label=body.label, + base_url=body.base_url, + api_mode=body.api_mode, + custom_headers=dict(body.custom_headers), secret_name=secret_name, models=list(body.models), created_at=_now(), @@ -425,10 +462,20 @@ async def update_connection( request: Request, connection_id: str, body: ConnectionUpdateRequest ) -> ConnectionResponse: """Update a connection: rotate key, rename label, or set selected models.""" - if body.key is None and body.label is None and body.models is None: + if ( + body.key is None + and body.label is None + and body.base_url is None + and body.api_mode is None + and body.custom_headers is None + and body.models is None + ): raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail="Provide at least one of: key, label, models", + detail=( + "Provide at least one of: key, label, base_url, api_mode, " + "custom_headers, models" + ), ) config = get_config(request) @@ -462,6 +509,14 @@ def patch(conn_list): ) if body.label is not None: c = c.model_copy(update={"label": body.label}) + if body.base_url is not None: + c = c.model_copy(update={"base_url": body.base_url}) + if body.api_mode is not None: + c = c.model_copy(update={"api_mode": body.api_mode}) + if body.custom_headers is not None: + c = c.model_copy( + update={"custom_headers": dict(body.custom_headers)} + ) if body.models is not None: c = c.model_copy(update={"models": list(body.models)}) conn_list.connections = [ @@ -479,9 +534,7 @@ def patch(conn_list): @connections_router.delete("/{connection_id}", response_model=DisconnectResponse) -async def delete_connection( - request: Request, connection_id: str -) -> DisconnectResponse: +async def delete_connection(request: Request, connection_id: str) -> DisconnectResponse: """Disconnect: delete the connection record and its named secret. Returns the names of LLM profiles that referenced the connection's key so the @@ -539,9 +592,7 @@ def _live_validation_default() -> bool: } -@connections_router.post( - "/{connection_id}/validate", response_model=ValidateResponse -) +@connections_router.post("/{connection_id}/validate", response_model=ValidateResponse) async def validate_connection( request: Request, connection_id: str, live: bool | None = None ) -> ValidateResponse: @@ -563,7 +614,13 @@ async def validate_connection( key = secrets_store.get_secret(conn.secret_name) or "" do_live = _live_validation_default() if live is None else live - result = validate_provider_key(conn.provider, key, live=do_live) + result = validate_provider_key( + conn.provider, + key, + live=do_live, + base_url=conn.base_url, + custom_headers=conn.custom_headers, + ) validated_at = _now() if result.ok: # Persist the catalog + timestamp so profile creation can reuse them. @@ -577,8 +634,7 @@ def stamp(conn_list): } ) conn_list.connections = [ - c if x.id == connection_id else x - for x in conn_list.connections + c if x.id == connection_id else x for x in conn_list.connections ] return conn_list return conn_list @@ -640,9 +696,14 @@ async def create_profile_from_connection( ), ) + api_mode = ( + conn.api_mode if conn.api_mode in {"auto", "chat", "responses"} else "auto" + ) llm = LLM( model=body.model, - base_url=body.base_url, + base_url=body.base_url or conn.base_url, + api_mode=api_mode, + extra_headers=dict(conn.custom_headers) or None, api_key=SecretStr(llm_secret_ref(conn.secret_name)), usage_id=body.profile_name, ) diff --git a/openhands-agent-server/openhands/agent_server/persistence/models.py b/openhands-agent-server/openhands/agent_server/persistence/models.py index 1fbf157d3d..12edbacab9 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/models.py +++ b/openhands-agent-server/openhands/agent_server/persistence/models.py @@ -582,6 +582,9 @@ class ProviderConnection(BaseModel): id: str = Field(..., min_length=1, max_length=128) provider: str = Field(..., min_length=1, max_length=128) label: str | None = Field(default=None, max_length=128) + base_url: str | None = Field(default=None, max_length=2048) + api_mode: str = Field(default="auto") + custom_headers: dict[str, str] = Field(default_factory=dict) secret_name: str = Field(..., min_length=1, max_length=128) models: list[str] = Field(default_factory=list) created_at: int = Field(..., description="Unix epoch seconds.") diff --git a/tests/agent_server/test_llm_connections.py b/tests/agent_server/test_llm_connections.py index c003453405..217f5101fa 100644 --- a/tests/agent_server/test_llm_connections.py +++ b/tests/agent_server/test_llm_connections.py @@ -53,11 +53,23 @@ def test_list_empty(client): def test_create_then_list(client): r = client.post( "/api/llm/connections", - json={"provider": "openai", "key": "sk-test", "models": ["gpt-4o"]}, + json={ + "provider": "openai", + "key": "sk-test", + "label": "work", + "base_url": "https://proxy.example/v1", + "api_mode": "chat", + "custom_headers": {"X-Org": "eng"}, + "models": ["gpt-4o"], + }, ) assert r.status_code == 201 body = r.json() assert body["provider"] == "openai" + assert body["label"] == "work" + assert body["base_url"] == "https://proxy.example/v1" + assert body["api_mode"] == "chat" + assert body["custom_headers"] == {"X-Org": "eng"} assert body["models"] == ["gpt-4o"] assert body["api_key_set"] is True # Key never echoed. @@ -70,6 +82,9 @@ def test_create_then_list(client): listed = r.json() assert len(listed) == 1 assert listed[0]["id"] == cid + assert listed[0]["base_url"] == "https://proxy.example/v1" + assert listed[0]["api_mode"] == "chat" + assert listed[0]["custom_headers"] == {"X-Org": "eng"} def test_create_unknown_provider_422(client): @@ -102,11 +117,20 @@ def test_patch_rotate_label_models(client): r = client.patch( f"/api/llm/connections/{cid}", - json={"label": "work", "models": ["gpt-4o", "gpt-4o-mini"]}, + json={ + "label": "work", + "base_url": "https://proxy.example/v1", + "api_mode": "responses", + "custom_headers": {"X-Team": "platform"}, + "models": ["gpt-4o", "gpt-4o-mini"], + }, ) assert r.status_code == 200 body = r.json() assert body["label"] == "work" + assert body["base_url"] == "https://proxy.example/v1" + assert body["api_mode"] == "responses" + assert body["custom_headers"] == {"X-Team": "platform"} assert body["models"] == ["gpt-4o", "gpt-4o-mini"] # Rotate key: api_key_set stays true. @@ -124,9 +148,7 @@ def test_patch_requires_a_field(client): def test_patch_missing_connection_404(client): - r = client.patch( - "/api/llm/connections/none", json={"label": "x"} - ) + r = client.patch("/api/llm/connections/none", json={"label": "x"}) # When only label/models are set (no key), the 404 comes from patch(). assert r.status_code == 404 @@ -171,15 +193,13 @@ def test_validate_success_stamps_timestamp(client): def test_validate_missing_connection_404(client): - assert ( - client.post("/api/llm/connections/none/validate").status_code == 404 - ) + assert client.post("/api/llm/connections/none/validate").status_code == 404 def test_validate_uses_injected_validator(client, monkeypatch): """validate_provider_key is module-level so tests can monkeypatch it.""" - def fake(provider, key, *, live=False): + def fake(provider, key, *, live=False, base_url=None, custom_headers=None): return conn_module.ValidationResult( ok=True, models=["fake-model-a", "fake-model-b"], @@ -201,7 +221,7 @@ def test_validate_live_flag_marks_verified(client, monkeypatch): """The ``live`` query flag drives a real probe and sets ``verified``.""" calls: list[bool] = [] - def fake(provider, key, *, live=False): + def fake(provider, key, *, live=False, base_url=None, custom_headers=None): calls.append(live) return conn_module.ValidationResult( ok=live, models=["m1"] if live else [], error=None, verified=live @@ -216,6 +236,46 @@ def fake(provider, key, *, live=False): assert body["verified"] is True +def test_validate_passes_endpoint_settings(client, monkeypatch): + calls: list[dict[str, object]] = [] + + def fake(provider, key, *, live=False, base_url=None, custom_headers=None): + calls.append( + { + "provider": provider, + "live": live, + "base_url": base_url, + "custom_headers": custom_headers, + } + ) + return conn_module.ValidationResult( + ok=True, models=["gpt-4o"], error=None, verified=live + ) + + monkeypatch.setattr(conn_module, "validate_provider_key", fake) + cid = client.post( + "/api/llm/connections", + json={ + "provider": "openai", + "key": "sk", + "base_url": "https://proxy.example/v1", + "custom_headers": {"X-Org": "eng"}, + }, + ).json()["id"] + + body = client.post(f"/api/llm/connections/{cid}/validate?live=true").json() + + assert body["ok"] is True + assert calls == [ + { + "provider": "openai", + "live": True, + "base_url": "https://proxy.example/v1", + "custom_headers": {"X-Org": "eng"}, + } + ] + + def test_create_profile_from_connection(client): """A connection can spawn an LLM profile that references its key by name.""" cid = client.post( @@ -237,6 +297,7 @@ def test_create_profile_from_connection(client): # api_key resolves through the connection rather than duplicating the key. detail = client.get("/api/profiles/work-gpt4o").json() assert detail["api_key_set"] is True + assert detail["config"]["api_mode"] == "auto" # Deleting the connection now reports the referencing profile. deleted = client.delete(f"/api/llm/connections/{cid}").json() @@ -255,6 +316,31 @@ def test_create_profile_rejects_model_not_in_catalog(client): assert r.status_code == 422 +def test_create_profile_inherits_connection_endpoint_settings(client): + cid = client.post( + "/api/llm/connections", + json={ + "provider": "openai", + "key": "sk-test", + "base_url": "https://proxy.example/v1", + "api_mode": "responses", + "custom_headers": {"X-Org": "eng"}, + "models": ["gpt-4o"], + }, + ).json()["id"] + + r = client.post( + f"/api/llm/connections/{cid}/profiles", + json={"profile_name": "gateway-gpt4o", "model": "gpt-4o"}, + ) + assert r.status_code == 201 + + detail = client.get("/api/profiles/gateway-gpt4o").json() + assert detail["config"]["base_url"] == "https://proxy.example/v1" + assert detail["config"]["api_mode"] == "responses" + assert detail["config"]["extra_headers"] == {"X-Org": "eng"} + + def test_create_limit_enforced(client, monkeypatch): monkeypatch.setattr(conn_module, "MAX_CONNECTIONS", 2) for i in range(2): @@ -265,9 +351,7 @@ def test_create_limit_enforced(client, monkeypatch): ).status_code == 201 ) - r = client.post( - "/api/llm/connections", json={"provider": "openai", "key": "sk-3"} - ) + r = client.post("/api/llm/connections", json={"provider": "openai", "key": "sk-3"}) assert r.status_code == 409 @@ -330,6 +414,9 @@ def test_connections_store_roundtrip(temp_dirs): id="abc", provider="openai", label="work", + base_url="https://proxy.example/v1", + api_mode="chat", + custom_headers={"X-Org": "eng"}, secret_name="llm_connection_abc", models=["gpt-4o"], created_at=1700000000, @@ -340,6 +427,9 @@ def test_connections_store_roundtrip(temp_dirs): reloaded = store.load() assert reloaded is not None assert reloaded.connections[0].secret_name == "llm_connection_abc" + assert reloaded.connections[0].base_url == "https://proxy.example/v1" + assert reloaded.connections[0].api_mode == "chat" + assert reloaded.connections[0].custom_headers == {"X-Org": "eng"} assert reloaded.schema_version == 1 From 4f41b7fe7a3d092b18d177db8852caa27fb6b122 Mon Sep 17 00:00:00 2001 From: openhands Date: Thu, 13 Aug 2026 11:22:54 -0300 Subject: [PATCH 5/7] Support OpenHands provider connections --- .../openhands/agent_server/llm_connections.py | 136 +++++++++++++++++- tests/agent_server/test_llm_connections.py | 77 +++++++++- 2 files changed, 206 insertions(+), 7 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/llm_connections.py b/openhands-agent-server/openhands/agent_server/llm_connections.py index 8e48a9bfd1..1f874e0cd7 100644 --- a/openhands-agent-server/openhands/agent_server/llm_connections.py +++ b/openhands-agent-server/openhands/agent_server/llm_connections.py @@ -87,9 +87,32 @@ def _now() -> int: def _provider_names() -> set[str]: + return _get_litellm_provider_names() | set(VERIFIED_MODELS) + + +def _litellm_provider_names() -> set[str]: return _get_litellm_provider_names() +def _model_provider(model: str) -> str | None: + provider, _, _ = _extract_model_and_provider(model) + if provider: + return provider + if "/" in model: + prefix = model.split("/", 1)[0].strip() + return prefix or None + return None + + +def _model_name_for_provider(provider: str, model: str) -> str: + prefix = f"{provider}/" + return model[len(prefix) :] if model.startswith(prefix) else model + + +def _qualified_model(provider: str, model: str) -> str: + return model if "/" in model else f"{provider}/{model}" + + # ── Request / Response models ──────────────────────────────────────────── @@ -232,6 +255,105 @@ def _profiles_referencing(secret_name: str) -> list[str]: return sorted(referrers) +def _backfill_connections_from_raw_profiles( + persisted: PersistedConnections, +) -> PersistedConnections: + """Promote existing raw-key LLM profiles into provider connections. + + Early versions of the onboarding flow saved only an LLM profile with a raw + ``api_key``. Once Provider Connections exist, those profiles should appear + in Model providers and should reference the shared named secret. This helper + performs that one-time local migration during connection listing: + + - raw profile key -> named secret + - profile api_key -> ``secret:`` + - connection.models includes the profile's current model + + Profiles already using ``secret:`` are left unchanged, so repeated + calls are idempotent. + """ + profile_store = get_llm_profile_store() + secrets_store = get_secrets_store() + existing_secret_names = {c.secret_name for c in persisted.connections} + + try: + summaries = profile_store.list_summaries() + except Exception: # noqa: BLE001 - listing should not fail provider page + return persisted + + changed = False + for summary in summaries: + name = summary.get("name") + if not isinstance(name, str): + continue + try: + llm = profile_store.load(name) + except Exception: # noqa: BLE001 - skip unreadable profiles + continue + + raw_key = ( + llm.api_key.get_secret_value() + if isinstance(llm.api_key, SecretStr) + else "" + ) + if not raw_key or raw_key.startswith("secret:"): + continue + + provider = _model_provider(llm.model) + if provider not in _provider_names(): + continue + if len(persisted.connections) >= MAX_CONNECTIONS: + logger.warning("Skipping profile-to-connection backfill: limit reached") + break + + connection_id = uuid.uuid4().hex + secret_name = _connection_secret_name(connection_id) + if secret_name in existing_secret_names: + continue + model_name = _model_name_for_provider(provider, llm.model) + + try: + secrets_store.set_secret( + name=secret_name, + value=raw_key, + description=f"LLM provider connection key for {provider}", + ) + migrated_llm = llm.model_copy( + update={"api_key": SecretStr(llm_secret_ref(secret_name))} + ) + profile_store.save(name, migrated_llm, include_secrets=True) + except Exception as e: # noqa: BLE001 - preserve the original profile + logger.warning(f"Failed to backfill provider connection for {name}: {e}") + try: + secrets_store.delete_secret(secret_name) + except Exception: # noqa: BLE001 - best-effort cleanup + pass + continue + + persisted.connections.append( + ProviderConnection( + id=connection_id, + provider=provider, + label=name, + base_url=llm.base_url, + api_mode=( + llm.api_mode + if llm.api_mode in {"auto", "chat", "responses"} + else "auto" + ), + custom_headers=dict(llm.extra_headers or {}), + secret_name=secret_name, + models=[model_name], + created_at=_now(), + last_validated_at=None, + ) + ) + existing_secret_names.add(secret_name) + changed = True + + return persisted if changed else persisted + + def _get_connection_or_404(connections, connection_id: str) -> ProviderConnection: for conn in connections.connections: if conn.id == connection_id: @@ -278,7 +400,8 @@ def _provider_catalog(provider: str) -> list[str]: for model in all_models: model_provider, _, _ = _extract_model_and_provider(model) if model_provider == provider or model in verified_provider_models: - filtered.append(model) + filtered.append(_model_name_for_provider(provider, model)) + filtered.extend(verified_provider_models) return sorted(set(filtered)) @@ -339,7 +462,7 @@ def validate_provider_key( ) catalog = _provider_catalog(provider) - if not live: + if not live or provider not in _litellm_provider_names(): return ValidationResult(ok=True, models=catalog, error=None, verified=False) ok, error = _live_probe( @@ -361,7 +484,7 @@ async def list_connections(request: Request) -> list[ConnectionResponse]: """List all saved provider connections (keys never returned).""" config = get_config(request) store = get_connections_store(config) - persisted = store.load() + persisted = store.update(_backfill_connections_from_raw_profiles) conns = persisted.connections if persisted is not None else [] return [_to_response(c, api_key_set=_api_key_set(c.secret_name)) for c in conns] @@ -699,8 +822,9 @@ async def create_profile_from_connection( api_mode = ( conn.api_mode if conn.api_mode in {"auto", "chat", "responses"} else "auto" ) + saved_model = _qualified_model(conn.provider, body.model) llm = LLM( - model=body.model, + model=saved_model, base_url=body.base_url or conn.base_url, api_mode=api_mode, extra_headers=dict(conn.custom_headers) or None, @@ -732,12 +856,12 @@ async def create_profile_from_connection( extra={ "connection_id": connection_id, "profile_name": body.profile_name, - "model": body.model, + "model": saved_model, }, ) return ProfileFromConnectionResponse( profile_name=body.profile_name, - model=body.model, + model=saved_model, provider=conn.provider, connection_id=connection_id, ) diff --git a/tests/agent_server/test_llm_connections.py b/tests/agent_server/test_llm_connections.py index 217f5101fa..ae6275e998 100644 --- a/tests/agent_server/test_llm_connections.py +++ b/tests/agent_server/test_llm_connections.py @@ -17,8 +17,10 @@ FileConnectionsStore, PersistedConnections, ProviderConnection, + get_llm_profile_store, reset_stores, ) +from openhands.sdk.llm import LLM @pytest.fixture @@ -95,6 +97,31 @@ def test_create_unknown_provider_422(client): assert r.status_code == 422 +def test_create_accepts_verified_provider_namespace(client): + r = client.post( + "/api/llm/connections", + json={"provider": "openhands", "key": "oh-key"}, + ) + assert r.status_code == 201 + assert r.json()["provider"] == "openhands" + + +def test_validate_verified_provider_namespace_is_catalog_only(client): + cid = client.post( + "/api/llm/connections", + json={"provider": "openhands", "key": "oh-key"}, + ).json()["id"] + + r = client.post(f"/api/llm/connections/{cid}/validate?live=true") + + assert r.status_code == 200 + body = r.json() + assert body["ok"] is True + assert body["verified"] is False + assert len(body["models"]) > 0 + assert all(not model.startswith("openhands/") for model in body["models"]) + + def test_get_connection(client): cid = client.post( "/api/llm/connections", json={"provider": "anthropic", "key": "sk-ant"} @@ -290,13 +317,14 @@ def test_create_profile_from_connection(client): assert r.status_code == 201 body = r.json() assert body["profile_name"] == "work-gpt4o" - assert body["model"] == "gpt-4o" + assert body["model"] == "openai/gpt-4o" assert body["connection_id"] == cid # The profile is saved and references the connection secret by name, so its # api_key resolves through the connection rather than duplicating the key. detail = client.get("/api/profiles/work-gpt4o").json() assert detail["api_key_set"] is True + assert detail["config"]["model"] == "openai/gpt-4o" assert detail["config"]["api_mode"] == "auto" # Deleting the connection now reports the referencing profile. @@ -316,6 +344,27 @@ def test_create_profile_rejects_model_not_in_catalog(client): assert r.status_code == 422 +def test_create_profile_from_openhands_connection_qualifies_model(client): + cid = client.post( + "/api/llm/connections", + json={ + "provider": "openhands", + "key": "oh-key", + "models": ["gpt-5.6"], + }, + ).json()["id"] + + r = client.post( + f"/api/llm/connections/{cid}/profiles", + json={"profile_name": "openhands-gpt", "model": "gpt-5.6"}, + ) + + assert r.status_code == 201 + assert r.json()["model"] == "openhands/gpt-5.6" + detail = client.get("/api/profiles/openhands-gpt").json() + assert detail["config"]["model"] == "openhands/gpt-5.6" + + def test_create_profile_inherits_connection_endpoint_settings(client): cid = client.post( "/api/llm/connections", @@ -336,11 +385,37 @@ def test_create_profile_inherits_connection_endpoint_settings(client): assert r.status_code == 201 detail = client.get("/api/profiles/gateway-gpt4o").json() + assert detail["config"]["model"] == "openai/gpt-4o" assert detail["config"]["base_url"] == "https://proxy.example/v1" assert detail["config"]["api_mode"] == "responses" assert detail["config"]["extra_headers"] == {"X-Org": "eng"} +def test_list_backfills_raw_profile_as_provider_connection(client): + profile_store = get_llm_profile_store() + profile_store.save( + "openhands-gpt", + LLM(model="openhands/gpt-5.6", api_key=SecretStr("oh-key")), + include_secrets=True, + ) + + r = client.get("/api/llm/connections") + + assert r.status_code == 200 + body = r.json() + assert len(body) == 1 + assert body[0]["provider"] == "openhands" + assert body[0]["label"] == "openhands-gpt" + assert body[0]["models"] == ["gpt-5.6"] + assert body[0]["api_key_set"] is True + + # The migration is idempotent because the profile now points at the + # connection's named secret instead of keeping the raw key. + assert len(client.get("/api/llm/connections").json()) == 1 + detail = client.get("/api/profiles/openhands-gpt").json() + assert detail["api_key_set"] is True + + def test_create_limit_enforced(client, monkeypatch): monkeypatch.setattr(conn_module, "MAX_CONNECTIONS", 2) for i in range(2): From 6d48ae8f89db19d1d78bfd6b3f7362d4d316abf1 Mon Sep 17 00:00:00 2001 From: openhands Date: Thu, 13 Aug 2026 12:21:24 -0300 Subject: [PATCH 6/7] Raise profile limits to 500 --- .../openhands/agent_server/agent_profiles_router.py | 2 +- .../openhands/agent_server/profiles_router.py | 2 +- tests/agent_server/test_agent_profiles_router.py | 9 ++++++--- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py index 9a448c05e7..30e86cda60 100644 --- a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -54,7 +54,7 @@ agent_profiles_router = APIRouter(prefix="/agent-profiles", tags=["Agent Profiles"]) -MAX_AGENT_PROFILES = 50 +MAX_AGENT_PROFILES = 500 ProfileName = Annotated[ str, diff --git a/openhands-agent-server/openhands/agent_server/profiles_router.py b/openhands-agent-server/openhands/agent_server/profiles_router.py index cea019baee..65d7a62aec 100644 --- a/openhands-agent-server/openhands/agent_server/profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/profiles_router.py @@ -37,7 +37,7 @@ profiles_router = APIRouter(prefix="/profiles", tags=["Profiles"]) -MAX_PROFILES = 50 +MAX_PROFILES = 500 ProfileName = Annotated[ str, diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index 735be21376..c2ab1c3825 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -19,7 +19,6 @@ from openhands.agent_server.api import create_app from openhands.agent_server.config import Config from openhands.agent_server.persistence import reset_stores -from openhands.agent_server.profiles_router import MAX_PROFILES from openhands.sdk.llm import LLM from openhands.sdk.llm.llm_profile_store import LLMProfileStore from openhands.sdk.profiles import ( @@ -219,7 +218,9 @@ def test_seed_does_not_clobber_differently_cased_default_llm_profile( assert reloaded.model == "existing/model" -def test_seed_llm_profile_limit_reached_does_not_500(client, default_llm_profile_store): +def test_seed_llm_profile_limit_reached_does_not_500( + client, default_llm_profile_store, monkeypatch +): """Hitting the LLM profile cap during backfill warns and continues instead of 500ing. @@ -229,7 +230,9 @@ def test_seed_llm_profile_limit_reached_does_not_500(client, default_llm_profile (``openhands.sdk.profiles``) — catching the wrong class let the real one propagate as an unhandled 500. """ - for i in range(MAX_PROFILES): + monkeypatch.setattr(router_module, "MAX_PROFILES", 2) + + for i in range(2): default_llm_profile_store.save(f"other-{i}", LLM(model="x")) response = client.get("/api/agent-profiles") From 8c3a9c82abaeb319cc4a7e4b387de0d04a8793d0 Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 14 Aug 2026 04:36:15 +0000 Subject: [PATCH 7/7] Rework provider connections into provider-centric model-provider store Replace the auto-catalog connection design with a provider-first model: providers hold one key (named secret) and a nested, user-managed model list. - persistence: ModelProvider/ProviderModel/PersistedProviders + FileProvidersStore - llm_providers.py: /api/llm/model-providers CRUD + nested model CRUD + optional key-probe test (never mutates the curated model list) - Remove the core-LLM global secret resolver, the backfill-on-GET that rewrote user LLM profiles, and validate() clobbering the model list - Keep named-secret key storage (secret_name + api_key_set; key never returned) Co-authored-by: openhands --- .../openhands/agent_server/api.py | 20 +- .../openhands/agent_server/llm_connections.py | 872 ------------------ .../openhands/agent_server/llm_providers.py | 581 ++++++++++++ .../agent_server/persistence/__init__.py | 32 +- .../agent_server/persistence/models.py | 88 +- .../agent_server/persistence/store.py | 88 +- openhands-sdk/openhands/sdk/llm/llm.py | 71 +- tests/agent_server/test_llm_connections.py | 514 ----------- tests/agent_server/test_llm_providers.py | 219 +++++ 9 files changed, 902 insertions(+), 1583 deletions(-) delete mode 100644 openhands-agent-server/openhands/agent_server/llm_connections.py create mode 100644 openhands-agent-server/openhands/agent_server/llm_providers.py delete mode 100644 tests/agent_server/test_llm_connections.py create mode 100644 tests/agent_server/test_llm_providers.py diff --git a/openhands-agent-server/openhands/agent_server/api.py b/openhands-agent-server/openhands/agent_server/api.py index 4c029b0209..e0fc27f887 100644 --- a/openhands-agent-server/openhands/agent_server/api.py +++ b/openhands-agent-server/openhands/agent_server/api.py @@ -48,7 +48,7 @@ init_router, require_initialized, ) -from openhands.agent_server.llm_connections import connections_router +from openhands.agent_server.llm_providers import providers_router from openhands.agent_server.llm_router import llm_router from openhands.agent_server.mcp_router import mcp_router from openhands.agent_server.middleware import CORSDispatcher @@ -441,7 +441,7 @@ def _add_api_routes(app: FastAPI) -> None: api_router.include_router(plugins_router) api_router.include_router(hooks_router) api_router.include_router(llm_router) - api_router.include_router(connections_router) + api_router.include_router(providers_router) api_router.include_router(mcp_router) api_router.include_router(settings_router) api_router.include_router(workspaces_router) @@ -697,22 +697,6 @@ def create_app(config: Config | None = None) -> FastAPI: _add_api_routes(app) _setup_static_files(app, config) - # Register the LLM ``secret:`` resolver so profiles spawned from a - # provider connection resolve their api_key against the agent-server's - # SecretsStore at call time (see LLM._get_api_key_value). - # - # NOTE: the resolver is a *process-global* installed on the SDK. In the - # normal one-app-per-process deployment this is exactly right. If multiple - # apps are created in one process (e.g. some test setups) the last - # ``create_app`` wins; the resolver reads the config-scoped secrets_store - # captured in the closure, so a stale registration would point at a - # previous app's store. Tests that need isolation should call - # ``register_llm_secret_resolver(None)`` in teardown. - from openhands.agent_server.persistence import get_secrets_store - from openhands.sdk.llm.llm import register_llm_secret_resolver - - secrets_store = get_secrets_store(config) - register_llm_secret_resolver(lambda name: secrets_store.get_secret(name)) app.add_middleware( CORSDispatcher, allow_origins=config.allow_cors_origins, diff --git a/openhands-agent-server/openhands/agent_server/llm_connections.py b/openhands-agent-server/openhands/agent_server/llm_connections.py deleted file mode 100644 index 1f874e0cd7..0000000000 --- a/openhands-agent-server/openhands/agent_server/llm_connections.py +++ /dev/null @@ -1,872 +0,0 @@ -"""Provider Connection endpoints: connect a vendor once, pick from its models. - -A Provider Connection is the persisted record for the "connect a provider once" -flow (OpenHands/OpenHands#15492). The connection stores a *reference* to a named -secret (the API key lives in the SecretsStore), plus the list of models the user -selected from the provider's catalog. The raw key is never returned to clients: -responses carry ``api_key_set`` and the connection's ``secret_name`` is treated -as sensitive metadata. - -The key is stored per-connection (not per-provider), so a second key for the same -provider is an additive connection later — multiple-keys-per-provider is -deferred but the data model already supports it. - -Endpoints (mounted under ``/api/llm``): - - - GET /connections list connections (masked, no keys) - - POST /connections create {provider, key, label?, models?} - - GET /connections/{id} connection + its selected models - - PATCH /connections/{id} rotate key / rename label / set models - - DELETE /connections/{id} disconnect (+ delete the named secret); - returns the profiles that referenced it - - POST /connections/{id}/validate test the key (catalog-only by default, - live probe with ``?live=true`` or - OH_CONNECTIONS_LIVE_VALIDATE) and return - the model catalog; the response carries - ``verified`` so clients never claim an - unchecked key was authenticated - - POST /connections/{id}/profiles create an LLM profile bound to this - connection's key (api_key by reference) - -LLM profiles spawned from a connection store ``api_key = "secret:"`` -(see :func:`openhands.agent_server.persistence.llm_secret_ref`) instead of the -raw key, so rotating the key is one SecretsStore write and every referencing -profile picks it up at call time (see ``LLM._get_api_key_value``). -""" - -from __future__ import annotations - -import os -import time -import uuid -from collections.abc import Callable -from typing import Literal - -from fastapi import APIRouter, HTTPException, Request, status -from pydantic import BaseModel, Field, SecretStr - -from openhands.agent_server._secrets_exposure import get_config -from openhands.agent_server.persistence import ( - PersistedConnections, - ProviderConnection, - get_connections_store, - get_llm_profile_store, - get_secrets_store, - llm_secret_ref, -) -from openhands.sdk.llm.utils.unverified_models import ( - _extract_model_and_provider, - _get_litellm_provider_names, - get_supported_llm_models, -) -from openhands.sdk.llm.utils.verified_models import VERIFIED_MODELS -from openhands.sdk.logger import get_logger - - -logger = get_logger(__name__) - -connections_router = APIRouter(prefix="/llm/connections", tags=["LLM Connections"]) - -# Cap on the number of saved connections. Per-connection keys mean a second key -# for the same provider is additive; 64 leaves ample headroom while bounding the -# catalog the GUI renders. -MAX_CONNECTIONS = 64 - -# Length of the per-connection id (uuid4 hex, 32 chars). Used only for the -# ``secret_name`` derivation below; the id itself is opaque to clients. -_SECRET_NAME_PREFIX = "llm_connection_" - - -def _connection_secret_name(connection_id: str) -> str: - """Derive the named-secret key under which a connection's key is stored.""" - return f"{_SECRET_NAME_PREFIX}{connection_id}" - - -def _now() -> int: - return int(time.time()) - - -def _provider_names() -> set[str]: - return _get_litellm_provider_names() | set(VERIFIED_MODELS) - - -def _litellm_provider_names() -> set[str]: - return _get_litellm_provider_names() - - -def _model_provider(model: str) -> str | None: - provider, _, _ = _extract_model_and_provider(model) - if provider: - return provider - if "/" in model: - prefix = model.split("/", 1)[0].strip() - return prefix or None - return None - - -def _model_name_for_provider(provider: str, model: str) -> str: - prefix = f"{provider}/" - return model[len(prefix) :] if model.startswith(prefix) else model - - -def _qualified_model(provider: str, model: str) -> str: - return model if "/" in model else f"{provider}/{model}" - - -# ── Request / Response models ──────────────────────────────────────────── - - -class ConnectionCreateRequest(BaseModel): - """Create a connection. ``key`` is written to the SecretsStore; never echoed.""" - - provider: str = Field(..., min_length=1, max_length=128) - key: SecretStr = Field(..., min_length=1) - label: str | None = Field(default=None, max_length=128) - base_url: str | None = Field(default=None, max_length=2048) - api_mode: Literal["auto", "chat", "responses"] = "auto" - custom_headers: dict[str, str] = Field(default_factory=dict) - models: list[str] = Field(default_factory=list) - - -class ConnectionUpdateRequest(BaseModel): - """Partial update a connection. - - ``key`` rotates the named secret (rewrites the SecretsStore entry). - ``label``, endpoint settings, and ``models`` are straightforward field - updates. At least one field is required. - """ - - key: SecretStr | None = None - label: str | None = None - base_url: str | None = Field(default=None, max_length=2048) - api_mode: Literal["auto", "chat", "responses"] | None = None - custom_headers: dict[str, str] | None = None - models: list[str] | None = None - - -class ConnectionResponse(BaseModel): - """Safe connection view — never includes the raw key or the secret value.""" - - id: str - provider: str - label: str | None = None - base_url: str | None = None - api_mode: Literal["auto", "chat", "responses"] = "auto" - custom_headers: dict[str, str] = Field(default_factory=dict) - models: list[str] = Field(default_factory=list) - created_at: int - last_validated_at: int | None = None - api_key_set: bool = False - - -class ValidateResponse(BaseModel): - """Result of testing a connection's key against the provider's catalog. - - ``verified`` distinguishes a real, network-checked key from a catalog-only - response: it is True only when a live probe confirmed the provider accepted - the key. Clients must not present the key as authenticated when ``verified`` - is False (the models are the provider's advertised catalog, not proven grants). - """ - - id: str - provider: str - ok: bool - verified: bool = False - models: list[str] = Field(default_factory=list) - error: str | None = None - validated_at: int - - -class DisconnectResponse(BaseModel): - """Result of a disconnect: which profiles now reference a missing key.""" - - id: str - affected_profiles: list[str] = Field(default_factory=list) - - -class CreateProfileFromConnectionRequest(BaseModel): - """Create an LLM profile that authenticates via this connection's key. - - The profile stores ``api_key = "secret:"`` rather than the - raw key, so rotating the connection updates every profile at once. ``model`` - must be one of the connection's selected/validated models. - """ - - profile_name: str = Field(..., min_length=1, max_length=64) - model: str = Field(..., min_length=1) - # Optional per-profile override. If omitted, the connection endpoint - # settings are used. - base_url: str | None = None - - -class ProfileFromConnectionResponse(BaseModel): - profile_name: str - model: str - provider: str - connection_id: str - - -def _to_response(conn: ProviderConnection, *, api_key_set: bool) -> ConnectionResponse: - api_mode = ( - conn.api_mode if conn.api_mode in {"auto", "chat", "responses"} else "auto" - ) - return ConnectionResponse( - id=conn.id, - provider=conn.provider, - label=conn.label, - base_url=conn.base_url, - api_mode=api_mode, - custom_headers=dict(conn.custom_headers), - models=list(conn.models), - created_at=conn.created_at, - last_validated_at=conn.last_validated_at, - api_key_set=api_key_set, - ) - - -def _api_key_set(secret_name: str) -> bool: - """True if the named secret backing a connection currently holds a value.""" - store = get_secrets_store() - value = store.get_secret(secret_name) - return bool(value and value.strip()) - - -def _profiles_referencing(secret_name: str) -> list[str]: - """Names of LLM profiles whose ``api_key`` points at this connection's secret. - - Used to warn the user before disconnect: these profiles would stop - authenticating once the named secret is deleted. - """ - ref = llm_secret_ref(secret_name) - store = get_llm_profile_store() - referrers: list[str] = [] - for summary in store.list_summaries(): - name = summary.get("name") - if not isinstance(name, str): - continue - try: - llm = store.load(name) - except Exception: # noqa: BLE001 - skip unreadable profiles - continue - api_key = llm.api_key - raw = api_key.get_secret_value() if isinstance(api_key, SecretStr) else None - if raw == ref: - referrers.append(name) - return sorted(referrers) - - -def _backfill_connections_from_raw_profiles( - persisted: PersistedConnections, -) -> PersistedConnections: - """Promote existing raw-key LLM profiles into provider connections. - - Early versions of the onboarding flow saved only an LLM profile with a raw - ``api_key``. Once Provider Connections exist, those profiles should appear - in Model providers and should reference the shared named secret. This helper - performs that one-time local migration during connection listing: - - - raw profile key -> named secret - - profile api_key -> ``secret:`` - - connection.models includes the profile's current model - - Profiles already using ``secret:`` are left unchanged, so repeated - calls are idempotent. - """ - profile_store = get_llm_profile_store() - secrets_store = get_secrets_store() - existing_secret_names = {c.secret_name for c in persisted.connections} - - try: - summaries = profile_store.list_summaries() - except Exception: # noqa: BLE001 - listing should not fail provider page - return persisted - - changed = False - for summary in summaries: - name = summary.get("name") - if not isinstance(name, str): - continue - try: - llm = profile_store.load(name) - except Exception: # noqa: BLE001 - skip unreadable profiles - continue - - raw_key = ( - llm.api_key.get_secret_value() - if isinstance(llm.api_key, SecretStr) - else "" - ) - if not raw_key or raw_key.startswith("secret:"): - continue - - provider = _model_provider(llm.model) - if provider not in _provider_names(): - continue - if len(persisted.connections) >= MAX_CONNECTIONS: - logger.warning("Skipping profile-to-connection backfill: limit reached") - break - - connection_id = uuid.uuid4().hex - secret_name = _connection_secret_name(connection_id) - if secret_name in existing_secret_names: - continue - model_name = _model_name_for_provider(provider, llm.model) - - try: - secrets_store.set_secret( - name=secret_name, - value=raw_key, - description=f"LLM provider connection key for {provider}", - ) - migrated_llm = llm.model_copy( - update={"api_key": SecretStr(llm_secret_ref(secret_name))} - ) - profile_store.save(name, migrated_llm, include_secrets=True) - except Exception as e: # noqa: BLE001 - preserve the original profile - logger.warning(f"Failed to backfill provider connection for {name}: {e}") - try: - secrets_store.delete_secret(secret_name) - except Exception: # noqa: BLE001 - best-effort cleanup - pass - continue - - persisted.connections.append( - ProviderConnection( - id=connection_id, - provider=provider, - label=name, - base_url=llm.base_url, - api_mode=( - llm.api_mode - if llm.api_mode in {"auto", "chat", "responses"} - else "auto" - ), - custom_headers=dict(llm.extra_headers or {}), - secret_name=secret_name, - models=[model_name], - created_at=_now(), - last_validated_at=None, - ) - ) - existing_secret_names.add(secret_name) - changed = True - - return persisted if changed else persisted - - -def _get_connection_or_404(connections, connection_id: str) -> ProviderConnection: - for conn in connections.connections: - if conn.id == connection_id: - return conn - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=f"Connection '{connection_id}' not found", - ) - - -# ── Provider key validation (injectable for tests) ─────────────────────── -# -# ``validate_provider_key`` confirms a key looks usable for a provider and -# returns that provider's model catalog. It reports two distinct things via the -# ``ValidationResult`` fields: -# -# - ``ok`` the request could proceed (non-empty key, known provider, and -# — when a live probe runs — the provider did not reject the key) -# - ``verified`` whether the key was actually checked against the provider over -# the network. When no live probe runs, ``verified`` is False and -# the catalog is the provider's *advertised* models, not the ones -# the key is proven to grant. Callers/UI must not claim the key -# was authenticated when ``verified`` is False. -# -# A live probe is opt-in (``OH_CONNECTIONS_LIVE_VALIDATE=1`` or ``live=True`` on -# the endpoint) because it costs a network round-trip and is not always reachable -# from every deployment. The function is module-level so tests monkeypatch it. - -ValidateFn = Callable[..., "ValidationResult"] - - -class ValidationResult(BaseModel): - ok: bool - models: list[str] = Field(default_factory=list) - error: str | None = None - verified: bool = False - - -def _provider_catalog(provider: str) -> list[str]: - """Return the provider's advertised model catalog (no network call).""" - all_models = get_supported_llm_models() - verified_provider_models = set(VERIFIED_MODELS.get(provider, ())) - filtered: list[str] = [] - for model in all_models: - model_provider, _, _ = _extract_model_and_provider(model) - if model_provider == provider or model in verified_provider_models: - filtered.append(_model_name_for_provider(provider, model)) - filtered.extend(verified_provider_models) - return sorted(set(filtered)) - - -def _live_probe( - provider: str, - key: str, - *, - base_url: str | None = None, - custom_headers: dict[str, str] | None = None, -) -> tuple[bool, str | None]: - """Cheaply check a key against a provider over the network. - - Returns ``(ok, error)``. Uses LiteLLM's provider-endpoint check, which lists - the provider's models using the supplied key without spending tokens. An - authentication/permission rejection maps to ``ok=False`` with a short cause; - connectivity problems are surfaced as an error but do not assert the key is - invalid. - """ - import litellm - from litellm.exceptions import AuthenticationError, PermissionDeniedError - - try: - litellm.get_valid_models( - check_provider_endpoint=True, - custom_llm_provider=provider, - api_key=key, - api_base=base_url, - extra_headers=custom_headers or None, - ) - return True, None - except (AuthenticationError, PermissionDeniedError) as e: - return False, f"Provider rejected the key: {str(e)[:200]}" - except Exception as e: # noqa: BLE001 - connectivity/other; don't assert invalid - logger.warning(f"Live validation probe failed for {provider}: {e}") - return False, f"Could not reach {provider} to verify the key: {str(e)[:200]}" - - -def validate_provider_key( - provider: str, - key: str, - *, - live: bool = False, - base_url: str | None = None, - custom_headers: dict[str, str] | None = None, -) -> ValidationResult: - """Validate a provider key and return the models it can select. - - With ``live=False`` (default) this performs input checks only and returns the - provider's advertised catalog with ``verified=False`` — it does *not* prove - the key authenticates. With ``live=True`` it additionally issues a cheap - network probe; on success ``verified`` is True. - """ - if not key or not key.strip(): - return ValidationResult(ok=False, models=[], error="API key is empty") - if provider not in _provider_names(): - return ValidationResult( - ok=False, models=[], error=f"Unknown provider '{provider}'" - ) - - catalog = _provider_catalog(provider) - if not live or provider not in _litellm_provider_names(): - return ValidationResult(ok=True, models=catalog, error=None, verified=False) - - ok, error = _live_probe( - provider, - key, - base_url=base_url, - custom_headers=custom_headers, - ) - return ValidationResult( - ok=ok, models=catalog if ok else [], error=error, verified=ok - ) - - -# ── Endpoints ──────────────────────────────────────────────────────────── - - -@connections_router.get("", response_model=list[ConnectionResponse]) -async def list_connections(request: Request) -> list[ConnectionResponse]: - """List all saved provider connections (keys never returned).""" - config = get_config(request) - store = get_connections_store(config) - persisted = store.update(_backfill_connections_from_raw_profiles) - conns = persisted.connections if persisted is not None else [] - return [_to_response(c, api_key_set=_api_key_set(c.secret_name)) for c in conns] - - -@connections_router.post( - "", - response_model=ConnectionResponse, - status_code=status.HTTP_201_CREATED, -) -async def create_connection( - request: Request, body: ConnectionCreateRequest -) -> ConnectionResponse: - """Create a connection: store the key as a named secret, then the record.""" - if body.provider not in _provider_names(): - raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=f"Unknown provider '{body.provider}'", - ) - - config = get_config(request) - store = get_connections_store(config) - secrets_store = get_secrets_store(config) - - connection_id = uuid.uuid4().hex - secret_name = _connection_secret_name(connection_id) - - def add(conn_list): - if len(conn_list.connections) >= MAX_CONNECTIONS: - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=( - f"Connection limit reached ({MAX_CONNECTIONS}). " - "Disconnect one before adding a new connection." - ), - ) - conn = ProviderConnection( - id=connection_id, - provider=body.provider, - label=body.label, - base_url=body.base_url, - api_mode=body.api_mode, - custom_headers=dict(body.custom_headers), - secret_name=secret_name, - models=list(body.models), - created_at=_now(), - ) - conn_list.connections.append(conn) - return conn_list - - try: - secrets_store.set_secret( - name=secret_name, - value=body.key.get_secret_value(), - description=f"LLM provider connection key for {body.provider}", - ) - except RuntimeError as e: - logger.error(f"Connection create blocked (secrets): {e}") - raise HTTPException( - status_code=500, - detail="Secrets file is corrupted or encrypted with a different key", - ) - - try: - persisted = store.update(add) - except HTTPException: - # Roll back the secret we just wrote so we don't leak orphaned keys. - try: - secrets_store.delete_secret(secret_name) - except Exception: # noqa: BLE001 - best-effort cleanup - logger.warning(f"Failed to roll back secret {secret_name}") - raise - - conn = next(c for c in persisted.connections if c.id == connection_id) - logger.info( - "Created provider connection", - extra={"connection_id": connection_id, "provider": body.provider}, - ) - return _to_response(conn, api_key_set=True) - - -@connections_router.get("/{connection_id}", response_model=ConnectionResponse) -async def get_connection(request: Request, connection_id: str) -> ConnectionResponse: - """Get a single connection (key never returned).""" - config = get_config(request) - store = get_connections_store(config) - persisted = store.load() - if persisted is None: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=f"Connection '{connection_id}' not found", - ) - conn = _get_connection_or_404(persisted, connection_id) - return _to_response(conn, api_key_set=_api_key_set(conn.secret_name)) - - -@connections_router.patch("/{connection_id}", response_model=ConnectionResponse) -async def update_connection( - request: Request, connection_id: str, body: ConnectionUpdateRequest -) -> ConnectionResponse: - """Update a connection: rotate key, rename label, or set selected models.""" - if ( - body.key is None - and body.label is None - and body.base_url is None - and body.api_mode is None - and body.custom_headers is None - and body.models is None - ): - raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=( - "Provide at least one of: key, label, base_url, api_mode, " - "custom_headers, models" - ), - ) - - config = get_config(request) - store = get_connections_store(config) - secrets_store = get_secrets_store(config) - - def patch(conn_list): - # The whole update runs under the connections lock. We only rotate the - # secret *after* confirming the connection still exists, so a concurrent - # delete can't leave an orphaned rotated key (the earlier version wrote - # the secret before the record check). - for c in conn_list.connections: - if c.id == connection_id: - if body.key is not None: - try: - secrets_store.set_secret( - name=c.secret_name, - value=body.key.get_secret_value(), - description=( - f"LLM provider connection key for {c.provider}" - ), - ) - except RuntimeError as e: - logger.error(f"Connection rotate blocked (secrets): {e}") - raise HTTPException( - status_code=500, - detail=( - "Secrets file is corrupted or encrypted with a " - "different key" - ), - ) - if body.label is not None: - c = c.model_copy(update={"label": body.label}) - if body.base_url is not None: - c = c.model_copy(update={"base_url": body.base_url}) - if body.api_mode is not None: - c = c.model_copy(update={"api_mode": body.api_mode}) - if body.custom_headers is not None: - c = c.model_copy( - update={"custom_headers": dict(body.custom_headers)} - ) - if body.models is not None: - c = c.model_copy(update={"models": list(body.models)}) - conn_list.connections = [ - c if x.id == connection_id else x for x in conn_list.connections - ] - return conn_list - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=f"Connection '{connection_id}' not found", - ) - - persisted = store.update(patch) - conn = next(c for c in persisted.connections if c.id == connection_id) - return _to_response(conn, api_key_set=_api_key_set(conn.secret_name)) - - -@connections_router.delete("/{connection_id}", response_model=DisconnectResponse) -async def delete_connection(request: Request, connection_id: str) -> DisconnectResponse: - """Disconnect: delete the connection record and its named secret. - - Returns the names of LLM profiles that referenced the connection's key so the - client can warn that they will stop authenticating until pointed at a new key. - The profiles are left intact (deleting them silently would be more surprising - than a clear "these now need a key" message). - """ - config = get_config(request) - store = get_connections_store(config) - secrets_store = get_secrets_store(config) - - deleted_secret_name: str | None = None - - def remove(conn_list): - nonlocal deleted_secret_name - for i, c in enumerate(conn_list.connections): - if c.id == connection_id: - deleted_secret_name = c.secret_name - conn_list.connections.pop(i) - return conn_list - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=f"Connection '{connection_id}' not found", - ) - - affected: list[str] = [] - if deleted_secret_name is None: - # Peek before mutating so we can report referrers in the response. - persisted = store.load() - if persisted is not None: - for c in persisted.connections: - if c.id == connection_id: - affected = _profiles_referencing(c.secret_name) - break - - store.update(remove) - if deleted_secret_name is not None: - try: - secrets_store.delete_secret(deleted_secret_name) - except Exception: # noqa: BLE001 - record already gone; best-effort - logger.warning(f"Failed to delete secret {deleted_secret_name}") - logger.info( - "Deleted provider connection", - extra={"connection_id": connection_id, "affected_profiles": len(affected)}, - ) - return DisconnectResponse(id=connection_id, affected_profiles=affected) - - -def _live_validation_default() -> bool: - """Whether validate should probe the provider live unless told otherwise.""" - return os.getenv("OH_CONNECTIONS_LIVE_VALIDATE", "").strip().lower() in { - "1", - "true", - "yes", - } - - -@connections_router.post("/{connection_id}/validate", response_model=ValidateResponse) -async def validate_connection( - request: Request, connection_id: str, live: bool | None = None -) -> ValidateResponse: - """Test the connection's key against the provider and return its catalog. - - When ``live`` is true (or ``OH_CONNECTIONS_LIVE_VALIDATE`` is set) the key is - probed against the provider over the network and ``verified`` reflects the - real result. Otherwise the response is catalog-only with ``verified=false``. - On a successful validation the connection's ``last_validated_at`` is stamped - and its ``models`` are set to the returned catalog so a profile can be spawned - from them without a second call. - """ - config = get_config(request) - store = get_connections_store(config) - secrets_store = get_secrets_store(config) - - persisted = store.load() or _empty() - conn = _get_connection_or_404(persisted, connection_id) - key = secrets_store.get_secret(conn.secret_name) or "" - - do_live = _live_validation_default() if live is None else live - result = validate_provider_key( - conn.provider, - key, - live=do_live, - base_url=conn.base_url, - custom_headers=conn.custom_headers, - ) - validated_at = _now() - if result.ok: - # Persist the catalog + timestamp so profile creation can reuse them. - def stamp(conn_list): - for c in conn_list.connections: - if c.id == connection_id: - c = c.model_copy( - update={ - "last_validated_at": validated_at, - "models": list(result.models), - } - ) - conn_list.connections = [ - c if x.id == connection_id else x for x in conn_list.connections - ] - return conn_list - return conn_list - - store.update(stamp) - - return ValidateResponse( - id=connection_id, - provider=conn.provider, - ok=result.ok, - verified=result.verified, - models=result.models, - error=result.error, - validated_at=validated_at, - ) - - -@connections_router.post( - "/{connection_id}/profiles", - response_model=ProfileFromConnectionResponse, - status_code=status.HTTP_201_CREATED, -) -async def create_profile_from_connection( - request: Request, - connection_id: str, - body: CreateProfileFromConnectionRequest, -) -> ProfileFromConnectionResponse: - """Create an LLM profile backed by this connection's key. - - This is the "pick from every model the connection offers" step: it saves a - named LLM profile whose ``api_key`` is a ``secret:`` reference to the - connection's stored key, so the profile authenticates without duplicating the - key and follows the key when it is rotated. ``model`` must be one of the - connection's selected/validated models. - """ - from openhands.sdk.llm import LLM - from openhands.sdk.llm.llm_profile_store import ( - PROFILE_NAME_REGEX, - ProfileLimitExceeded, - ) - - config = get_config(request) - store = get_connections_store(config) - - persisted = store.load() or _empty() - conn = _get_connection_or_404(persisted, connection_id) - - if not PROFILE_NAME_REGEX.match(body.profile_name): - raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=f"Invalid profile name '{body.profile_name}'", - ) - if conn.models and body.model not in conn.models: - raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=( - f"Model '{body.model}' is not one of the connection's selected " - "models. Validate the connection or choose a listed model." - ), - ) - - api_mode = ( - conn.api_mode if conn.api_mode in {"auto", "chat", "responses"} else "auto" - ) - saved_model = _qualified_model(conn.provider, body.model) - llm = LLM( - model=saved_model, - base_url=body.base_url or conn.base_url, - api_mode=api_mode, - extra_headers=dict(conn.custom_headers) or None, - api_key=SecretStr(llm_secret_ref(conn.secret_name)), - usage_id=body.profile_name, - ) - - profile_store = get_llm_profile_store() - from openhands.agent_server.profiles_router import MAX_PROFILES - - try: - profile_store.save( - body.profile_name, - llm, - include_secrets=True, - max_profiles=MAX_PROFILES, - ) - except ProfileLimitExceeded: - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=( - f"Profile limit reached ({MAX_PROFILES}). " - "Delete a profile before creating a new one." - ), - ) - - logger.info( - "Created profile from connection", - extra={ - "connection_id": connection_id, - "profile_name": body.profile_name, - "model": saved_model, - }, - ) - return ProfileFromConnectionResponse( - profile_name=body.profile_name, - model=saved_model, - provider=conn.provider, - connection_id=connection_id, - ) - - -def _empty() -> PersistedConnections: - """Return a fresh empty PersistedConnections (avoids ``load() or None`` chains).""" - return PersistedConnections() diff --git a/openhands-agent-server/openhands/agent_server/llm_providers.py b/openhands-agent-server/openhands/agent_server/llm_providers.py new file mode 100644 index 0000000000..5694591058 --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/llm_providers.py @@ -0,0 +1,581 @@ +"""Model provider endpoints: connect a provider once, manage its models under it. + +A "model provider" is the persisted record for the provider-centric flow in +OpenHands/OpenHands#15492. One API key is held on the provider and shared by +every model nested under it; the user manages those models (add / edit / remove) +directly on the provider. The key is stored as a *named secret* in the +SecretsStore — the provider record keeps only ``secret_name`` and the raw key is +never returned (responses carry ``api_key_set`` instead). + +Endpoints (mounted under ``/api/llm``). The list of *available provider kinds* +for the "add provider" preset picker stays at ``GET /api/llm/providers`` +(see ``llm_router``); these configured-provider records live under +``/api/llm/model-providers`` to avoid shadowing it: + + - GET /model-providers list providers (masked, no keys) + - POST /model-providers create {display_name, kind, + base_url, wire_api, key, + custom_headers, models?} + - GET /model-providers/{id} a single provider (masked) + - PATCH /model-providers/{id} update fields / rotate key + - DELETE /model-providers/{id} remove provider + its named secret + - POST /model-providers/{id}/models add a nested model {name, wire_api?} + - PATCH /model-providers/{id}/models/{name} edit a nested model + - DELETE /model-providers/{id}/models/{name} remove a nested model + - POST /model-providers/{id}/test optional key probe; NEVER mutates + the curated model list +""" + +from __future__ import annotations + +import time +import uuid + +from fastapi import APIRouter, HTTPException, Request, status +from pydantic import BaseModel, Field, SecretStr + +from openhands.agent_server._secrets_exposure import get_config +from openhands.agent_server.persistence import ( + ModelProvider, + PersistedProviders, + ProviderModel, + get_providers_store, + get_secrets_store, +) +from openhands.agent_server.persistence.models import WireApi +from openhands.sdk.llm.utils.unverified_models import ( + _extract_model_and_provider, + _get_litellm_provider_names, + get_supported_llm_models, +) +from openhands.sdk.llm.utils.verified_models import VERIFIED_MODELS +from openhands.sdk.logger import get_logger + + +logger = get_logger(__name__) + +providers_router = APIRouter(prefix="/llm/model-providers", tags=["Model Providers"]) + +# Cap on saved providers. Generous headroom while bounding the config document. +MAX_PROVIDERS = 64 +# Cap on models nested under a single provider. +MAX_MODELS_PER_PROVIDER = 256 + +_SECRET_NAME_PREFIX = "llm_provider_" + + +def _secret_name(provider_id: str) -> str: + return f"{_SECRET_NAME_PREFIX}{provider_id}" + + +def _now() -> int: + return int(time.time()) + + +# ── Request / Response models ──────────────────────────────────────────── + + +class ProviderModelPayload(BaseModel): + name: str = Field(..., min_length=1, max_length=256) + wire_api: WireApi | None = None + + +class ProviderCreateRequest(BaseModel): + """Create a provider. ``key`` is written to the SecretsStore; never echoed.""" + + display_name: str = Field(..., min_length=1, max_length=128) + kind: str = Field(default="custom", max_length=128) + key: SecretStr = Field(..., min_length=1) + base_url: str | None = Field(default=None, max_length=2048) + wire_api: WireApi = "auto" + custom_headers: dict[str, str] = Field(default_factory=dict) + models: list[ProviderModelPayload] = Field(default_factory=list) + + +class ProviderUpdateRequest(BaseModel): + """Partial update. ``key`` rotates the named secret. At least one field.""" + + display_name: str | None = Field(default=None, min_length=1, max_length=128) + kind: str | None = Field(default=None, max_length=128) + key: SecretStr | None = None + base_url: str | None = Field(default=None, max_length=2048) + wire_api: WireApi | None = None + custom_headers: dict[str, str] | None = None + + +class ModelResponse(BaseModel): + name: str + wire_api: WireApi | None = None + + +class ProviderResponse(BaseModel): + """Safe provider view — never includes the raw key or the secret name.""" + + id: str + display_name: str + kind: str + base_url: str | None = None + wire_api: WireApi = "auto" + custom_headers: dict[str, str] = Field(default_factory=dict) + models: list[ModelResponse] = Field(default_factory=list) + created_at: int + updated_at: int + api_key_set: bool = False + + +class TestResponse(BaseModel): + """Result of probing a provider's key. Never mutates the model list. + + ``verified`` is True only when a live network probe confirmed the provider + accepted the key. ``suggested_models`` is the provider's advertised catalog, + offered purely as a convenience for populating model rows. + """ + + id: str + ok: bool + verified: bool = False + suggested_models: list[str] = Field(default_factory=list) + error: str | None = None + + +# ── Helpers ────────────────────────────────────────────────────────────── + + +def _to_response(p: ModelProvider, *, api_key_set: bool) -> ProviderResponse: + return ProviderResponse( + id=p.id, + display_name=p.display_name, + kind=p.kind, + base_url=p.base_url, + wire_api=p.wire_api, + custom_headers=dict(p.custom_headers), + models=[ModelResponse(name=m.name, wire_api=m.wire_api) for m in p.models], + created_at=p.created_at, + updated_at=p.updated_at, + api_key_set=api_key_set, + ) + + +def _api_key_set(secret_name: str) -> bool: + value = get_secrets_store().get_secret(secret_name) + return bool(value and value.strip()) + + +def _get_provider_or_404( + persisted: PersistedProviders | None, provider_id: str +) -> ModelProvider: + for p in persisted.providers if persisted else []: + if p.id == provider_id: + return p + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Provider '{provider_id}' not found", + ) + + +def _provider_catalog(kind: str) -> list[str]: + """Provider's advertised model catalog (no network call), for suggestions.""" + all_models = get_supported_llm_models() + verified = set(VERIFIED_MODELS.get(kind, ())) + out: list[str] = [] + for model in all_models: + model_provider, _, _ = _extract_model_and_provider(model) + if model_provider == kind or model in verified: + prefix = f"{kind}/" + out.append(model[len(prefix) :] if model.startswith(prefix) else model) + out.extend(verified) + return sorted(set(out)) + + +def _live_probe( + kind: str, + key: str, + *, + base_url: str | None, +) -> tuple[bool, str | None]: + """Cheaply check a key against a provider over the network. + + Uses LiteLLM's provider-endpoint listing, which validates the key without + spending tokens. Auth/permission rejections map to ``(False, cause)``; + connectivity failures are surfaced as an error but do not assert invalidity. + """ + import litellm + from litellm.exceptions import AuthenticationError, PermissionDeniedError + + try: + litellm.get_valid_models( + check_provider_endpoint=True, + custom_llm_provider=kind, + api_key=key, + api_base=base_url, + ) + return True, None + except (AuthenticationError, PermissionDeniedError) as e: + return False, f"Provider rejected the key: {str(e)[:200]}" + except Exception as e: # noqa: BLE001 - connectivity/other; don't assert invalid + logger.warning(f"Live probe failed for {kind}: {e}") + return False, f"Could not reach {kind} to verify the key: {str(e)[:200]}" + + +# ── Provider endpoints ─────────────────────────────────────────────────── + + +@providers_router.get("", response_model=list[ProviderResponse]) +async def list_providers(request: Request) -> list[ProviderResponse]: + """List all saved model providers (keys never returned).""" + store = get_providers_store(get_config(request)) + persisted = store.load() + providers = persisted.providers if persisted else [] + return [_to_response(p, api_key_set=_api_key_set(p.secret_name)) for p in providers] + + +@providers_router.post( + "", response_model=ProviderResponse, status_code=status.HTTP_201_CREATED +) +async def create_provider( + request: Request, body: ProviderCreateRequest +) -> ProviderResponse: + """Create a provider: store the key as a named secret, then the record.""" + config = get_config(request) + store = get_providers_store(config) + secrets_store = get_secrets_store(config) + + provider_id = uuid.uuid4().hex + secret_name = _secret_name(provider_id) + now = _now() + + def add(persisted: PersistedProviders) -> PersistedProviders: + if len(persisted.providers) >= MAX_PROVIDERS: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=( + f"Provider limit reached ({MAX_PROVIDERS}). " + "Remove one before adding a new provider." + ), + ) + if len(body.models) > MAX_MODELS_PER_PROVIDER: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"Too many models (max {MAX_MODELS_PER_PROVIDER})", + ) + persisted.providers.append( + ModelProvider( + id=provider_id, + display_name=body.display_name, + kind=body.kind, + base_url=body.base_url, + wire_api=body.wire_api, + custom_headers=dict(body.custom_headers), + secret_name=secret_name, + models=[ + ProviderModel(name=m.name, wire_api=m.wire_api) for m in body.models + ], + created_at=now, + updated_at=now, + ) + ) + return persisted + + try: + secrets_store.set_secret( + name=secret_name, + value=body.key.get_secret_value(), + description=f"LLM provider key for {body.display_name}", + ) + except RuntimeError as e: + logger.error(f"Provider create blocked (secrets): {e}") + raise HTTPException( + status_code=500, + detail="Secrets file is corrupted or encrypted with a different key", + ) + + try: + persisted = store.update(add) + except HTTPException: + # Roll back the secret we just wrote so we don't leak an orphaned key. + try: + secrets_store.delete_secret(secret_name) + except Exception: # noqa: BLE001 - best-effort cleanup + logger.warning(f"Failed to roll back secret {secret_name}") + raise + + provider = next(p for p in persisted.providers if p.id == provider_id) + logger.info("Created model provider", extra={"provider_id": provider_id}) + return _to_response(provider, api_key_set=True) + + +@providers_router.get("/{provider_id}", response_model=ProviderResponse) +async def get_provider(request: Request, provider_id: str) -> ProviderResponse: + """Get a single provider (key never returned).""" + store = get_providers_store(get_config(request)) + provider = _get_provider_or_404(store.load(), provider_id) + return _to_response(provider, api_key_set=_api_key_set(provider.secret_name)) + + +@providers_router.patch("/{provider_id}", response_model=ProviderResponse) +async def update_provider( + request: Request, provider_id: str, body: ProviderUpdateRequest +) -> ProviderResponse: + """Update provider fields or rotate its key. Models are managed separately.""" + if all( + v is None + for v in ( + body.display_name, + body.kind, + body.key, + body.base_url, + body.wire_api, + body.custom_headers, + ) + ): + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=( + "Provide at least one of: display_name, kind, key, base_url, " + "wire_api, custom_headers" + ), + ) + + config = get_config(request) + store = get_providers_store(config) + secrets_store = get_secrets_store(config) + + def patch(persisted: PersistedProviders) -> PersistedProviders: + # Runs under the providers lock; rotate the secret only after confirming + # the provider still exists so a concurrent delete can't orphan a key. + for p in persisted.providers: + if p.id == provider_id: + if body.key is not None: + try: + secrets_store.set_secret( + name=p.secret_name, + value=body.key.get_secret_value(), + description=f"LLM provider key for {p.display_name}", + ) + except RuntimeError as e: + logger.error(f"Provider rotate blocked (secrets): {e}") + raise HTTPException( + status_code=500, + detail=( + "Secrets file is corrupted or encrypted with a " + "different key" + ), + ) + updates: dict = {"updated_at": _now()} + if body.display_name is not None: + updates["display_name"] = body.display_name + if body.kind is not None: + updates["kind"] = body.kind + if body.base_url is not None: + updates["base_url"] = body.base_url + if body.wire_api is not None: + updates["wire_api"] = body.wire_api + if body.custom_headers is not None: + updates["custom_headers"] = dict(body.custom_headers) + updated = p.model_copy(update=updates) + persisted.providers = [ + updated if x.id == provider_id else x for x in persisted.providers + ] + return persisted + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Provider '{provider_id}' not found", + ) + + persisted = store.update(patch) + provider = next(p for p in persisted.providers if p.id == provider_id) + return _to_response(provider, api_key_set=_api_key_set(provider.secret_name)) + + +@providers_router.delete( + "/{provider_id}", status_code=status.HTTP_200_OK, response_model=ProviderResponse +) +async def delete_provider(request: Request, provider_id: str) -> ProviderResponse: + """Remove a provider and its named secret.""" + config = get_config(request) + store = get_providers_store(config) + secrets_store = get_secrets_store(config) + + removed: dict[str, ModelProvider] = {} + + def remove(persisted: PersistedProviders) -> PersistedProviders: + for i, p in enumerate(persisted.providers): + if p.id == provider_id: + removed["p"] = persisted.providers.pop(i) + return persisted + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Provider '{provider_id}' not found", + ) + + store.update(remove) + provider = removed["p"] + try: + secrets_store.delete_secret(provider.secret_name) + except Exception: # noqa: BLE001 - record already gone; best-effort + logger.warning(f"Failed to delete secret {provider.secret_name}") + logger.info("Deleted model provider", extra={"provider_id": provider_id}) + return _to_response(provider, api_key_set=False) + + +# ── Nested model endpoints ─────────────────────────────────────────────── + + +@providers_router.post( + "/{provider_id}/models", + response_model=ProviderResponse, + status_code=status.HTTP_201_CREATED, +) +async def add_model( + request: Request, provider_id: str, body: ProviderModelPayload +) -> ProviderResponse: + """Add a model under the provider (shares the provider's key/endpoint).""" + store = get_providers_store(get_config(request)) + + def mutate(persisted: PersistedProviders) -> PersistedProviders: + for p in persisted.providers: + if p.id == provider_id: + if any(m.name == body.name for m in p.models): + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"Model '{body.name}' already exists", + ) + if len(p.models) >= MAX_MODELS_PER_PROVIDER: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"Model limit reached ({MAX_MODELS_PER_PROVIDER})", + ) + models = [ + *p.models, + ProviderModel(name=body.name, wire_api=body.wire_api), + ] + updated = p.model_copy(update={"models": models, "updated_at": _now()}) + persisted.providers = [ + updated if x.id == provider_id else x for x in persisted.providers + ] + return persisted + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Provider '{provider_id}' not found", + ) + + persisted = store.update(mutate) + provider = next(p for p in persisted.providers if p.id == provider_id) + return _to_response(provider, api_key_set=_api_key_set(provider.secret_name)) + + +@providers_router.patch( + "/{provider_id}/models/{model_name}", response_model=ProviderResponse +) +async def update_model( + request: Request, + provider_id: str, + model_name: str, + body: ProviderModelPayload, +) -> ProviderResponse: + """Rename a model and/or change its per-model wire-API override.""" + store = get_providers_store(get_config(request)) + + def mutate(persisted: PersistedProviders) -> PersistedProviders: + for p in persisted.providers: + if p.id == provider_id: + if not any(m.name == model_name for m in p.models): + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Model '{model_name}' not found", + ) + if body.name != model_name and any( + m.name == body.name for m in p.models + ): + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"Model '{body.name}' already exists", + ) + models = [ + ProviderModel(name=body.name, wire_api=body.wire_api) + if m.name == model_name + else m + for m in p.models + ] + updated = p.model_copy(update={"models": models, "updated_at": _now()}) + persisted.providers = [ + updated if x.id == provider_id else x for x in persisted.providers + ] + return persisted + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Provider '{provider_id}' not found", + ) + + persisted = store.update(mutate) + provider = next(p for p in persisted.providers if p.id == provider_id) + return _to_response(provider, api_key_set=_api_key_set(provider.secret_name)) + + +@providers_router.delete( + "/{provider_id}/models/{model_name}", response_model=ProviderResponse +) +async def remove_model( + request: Request, provider_id: str, model_name: str +) -> ProviderResponse: + """Remove a model from the provider.""" + store = get_providers_store(get_config(request)) + + def mutate(persisted: PersistedProviders) -> PersistedProviders: + for p in persisted.providers: + if p.id == provider_id: + if not any(m.name == model_name for m in p.models): + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Model '{model_name}' not found", + ) + models = [m for m in p.models if m.name != model_name] + updated = p.model_copy(update={"models": models, "updated_at": _now()}) + persisted.providers = [ + updated if x.id == provider_id else x for x in persisted.providers + ] + return persisted + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Provider '{provider_id}' not found", + ) + + persisted = store.update(mutate) + provider = next(p for p in persisted.providers if p.id == provider_id) + return _to_response(provider, api_key_set=_api_key_set(provider.secret_name)) + + +# ── Optional test probe ────────────────────────────────────────────────── + + +@providers_router.post("/{provider_id}/test", response_model=TestResponse) +async def test_provider(request: Request, provider_id: str) -> TestResponse: + """Probe the provider's stored key and suggest catalog models. + + This never mutates the provider's curated model list — ``suggested_models`` + is offered only as a convenience for the "add model" affordance. + """ + config = get_config(request) + store = get_providers_store(config) + secrets_store = get_secrets_store(config) + + provider = _get_provider_or_404(store.load(), provider_id) + key = secrets_store.get_secret(provider.secret_name) or "" + if not key.strip(): + return TestResponse(id=provider_id, ok=False, error="No API key stored") + + suggested = _provider_catalog(provider.kind) + if provider.kind not in _get_litellm_provider_names(): + # Unknown/custom endpoint: can't probe, only offer the catalog. + return TestResponse( + id=provider_id, ok=True, verified=False, suggested_models=suggested + ) + + ok, error = _live_probe(provider.kind, key, base_url=provider.base_url) + return TestResponse( + id=provider_id, + ok=ok, + verified=ok, + suggested_models=suggested if ok else [], + error=error, + ) diff --git a/openhands-agent-server/openhands/agent_server/persistence/__init__.py b/openhands-agent-server/openhands/agent_server/persistence/__init__.py index 75d76e79e2..81ff9efd52 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/__init__.py +++ b/openhands-agent-server/openhands/agent_server/persistence/__init__.py @@ -7,35 +7,33 @@ """ from openhands.agent_server.persistence.models import ( - CONNECTIONS_SCHEMA_VERSION, - LLM_SECRET_REF_PREFIX, PERSISTED_SETTINGS_SCHEMA_VERSION, + PROVIDERS_SCHEMA_VERSION, SECRET_NAME_PATTERN, WORKSPACES_SCHEMA_VERSION, CustomSecret, - PersistedConnections, + ModelProvider, + PersistedProviders, PersistedSettings, PersistedWorkspaces, - ProviderConnection, + ProviderModel, Secrets, SettingsUpdatePayload, WorkspaceItem, WorkspaceParentItem, - llm_secret_ref, - parse_llm_secret_ref, ) from openhands.agent_server.persistence.store import ( - ConnectionsStore, - FileConnectionsStore, + FileProvidersStore, FileSecretsStore, FileSettingsStore, FileWorkspacesStore, + ProvidersStore, SecretsStore, SettingsStore, WorkspacesStore, get_agent_profile_store, - get_connections_store, get_llm_profile_store, + get_providers_store, get_secrets_store, get_settings_store, get_workspaces_store, @@ -45,35 +43,33 @@ __all__ = [ # Constants - "CONNECTIONS_SCHEMA_VERSION", - "LLM_SECRET_REF_PREFIX", "PERSISTED_SETTINGS_SCHEMA_VERSION", + "PROVIDERS_SCHEMA_VERSION", "SECRET_NAME_PATTERN", "WORKSPACES_SCHEMA_VERSION", # Models "CustomSecret", - "PersistedConnections", + "ModelProvider", + "PersistedProviders", "PersistedSettings", "PersistedWorkspaces", - "ProviderConnection", + "ProviderModel", "Secrets", "SettingsUpdatePayload", "WorkspaceItem", "WorkspaceParentItem", - "llm_secret_ref", - "parse_llm_secret_ref", # Stores - "FileConnectionsStore", + "FileProvidersStore", "FileSecretsStore", "FileSettingsStore", "FileWorkspacesStore", - "ConnectionsStore", + "ProvidersStore", "SecretsStore", "SettingsStore", "WorkspacesStore", "get_agent_profile_store", - "get_connections_store", "get_llm_profile_store", + "get_providers_store", "get_secrets_store", "get_settings_store", "get_workspaces_store", diff --git a/openhands-agent-server/openhands/agent_server/persistence/models.py b/openhands-agent-server/openhands/agent_server/persistence/models.py index 12edbacab9..0096f6a1f6 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/models.py +++ b/openhands-agent-server/openhands/agent_server/persistence/models.py @@ -9,7 +9,7 @@ import re from collections.abc import Mapping -from typing import Any, TypedDict +from typing import Any, Literal, TypedDict from pydantic import ( BaseModel, @@ -537,86 +537,80 @@ def from_persisted(cls, data: Any) -> PersistedWorkspaces: return cls.model_validate(payload) -# ── Provider Connections ───────────────────────────────────────────────── +# ── Model Providers ────────────────────────────────────────────────────── # -# A "Provider Connection" is the persisted record for "connect a vendor once -# with one key" (OpenHands/OpenHands#15492). The connection stores a *reference* -# to a named secret (the API key lives in the SecretsStore), plus the list of -# models the user selected from the provider's catalog. The raw key is never -# stored in the connection record — only ``secret_name`` — so rotating the key -# is one SecretsStore write and every spawned LLM profile that references the -# same secret picks it up. The key is stored per-connection (not per-provider), -# so a second key for the same provider is an additive connection later. +# A "Provider" is the persisted record for "connect a provider once, then manage +# its models under it" (OpenHands/OpenHands#15492). One key is held on the +# provider and shared by every model nested under it. The key lives in the +# SecretsStore; the provider record stores only ``secret_name`` (never the value), +# so rotating the key is a single SecretsStore write. Models are a nested list +# the user manages (add / edit / remove) — not a fan-out of standalone records. -CONNECTIONS_SCHEMA_VERSION = 1 +PROVIDERS_SCHEMA_VERSION = 1 -# Marker prefix an LLM profile's ``api_key`` uses to point at a named secret -# managed by a Provider Connection, instead of holding the raw key inline. -# Resolution happens at call time in ``LLM._get_api_key_value``. -LLM_SECRET_REF_PREFIX = "secret:" +WireApi = Literal["auto", "chat", "responses"] -def parse_llm_secret_ref(api_key: str | None) -> str | None: - """Return the secret name referenced by a ``secret:`` api_key, else None.""" - if not isinstance(api_key, str): - return None - if not api_key.startswith(LLM_SECRET_REF_PREFIX): - return None - name = api_key[len(LLM_SECRET_REF_PREFIX) :].strip() - return name or None +class ProviderModel(BaseModel): + """A model offered by a provider. Inherits the provider's key and endpoint. + ``wire_api`` optionally overrides the provider default for this one model. + """ + + name: str = Field(..., min_length=1, max_length=256) + wire_api: WireApi | None = Field(default=None) -def llm_secret_ref(secret_name: str) -> str: - """Build the ``secret:`` reference string stored in a profile's api_key.""" - return f"{LLM_SECRET_REF_PREFIX}{secret_name}" + model_config = ConfigDict(populate_by_name=True) -class ProviderConnection(BaseModel): - """A saved provider connection (one key, many models). +class ModelProvider(BaseModel): + """A saved model provider (one key, many nested models). ``secret_name`` references the key stored in the SecretsStore; the value is never held here. Responses to clients must mask the key (``api_key_set``) - and never return ``secret_name``'s value. + and never return ``secret_name``. """ id: str = Field(..., min_length=1, max_length=128) - provider: str = Field(..., min_length=1, max_length=128) - label: str | None = Field(default=None, max_length=128) + display_name: str = Field(..., min_length=1, max_length=128) + kind: str = Field( + default="custom", + max_length=128, + description="Preset id or litellm provider key (e.g. 'openai', 'custom').", + ) base_url: str | None = Field(default=None, max_length=2048) - api_mode: str = Field(default="auto") + wire_api: WireApi = Field(default="auto") custom_headers: dict[str, str] = Field(default_factory=dict) secret_name: str = Field(..., min_length=1, max_length=128) - models: list[str] = Field(default_factory=list) + models: list[ProviderModel] = Field(default_factory=list) created_at: int = Field(..., description="Unix epoch seconds.") - last_validated_at: int | None = Field( - default=None, description="Unix epoch seconds of last successful validate." - ) + updated_at: int = Field(..., description="Unix epoch seconds.") model_config = ConfigDict(populate_by_name=True) -class PersistedConnections(BaseModel): - """Container for all provider connections (single JSON document).""" +class PersistedProviders(BaseModel): + """Container for all model providers (single JSON document).""" - schema_version: int = Field(default=CONNECTIONS_SCHEMA_VERSION) - connections: list[ProviderConnection] = Field(default_factory=list) + schema_version: int = Field(default=PROVIDERS_SCHEMA_VERSION) + providers: list[ModelProvider] = Field(default_factory=list) model_config = ConfigDict(populate_by_name=True) @classmethod - def from_persisted(cls, data: Any) -> PersistedConnections: + def from_persisted(cls, data: Any) -> PersistedProviders: if not isinstance(data, dict): return cls.model_validate(data) payload = dict(data) - version = payload.get("schema_version", CONNECTIONS_SCHEMA_VERSION) + version = payload.get("schema_version", PROVIDERS_SCHEMA_VERSION) if not isinstance(version, int): - raise ValueError("PersistedConnections schema_version must be an integer") - if version > CONNECTIONS_SCHEMA_VERSION: + raise ValueError("PersistedProviders schema_version must be an integer") + if version > PROVIDERS_SCHEMA_VERSION: raise ValueError( - f"PersistedConnections schema_version {version} is newer than " - f"supported {CONNECTIONS_SCHEMA_VERSION}" + f"PersistedProviders schema_version {version} is newer than " + f"supported {PROVIDERS_SCHEMA_VERSION}" ) - payload["schema_version"] = CONNECTIONS_SCHEMA_VERSION + payload["schema_version"] = PROVIDERS_SCHEMA_VERSION return cls.model_validate(payload) diff --git a/openhands-agent-server/openhands/agent_server/persistence/store.py b/openhands-agent-server/openhands/agent_server/persistence/store.py index a449fc32ba..4f1f6a2d7b 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/store.py +++ b/openhands-agent-server/openhands/agent_server/persistence/store.py @@ -24,7 +24,7 @@ from openhands.agent_server.persistence.models import ( CustomSecret, - PersistedConnections, + PersistedProviders, PersistedSettings, PersistedWorkspaces, Secrets, @@ -787,84 +787,84 @@ def update( return updated -class ConnectionsStore(ABC): - """Abstract base class for provider-connection storage.""" +class ProvidersStore(ABC): + """Abstract base class for model-provider storage.""" @abstractmethod - def load(self) -> PersistedConnections | None: - """Load connections from storage.""" + def load(self) -> PersistedProviders | None: + """Load providers from storage.""" @abstractmethod - def save(self, connections: PersistedConnections) -> None: - """Save connections to storage.""" + def save(self, providers: PersistedProviders) -> None: + """Save providers to storage.""" @abstractmethod def update( self, - update_fn: Callable[[PersistedConnections], PersistedConnections], - ) -> PersistedConnections: - """Atomically update connections with file locking.""" + update_fn: Callable[[PersistedProviders], PersistedProviders], + ) -> PersistedProviders: + """Atomically update providers with file locking.""" -class FileConnectionsStore(ConnectionsStore): - """File-based storage for provider connections. +class FileProvidersStore(ProvidersStore): + """File-based storage for model providers. - Persists a single JSON document at ``/connections.json`` + Persists a single JSON document at ``/providers.json`` using the same atomic-write + file-lock primitives as ``FileWorkspacesStore``. - Connection records hold a ``secret_name`` reference to the key (stored in the + Provider records hold a ``secret_name`` reference to the key (stored in the SecretsStore), never the key value itself, so no cipher is needed here. """ def __init__( self, persistence_dir: Path | str, - filename: str = "connections.json", + filename: str = "providers.json", ): _validate_filename(filename) self.persistence_dir = Path(persistence_dir) self.filename = filename self._path = self.persistence_dir / filename - self._lock_path = self.persistence_dir / ".connections.lock" + self._lock_path = self.persistence_dir / ".providers.lock" - def load(self) -> PersistedConnections | None: + def load(self) -> PersistedProviders | None: if not self._path.exists(): return None try: with self._path.open("r", encoding="utf-8") as f: data = json.load(f) - return PersistedConnections.from_persisted(data) + return PersistedProviders.from_persisted(data) except (PermissionError, OSError) as e: - logger.error(f"Cannot access connections file: {e}") + logger.error(f"Cannot access providers file: {e}") raise except json.JSONDecodeError as e: - logger.error(f"Connections file is corrupted: {e}") + logger.error(f"Providers file is corrupted: {e}") return None except Exception: - logger.error("Failed to load connections", exc_info=True) + logger.error("Failed to load providers", exc_info=True) return None - def save(self, connections: PersistedConnections) -> None: + def save(self, providers: PersistedProviders) -> None: _ensure_secure_directory(self.persistence_dir) - data = connections.model_dump(mode="json", exclude_none=True) + data = providers.model_dump(mode="json", exclude_none=True) _atomic_write_json(self._path, data) - logger.debug(f"Connections saved to {self._path}") + logger.debug(f"Providers saved to {self._path}") def update( self, - update_fn: Callable[[PersistedConnections], PersistedConnections], - ) -> PersistedConnections: + update_fn: Callable[[PersistedProviders], PersistedProviders], + ) -> PersistedProviders: with _file_lock(self._lock_path): - connections = self.load() - if connections is None: + providers = self.load() + if providers is None: if self._path.exists(): raise RuntimeError( - f"Cannot load connections from {self._path}. " + f"Cannot load providers from {self._path}. " "File may be corrupted. " "Refusing to overwrite with defaults to prevent data loss." ) - connections = PersistedConnections() - updated = update_fn(connections) + providers = PersistedProviders() + updated = update_fn(providers) self.save(updated) return updated @@ -874,7 +874,7 @@ def update( _settings_store: FileSettingsStore | None = None _secrets_store: FileSecretsStore | None = None _workspaces_store: FileWorkspacesStore | None = None -_connections_store: FileConnectionsStore | None = None +_providers_store: FileProvidersStore | None = None _llm_profile_store: LLMProfileStore | None = None _agent_profile_store: AgentProfileStore | None = None _store_lock = threading.Lock() @@ -998,26 +998,26 @@ def get_workspaces_store(config: Config | None = None) -> FileWorkspacesStore: return _workspaces_store -def get_connections_store(config: Config | None = None) -> FileConnectionsStore: # noqa: ARG001 - """Get the global provider-connections store instance (thread-safe). +def get_providers_store(config: Config | None = None) -> FileProvidersStore: # noqa: ARG001 + """Get the global model-providers store instance (thread-safe). - Connection records hold only a ``secret_name`` reference to the key (the key + Provider records hold only a ``secret_name`` reference to the key (the key lives in the SecretsStore), so no cipher is used here. Stored in the profile persistence dir (same as secrets/profiles) so credentials stay in the user's config directory, never workspace-relative. ``config`` is accepted for parity - with the other store factories; the connections dir is resolved from + with the other store factories; the providers dir is resolved from ``OH_PERSISTENCE_DIR`` / ``~/.openhands`` (see ``_get_profile_persistence_dir``). """ - global _connections_store - if _connections_store is not None: - return _connections_store + global _providers_store + if _providers_store is not None: + return _providers_store with _store_lock: - if _connections_store is None: - _connections_store = FileConnectionsStore( + if _providers_store is None: + _providers_store = FileProvidersStore( persistence_dir=_get_profile_persistence_dir(), ) - return _connections_store + return _providers_store def get_llm_profile_store() -> LLMProfileStore: @@ -1063,12 +1063,12 @@ def get_agent_profile_store() -> AgentProfileStore: def reset_stores() -> None: """Reset global store instances (for testing).""" global _settings_store, _secrets_store, _workspaces_store - global _connections_store + global _providers_store global _llm_profile_store, _agent_profile_store with _store_lock: _settings_store = None _secrets_store = None _workspaces_store = None - _connections_store = None + _providers_store = None _llm_profile_store = None _agent_profile_store = None diff --git a/openhands-sdk/openhands/sdk/llm/llm.py b/openhands-sdk/openhands/sdk/llm/llm.py index 583216727e..1c7c43104b 100644 --- a/openhands-sdk/openhands/sdk/llm/llm.py +++ b/openhands-sdk/openhands/sdk/llm/llm.py @@ -125,70 +125,6 @@ logger = get_logger(__name__) -# ── Secret-reference resolution for provider connections ───────────────── -# -# An LLM profile spawned from a "provider connection" (OpenHands/OpenHands#15492) -# stores ``api_key = "secret:"`` instead of the raw key, so rotating -# the key is one SecretsStore write and every referencing profile picks it up. -# At call time, ``_get_api_key_value`` resolves the ``secret:`` prefix via this -# hook. The SDK stays decoupled from any concrete secret store: the agent-server -# installs a resolver (``register_llm_secret_resolver``); without one, a -# ``secret:`` reference resolves to ``None`` (the same behavior as a missing key), -# so a standalone SDK use that never set a resolver is unaffected. -LLM_SECRET_REF_PREFIX = "secret:" - - -def parse_llm_secret_ref(api_key: str | None) -> str | None: - """Return the secret name referenced by a ``secret:`` api_key, else None.""" - if not isinstance(api_key, str): - return None - if not api_key.startswith(LLM_SECRET_REF_PREFIX): - return None - name = api_key[len(LLM_SECRET_REF_PREFIX) :].strip() - return name or None - - -def llm_secret_ref(secret_name: str) -> str: - """Build the ``secret:`` reference string stored in a profile's api_key.""" - return f"{LLM_SECRET_REF_PREFIX}{secret_name}" - - -_LLMSecretResolver = Callable[[str], str | None] -_llm_secret_resolver: _LLMSecretResolver | None = None -_llm_secret_resolver_lock = threading.Lock() - - -def register_llm_secret_resolver(resolver: _LLMSecretResolver | None) -> None: - """Install (or clear) the resolver used for ``secret:`` api_key values. - - The agent-server registers a resolver that reads its SecretsStore so a - profile's ``secret:`` api_key resolves to the real key at call time. - Passing ``None`` clears the resolver (e.g. between tests). - """ - global _llm_secret_resolver - with _llm_secret_resolver_lock: - _llm_secret_resolver = resolver - - -def _resolve_api_key(api_key: str | None) -> str | None: - """Resolve a possibly-``secret:``-prefixed api_key to its raw value.""" - ref = parse_llm_secret_ref(api_key) - if ref is None: - # Not a reference: ``api_key`` is either a raw key (str) or None. - return api_key if isinstance(api_key, str) else None - with _llm_secret_resolver_lock: - resolver = _llm_secret_resolver - if resolver is None: - return None - try: - return resolver(ref) - except Exception: # noqa: BLE001 - never leak resolver internals to the LLM call - logger.warning( - f"Failed to resolve LLM secret reference '{ref}' - treating as missing" - ) - return None - - _serialized_is_subscription = ContextVar( "serialized_is_subscription", default=False, @@ -2103,12 +2039,7 @@ def _get_api_key_value(self) -> str | None: if self.api_key is None: return None assert isinstance(self.api_key, SecretStr) - raw = self.api_key.get_secret_value() - # ``secret:`` references a named secret managed by a provider - # connection (see ``register_llm_secret_resolver``); resolve it at call - # time so rotation is one store write picked up by every profile. A raw - # key (no prefix) passes through unchanged. - return _resolve_api_key(raw) + return self.api_key.get_secret_value() def _subscription_headers_from_credentials( self, auth: Any, credentials: Any diff --git a/tests/agent_server/test_llm_connections.py b/tests/agent_server/test_llm_connections.py deleted file mode 100644 index ae6275e998..0000000000 --- a/tests/agent_server/test_llm_connections.py +++ /dev/null @@ -1,514 +0,0 @@ -"""Tests for the Provider Connection endpoints (OpenHands/OpenHands#15492).""" - -from __future__ import annotations - -import tempfile -from pathlib import Path -from unittest.mock import patch - -import pytest -from fastapi.testclient import TestClient -from pydantic import SecretStr - -from openhands.agent_server import llm_connections as conn_module -from openhands.agent_server.api import create_app -from openhands.agent_server.config import Config -from openhands.agent_server.persistence import ( - FileConnectionsStore, - PersistedConnections, - ProviderConnection, - get_llm_profile_store, - reset_stores, -) -from openhands.sdk.llm import LLM - - -@pytest.fixture -def temp_dirs(): - with tempfile.TemporaryDirectory() as tmpdir: - base = Path(tmpdir) - (base / "profiles").mkdir(parents=True, exist_ok=True) - yield base - - -@pytest.fixture -def client(temp_dirs, monkeypatch): - reset_stores() - monkeypatch.setenv("OH_PERSISTENCE_DIR", str(temp_dirs)) - config = Config(static_files_path=None, session_api_keys=[], secret_key=None) - # Patch the connections store to the temp dir (mirrors profiles_router tests). - with patch( - "openhands.agent_server.llm_connections.get_connections_store", - lambda *_a, **_kw: FileConnectionsStore(persistence_dir=temp_dirs), - ): - app = create_app(config) - yield TestClient(app) - reset_stores() - - -def test_list_empty(client): - r = client.get("/api/llm/connections") - assert r.status_code == 200 - assert r.json() == [] - - -def test_create_then_list(client): - r = client.post( - "/api/llm/connections", - json={ - "provider": "openai", - "key": "sk-test", - "label": "work", - "base_url": "https://proxy.example/v1", - "api_mode": "chat", - "custom_headers": {"X-Org": "eng"}, - "models": ["gpt-4o"], - }, - ) - assert r.status_code == 201 - body = r.json() - assert body["provider"] == "openai" - assert body["label"] == "work" - assert body["base_url"] == "https://proxy.example/v1" - assert body["api_mode"] == "chat" - assert body["custom_headers"] == {"X-Org": "eng"} - assert body["models"] == ["gpt-4o"] - assert body["api_key_set"] is True - # Key never echoed. - assert "key" not in body - assert "secret_name" not in body - cid = body["id"] - - r = client.get("/api/llm/connections") - assert r.status_code == 200 - listed = r.json() - assert len(listed) == 1 - assert listed[0]["id"] == cid - assert listed[0]["base_url"] == "https://proxy.example/v1" - assert listed[0]["api_mode"] == "chat" - assert listed[0]["custom_headers"] == {"X-Org": "eng"} - - -def test_create_unknown_provider_422(client): - r = client.post( - "/api/llm/connections", - json={"provider": "nope_provider", "key": "k"}, - ) - assert r.status_code == 422 - - -def test_create_accepts_verified_provider_namespace(client): - r = client.post( - "/api/llm/connections", - json={"provider": "openhands", "key": "oh-key"}, - ) - assert r.status_code == 201 - assert r.json()["provider"] == "openhands" - - -def test_validate_verified_provider_namespace_is_catalog_only(client): - cid = client.post( - "/api/llm/connections", - json={"provider": "openhands", "key": "oh-key"}, - ).json()["id"] - - r = client.post(f"/api/llm/connections/{cid}/validate?live=true") - - assert r.status_code == 200 - body = r.json() - assert body["ok"] is True - assert body["verified"] is False - assert len(body["models"]) > 0 - assert all(not model.startswith("openhands/") for model in body["models"]) - - -def test_get_connection(client): - cid = client.post( - "/api/llm/connections", json={"provider": "anthropic", "key": "sk-ant"} - ).json()["id"] - r = client.get(f"/api/llm/connections/{cid}") - assert r.status_code == 200 - assert r.json()["provider"] == "anthropic" - assert r.json()["api_key_set"] is True - - -def test_get_missing_404(client): - r = client.get("/api/llm/connections/does-not-exist") - assert r.status_code == 404 - - -def test_patch_rotate_label_models(client): - cid = client.post( - "/api/llm/connections", json={"provider": "openai", "key": "sk-1"} - ).json()["id"] - - r = client.patch( - f"/api/llm/connections/{cid}", - json={ - "label": "work", - "base_url": "https://proxy.example/v1", - "api_mode": "responses", - "custom_headers": {"X-Team": "platform"}, - "models": ["gpt-4o", "gpt-4o-mini"], - }, - ) - assert r.status_code == 200 - body = r.json() - assert body["label"] == "work" - assert body["base_url"] == "https://proxy.example/v1" - assert body["api_mode"] == "responses" - assert body["custom_headers"] == {"X-Team": "platform"} - assert body["models"] == ["gpt-4o", "gpt-4o-mini"] - - # Rotate key: api_key_set stays true. - r = client.patch(f"/api/llm/connections/{cid}", json={"key": "sk-2"}) - assert r.status_code == 200 - assert r.json()["api_key_set"] is True - - -def test_patch_requires_a_field(client): - cid = client.post( - "/api/llm/connections", json={"provider": "openai", "key": "sk-1"} - ).json()["id"] - r = client.patch(f"/api/llm/connections/{cid}", json={}) - assert r.status_code == 422 - - -def test_patch_missing_connection_404(client): - r = client.patch("/api/llm/connections/none", json={"label": "x"}) - # When only label/models are set (no key), the 404 comes from patch(). - assert r.status_code == 404 - - -def test_delete_removes_connection_and_secret(client): - cid = client.post( - "/api/llm/connections", json={"provider": "openai", "key": "sk-1"} - ).json()["id"] - r = client.delete(f"/api/llm/connections/{cid}") - assert r.status_code == 200 - body = r.json() - assert body["id"] == cid - # No profiles reference this connection, so nothing is affected. - assert body["affected_profiles"] == [] - assert client.get(f"/api/llm/connections/{cid}").status_code == 404 - # Listing is empty again. - assert client.get("/api/llm/connections").json() == [] - - -def test_delete_missing_404(client): - assert client.delete("/api/llm/connections/none").status_code == 404 - - -def test_validate_success_stamps_timestamp(client): - cid = client.post( - "/api/llm/connections", - json={"provider": "openai", "key": "sk-test"}, - ).json()["id"] - r = client.post(f"/api/llm/connections/{cid}/validate") - assert r.status_code == 200 - body = r.json() - assert body["ok"] is True - assert body["error"] is None - assert len(body["models"]) > 0 - # Catalog-only validation must not claim the key was network-verified. - assert body["verified"] is False - - # last_validated_at got stamped and the catalog was persisted onto models. - conn = client.get(f"/api/llm/connections/{cid}").json() - assert conn["last_validated_at"] is not None - assert len(conn["models"]) > 0 - - -def test_validate_missing_connection_404(client): - assert client.post("/api/llm/connections/none/validate").status_code == 404 - - -def test_validate_uses_injected_validator(client, monkeypatch): - """validate_provider_key is module-level so tests can monkeypatch it.""" - - def fake(provider, key, *, live=False, base_url=None, custom_headers=None): - return conn_module.ValidationResult( - ok=True, - models=["fake-model-a", "fake-model-b"], - error=None, - verified=True, - ) - - monkeypatch.setattr(conn_module, "validate_provider_key", fake) - cid = client.post( - "/api/llm/connections", json={"provider": "openai", "key": "sk"} - ).json()["id"] - body = client.post(f"/api/llm/connections/{cid}/validate").json() - assert body["ok"] is True - assert body["verified"] is True - assert body["models"] == ["fake-model-a", "fake-model-b"] - - -def test_validate_live_flag_marks_verified(client, monkeypatch): - """The ``live`` query flag drives a real probe and sets ``verified``.""" - calls: list[bool] = [] - - def fake(provider, key, *, live=False, base_url=None, custom_headers=None): - calls.append(live) - return conn_module.ValidationResult( - ok=live, models=["m1"] if live else [], error=None, verified=live - ) - - monkeypatch.setattr(conn_module, "validate_provider_key", fake) - cid = client.post( - "/api/llm/connections", json={"provider": "openai", "key": "sk"} - ).json()["id"] - body = client.post(f"/api/llm/connections/{cid}/validate?live=true").json() - assert calls == [True] - assert body["verified"] is True - - -def test_validate_passes_endpoint_settings(client, monkeypatch): - calls: list[dict[str, object]] = [] - - def fake(provider, key, *, live=False, base_url=None, custom_headers=None): - calls.append( - { - "provider": provider, - "live": live, - "base_url": base_url, - "custom_headers": custom_headers, - } - ) - return conn_module.ValidationResult( - ok=True, models=["gpt-4o"], error=None, verified=live - ) - - monkeypatch.setattr(conn_module, "validate_provider_key", fake) - cid = client.post( - "/api/llm/connections", - json={ - "provider": "openai", - "key": "sk", - "base_url": "https://proxy.example/v1", - "custom_headers": {"X-Org": "eng"}, - }, - ).json()["id"] - - body = client.post(f"/api/llm/connections/{cid}/validate?live=true").json() - - assert body["ok"] is True - assert calls == [ - { - "provider": "openai", - "live": True, - "base_url": "https://proxy.example/v1", - "custom_headers": {"X-Org": "eng"}, - } - ] - - -def test_create_profile_from_connection(client): - """A connection can spawn an LLM profile that references its key by name.""" - cid = client.post( - "/api/llm/connections", - json={"provider": "openai", "key": "sk-test", "models": ["gpt-4o"]}, - ).json()["id"] - - r = client.post( - f"/api/llm/connections/{cid}/profiles", - json={"profile_name": "work-gpt4o", "model": "gpt-4o"}, - ) - assert r.status_code == 201 - body = r.json() - assert body["profile_name"] == "work-gpt4o" - assert body["model"] == "openai/gpt-4o" - assert body["connection_id"] == cid - - # The profile is saved and references the connection secret by name, so its - # api_key resolves through the connection rather than duplicating the key. - detail = client.get("/api/profiles/work-gpt4o").json() - assert detail["api_key_set"] is True - assert detail["config"]["model"] == "openai/gpt-4o" - assert detail["config"]["api_mode"] == "auto" - - # Deleting the connection now reports the referencing profile. - deleted = client.delete(f"/api/llm/connections/{cid}").json() - assert "work-gpt4o" in deleted["affected_profiles"] - - -def test_create_profile_rejects_model_not_in_catalog(client): - cid = client.post( - "/api/llm/connections", - json={"provider": "openai", "key": "sk-test", "models": ["gpt-4o"]}, - ).json()["id"] - r = client.post( - f"/api/llm/connections/{cid}/profiles", - json={"profile_name": "nope", "model": "not-a-model"}, - ) - assert r.status_code == 422 - - -def test_create_profile_from_openhands_connection_qualifies_model(client): - cid = client.post( - "/api/llm/connections", - json={ - "provider": "openhands", - "key": "oh-key", - "models": ["gpt-5.6"], - }, - ).json()["id"] - - r = client.post( - f"/api/llm/connections/{cid}/profiles", - json={"profile_name": "openhands-gpt", "model": "gpt-5.6"}, - ) - - assert r.status_code == 201 - assert r.json()["model"] == "openhands/gpt-5.6" - detail = client.get("/api/profiles/openhands-gpt").json() - assert detail["config"]["model"] == "openhands/gpt-5.6" - - -def test_create_profile_inherits_connection_endpoint_settings(client): - cid = client.post( - "/api/llm/connections", - json={ - "provider": "openai", - "key": "sk-test", - "base_url": "https://proxy.example/v1", - "api_mode": "responses", - "custom_headers": {"X-Org": "eng"}, - "models": ["gpt-4o"], - }, - ).json()["id"] - - r = client.post( - f"/api/llm/connections/{cid}/profiles", - json={"profile_name": "gateway-gpt4o", "model": "gpt-4o"}, - ) - assert r.status_code == 201 - - detail = client.get("/api/profiles/gateway-gpt4o").json() - assert detail["config"]["model"] == "openai/gpt-4o" - assert detail["config"]["base_url"] == "https://proxy.example/v1" - assert detail["config"]["api_mode"] == "responses" - assert detail["config"]["extra_headers"] == {"X-Org": "eng"} - - -def test_list_backfills_raw_profile_as_provider_connection(client): - profile_store = get_llm_profile_store() - profile_store.save( - "openhands-gpt", - LLM(model="openhands/gpt-5.6", api_key=SecretStr("oh-key")), - include_secrets=True, - ) - - r = client.get("/api/llm/connections") - - assert r.status_code == 200 - body = r.json() - assert len(body) == 1 - assert body[0]["provider"] == "openhands" - assert body[0]["label"] == "openhands-gpt" - assert body[0]["models"] == ["gpt-5.6"] - assert body[0]["api_key_set"] is True - - # The migration is idempotent because the profile now points at the - # connection's named secret instead of keeping the raw key. - assert len(client.get("/api/llm/connections").json()) == 1 - detail = client.get("/api/profiles/openhands-gpt").json() - assert detail["api_key_set"] is True - - -def test_create_limit_enforced(client, monkeypatch): - monkeypatch.setattr(conn_module, "MAX_CONNECTIONS", 2) - for i in range(2): - assert ( - client.post( - "/api/llm/connections", - json={"provider": "openai", "key": f"sk-{i}"}, - ).status_code - == 201 - ) - r = client.post("/api/llm/connections", json={"provider": "openai", "key": "sk-3"}) - assert r.status_code == 409 - - -# ── secret-by-name resolution at the LLM layer ──────────────────────────── - - -def test_llm_secret_ref_helpers_roundtrip(): - from openhands.sdk.llm.llm import ( - LLM_SECRET_REF_PREFIX, - llm_secret_ref, - parse_llm_secret_ref, - ) - - ref = llm_secret_ref("llm_connection_abc") - assert ref == f"{LLM_SECRET_REF_PREFIX}llm_connection_abc" - assert parse_llm_secret_ref(ref) == "llm_connection_abc" - # Raw key (no prefix) is not a reference. - assert parse_llm_secret_ref("sk-raw-key") is None - assert parse_llm_secret_ref(None) is None - - -def test_llm_resolves_secret_ref_via_resolver(): - from openhands.sdk.llm.llm import LLM, register_llm_secret_resolver - - register_llm_secret_resolver(lambda name: f"resolved-{name}" if name else None) - try: - llm = LLM(model="gpt-4o", api_key=SecretStr("secret:llm_connection_abc")) - assert llm._get_api_key_value() == "resolved-llm_connection_abc" - finally: - register_llm_secret_resolver(None) - - -def test_llm_secret_ref_without_resolver_is_none(): - from openhands.sdk.llm.llm import LLM, register_llm_secret_resolver - - register_llm_secret_resolver(None) - llm = LLM(model="gpt-4o", api_key=SecretStr("secret:llm_connection_abc")) - assert llm._get_api_key_value() is None - - -def test_llm_raw_key_unaffected_by_resolver(): - from openhands.sdk.llm.llm import LLM, register_llm_secret_resolver - - register_llm_secret_resolver(lambda name: "should-not-be-used") - try: - llm = LLM(model="gpt-4o", api_key=SecretStr("sk-raw-key")) - assert llm._get_api_key_value() == "sk-raw-key" - finally: - register_llm_secret_resolver(None) - - -# ── persistence layer ───────────────────────────────────────────────────── - - -def test_connections_store_roundtrip(temp_dirs): - store = FileConnectionsStore(persistence_dir=temp_dirs) - assert store.load() is None - - conn = ProviderConnection( - id="abc", - provider="openai", - label="work", - base_url="https://proxy.example/v1", - api_mode="chat", - custom_headers={"X-Org": "eng"}, - secret_name="llm_connection_abc", - models=["gpt-4o"], - created_at=1700000000, - ) - persisted = store.update(lambda c: PersistedConnections(connections=[conn])) - assert persisted.connections[0].id == "abc" - - reloaded = store.load() - assert reloaded is not None - assert reloaded.connections[0].secret_name == "llm_connection_abc" - assert reloaded.connections[0].base_url == "https://proxy.example/v1" - assert reloaded.connections[0].api_mode == "chat" - assert reloaded.connections[0].custom_headers == {"X-Org": "eng"} - assert reloaded.schema_version == 1 - - -def test_persisted_connections_schema_version_guard(): - # Newer schema versions are rejected to avoid silent data loss. - with pytest.raises(ValueError): - PersistedConnections.from_persisted({"schema_version": 99, "connections": []}) diff --git a/tests/agent_server/test_llm_providers.py b/tests/agent_server/test_llm_providers.py new file mode 100644 index 0000000000..48701c93f1 --- /dev/null +++ b/tests/agent_server/test_llm_providers.py @@ -0,0 +1,219 @@ +"""Tests for the Model Provider endpoints (OpenHands/OpenHands#15492).""" + +from __future__ import annotations + +import tempfile +from pathlib import Path +from unittest.mock import patch + +import pytest +from fastapi.testclient import TestClient + +from openhands.agent_server import llm_providers as prov_module +from openhands.agent_server.api import create_app +from openhands.agent_server.config import Config +from openhands.agent_server.persistence import ( + FileProvidersStore, + get_secrets_store, + reset_stores, +) + + +@pytest.fixture +def temp_dirs(): + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + (base / "profiles").mkdir(parents=True, exist_ok=True) + yield base + + +@pytest.fixture +def client(temp_dirs, monkeypatch): + reset_stores() + monkeypatch.setenv("OH_PERSISTENCE_DIR", str(temp_dirs)) + config = Config(static_files_path=None, session_api_keys=[], secret_key=None) + with patch( + "openhands.agent_server.llm_providers.get_providers_store", + lambda *_a, **_kw: FileProvidersStore(persistence_dir=temp_dirs), + ): + app = create_app(config) + yield TestClient(app) + reset_stores() + + +def _create(client, **overrides): + body = { + "display_name": "OpenAI", + "kind": "openai", + "key": "sk-test", + "base_url": "https://api.openai.com/v1", + "wire_api": "chat", + "custom_headers": {"X-Org": "eng"}, + "models": [{"name": "gpt-5.6-luna"}], + } + body.update(overrides) + return client.post("/api/llm/model-providers", json=body) + + +def test_list_empty(client): + r = client.get("/api/llm/model-providers") + assert r.status_code == 200 + assert r.json() == [] + + +def test_create_then_list(client): + r = _create(client) + assert r.status_code == 201 + body = r.json() + assert body["display_name"] == "OpenAI" + assert body["kind"] == "openai" + assert body["base_url"] == "https://api.openai.com/v1" + assert body["wire_api"] == "chat" + assert body["custom_headers"] == {"X-Org": "eng"} + assert body["models"] == [{"name": "gpt-5.6-luna", "wire_api": None}] + assert body["api_key_set"] is True + # Key/secret never echoed. + assert "key" not in body + assert "secret_name" not in body + + r2 = client.get("/api/llm/model-providers") + assert r2.status_code == 200 + listed = r2.json() + assert len(listed) == 1 + assert listed[0]["id"] == body["id"] + assert listed[0]["api_key_set"] is True + + +def test_key_stored_as_named_secret(client, temp_dirs, monkeypatch): + r = _create(client) + pid = r.json()["id"] + monkeypatch.setenv("OH_PERSISTENCE_DIR", str(temp_dirs)) + reset_stores() + store = get_secrets_store() + assert store.get_secret(f"llm_provider_{pid}") == "sk-test" + + +def test_get_and_404(client): + r = _create(client) + pid = r.json()["id"] + assert client.get(f"/api/llm/model-providers/{pid}").status_code == 200 + assert client.get("/api/llm/model-providers/nope").status_code == 404 + + +def test_update_fields_and_rotate_key(client, temp_dirs, monkeypatch): + pid = _create(client).json()["id"] + r = client.patch( + f"/api/llm/model-providers/{pid}", + json={ + "display_name": "OpenAI Prod", + "key": "sk-rotated", + "wire_api": "responses", + }, + ) + assert r.status_code == 200 + body = r.json() + assert body["display_name"] == "OpenAI Prod" + assert body["wire_api"] == "responses" + + monkeypatch.setenv("OH_PERSISTENCE_DIR", str(temp_dirs)) + reset_stores() + assert get_secrets_store().get_secret(f"llm_provider_{pid}") == "sk-rotated" + + +def test_update_requires_a_field(client): + pid = _create(client).json()["id"] + r = client.patch(f"/api/llm/model-providers/{pid}", json={}) + assert r.status_code == 422 + + +def test_delete_removes_provider_and_secret(client, temp_dirs, monkeypatch): + pid = _create(client).json()["id"] + r = client.delete(f"/api/llm/model-providers/{pid}") + assert r.status_code == 200 + assert r.json()["api_key_set"] is False + assert client.get(f"/api/llm/model-providers/{pid}").status_code == 404 + + monkeypatch.setenv("OH_PERSISTENCE_DIR", str(temp_dirs)) + reset_stores() + assert get_secrets_store().get_secret(f"llm_provider_{pid}") is None + + +def test_add_edit_remove_model(client): + pid = _create(client).json()["id"] + + # Add + r = client.post( + f"/api/llm/model-providers/{pid}/models", + json={"name": "gpt-5.6-sol", "wire_api": "responses"}, + ) + assert r.status_code == 201 + names = [m["name"] for m in r.json()["models"]] + assert names == ["gpt-5.6-luna", "gpt-5.6-sol"] + + # Duplicate add -> 409 + dup = client.post( + f"/api/llm/model-providers/{pid}/models", json={"name": "gpt-5.6-sol"} + ) + assert dup.status_code == 409 + + # Edit (rename + change wire api) + r = client.patch( + f"/api/llm/model-providers/{pid}/models/gpt-5.6-sol", + json={"name": "gpt-5.6-terra", "wire_api": "chat"}, + ) + assert r.status_code == 200 + models = {m["name"]: m["wire_api"] for m in r.json()["models"]} + assert models == {"gpt-5.6-luna": None, "gpt-5.6-terra": "chat"} + + # Remove + r = client.delete(f"/api/llm/model-providers/{pid}/models/gpt-5.6-luna") + assert r.status_code == 200 + assert [m["name"] for m in r.json()["models"]] == ["gpt-5.6-terra"] + + # Remove missing -> 404 + assert ( + client.delete(f"/api/llm/model-providers/{pid}/models/nope").status_code == 404 + ) + + +def test_test_probe_never_mutates_models(client, monkeypatch): + pid = _create(client).json()["id"] + + monkeypatch.setattr(prov_module, "_live_probe", lambda *a, **k: (True, None)) + r = client.post(f"/api/llm/model-providers/{pid}/test") + assert r.status_code == 200 + body = r.json() + assert body["ok"] is True + assert body["verified"] is True + assert isinstance(body["suggested_models"], list) + + # The provider's curated model list is unchanged. + after = client.get(f"/api/llm/model-providers/{pid}").json() + assert [m["name"] for m in after["models"]] == ["gpt-5.6-luna"] + + +def test_test_probe_reports_bad_key(client, monkeypatch): + pid = _create(client).json()["id"] + monkeypatch.setattr( + prov_module, "_live_probe", lambda *a, **k: (False, "401 invalid key") + ) + r = client.post(f"/api/llm/model-providers/{pid}/test") + assert r.status_code == 200 + body = r.json() + assert body["ok"] is False + assert body["verified"] is False + assert body["suggested_models"] == [] + assert "401" in body["error"] + + +def test_custom_endpoint_test_offers_catalog_without_probe(client): + # A kind litellm doesn't recognize (a custom OpenAI-compatible endpoint): + # ``test`` can't probe it, so it returns ok=True but verified=False. + pid = _create(client, kind="my-vllm", base_url="http://localhost:1234/v1").json()[ + "id" + ] + r = client.post(f"/api/llm/model-providers/{pid}/test") + assert r.status_code == 200 + body = r.json() + assert body["ok"] is True + assert body["verified"] is False