From d099c21f5d1ec06560f8a5cbb0ebe47fa8ce3e66 Mon Sep 17 00:00:00 2001 From: Alona Date: Tue, 30 Sep 2025 17:20:48 -0500 Subject: [PATCH] fix: add provider tokens to resume conversation endpoint (#11155) --- openhands/integrations/provider.py | 20 ++- .../server/routes/manage_conversations.py | 20 ++- .../server/services/conversation_service.py | 33 +++- .../unit/server/routes/test_start_endpoint.py | 150 ++++++++++++++++ .../services/test_conversation_service.py | 162 ++++++++++++++++++ 5 files changed, 374 insertions(+), 11 deletions(-) create mode 100644 tests/unit/server/routes/test_start_endpoint.py create mode 100644 tests/unit/server/services/test_conversation_service.py diff --git a/openhands/integrations/provider.py b/openhands/integrations/provider.py index f4a26d8a3f..df15895d06 100644 --- a/openhands/integrations/provider.py +++ b/openhands/integrations/provider.py @@ -466,10 +466,22 @@ class ProviderHandler: except Exception as e: errors.append(f'{provider.value}: {str(e)}') - # Log all accumulated errors before raising AuthenticationError - logger.error( - f'Failed to access repository {repository} with all available providers. Errors: {"; ".join(errors)}' - ) + # Log detailed error based on whether we had tokens or not + if not self.provider_tokens: + logger.error( + f'Failed to access repository {repository}: No provider tokens available. ' + f'provider_tokens dict is empty.' + ) + elif errors: + logger.error( + f'Failed to access repository {repository} with all available providers. ' + f'Tried providers: {list(self.provider_tokens.keys())}. ' + f'Errors: {"; ".join(errors)}' + ) + else: + logger.error( + f'Failed to access repository {repository}: Unknown error (no providers tried, no errors recorded)' + ) raise AuthenticationError(f'Unable to access repo {repository}') async def get_branches( diff --git a/openhands/server/routes/manage_conversations.py b/openhands/server/routes/manage_conversations.py index 08271a6152..84868f34cf 100644 --- a/openhands/server/routes/manage_conversations.py +++ b/openhands/server/routes/manage_conversations.py @@ -462,6 +462,7 @@ async def start_conversation( providers_set: ProvidersSetModel, conversation_id: str = Depends(validate_conversation_id), user_id: str = Depends(get_user_id), + provider_tokens: PROVIDER_TOKEN_TYPE = Depends(get_provider_tokens), settings: Settings = Depends(get_user_settings), conversation_store: ConversationStore = Depends(get_conversation_store), ) -> ConversationResponse: @@ -471,7 +472,22 @@ async def start_conversation( to start a conversation. If the conversation is already running, it will return the existing agent loop info. """ - logger.info(f'Starting conversation: {conversation_id}') + logger.info( + f'Starting conversation: {conversation_id}', + extra={'session_id': conversation_id}, + ) + + # Log token fetch status + if provider_tokens: + logger.info( + f'/start endpoint: Fetched provider tokens: {list(provider_tokens.keys())}', + extra={'session_id': conversation_id}, + ) + else: + logger.warning( + '/start endpoint: No provider tokens fetched (provider_tokens is None/empty)', + extra={'session_id': conversation_id}, + ) try: # Check that the conversation exists @@ -488,7 +504,7 @@ async def start_conversation( # Set up conversation init data with provider information conversation_init_data = await setup_init_conversation_settings( - user_id, conversation_id, providers_set.providers_set or [] + user_id, conversation_id, providers_set.providers_set or [], provider_tokens ) # Start the agent loop diff --git a/openhands/server/services/conversation_service.py b/openhands/server/services/conversation_service.py index 9adfe8849e..093bff3c09 100644 --- a/openhands/server/services/conversation_service.py +++ b/openhands/server/services/conversation_service.py @@ -215,7 +215,10 @@ def create_provider_tokens_object( async def setup_init_conversation_settings( - user_id: str | None, conversation_id: str, providers_set: list[ProviderType] + user_id: str | None, + conversation_id: str, + providers_set: list[ProviderType], + provider_tokens: PROVIDER_TOKEN_TYPE | None = None, ) -> ConversationInitData: """Set up conversation initialization data with provider tokens. @@ -223,6 +226,7 @@ async def setup_init_conversation_settings( user_id: The user ID conversation_id: The conversation ID providers_set: List of provider types to set up tokens for + provider_tokens: Optional provider tokens to use (for SAAS mode resume) Returns: ConversationInitData with provider tokens configured @@ -243,11 +247,30 @@ async def setup_init_conversation_settings( session_init_args: dict = {} session_init_args = {**settings.__dict__, **session_init_args} - git_provider_tokens = create_provider_tokens_object(providers_set) - logger.info(f'Git provider scaffold: {git_provider_tokens}') + # Use provided tokens if available (for SAAS resume), otherwise create scaffold + if provider_tokens: + logger.info( + f'Using provided provider_tokens: {list(provider_tokens.keys())}', + extra={'session_id': conversation_id}, + ) + git_provider_tokens = provider_tokens + else: + logger.info( + f'No provider_tokens provided, creating scaffold for: {providers_set}', + extra={'session_id': conversation_id}, + ) + git_provider_tokens = create_provider_tokens_object(providers_set) + logger.info( + f'Git provider scaffold: {git_provider_tokens}', + extra={'session_id': conversation_id}, + ) - if server_config.app_mode != AppMode.SAAS and user_secrets: - git_provider_tokens = user_secrets.provider_tokens + if server_config.app_mode != AppMode.SAAS and user_secrets: + logger.info( + f'Non-SaaS mode: Overriding with user_secrets provider tokens: {list(user_secrets.provider_tokens.keys())}', + extra={'session_id': conversation_id}, + ) + git_provider_tokens = user_secrets.provider_tokens session_init_args['git_provider_tokens'] = git_provider_tokens if user_secrets: diff --git a/tests/unit/server/routes/test_start_endpoint.py b/tests/unit/server/routes/test_start_endpoint.py new file mode 100644 index 0000000000..3581523445 --- /dev/null +++ b/tests/unit/server/routes/test_start_endpoint.py @@ -0,0 +1,150 @@ +"""Tests for /start endpoint provider token handling.""" + +from types import MappingProxyType +from unittest.mock import AsyncMock, patch + +import pytest +from pydantic import SecretStr + +from openhands.integrations.provider import ProviderToken, ProviderType +from openhands.server.data_models.agent_loop_info import AgentLoopInfo +from openhands.server.routes.manage_conversations import ( + ProvidersSetModel, + start_conversation, +) +from openhands.server.types import AppMode +from openhands.storage.data_models.conversation_metadata import ConversationMetadata +from openhands.storage.data_models.conversation_status import ConversationStatus +from openhands.storage.data_models.settings import Settings + + +@pytest.fixture +def mock_settings(): + """Create a real Settings object with minimal required fields.""" + return Settings( + language='en', + agent='CodeActAgent', + max_iterations=100, + llm_model='anthropic/claude-3-5-sonnet-20241022', + llm_api_key=SecretStr('test_api_key_12345'), + llm_base_url=None, + ) + + +@pytest.fixture +def mock_provider_tokens(): + """Create real provider tokens to test with.""" + return MappingProxyType( + { + ProviderType.GITHUB: ProviderToken( + token=SecretStr('ghp_real_token_test123'), user_id='test_user_456' + ) + } + ) + + +@pytest.fixture +def mock_conversation_metadata(): + """Create a real ConversationMetadata object.""" + return ConversationMetadata( + conversation_id='test_conv_123', + user_id='test_user_456', + title='Test Conversation', + selected_repository='test/repo', + selected_branch='main', + git_provider=ProviderType.GITHUB, + ) + + +@pytest.mark.asyncio +async def test_start_endpoint_passes_provider_tokens( + mock_settings, mock_provider_tokens, mock_conversation_metadata +): + """Test that /start endpoint passes provider_tokens to setup_init_conversation_settings. + + This test verifies the full end-to-end flow with real tokens through to ConversationInitData. + """ + conversation_id = 'test_conv_123' + user_id = 'test_user_456' + providers_set = ProvidersSetModel(providers_set=[ProviderType.GITHUB]) + + # Mock conversation store + mock_conversation_store = AsyncMock() + mock_conversation_store.get_metadata = AsyncMock( + return_value=mock_conversation_metadata + ) + + # Mock agent loop info that will be returned + mock_agent_loop_info = AgentLoopInfo( + conversation_id=conversation_id, + url=None, + session_api_key=None, + event_store=None, + status=ConversationStatus.RUNNING, + ) + + # Mock only infrastructure - let setup_init_conversation_settings run for real + with patch( + 'openhands.server.routes.manage_conversations.conversation_manager' + ) as mock_manager: + # Mock the stores that setup_init_conversation_settings needs + with patch( + 'openhands.server.services.conversation_service.SettingsStoreImpl.get_instance' + ) as mock_settings_store_cls: + with patch( + 'openhands.server.services.conversation_service.SecretsStoreImpl.get_instance' + ) as mock_secrets_store_cls: + with patch( + 'openhands.server.services.conversation_service.server_config' + ) as mock_server_config: + # Setup store mocks + mock_settings_store = AsyncMock() + mock_settings_store.load = AsyncMock(return_value=mock_settings) + mock_settings_store_cls.return_value = mock_settings_store + + mock_secrets_store = AsyncMock() + mock_secrets_store.load = AsyncMock(return_value=None) + mock_secrets_store_cls.return_value = mock_secrets_store + + mock_server_config.app_mode = AppMode.SAAS + + mock_manager.maybe_start_agent_loop = AsyncMock( + return_value=mock_agent_loop_info + ) + + # Call endpoint with provider tokens + response = await start_conversation( + providers_set=providers_set, + conversation_id=conversation_id, + user_id=user_id, + provider_tokens=mock_provider_tokens, + settings=mock_settings, + conversation_store=mock_conversation_store, + ) + + # Verify ConversationInitData has real provider tokens + mock_manager.maybe_start_agent_loop.assert_called_once() + call_kwargs = mock_manager.maybe_start_agent_loop.call_args[1] + conversation_init_data = call_kwargs['settings'] + + assert conversation_init_data.git_provider_tokens is not None + assert ( + conversation_init_data.git_provider_tokens + == mock_provider_tokens + ) + assert ( + ProviderType.GITHUB + in conversation_init_data.git_provider_tokens + ) + + github_token = conversation_init_data.git_provider_tokens[ + ProviderType.GITHUB + ] + assert ( + github_token.token.get_secret_value() + == 'ghp_real_token_test123' + ) + assert github_token.user_id == 'test_user_456' + + assert response.status == 'ok' + assert response.conversation_id == conversation_id diff --git a/tests/unit/server/services/test_conversation_service.py b/tests/unit/server/services/test_conversation_service.py new file mode 100644 index 0000000000..be247390ac --- /dev/null +++ b/tests/unit/server/services/test_conversation_service.py @@ -0,0 +1,162 @@ +"""Unit tests for conversation_service.py - specifically testing provider token handling. + +These tests verify that setup_init_conversation_settings correctly handles provider tokens +in different scenarios (provided tokens vs scaffold creation). +""" + +from types import MappingProxyType +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from pydantic import SecretStr + +from openhands.integrations.provider import ProviderToken, ProviderType +from openhands.server.services.conversation_service import ( + setup_init_conversation_settings, +) +from openhands.server.types import AppMode +from openhands.storage.data_models.settings import Settings + + +@pytest.fixture +def mock_settings(): + """Create a real Settings object with minimal required fields.""" + return Settings( + language='en', + agent='CodeActAgent', + max_iterations=100, + llm_model='anthropic/claude-3-5-sonnet-20241022', + llm_api_key=SecretStr('test_api_key_12345'), + llm_base_url=None, + ) + + +@pytest.fixture +def mock_provider_tokens(): + """Create real provider tokens to test with.""" + return MappingProxyType( + { + ProviderType.GITHUB: ProviderToken( + token=SecretStr('ghp_real_token_test123'), user_id='test_user_456' + ) + } + ) + + +@pytest.mark.asyncio +async def test_setup_with_provided_tokens_uses_real_tokens( + mock_settings, mock_provider_tokens +): + """Test that real tokens are used when provided in SAAS mode. + + Verifies provider tokens passed in are used in ConversationInitData. + """ + user_id = 'test_user_456' + conversation_id = 'test_conv_123' + providers_set = [ProviderType.GITHUB] + + # Mock the stores to return our test settings + with patch( + 'openhands.server.services.conversation_service.SettingsStoreImpl.get_instance' + ) as mock_settings_store_cls: + with patch( + 'openhands.server.services.conversation_service.SecretsStoreImpl.get_instance' + ) as mock_secrets_store_cls: + with patch( + 'openhands.server.services.conversation_service.server_config' + ) as mock_server_config: + # Setup mocks + mock_settings_store = AsyncMock() + mock_settings_store.load = AsyncMock(return_value=mock_settings) + mock_settings_store_cls.return_value = mock_settings_store + + mock_secrets_store = AsyncMock() + mock_secrets_store.load = AsyncMock(return_value=None) + mock_secrets_store_cls.return_value = mock_secrets_store + + mock_server_config.app_mode = AppMode.SAAS + + # Call with real tokens + result = await setup_init_conversation_settings( + user_id=user_id, + conversation_id=conversation_id, + providers_set=providers_set, + provider_tokens=mock_provider_tokens, + ) + + # Verify real tokens are used + assert result.git_provider_tokens is not None + assert result.git_provider_tokens == mock_provider_tokens + assert ProviderType.GITHUB in result.git_provider_tokens, ( + 'GitHub provider should be in tokens' + ) + + github_token = result.git_provider_tokens[ProviderType.GITHUB] + assert ( + github_token.token.get_secret_value() == 'ghp_real_token_test123' + ), 'Should use real token, not None' + assert github_token.user_id == 'test_user_456', ( + 'Should preserve user_id from real token' + ) + + +@pytest.mark.asyncio +async def test_setup_without_tokens_non_saas_uses_user_secrets(mock_settings): + """Test that OSS mode uses user_secrets.provider_tokens when no tokens provided. + + This test verifies OSS mode backward compatibility - tokens come from local config, not endpoint. + """ + user_id = 'test_user_456' + conversation_id = 'test_conv_123' + providers_set = [ProviderType.GITHUB] + + # Create user_secrets with real tokens + mock_user_secrets = MagicMock() + mock_user_secrets.provider_tokens = MappingProxyType( + { + ProviderType.GITHUB: ProviderToken( + token=SecretStr('ghp_local_token_from_config'), + user_id='local_user_123', + ) + } + ) + mock_user_secrets.custom_secrets = MappingProxyType({}) # Empty dict is fine + + with patch( + 'openhands.server.services.conversation_service.SettingsStoreImpl.get_instance' + ) as mock_settings_store_cls: + with patch( + 'openhands.server.services.conversation_service.SecretsStoreImpl.get_instance' + ) as mock_secrets_store_cls: + with patch( + 'openhands.server.services.conversation_service.server_config' + ) as mock_server_config: + # Setup mocks + mock_settings_store = AsyncMock() + mock_settings_store.load = AsyncMock(return_value=mock_settings) + mock_settings_store_cls.return_value = mock_settings_store + + mock_secrets_store = AsyncMock() + mock_secrets_store.load = AsyncMock(return_value=mock_user_secrets) + mock_secrets_store_cls.return_value = mock_secrets_store + + mock_server_config.app_mode = AppMode.OSS + + # Call without endpoint tokens + result = await setup_init_conversation_settings( + user_id=user_id, + conversation_id=conversation_id, + providers_set=providers_set, + provider_tokens=None, + ) + + # Verify user_secrets tokens are used + assert result.git_provider_tokens is not None + assert ProviderType.GITHUB in result.git_provider_tokens + + github_token = result.git_provider_tokens[ProviderType.GITHUB] + assert ( + github_token.token.get_secret_value() + == 'ghp_local_token_from_config' + ) + assert github_token.user_id == 'local_user_123'