mirror of
https://github.com/All-Hands-AI/OpenHands.git
synced 2026-04-29 03:00:45 -04:00
Compare commits
13 Commits
ci/pr-amd6
...
refactor/c
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
f0499a26fb | ||
|
|
8ee823b624 | ||
|
|
2fce463a64 | ||
|
|
fcdf01ef21 | ||
|
|
f424211dcc | ||
|
|
c46d2054f2 | ||
|
|
fc0ab3b3d4 | ||
|
|
46545ef9ad | ||
|
|
5dd6f80d48 | ||
|
|
948407f5ef | ||
|
|
286348a901 | ||
|
|
0823bc17be | ||
|
|
a6cb9c6fea |
@@ -2,17 +2,28 @@
|
||||
|
||||
## Overview
|
||||
|
||||
OpenHands uses a flexible configuration system that allows settings to be defined through environment variables, TOML files, and command-line arguments. The configuration is managed through a package structure in `openhands/core/config/`.
|
||||
OpenHands uses a flexible configuration system that allows settings to be defined through environment variables, TOML files, command-line arguments, and user settings. The configuration is managed through a package structure in `openhands/core/config/`.
|
||||
|
||||
## Configuration Sources and Precedence
|
||||
|
||||
OpenHands configuration comes from multiple sources, with the following precedence (highest to lowest):
|
||||
|
||||
1. **Command-line Arguments**: Settings provided as command-line arguments
|
||||
2. **User Settings**: Settings configured by users through the UI or API
|
||||
3. **Environment Variables**: Settings defined in the environment
|
||||
4. **TOML Files**: Settings defined in `config.toml`
|
||||
5. **Default Values**: Default values defined in the configuration classes
|
||||
|
||||
## Configuration Classes
|
||||
|
||||
The main configuration classes are:
|
||||
|
||||
- `AppConfig`: The root configuration class
|
||||
- `OpenHandsConfig`: The root configuration class (formerly AppConfig)
|
||||
- `LLMConfig`: Configuration for the Language Model
|
||||
- `AgentConfig`: Configuration for the agent
|
||||
- `SandboxConfig`: Configuration for the sandbox environment
|
||||
- `SecurityConfig`: Configuration for security settings
|
||||
- `MCPConfig`: Configuration for Model Context Protocol
|
||||
|
||||
These classes are defined as dataclasses, with class attributes holding default values for all fields.
|
||||
|
||||
@@ -55,23 +66,24 @@ Be cautious when setting sensitive information like API keys in environment vari
|
||||
|
||||
## Usage
|
||||
|
||||
The `load_app_config()` function is the recommended way to initialize your configuration. It performs the following steps:
|
||||
The `load_openhands_config()` function is the recommended way to initialize your configuration. It performs the following steps:
|
||||
|
||||
1. Creates an instance of `AppConfig`
|
||||
1. Creates an instance of `OpenHandsConfig` with default values
|
||||
2. Loads settings from the `config.toml` file (if present)
|
||||
3. Loads settings from environment variables, overriding TOML settings if applicable
|
||||
4. Applies final tweaks and validations to the configuration, falling back to the default values specified in the code
|
||||
5. Optionally sets global logging levels based on the configuration
|
||||
3. Loads settings from environment variables, overriding TOML settings
|
||||
4. Processes command-line arguments, which have the highest precedence and override all other sources
|
||||
5. Applies final tweaks and validations to the configuration
|
||||
6. Optionally sets global logging levels based on the configuration
|
||||
|
||||
There are also command line args, which may work to override other sources.
|
||||
When used in the server context, user settings (from the UI/API) are applied after loading the configuration, taking precedence over environment variables and TOML settings but not over command-line arguments.
|
||||
|
||||
Here's an example of how to use `load_app_config()`:
|
||||
Here's an example of how to use `load_openhands_config()`:
|
||||
|
||||
````python
|
||||
from openhands.core.config import load_app_config
|
||||
```python
|
||||
from openhands.core.config import load_openhands_config
|
||||
|
||||
# Load all configuration settings
|
||||
config = load_app_config()
|
||||
config = load_openhands_config()
|
||||
|
||||
# Now you can access your configuration
|
||||
llm_config = config.get_llm_config()
|
||||
@@ -82,9 +94,39 @@ sandbox_config = config.sandbox
|
||||
print(f"Using LLM model: {llm_config.model}")
|
||||
print(f"Agent memory enabled: {agent_config.memory_enabled}")
|
||||
print(f"Sandbox timeout: {sandbox_config.timeout}")
|
||||
````
|
||||
```
|
||||
|
||||
By using `load_app_config()`, you ensure that all configuration sources are properly loaded and processed, providing a consistent and fully initialized configuration for your application.
|
||||
By using `load_openhands_config()`, you ensure that all configuration sources are properly loaded and processed, providing a consistent and fully initialized configuration for your application.
|
||||
|
||||
## Merging User Settings with Configuration
|
||||
|
||||
OpenHands provides a `ConfigurationMerger` utility for merging user settings with the application configuration:
|
||||
|
||||
```python
|
||||
from openhands.core.config import ConfigurationMerger, OpenHandsConfig
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
|
||||
# Load the application configuration
|
||||
config = OpenHandsConfig()
|
||||
|
||||
# Create or load user settings
|
||||
settings = Settings(llm_model="gpt-4", max_iterations=100)
|
||||
|
||||
# Merge settings with configuration (settings take precedence)
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(settings, config)
|
||||
|
||||
# You can also convert configuration to settings
|
||||
settings = ConfigurationMerger.config_to_settings(config)
|
||||
```
|
||||
|
||||
### Special Handling for MCP Configuration
|
||||
|
||||
MCP (Multi-Channel Provider) configuration is special because it represents a list of providers that can come from multiple sources. When merging MCP configuration:
|
||||
|
||||
1. Providers from `config.toml` appear first in the merged list
|
||||
2. Providers from user settings are appended to the list
|
||||
|
||||
This allows both system-wide providers (from `config.toml`) and user-specific providers (from settings) to be used together.
|
||||
|
||||
## Additional Configuration Methods
|
||||
|
||||
@@ -97,4 +139,6 @@ These methods are handled by separate functions in the config package.
|
||||
|
||||
## Conclusion
|
||||
|
||||
The OpenHands configuration system provides a flexible and type-safe way to manage application settings. By following the naming conventions and utilizing the provided functions, developers can easily customize the behavior of OpenHands components through environment variables and other configuration sources.
|
||||
The OpenHands configuration system provides a flexible and type-safe way to manage application settings from multiple sources. By following the naming conventions and utilizing the provided functions, developers can easily customize the behavior of OpenHands components through environment variables, configuration files, and user settings.
|
||||
|
||||
The `ConfigurationMerger` utility provides a clean way to merge user settings with application configuration, ensuring that settings from different sources are properly combined according to the defined precedence rules.
|
||||
|
||||
@@ -5,6 +5,7 @@ from openhands.core.config.config_utils import (
|
||||
OH_MAX_ITERATIONS,
|
||||
get_field_info,
|
||||
)
|
||||
from openhands.core.config.configuration_merger import ConfigurationMerger
|
||||
from openhands.core.config.extended_config import ExtendedConfig
|
||||
from openhands.core.config.llm_config import LLMConfig
|
||||
from openhands.core.config.mcp_config import MCPConfig
|
||||
@@ -34,6 +35,7 @@ __all__ = [
|
||||
'SandboxConfig',
|
||||
'SecurityConfig',
|
||||
'ExtendedConfig',
|
||||
'ConfigurationMerger',
|
||||
'load_openhands_config',
|
||||
'load_from_env',
|
||||
'load_from_toml',
|
||||
|
||||
256
openhands/core/config/configuration_merger.py
Normal file
256
openhands/core/config/configuration_merger.py
Normal file
@@ -0,0 +1,256 @@
|
||||
import logging
|
||||
from copy import deepcopy
|
||||
from typing import TYPE_CHECKING, cast
|
||||
|
||||
from pydantic import SecretStr
|
||||
|
||||
# Import MCPConfig directly to avoid circular imports
|
||||
from openhands.core.config.mcp_config import MCPConfig
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from openhands.core.config.openhands_config import OpenHandsConfig
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
|
||||
|
||||
class ConfigurationMerger:
|
||||
"""Utility class for merging Settings with OpenHandsConfig."""
|
||||
|
||||
# Mapping between Settings fields and OpenHandsConfig fields
|
||||
# Format: (settings_field, config_field, config_nested_object)
|
||||
FIELD_MAPPING = [
|
||||
# Core settings
|
||||
('agent', 'default_agent', None),
|
||||
('max_iterations', 'max_iterations', None),
|
||||
('max_budget_per_task', 'max_budget_per_task', None),
|
||||
('git_user_name', 'git_user_name', None),
|
||||
('git_user_email', 'git_user_email', None),
|
||||
('search_api_key', 'search_api_key', None),
|
||||
# LLM settings
|
||||
('llm_model', 'model', 'llm_config'),
|
||||
('llm_api_key', 'api_key', 'llm_config'),
|
||||
('llm_base_url', 'base_url', 'llm_config'),
|
||||
# Security settings
|
||||
('security_analyzer', 'security_analyzer', 'security'),
|
||||
('confirmation_mode', 'confirmation_mode', 'security'),
|
||||
# Sandbox settings
|
||||
('remote_runtime_resource_factor', 'remote_runtime_resource_factor', 'sandbox'),
|
||||
('sandbox_base_container_image', 'base_container_image', 'sandbox'),
|
||||
('sandbox_runtime_container_image', 'runtime_container_image', 'sandbox'),
|
||||
('sandbox_api_key', 'api_key', 'sandbox'),
|
||||
]
|
||||
|
||||
"""
|
||||
This class provides a centralized way to merge user settings with the
|
||||
application configuration, ensuring consistent precedence rules across
|
||||
all configuration types.
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
def merge_settings_with_config(
|
||||
settings: 'Settings', config: 'OpenHandsConfig'
|
||||
) -> 'OpenHandsConfig':
|
||||
"""Merge user settings with application configuration.
|
||||
|
||||
Creates a new config object with settings applied according to
|
||||
consistent precedence rules. Settings generally take precedence
|
||||
over config values, except for MCP settings which are merged.
|
||||
|
||||
Args:
|
||||
settings: User settings to apply
|
||||
config: Base configuration to merge with
|
||||
|
||||
Returns:
|
||||
A new OpenHandsConfig instance with settings applied
|
||||
"""
|
||||
# Create a deep copy to avoid modifying the original
|
||||
merged_config = deepcopy(config)
|
||||
|
||||
# Apply settings to config with clear precedence rules
|
||||
ConfigurationMerger._merge_settings(merged_config, settings)
|
||||
|
||||
return merged_config
|
||||
|
||||
@staticmethod
|
||||
def _merge_settings(config: 'OpenHandsConfig', settings: 'Settings') -> None:
|
||||
"""Apply settings to config with consistent precedence rules."""
|
||||
# Get LLM config once to avoid repeated calls
|
||||
llm_config = config.get_llm_config()
|
||||
|
||||
# Use the field mapping to apply settings to config
|
||||
for (
|
||||
settings_field,
|
||||
config_field,
|
||||
nested_obj,
|
||||
) in ConfigurationMerger.FIELD_MAPPING:
|
||||
# Get the value from settings
|
||||
settings_value = getattr(settings, settings_field, None)
|
||||
|
||||
# Skip if the settings value is None
|
||||
if settings_value is None:
|
||||
continue
|
||||
|
||||
# Apply the value to the appropriate config object
|
||||
if nested_obj is None:
|
||||
# Apply to the main config object
|
||||
logger.debug(
|
||||
f'Merging setting {settings_field} -> config.{config_field}'
|
||||
)
|
||||
setattr(config, config_field, settings_value)
|
||||
elif nested_obj == 'llm_config':
|
||||
# Apply to the LLM config
|
||||
logger.debug(
|
||||
f'Merging setting {settings_field} -> config.llm_config.{config_field}'
|
||||
)
|
||||
setattr(llm_config, config_field, settings_value)
|
||||
elif nested_obj == 'security':
|
||||
# Apply to the security config
|
||||
logger.debug(
|
||||
f'Merging setting {settings_field} -> config.security.{config_field}'
|
||||
)
|
||||
setattr(config.security, config_field, settings_value)
|
||||
elif nested_obj == 'sandbox':
|
||||
# Apply to the sandbox config
|
||||
logger.debug(
|
||||
f'Merging setting {settings_field} -> config.sandbox.{config_field}'
|
||||
)
|
||||
setattr(config.sandbox, config_field, settings_value)
|
||||
else:
|
||||
# Unsupported nested object
|
||||
raise ValueError(
|
||||
f"Unsupported nested_obj '{nested_obj}' for field mapping: "
|
||||
f'({settings_field}, {config_field}, {nested_obj})'
|
||||
)
|
||||
|
||||
# MCP settings need special handling for merging
|
||||
ConfigurationMerger._merge_mcp_settings(config, settings)
|
||||
|
||||
@staticmethod
|
||||
def _merge_mcp_settings(config: 'OpenHandsConfig', settings: 'Settings') -> None:
|
||||
"""Merge MCP-specific settings."""
|
||||
if settings.mcp_config is None:
|
||||
logger.debug('No MCP config in settings, skipping MCP merge')
|
||||
return
|
||||
|
||||
# Check if config has any MCP servers configured
|
||||
# Note: config.mcp should never be None in normal usage (it has a default_factory),
|
||||
# but tests may set it to None, so we handle both cases
|
||||
mcp_config = cast('MCPConfig | None', config.mcp)
|
||||
if mcp_config is None:
|
||||
config_has_servers = False
|
||||
else:
|
||||
config_has_servers = bool(
|
||||
mcp_config.sse_servers
|
||||
or mcp_config.stdio_servers
|
||||
or mcp_config.shttp_servers
|
||||
)
|
||||
|
||||
if not config_has_servers:
|
||||
logger.debug('No MCP servers in config, using settings MCP config directly')
|
||||
config.mcp = settings.mcp_config
|
||||
return
|
||||
|
||||
logger.debug('Merging MCP configs with config servers taking precedence')
|
||||
|
||||
# Merge MCP configs with config.toml taking priority (appearing first)
|
||||
merged_mcp = MCPConfig(
|
||||
sse_servers=list(config.mcp.sse_servers)
|
||||
+ list(settings.mcp_config.sse_servers),
|
||||
stdio_servers=list(config.mcp.stdio_servers)
|
||||
+ list(settings.mcp_config.stdio_servers),
|
||||
shttp_servers=list(config.mcp.shttp_servers)
|
||||
+ list(settings.mcp_config.shttp_servers),
|
||||
)
|
||||
|
||||
logger.debug(
|
||||
f'Merged MCP config has {len(merged_mcp.sse_servers)} SSE servers, '
|
||||
f'{len(merged_mcp.stdio_servers)} stdio servers, and '
|
||||
f'{len(merged_mcp.shttp_servers)} shttp servers'
|
||||
)
|
||||
|
||||
config.mcp = merged_mcp
|
||||
|
||||
@staticmethod
|
||||
def config_to_settings(config: 'OpenHandsConfig') -> 'Settings':
|
||||
"""Convert an OpenHandsConfig to a Settings object.
|
||||
|
||||
This method creates a new Settings object with values from the provided
|
||||
OpenHandsConfig. It's the inverse operation of merge_settings_with_config.
|
||||
|
||||
Args:
|
||||
config: The configuration to convert
|
||||
|
||||
Returns:
|
||||
A new Settings instance with values from the config
|
||||
"""
|
||||
# Import here to avoid circular imports
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
|
||||
# Create a new Settings object with default values
|
||||
settings = Settings(language='en')
|
||||
|
||||
# Get config objects that will be accessed frequently
|
||||
llm_config = config.get_llm_config()
|
||||
|
||||
# Use the field mapping to apply config values to settings
|
||||
for (
|
||||
settings_field,
|
||||
config_field,
|
||||
nested_obj,
|
||||
) in ConfigurationMerger.FIELD_MAPPING:
|
||||
# Get the value from the appropriate config object
|
||||
if nested_obj is None:
|
||||
config_value = getattr(config, config_field, None)
|
||||
logger.debug(
|
||||
f'Converting config.{config_field} -> settings.{settings_field}'
|
||||
)
|
||||
elif nested_obj == 'llm_config':
|
||||
config_value = getattr(llm_config, config_field, None)
|
||||
logger.debug(
|
||||
f'Converting config.llm_config.{config_field} -> settings.{settings_field}'
|
||||
)
|
||||
elif nested_obj == 'security':
|
||||
config_value = getattr(config.security, config_field, None)
|
||||
logger.debug(
|
||||
f'Converting config.security.{config_field} -> settings.{settings_field}'
|
||||
)
|
||||
elif nested_obj == 'sandbox':
|
||||
config_value = getattr(config.sandbox, config_field, None)
|
||||
logger.debug(
|
||||
f'Converting config.sandbox.{config_field} -> settings.{settings_field}'
|
||||
)
|
||||
else:
|
||||
# Unsupported nested object
|
||||
raise ValueError(
|
||||
f"Unsupported nested_obj '{nested_obj}' for field mapping: "
|
||||
f'({settings_field}, {config_field}, {nested_obj})'
|
||||
)
|
||||
|
||||
# Skip if the config value is None
|
||||
if config_value is None:
|
||||
continue
|
||||
|
||||
# Handle special cases for API keys (convert to SecretStr)
|
||||
if settings_field in ('llm_api_key', 'sandbox_api_key', 'search_api_key'):
|
||||
if isinstance(config_value, str):
|
||||
logger.debug(
|
||||
f'Converting string API key to SecretStr for {settings_field}'
|
||||
)
|
||||
setattr(settings, settings_field, SecretStr(config_value))
|
||||
elif hasattr(config_value, 'get_secret_value'):
|
||||
# It's already a SecretStr
|
||||
setattr(settings, settings_field, config_value)
|
||||
else:
|
||||
# Skip mock objects or other non-string types
|
||||
logger.debug(f'Skipping non-string API key for {settings_field}')
|
||||
else:
|
||||
# Apply the value directly
|
||||
setattr(settings, settings_field, config_value)
|
||||
|
||||
# Special handling for MCP config
|
||||
if hasattr(config, 'mcp') and config.mcp is not None:
|
||||
logger.debug('Adding MCP config to settings')
|
||||
settings.mcp_config = config.mcp
|
||||
|
||||
return settings
|
||||
@@ -175,24 +175,16 @@ def make_commit(
|
||||
git_user_name: Git username for commits
|
||||
git_user_email: Git email for commits
|
||||
"""
|
||||
# Check if git username is set
|
||||
result = subprocess.run(
|
||||
f'git -C {repo_dir} config user.name',
|
||||
# Always set git username and email for this repository
|
||||
# This ensures tests work in CI environments where global git config might not be set
|
||||
subprocess.run(
|
||||
f'git -C {repo_dir} config user.name "{git_user_name}" && '
|
||||
f'git -C {repo_dir} config user.email "{git_user_email}" && '
|
||||
f'git -C {repo_dir} config alias.git "git --no-pager"',
|
||||
shell=True,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=True,
|
||||
)
|
||||
|
||||
if not result.stdout.strip():
|
||||
# If username is not set, configure git with the provided credentials
|
||||
subprocess.run(
|
||||
f'git -C {repo_dir} config user.name "{git_user_name}" && '
|
||||
f'git -C {repo_dir} config user.email "{git_user_email}" && '
|
||||
f'git -C {repo_dir} config alias.git "git --no-pager"',
|
||||
shell=True,
|
||||
check=True,
|
||||
)
|
||||
logger.info(f'Git user configured as {git_user_name} <{git_user_email}>')
|
||||
logger.info(f'Git user configured as {git_user_name} <{git_user_email}>')
|
||||
|
||||
# Add all changes to the git index
|
||||
result = subprocess.run(
|
||||
@@ -220,9 +212,24 @@ def make_commit(
|
||||
# Prepare the commit message
|
||||
commit_message = f'Fix {issue_type} #{issue.number}: {issue.title}'
|
||||
|
||||
# Commit the changes
|
||||
# Commit the changes with explicit author and committer
|
||||
author = f'{git_user_name} <{git_user_email}>'
|
||||
# Use -c to set the committer identity for this command only
|
||||
result = subprocess.run(
|
||||
['git', '-C', repo_dir, 'commit', '-m', commit_message],
|
||||
[
|
||||
'git',
|
||||
'-C',
|
||||
repo_dir,
|
||||
'-c',
|
||||
f'user.name={git_user_name}',
|
||||
'-c',
|
||||
f'user.email={git_user_email}',
|
||||
'commit',
|
||||
'--author',
|
||||
author,
|
||||
'-m',
|
||||
commit_message,
|
||||
],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
)
|
||||
|
||||
@@ -13,6 +13,7 @@ from openhands.core.config.condenser_config import (
|
||||
ConversationWindowCondenserConfig,
|
||||
LLMSummarizingCondenserConfig,
|
||||
)
|
||||
from openhands.core.config.configuration_merger import ConfigurationMerger
|
||||
from openhands.core.config.mcp_config import OpenHandsMCPConfigImpl
|
||||
from openhands.core.exceptions import MicroagentValidationError
|
||||
from openhands.core.logger import OpenHandsLoggerAdapter
|
||||
@@ -103,52 +104,14 @@ class Session:
|
||||
AgentStateChangedObservation('', AgentState.LOADING),
|
||||
EventSource.ENVIRONMENT,
|
||||
)
|
||||
agent_cls = settings.agent or self.config.default_agent
|
||||
self.config.security.confirmation_mode = (
|
||||
self.config.security.confirmation_mode
|
||||
if settings.confirmation_mode is None
|
||||
else settings.confirmation_mode
|
||||
)
|
||||
self.config.security.security_analyzer = (
|
||||
settings.security_analyzer or self.config.security.security_analyzer
|
||||
)
|
||||
self.config.sandbox.base_container_image = (
|
||||
settings.sandbox_base_container_image
|
||||
or self.config.sandbox.base_container_image
|
||||
)
|
||||
self.config.sandbox.runtime_container_image = (
|
||||
settings.sandbox_runtime_container_image
|
||||
if settings.sandbox_base_container_image
|
||||
or settings.sandbox_runtime_container_image
|
||||
else self.config.sandbox.runtime_container_image
|
||||
|
||||
# Use the configuration merger to merge settings with config
|
||||
# This replaces all the individual settings assignments that were previously here
|
||||
self.config = ConfigurationMerger.merge_settings_with_config(
|
||||
settings, self.config
|
||||
)
|
||||
|
||||
# Set Git user configuration if provided in settings
|
||||
if hasattr(settings, 'git_user_name') and settings.git_user_name:
|
||||
self.config.git_user_name = settings.git_user_name
|
||||
if hasattr(settings, 'git_user_email') and settings.git_user_email:
|
||||
self.config.git_user_email = settings.git_user_email
|
||||
max_iterations = settings.max_iterations or self.config.max_iterations
|
||||
|
||||
# Prioritize settings over config for max_budget_per_task
|
||||
max_budget_per_task = (
|
||||
settings.max_budget_per_task
|
||||
if settings.max_budget_per_task is not None
|
||||
else self.config.max_budget_per_task
|
||||
)
|
||||
|
||||
# This is a shallow copy of the default LLM config, so changes here will
|
||||
# persist if we retrieve the default LLM config again when constructing
|
||||
# the agent
|
||||
default_llm_config = self.config.get_llm_config()
|
||||
default_llm_config.model = settings.llm_model or ''
|
||||
default_llm_config.api_key = settings.llm_api_key
|
||||
default_llm_config.base_url = settings.llm_base_url
|
||||
self.config.search_api_key = settings.search_api_key
|
||||
if settings.sandbox_api_key:
|
||||
self.config.sandbox.api_key = settings.sandbox_api_key.get_secret_value()
|
||||
|
||||
# NOTE: this need to happen AFTER the config is updated with the search_api_key
|
||||
# NOTE: this needs to happen AFTER the config is updated with the search_api_key
|
||||
self.logger.debug(
|
||||
f'MCP configuration before setup - self.config.mcp_config: {self.config.mcp}'
|
||||
)
|
||||
@@ -169,9 +132,13 @@ class Session:
|
||||
f'MCP configuration after setup - self.config.mcp: {self.config.mcp}'
|
||||
)
|
||||
|
||||
# TODO: override other LLM config & agent config groups (#2075)
|
||||
# Get agent class from merged config
|
||||
agent_cls = self.config.default_agent
|
||||
|
||||
# Create LLM with the merged config
|
||||
llm = self._create_llm(agent_cls)
|
||||
|
||||
# Get agent config from the merged config
|
||||
agent_config = self.config.get_agent_config(agent_cls)
|
||||
|
||||
if settings.enable_default_condenser:
|
||||
@@ -202,8 +169,11 @@ class Session:
|
||||
f' keep_first=4, max_size=80)'
|
||||
)
|
||||
agent_config.condenser = default_condenser_config
|
||||
|
||||
# Create the agent with the merged config
|
||||
agent = Agent.get_cls(agent_cls)(llm, agent_config)
|
||||
|
||||
# Extract additional settings for conversation initialization
|
||||
git_provider_tokens = None
|
||||
selected_repository = None
|
||||
selected_branch = None
|
||||
@@ -221,8 +191,8 @@ class Session:
|
||||
runtime_name=self.config.runtime,
|
||||
config=self.config,
|
||||
agent=agent,
|
||||
max_iterations=max_iterations,
|
||||
max_budget_per_task=max_budget_per_task,
|
||||
max_iterations=self.config.max_iterations,
|
||||
max_budget_per_task=self.config.max_budget_per_task,
|
||||
agent_to_llm_config=self.config.get_agent_to_llm_config_map(),
|
||||
agent_configs=self.config.get_agent_configs(),
|
||||
git_provider_tokens=git_provider_tokens,
|
||||
|
||||
@@ -3,6 +3,7 @@ from dataclasses import dataclass
|
||||
from fastapi import Request
|
||||
from pydantic import SecretStr
|
||||
|
||||
from openhands.core.config import ConfigurationMerger, OpenHandsConfig
|
||||
from openhands.integrations.provider import PROVIDER_TOKEN_TYPE
|
||||
from openhands.server import shared
|
||||
from openhands.server.settings import Settings
|
||||
@@ -47,18 +48,48 @@ class DefaultUserAuth(UserAuth):
|
||||
return settings_store
|
||||
|
||||
async def get_user_settings(self) -> Settings | None:
|
||||
"""Get user settings with proper precedence handling.
|
||||
|
||||
This method retrieves user settings from the settings store and applies the correct
|
||||
precedence order using the ConfigurationMerger:
|
||||
1. Command-line arguments (already in OpenHandsConfig)
|
||||
2. User settings (from settings store)
|
||||
3. Environment variables (already in OpenHandsConfig)
|
||||
4. TOML files (already in OpenHandsConfig)
|
||||
5. Default values (already in OpenHandsConfig)
|
||||
|
||||
For MCP configuration, we merge the lists from both sources.
|
||||
|
||||
Returns:
|
||||
The merged settings, or None if no settings exist.
|
||||
"""
|
||||
# Return cached settings if available
|
||||
settings = self._settings
|
||||
if settings:
|
||||
return settings
|
||||
|
||||
# Load settings from store
|
||||
settings_store = await self.get_user_settings_store()
|
||||
settings = await settings_store.load()
|
||||
|
||||
# Merge config.toml settings with stored settings
|
||||
if settings:
|
||||
settings = settings.merge_with_config_settings()
|
||||
# If no settings exist, return None
|
||||
if not settings:
|
||||
self._settings = None
|
||||
return None
|
||||
|
||||
self._settings = settings
|
||||
return settings
|
||||
# Load config (contains command-line args, env vars, and TOML settings)
|
||||
config = OpenHandsConfig()
|
||||
|
||||
# Use ConfigurationMerger to properly merge settings with config
|
||||
# This ensures correct precedence: config values (CLI, env, TOML) override user settings
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(settings, config)
|
||||
|
||||
# Convert the merged config back to settings format
|
||||
final_settings = ConfigurationMerger.config_to_settings(merged_config)
|
||||
|
||||
# Cache and return settings
|
||||
self._settings = final_settings
|
||||
return final_settings
|
||||
|
||||
async def get_secrets_store(self) -> SecretsStore:
|
||||
secrets_store = self._secrets_store
|
||||
|
||||
@@ -11,9 +11,7 @@ from pydantic import (
|
||||
)
|
||||
from pydantic.json import pydantic_encoder
|
||||
|
||||
from openhands.core.config.llm_config import LLMConfig
|
||||
from openhands.core.config.mcp_config import MCPConfig
|
||||
from openhands.core.config.utils import load_openhands_config
|
||||
from openhands.storage.data_models.user_secrets import UserSecrets
|
||||
|
||||
|
||||
@@ -109,63 +107,3 @@ class Settings(BaseModel):
|
||||
|
||||
"""Force invalidate secret store"""
|
||||
return {'provider_tokens': {}}
|
||||
|
||||
@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(
|
||||
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,
|
||||
)
|
||||
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
|
||||
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),
|
||||
stdio_servers=list(config_settings.mcp_config.stdio_servers)
|
||||
+ list(self.mcp_config.stdio_servers),
|
||||
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
|
||||
|
||||
0
tests/unit/core/__init__.py
Normal file
0
tests/unit/core/__init__.py
Normal file
0
tests/unit/core/config/__init__.py
Normal file
0
tests/unit/core/config/__init__.py
Normal file
335
tests/unit/core/config/test_configuration_merger.py
Normal file
335
tests/unit/core/config/test_configuration_merger.py
Normal file
@@ -0,0 +1,335 @@
|
||||
"""Tests for the ConfigurationMerger class."""
|
||||
|
||||
from pydantic import SecretStr
|
||||
|
||||
from openhands.core.config import ConfigurationMerger, OpenHandsConfig
|
||||
from openhands.core.config.mcp_config import (
|
||||
MCPConfig,
|
||||
MCPSSEServerConfig,
|
||||
MCPStdioServerConfig,
|
||||
)
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
|
||||
|
||||
def test_merge_core_settings():
|
||||
"""Test merging core settings."""
|
||||
# Create a config with default values
|
||||
config = OpenHandsConfig()
|
||||
|
||||
# Create settings with different values
|
||||
settings = Settings(
|
||||
agent='TestAgent',
|
||||
max_iterations=100,
|
||||
max_budget_per_task=50.0,
|
||||
git_user_name='test-user',
|
||||
git_user_email='test@example.com',
|
||||
search_api_key=SecretStr('test-search-key'),
|
||||
)
|
||||
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(settings, config)
|
||||
|
||||
# Check that the merged config has the values from settings
|
||||
assert merged_config.default_agent == 'TestAgent'
|
||||
assert merged_config.max_iterations == 100
|
||||
assert merged_config.max_budget_per_task == 50.0
|
||||
assert merged_config.git_user_name == 'test-user'
|
||||
assert merged_config.git_user_email == 'test@example.com'
|
||||
assert merged_config.search_api_key.get_secret_value() == 'test-search-key'
|
||||
|
||||
# Check that the original config is unchanged
|
||||
assert config.default_agent != 'TestAgent'
|
||||
assert config.max_iterations != 100
|
||||
assert config.max_budget_per_task != 50.0
|
||||
assert config.git_user_name != 'test-user'
|
||||
assert config.git_user_email != 'test@example.com'
|
||||
assert config.search_api_key != SecretStr('test-search-key')
|
||||
|
||||
|
||||
def test_merge_llm_settings():
|
||||
"""Test merging LLM settings."""
|
||||
# Create a config with default values
|
||||
config = OpenHandsConfig()
|
||||
|
||||
# Create settings with different values
|
||||
settings = Settings(
|
||||
llm_model='test-model',
|
||||
llm_api_key=SecretStr('test-api-key'),
|
||||
llm_base_url='https://test-api.example.com',
|
||||
)
|
||||
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(settings, config)
|
||||
|
||||
# Check that the merged config has the values from settings
|
||||
llm_config = merged_config.get_llm_config()
|
||||
assert llm_config.model == 'test-model'
|
||||
assert llm_config.api_key.get_secret_value() == 'test-api-key'
|
||||
assert llm_config.base_url == 'https://test-api.example.com'
|
||||
|
||||
# Check that the original config is unchanged
|
||||
original_llm_config = config.get_llm_config()
|
||||
assert original_llm_config.model != 'test-model'
|
||||
assert original_llm_config.api_key != SecretStr('test-api-key')
|
||||
assert original_llm_config.base_url != 'https://test-api.example.com'
|
||||
|
||||
|
||||
def test_merge_security_settings():
|
||||
"""Test merging security settings."""
|
||||
# Create a config with default values
|
||||
config = OpenHandsConfig()
|
||||
|
||||
# Create settings with different values
|
||||
settings = Settings(
|
||||
confirmation_mode=True,
|
||||
security_analyzer='test-analyzer',
|
||||
)
|
||||
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(settings, config)
|
||||
|
||||
# Check that the merged config has the values from settings
|
||||
assert merged_config.security.confirmation_mode is True
|
||||
assert merged_config.security.security_analyzer == 'test-analyzer'
|
||||
|
||||
# Check that the original config is unchanged
|
||||
assert config.security.confirmation_mode is not True
|
||||
assert config.security.security_analyzer != 'test-analyzer'
|
||||
|
||||
|
||||
def test_merge_sandbox_settings():
|
||||
"""Test merging sandbox settings."""
|
||||
# Create a config with default values
|
||||
config = OpenHandsConfig()
|
||||
|
||||
# Create settings with different values
|
||||
settings = Settings(
|
||||
sandbox_base_container_image='test-base-image',
|
||||
sandbox_runtime_container_image='test-runtime-image',
|
||||
sandbox_api_key=SecretStr('test-sandbox-key'),
|
||||
)
|
||||
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(settings, config)
|
||||
|
||||
# Check that the merged config has the values from settings
|
||||
assert merged_config.sandbox.base_container_image == 'test-base-image'
|
||||
assert merged_config.sandbox.runtime_container_image == 'test-runtime-image'
|
||||
# API key is now stored as SecretStr
|
||||
assert merged_config.sandbox.api_key.get_secret_value() == 'test-sandbox-key'
|
||||
|
||||
# Check that the original config is unchanged
|
||||
assert config.sandbox.base_container_image != 'test-base-image'
|
||||
assert config.sandbox.runtime_container_image != 'test-runtime-image'
|
||||
assert config.sandbox.api_key != 'test-sandbox-key'
|
||||
|
||||
|
||||
def test_merge_mcp_settings_config_only():
|
||||
"""Test merging MCP settings when only config has MCP settings."""
|
||||
# Create a config with MCP settings
|
||||
config = OpenHandsConfig()
|
||||
config.mcp = MCPConfig(
|
||||
sse_servers=[MCPSSEServerConfig(url='http://config-server.com')],
|
||||
stdio_servers=[],
|
||||
shttp_servers=[],
|
||||
)
|
||||
|
||||
# Create settings without MCP config
|
||||
settings = Settings(llm_model='test-model')
|
||||
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(settings, config)
|
||||
|
||||
# Check that the merged config has the MCP settings from config
|
||||
assert len(merged_config.mcp.sse_servers) == 1
|
||||
assert merged_config.mcp.sse_servers[0].url == 'http://config-server.com'
|
||||
|
||||
# Check that other settings are applied
|
||||
assert merged_config.get_llm_config().model == 'test-model'
|
||||
|
||||
|
||||
def test_merge_mcp_settings_settings_only():
|
||||
"""Test merging MCP settings when only settings has MCP settings."""
|
||||
# Create a config without MCP settings
|
||||
config = OpenHandsConfig()
|
||||
config.mcp = None
|
||||
|
||||
# Create settings with MCP config
|
||||
settings = Settings(
|
||||
llm_model='test-model',
|
||||
mcp_config=MCPConfig(
|
||||
sse_servers=[MCPSSEServerConfig(url='http://settings-server.com')],
|
||||
stdio_servers=[],
|
||||
shttp_servers=[],
|
||||
),
|
||||
)
|
||||
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(settings, config)
|
||||
|
||||
# Check that the merged config has the MCP settings from settings
|
||||
assert merged_config.mcp is not None
|
||||
assert len(merged_config.mcp.sse_servers) == 1
|
||||
assert merged_config.mcp.sse_servers[0].url == 'http://settings-server.com'
|
||||
|
||||
# Check that other settings are applied
|
||||
assert merged_config.get_llm_config().model == 'test-model'
|
||||
|
||||
|
||||
def test_merge_mcp_settings_both():
|
||||
"""Test merging MCP settings when both config and settings have MCP settings."""
|
||||
# Create a config with MCP settings
|
||||
config = OpenHandsConfig()
|
||||
config.mcp = MCPConfig(
|
||||
sse_servers=[MCPSSEServerConfig(url='http://config-server.com')],
|
||||
stdio_servers=[
|
||||
MCPStdioServerConfig(
|
||||
name='config-stdio', command='config-cmd', args=['arg1']
|
||||
)
|
||||
],
|
||||
shttp_servers=[],
|
||||
)
|
||||
|
||||
# Create settings with MCP config
|
||||
settings = Settings(
|
||||
llm_model='test-model',
|
||||
mcp_config=MCPConfig(
|
||||
sse_servers=[MCPSSEServerConfig(url='http://settings-server.com')],
|
||||
stdio_servers=[
|
||||
MCPStdioServerConfig(
|
||||
name='settings-stdio', command='settings-cmd', args=['arg2']
|
||||
)
|
||||
],
|
||||
shttp_servers=[],
|
||||
),
|
||||
)
|
||||
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(settings, config)
|
||||
|
||||
# Check that the merged config has the MCP settings from both config and settings
|
||||
assert merged_config.mcp is not None
|
||||
assert len(merged_config.mcp.sse_servers) == 2
|
||||
assert merged_config.mcp.sse_servers[0].url == 'http://config-server.com'
|
||||
assert merged_config.mcp.sse_servers[1].url == 'http://settings-server.com'
|
||||
|
||||
assert len(merged_config.mcp.stdio_servers) == 2
|
||||
assert merged_config.mcp.stdio_servers[0].name == 'config-stdio'
|
||||
assert merged_config.mcp.stdio_servers[1].name == 'settings-stdio'
|
||||
|
||||
# Check that other settings are applied
|
||||
assert merged_config.get_llm_config().model == 'test-model'
|
||||
|
||||
|
||||
def test_merge_with_none_values():
|
||||
"""Test merging with None values in settings."""
|
||||
# Create a config with default values
|
||||
config = OpenHandsConfig()
|
||||
config.default_agent = 'DefaultAgent'
|
||||
config.max_iterations = 200
|
||||
|
||||
# Create settings with some None values
|
||||
settings = Settings(
|
||||
agent=None, # Should not override
|
||||
max_iterations=100, # Should override
|
||||
)
|
||||
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(settings, config)
|
||||
|
||||
# Check that None values in settings don't override config values
|
||||
assert merged_config.default_agent == 'DefaultAgent'
|
||||
|
||||
# Check that non-None values in settings do override config values
|
||||
assert merged_config.max_iterations == 100
|
||||
|
||||
|
||||
def test_config_to_settings():
|
||||
"""Test converting config to settings."""
|
||||
# Create a config with custom values
|
||||
config = OpenHandsConfig()
|
||||
config.default_agent = 'TestAgent'
|
||||
config.max_iterations = 100
|
||||
config.max_budget_per_task = 50.0
|
||||
config.git_user_name = 'test-user'
|
||||
config.git_user_email = 'test@example.com'
|
||||
config.search_api_key = 'test-search-key'
|
||||
|
||||
# Set LLM config
|
||||
llm_config = config.get_llm_config()
|
||||
llm_config.model = 'test-model'
|
||||
llm_config.api_key = 'test-api-key'
|
||||
llm_config.base_url = 'https://test-api.example.com'
|
||||
|
||||
# Set security config
|
||||
config.security.confirmation_mode = True
|
||||
config.security.security_analyzer = 'test-analyzer'
|
||||
|
||||
# Set sandbox config
|
||||
config.sandbox.base_container_image = 'test-base-image'
|
||||
config.sandbox.runtime_container_image = 'test-runtime-image'
|
||||
config.sandbox.api_key = 'test-sandbox-key'
|
||||
|
||||
# Set MCP config
|
||||
config.mcp = MCPConfig(
|
||||
sse_servers=[MCPSSEServerConfig(url='http://config-server.com')],
|
||||
stdio_servers=[
|
||||
MCPStdioServerConfig(
|
||||
name='config-stdio', command='config-cmd', args=['arg1']
|
||||
)
|
||||
],
|
||||
shttp_servers=[],
|
||||
)
|
||||
|
||||
# Convert config to settings
|
||||
settings = ConfigurationMerger.config_to_settings(config)
|
||||
|
||||
# Check that settings has the values from config
|
||||
assert settings.agent == 'TestAgent'
|
||||
assert settings.max_iterations == 100
|
||||
assert settings.max_budget_per_task == 50.0
|
||||
assert settings.git_user_name == 'test-user'
|
||||
assert settings.git_user_email == 'test@example.com'
|
||||
assert settings.search_api_key.get_secret_value() == 'test-search-key'
|
||||
|
||||
assert settings.llm_model == 'test-model'
|
||||
assert settings.llm_api_key.get_secret_value() == 'test-api-key'
|
||||
assert settings.llm_base_url == 'https://test-api.example.com'
|
||||
|
||||
assert settings.confirmation_mode is True
|
||||
assert settings.security_analyzer == 'test-analyzer'
|
||||
|
||||
assert settings.sandbox_base_container_image == 'test-base-image'
|
||||
assert settings.sandbox_runtime_container_image == 'test-runtime-image'
|
||||
assert settings.sandbox_api_key.get_secret_value() == 'test-sandbox-key'
|
||||
|
||||
assert settings.mcp_config is not None
|
||||
assert len(settings.mcp_config.sse_servers) == 1
|
||||
assert settings.mcp_config.sse_servers[0].url == 'http://config-server.com'
|
||||
assert len(settings.mcp_config.stdio_servers) == 1
|
||||
assert settings.mcp_config.stdio_servers[0].name == 'config-stdio'
|
||||
|
||||
|
||||
def test_bidirectional_conversion():
|
||||
"""Test that config -> settings -> config preserves values."""
|
||||
# Create a config with custom values
|
||||
original_config = OpenHandsConfig()
|
||||
original_config.default_agent = 'TestAgent'
|
||||
original_config.max_iterations = 100
|
||||
|
||||
# Set LLM config
|
||||
llm_config = original_config.get_llm_config()
|
||||
llm_config.model = 'test-model'
|
||||
llm_config.api_key = 'test-api-key'
|
||||
|
||||
# Convert config to settings
|
||||
settings = ConfigurationMerger.config_to_settings(original_config)
|
||||
|
||||
# Convert settings back to config
|
||||
config = ConfigurationMerger.merge_settings_with_config(settings, OpenHandsConfig())
|
||||
|
||||
# Check that the values are preserved
|
||||
assert config.default_agent == 'TestAgent'
|
||||
assert config.max_iterations == 100
|
||||
assert config.get_llm_config().model == 'test-model'
|
||||
assert config.get_llm_config().api_key.get_secret_value() == 'test-api-key'
|
||||
@@ -12,6 +12,15 @@ def test_commit_message_with_quotes():
|
||||
with tempfile.TemporaryDirectory() as temp_dir:
|
||||
subprocess.run(['git', 'init', temp_dir], check=True)
|
||||
|
||||
# Set git config for the test repository
|
||||
subprocess.run(
|
||||
['git', '-C', temp_dir, 'config', 'user.name', 'Test User'], check=True
|
||||
)
|
||||
subprocess.run(
|
||||
['git', '-C', temp_dir, 'config', 'user.email', 'test@example.com'],
|
||||
check=True,
|
||||
)
|
||||
|
||||
# Create a test file and add it to git
|
||||
test_file = os.path.join(temp_dir, 'test.txt')
|
||||
with open(test_file, 'w') as f:
|
||||
@@ -36,8 +45,14 @@ def test_commit_message_with_quotes():
|
||||
thread_ids=None,
|
||||
)
|
||||
|
||||
# Make the commit
|
||||
make_commit(temp_dir, issue, 'issue')
|
||||
# Make the commit with the same user name and email as configured above
|
||||
make_commit(
|
||||
temp_dir,
|
||||
issue,
|
||||
'issue',
|
||||
git_user_name='Test User',
|
||||
git_user_email='test@example.com',
|
||||
)
|
||||
|
||||
# Get the commit message
|
||||
result = subprocess.run(
|
||||
@@ -130,8 +145,21 @@ def test_pr_title_with_quotes(monkeypatch):
|
||||
|
||||
print('Adding and committing test file...')
|
||||
subprocess.run(['git', '-C', temp_dir, 'add', 'test.txt'], check=True)
|
||||
# Use -c to set the committer identity for this command only
|
||||
subprocess.run(
|
||||
['git', '-C', temp_dir, 'commit', '-m', 'Initial commit'], check=True
|
||||
[
|
||||
'git',
|
||||
'-C',
|
||||
temp_dir,
|
||||
'-c',
|
||||
'user.name=Test User',
|
||||
'-c',
|
||||
'user.email=test@example.com',
|
||||
'commit',
|
||||
'-m',
|
||||
'Initial commit',
|
||||
],
|
||||
check=True,
|
||||
)
|
||||
|
||||
# Create a test issue with problematic title
|
||||
|
||||
@@ -13,6 +13,15 @@ def test_commit_message_with_quotes():
|
||||
with tempfile.TemporaryDirectory() as temp_dir:
|
||||
subprocess.run(['git', 'init', temp_dir], check=True)
|
||||
|
||||
# Set git config for the test repository
|
||||
subprocess.run(
|
||||
['git', '-C', temp_dir, 'config', 'user.name', 'Test User'], check=True
|
||||
)
|
||||
subprocess.run(
|
||||
['git', '-C', temp_dir, 'config', 'user.email', 'test@example.com'],
|
||||
check=True,
|
||||
)
|
||||
|
||||
# Create a test file and add it to git
|
||||
test_file = os.path.join(temp_dir, 'test.txt')
|
||||
with open(test_file, 'w') as f:
|
||||
@@ -37,8 +46,14 @@ def test_commit_message_with_quotes():
|
||||
thread_ids=None,
|
||||
)
|
||||
|
||||
# Make the commit
|
||||
make_commit(temp_dir, issue, 'issue')
|
||||
# Make the commit with the same user name and email as configured above
|
||||
make_commit(
|
||||
temp_dir,
|
||||
issue,
|
||||
'issue',
|
||||
git_user_name='Test User',
|
||||
git_user_email='test@example.com',
|
||||
)
|
||||
|
||||
# Get the commit message
|
||||
result = subprocess.run(
|
||||
@@ -131,8 +146,21 @@ def test_pr_title_with_quotes(monkeypatch):
|
||||
|
||||
logger.info('Adding and committing test file...')
|
||||
subprocess.run(['git', '-C', temp_dir, 'add', 'test.txt'], check=True)
|
||||
# Use -c to set the committer identity for this command only
|
||||
subprocess.run(
|
||||
['git', '-C', temp_dir, 'commit', '-m', 'Initial commit'], check=True
|
||||
[
|
||||
'git',
|
||||
'-C',
|
||||
temp_dir,
|
||||
'-c',
|
||||
'user.name=Test User',
|
||||
'-c',
|
||||
'user.email=test@example.com',
|
||||
'commit',
|
||||
'-m',
|
||||
'Initial commit',
|
||||
],
|
||||
check=True,
|
||||
)
|
||||
|
||||
# Create a test issue with problematic title
|
||||
|
||||
@@ -4,6 +4,7 @@ from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from openhands.core.config import OpenHandsConfig
|
||||
from openhands.core.config.mcp_config import MCPConfig, MCPSSEServerConfig
|
||||
from openhands.server.user_auth.default_user_auth import DefaultUserAuth
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
@@ -36,10 +37,17 @@ async def test_user_auth_mcp_merging_integration():
|
||||
mock_settings_store = AsyncMock(spec=FileSettingsStore)
|
||||
mock_settings_store.load.return_value = stored_settings
|
||||
|
||||
# Create a mock config with MCP settings
|
||||
mock_config = OpenHandsConfig()
|
||||
mock_config.mcp = config_settings.mcp_config
|
||||
|
||||
with patch.object(
|
||||
user_auth, 'get_user_settings_store', return_value=mock_settings_store
|
||||
):
|
||||
with patch.object(Settings, 'from_config', return_value=config_settings):
|
||||
with patch(
|
||||
'openhands.server.user_auth.default_user_auth.OpenHandsConfig',
|
||||
return_value=mock_config,
|
||||
):
|
||||
# Get user settings - this should trigger the merging
|
||||
merged_settings = await user_auth.get_user_settings()
|
||||
|
||||
@@ -76,12 +84,17 @@ async def test_user_auth_caching_behavior():
|
||||
mock_settings_store = AsyncMock(spec=FileSettingsStore)
|
||||
mock_settings_store.load.return_value = stored_settings
|
||||
|
||||
# Create a mock config with MCP settings
|
||||
mock_config = OpenHandsConfig()
|
||||
mock_config.mcp = config_settings.mcp_config
|
||||
|
||||
with patch.object(
|
||||
user_auth, 'get_user_settings_store', return_value=mock_settings_store
|
||||
):
|
||||
with patch.object(
|
||||
Settings, 'from_config', return_value=config_settings
|
||||
) as mock_from_config:
|
||||
with patch(
|
||||
'openhands.server.user_auth.default_user_auth.OpenHandsConfig',
|
||||
return_value=mock_config,
|
||||
) as mock_config_class:
|
||||
# First call should load and merge
|
||||
settings1 = await user_auth.get_user_settings()
|
||||
|
||||
@@ -95,8 +108,8 @@ async def test_user_auth_caching_behavior():
|
||||
# Settings store should only be called once (first time)
|
||||
mock_settings_store.load.assert_called_once()
|
||||
|
||||
# from_config should only be called once (during merging)
|
||||
mock_from_config.assert_called_once()
|
||||
# OpenHandsConfig should only be called once (during merging)
|
||||
mock_config_class.assert_called_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
@@ -1,9 +1,8 @@
|
||||
"""Test MCP settings merging functionality."""
|
||||
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
from openhands.core.config import ConfigurationMerger, OpenHandsConfig
|
||||
from openhands.core.config.mcp_config import (
|
||||
MCPConfig,
|
||||
MCPSSEServerConfig,
|
||||
@@ -15,31 +14,37 @@ from openhands.storage.data_models.settings import Settings
|
||||
@pytest.mark.asyncio
|
||||
async def test_mcp_settings_merge_config_only():
|
||||
"""Test merging when only config.toml has MCP settings."""
|
||||
# Mock config.toml with MCP settings
|
||||
mock_config_settings = Settings(
|
||||
mcp_config=MCPConfig(
|
||||
sse_servers=[MCPSSEServerConfig(url='http://config-server.com')]
|
||||
)
|
||||
# Create a config with MCP settings
|
||||
config = OpenHandsConfig()
|
||||
config.mcp = MCPConfig(
|
||||
sse_servers=[MCPSSEServerConfig(url='http://config-server.com')]
|
||||
)
|
||||
|
||||
# Frontend settings without MCP config
|
||||
frontend_settings = Settings(llm_model='gpt-4')
|
||||
|
||||
with patch.object(Settings, 'from_config', return_value=mock_config_settings):
|
||||
merged_settings = frontend_settings.merge_with_config_settings()
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(
|
||||
frontend_settings, config
|
||||
)
|
||||
|
||||
# Should use config.toml MCP settings
|
||||
assert merged_settings.mcp_config is not None
|
||||
assert len(merged_settings.mcp_config.sse_servers) == 1
|
||||
assert merged_settings.mcp_config.sse_servers[0].url == 'http://config-server.com'
|
||||
assert merged_settings.llm_model == 'gpt-4'
|
||||
assert merged_config.mcp is not None
|
||||
assert len(merged_config.mcp.sse_servers) == 1
|
||||
assert merged_config.mcp.sse_servers[0].url == 'http://config-server.com'
|
||||
assert merged_config.get_llm_config().model == 'gpt-4'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_mcp_settings_merge_frontend_only():
|
||||
"""Test merging when only frontend has MCP settings."""
|
||||
# Mock config.toml without MCP settings
|
||||
mock_config_settings = Settings(llm_model='claude-3')
|
||||
# Create a config without MCP settings
|
||||
config = OpenHandsConfig()
|
||||
config.mcp = None
|
||||
|
||||
# Set a different LLM model in config
|
||||
llm_config = config.get_llm_config()
|
||||
llm_config.model = 'claude-3'
|
||||
|
||||
# Frontend settings with MCP config
|
||||
frontend_settings = Settings(
|
||||
@@ -49,29 +54,30 @@ async def test_mcp_settings_merge_frontend_only():
|
||||
),
|
||||
)
|
||||
|
||||
with patch.object(Settings, 'from_config', return_value=mock_config_settings):
|
||||
merged_settings = frontend_settings.merge_with_config_settings()
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(
|
||||
frontend_settings, config
|
||||
)
|
||||
|
||||
# Should keep frontend MCP settings
|
||||
assert merged_settings.mcp_config is not None
|
||||
assert len(merged_settings.mcp_config.sse_servers) == 1
|
||||
assert merged_settings.mcp_config.sse_servers[0].url == 'http://frontend-server.com'
|
||||
assert merged_settings.llm_model == 'gpt-4'
|
||||
assert merged_config.mcp is not None
|
||||
assert len(merged_config.mcp.sse_servers) == 1
|
||||
assert merged_config.mcp.sse_servers[0].url == 'http://frontend-server.com'
|
||||
assert merged_config.get_llm_config().model == 'gpt-4'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_mcp_settings_merge_both_present():
|
||||
"""Test merging when both config.toml and frontend have MCP settings."""
|
||||
# Mock config.toml with MCP settings
|
||||
mock_config_settings = Settings(
|
||||
mcp_config=MCPConfig(
|
||||
sse_servers=[MCPSSEServerConfig(url='http://config-server.com')],
|
||||
stdio_servers=[
|
||||
MCPStdioServerConfig(
|
||||
name='config-stdio', command='config-cmd', args=['arg1']
|
||||
)
|
||||
],
|
||||
)
|
||||
# Create a config with MCP settings
|
||||
config = OpenHandsConfig()
|
||||
config.mcp = MCPConfig(
|
||||
sse_servers=[MCPSSEServerConfig(url='http://config-server.com')],
|
||||
stdio_servers=[
|
||||
MCPStdioServerConfig(
|
||||
name='config-stdio', command='config-cmd', args=['arg1']
|
||||
)
|
||||
],
|
||||
)
|
||||
|
||||
# Frontend settings with different MCP config
|
||||
@@ -87,27 +93,30 @@ async def test_mcp_settings_merge_both_present():
|
||||
),
|
||||
)
|
||||
|
||||
with patch.object(Settings, 'from_config', return_value=mock_config_settings):
|
||||
merged_settings = frontend_settings.merge_with_config_settings()
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(
|
||||
frontend_settings, config
|
||||
)
|
||||
|
||||
# Should merge both with config.toml taking priority (appearing first)
|
||||
assert merged_settings.mcp_config is not None
|
||||
assert len(merged_settings.mcp_config.sse_servers) == 2
|
||||
assert merged_settings.mcp_config.sse_servers[0].url == 'http://config-server.com'
|
||||
assert merged_settings.mcp_config.sse_servers[1].url == 'http://frontend-server.com'
|
||||
assert merged_config.mcp is not None
|
||||
assert len(merged_config.mcp.sse_servers) == 2
|
||||
assert merged_config.mcp.sse_servers[0].url == 'http://config-server.com'
|
||||
assert merged_config.mcp.sse_servers[1].url == 'http://frontend-server.com'
|
||||
|
||||
assert len(merged_settings.mcp_config.stdio_servers) == 2
|
||||
assert merged_settings.mcp_config.stdio_servers[0].name == 'config-stdio'
|
||||
assert merged_settings.mcp_config.stdio_servers[1].name == 'frontend-stdio'
|
||||
assert len(merged_config.mcp.stdio_servers) == 2
|
||||
assert merged_config.mcp.stdio_servers[0].name == 'config-stdio'
|
||||
assert merged_config.mcp.stdio_servers[1].name == 'frontend-stdio'
|
||||
|
||||
assert merged_settings.llm_model == 'gpt-4'
|
||||
assert merged_config.get_llm_config().model == 'gpt-4'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_mcp_settings_merge_no_config():
|
||||
"""Test merging when config.toml has no MCP settings."""
|
||||
# Mock config.toml without MCP settings
|
||||
mock_config_settings = None
|
||||
"""Test merging when config has no MCP settings."""
|
||||
# Create a config without MCP settings
|
||||
config = OpenHandsConfig()
|
||||
config.mcp = None
|
||||
|
||||
# Frontend settings with MCP config
|
||||
frontend_settings = Settings(
|
||||
@@ -117,28 +126,37 @@ async def test_mcp_settings_merge_no_config():
|
||||
),
|
||||
)
|
||||
|
||||
with patch.object(Settings, 'from_config', return_value=mock_config_settings):
|
||||
merged_settings = frontend_settings.merge_with_config_settings()
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(
|
||||
frontend_settings, config
|
||||
)
|
||||
|
||||
# Should keep frontend settings unchanged
|
||||
assert merged_settings.mcp_config is not None
|
||||
assert len(merged_settings.mcp_config.sse_servers) == 1
|
||||
assert merged_settings.mcp_config.sse_servers[0].url == 'http://frontend-server.com'
|
||||
assert merged_settings.llm_model == 'gpt-4'
|
||||
assert merged_config.mcp is not None
|
||||
assert len(merged_config.mcp.sse_servers) == 1
|
||||
assert merged_config.mcp.sse_servers[0].url == 'http://frontend-server.com'
|
||||
assert merged_config.get_llm_config().model == 'gpt-4'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_mcp_settings_merge_neither_present():
|
||||
"""Test merging when neither config.toml nor frontend have MCP settings."""
|
||||
# Mock config.toml without MCP settings
|
||||
mock_config_settings = Settings(llm_model='claude-3')
|
||||
"""Test merging when neither config nor frontend have MCP settings."""
|
||||
# Create a config without MCP settings
|
||||
config = OpenHandsConfig()
|
||||
config.mcp = None
|
||||
|
||||
# Set a different LLM model in config
|
||||
llm_config = config.get_llm_config()
|
||||
llm_config.model = 'claude-3'
|
||||
|
||||
# Frontend settings without MCP config
|
||||
frontend_settings = Settings(llm_model='gpt-4')
|
||||
|
||||
with patch.object(Settings, 'from_config', return_value=mock_config_settings):
|
||||
merged_settings = frontend_settings.merge_with_config_settings()
|
||||
# Merge settings with config
|
||||
merged_config = ConfigurationMerger.merge_settings_with_config(
|
||||
frontend_settings, config
|
||||
)
|
||||
|
||||
# Should keep frontend settings unchanged
|
||||
assert merged_settings.mcp_config is None
|
||||
assert merged_settings.llm_model == 'gpt-4'
|
||||
assert merged_config.mcp is None
|
||||
assert merged_config.get_llm_config().model == 'gpt-4'
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
from unittest.mock import patch
|
||||
|
||||
from pydantic import SecretStr
|
||||
|
||||
from openhands.core.config.configuration_merger import ConfigurationMerger
|
||||
from openhands.core.config.llm_config import LLMConfig
|
||||
from openhands.core.config.openhands_config import OpenHandsConfig
|
||||
from openhands.core.config.sandbox_config import SandboxConfig
|
||||
@@ -10,9 +9,9 @@ from openhands.server.routes.settings import convert_to_settings
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
|
||||
|
||||
def test_settings_from_config():
|
||||
# Mock configuration
|
||||
mock_app_config = OpenHandsConfig(
|
||||
def test_config_to_settings():
|
||||
# Create a configuration
|
||||
config = OpenHandsConfig(
|
||||
default_agent='test-agent',
|
||||
max_iterations=100,
|
||||
security=SecurityConfig(
|
||||
@@ -28,28 +27,26 @@ def test_settings_from_config():
|
||||
sandbox=SandboxConfig(remote_runtime_resource_factor=2),
|
||||
)
|
||||
|
||||
with patch(
|
||||
'openhands.storage.data_models.settings.load_openhands_config',
|
||||
return_value=mock_app_config,
|
||||
):
|
||||
settings = Settings.from_config()
|
||||
# Convert config to settings
|
||||
settings = ConfigurationMerger.config_to_settings(config)
|
||||
|
||||
assert settings is not None
|
||||
assert settings.language == 'en'
|
||||
assert settings.agent == 'test-agent'
|
||||
assert settings.max_iterations == 100
|
||||
assert settings.security_analyzer == 'test-analyzer'
|
||||
assert settings.confirmation_mode is True
|
||||
assert settings.llm_model == 'test-model'
|
||||
assert settings.llm_api_key.get_secret_value() == 'test-key'
|
||||
assert settings.llm_base_url == 'https://test.example.com'
|
||||
assert settings.remote_runtime_resource_factor == 2
|
||||
assert not settings.secrets_store.provider_tokens
|
||||
# Verify settings
|
||||
assert settings is not None
|
||||
assert settings.language == 'en'
|
||||
assert settings.agent == 'test-agent'
|
||||
assert settings.max_iterations == 100
|
||||
assert settings.security_analyzer == 'test-analyzer'
|
||||
assert settings.confirmation_mode is True
|
||||
assert settings.llm_model == 'test-model'
|
||||
assert settings.llm_api_key.get_secret_value() == 'test-key'
|
||||
assert settings.llm_base_url == 'https://test.example.com'
|
||||
assert settings.remote_runtime_resource_factor == 2
|
||||
assert not settings.secrets_store.provider_tokens
|
||||
|
||||
|
||||
def test_settings_from_config_no_api_key():
|
||||
# Mock configuration without API key
|
||||
mock_app_config = OpenHandsConfig(
|
||||
def test_config_to_settings_no_api_key():
|
||||
# Create a configuration without API key
|
||||
config = OpenHandsConfig(
|
||||
default_agent='test-agent',
|
||||
max_iterations=100,
|
||||
security=SecurityConfig(
|
||||
@@ -63,12 +60,12 @@ def test_settings_from_config_no_api_key():
|
||||
sandbox=SandboxConfig(remote_runtime_resource_factor=2),
|
||||
)
|
||||
|
||||
with patch(
|
||||
'openhands.storage.data_models.settings.load_openhands_config',
|
||||
return_value=mock_app_config,
|
||||
):
|
||||
settings = Settings.from_config()
|
||||
assert settings is None
|
||||
# Convert config to settings
|
||||
settings = ConfigurationMerger.config_to_settings(config)
|
||||
|
||||
# Verify settings
|
||||
assert settings is not None
|
||||
assert settings.llm_api_key is None
|
||||
|
||||
|
||||
def test_settings_handles_sensitive_data():
|
||||
|
||||
Reference in New Issue
Block a user