From 1b57fd4d1e252a987ce1c626d4e63b3f8ddc7f8a Mon Sep 17 00:00:00 2001 From: Robert Brennan Date: Wed, 14 May 2025 10:17:35 -0400 Subject: [PATCH] Remove github_user_id in favor of user_id (#8406) Co-authored-by: openhands --- .../conversation_manager.py | 2 -- .../standalone_conversation_manager.py | 26 +++++-------------- openhands/server/listen_socket.py | 3 +-- .../server/routes/manage_conversations.py | 1 - openhands/server/user_auth/__init__.py | 11 -------- .../conversation/conversation_store.py | 7 ++--- .../conversation/conversation_validator.py | 4 +-- .../conversation/file_conversation_store.py | 8 +++--- .../data_models/conversation_metadata.py | 1 - tests/unit/test_auto_generate_title.py | 5 +--- tests/unit/test_conversation.py | 4 --- tests/unit/test_file_conversation_store.py | 10 ------- .../test_standalone_conversation_manager.py | 5 +--- 13 files changed, 18 insertions(+), 69 deletions(-) diff --git a/openhands/server/conversation_manager/conversation_manager.py b/openhands/server/conversation_manager/conversation_manager.py index 6f7e5716e8..ef6b4a6593 100644 --- a/openhands/server/conversation_manager/conversation_manager.py +++ b/openhands/server/conversation_manager/conversation_manager.py @@ -53,7 +53,6 @@ class ConversationManager(ABC): connection_id: str, settings: Settings, user_id: str | None, - github_user_id: str | None, ) -> EventStore | None: """Join a conversation and return its event stream.""" @@ -82,7 +81,6 @@ class ConversationManager(ABC): user_id: str | None, initial_user_msg: MessageAction | None = None, replay_json: str | None = None, - github_user_id: str | None = None, ) -> EventStore: """Start an event loop if one is not already running""" diff --git a/openhands/server/conversation_manager/standalone_conversation_manager.py b/openhands/server/conversation_manager/standalone_conversation_manager.py index 32485b650d..0309767ee2 100644 --- a/openhands/server/conversation_manager/standalone_conversation_manager.py +++ b/openhands/server/conversation_manager/standalone_conversation_manager.py @@ -119,7 +119,6 @@ class StandaloneConversationManager(ConversationManager): connection_id: str, settings: Settings, user_id: str | None, - github_user_id: str | None, ) -> EventStore: logger.info( f'join_conversation:{sid}:{connection_id}', @@ -127,9 +126,7 @@ class StandaloneConversationManager(ConversationManager): ) await self.sio.enter_room(connection_id, ROOM_KEY.format(sid=sid)) self._local_connection_id_to_session_id[connection_id] = sid - event_stream = await self.maybe_start_agent_loop( - sid, settings, user_id, github_user_id=github_user_id - ) + event_stream = await self.maybe_start_agent_loop(sid, settings, user_id) if not event_stream: logger.error( f'No event stream after joining conversation: {sid}', @@ -197,9 +194,7 @@ class StandaloneConversationManager(ConversationManager): logger.error('error_cleaning_stale') await asyncio.sleep(_CLEANUP_INTERVAL) - async def _get_conversation_store( - self, user_id: str | None, github_user_id: str | None - ) -> ConversationStore: + async def _get_conversation_store(self, user_id: str | None) -> ConversationStore: conversation_store_class = self._conversation_store_class if not conversation_store_class: self._conversation_store_class = conversation_store_class = get_impl( @@ -256,12 +251,11 @@ class StandaloneConversationManager(ConversationManager): user_id: str | None, initial_user_msg: MessageAction | None = None, replay_json: str | None = None, - github_user_id: str | None = None, ) -> EventStore: logger.info(f'maybe_start_agent_loop:{sid}', extra={'session_id': sid}) if not await self.is_agent_loop_running(sid): await self._start_agent_loop( - sid, settings, user_id, initial_user_msg, replay_json, github_user_id + sid, settings, user_id, initial_user_msg, replay_json ) event_store = await self._get_event_store(sid, user_id) @@ -280,7 +274,6 @@ class StandaloneConversationManager(ConversationManager): user_id: str | None, initial_user_msg: MessageAction | None = None, replay_json: str | None = None, - github_user_id: str | None = None, ) -> Session: logger.info(f'starting_agent_loop:{sid}', extra={'session_id': sid}) @@ -291,9 +284,7 @@ class StandaloneConversationManager(ConversationManager): extra={'session_id': sid, 'user_id': user_id}, ) # Get the conversations sorted (oldest first) - conversation_store = await self._get_conversation_store( - user_id, github_user_id - ) + conversation_store = await self._get_conversation_store(user_id) conversations = await conversation_store.get_all_metadata(response_ids) conversations.sort(key=_last_updated_at_key, reverse=True) @@ -332,9 +323,7 @@ class StandaloneConversationManager(ConversationManager): try: session.agent_session.event_stream.subscribe( EventStreamSubscriber.SERVER, - self._create_conversation_update_callback( - user_id, github_user_id, sid, settings - ), + self._create_conversation_update_callback(user_id, sid, settings), UPDATED_AT_CALLBACK_ID, ) except ValueError: @@ -433,7 +422,6 @@ class StandaloneConversationManager(ConversationManager): def _create_conversation_update_callback( self, user_id: str | None, - github_user_id: str | None, conversation_id: str, settings: Settings, ) -> Callable: @@ -442,7 +430,6 @@ class StandaloneConversationManager(ConversationManager): self._update_conversation_for_event, GENERAL_TIMEOUT, user_id, - github_user_id, conversation_id, settings, event, @@ -453,12 +440,11 @@ class StandaloneConversationManager(ConversationManager): async def _update_conversation_for_event( self, user_id: str, - github_user_id: str, conversation_id: str, settings: Settings, event=None, ): - conversation_store = await self._get_conversation_store(user_id, github_user_id) + conversation_store = await self._get_conversation_store(user_id) conversation = await conversation_store.get_metadata(conversation_id) conversation.last_updated_at = datetime.now(timezone.utc) diff --git a/openhands/server/listen_socket.py b/openhands/server/listen_socket.py index fe0414bfcc..d72e36c344 100644 --- a/openhands/server/listen_socket.py +++ b/openhands/server/listen_socket.py @@ -77,7 +77,7 @@ async def connect(connection_id: str, environ: dict) -> None: # Headers in WSGI/ASGI are prefixed with 'HTTP_' and have dashes replaced with underscores authorization_header = environ.get('HTTP_AUTHORIZATION', None) conversation_validator = create_conversation_validator() - user_id, github_user_id = await conversation_validator.validate( + user_id = await conversation_validator.validate( conversation_id, cookies_str, authorization_header ) @@ -108,7 +108,6 @@ async def connect(connection_id: str, environ: dict) -> None: connection_id, conversation_init_data, user_id, - github_user_id, ) logger.info( f'Connected to conversation {conversation_id} with connection_id {connection_id}. Replaying event stream...' diff --git a/openhands/server/routes/manage_conversations.py b/openhands/server/routes/manage_conversations.py index 06922f0f6e..6627a8e7f2 100644 --- a/openhands/server/routes/manage_conversations.py +++ b/openhands/server/routes/manage_conversations.py @@ -129,7 +129,6 @@ async def _create_new_conversation( conversation_id=conversation_id, title=conversation_title, user_id=user_id, - github_user_id=None, selected_repository=selected_repository, selected_branch=selected_branch, ) diff --git a/openhands/server/user_auth/__init__.py b/openhands/server/user_auth/__init__.py index fd51cd5660..b87b864580 100644 --- a/openhands/server/user_auth/__init__.py +++ b/openhands/server/user_auth/__init__.py @@ -2,7 +2,6 @@ from fastapi import Request from pydantic import SecretStr from openhands.integrations.provider import PROVIDER_TOKEN_TYPE -from openhands.integrations.service_types import ProviderType from openhands.server.settings import Settings from openhands.server.user_auth.user_auth import AuthType, get_user_auth from openhands.storage.data_models.user_secrets import UserSecrets @@ -28,16 +27,6 @@ async def get_user_id(request: Request) -> str | None: return user_id -async def get_github_user_id(request: Request) -> str | None: - provider_tokens = await get_provider_tokens(request) - if not provider_tokens: - return None - github_provider = provider_tokens.get(ProviderType.GITHUB) - if github_provider: - return github_provider.user_id - return None - - async def get_user_settings(request: Request) -> Settings | None: user_auth = await get_user_auth(request) user_settings = await user_auth.get_user_settings() diff --git a/openhands/storage/conversation/conversation_store.py b/openhands/storage/conversation/conversation_store.py index 29efd30c61..859084ee66 100644 --- a/openhands/storage/conversation/conversation_store.py +++ b/openhands/storage/conversation/conversation_store.py @@ -23,14 +23,11 @@ class ConversationStore(ABC): """Load conversation metadata.""" async def validate_metadata( - self, conversation_id: str, user_id: str, github_user_id: str + self, conversation_id: str, user_id: str ) -> bool: """Validate that conversation belongs to the current user.""" - # TODO: remove github_user_id after transition to Keycloak is complete. metadata = await self.get_metadata(conversation_id) - if (not metadata.user_id and not metadata.github_user_id) or ( - metadata.user_id != user_id and metadata.github_user_id != github_user_id - ): + if not metadata.user_id or metadata.user_id != user_id: return False else: return True diff --git a/openhands/storage/conversation/conversation_validator.py b/openhands/storage/conversation/conversation_validator.py index 7d00f2b3f7..a6399089da 100644 --- a/openhands/storage/conversation/conversation_validator.py +++ b/openhands/storage/conversation/conversation_validator.py @@ -11,8 +11,8 @@ class ConversationValidator: conversation_id: str, cookies_str: str, authorization_header: str | None = None, - ) -> tuple[None, None]: - return None, None + ) -> str | None: + return None def create_conversation_validator() -> ConversationValidator: diff --git a/openhands/storage/conversation/file_conversation_store.py b/openhands/storage/conversation/file_conversation_store.py index 8de6dc6746..c8ac1c6578 100644 --- a/openhands/storage/conversation/file_conversation_store.py +++ b/openhands/storage/conversation/file_conversation_store.py @@ -38,12 +38,14 @@ class FileConversationStore(ConversationStore): path = self.get_conversation_metadata_filename(conversation_id) json_str = await call_sync_from_async(self.file_store.read, path) - # Temp: force int to str to stop pydandic being, well... pedantic + # Validate the JSON json_obj = json.loads(json_str) if 'created_at' not in json_obj: raise FileNotFoundError(path) - if isinstance(json_obj.get('github_user_id'), int): - json_obj['github_user_id'] = str(json_obj.get('github_user_id')) + + # Remove github_user_id if it exists + if 'github_user_id' in json_obj: + json_obj.pop('github_user_id') result = conversation_metadata_type_adapter.validate_python(json_obj) return result diff --git a/openhands/storage/data_models/conversation_metadata.py b/openhands/storage/data_models/conversation_metadata.py index 18d84c076c..dbf2fee629 100644 --- a/openhands/storage/data_models/conversation_metadata.py +++ b/openhands/storage/data_models/conversation_metadata.py @@ -14,7 +14,6 @@ class ConversationTrigger(Enum): @dataclass class ConversationMetadata: conversation_id: str - github_user_id: str | None selected_repository: str | None user_id: str | None = None selected_branch: str | None = None diff --git a/tests/unit/test_auto_generate_title.py b/tests/unit/test_auto_generate_title.py index 271b5cb561..dd7bbe5ca6 100644 --- a/tests/unit/test_auto_generate_title.py +++ b/tests/unit/test_auto_generate_title.py @@ -190,7 +190,6 @@ async def test_update_conversation_with_title(): # Create test conversation conversation_id = 'test-conversation' user_id = 'test-user' - github_user_id = 'test-github-user' # Create test settings settings = Settings( @@ -223,9 +222,7 @@ async def test_update_conversation_with_title(): AsyncMock(return_value='Generated Title'), ): # Call the method - await manager._update_conversation_for_event( - user_id, github_user_id, conversation_id, settings - ) + await manager._update_conversation_for_event(user_id, conversation_id, settings) # Verify the title was updated assert mock_metadata.title == 'Generated Title' diff --git a/tests/unit/test_conversation.py b/tests/unit/test_conversation.py index 4f70b748a6..268b5e22da 100644 --- a/tests/unit/test_conversation.py +++ b/tests/unit/test_conversation.py @@ -48,7 +48,6 @@ def _patch_store(): 'title': 'Some Conversation', 'selected_repository': 'foobar', 'conversation_id': 'some_conversation_id', - 'github_user_id': '12345', 'user_id': '12345', 'created_at': '2025-01-01T00:00:00+00:00', 'last_updated_at': '2025-01-01T00:01:00+00:00', @@ -139,7 +138,6 @@ async def test_search_conversations(): '2025-01-01T00:01:00+00:00' ), selected_repository='foobar', - github_user_id='12345', user_id='12345', ) ] @@ -185,7 +183,6 @@ async def test_get_conversation(): created_at=datetime.fromisoformat('2025-01-01T00:00:00+00:00'), last_updated_at=datetime.fromisoformat('2025-01-01T00:01:00+00:00'), selected_repository='foobar', - github_user_id='12345', user_id='12345', ) ) @@ -409,7 +406,6 @@ async def test_delete_conversation(): created_at=datetime.fromisoformat('2025-01-01T00:00:00+00:00'), last_updated_at=datetime.fromisoformat('2025-01-01T00:01:00+00:00'), selected_repository='foobar', - github_user_id='12345', user_id='12345', ) ) diff --git a/tests/unit/test_file_conversation_store.py b/tests/unit/test_file_conversation_store.py index a0d9ab694d..c7daadae6b 100644 --- a/tests/unit/test_file_conversation_store.py +++ b/tests/unit/test_file_conversation_store.py @@ -14,7 +14,6 @@ async def test_load_store(): expected = ConversationMetadata( conversation_id='some-conversation-id', user_id='some-user-id', - github_user_id='12345', selected_repository='some-repo', title="Let's talk about trains", ) @@ -31,7 +30,6 @@ async def test_load_int_user_id(): get_conversation_metadata_filename('some-conversation-id'): json.dumps( { 'conversation_id': 'some-conversation-id', - 'github_user_id': 12345, 'user_id': '67890', 'selected_repository': 'some-repo', 'title': "Let's talk about trains", @@ -42,7 +40,6 @@ async def test_load_int_user_id(): ) ) found = await store.get_metadata('some-conversation-id') - assert found.github_user_id == '12345' assert found.user_id == '67890' @@ -63,7 +60,6 @@ async def test_search_basic(): get_conversation_metadata_filename('conv1'): json.dumps( { 'conversation_id': 'conv1', - 'github_user_id': '123', 'user_id': '123', 'selected_repository': 'repo1', 'title': 'First conversation', @@ -73,7 +69,6 @@ async def test_search_basic(): get_conversation_metadata_filename('conv2'): json.dumps( { 'conversation_id': 'conv2', - 'github_user_id': '123', 'user_id': '123', 'selected_repository': 'repo1', 'title': 'Second conversation', @@ -83,7 +78,6 @@ async def test_search_basic(): get_conversation_metadata_filename('conv3'): json.dumps( { 'conversation_id': 'conv3', - 'github_user_id': '123', 'user_id': '123', 'selected_repository': 'repo1', 'title': 'Third conversation', @@ -112,7 +106,6 @@ async def test_search_pagination(): get_conversation_metadata_filename(f'conv{i}'): json.dumps( { 'conversation_id': f'conv{i}', - 'github_user_id': '123', 'user_id': '123', 'selected_repository': 'repo1', 'title': f'Conversation {i}', @@ -154,7 +147,6 @@ async def test_search_with_invalid_conversation(): get_conversation_metadata_filename('conv1'): json.dumps( { 'conversation_id': 'conv1', - 'github_user_id': '123', 'user_id': '123', 'selected_repository': 'repo1', 'title': 'Valid conversation', @@ -183,7 +175,6 @@ async def test_get_all_metadata(): get_conversation_metadata_filename('conv1'): json.dumps( { 'conversation_id': 'conv1', - 'github_user_id': '123', 'user_id': '123', 'selected_repository': 'repo1', 'title': 'First conversation', @@ -193,7 +184,6 @@ async def test_get_all_metadata(): get_conversation_metadata_filename('conv2'): json.dumps( { 'conversation_id': 'conv2', - 'github_user_id': '123', 'user_id': '123', 'selected_repository': 'repo1', 'title': 'Second conversation', diff --git a/tests/unit/test_standalone_conversation_manager.py b/tests/unit/test_standalone_conversation_manager.py index 255137d3cd..7174c6ae3a 100644 --- a/tests/unit/test_standalone_conversation_manager.py +++ b/tests/unit/test_standalone_conversation_manager.py @@ -74,7 +74,6 @@ async def test_init_new_local_session(): 'new-session-id', ConversationInitData(), 1, - '12345', ) assert session_instance.initialize_agent.call_count == 1 assert sio.enter_room.await_count == 1 @@ -119,14 +118,12 @@ async def test_join_local_session(): 'new-session-id', ConversationInitData(), None, - '12345', ) await conversation_manager.join_conversation( 'new-session-id', 'new-session-id', ConversationInitData(), None, - '12345', ) assert session_instance.initialize_agent.call_count == 1 assert sio.enter_room.await_count == 2 @@ -159,7 +156,7 @@ async def test_add_to_local_event_stream(): 'new-session-id', ConversationInitData(), 1 ) await conversation_manager.join_conversation( - 'new-session-id', 'connection-id', ConversationInitData(), 1, '12345' + 'new-session-id', 'connection-id', ConversationInitData(), 1 ) await conversation_manager.send_to_event_stream( 'connection-id', {'event_type': 'some_event'}