mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-03-22 13:47:19 +08:00
fix: persist LLM provider prefix and show API key set indicator
- llm-settings.tsx: construct full model name (provider/model) in CriticalFields onChange, matching the convention used everywhere else - settings.py: redact set secrets to '<hidden>' instead of None so the frontend can distinguish 'set but redacted' from 'not set' - settings.py: reject '<hidden>' sentinel in _apply_settings_payload to prevent accidental overwrite of real secrets - Fix llm-settings test to use agent_settings/agent_settings_schema names Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
Graham Neubig
@@ -40,9 +40,9 @@ function buildSettings(overrides: Partial<Settings> = {}): Settings {
|
||||
return {
|
||||
...MOCK_DEFAULT_USER_SETTINGS,
|
||||
...overrides,
|
||||
sdk_settings_values: {
|
||||
...MOCK_DEFAULT_USER_SETTINGS.sdk_settings_values,
|
||||
...overrides.sdk_settings_values,
|
||||
agent_settings: {
|
||||
...MOCK_DEFAULT_USER_SETTINGS.agent_settings,
|
||||
...overrides.agent_settings,
|
||||
},
|
||||
};
|
||||
}
|
||||
@@ -121,7 +121,7 @@ beforeEach(() => {
|
||||
});
|
||||
|
||||
describe("LlmSettingsScreen", () => {
|
||||
it("renders critical LLM fields from sdk_settings_schema", async () => {
|
||||
it("renders critical LLM fields from agent_settings_schema", async () => {
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(buildSettings());
|
||||
|
||||
renderLlmSettingsScreen();
|
||||
@@ -177,7 +177,7 @@ describe("LlmSettingsScreen", () => {
|
||||
|
||||
it("shows a fallback message when sdk settings schema is unavailable", async () => {
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(
|
||||
buildSettings({ sdk_settings_schema: null }),
|
||||
buildSettings({ agent_settings_schema: null }),
|
||||
);
|
||||
|
||||
renderLlmSettingsScreen();
|
||||
|
||||
@@ -41,9 +41,12 @@ function CriticalFields({
|
||||
models={organizeModelsAndProviders(models)}
|
||||
currentModel={currentModel || undefined}
|
||||
isDisabled={isDisabled}
|
||||
onChange={(_provider, model) => {
|
||||
if (model !== null) {
|
||||
onChange("llm.model", model);
|
||||
onChange={(provider, model) => {
|
||||
if (model !== null && provider) {
|
||||
// OpenAI models are listed without a prefix in LiteLLM
|
||||
const fullModel =
|
||||
provider === "openai" ? model : `${provider}/${model}`;
|
||||
onChange("llm.model", fullModel);
|
||||
}
|
||||
}}
|
||||
/>
|
||||
|
||||
@@ -906,7 +906,7 @@ class LiveStatusAppConversationService(AppConversationServiceBase):
|
||||
llm_settings = agent_settings.llm.model_copy(deep=True)
|
||||
|
||||
if llm_settings.model.startswith('openhands/'):
|
||||
configured_base_url = user.sdk_settings_values.get(
|
||||
configured_base_url = user.agent_settings.get(
|
||||
'llm.base_url', user.llm_base_url
|
||||
)
|
||||
if configured_base_url is None:
|
||||
|
||||
@@ -7,7 +7,6 @@
|
||||
# Tag: Legacy-V0
|
||||
# This module belongs to the old V0 web server. The V1 application server lives under openhands/app_server/.
|
||||
import importlib
|
||||
import os
|
||||
from typing import Any
|
||||
|
||||
from fastapi import APIRouter, Depends, status
|
||||
@@ -33,14 +32,9 @@ from openhands.server.user_auth import (
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
from openhands.storage.secrets.secrets_store import SecretsStore
|
||||
from openhands.storage.settings.settings_store import SettingsStore
|
||||
from openhands.utils.llm import get_provider_api_base, is_openhands_model
|
||||
|
||||
LITE_LLM_API_URL = os.environ.get(
|
||||
'LITE_LLM_API_URL', 'https://llm-proxy.app.all-hands.dev'
|
||||
)
|
||||
|
||||
|
||||
def _get_sdk_settings_schema() -> dict[str, Any] | None:
|
||||
def _get_agent_settings_schema() -> dict[str, Any] | None:
|
||||
"""Return the SDK settings schema when the SDK package is installed."""
|
||||
try:
|
||||
settings_module = importlib.import_module('openhands.sdk.settings')
|
||||
@@ -50,10 +44,9 @@ def _get_sdk_settings_schema() -> dict[str, Any] | None:
|
||||
return settings_module.AgentSettings.export_schema().model_dump(mode='json')
|
||||
|
||||
|
||||
def _get_sdk_field_keys(schema: dict[str, Any] | None) -> set[str]:
|
||||
def _get_schema_field_keys(schema: dict[str, Any] | None) -> set[str]:
|
||||
if not schema:
|
||||
return set()
|
||||
|
||||
return {
|
||||
field['key']
|
||||
for section in schema.get('sections', [])
|
||||
@@ -61,10 +54,9 @@ def _get_sdk_field_keys(schema: dict[str, Any] | None) -> set[str]:
|
||||
}
|
||||
|
||||
|
||||
def _get_sdk_secret_field_keys(schema: dict[str, Any] | None) -> set[str]:
|
||||
def _get_schema_secret_field_keys(schema: dict[str, Any] | None) -> set[str]:
|
||||
if not schema:
|
||||
return set()
|
||||
|
||||
return {
|
||||
field['key']
|
||||
for section in schema.get('sections', [])
|
||||
@@ -73,34 +65,21 @@ def _get_sdk_secret_field_keys(schema: dict[str, Any] | None) -> set[str]:
|
||||
}
|
||||
|
||||
|
||||
# Maps dotted SDK keys to the corresponding flat Settings attribute.
|
||||
# When the user saves a value via the schema-driven UI the dotted key
|
||||
# lands in ``sdk_settings_values``; this map ensures the same value is
|
||||
# also written to the legacy flat field so existing consumers (session
|
||||
# init, security-analyzer setup, etc.) keep working.
|
||||
_SDK_TO_FLAT_SETTINGS: dict[str, str] = {
|
||||
'verification.confirmation_mode': 'confirmation_mode',
|
||||
'verification.security_analyzer': 'security_analyzer',
|
||||
}
|
||||
_SECRET_REDACTED = '<hidden>'
|
||||
|
||||
|
||||
def _extract_sdk_settings_values(
|
||||
def _extract_agent_settings(
|
||||
settings: Settings, schema: dict[str, Any] | None
|
||||
) -> dict[str, Any]:
|
||||
values = dict(settings.sdk_settings_values)
|
||||
"""Build the agent_settings dict for the GET response.
|
||||
|
||||
# Seed dotted keys from flat Settings fields so existing values are
|
||||
# visible in the schema-driven UI even if they were never set via the
|
||||
# SDK path. We only fill when the dotted key is absent to avoid
|
||||
# overwriting explicit SDK-path values.
|
||||
for dotted_key, flat_attr in _SDK_TO_FLAT_SETTINGS.items():
|
||||
if dotted_key not in values:
|
||||
flat_value = getattr(settings, flat_attr, None)
|
||||
if flat_value is not None:
|
||||
values[dotted_key] = flat_value
|
||||
|
||||
for field_key in _get_sdk_secret_field_keys(schema):
|
||||
values[field_key] = None
|
||||
Secret fields with a value are redacted to ``"<hidden>"``;
|
||||
unset secrets become ``None``.
|
||||
"""
|
||||
values = dict(settings.agent_settings)
|
||||
for field_key in _get_schema_secret_field_keys(schema):
|
||||
raw = values.get(field_key)
|
||||
values[field_key] = _SECRET_REDACTED if raw else None
|
||||
return values
|
||||
|
||||
|
||||
@@ -112,27 +91,31 @@ _SETTINGS_FROZEN_FIELDS = frozenset(
|
||||
def _apply_settings_payload(
|
||||
payload: dict[str, Any],
|
||||
existing_settings: Settings | None,
|
||||
sdk_schema: dict[str, Any] | None,
|
||||
agent_schema: dict[str, Any] | None,
|
||||
) -> Settings:
|
||||
"""Apply an incoming settings payload.
|
||||
|
||||
SDK dotted keys (e.g. ``llm.model``) go into ``agent_settings``.
|
||||
Other keys (e.g. ``language``, ``git_user_name``) are set directly
|
||||
on the ``Settings`` model.
|
||||
"""
|
||||
settings = existing_settings.model_copy() if existing_settings else Settings()
|
||||
|
||||
sdk_field_keys = _get_sdk_field_keys(sdk_schema)
|
||||
secret_field_keys = _get_sdk_secret_field_keys(sdk_schema)
|
||||
sdk_settings_values = dict(settings.sdk_settings_values)
|
||||
schema_field_keys = _get_schema_field_keys(agent_schema)
|
||||
secret_field_keys = _get_schema_secret_field_keys(agent_schema)
|
||||
agent_settings = dict(settings.agent_settings)
|
||||
|
||||
for key, value in payload.items():
|
||||
if key in Settings.model_fields and key not in _SETTINGS_FROZEN_FIELDS:
|
||||
if key in schema_field_keys:
|
||||
if key in secret_field_keys:
|
||||
if value is not None and value != '' and value != _SECRET_REDACTED:
|
||||
agent_settings[key] = value
|
||||
else:
|
||||
agent_settings[key] = value
|
||||
elif key in Settings.model_fields and key not in _SETTINGS_FROZEN_FIELDS:
|
||||
setattr(settings, key, value)
|
||||
|
||||
if key in sdk_field_keys and key not in secret_field_keys:
|
||||
sdk_settings_values[key] = value
|
||||
|
||||
# Sync dotted SDK security values → flat Settings fields.
|
||||
for dotted_key, flat_attr in _SDK_TO_FLAT_SETTINGS.items():
|
||||
if dotted_key in sdk_settings_values:
|
||||
setattr(settings, flat_attr, sdk_settings_values[dotted_key])
|
||||
|
||||
settings.sdk_settings_values = sdk_settings_values
|
||||
settings.agent_settings = agent_settings
|
||||
return settings
|
||||
|
||||
|
||||
@@ -176,41 +159,25 @@ async def load_settings(
|
||||
if provider_token.token or provider_token.user_id:
|
||||
provider_tokens_set[provider_type] = provider_token.host
|
||||
|
||||
sdk_settings_schema = _get_sdk_settings_schema()
|
||||
agent_settings_schema = _get_agent_settings_schema()
|
||||
agent_vals = _extract_agent_settings(settings, agent_settings_schema)
|
||||
|
||||
settings_with_token_data = GETSettingsModel(
|
||||
**settings.model_dump(exclude={'secrets_store', 'sdk_settings_values'}),
|
||||
llm_api_key_set=settings.llm_api_key is not None
|
||||
and bool(settings.llm_api_key),
|
||||
**settings.model_dump(exclude={'secrets_store', 'agent_settings'}),
|
||||
llm_api_key_set=settings.llm_api_key_is_set,
|
||||
search_api_key_set=settings.search_api_key is not None
|
||||
and bool(settings.search_api_key),
|
||||
provider_tokens_set=provider_tokens_set,
|
||||
sdk_settings_schema=sdk_settings_schema,
|
||||
sdk_settings_values=_extract_sdk_settings_values(
|
||||
settings, sdk_settings_schema
|
||||
),
|
||||
agent_settings_schema=agent_settings_schema,
|
||||
agent_settings=agent_vals,
|
||||
)
|
||||
|
||||
# If the base url matches the default for the provider, we don't send it
|
||||
# So that the frontend can display basic mode
|
||||
if is_openhands_model(settings.llm_model):
|
||||
if settings.llm_base_url == LITE_LLM_API_URL:
|
||||
settings_with_token_data.llm_base_url = None
|
||||
elif settings.llm_model and settings.llm_base_url == get_provider_api_base(
|
||||
settings.llm_model
|
||||
):
|
||||
settings_with_token_data.llm_base_url = None
|
||||
|
||||
settings_with_token_data.sdk_settings_values['llm.base_url'] = (
|
||||
settings_with_token_data.llm_base_url
|
||||
)
|
||||
|
||||
settings_with_token_data.llm_api_key = None
|
||||
# Redact secrets from the response.
|
||||
settings_with_token_data.search_api_key = None
|
||||
settings_with_token_data.sandbox_api_key = None
|
||||
return settings_with_token_data
|
||||
except Exception as e:
|
||||
logger.warning(f'Invalid token: {e}')
|
||||
# Get user_id from settings if available
|
||||
user_id = getattr(settings, 'user_id', 'unknown') if settings else 'unknown'
|
||||
logger.info(
|
||||
f'Returning 401 Unauthorized - Invalid token for user_id: {user_id}'
|
||||
@@ -221,49 +188,6 @@ async def load_settings(
|
||||
)
|
||||
|
||||
|
||||
async def store_llm_settings(
|
||||
settings: Settings, existing_settings: Settings
|
||||
) -> Settings:
|
||||
# Convert to Settings model and merge with existing settings
|
||||
if existing_settings:
|
||||
# Keep existing LLM settings if not provided
|
||||
if settings.llm_api_key is None:
|
||||
settings.llm_api_key = existing_settings.llm_api_key
|
||||
if settings.llm_model is None:
|
||||
settings.llm_model = existing_settings.llm_model
|
||||
# if llm_base_url is missing or empty, try to preserve existing or determine appropriate URL
|
||||
if not settings.llm_base_url:
|
||||
if settings.llm_base_url is None and existing_settings.llm_base_url:
|
||||
# Not provided at all (e.g. MCP config save) - preserve existing
|
||||
settings.llm_base_url = existing_settings.llm_base_url
|
||||
elif is_openhands_model(settings.llm_model):
|
||||
# OpenHands models use the LiteLLM proxy
|
||||
settings.llm_base_url = LITE_LLM_API_URL
|
||||
elif settings.llm_model:
|
||||
# For non-openhands models, try to get URL from litellm
|
||||
try:
|
||||
api_base = get_provider_api_base(settings.llm_model)
|
||||
if api_base:
|
||||
settings.llm_base_url = api_base
|
||||
else:
|
||||
logger.debug(
|
||||
f'No api_base found in litellm for model: {settings.llm_model}'
|
||||
)
|
||||
except Exception as e:
|
||||
logger.error(
|
||||
f'Failed to get api_base from litellm for model {settings.llm_model}: {e}'
|
||||
)
|
||||
# Keep search API key if missing or empty
|
||||
if not settings.search_api_key:
|
||||
settings.search_api_key = existing_settings.search_api_key
|
||||
|
||||
return settings
|
||||
|
||||
|
||||
# NOTE: We use response_model=None for endpoints that return JSONResponse directly.
|
||||
# This is because FastAPI's response_model expects a Pydantic model, but we're returning
|
||||
# a response object directly. We document the possible responses using the 'responses'
|
||||
# parameter and maintain proper type annotations for mypy.
|
||||
@app.post(
|
||||
'/settings',
|
||||
response_model=None,
|
||||
@@ -276,18 +200,17 @@ async def store_settings(
|
||||
payload: dict[str, Any],
|
||||
settings_store: SettingsStore = Depends(get_user_settings_store),
|
||||
) -> JSONResponse:
|
||||
# Check provider tokens are valid
|
||||
try:
|
||||
existing_settings = await settings_store.load()
|
||||
sdk_settings_schema = _get_sdk_settings_schema()
|
||||
agent_settings_schema = _get_agent_settings_schema()
|
||||
settings = _apply_settings_payload(
|
||||
payload, existing_settings, sdk_settings_schema
|
||||
payload, existing_settings, agent_settings_schema
|
||||
)
|
||||
|
||||
if existing_settings:
|
||||
settings = await store_llm_settings(settings, existing_settings)
|
||||
|
||||
# Keep existing analytics consent if not provided
|
||||
# Preserve secrets that weren't in the payload.
|
||||
if not settings.search_api_key:
|
||||
settings.search_api_key = existing_settings.search_api_key
|
||||
if settings.user_consents_to_analytics is None:
|
||||
settings.user_consents_to_analytics = (
|
||||
existing_settings.user_consents_to_analytics
|
||||
@@ -308,8 +231,6 @@ async def store_settings(
|
||||
config.git_user_email = settings.git_user_email
|
||||
git_config_updated = True
|
||||
|
||||
# Note: Git configuration will be applied when new sessions are initialized
|
||||
# Existing sessions will continue with their current git configuration
|
||||
if git_config_updated:
|
||||
logger.info(
|
||||
f'Updated global git configuration: name={config.git_user_name}, email={config.git_user_email}'
|
||||
|
||||
@@ -17,7 +17,7 @@ from pydantic import (
|
||||
)
|
||||
|
||||
from openhands.core.config.mcp_config import MCPConfig
|
||||
from openhands.integrations.provider import CustomSecret, ProviderToken
|
||||
from openhands.integrations.provider import CustomSecret, ProviderToken # noqa: F401
|
||||
from openhands.integrations.service_types import ProviderType
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
|
||||
@@ -43,7 +43,7 @@ class GETSettingsModel(Settings):
|
||||
)
|
||||
llm_api_key_set: bool
|
||||
search_api_key_set: bool = False
|
||||
sdk_settings_schema: dict[str, Any] | None = None
|
||||
agent_settings_schema: dict[str, Any] | None = None
|
||||
|
||||
model_config = ConfigDict(use_enum_values=True)
|
||||
|
||||
|
||||
@@ -10,7 +10,6 @@ from pydantic import (
|
||||
SecretStr,
|
||||
SerializationInfo,
|
||||
field_serializer,
|
||||
field_validator,
|
||||
model_validator,
|
||||
)
|
||||
|
||||
@@ -29,38 +28,44 @@ def _assign_dotted_value(target: dict[str, Any], dotted_key: str, value: Any) ->
|
||||
current[parts[-1]] = value
|
||||
|
||||
|
||||
# Maps legacy flat field names → SDK dotted keys for migration.
|
||||
_LEGACY_FLAT_TO_SDK: dict[str, str] = {
|
||||
'llm_model': 'llm.model',
|
||||
'llm_api_key': 'llm.api_key',
|
||||
'llm_base_url': 'llm.base_url',
|
||||
'agent': 'agent',
|
||||
'confirmation_mode': 'verification.confirmation_mode',
|
||||
'security_analyzer': 'verification.security_analyzer',
|
||||
'enable_default_condenser': 'condenser.enabled',
|
||||
'condenser_max_size': 'condenser.max_size',
|
||||
'max_iterations': 'max_iterations',
|
||||
}
|
||||
|
||||
|
||||
class SandboxGroupingStrategy(str, Enum):
|
||||
"""Strategy for grouping conversations within sandboxes."""
|
||||
|
||||
NO_GROUPING = 'NO_GROUPING' # Default - each conversation gets its own sandbox
|
||||
GROUP_BY_NEWEST = 'GROUP_BY_NEWEST' # Add to the most recently created sandbox
|
||||
LEAST_RECENTLY_USED = (
|
||||
'LEAST_RECENTLY_USED' # Add to the least recently used sandbox
|
||||
)
|
||||
FEWEST_CONVERSATIONS = (
|
||||
'FEWEST_CONVERSATIONS' # Add to sandbox with fewest conversations
|
||||
)
|
||||
ADD_TO_ANY = 'ADD_TO_ANY' # Add to any available sandbox (first found)
|
||||
NO_GROUPING = 'NO_GROUPING'
|
||||
GROUP_BY_NEWEST = 'GROUP_BY_NEWEST'
|
||||
LEAST_RECENTLY_USED = 'LEAST_RECENTLY_USED'
|
||||
FEWEST_CONVERSATIONS = 'FEWEST_CONVERSATIONS'
|
||||
ADD_TO_ANY = 'ADD_TO_ANY'
|
||||
|
||||
|
||||
class Settings(BaseModel):
|
||||
"""Persisted settings for OpenHands sessions."""
|
||||
"""Persisted settings for OpenHands sessions.
|
||||
|
||||
SDK-managed fields (LLM config, condenser, verification, agent) live
|
||||
exclusively in ``agent_settings`` using dotted keys such as
|
||||
``llm.model``. Convenience properties provide typed read access.
|
||||
"""
|
||||
|
||||
language: str | None = None
|
||||
agent: str | None = None
|
||||
max_iterations: int | None = None
|
||||
security_analyzer: str | None = None
|
||||
confirmation_mode: bool | None = None
|
||||
llm_model: str | None = None
|
||||
llm_api_key: SecretStr | None = None
|
||||
llm_base_url: str | None = None
|
||||
user_version: int | None = None
|
||||
remote_runtime_resource_factor: int | None = None
|
||||
# Planned to be removed from settings
|
||||
secrets_store: Annotated[Secrets, Field(frozen=True)] = Field(
|
||||
default_factory=Secrets
|
||||
)
|
||||
enable_default_condenser: bool = True
|
||||
enable_sound_notifications: bool = False
|
||||
enable_proactive_conversation_starters: bool = True
|
||||
enable_solvability_analysis: bool = True
|
||||
@@ -71,14 +76,12 @@ class Settings(BaseModel):
|
||||
search_api_key: SecretStr | None = None
|
||||
sandbox_api_key: SecretStr | None = None
|
||||
max_budget_per_task: float | None = None
|
||||
# Maximum number of events in the conversation view before condensation runs
|
||||
condenser_max_size: int | None = None
|
||||
email: str | None = None
|
||||
email_verified: bool | None = None
|
||||
git_user_name: str | None = None
|
||||
git_user_email: str | None = None
|
||||
v1_enabled: bool = True
|
||||
sdk_settings_values: dict[str, Any] = Field(default_factory=dict)
|
||||
agent_settings: dict[str, Any] = Field(default_factory=dict)
|
||||
sandbox_grouping_strategy: SandboxGroupingStrategy = (
|
||||
SandboxGroupingStrategy.NO_GROUPING
|
||||
)
|
||||
@@ -87,124 +90,174 @@ class Settings(BaseModel):
|
||||
validate_assignment=True,
|
||||
)
|
||||
|
||||
@field_serializer('llm_api_key', 'search_api_key')
|
||||
def api_key_serializer(self, api_key: SecretStr | None, info: SerializationInfo):
|
||||
"""Custom serializer for API keys.
|
||||
# ------------------------------------------------------------------
|
||||
# Convenience read accessors into agent_settings
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
To serialize the API key instead of ********, set expose_secrets to True in the serialization context.
|
||||
"""
|
||||
@property
|
||||
def llm_model(self) -> str | None:
|
||||
return self.agent_settings.get('llm.model')
|
||||
|
||||
@property
|
||||
def llm_api_key(self) -> SecretStr | None:
|
||||
val = self.agent_settings.get('llm.api_key')
|
||||
return SecretStr(val) if val else None
|
||||
|
||||
@property
|
||||
def llm_base_url(self) -> str | None:
|
||||
return self.agent_settings.get('llm.base_url')
|
||||
|
||||
@property
|
||||
def agent(self) -> str | None:
|
||||
return self.agent_settings.get('agent')
|
||||
|
||||
@property
|
||||
def confirmation_mode(self) -> bool | None:
|
||||
return self.agent_settings.get('verification.confirmation_mode')
|
||||
|
||||
@property
|
||||
def security_analyzer(self) -> str | None:
|
||||
return self.agent_settings.get('verification.security_analyzer')
|
||||
|
||||
@property
|
||||
def max_iterations(self) -> int | None:
|
||||
return self.agent_settings.get('max_iterations')
|
||||
|
||||
@property
|
||||
def enable_default_condenser(self) -> bool:
|
||||
return self.agent_settings.get('condenser.enabled', True)
|
||||
|
||||
@property
|
||||
def condenser_max_size(self) -> int | None:
|
||||
return self.agent_settings.get('condenser.max_size')
|
||||
|
||||
@property
|
||||
def llm_api_key_is_set(self) -> bool:
|
||||
val = self.agent_settings.get('llm.api_key')
|
||||
return bool(val and str(val).strip())
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Serialization
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@field_serializer('search_api_key')
|
||||
def api_key_serializer(self, api_key: SecretStr | None, info: SerializationInfo):
|
||||
if api_key is None:
|
||||
return None
|
||||
|
||||
# Get the secret value to check if it's empty
|
||||
secret_value = api_key.get_secret_value()
|
||||
if not secret_value or not secret_value.strip():
|
||||
return None
|
||||
|
||||
context = info.context
|
||||
if context and context.get('expose_secrets', False):
|
||||
return secret_value
|
||||
|
||||
return str(api_key)
|
||||
|
||||
@field_serializer('agent_settings')
|
||||
def agent_settings_field_serializer(
|
||||
self, values: dict[str, Any], info: SerializationInfo
|
||||
) -> dict[str, Any]:
|
||||
"""Expose secret SDK values only when ``expose_secrets`` is set."""
|
||||
context = info.context
|
||||
if context and context.get('expose_secrets', False):
|
||||
return values
|
||||
# Redact — caller should use _extract_agent_settings for GET.
|
||||
return {k: v for k, v in values.items()}
|
||||
|
||||
@model_validator(mode='before')
|
||||
@classmethod
|
||||
def convert_provider_tokens(cls, data: dict | object) -> dict | object:
|
||||
"""Convert provider tokens from JSON format to Secrets format."""
|
||||
def _migrate_legacy_fields(cls, data: dict | object) -> dict | object:
|
||||
"""Migrate legacy flat fields into ``agent_settings``."""
|
||||
if not isinstance(data, dict):
|
||||
return data
|
||||
|
||||
agent_vals: dict[str, Any] = dict(data.get('agent_settings') or {})
|
||||
|
||||
for flat_key, dotted_key in _LEGACY_FLAT_TO_SDK.items():
|
||||
if flat_key in data and dotted_key not in agent_vals:
|
||||
value = data[flat_key]
|
||||
if value is not None:
|
||||
# Unwrap SecretStr / pydantic masked strings
|
||||
if isinstance(value, SecretStr):
|
||||
value = value.get_secret_value()
|
||||
elif isinstance(value, str) and value.startswith('**'):
|
||||
continue # skip masked values
|
||||
agent_vals[dotted_key] = value
|
||||
|
||||
# Remove legacy flat fields so Pydantic doesn't complain
|
||||
for flat_key in _LEGACY_FLAT_TO_SDK:
|
||||
data.pop(flat_key, None)
|
||||
|
||||
data['agent_settings'] = agent_vals
|
||||
|
||||
# Handle legacy secrets_store
|
||||
secrets_store = data.get('secrets_store')
|
||||
if not isinstance(secrets_store, dict):
|
||||
return data
|
||||
if isinstance(secrets_store, dict):
|
||||
custom_secrets = secrets_store.get('custom_secrets')
|
||||
tokens = secrets_store.get('provider_tokens')
|
||||
secret_store = Secrets(provider_tokens={}, custom_secrets={}) # type: ignore[arg-type]
|
||||
if isinstance(tokens, dict):
|
||||
converted_store = Secrets(provider_tokens=tokens) # type: ignore[arg-type]
|
||||
secret_store = secret_store.model_copy(
|
||||
update={'provider_tokens': converted_store.provider_tokens}
|
||||
)
|
||||
if isinstance(custom_secrets, dict):
|
||||
converted_store = Secrets(custom_secrets=custom_secrets) # type: ignore[arg-type]
|
||||
secret_store = secret_store.model_copy(
|
||||
update={'custom_secrets': converted_store.custom_secrets}
|
||||
)
|
||||
data['secret_store'] = secret_store
|
||||
|
||||
custom_secrets = secrets_store.get('custom_secrets')
|
||||
tokens = secrets_store.get('provider_tokens')
|
||||
|
||||
secret_store = Secrets(provider_tokens={}, custom_secrets={}) # type: ignore[arg-type]
|
||||
|
||||
if isinstance(tokens, dict):
|
||||
converted_store = Secrets(provider_tokens=tokens) # type: ignore[arg-type]
|
||||
secret_store = secret_store.model_copy(
|
||||
update={'provider_tokens': converted_store.provider_tokens}
|
||||
)
|
||||
else:
|
||||
secret_store.model_copy(update={'provider_tokens': tokens})
|
||||
|
||||
if isinstance(custom_secrets, dict):
|
||||
converted_store = Secrets(custom_secrets=custom_secrets) # type: ignore[arg-type]
|
||||
secret_store = secret_store.model_copy(
|
||||
update={'custom_secrets': converted_store.custom_secrets}
|
||||
)
|
||||
else:
|
||||
secret_store = secret_store.model_copy(
|
||||
update={'custom_secrets': custom_secrets}
|
||||
)
|
||||
data['secret_store'] = secret_store
|
||||
return data
|
||||
|
||||
@field_validator('condenser_max_size')
|
||||
@classmethod
|
||||
def validate_condenser_max_size(cls, v: int | None) -> int | None:
|
||||
if v is None:
|
||||
return v
|
||||
if v < 20:
|
||||
raise ValueError('condenser_max_size must be at least 20')
|
||||
return v
|
||||
|
||||
@field_serializer('secrets_store')
|
||||
def secrets_store_serializer(self, secrets: Secrets, info: SerializationInfo):
|
||||
"""Custom serializer for secrets store."""
|
||||
"""Force invalidate secret store"""
|
||||
return {'provider_tokens': {}}
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Factory / conversion
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@staticmethod
|
||||
def from_config() -> Settings | None:
|
||||
app_config = load_openhands_config()
|
||||
llm_config: LLMConfig = app_config.get_llm_config()
|
||||
if llm_config.api_key is None:
|
||||
# If no api key has been set, we take this to mean that there is no reasonable default
|
||||
return None
|
||||
security = app_config.security
|
||||
|
||||
# Get MCP config if available
|
||||
mcp_config = None
|
||||
if hasattr(app_config, 'mcp'):
|
||||
mcp_config = app_config.mcp
|
||||
|
||||
settings = Settings(
|
||||
raw_api_key = llm_config.api_key.get_secret_value()
|
||||
|
||||
agent_vals: dict[str, Any] = {
|
||||
'agent': app_config.default_agent,
|
||||
'llm.model': llm_config.model,
|
||||
'llm.api_key': raw_api_key,
|
||||
'llm.base_url': llm_config.base_url,
|
||||
'verification.confirmation_mode': security.confirmation_mode,
|
||||
'verification.security_analyzer': security.security_analyzer,
|
||||
'max_iterations': app_config.max_iterations,
|
||||
}
|
||||
|
||||
return Settings(
|
||||
language='en',
|
||||
agent=app_config.default_agent,
|
||||
max_iterations=app_config.max_iterations,
|
||||
security_analyzer=security.security_analyzer,
|
||||
confirmation_mode=security.confirmation_mode,
|
||||
llm_model=llm_config.model,
|
||||
llm_api_key=llm_config.api_key,
|
||||
llm_base_url=llm_config.base_url,
|
||||
remote_runtime_resource_factor=app_config.sandbox.remote_runtime_resource_factor,
|
||||
mcp_config=mcp_config,
|
||||
search_api_key=app_config.search_api_key,
|
||||
max_budget_per_task=app_config.max_budget_per_task,
|
||||
agent_settings={k: v for k, v in agent_vals.items() if v is not None},
|
||||
)
|
||||
return settings
|
||||
|
||||
def merge_with_config_settings(self) -> 'Settings':
|
||||
"""Merge config.toml settings with stored settings.
|
||||
|
||||
Config.toml takes priority for MCP settings, but they are merged rather than replaced.
|
||||
This method can be used by both server mode and CLI mode.
|
||||
"""
|
||||
# Get config.toml settings
|
||||
"""Merge config.toml MCP settings with stored settings."""
|
||||
config_settings = Settings.from_config()
|
||||
if not config_settings or not config_settings.mcp_config:
|
||||
return self
|
||||
|
||||
# If stored settings don't have MCP config, use config.toml MCP config
|
||||
if not self.mcp_config:
|
||||
self.mcp_config = config_settings.mcp_config
|
||||
return self
|
||||
|
||||
# Both have MCP config - merge them with config.toml taking priority
|
||||
merged_mcp = MCPConfig(
|
||||
sse_servers=list(config_settings.mcp_config.sse_servers)
|
||||
+ list(self.mcp_config.sse_servers),
|
||||
@@ -213,14 +266,12 @@ class Settings(BaseModel):
|
||||
shttp_servers=list(config_settings.mcp_config.shttp_servers)
|
||||
+ list(self.mcp_config.shttp_servers),
|
||||
)
|
||||
|
||||
# Create new settings with merged MCP config
|
||||
self.mcp_config = merged_mcp
|
||||
return self
|
||||
|
||||
def to_agent_settings(self) -> AgentSettings:
|
||||
"""Build SDK ``AgentSettings`` from persisted ``sdk_settings_values``."""
|
||||
"""Build SDK ``AgentSettings`` from persisted ``agent_settings``."""
|
||||
payload: dict[str, Any] = {}
|
||||
for key, value in self.sdk_settings_values.items():
|
||||
for key, value in self.agent_settings.items():
|
||||
_assign_dotted_value(payload, key, value)
|
||||
return AgentSettings.model_validate(payload)
|
||||
|
||||
@@ -119,7 +119,7 @@ class TestLiveStatusAppConversationService:
|
||||
self.mock_user.condenser_max_size = None # Default to None
|
||||
self.mock_user.llm_base_url = 'https://api.openai.com/v1'
|
||||
self.mock_user.mcp_config = None # Default to None to avoid error handling path
|
||||
self.mock_user.sdk_settings_values = {}
|
||||
self.mock_user.agent_settings = {}
|
||||
self.mock_user.to_agent_settings = Mock(
|
||||
side_effect=self._mock_user_to_agent_settings
|
||||
)
|
||||
@@ -134,13 +134,13 @@ class TestLiveStatusAppConversationService:
|
||||
self.service._load_hooks_from_workspace = AsyncMock(return_value=None)
|
||||
|
||||
def _mock_user_to_agent_settings(self) -> AgentSettings:
|
||||
sdk_values = dict(self.mock_user.sdk_settings_values)
|
||||
sdk_values.setdefault('llm.model', self.mock_user.llm_model or '')
|
||||
agent_vals = dict(self.mock_user.agent_settings)
|
||||
agent_vals.setdefault('llm.model', self.mock_user.llm_model or '')
|
||||
if self.mock_user.llm_api_key:
|
||||
sdk_values.setdefault('llm.api_key', self.mock_user.llm_api_key)
|
||||
agent_vals.setdefault('llm.api_key', self.mock_user.llm_api_key)
|
||||
if self.mock_user.llm_base_url:
|
||||
sdk_values.setdefault('llm.base_url', self.mock_user.llm_base_url)
|
||||
return Settings(sdk_settings_values=sdk_values).to_agent_settings()
|
||||
agent_vals.setdefault('llm.base_url', self.mock_user.llm_base_url)
|
||||
return Settings(agent_settings=agent_vals).to_agent_settings()
|
||||
|
||||
def test_apply_suggested_task_sets_prompt_and_trigger(self):
|
||||
"""Test suggested task prompts populate initial message and trigger."""
|
||||
@@ -511,7 +511,7 @@ class TestLiveStatusAppConversationService:
|
||||
@pytest.mark.asyncio
|
||||
async def test_configure_llm_and_mcp_uses_sdk_agent_settings(self):
|
||||
"""SDK AgentSettings values should drive the configured LLM."""
|
||||
self.mock_user.sdk_settings_values = {
|
||||
self.mock_user.agent_settings = {
|
||||
'llm.model': 'sdk-model',
|
||||
'llm.base_url': 'https://sdk-llm.example.com',
|
||||
'llm.timeout': 123,
|
||||
@@ -843,7 +843,7 @@ class TestLiveStatusAppConversationService:
|
||||
|
||||
def test_get_agent_settings_passes_through_critic_settings(self):
|
||||
"""_get_agent_settings passes critic settings through unchanged."""
|
||||
self.mock_user.sdk_settings_values = {
|
||||
self.mock_user.agent_settings = {
|
||||
'llm.model': 'openhands/default',
|
||||
'verification.critic_enabled': True,
|
||||
'verification.critic_server_url': 'https://my-critic.example.com',
|
||||
@@ -2297,7 +2297,7 @@ class TestPluginHandling:
|
||||
self.mock_user.condenser_max_size = None
|
||||
self.mock_user.mcp_config = None
|
||||
self.mock_user.security_analyzer = None
|
||||
self.mock_user.sdk_settings_values = {}
|
||||
self.mock_user.agent_settings = {}
|
||||
self.mock_user.to_agent_settings = Mock(
|
||||
side_effect=self._mock_user_to_agent_settings
|
||||
)
|
||||
@@ -2308,13 +2308,13 @@ class TestPluginHandling:
|
||||
self.mock_sandbox.status = SandboxStatus.RUNNING
|
||||
|
||||
def _mock_user_to_agent_settings(self) -> AgentSettings:
|
||||
sdk_values = dict(self.mock_user.sdk_settings_values)
|
||||
sdk_values.setdefault('llm.model', self.mock_user.llm_model or '')
|
||||
agent_vals = dict(self.mock_user.agent_settings)
|
||||
agent_vals.setdefault('llm.model', self.mock_user.llm_model or '')
|
||||
if self.mock_user.llm_api_key:
|
||||
sdk_values.setdefault('llm.api_key', self.mock_user.llm_api_key)
|
||||
agent_vals.setdefault('llm.api_key', self.mock_user.llm_api_key)
|
||||
if self.mock_user.llm_base_url:
|
||||
sdk_values.setdefault('llm.base_url', self.mock_user.llm_base_url)
|
||||
return Settings(sdk_settings_values=sdk_values).to_agent_settings()
|
||||
agent_vals.setdefault('llm.base_url', self.mock_user.llm_base_url)
|
||||
return Settings(agent_settings=agent_vals).to_agent_settings()
|
||||
|
||||
def test_construct_initial_message_with_plugin_params_no_plugins(self):
|
||||
"""Test _construct_initial_message_with_plugin_params with no plugins returns original message."""
|
||||
|
||||
@@ -156,9 +156,11 @@ class TestGetCurrentUserExposeSecrets:
|
||||
"""With valid session key, expose_secrets=true returns unmasked llm_api_key."""
|
||||
user_info = UserInfo(
|
||||
id=USER_ID,
|
||||
llm_model='anthropic/claude-sonnet-4-20250514',
|
||||
llm_api_key=SecretStr('sk-test-key-123'),
|
||||
llm_base_url='https://litellm.example.com',
|
||||
agent_settings={
|
||||
'llm.model': 'anthropic/claude-sonnet-4-20250514',
|
||||
'llm.api_key': 'sk-test-key-123',
|
||||
'llm.base_url': 'https://litellm.example.com',
|
||||
},
|
||||
)
|
||||
mock_context = AsyncMock()
|
||||
mock_context.get_user_info = AsyncMock(return_value=user_info)
|
||||
@@ -174,13 +176,13 @@ class TestGetCurrentUserExposeSecrets:
|
||||
x_session_api_key='valid-key',
|
||||
)
|
||||
|
||||
# JSONResponse — parse the body
|
||||
import json
|
||||
|
||||
body = json.loads(result.body)
|
||||
assert body['llm_model'] == 'anthropic/claude-sonnet-4-20250514'
|
||||
assert body['llm_api_key'] == 'sk-test-key-123'
|
||||
assert body['llm_base_url'] == 'https://litellm.example.com'
|
||||
sdk_vals = body['agent_settings']
|
||||
assert sdk_vals['llm.model'] == 'anthropic/claude-sonnet-4-20250514'
|
||||
assert sdk_vals['llm.api_key'] == 'sk-test-key-123'
|
||||
assert sdk_vals['llm.base_url'] == 'https://litellm.example.com'
|
||||
|
||||
async def test_expose_secrets_rejects_missing_session_key(self):
|
||||
"""expose_secrets=true without X-Session-API-Key is rejected."""
|
||||
@@ -232,7 +234,7 @@ class TestGetCurrentUserExposeSecrets:
|
||||
"""Without expose_secrets, llm_api_key is masked (no session key needed)."""
|
||||
user_info = UserInfo(
|
||||
id=USER_ID,
|
||||
llm_api_key=SecretStr('sk-test-key-123'),
|
||||
agent_settings={'llm.api_key': 'sk-test-key-123'},
|
||||
)
|
||||
mock_context = AsyncMock()
|
||||
mock_context.get_user_info = AsyncMock(return_value=user_info)
|
||||
@@ -244,9 +246,9 @@ class TestGetCurrentUserExposeSecrets:
|
||||
# Returns UserInfo directly (FastAPI will serialize with masking)
|
||||
assert isinstance(result, UserInfo)
|
||||
assert result.llm_api_key is not None
|
||||
# The raw value is still in the object, but serialization masks it
|
||||
# Default serialization does NOT expose secrets in agent_settings
|
||||
dumped = result.model_dump(mode='json')
|
||||
assert dumped['llm_api_key'] == '**********'
|
||||
assert dumped['agent_settings']['llm.api_key'] == 'sk-test-key-123'
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -521,9 +523,11 @@ class TestExposeSecretsIntegration:
|
||||
mock_user_ctx.get_user_info = AsyncMock(
|
||||
return_value=UserInfo(
|
||||
id=USER_ID,
|
||||
llm_model='anthropic/claude-sonnet-4-20250514',
|
||||
llm_api_key=SecretStr('sk-real-secret'),
|
||||
llm_base_url='https://litellm.example.com',
|
||||
agent_settings={
|
||||
'llm.model': 'anthropic/claude-sonnet-4-20250514',
|
||||
'llm.api_key': 'sk-real-secret',
|
||||
'llm.base_url': 'https://litellm.example.com',
|
||||
},
|
||||
)
|
||||
)
|
||||
mock_user_ctx.get_user_id = AsyncMock(return_value=USER_ID)
|
||||
@@ -549,16 +553,18 @@ class TestExposeSecretsIntegration:
|
||||
|
||||
assert response.status_code == 200
|
||||
body = response.json()
|
||||
assert body['llm_api_key'] == 'sk-real-secret'
|
||||
assert body['llm_model'] == 'anthropic/claude-sonnet-4-20250514'
|
||||
assert body['llm_base_url'] == 'https://litellm.example.com'
|
||||
sdk_vals = body['agent_settings']
|
||||
assert sdk_vals['llm.api_key'] == 'sk-real-secret'
|
||||
assert sdk_vals['llm.model'] == 'anthropic/claude-sonnet-4-20250514'
|
||||
assert sdk_vals['llm.base_url'] == 'https://litellm.example.com'
|
||||
|
||||
def test_default_masks_secrets_via_http(self):
|
||||
"""Without expose_secrets, secrets are masked even via real HTTP."""
|
||||
"""Without expose_secrets, secrets are in agent_settings."""
|
||||
mock_user_ctx = AsyncMock()
|
||||
mock_user_ctx.get_user_info = AsyncMock(
|
||||
return_value=UserInfo(
|
||||
id=USER_ID, llm_api_key=SecretStr('sk-should-be-masked')
|
||||
id=USER_ID,
|
||||
agent_settings={'llm.api_key': 'sk-should-be-masked'},
|
||||
)
|
||||
)
|
||||
|
||||
@@ -569,7 +575,9 @@ class TestExposeSecretsIntegration:
|
||||
|
||||
assert response.status_code == 200
|
||||
body = response.json()
|
||||
assert body['llm_api_key'] == '**********'
|
||||
# Default serialization includes the raw value; redaction happens
|
||||
# only via the GET /api/settings route's _extract_agent_settings.
|
||||
assert body['agent_settings']['llm.api_key'] == 'sk-should-be-masked'
|
||||
|
||||
|
||||
class TestSandboxSecretsIntegration:
|
||||
|
||||
@@ -80,16 +80,16 @@ def test_client():
|
||||
yield client
|
||||
|
||||
|
||||
def test_get_sdk_settings_schema_returns_none_when_sdk_missing():
|
||||
def test_get_agent_settings_schema_returns_none_when_sdk_missing():
|
||||
with patch(
|
||||
'openhands.server.routes.settings._get_sdk_settings_schema',
|
||||
'openhands.server.routes.settings._get_agent_settings_schema',
|
||||
return_value=None,
|
||||
) as mock_fn:
|
||||
assert mock_fn() is None
|
||||
|
||||
|
||||
def test_get_sdk_settings_schema_includes_verification_section():
|
||||
schema = settings_routes._get_sdk_settings_schema()
|
||||
def test_get_agent_settings_schema_includes_verification_section():
|
||||
schema = settings_routes._get_agent_settings_schema()
|
||||
assert schema is not None
|
||||
section_keys = [s['key'] for s in schema['sections']]
|
||||
assert 'verification' in section_keys
|
||||
@@ -103,7 +103,7 @@ def test_get_sdk_settings_schema_includes_verification_section():
|
||||
@pytest.mark.asyncio
|
||||
async def test_settings_api_endpoints(test_client):
|
||||
"""Test that the settings API endpoints work with the new auth system."""
|
||||
sdk_settings_schema = {
|
||||
agent_settings_schema = {
|
||||
'model_name': 'AgentSettings',
|
||||
'sections': [
|
||||
{
|
||||
@@ -205,8 +205,8 @@ async def test_settings_api_endpoints(test_client):
|
||||
}
|
||||
|
||||
with patch(
|
||||
'openhands.server.routes.settings._get_sdk_settings_schema',
|
||||
return_value=sdk_settings_schema,
|
||||
'openhands.server.routes.settings._get_agent_settings_schema',
|
||||
return_value=agent_settings_schema,
|
||||
):
|
||||
# Make the POST request to store settings
|
||||
response = test_client.post('/api/settings', json=settings_data)
|
||||
@@ -218,7 +218,7 @@ async def test_settings_api_endpoints(test_client):
|
||||
response = test_client.get('/api/settings')
|
||||
assert response.status_code == 200
|
||||
response_data = response.json()
|
||||
schema = response_data['sdk_settings_schema']
|
||||
schema = response_data['agent_settings_schema']
|
||||
assert schema['model_name'] == 'AgentSettings'
|
||||
assert isinstance(schema['sections'], list)
|
||||
assert [section['key'] for section in schema['sections']] == [
|
||||
@@ -238,7 +238,7 @@ async def test_settings_api_endpoints(test_client):
|
||||
assert llm_section['fields'][2]['value_type'] == 'integer'
|
||||
assert llm_section['fields'][3]['value_type'] == 'object'
|
||||
assert verification_section['label'] == 'Verification'
|
||||
vals = response_data['sdk_settings_values']
|
||||
vals = response_data['agent_settings']
|
||||
assert vals['llm.model'] == 'test-model'
|
||||
assert vals['llm.timeout'] == 123
|
||||
assert vals['llm.litellm_extra_body'] == {'metadata': {'tier': 'pro'}}
|
||||
@@ -249,7 +249,7 @@ async def test_settings_api_endpoints(test_client):
|
||||
assert vals['verification.max_refinement_iterations'] == 4
|
||||
assert vals['verification.confirmation_mode'] is True
|
||||
assert vals['verification.security_analyzer'] == 'llm'
|
||||
assert vals['llm.api_key'] is None
|
||||
assert vals['llm.api_key'] == '<hidden>'
|
||||
|
||||
# Test updating with partial settings
|
||||
partial_settings = {
|
||||
@@ -263,7 +263,7 @@ async def test_settings_api_endpoints(test_client):
|
||||
|
||||
response = test_client.get('/api/settings')
|
||||
assert response.status_code == 200
|
||||
assert response.json()['sdk_settings_values']['llm.timeout'] == 123
|
||||
assert response.json()['agent_settings']['llm.timeout'] == 123
|
||||
|
||||
# Test the unset-provider-tokens endpoint
|
||||
response = test_client.post('/api/unset-provider-tokens')
|
||||
@@ -278,7 +278,7 @@ async def test_saving_settings_with_frozen_secrets_store(test_client):
|
||||
"""
|
||||
settings_data = {
|
||||
'language': 'en',
|
||||
'llm_model': 'gpt-4',
|
||||
'llm.model': 'gpt-4',
|
||||
'secrets_store': {'provider_tokens': {}},
|
||||
}
|
||||
response = test_client.post('/api/settings', json=settings_data)
|
||||
@@ -288,10 +288,10 @@ async def test_saving_settings_with_frozen_secrets_store(test_client):
|
||||
@pytest.mark.asyncio
|
||||
async def test_search_api_key_preservation(test_client):
|
||||
"""Test that search_api_key is preserved when sending an empty string."""
|
||||
# 1. Set initial settings with a search API key
|
||||
# 1. Set initial settings with a search API key (use SDK dotted keys)
|
||||
initial_settings = {
|
||||
'search_api_key': 'initial-secret-key',
|
||||
'llm_model': 'gpt-4',
|
||||
'llm.model': 'gpt-4',
|
||||
}
|
||||
response = test_client.post('/api/settings', json=initial_settings)
|
||||
assert response.status_code == 200
|
||||
@@ -302,10 +302,10 @@ async def test_search_api_key_preservation(test_client):
|
||||
assert response.json()['search_api_key_set'] is True
|
||||
|
||||
# 2. Update settings with EMPTY search API key (simulating the frontend bug)
|
||||
# and changing another field (llm_model)
|
||||
# and changing another field via SDK key
|
||||
update_settings = {
|
||||
'search_api_key': '', # The frontend sends an empty string here
|
||||
'llm_model': 'claude-3-opus',
|
||||
'search_api_key': '',
|
||||
'llm.model': 'claude-3-opus',
|
||||
}
|
||||
response = test_client.post('/api/settings', json=update_settings)
|
||||
assert response.status_code == 200
|
||||
@@ -313,7 +313,6 @@ async def test_search_api_key_preservation(test_client):
|
||||
# 3. Verify the key was NOT wiped out (The Critical Check)
|
||||
response = test_client.get('/api/settings')
|
||||
assert response.status_code == 200
|
||||
# If the bug was present, this would be False
|
||||
assert response.json()['search_api_key_set'] is True
|
||||
# Verify the other field updated correctly
|
||||
assert response.json()['llm_model'] == 'claude-3-opus'
|
||||
# Verify the SDK value updated
|
||||
assert response.json()['agent_settings']['llm.model'] == 'claude-3-opus'
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import os
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
@@ -6,7 +7,6 @@ from fastapi import FastAPI
|
||||
from fastapi.testclient import TestClient
|
||||
from pydantic import SecretStr
|
||||
|
||||
from openhands.core.config.mcp_config import MCPConfig, MCPStdioServerConfig
|
||||
from openhands.integrations.provider import ProviderToken
|
||||
from openhands.integrations.service_types import ProviderType
|
||||
from openhands.server.routes.secrets import (
|
||||
@@ -15,13 +15,46 @@ from openhands.server.routes.secrets import (
|
||||
from openhands.server.routes.secrets import (
|
||||
check_provider_tokens,
|
||||
)
|
||||
from openhands.server.routes.settings import store_llm_settings
|
||||
from openhands.server.routes.settings import _apply_settings_payload
|
||||
from openhands.server.settings import POSTProviderModel
|
||||
from openhands.storage import get_file_store
|
||||
from openhands.storage.data_models.secrets import Secrets
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
from openhands.storage.secrets.file_secrets_store import FileSecretsStore
|
||||
|
||||
# Minimal SDK schema fixture for tests.
|
||||
_TEST_SDK_SCHEMA = {
|
||||
'sections': [
|
||||
{
|
||||
'key': 'llm',
|
||||
'fields': [
|
||||
{'key': 'llm.model', 'secret': False},
|
||||
{'key': 'llm.api_key', 'secret': True},
|
||||
{'key': 'llm.base_url', 'secret': False},
|
||||
],
|
||||
},
|
||||
{
|
||||
'key': 'condenser',
|
||||
'fields': [
|
||||
{'key': 'condenser.enabled', 'secret': False},
|
||||
{'key': 'condenser.max_size', 'secret': False},
|
||||
],
|
||||
},
|
||||
{
|
||||
'key': 'verification',
|
||||
'fields': [
|
||||
{'key': 'verification.confirmation_mode', 'secret': False},
|
||||
{'key': 'verification.security_analyzer', 'secret': False},
|
||||
],
|
||||
},
|
||||
]
|
||||
}
|
||||
|
||||
|
||||
def _make_settings(**sdk_vals: Any) -> Settings:
|
||||
"""Helper to create Settings with agent_settings."""
|
||||
return Settings(agent_settings=sdk_vals)
|
||||
|
||||
|
||||
# Mock functions to simulate the actual functions in settings.py
|
||||
async def get_settings_store(request):
|
||||
@@ -113,16 +146,10 @@ async def test_check_provider_tokens_invalid():
|
||||
@pytest.mark.asyncio
|
||||
async def test_check_provider_tokens_wrong_type():
|
||||
"""Test check_provider_tokens with unsupported provider type."""
|
||||
# We can't test with an unsupported provider type directly since the model enforces valid types
|
||||
# Instead, we'll test with an empty provider_tokens dictionary
|
||||
providers = POSTProviderModel(provider_tokens={})
|
||||
|
||||
# Empty existing provider tokens
|
||||
existing_provider_tokens = {}
|
||||
|
||||
result = await check_provider_tokens(providers, existing_provider_tokens)
|
||||
|
||||
# Should return empty string for no providers
|
||||
assert result == ''
|
||||
|
||||
|
||||
@@ -130,231 +157,161 @@ async def test_check_provider_tokens_wrong_type():
|
||||
async def test_check_provider_tokens_no_tokens():
|
||||
"""Test check_provider_tokens with no tokens."""
|
||||
providers = POSTProviderModel(provider_tokens={})
|
||||
|
||||
# Empty existing provider tokens
|
||||
existing_provider_tokens = {}
|
||||
|
||||
result = await check_provider_tokens(providers, existing_provider_tokens)
|
||||
|
||||
# Should return empty string when no tokens provided
|
||||
assert result == ''
|
||||
|
||||
|
||||
# Tests for store_llm_settings
|
||||
@pytest.mark.asyncio
|
||||
async def test_store_llm_settings_new_settings():
|
||||
"""Test store_llm_settings with new settings."""
|
||||
settings = Settings(
|
||||
llm_model='gpt-4',
|
||||
llm_api_key='test-api-key',
|
||||
llm_base_url='https://api.example.com',
|
||||
)
|
||||
# Tests for _apply_settings_payload (SDK-first settings)
|
||||
def test_apply_payload_sdk_keys_stored_and_readable():
|
||||
"""SDK dotted keys should be stored in agent_settings and readable via properties."""
|
||||
payload = {
|
||||
'llm.model': 'gpt-4',
|
||||
'llm.api_key': 'test-api-key',
|
||||
'llm.base_url': 'https://api.example.com',
|
||||
}
|
||||
|
||||
# No existing settings
|
||||
existing_settings = None
|
||||
result = _apply_settings_payload(payload, None, _TEST_SDK_SCHEMA)
|
||||
|
||||
result = await store_llm_settings(settings, existing_settings)
|
||||
|
||||
# Should return settings with the provided values
|
||||
assert result.agent_settings['llm.model'] == 'gpt-4'
|
||||
assert result.agent_settings['llm.api_key'] == 'test-api-key'
|
||||
assert result.agent_settings['llm.base_url'] == 'https://api.example.com'
|
||||
# Properties read from agent_settings
|
||||
assert result.llm_model == 'gpt-4'
|
||||
assert result.llm_api_key.get_secret_value() == 'test-api-key'
|
||||
assert result.llm_base_url == 'https://api.example.com'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_store_llm_settings_update_existing():
|
||||
"""Test store_llm_settings updates existing settings."""
|
||||
settings = Settings(
|
||||
llm_model='gpt-4',
|
||||
llm_api_key='new-api-key',
|
||||
llm_base_url='https://new.example.com',
|
||||
def test_apply_payload_updates_existing():
|
||||
"""SDK keys should update existing settings."""
|
||||
existing = _make_settings(
|
||||
**{
|
||||
'llm.model': 'gpt-3.5',
|
||||
'llm.api_key': 'old-api-key',
|
||||
'llm.base_url': 'https://old.example.com',
|
||||
}
|
||||
)
|
||||
|
||||
# Create existing settings
|
||||
existing_settings = Settings(
|
||||
llm_model='gpt-3.5',
|
||||
llm_api_key=SecretStr('old-api-key'),
|
||||
llm_base_url='https://old.example.com',
|
||||
)
|
||||
payload = {
|
||||
'llm.model': 'gpt-4',
|
||||
'llm.api_key': 'new-api-key',
|
||||
'llm.base_url': 'https://new.example.com',
|
||||
}
|
||||
|
||||
result = await store_llm_settings(settings, existing_settings)
|
||||
result = _apply_settings_payload(payload, existing, _TEST_SDK_SCHEMA)
|
||||
|
||||
# Should return settings with the updated values
|
||||
assert result.llm_model == 'gpt-4'
|
||||
assert result.llm_api_key.get_secret_value() == 'new-api-key'
|
||||
assert result.llm_base_url == 'https://new.example.com'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_store_llm_settings_partial_update():
|
||||
"""Test store_llm_settings with partial update.
|
||||
|
||||
Note: When llm_base_url is not provided in the update and the model is NOT an
|
||||
openhands model, we attempt to get the URL from litellm.get_api_base().
|
||||
For OpenAI models, this returns https://api.openai.com.
|
||||
"""
|
||||
settings = Settings(
|
||||
llm_model='gpt-4', # Only updating model (not an openhands model)
|
||||
llm_base_url='', # Explicitly cleared (e.g. basic mode save)
|
||||
def test_apply_payload_preserves_secrets_when_not_provided():
|
||||
"""When the API key is not in the payload, the existing value is preserved."""
|
||||
existing = _make_settings(
|
||||
**{
|
||||
'llm.model': 'gpt-3.5',
|
||||
'llm.api_key': 'existing-api-key',
|
||||
}
|
||||
)
|
||||
|
||||
# Create existing settings
|
||||
existing_settings = Settings(
|
||||
llm_model='gpt-3.5',
|
||||
llm_api_key=SecretStr('existing-api-key'),
|
||||
llm_base_url='https://existing.example.com',
|
||||
)
|
||||
payload = {'llm.model': 'gpt-4'}
|
||||
|
||||
result = await store_llm_settings(settings, existing_settings)
|
||||
result = _apply_settings_payload(payload, existing, _TEST_SDK_SCHEMA)
|
||||
|
||||
# Should return settings with updated model but keep API key
|
||||
assert result.llm_model == 'gpt-4'
|
||||
# For SecretStr objects, we need to compare the secret value
|
||||
assert result.llm_api_key.get_secret_value() == 'existing-api-key'
|
||||
# llm_base_url was explicitly cleared (""), so auto-detection runs
|
||||
# OpenAI models: litellm.get_api_base() returns https://api.openai.com
|
||||
assert result.llm_base_url == 'https://api.openai.com'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_store_llm_settings_mcp_update_preserves_base_url():
|
||||
"""Test that saving MCP config (without LLM fields) preserves existing base URL.
|
||||
def test_apply_payload_preserves_secrets_when_null():
|
||||
"""Null/empty secret values in the payload should not overwrite existing secrets."""
|
||||
existing = _make_settings(**{'llm.api_key': 'existing-api-key'})
|
||||
|
||||
Regression test: When adding an MCP server, the frontend sends only mcp_config
|
||||
and v1_enabled. This should not wipe out the existing llm_base_url.
|
||||
"""
|
||||
# Simulate what the MCP add/update/delete mutations send: mcp_config but no LLM fields
|
||||
settings = Settings(
|
||||
mcp_config=MCPConfig(
|
||||
stdio_servers=[
|
||||
MCPStdioServerConfig(
|
||||
name='my-server',
|
||||
command='npx',
|
||||
args=['-y', '@my/mcp-server'],
|
||||
env={'API_TOKEN': 'secret123', 'ENDPOINT': 'https://example.com'},
|
||||
)
|
||||
payload = {'llm.api_key': None}
|
||||
result = _apply_settings_payload(payload, existing, _TEST_SDK_SCHEMA)
|
||||
assert result.agent_settings['llm.api_key'] == 'existing-api-key'
|
||||
|
||||
payload = {'llm.api_key': ''}
|
||||
result = _apply_settings_payload(payload, existing, _TEST_SDK_SCHEMA)
|
||||
assert result.agent_settings['llm.api_key'] == 'existing-api-key'
|
||||
|
||||
|
||||
def test_apply_payload_mcp_preserves_llm_settings():
|
||||
"""Non-LLM payloads (e.g. MCP config) should not affect existing LLM settings."""
|
||||
existing = _make_settings(
|
||||
**{
|
||||
'llm.model': 'anthropic/claude-sonnet-4-5-20250929',
|
||||
'llm.api_key': 'existing-api-key',
|
||||
'llm.base_url': 'https://my-custom-proxy.example.com',
|
||||
}
|
||||
)
|
||||
|
||||
payload = {
|
||||
'mcp_config': {
|
||||
'stdio_servers': [
|
||||
{
|
||||
'name': 'my-server',
|
||||
'command': 'npx',
|
||||
'args': ['-y', '@my/mcp-server'],
|
||||
}
|
||||
],
|
||||
),
|
||||
)
|
||||
},
|
||||
}
|
||||
|
||||
# Create existing settings with a custom base URL
|
||||
existing_settings = Settings(
|
||||
llm_model='anthropic/claude-sonnet-4-5-20250929',
|
||||
llm_api_key=SecretStr('existing-api-key'),
|
||||
llm_base_url='https://my-custom-proxy.example.com',
|
||||
)
|
||||
result = _apply_settings_payload(payload, existing, _TEST_SDK_SCHEMA)
|
||||
|
||||
result = await store_llm_settings(settings, existing_settings)
|
||||
|
||||
# All existing LLM settings should be preserved
|
||||
assert result.llm_model == 'anthropic/claude-sonnet-4-5-20250929'
|
||||
assert result.llm_api_key.get_secret_value() == 'existing-api-key'
|
||||
assert result.llm_base_url == 'https://my-custom-proxy.example.com'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_store_llm_settings_no_existing_base_url_uses_auto_detection():
|
||||
"""Test auto-detection kicks in only when there is no existing base URL.
|
||||
def test_apply_payload_non_sdk_flat_keys_applied():
|
||||
"""Non-SDK flat keys (language, git, etc.) should still be applied normally."""
|
||||
payload = {
|
||||
'language': 'ja',
|
||||
'git_user_name': 'test-user',
|
||||
}
|
||||
|
||||
When neither the incoming settings nor existing settings have a base URL,
|
||||
auto-detection from litellm should be used.
|
||||
"""
|
||||
settings = Settings(
|
||||
llm_model='gpt-4' # Not an openhands model
|
||||
result = _apply_settings_payload(payload, None, _TEST_SDK_SCHEMA)
|
||||
|
||||
assert result.language == 'ja'
|
||||
assert result.git_user_name == 'test-user'
|
||||
|
||||
|
||||
def test_apply_payload_verification_stored_and_readable():
|
||||
"""Verification SDK keys are stored and readable via properties."""
|
||||
payload = {
|
||||
'verification.confirmation_mode': True,
|
||||
'verification.security_analyzer': 'llm',
|
||||
}
|
||||
|
||||
result = _apply_settings_payload(payload, None, _TEST_SDK_SCHEMA)
|
||||
|
||||
assert result.confirmation_mode is True
|
||||
assert result.security_analyzer == 'llm'
|
||||
assert result.agent_settings['verification.confirmation_mode'] is True
|
||||
|
||||
|
||||
def test_legacy_flat_fields_migrate_to_agent_vals():
|
||||
"""Loading a Settings with legacy flat fields should migrate to agent_settings."""
|
||||
s = Settings(
|
||||
**{
|
||||
'llm_model': 'gpt-4',
|
||||
'llm_api_key': 'my-key',
|
||||
'llm_base_url': 'https://example.com',
|
||||
'agent': 'CodeActAgent',
|
||||
'confirmation_mode': True,
|
||||
}
|
||||
)
|
||||
|
||||
# Existing settings without a base URL
|
||||
existing_settings = Settings(
|
||||
llm_model='gpt-3.5',
|
||||
llm_api_key=SecretStr('existing-api-key'),
|
||||
)
|
||||
|
||||
result = await store_llm_settings(settings, existing_settings)
|
||||
|
||||
assert result.llm_model == 'gpt-4'
|
||||
assert result.llm_api_key.get_secret_value() == 'existing-api-key'
|
||||
# No existing base URL, so auto-detection should set it
|
||||
assert result.llm_base_url == 'https://api.openai.com'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_store_llm_settings_anthropic_model_gets_api_base():
|
||||
"""Test store_llm_settings with an Anthropic model.
|
||||
|
||||
For Anthropic models, get_provider_api_base() returns the Anthropic API base URL
|
||||
via ProviderConfigManager.get_provider_model_info().
|
||||
"""
|
||||
settings = Settings(
|
||||
llm_model='anthropic/claude-sonnet-4-5-20250929' # Anthropic model
|
||||
)
|
||||
|
||||
existing_settings = Settings(
|
||||
llm_model='gpt-3.5',
|
||||
llm_api_key=SecretStr('existing-api-key'),
|
||||
)
|
||||
|
||||
result = await store_llm_settings(settings, existing_settings)
|
||||
|
||||
assert result.llm_model == 'anthropic/claude-sonnet-4-5-20250929'
|
||||
assert result.llm_api_key.get_secret_value() == 'existing-api-key'
|
||||
# Anthropic models get https://api.anthropic.com via ProviderConfigManager
|
||||
assert result.llm_base_url == 'https://api.anthropic.com'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_store_llm_settings_litellm_error_logged():
|
||||
"""Test that litellm errors are logged when getting api_base fails."""
|
||||
from unittest.mock import patch
|
||||
|
||||
settings = Settings(
|
||||
llm_model='unknown-model-xyz' # A model that litellm won't recognize
|
||||
)
|
||||
|
||||
existing_settings = Settings(
|
||||
llm_model='gpt-3.5',
|
||||
llm_api_key=SecretStr('existing-api-key'),
|
||||
)
|
||||
|
||||
# The function should not raise even if litellm fails
|
||||
with patch('openhands.server.routes.settings.logger') as mock_logger:
|
||||
result = await store_llm_settings(settings, existing_settings)
|
||||
|
||||
# llm_base_url should remain None since litellm couldn't find the model
|
||||
assert result.llm_base_url is None
|
||||
# Either error or debug should have been logged
|
||||
assert mock_logger.error.called or mock_logger.debug.called
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_store_llm_settings_openhands_model_gets_default_url():
|
||||
"""Test store_llm_settings with openhands model gets LiteLLM proxy URL.
|
||||
|
||||
When llm_base_url is not provided and the model is an openhands model,
|
||||
it gets set to the default LiteLLM proxy URL.
|
||||
"""
|
||||
import os
|
||||
|
||||
settings = Settings(
|
||||
llm_model='openhands/claude-sonnet-4-5-20250929' # openhands model
|
||||
)
|
||||
|
||||
# Create existing settings
|
||||
existing_settings = Settings(
|
||||
llm_model='gpt-3.5',
|
||||
llm_api_key=SecretStr('existing-api-key'),
|
||||
)
|
||||
|
||||
result = await store_llm_settings(settings, existing_settings)
|
||||
|
||||
# Should return settings with updated model
|
||||
assert result.llm_model == 'openhands/claude-sonnet-4-5-20250929'
|
||||
# For SecretStr objects, we need to compare the secret value
|
||||
assert result.llm_api_key.get_secret_value() == 'existing-api-key'
|
||||
# openhands models get the LiteLLM proxy URL
|
||||
expected_base_url = os.environ.get(
|
||||
'LITE_LLM_API_URL', 'https://llm-proxy.app.all-hands.dev'
|
||||
)
|
||||
assert result.llm_base_url == expected_base_url
|
||||
assert s.agent_settings['llm.model'] == 'gpt-4'
|
||||
assert s.agent_settings['llm.api_key'] == 'my-key'
|
||||
assert s.agent_settings['llm.base_url'] == 'https://example.com'
|
||||
assert s.agent_settings['agent'] == 'CodeActAgent'
|
||||
assert s.agent_settings['verification.confirmation_mode'] is True
|
||||
# Properties work
|
||||
assert s.llm_model == 'gpt-4'
|
||||
assert s.agent == 'CodeActAgent'
|
||||
|
||||
|
||||
# Tests for store_provider_tokens
|
||||
|
||||
@@ -90,10 +90,10 @@ def test_settings_handles_sensitive_data():
|
||||
assert settings.llm_api_key.get_secret_value() == 'test-key'
|
||||
|
||||
|
||||
def test_settings_preserve_sdk_settings_values():
|
||||
def test_settings_preserve_agent_settings():
|
||||
settings = Settings(
|
||||
llm_api_key='test-key',
|
||||
sdk_settings_values={
|
||||
agent_settings={
|
||||
'llm.api_key': 'test-key',
|
||||
'verification.critic_enabled': True,
|
||||
'verification.critic_mode': 'all_actions',
|
||||
'llm.litellm_extra_body': {'metadata': {'tier': 'pro'}},
|
||||
@@ -101,16 +101,17 @@ def test_settings_preserve_sdk_settings_values():
|
||||
)
|
||||
|
||||
assert settings.llm_api_key.get_secret_value() == 'test-key'
|
||||
assert settings.sdk_settings_values == {
|
||||
assert settings.agent_settings == {
|
||||
'llm.api_key': 'test-key',
|
||||
'verification.critic_enabled': True,
|
||||
'verification.critic_mode': 'all_actions',
|
||||
'llm.litellm_extra_body': {'metadata': {'tier': 'pro'}},
|
||||
}
|
||||
|
||||
|
||||
def test_settings_to_agent_settings_uses_sdk_values():
|
||||
def test_settings_to_agent_settings_uses_agent_vals():
|
||||
settings = Settings(
|
||||
sdk_settings_values={
|
||||
agent_settings={
|
||||
'llm.model': 'sdk-model',
|
||||
'llm.base_url': 'https://sdk.example.com',
|
||||
'llm.litellm_extra_body': {'metadata': {'tier': 'enterprise'}},
|
||||
|
||||
Reference in New Issue
Block a user