From 9686ee02f3e8525e8da062ee326f2ef139e53ca5 Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Tue, 6 Jan 2026 11:33:54 -0800 Subject: [PATCH] V1 GitHub resolver fixes (#12199) Co-authored-by: openhands --- .../integrations/github/github_manager.py | 2 +- .../github}/github_v1_callback_processor.py | 34 ++-- enterprise/integrations/github/github_view.py | 86 +++----- enterprise/integrations/utils.py | 5 +- .../test_github_v1_callback_processor.py | 186 ++++++++++-------- enterprise/tests/unit/test_github_view.py | 108 +++++----- .../app_conversation_models.py | 7 +- ...sql_app_conversation_start_task_service.py | 6 +- .../app_server/event_callback/__init__.py | 2 - .../sql_event_callback_service.py | 12 +- openhands/app_server/event_callback/util.py | 20 -- .../sandbox/remote_sandbox_service.py | 2 +- tests/unit/app_server/test_models.py | 41 ++++ 13 files changed, 267 insertions(+), 244 deletions(-) rename {openhands/app_server/event_callback => enterprise/integrations/github}/github_v1_callback_processor.py (91%) rename {tests/unit/app_server => enterprise/tests/unit/integrations/github}/test_github_v1_callback_processor.py (85%) create mode 100644 tests/unit/app_server/test_models.py diff --git a/enterprise/integrations/github/github_manager.py b/enterprise/integrations/github/github_manager.py index 1451595357..5a1258eb76 100644 --- a/enterprise/integrations/github/github_manager.py +++ b/enterprise/integrations/github/github_manager.py @@ -310,7 +310,7 @@ class GithubManager(Manager): f'[GitHub] Created conversation {conversation_id} for user {user_info.username}' ) - if not github_view.v1: + if not github_view.v1_enabled: # Create a GithubCallbackProcessor processor = GithubCallbackProcessor( github_view=github_view, diff --git a/openhands/app_server/event_callback/github_v1_callback_processor.py b/enterprise/integrations/github/github_v1_callback_processor.py similarity index 91% rename from openhands/app_server/event_callback/github_v1_callback_processor.py rename to enterprise/integrations/github/github_v1_callback_processor.py index dc48ab5dcf..ecfa62978f 100644 --- a/openhands/app_server/event_callback/github_v1_callback_processor.py +++ b/enterprise/integrations/github/github_v1_callback_processor.py @@ -1,11 +1,12 @@ import logging -import os from typing import Any from uuid import UUID import httpx from github import Auth, Github, GithubIntegration +from integrations.utils import CONVERSATION_URL, get_summary_instruction from pydantic import Field +from server.auth.constants import GITHUB_APP_CLIENT_ID, GITHUB_APP_PRIVATE_KEY from openhands.agent_server.models import AskAgentRequest, AskAgentResponse from openhands.app_server.event_callback.event_callback_models import ( @@ -20,8 +21,6 @@ from openhands.app_server.event_callback.util import ( ensure_conversation_found, ensure_running_sandbox, get_agent_server_url_from_sandbox, - get_conversation_url, - get_prompt_template, ) from openhands.sdk import Event from openhands.sdk.event import ConversationStateUpdateEvent @@ -34,7 +33,6 @@ class GithubV1CallbackProcessor(EventCallbackProcessor): github_view_data: dict[str, Any] = Field(default_factory=dict) should_request_summary: bool = Field(default=True) - should_extract: bool = Field(default=True) inline_pr_comment: bool = Field(default=False) async def __call__( @@ -64,7 +62,12 @@ class GithubV1CallbackProcessor(EventCallbackProcessor): self.should_request_summary = False try: + _logger.info(f'[GitHub V1] Requesting summary {conversation_id}') summary = await self._request_summary(conversation_id) + _logger.info( + f'[GitHub V1] Posting summary {conversation_id}', + extra={'summary': summary}, + ) await self._post_summary_to_github(summary) return EventCallbackResult( @@ -82,12 +85,12 @@ class GithubV1CallbackProcessor(EventCallbackProcessor): # Check if we have installation ID and credentials before posting if ( self.github_view_data.get('installation_id') - and os.getenv('GITHUB_APP_CLIENT_ID') - and os.getenv('GITHUB_APP_PRIVATE_KEY') + and GITHUB_APP_CLIENT_ID + and GITHUB_APP_PRIVATE_KEY ): await self._post_summary_to_github( f'OpenHands encountered an error: **{str(e)}**.\n\n' - f'[See the conversation]({get_conversation_url().format(conversation_id)})' + f'[See the conversation]({CONVERSATION_URL.format(conversation_id)})' 'for more information.' ) except Exception as post_error: @@ -115,16 +118,11 @@ class GithubV1CallbackProcessor(EventCallbackProcessor): f'Missing installation ID for GitHub payload: {self.github_view_data}' ) - github_app_client_id = os.getenv('GITHUB_APP_CLIENT_ID', '').strip() - github_app_private_key = os.getenv('GITHUB_APP_PRIVATE_KEY', '').replace( - '\\n', '\n' - ) - - if not github_app_client_id or not github_app_private_key: + if not GITHUB_APP_CLIENT_ID or not GITHUB_APP_PRIVATE_KEY: raise ValueError('GitHub App credentials are not configured') github_integration = GithubIntegration( - auth=Auth.AppAuth(github_app_client_id, github_app_private_key), + auth=Auth.AppAuth(GITHUB_APP_CLIENT_ID, GITHUB_APP_PRIVATE_KEY), ) token_data = github_integration.get_access_token(installation_id) return token_data.token @@ -274,16 +272,16 @@ class GithubV1CallbackProcessor(EventCallbackProcessor): app_conversation_info.sandbox_id, ) - assert sandbox.session_api_key is not None, ( - f'No session API key for sandbox: {sandbox.id}' - ) + assert ( + sandbox.session_api_key is not None + ), f'No session API key for sandbox: {sandbox.id}' # 3. URL + instruction agent_server_url = get_agent_server_url_from_sandbox(sandbox) agent_server_url = get_agent_server_url_from_sandbox(sandbox) # Prepare message based on agent state - message_content = get_prompt_template('summary_prompt.j2') + message_content = get_summary_instruction() # Ask the agent and return the response text return await self._ask_question( diff --git a/enterprise/integrations/github/github_view.py b/enterprise/integrations/github/github_view.py index c11eafb9ee..8a0e686f8d 100644 --- a/enterprise/integrations/github/github_view.py +++ b/enterprise/integrations/github/github_view.py @@ -140,7 +140,10 @@ class GithubIssue(ResolverViewInterface): title: str description: str previous_comments: list[Comment] - v1: bool + v1_enabled: bool + + def _get_branch_name(self) -> str | None: + return getattr(self, 'branch_name', None) async def _load_resolver_context(self): github_service = GithubServiceImpl( @@ -188,23 +191,27 @@ class GithubIssue(ResolverViewInterface): async def initialize_new_conversation(self) -> ConversationMetadata: # FIXME: Handle if initialize_conversation returns None - v1_enabled = await get_user_v1_enabled_setting(self.user_info.keycloak_user_id) - logger.info( - f'[GitHub V1]: User flag found for {self.user_info.keycloak_user_id} is {v1_enabled}' + self.v1_enabled = await get_user_v1_enabled_setting( + self.user_info.keycloak_user_id ) - if v1_enabled: + logger.info( + f'[GitHub V1]: User flag found for {self.user_info.keycloak_user_id} is {self.v1_enabled}' + ) + if self.v1_enabled: # Create dummy conversationm metadata # Don't save to conversation store # V1 conversations are stored in a separate table + self.conversation_id = uuid4().hex return ConversationMetadata( - conversation_id=uuid4().hex, selected_repository=self.full_repo_name + conversation_id=self.conversation_id, + selected_repository=self.full_repo_name, ) conversation_metadata: ConversationMetadata = await initialize_conversation( # type: ignore[assignment] user_id=self.user_info.keycloak_user_id, conversation_id=None, selected_repository=self.full_repo_name, - selected_branch=None, + selected_branch=self._get_branch_name(), conversation_trigger=ConversationTrigger.RESOLVER, git_provider=ProviderType.GITHUB, ) @@ -218,25 +225,18 @@ class GithubIssue(ResolverViewInterface): conversation_metadata: ConversationMetadata, saas_user_auth: UserAuth, ): - v1_enabled = await get_user_v1_enabled_setting(self.user_info.keycloak_user_id) logger.info( - f'[GitHub V1]: User flag found for {self.user_info.keycloak_user_id} is {v1_enabled}' - ) - if v1_enabled: - try: - # Use V1 app conversation service - await self._create_v1_conversation( - jinja_env, saas_user_auth, conversation_metadata - ) - return - - except Exception as e: - logger.warning(f'Error checking V1 settings, falling back to V0: {e}') - - # Use existing V0 conversation service - await self._create_v0_conversation( - jinja_env, git_provider_tokens, conversation_metadata + f'[GitHub V1]: User flag found for {self.user_info.keycloak_user_id} is {self.v1_enabled}' ) + if self.v1_enabled: + # Use V1 app conversation service + await self._create_v1_conversation( + jinja_env, saas_user_auth, conversation_metadata + ) + else: + await self._create_v0_conversation( + jinja_env, git_provider_tokens, conversation_metadata + ) async def _create_v0_conversation( self, @@ -294,6 +294,7 @@ class GithubIssue(ResolverViewInterface): system_message_suffix=conversation_instructions, initial_message=initial_message, selected_repository=self.full_repo_name, + selected_branch=self._get_branch_name(), git_provider=ProviderType.GITHUB, title=f'GitHub Issue #{self.issue_number}: {self.title}', trigger=ConversationTrigger.RESOLVER, @@ -318,11 +319,9 @@ class GithubIssue(ResolverViewInterface): f'Failed to start V1 conversation: {task.detail}' ) - self.v1 = True - def _create_github_v1_callback_processor(self): """Create a V1 callback processor for GitHub integration.""" - from openhands.app_server.event_callback.github_v1_callback_processor import ( + from integrations.github.github_v1_callback_processor import ( GithubV1CallbackProcessor, ) @@ -390,31 +389,6 @@ class GithubPRComment(GithubIssueComment): return user_instructions, conversation_instructions - async def initialize_new_conversation(self) -> ConversationMetadata: - v1_enabled = await get_user_v1_enabled_setting(self.user_info.keycloak_user_id) - logger.info( - f'[GitHub V1]: User flag found for {self.user_info.keycloak_user_id} is {v1_enabled}' - ) - if v1_enabled: - # Create dummy conversationm metadata - # Don't save to conversation store - # V1 conversations are stored in a separate table - return ConversationMetadata( - conversation_id=uuid4().hex, selected_repository=self.full_repo_name - ) - - conversation_metadata: ConversationMetadata = await initialize_conversation( # type: ignore[assignment] - user_id=self.user_info.keycloak_user_id, - conversation_id=None, - selected_repository=self.full_repo_name, - selected_branch=self.branch_name, - conversation_trigger=ConversationTrigger.RESOLVER, - git_provider=ProviderType.GITHUB, - ) - - self.conversation_id = conversation_metadata.conversation_id - return conversation_metadata - @dataclass class GithubInlinePRComment(GithubPRComment): @@ -830,7 +804,7 @@ class GithubFactory: title='', description='', previous_comments=[], - v1=False, + v1_enabled=False, ) elif GithubFactory.is_issue_comment(message): @@ -856,7 +830,7 @@ class GithubFactory: title='', description='', previous_comments=[], - v1=False, + v1_enabled=False, ) elif GithubFactory.is_pr_comment(message): @@ -898,7 +872,7 @@ class GithubFactory: title='', description='', previous_comments=[], - v1=False, + v1_enabled=False, ) elif GithubFactory.is_inline_pr_comment(message): @@ -932,7 +906,7 @@ class GithubFactory: title='', description='', previous_comments=[], - v1=False, + v1_enabled=False, ) else: diff --git a/enterprise/integrations/utils.py b/enterprise/integrations/utils.py index 49cc4087c2..577f819724 100644 --- a/enterprise/integrations/utils.py +++ b/enterprise/integrations/utils.py @@ -79,7 +79,10 @@ ENABLE_V1_GITHUB_RESOLVER = ( ) -OPENHANDS_RESOLVER_TEMPLATES_DIR = 'openhands/integrations/templates/resolver/' +OPENHANDS_RESOLVER_TEMPLATES_DIR = ( + os.getenv('OPENHANDS_RESOLVER_TEMPLATES_DIR') + or 'openhands/integrations/templates/resolver/' +) jinja_env = Environment(loader=FileSystemLoader(OPENHANDS_RESOLVER_TEMPLATES_DIR)) diff --git a/tests/unit/app_server/test_github_v1_callback_processor.py b/enterprise/tests/unit/integrations/github/test_github_v1_callback_processor.py similarity index 85% rename from tests/unit/app_server/test_github_v1_callback_processor.py rename to enterprise/tests/unit/integrations/github/test_github_v1_callback_processor.py index 037485e278..b9cce25e2e 100644 --- a/tests/unit/app_server/test_github_v1_callback_processor.py +++ b/enterprise/tests/unit/integrations/github/test_github_v1_callback_processor.py @@ -10,12 +10,14 @@ Covers: - Low-level helper methods """ -import os from unittest.mock import AsyncMock, MagicMock, patch from uuid import uuid4 import httpx import pytest +from integrations.github.github_v1_callback_processor import ( + GithubV1CallbackProcessor, +) from openhands.app_server.app_conversation.app_conversation_models import ( AppConversationInfo, @@ -24,9 +26,6 @@ from openhands.app_server.event_callback.event_callback_models import EventCallb from openhands.app_server.event_callback.event_callback_result_models import ( EventCallbackResultStatus, ) -from openhands.app_server.event_callback.github_v1_callback_processor import ( - GithubV1CallbackProcessor, -) from openhands.app_server.sandbox.sandbox_models import ( ExposedUrl, SandboxInfo, @@ -198,30 +197,27 @@ class TestGithubV1CallbackProcessor: # Successful paths # ------------------------------------------------------------------ # - @patch.dict( - os.environ, - { - 'GITHUB_APP_CLIENT_ID': 'test_client_id', - 'GITHUB_APP_PRIVATE_KEY': 'test_private_key', - }, + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_CLIENT_ID', + 'test_client_id', + ) + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_PRIVATE_KEY', + 'test_private_key', ) @patch('openhands.app_server.config.get_app_conversation_info_service') @patch('openhands.app_server.config.get_sandbox_service') @patch('openhands.app_server.config.get_httpx_client') - @patch( - 'openhands.app_server.event_callback.github_v1_callback_processor.get_prompt_template' - ) - @patch('openhands.app_server.event_callback.github_v1_callback_processor.Auth') - @patch( - 'openhands.app_server.event_callback.github_v1_callback_processor.GithubIntegration' - ) - @patch('openhands.app_server.event_callback.github_v1_callback_processor.Github') + @patch('integrations.github.github_v1_callback_processor.get_summary_instruction') + @patch('integrations.github.github_v1_callback_processor.Auth') + @patch('integrations.github.github_v1_callback_processor.GithubIntegration') + @patch('integrations.github.github_v1_callback_processor.Github') async def test_successful_callback_execution( self, mock_github, mock_github_integration, mock_auth, - mock_get_prompt_template, + mock_get_summary_instruction, mock_get_httpx_client, mock_get_sandbox_service, mock_get_app_conversation_info_service, @@ -242,7 +238,7 @@ class TestGithubV1CallbackProcessor: mock_sandbox_info, ) - mock_get_prompt_template.return_value = 'Please provide a summary' + mock_get_summary_instruction.return_value = 'Please provide a summary' # Auth.AppAuth mock mock_app_auth_instance = MagicMock() @@ -293,28 +289,25 @@ class TestGithubV1CallbackProcessor: assert kwargs['headers']['X-Session-API-Key'] == 'test_api_key' assert kwargs['json']['question'] == 'Please provide a summary' - @patch.dict( - os.environ, - { - 'GITHUB_APP_CLIENT_ID': 'test_client_id', - 'GITHUB_APP_PRIVATE_KEY': 'test_private_key', - }, + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_CLIENT_ID', + 'test_client_id', + ) + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_PRIVATE_KEY', + 'test_private_key', ) @patch('openhands.app_server.config.get_app_conversation_info_service') @patch('openhands.app_server.config.get_sandbox_service') @patch('openhands.app_server.config.get_httpx_client') - @patch( - 'openhands.app_server.event_callback.github_v1_callback_processor.get_prompt_template' - ) - @patch( - 'openhands.app_server.event_callback.github_v1_callback_processor.GithubIntegration' - ) - @patch('openhands.app_server.event_callback.github_v1_callback_processor.Github') + @patch('integrations.github.github_v1_callback_processor.get_summary_instruction') + @patch('integrations.github.github_v1_callback_processor.GithubIntegration') + @patch('integrations.github.github_v1_callback_processor.Github') async def test_successful_inline_pr_comment( self, mock_github, mock_github_integration, - mock_get_prompt_template, + mock_get_summary_instruction, mock_get_httpx_client, mock_get_sandbox_service, mock_get_app_conversation_info_service, @@ -334,7 +327,7 @@ class TestGithubV1CallbackProcessor: mock_sandbox_info, ) - mock_get_prompt_template.return_value = 'Please provide a summary' + mock_get_summary_instruction.return_value = 'Please provide a summary' mock_token_data = MagicMock() mock_token_data.token = 'test_access_token' @@ -367,6 +360,7 @@ class TestGithubV1CallbackProcessor: # Error paths # ------------------------------------------------------------------ # + @patch('integrations.github.github_v1_callback_processor.get_summary_instruction') @patch('openhands.app_server.config.get_httpx_client') @patch('openhands.app_server.config.get_sandbox_service') @patch('openhands.app_server.config.get_app_conversation_info_service') @@ -375,6 +369,7 @@ class TestGithubV1CallbackProcessor: mock_get_app_conversation_info_service, mock_get_sandbox_service, mock_get_httpx_client, + mock_get_summary_instruction, conversation_state_update_event, event_callback, mock_app_conversation_info, @@ -393,6 +388,8 @@ class TestGithubV1CallbackProcessor: mock_sandbox_info, ) + mock_get_summary_instruction.return_value = 'Please provide a summary' + result = await processor( conversation_id=conversation_id, callback=event_callback, @@ -403,7 +400,15 @@ class TestGithubV1CallbackProcessor: assert result.status == EventCallbackResultStatus.ERROR assert 'Missing installation ID' in result.detail - @patch.dict(os.environ, {}, clear=True) + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_CLIENT_ID', + '', + ) + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_PRIVATE_KEY', + '', + ) + @patch('integrations.github.github_v1_callback_processor.get_summary_instruction') @patch('openhands.app_server.config.get_httpx_client') @patch('openhands.app_server.config.get_sandbox_service') @patch('openhands.app_server.config.get_app_conversation_info_service') @@ -412,6 +417,7 @@ class TestGithubV1CallbackProcessor: mock_get_app_conversation_info_service, mock_get_sandbox_service, mock_get_httpx_client, + mock_get_summary_instruction, github_callback_processor, conversation_state_update_event, event_callback, @@ -428,6 +434,8 @@ class TestGithubV1CallbackProcessor: mock_sandbox_info, ) + mock_get_summary_instruction.return_value = 'Please provide a summary' + result = await github_callback_processor( conversation_id=conversation_id, callback=event_callback, @@ -438,12 +446,13 @@ class TestGithubV1CallbackProcessor: assert result.status == EventCallbackResultStatus.ERROR assert 'GitHub App credentials are not configured' in result.detail - @patch.dict( - os.environ, - { - 'GITHUB_APP_CLIENT_ID': 'test_client_id', - 'GITHUB_APP_PRIVATE_KEY': 'test_private_key', - }, + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_CLIENT_ID', + 'test_client_id', + ) + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_PRIVATE_KEY', + 'test_private_key', ) @patch('openhands.app_server.config.get_app_conversation_info_service') @patch('openhands.app_server.config.get_sandbox_service') @@ -489,22 +498,21 @@ class TestGithubV1CallbackProcessor: assert result.status == EventCallbackResultStatus.ERROR assert 'Sandbox not running' in result.detail - @patch.dict( - os.environ, - { - 'GITHUB_APP_CLIENT_ID': 'test_client_id', - 'GITHUB_APP_PRIVATE_KEY': 'test_private_key', - }, + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_CLIENT_ID', + 'test_client_id', + ) + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_PRIVATE_KEY', + 'test_private_key', ) @patch('openhands.app_server.config.get_app_conversation_info_service') @patch('openhands.app_server.config.get_sandbox_service') @patch('openhands.app_server.config.get_httpx_client') - @patch( - 'openhands.app_server.event_callback.github_v1_callback_processor.get_prompt_template' - ) + @patch('integrations.github.github_v1_callback_processor.get_summary_instruction') async def test_agent_server_http_error( self, - mock_get_prompt_template, + mock_get_summary_instruction, mock_get_httpx_client, mock_get_sandbox_service, mock_get_app_conversation_info_service, @@ -525,7 +533,7 @@ class TestGithubV1CallbackProcessor: mock_sandbox_info, ) - mock_get_prompt_template.return_value = 'Please provide a summary' + mock_get_summary_instruction.return_value = 'Please provide a summary' mock_httpx_client = mock_get_httpx_client.return_value.__aenter__.return_value mock_response = MagicMock() @@ -547,22 +555,21 @@ class TestGithubV1CallbackProcessor: assert result.status == EventCallbackResultStatus.ERROR assert 'Failed to send message to agent server' in result.detail - @patch.dict( - os.environ, - { - 'GITHUB_APP_CLIENT_ID': 'test_client_id', - 'GITHUB_APP_PRIVATE_KEY': 'test_private_key', - }, + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_CLIENT_ID', + 'test_client_id', + ) + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_PRIVATE_KEY', + 'test_private_key', ) @patch('openhands.app_server.config.get_app_conversation_info_service') @patch('openhands.app_server.config.get_sandbox_service') @patch('openhands.app_server.config.get_httpx_client') - @patch( - 'openhands.app_server.event_callback.github_v1_callback_processor.get_prompt_template' - ) + @patch('integrations.github.github_v1_callback_processor.get_summary_instruction') async def test_agent_server_timeout( self, - mock_get_prompt_template, + mock_get_summary_instruction, mock_get_httpx_client, mock_get_sandbox_service, mock_get_app_conversation_info_service, @@ -582,7 +589,7 @@ class TestGithubV1CallbackProcessor: mock_sandbox_info, ) - mock_get_prompt_template.return_value = 'Please provide a summary' + mock_get_summary_instruction.return_value = 'Please provide a summary' mock_httpx_client = mock_get_httpx_client.return_value.__aenter__.return_value mock_httpx_client.post.side_effect = httpx.TimeoutException('Request timeout') @@ -607,7 +614,14 @@ class TestGithubV1CallbackProcessor: with pytest.raises(ValueError, match='Missing installation ID'): processor._get_installation_access_token() - @patch.dict(os.environ, {}, clear=True) + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_CLIENT_ID', + '', + ) + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_PRIVATE_KEY', + '', + ) def test_get_installation_access_token_missing_credentials( self, github_callback_processor ): @@ -616,17 +630,16 @@ class TestGithubV1CallbackProcessor: ): github_callback_processor._get_installation_access_token() - @patch.dict( - os.environ, - { - 'GITHUB_APP_CLIENT_ID': 'test_client_id', - 'GITHUB_APP_PRIVATE_KEY': 'test_private_key\\nwith_newlines', - }, - ) - @patch('openhands.app_server.event_callback.github_v1_callback_processor.Auth') @patch( - 'openhands.app_server.event_callback.github_v1_callback_processor.GithubIntegration' + 'integrations.github.github_v1_callback_processor.GITHUB_APP_CLIENT_ID', + 'test_client_id', ) + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_PRIVATE_KEY', + 'test_private_key\nwith_newlines', + ) + @patch('integrations.github.github_v1_callback_processor.Auth') + @patch('integrations.github.github_v1_callback_processor.GithubIntegration') def test_get_installation_access_token_success( self, mock_github_integration, mock_auth, github_callback_processor ): @@ -649,7 +662,7 @@ class TestGithubV1CallbackProcessor: mock_github_integration.assert_called_once_with(auth=mock_app_auth_instance) mock_integration_instance.get_access_token.assert_called_once_with(12345) - @patch('openhands.app_server.event_callback.github_v1_callback_processor.Github') + @patch('integrations.github.github_v1_callback_processor.Github') async def test_post_summary_to_github_issue_comment( self, mock_github, github_callback_processor ): @@ -672,7 +685,7 @@ class TestGithubV1CallbackProcessor: mock_repo.get_issue.assert_called_once_with(number=42) mock_issue.create_comment.assert_called_once_with('Test summary') - @patch('openhands.app_server.event_callback.github_v1_callback_processor.Github') + @patch('integrations.github.github_v1_callback_processor.Github') async def test_post_summary_to_github_pr_comment( self, mock_github, github_callback_processor_inline ): @@ -708,14 +721,15 @@ class TestGithubV1CallbackProcessor: with pytest.raises(RuntimeError, match='Missing GitHub credentials'): await github_callback_processor._post_summary_to_github('Test summary') - @patch.dict( - os.environ, - { - 'GITHUB_APP_CLIENT_ID': 'test_client_id', - 'GITHUB_APP_PRIVATE_KEY': 'test_private_key', - 'WEB_HOST': 'test.example.com', - }, + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_CLIENT_ID', + 'test_client_id', ) + @patch( + 'integrations.github.github_v1_callback_processor.GITHUB_APP_PRIVATE_KEY', + 'test_private_key', + ) + @patch('integrations.github.github_v1_callback_processor.get_summary_instruction') @patch('openhands.app_server.config.get_httpx_client') @patch('openhands.app_server.config.get_sandbox_service') @patch('openhands.app_server.config.get_app_conversation_info_service') @@ -724,6 +738,7 @@ class TestGithubV1CallbackProcessor: mock_get_app_conversation_info_service, mock_get_sandbox_service, mock_get_httpx_client, + mock_get_summary_instruction, github_callback_processor, conversation_state_update_event, event_callback, @@ -741,13 +756,14 @@ class TestGithubV1CallbackProcessor: mock_sandbox_info, ) mock_httpx_client.post.side_effect = Exception('Simulated agent server error') + mock_get_summary_instruction.return_value = 'Please provide a summary' with ( patch( - 'openhands.app_server.event_callback.github_v1_callback_processor.GithubIntegration' + 'integrations.github.github_v1_callback_processor.GithubIntegration' ) as mock_github_integration, patch( - 'openhands.app_server.event_callback.github_v1_callback_processor.Github' + 'integrations.github.github_v1_callback_processor.Github' ) as mock_github, ): mock_integration = MagicMock() diff --git a/enterprise/tests/unit/test_github_view.py b/enterprise/tests/unit/test_github_view.py index 1edc46bc2a..020e266bd4 100644 --- a/enterprise/tests/unit/test_github_view.py +++ b/enterprise/tests/unit/test_github_view.py @@ -86,12 +86,12 @@ class TestGithubV1ConversationRouting(TestCase): def setUp(self): """Set up test fixtures.""" # Create a proper UserData instance instead of MagicMock - user_data = UserData( + self.user_data = UserData( user_id=123, username='testuser', keycloak_user_id='test-keycloak-id' ) # Create a mock raw_payload - raw_payload = Message( + self.raw_payload = Message( source=SourceType.GITHUB, message={ 'payload': { @@ -101,8 +101,10 @@ class TestGithubV1ConversationRouting(TestCase): }, ) - self.github_issue = GithubIssue( - user_info=user_data, + def _create_github_issue(self): + """Create a GithubIssue instance for testing.""" + return GithubIssue( + user_info=self.user_data, full_repo_name='test/repo', issue_number=123, installation_id=456, @@ -110,35 +112,72 @@ class TestGithubV1ConversationRouting(TestCase): should_extract=True, send_summary_instruction=False, is_public_repo=True, - raw_payload=raw_payload, + raw_payload=self.raw_payload, uuid='test-uuid', title='Test Issue', description='Test issue description', previous_comments=[], - v1=False, + v1_enabled=False, ) @pytest.mark.asyncio + @patch('integrations.github.github_view.initialize_conversation') @patch('integrations.github.github_view.get_user_v1_enabled_setting') + async def test_initialize_sets_v1_enabled_from_setting_when_false( + self, mock_get_v1_setting, mock_initialize_conversation + ): + """Test that initialize_new_conversation sets v1_enabled from get_user_v1_enabled_setting.""" + mock_get_v1_setting.return_value = False + mock_initialize_conversation.return_value = MagicMock( + conversation_id='new-conversation-id' + ) + + github_issue = self._create_github_issue() + await github_issue.initialize_new_conversation() + + # Verify get_user_v1_enabled_setting was called with correct user ID + mock_get_v1_setting.assert_called_once_with('test-keycloak-id') + # Verify v1_enabled was set to False + self.assertFalse(github_issue.v1_enabled) + + @pytest.mark.asyncio + @patch('integrations.github.github_view.get_user_v1_enabled_setting') + async def test_initialize_sets_v1_enabled_from_setting_when_true( + self, mock_get_v1_setting + ): + """Test that initialize_new_conversation sets v1_enabled to True when setting returns True.""" + mock_get_v1_setting.return_value = True + + github_issue = self._create_github_issue() + await github_issue.initialize_new_conversation() + + # Verify get_user_v1_enabled_setting was called with correct user ID + mock_get_v1_setting.assert_called_once_with('test-keycloak-id') + # Verify v1_enabled was set to True + self.assertTrue(github_issue.v1_enabled) + + @pytest.mark.asyncio @patch.object(GithubIssue, '_create_v0_conversation') @patch.object(GithubIssue, '_create_v1_conversation') async def test_create_new_conversation_routes_to_v0_when_disabled( - self, mock_create_v1, mock_create_v0, mock_get_v1_setting + self, mock_create_v1, mock_create_v0 ): """Test that conversation creation routes to V0 when v1_enabled is False.""" - # Mock v1_enabled as False - mock_get_v1_setting.return_value = False mock_create_v0.return_value = None mock_create_v1.return_value = None + github_issue = self._create_github_issue() + github_issue.v1_enabled = False + # Mock parameters jinja_env = MagicMock() git_provider_tokens = MagicMock() conversation_metadata = MagicMock() + saas_user_auth = MagicMock() # Call the method - await self.github_issue.create_new_conversation( - jinja_env, git_provider_tokens, conversation_metadata + await github_issue.create_new_conversation( + jinja_env, git_provider_tokens, conversation_metadata, saas_user_auth ) # Verify V0 was called and V1 was not @@ -148,62 +187,31 @@ class TestGithubV1ConversationRouting(TestCase): mock_create_v1.assert_not_called() @pytest.mark.asyncio - @patch('integrations.github.github_view.get_user_v1_enabled_setting') @patch.object(GithubIssue, '_create_v0_conversation') @patch.object(GithubIssue, '_create_v1_conversation') async def test_create_new_conversation_routes_to_v1_when_enabled( - self, mock_create_v1, mock_create_v0, mock_get_v1_setting + self, mock_create_v1, mock_create_v0 ): """Test that conversation creation routes to V1 when v1_enabled is True.""" - # Mock v1_enabled as True - mock_get_v1_setting.return_value = True mock_create_v0.return_value = None mock_create_v1.return_value = None + github_issue = self._create_github_issue() + github_issue.v1_enabled = True + # Mock parameters jinja_env = MagicMock() git_provider_tokens = MagicMock() conversation_metadata = MagicMock() + saas_user_auth = MagicMock() # Call the method - await self.github_issue.create_new_conversation( - jinja_env, git_provider_tokens, conversation_metadata + await github_issue.create_new_conversation( + jinja_env, git_provider_tokens, conversation_metadata, saas_user_auth ) # Verify V1 was called and V0 was not mock_create_v1.assert_called_once_with( - jinja_env, git_provider_tokens, conversation_metadata + jinja_env, saas_user_auth, conversation_metadata ) mock_create_v0.assert_not_called() - - @pytest.mark.asyncio - @patch('integrations.github.github_view.get_user_v1_enabled_setting') - @patch.object(GithubIssue, '_create_v0_conversation') - @patch.object(GithubIssue, '_create_v1_conversation') - async def test_create_new_conversation_fallback_on_v1_setting_error( - self, mock_create_v1, mock_create_v0, mock_get_v1_setting - ): - """Test that conversation creation falls back to V0 when _create_v1_conversation fails.""" - # Mock v1_enabled as True so V1 is attempted - mock_get_v1_setting.return_value = True - # Mock _create_v1_conversation to raise an exception - mock_create_v1.side_effect = Exception('V1 conversation creation failed') - mock_create_v0.return_value = None - - # Mock parameters - jinja_env = MagicMock() - git_provider_tokens = MagicMock() - conversation_metadata = MagicMock() - - # Call the method - await self.github_issue.create_new_conversation( - jinja_env, git_provider_tokens, conversation_metadata - ) - - # Verify V1 was attempted first, then V0 was called as fallback - mock_create_v1.assert_called_once_with( - jinja_env, git_provider_tokens, conversation_metadata - ) - mock_create_v0.assert_called_once_with( - jinja_env, git_provider_tokens, conversation_metadata - ) diff --git a/openhands/app_server/app_conversation/app_conversation_models.py b/openhands/app_server/app_conversation/app_conversation_models.py index 58a63a95d6..34d9c46031 100644 --- a/openhands/app_server/app_conversation/app_conversation_models.py +++ b/openhands/app_server/app_conversation/app_conversation_models.py @@ -14,6 +14,7 @@ from openhands.app_server.sandbox.sandbox_models import SandboxStatus from openhands.integrations.service_types import ProviderType from openhands.sdk.conversation.state import ConversationExecutionStatus from openhands.sdk.llm import MetricsSnapshot +from openhands.sdk.utils.models import OpenHandsModel from openhands.storage.data_models.conversation_metadata import ConversationTrigger @@ -91,7 +92,7 @@ class AppConversationPage(BaseModel): next_page_id: str | None = None -class AppConversationStartRequest(BaseModel): +class AppConversationStartRequest(OpenHandsModel): """Start conversation request object. Although a user can go directly to the sandbox and start conversations, they @@ -142,7 +143,7 @@ class AppConversationStartTaskSortOrder(Enum): UPDATED_AT_DESC = 'UPDATED_AT_DESC' -class AppConversationStartTask(BaseModel): +class AppConversationStartTask(OpenHandsModel): """Object describing the start process for an app conversation. Because starting an app conversation can be slow (And can involve starting a sandbox), @@ -167,7 +168,7 @@ class AppConversationStartTask(BaseModel): updated_at: datetime = Field(default_factory=utc_now) -class AppConversationStartTaskPage(BaseModel): +class AppConversationStartTaskPage(OpenHandsModel): items: list[AppConversationStartTask] next_page_id: str | None = None diff --git a/openhands/app_server/app_conversation/sql_app_conversation_start_task_service.py b/openhands/app_server/app_conversation/sql_app_conversation_start_task_service.py index 4913e795bb..b6c669149c 100644 --- a/openhands/app_server/app_conversation/sql_app_conversation_start_task_service.py +++ b/openhands/app_server/app_conversation/sql_app_conversation_start_task_service.py @@ -135,7 +135,7 @@ class SQLAppConversationStartTaskService(AppConversationStartTaskService): if has_more: rows = rows[:limit] - items = [AppConversationStartTask(**row2dict(row)) for row in rows] + items = [AppConversationStartTask.model_validate(row2dict(row)) for row in rows] # Calculate next page ID next_page_id = None @@ -196,7 +196,7 @@ class SQLAppConversationStartTaskService(AppConversationStartTaskService): # Return tasks in the same order as requested, with None for missing ones return [ ( - AppConversationStartTask(**row2dict(tasks_by_id[task_id])) + AppConversationStartTask.model_validate(row2dict(tasks_by_id[task_id])) if task_id in tasks_by_id else None ) @@ -218,7 +218,7 @@ class SQLAppConversationStartTaskService(AppConversationStartTaskService): result = await self.session.execute(query) stored_task = result.scalar_one_or_none() if stored_task: - return AppConversationStartTask(**row2dict(stored_task)) + return AppConversationStartTask.model_validate(row2dict(stored_task)) return None async def save_app_conversation_start_task( diff --git a/openhands/app_server/event_callback/__init__.py b/openhands/app_server/event_callback/__init__.py index 41be0a7320..143d5f0a00 100644 --- a/openhands/app_server/event_callback/__init__.py +++ b/openhands/app_server/event_callback/__init__.py @@ -9,7 +9,6 @@ with the discriminated union system used by Pydantic for validation. # Import base classes and processors without circular dependencies from .event_callback_models import EventCallbackProcessor, LoggingCallbackProcessor -from .github_v1_callback_processor import GithubV1CallbackProcessor # Note: SetTitleCallbackProcessor is not imported here to avoid circular imports # It will be registered when imported elsewhere in the application @@ -17,5 +16,4 @@ from .github_v1_callback_processor import GithubV1CallbackProcessor __all__ = [ 'EventCallbackProcessor', 'LoggingCallbackProcessor', - 'GithubV1CallbackProcessor', ] diff --git a/openhands/app_server/event_callback/sql_event_callback_service.py b/openhands/app_server/event_callback/sql_event_callback_service.py index c45416c37c..f191ab5e81 100644 --- a/openhands/app_server/event_callback/sql_event_callback_service.py +++ b/openhands/app_server/event_callback/sql_event_callback_service.py @@ -90,7 +90,7 @@ class SQLEventCallbackService(EventCallbackService): self.db_session.add(stored_callback) await self.db_session.commit() await self.db_session.refresh(stored_callback) - return EventCallback(**row2dict(stored_callback)) + return EventCallback.model_validate(row2dict(stored_callback)) async def get_event_callback(self, id: UUID) -> EventCallback | None: """Get a single event callback, returning None if not found.""" @@ -98,7 +98,7 @@ class SQLEventCallbackService(EventCallbackService): result = await self.db_session.execute(stmt) stored_callback = result.scalar_one_or_none() if stored_callback: - return EventCallback(**row2dict(stored_callback)) + return EventCallback.model_validate(row2dict(stored_callback)) return None async def delete_event_callback(self, id: UUID) -> bool: @@ -173,7 +173,9 @@ class SQLEventCallbackService(EventCallbackService): next_page_id = str(offset + limit) # Convert stored callbacks to domain models - callbacks = [EventCallback(**row2dict(cb)) for cb in stored_callbacks] + callbacks = [ + EventCallback.model_validate(row2dict(cb)) for cb in stored_callbacks + ] return EventCallbackPage(items=callbacks, next_page_id=next_page_id) async def save_event_callback(self, event_callback: EventCallback) -> EventCallback: @@ -202,7 +204,9 @@ class SQLEventCallbackService(EventCallbackService): result = await self.db_session.execute(query) stored_callbacks = result.scalars().all() if stored_callbacks: - callbacks = [EventCallback(**row2dict(cb)) for cb in stored_callbacks] + callbacks = [ + EventCallback.model_validate(row2dict(cb)) for cb in stored_callbacks + ] await asyncio.gather( *[ self.execute_callback(conversation_id, callback, event) diff --git a/openhands/app_server/event_callback/util.py b/openhands/app_server/event_callback/util.py index 1c9e568935..c4df9d87ca 100644 --- a/openhands/app_server/event_callback/util.py +++ b/openhands/app_server/event_callback/util.py @@ -18,15 +18,6 @@ if TYPE_CHECKING: ) -def get_conversation_url() -> str: - from openhands.app_server.config import get_global_config - - web_url = get_global_config().web_url - conversation_prefix = 'conversations/{}' - conversation_url = f'{web_url}/{conversation_prefix}' - return conversation_url - - def ensure_conversation_found( app_conversation_info: AppConversationInfo | None, conversation_id: UUID ) -> AppConversationInfo: @@ -68,14 +59,3 @@ def get_agent_server_url_from_sandbox(sandbox: SandboxInfo) -> str: ) from None return replace_localhost_hostname_for_docker(agent_server_url) - - -def get_prompt_template(template_name: str) -> str: - from jinja2 import Environment, FileSystemLoader - - jinja_env = Environment( - loader=FileSystemLoader('openhands/integrations/templates/resolver/') - ) - summary_instruction_template = jinja_env.get_template(template_name) - summary_instruction = summary_instruction_template.render() - return summary_instruction diff --git a/openhands/app_server/sandbox/remote_sandbox_service.py b/openhands/app_server/sandbox/remote_sandbox_service.py index 1606fc81ae..8150360090 100644 --- a/openhands/app_server/sandbox/remote_sandbox_service.py +++ b/openhands/app_server/sandbox/remote_sandbox_service.py @@ -790,7 +790,7 @@ class RemoteSandboxServiceInjector(SandboxServiceInjector): # This is primarily used for local development rather than production config = get_global_config() web_url = config.web_url - if web_url is None: + if web_url is None or 'localhost' in web_url: global polling_task if polling_task is None: polling_task = asyncio.create_task( diff --git a/tests/unit/app_server/test_models.py b/tests/unit/app_server/test_models.py new file mode 100644 index 0000000000..16863aca3c --- /dev/null +++ b/tests/unit/app_server/test_models.py @@ -0,0 +1,41 @@ +from unittest.mock import MagicMock +from uuid import UUID, uuid4 + +import pytest + +from openhands.app_server.app_conversation.app_conversation_models import ( + AppConversationStartRequest, +) +from openhands.app_server.event_callback.event_callback_models import ( + EventCallback, + EventCallbackProcessor, +) +from openhands.app_server.event_callback.event_callback_result_models import ( + EventCallbackResult, + EventCallbackResultStatus, +) +from openhands.sdk import Event + + +@pytest.mark.asyncio +async def test_app_conversation_start_request_polymorphism(): + class MyCallbackProcessor(EventCallbackProcessor): + async def __call__( + self, + conversation_id: UUID, + callback: EventCallback, + event: Event, + ) -> EventCallbackResult | None: + return EventCallbackResult( + status=EventCallbackResultStatus.SUCCESS, + event_callback_id=callback.id, + event_id=event.id, + conversation_id=conversation_id, + detail='Live long and prosper!', + ) + + req = AppConversationStartRequest(processors=[MyCallbackProcessor()]) + assert len(req.processors) == 1 + processor = req.processors[0] + result = await processor(uuid4(), MagicMock(id=uuid4()), MagicMock(id=str(uuid4()))) + assert result.detail == 'Live long and prosper!'