mirror of
https://github.com/OpenHands/OpenHands.git
synced 2025-12-26 05:48:36 +08:00
fix: add provider tokens to resume conversation endpoint (#11155)
This commit is contained in:
Alona
GitHub
parent
4c89b5ad91
commit
d099c21f5d
@ -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(
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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:
|
||||
|
||||
150
tests/unit/server/routes/test_start_endpoint.py
Normal file
150
tests/unit/server/routes/test_start_endpoint.py
Normal file
@ -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
|
||||
162
tests/unit/server/services/test_conversation_service.py
Normal file
162
tests/unit/server/services/test_conversation_service.py
Normal file
@ -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'
|
||||
Loading…
x
Reference in New Issue
Block a user