Merge branch 'main' into fix/cleanup-orphaned-localstorage-on-conversation-delete

This commit is contained in:
Tim O'Farrell
2026-01-04 20:45:03 -07:00
committed by GitHub
14 changed files with 437 additions and 48 deletions

View File

@@ -143,7 +143,7 @@ class GitHubDataCollector:
try:
installation_token = self._get_installation_access_token(installation_id)
with Github(installation_token) as github_client:
with Github(auth=Auth.Token(installation_token)) as github_client:
repo = github_client.get_repo(repo_name)
issue = repo.get_issue(issue_number)
comments = []
@@ -237,7 +237,7 @@ class GitHubDataCollector:
def _get_pr_commits(self, installation_id: str, repo_name: str, pr_number: int):
commits = []
installation_token = self._get_installation_access_token(installation_id)
with Github(installation_token) as github_client:
with Github(auth=Auth.Token(installation_token)) as github_client:
repo = github_client.get_repo(repo_name)
pr = repo.get_pull(pr_number)

View File

@@ -77,7 +77,7 @@ class GithubManager(Manager):
reaction: The reaction to add (e.g. "eyes", "+1", "-1", "laugh", "confused", "heart", "hooray", "rocket")
installation_token: GitHub installation access token for API access
"""
with Github(installation_token) as github_client:
with Github(auth=Auth.Token(installation_token)) as github_client:
repo = github_client.get_repo(github_view.full_repo_name)
# Add reaction based on view type
if isinstance(github_view, GithubInlinePRComment):
@@ -199,7 +199,7 @@ class GithubManager(Manager):
outgoing_message = message.message
if isinstance(github_view, GithubInlinePRComment):
with Github(installation_token) as github_client:
with Github(auth=Auth.Token(installation_token)) as github_client:
repo = github_client.get_repo(github_view.full_repo_name)
pr = repo.get_pull(github_view.issue_number)
pr.create_review_comment_reply(
@@ -211,7 +211,7 @@ class GithubManager(Manager):
or isinstance(github_view, GithubIssueComment)
or isinstance(github_view, GithubIssue)
):
with Github(installation_token) as github_client:
with Github(auth=Auth.Token(installation_token)) as github_client:
repo = github_client.get_repo(github_view.full_repo_name)
issue = repo.get_issue(number=github_view.issue_number)
issue.create_comment(outgoing_message)

View File

@@ -1,7 +1,7 @@
import asyncio
import time
from github import Github
from github import Auth, Github
from integrations.github.github_view import (
GithubInlinePRComment,
GithubIssueComment,
@@ -47,7 +47,7 @@ def fetch_github_issue_context(
context_parts.append(f'Title: {github_view.title}')
context_parts.append(f'Description:\n{github_view.description}')
with Github(user_token) as github_client:
with Github(auth=Auth.Token(user_token)) as github_client:
repo = github_client.get_repo(github_view.full_repo_name)
issue = repo.get_issue(github_view.issue_number)
if issue.labels:

View File

@@ -735,7 +735,7 @@ class GithubFactory:
payload['installation']['id']
).token
with Github(access_token) as gh:
with Github(auth=Auth.Token(access_token)) as gh:
repo = gh.get_repo(selected_repo)
login = (
payload['organization']['login']
@@ -872,7 +872,7 @@ class GithubFactory:
access_token = integration.get_access_token(installation_id).token
head_ref = None
with Github(access_token) as gh:
with Github(auth=Auth.Token(access_token)) as gh:
repo = gh.get_repo(selected_repo)
pull_request = repo.get_pull(issue_number)
head_ref = pull_request.head.ref

View File

@@ -20,6 +20,7 @@ from openhands.events.action import (
AgentFinishAction,
MessageAction,
)
from openhands.events.event_filter import EventFilter
from openhands.events.event_store_abc import EventStoreABC
from openhands.events.observation.agent import AgentStateChangedObservation
from openhands.integrations.service_types import Repository
@@ -203,18 +204,35 @@ def get_summary_for_agent_state(
def get_final_agent_observation(
event_store: EventStoreABC,
) -> list[AgentStateChangedObservation]:
return event_store.get_matching_events(
source=EventSource.ENVIRONMENT,
event_types=(AgentStateChangedObservation,),
limit=1,
reverse=True,
events = list(
event_store.search_events(
filter=EventFilter(
source=EventSource.ENVIRONMENT,
include_types=(AgentStateChangedObservation,),
),
limit=1,
reverse=True,
)
)
result = [e for e in events if isinstance(e, AgentStateChangedObservation)]
assert len(result) == len(events)
return result
def get_last_user_msg(event_store: EventStoreABC) -> list[MessageAction]:
return event_store.get_matching_events(
source=EventSource.USER, event_types=(MessageAction,), limit=1, reverse='true'
events = list(
event_store.search_events(
filter=EventFilter(
source=EventSource.USER,
include_types=(MessageAction,),
),
limit=1,
reverse=True,
)
)
result = [e for e in events if isinstance(e, MessageAction)]
assert len(result) == len(events)
return result
def extract_summary_from_event_store(
@@ -226,18 +244,22 @@ def extract_summary_from_event_store(
conversation_link = CONVERSATION_URL.format(conversation_id)
summary_instruction = get_summary_instruction()
instruction_event: list[MessageAction] = event_store.get_matching_events(
query=json.dumps(summary_instruction),
source=EventSource.USER,
event_types=(MessageAction,),
limit=1,
reverse=True,
instruction_events = list(
event_store.search_events(
filter=EventFilter(
query=json.dumps(summary_instruction),
source=EventSource.USER,
include_types=(MessageAction,),
),
limit=1,
reverse=True,
)
)
final_agent_observation = get_final_agent_observation(event_store)
# Find summary instruction event ID
if len(instruction_event) == 0:
if not instruction_events:
logger.warning(
'no_instruction_event_found', extra={'conversation_id': conversation_id}
)
@@ -245,19 +267,19 @@ def extract_summary_from_event_store(
final_agent_observation, conversation_link
) # Agent did not receive summary instruction
event_id: int = instruction_event[0].id
agent_messages: list[MessageAction | AgentFinishAction] = (
event_store.get_matching_events(
start_id=event_id,
source=EventSource.AGENT,
event_types=(MessageAction, AgentFinishAction),
reverse=True,
summary_events = list(
event_store.search_events(
filter=EventFilter(
source=EventSource.AGENT,
include_types=(MessageAction, AgentFinishAction),
),
limit=1,
reverse=True,
start_id=instruction_events[0].id,
)
)
if len(agent_messages) == 0:
if not summary_events:
logger.warning(
'no_agent_messages_found', extra={'conversation_id': conversation_id}
)
@@ -265,10 +287,11 @@ def extract_summary_from_event_store(
final_agent_observation, conversation_link
) # Agent failed to generate summary
summary_event: MessageAction | AgentFinishAction = agent_messages[0]
summary_event = summary_events[0]
if isinstance(summary_event, MessageAction):
return summary_event.content
assert isinstance(summary_event, AgentFinishAction)
return summary_event.final_thought

View File

@@ -285,14 +285,21 @@ class SaasSettingsStore(SettingsStore):
'x-goog-api-key': LITE_LLM_API_KEY,
},
) as client:
# Get the previous max budget to prevent accidental loss
# In Litellm a get always succeeds, regardless of whether the user actually exists
# Get the previous max budget to prevent accidental loss.
#
# LiteLLM v1.80+ returns 404 for non-existent users (previously returned empty user_info)
response = await client.get(
f'{LITE_LLM_API_URL}/user/info?user_id={self.user_id}'
)
response.raise_for_status()
response_json = response.json()
user_info = response_json.get('user_info') or {}
user_info: dict
if response.status_code == 404:
# New user - doesn't exist in LiteLLM yet (v1.80+ behavior)
user_info = {}
else:
# For any other status, use standard error handling
response.raise_for_status()
response_json = response.json()
user_info = response_json.get('user_info') or {}
logger.info(
f'creating_litellm_user: {self.user_id}; prev_max_budget: {user_info.get("max_budget")}; prev_metadata: {user_info.get("metadata")}'
)

View File

@@ -1,5 +1,6 @@
from unittest.mock import AsyncMock, MagicMock, patch
import httpx
import pytest
from pydantic import SecretStr
from server.constants import (
@@ -335,6 +336,80 @@ async def test_update_settings_with_litellm_default_error(settings_store):
assert settings is None
@pytest.mark.asyncio
@pytest.mark.parametrize(
'status_code,user_info_response,should_succeed',
[
# 200 OK with user info - existing user (v1.79.x and v1.80+ behavior)
(200, {'user_info': {'max_budget': 10, 'spend': 5}}, True),
# 200 OK with empty user info - new user (v1.79.x behavior)
(200, {'user_info': None}, True),
# 404 Not Found - new user (v1.80+ behavior)
(404, None, True),
# 500 Internal Server Error - should fail
(500, None, False),
],
)
async def test_update_settings_with_litellm_default_handles_user_info_responses(
settings_store, session_maker, status_code, user_info_response, should_succeed
):
"""Test that various LiteLLM user/info responses are handled correctly.
LiteLLM API behavior changed between versions:
- v1.79.x and earlier: GET /user/info always succeeds with empty user_info
- v1.80.x and later: GET /user/info returns 404 for non-existent users
"""
mock_get_response = MagicMock()
mock_get_response.status_code = status_code
if user_info_response is not None:
mock_get_response.json = MagicMock(return_value=user_info_response)
mock_get_response.raise_for_status = MagicMock()
else:
mock_get_response.raise_for_status = MagicMock(
side_effect=httpx.HTTPStatusError(
'Error', request=MagicMock(), response=mock_get_response
)
if status_code >= 500
else None
)
# Mock successful responses for POST operations (delete and create)
mock_post_response = MagicMock()
mock_post_response.is_success = True
mock_post_response.json = MagicMock(return_value={'key': 'new_user_api_key'})
with (
patch('storage.saas_settings_store.LITE_LLM_API_KEY', 'test_key'),
patch('storage.saas_settings_store.LITE_LLM_API_URL', 'http://test.url'),
patch('storage.saas_settings_store.LITE_LLM_TEAM_ID', 'test_team'),
patch(
'server.auth.token_manager.TokenManager.get_user_info_from_user_id',
AsyncMock(return_value={'email': 'testuser@example.com'}),
),
patch('httpx.AsyncClient') as mock_client,
patch('storage.saas_settings_store.session_maker', session_maker),
):
# Set up the mock client
mock_client.return_value.__aenter__.return_value.get.return_value = (
mock_get_response
)
mock_client.return_value.__aenter__.return_value.post.return_value = (
mock_post_response
)
settings = Settings()
if should_succeed:
settings = await settings_store.update_settings_with_litellm_default(
settings
)
assert settings is not None
assert settings.llm_api_key is not None
assert settings.llm_api_key.get_secret_value() == 'new_user_api_key'
else:
with pytest.raises(httpx.HTTPStatusError):
await settings_store.update_settings_with_litellm_default(settings)
@pytest.mark.asyncio
async def test_update_settings_with_litellm_retry_on_duplicate_email(
settings_store, mock_litellm_api, session_maker

View File

@@ -67,7 +67,8 @@ def get_default_persistence_dir() -> Path:
def get_default_web_url() -> str | None:
"""Get legacy web host parameter.
If present, we assume we are running under https."""
If present, we assume we are running under https.
"""
web_host = os.getenv('WEB_HOST')
if not web_host:
return None
@@ -175,7 +176,17 @@ def config_from_env() -> AppServerConfig:
elif os.getenv('RUNTIME') in ('local', 'process'):
config.sandbox = ProcessSandboxServiceInjector()
else:
config.sandbox = DockerSandboxServiceInjector()
# Support legacy environment variables for Docker sandbox configuration
docker_sandbox_kwargs: dict = {}
if os.getenv('SANDBOX_HOST_PORT'):
docker_sandbox_kwargs['host_port'] = int(
os.environ['SANDBOX_HOST_PORT']
)
if os.getenv('SANDBOX_CONTAINER_URL_PATTERN'):
docker_sandbox_kwargs['container_url_pattern'] = os.environ[
'SANDBOX_CONTAINER_URL_PATTERN'
]
config.sandbox = DockerSandboxServiceInjector(**docker_sandbox_kwargs)
if config.sandbox_spec is None:
if os.getenv('RUNTIME') == 'remote':

View File

@@ -78,6 +78,7 @@ class DockerSandboxService(SandboxService):
health_check_path: str | None
httpx_client: httpx.AsyncClient
max_num_sandboxes: int
extra_hosts: dict[str, str] = field(default_factory=dict)
docker_client: docker.DockerClient = field(default_factory=get_docker_client)
def _find_unused_port(self) -> int:
@@ -349,6 +350,9 @@ class DockerSandboxService(SandboxService):
# Use Docker's tini init process to ensure proper signal handling and reaping of
# zombie child processes.
init=True,
# Allow agent-server containers to resolve host.docker.internal
# and other custom hostnames for LAN deployments
extra_hosts=self.extra_hosts if self.extra_hosts else None,
)
sandbox_info = await self._container_to_sandbox_info(container)
@@ -422,8 +426,23 @@ class DockerSandboxService(SandboxService):
class DockerSandboxServiceInjector(SandboxServiceInjector):
"""Dependency injector for docker sandbox services."""
container_url_pattern: str = 'http://localhost:{port}'
host_port: int = 3000
container_url_pattern: str = Field(
default='http://localhost:{port}',
description=(
'URL pattern for exposed sandbox ports. Use {port} as placeholder. '
'For remote access, set to your server IP (e.g., http://192.168.1.100:{port}). '
'Configure via OH_SANDBOX_CONTAINER_URL_PATTERN environment variable.'
),
)
host_port: int = Field(
default=3000,
description=(
'The port on which the main OpenHands app server is running. '
'Used for webhook callbacks from agent-server containers. '
'If running OpenHands on a non-default port, set this to match. '
'Configure via OH_SANDBOX_HOST_PORT environment variable.'
),
)
container_name_prefix: str = 'oh-agent-server-'
max_num_sandboxes: int = Field(
default=5,
@@ -469,6 +488,15 @@ class DockerSandboxServiceInjector(SandboxServiceInjector):
'determine whether the server is running'
),
)
extra_hosts: dict[str, str] = Field(
default_factory=lambda: {'host.docker.internal': 'host-gateway'},
description=(
'Extra hostname mappings to add to agent-server containers. '
'This allows containers to resolve hostnames like host.docker.internal '
'for LAN deployments and MCP connections. '
'Format: {"hostname": "ip_or_gateway"}'
),
)
async def inject(
self, state: InjectorState, request: Request | None = None
@@ -493,4 +521,5 @@ class DockerSandboxServiceInjector(SandboxServiceInjector):
health_check_path=self.health_check_path,
httpx_client=httpx_client,
max_num_sandboxes=self.max_num_sandboxes,
extra_hosts=self.extra_hosts,
)

View File

@@ -552,11 +552,11 @@ def get_uvicorn_json_log_config() -> dict:
},
# Actual JSON formatters used by handlers below
'json': {
'()': 'pythonjsonlogger.jsonlogger.JsonFormatter',
'()': 'pythonjsonlogger.json.JsonFormatter',
'fmt': '%(message)s %(levelname)s %(name)s %(asctime)s %(exc_info)s',
},
'json_access': {
'()': 'pythonjsonlogger.jsonlogger.JsonFormatter',
'()': 'pythonjsonlogger.json.JsonFormatter',
'fmt': '%(message)s %(levelname)s %(name)s %(asctime)s %(client_addr)s %(request_line)s %(status_code)s',
},
},

View File

@@ -22,7 +22,7 @@ import base64
from typing import Any
import docx
import PyPDF2
import pypdf
from pptx import Presentation
from pylatexenc.latex2text import LatexNodes2Text
@@ -42,7 +42,7 @@ def parse_pdf(file_path: str) -> None:
file_path: str: The path to the file to open.
"""
print(f'[Reading PDF file from {file_path}]')
content = PyPDF2.PdfReader(file_path)
content = pypdf.PdfReader(file_path)
text = ''
for page_idx in range(len(content.pages)):
text += (

2
poetry.lock generated
View File

@@ -16824,4 +16824,4 @@ third-party-runtimes = ["daytona", "e2b-code-interpreter", "modal", "runloop-api
[metadata]
lock-version = "2.1"
python-versions = "^3.12,<3.14"
content-hash = "9360db8d9ee46922f780ac13e2954c0b62166efd9c3d1b3cf61a9228889152fa"
content-hash = "ea3a3dcacf87517954778e7b04f0a5865bf213442a7bdbc4f2dc467713dbf82f"

View File

@@ -77,7 +77,6 @@ shellingham = "^1.5.4"
# TODO: Should these go into the runtime group?
ipywidgets = "^8.1.5"
qtconsole = "^5.6.1"
PyPDF2 = "*"
python-pptx = "*"
pylatexenc = "*"
python-docx = "*"

View File

@@ -444,6 +444,138 @@ class TestDockerSandboxService:
):
await service.start_sandbox()
@patch('openhands.app_server.sandbox.docker_sandbox_service.base62.encodebytes')
@patch('os.urandom')
async def test_start_sandbox_with_extra_hosts(
self,
mock_urandom,
mock_encodebytes,
mock_sandbox_spec_service,
mock_httpx_client,
mock_docker_client,
):
"""Test that extra_hosts are passed to container creation."""
# Setup
mock_urandom.side_effect = [b'container_id', b'session_key']
mock_encodebytes.side_effect = ['test_container_id', 'test_session_key']
mock_container = MagicMock()
mock_container.name = 'oh-test-test_container_id'
mock_container.status = 'running'
mock_container.image.tags = ['test-image:latest']
mock_container.attrs = {
'Created': '2024-01-15T10:30:00.000000000Z',
'Config': {
'Env': ['OH_SESSION_API_KEYS_0=test_session_key', 'TEST_VAR=test_value']
},
'NetworkSettings': {'Ports': {}},
}
mock_docker_client.containers.run.return_value = mock_container
# Create service with extra_hosts
service_with_extra_hosts = DockerSandboxService(
sandbox_spec_service=mock_sandbox_spec_service,
container_name_prefix='oh-test-',
host_port=3000,
container_url_pattern='http://localhost:{port}',
mounts=[],
exposed_ports=[
ExposedPort(
name=AGENT_SERVER, description='Agent server', container_port=8000
),
],
health_check_path='/health',
httpx_client=mock_httpx_client,
max_num_sandboxes=3,
extra_hosts={
'host.docker.internal': 'host-gateway',
'custom.host': '192.168.1.100',
},
docker_client=mock_docker_client,
)
with (
patch.object(
service_with_extra_hosts, '_find_unused_port', return_value=12345
),
patch.object(
service_with_extra_hosts, 'pause_old_sandboxes', return_value=[]
),
):
# Execute
await service_with_extra_hosts.start_sandbox()
# Verify extra_hosts was passed to container creation
mock_docker_client.containers.run.assert_called_once()
call_args = mock_docker_client.containers.run.call_args
assert call_args[1]['extra_hosts'] == {
'host.docker.internal': 'host-gateway',
'custom.host': '192.168.1.100',
}
@patch('openhands.app_server.sandbox.docker_sandbox_service.base62.encodebytes')
@patch('os.urandom')
async def test_start_sandbox_without_extra_hosts(
self,
mock_urandom,
mock_encodebytes,
mock_sandbox_spec_service,
mock_httpx_client,
mock_docker_client,
):
"""Test that extra_hosts is None when not configured."""
# Setup
mock_urandom.side_effect = [b'container_id', b'session_key']
mock_encodebytes.side_effect = ['test_container_id', 'test_session_key']
mock_container = MagicMock()
mock_container.name = 'oh-test-test_container_id'
mock_container.status = 'running'
mock_container.image.tags = ['test-image:latest']
mock_container.attrs = {
'Created': '2024-01-15T10:30:00.000000000Z',
'Config': {
'Env': ['OH_SESSION_API_KEYS_0=test_session_key', 'TEST_VAR=test_value']
},
'NetworkSettings': {'Ports': {}},
}
mock_docker_client.containers.run.return_value = mock_container
# Create service without extra_hosts (empty dict)
service_without_extra_hosts = DockerSandboxService(
sandbox_spec_service=mock_sandbox_spec_service,
container_name_prefix='oh-test-',
host_port=3000,
container_url_pattern='http://localhost:{port}',
mounts=[],
exposed_ports=[
ExposedPort(
name=AGENT_SERVER, description='Agent server', container_port=8000
),
],
health_check_path='/health',
httpx_client=mock_httpx_client,
max_num_sandboxes=3,
extra_hosts={},
docker_client=mock_docker_client,
)
with (
patch.object(
service_without_extra_hosts, '_find_unused_port', return_value=12345
),
patch.object(
service_without_extra_hosts, 'pause_old_sandboxes', return_value=[]
),
):
# Execute
await service_without_extra_hosts.start_sandbox()
# Verify extra_hosts is None when empty dict is provided
mock_docker_client.containers.run.assert_called_once()
call_args = mock_docker_client.containers.run.call_args
assert call_args[1]['extra_hosts'] is None
async def test_resume_sandbox_from_paused(self, service):
"""Test resuming a paused sandbox."""
# Setup
@@ -841,3 +973,116 @@ class TestExposedPort:
port = ExposedPort(name='test', description='Test port')
with pytest.raises(ValueError): # Should raise validation error
port.name = 'new_name'
class TestDockerSandboxServiceInjector:
"""Test cases for DockerSandboxServiceInjector configuration."""
def test_default_values(self):
"""Test default configuration values."""
from openhands.app_server.sandbox.docker_sandbox_service import (
DockerSandboxServiceInjector,
)
injector = DockerSandboxServiceInjector()
assert injector.host_port == 3000
assert injector.container_url_pattern == 'http://localhost:{port}'
def test_custom_host_port(self):
"""Test custom host_port configuration."""
from openhands.app_server.sandbox.docker_sandbox_service import (
DockerSandboxServiceInjector,
)
injector = DockerSandboxServiceInjector(host_port=4000)
assert injector.host_port == 4000
def test_custom_container_url_pattern(self):
"""Test custom container_url_pattern configuration."""
from openhands.app_server.sandbox.docker_sandbox_service import (
DockerSandboxServiceInjector,
)
injector = DockerSandboxServiceInjector(
container_url_pattern='http://192.168.1.100:{port}'
)
assert injector.container_url_pattern == 'http://192.168.1.100:{port}'
def test_custom_configuration_combined(self):
"""Test combined custom configuration for remote access."""
from openhands.app_server.sandbox.docker_sandbox_service import (
DockerSandboxServiceInjector,
)
injector = DockerSandboxServiceInjector(
host_port=4000,
container_url_pattern='http://192.168.1.100:{port}',
)
assert injector.host_port == 4000
assert injector.container_url_pattern == 'http://192.168.1.100:{port}'
class TestDockerSandboxServiceInjectorFromEnv:
"""Test cases for DockerSandboxServiceInjector environment variable configuration."""
def test_config_from_env_with_sandbox_host_port(self):
"""Test that SANDBOX_HOST_PORT environment variable is respected."""
import os
from unittest.mock import patch
env_vars = {
'SANDBOX_HOST_PORT': '4000',
}
with patch.dict(os.environ, env_vars, clear=False):
# Clear the global config to force reload
import openhands.app_server.config as config_module
from openhands.app_server.config import config_from_env
config_module._global_config = None
config = config_from_env()
assert config.sandbox is not None
assert config.sandbox.host_port == 4000
def test_config_from_env_with_sandbox_container_url_pattern(self):
"""Test that SANDBOX_CONTAINER_URL_PATTERN environment variable is respected."""
import os
from unittest.mock import patch
env_vars = {
'SANDBOX_CONTAINER_URL_PATTERN': 'http://192.168.1.100:{port}',
}
with patch.dict(os.environ, env_vars, clear=False):
# Clear the global config to force reload
import openhands.app_server.config as config_module
from openhands.app_server.config import config_from_env
config_module._global_config = None
config = config_from_env()
assert config.sandbox is not None
assert config.sandbox.container_url_pattern == 'http://192.168.1.100:{port}'
def test_config_from_env_with_both_sandbox_vars(self):
"""Test that both SANDBOX_HOST_PORT and SANDBOX_CONTAINER_URL_PATTERN work together."""
import os
from unittest.mock import patch
env_vars = {
'SANDBOX_HOST_PORT': '4000',
'SANDBOX_CONTAINER_URL_PATTERN': 'http://192.168.1.100:{port}',
}
with patch.dict(os.environ, env_vars, clear=False):
# Clear the global config to force reload
import openhands.app_server.config as config_module
from openhands.app_server.config import config_from_env
config_module._global_config = None
config = config_from_env()
assert config.sandbox is not None
assert config.sandbox.host_port == 4000
assert config.sandbox.container_url_pattern == 'http://192.168.1.100:{port}'