From 8b8ed5be96674f236a4f2028e26491dfa62a2d14 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Thu, 12 Mar 2026 19:08:04 -0600 Subject: [PATCH] fix: Revert on_conversation_update to load conversation inside method (#13368) Co-authored-by: openhands --- .../event_callback/webhook_router.py | 4 +- .../app_server/test_webhook_router_auth.py | 91 ++++++++++- ...test_webhook_router_parent_conversation.py | 141 +++++++++--------- 3 files changed, 164 insertions(+), 72 deletions(-) diff --git a/openhands/app_server/event_callback/webhook_router.py b/openhands/app_server/event_callback/webhook_router.py index 70f1a2a203..3b4d6a7c5e 100644 --- a/openhands/app_server/event_callback/webhook_router.py +++ b/openhands/app_server/event_callback/webhook_router.py @@ -128,10 +128,12 @@ async def valid_conversation( async def on_conversation_update( conversation_info: ConversationInfo, sandbox_info: SandboxInfo = Depends(valid_sandbox), - existing: AppConversationInfo = Depends(valid_conversation), app_conversation_info_service: AppConversationInfoService = app_conversation_info_service_dependency, ) -> Success: """Webhook callback for when a conversation starts, pauses, resumes, or deletes.""" + existing = await valid_conversation( + conversation_info.id, sandbox_info, app_conversation_info_service + ) # If the conversation is being deleted, no action is required... # Later we may consider deleting the conversation if it exists... diff --git a/tests/unit/app_server/test_webhook_router_auth.py b/tests/unit/app_server/test_webhook_router_auth.py index b2ba0a0626..3d6880b850 100644 --- a/tests/unit/app_server/test_webhook_router_auth.py +++ b/tests/unit/app_server/test_webhook_router_auth.py @@ -8,8 +8,12 @@ from unittest.mock import AsyncMock, MagicMock, patch from uuid import uuid4 import pytest -from fastapi import HTTPException, status +from fastapi import FastAPI, HTTPException, status +from fastapi.testclient import TestClient +from openhands.app_server.event_callback.webhook_router import ( + router as webhook_router, +) from openhands.app_server.event_callback.webhook_router import ( valid_conversation, valid_sandbox, @@ -531,3 +535,88 @@ class TestWebhookAuthenticationIntegration: sandbox_info=sandbox_result, app_conversation_info_service=mock_conversation_service, ) + + +class TestWebhookRouterHTTPIntegration: + """Integration tests for webhook router HTTP layer. + + These tests validate that FastAPI routing correctly extracts conversation_id + from the request body rather than requiring it as a query parameter. + """ + + def test_conversation_update_endpoint_does_not_require_query_param(self): + """Test that /webhooks/conversations endpoint accepts conversation_id in body only. + + This test validates the fix for the regression where the endpoint incorrectly + required conversation_id as a query parameter due to using Depends(valid_conversation). + + The endpoint should: + 1. Accept POST requests without any query parameters + 2. Extract conversation_id from the request body (conversation_info.id) + 3. Return 401 (not 422) when auth fails, proving the request was parsed correctly + """ + # Create a minimal FastAPI app with just the webhook router + app = FastAPI() + app.include_router(webhook_router, prefix='/api/v1') + + client = TestClient(app, raise_server_exceptions=False) + + # Create a valid request body with conversation_id in it + conversation_id = str(uuid4()) + request_body = { + 'id': conversation_id, + 'execution_status': 'running', + 'agent': { + 'llm': { + 'model': 'gpt-4', + }, + }, + 'stats': { + 'usage_to_metrics': {}, + }, + } + + # POST to /webhooks/conversations WITHOUT any query parameters + # If the old bug existed (conversation_id required as query param), + # FastAPI would return 422 Unprocessable Entity + response = client.post( + '/api/v1/webhooks/conversations', + json=request_body, + # No X-Session-API-Key header - should fail auth but NOT validation + ) + + # We expect 401 Unauthorized (missing session API key) + # NOT 422 Unprocessable Entity (which would indicate conversation_id + # was incorrectly required as a query parameter) + assert response.status_code == status.HTTP_401_UNAUTHORIZED, ( + f'Expected 401 (auth failure), got {response.status_code}. ' + f'If 422, the endpoint incorrectly requires conversation_id as query param. ' + f'Response: {response.json()}' + ) + assert response.json()['detail'] == 'X-Session-API-Key header is required' + + def test_events_endpoint_still_requires_conversation_id_in_path(self): + """Test that /webhooks/events/{conversation_id} correctly requires path param. + + This ensures we didn't accidentally break the events endpoint which legitimately + requires conversation_id as a path parameter. + """ + # Create a minimal FastAPI app with just the webhook router + app = FastAPI() + app.include_router(webhook_router, prefix='/api/v1') + + client = TestClient(app, raise_server_exceptions=False) + + conversation_id = str(uuid4()) + request_body = [] # Empty events list + + # POST to /webhooks/events/{conversation_id} with path parameter + response = client.post( + f'/api/v1/webhooks/events/{conversation_id}', + json=request_body, + # No X-Session-API-Key header - should fail auth but NOT validation + ) + + # We expect 401 Unauthorized (missing session API key) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.json()['detail'] == 'X-Session-API-Key header is required' diff --git a/tests/unit/app_server/test_webhook_router_parent_conversation.py b/tests/unit/app_server/test_webhook_router_parent_conversation.py index 23ae6a0612..4ad846f7f2 100644 --- a/tests/unit/app_server/test_webhook_router_parent_conversation.py +++ b/tests/unit/app_server/test_webhook_router_parent_conversation.py @@ -5,7 +5,7 @@ conversations are updated via the on_conversation_update webhook endpoint. """ from typing import AsyncGenerator -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch from uuid import uuid4 import pytest @@ -19,6 +19,7 @@ from openhands.app_server.app_conversation.app_conversation_models import ( from openhands.app_server.app_conversation.sql_app_conversation_info_service import ( SQLAppConversationInfoService, ) +from openhands.app_server.event_callback.webhook_router import on_conversation_update from openhands.app_server.sandbox.sandbox_models import SandboxInfo, SandboxStatus from openhands.app_server.user.specifiy_user_context import SpecifyUserContext from openhands.app_server.utils.sql_utils import Base @@ -118,9 +119,6 @@ class TestOnConversationUpdateParentConversationId: Assert: - Saved conversation retains the parent_conversation_id """ - from openhands.app_server.event_callback.webhook_router import ( - on_conversation_update, - ) # Arrange parent_id = uuid4() @@ -137,13 +135,16 @@ class TestOnConversationUpdateParentConversationId: parent_conversation_id=parent_id, ) - # Act - call on_conversation_update directly with dependencies - result = await on_conversation_update( - conversation_info=mock_conversation_info, - sandbox_info=sandbox_info, - existing=existing_conv, - app_conversation_info_service=app_conversation_info_service, - ) + # Act - call on_conversation_update directly with mocked valid_conversation + with patch( + 'openhands.app_server.event_callback.webhook_router.valid_conversation', + return_value=existing_conv, + ): + result = await on_conversation_update( + conversation_info=mock_conversation_info, + sandbox_info=sandbox_info, + app_conversation_info_service=app_conversation_info_service, + ) # Assert assert isinstance(result, Success) @@ -171,9 +172,6 @@ class TestOnConversationUpdateParentConversationId: Assert: - Saved conversation has parent_conversation_id as None """ - from openhands.app_server.event_callback.webhook_router import ( - on_conversation_update, - ) # Arrange conversation_id = mock_conversation_info.id @@ -187,13 +185,16 @@ class TestOnConversationUpdateParentConversationId: parent_conversation_id=None, ) - # Act - call on_conversation_update directly with dependencies - result = await on_conversation_update( - conversation_info=mock_conversation_info, - sandbox_info=sandbox_info, - existing=existing_conv, - app_conversation_info_service=app_conversation_info_service, - ) + # Act - call on_conversation_update directly with mocked valid_conversation + with patch( + 'openhands.app_server.event_callback.webhook_router.valid_conversation', + return_value=existing_conv, + ): + result = await on_conversation_update( + conversation_info=mock_conversation_info, + sandbox_info=sandbox_info, + app_conversation_info_service=app_conversation_info_service, + ) # Assert assert isinstance(result, Success) @@ -220,9 +221,6 @@ class TestOnConversationUpdateParentConversationId: Assert: - New conversation has parent_conversation_id as None """ - from openhands.app_server.event_callback.webhook_router import ( - on_conversation_update, - ) # Arrange conversation_id = mock_conversation_info.id @@ -234,13 +232,16 @@ class TestOnConversationUpdateParentConversationId: created_by_user_id=sandbox_info.created_by_user_id, ) - # Act - call on_conversation_update directly with dependencies - result = await on_conversation_update( - conversation_info=mock_conversation_info, - sandbox_info=sandbox_info, - existing=stub_conv, - app_conversation_info_service=app_conversation_info_service, - ) + # Act - call on_conversation_update directly with mocked valid_conversation + with patch( + 'openhands.app_server.event_callback.webhook_router.valid_conversation', + return_value=stub_conv, + ): + result = await on_conversation_update( + conversation_info=mock_conversation_info, + sandbox_info=sandbox_info, + app_conversation_info_service=app_conversation_info_service, + ) # Assert assert isinstance(result, Success) @@ -268,9 +269,6 @@ class TestOnConversationUpdateParentConversationId: Assert: - All metadata including parent_conversation_id is preserved """ - from openhands.app_server.event_callback.webhook_router import ( - on_conversation_update, - ) # Arrange parent_id = uuid4() @@ -290,13 +288,16 @@ class TestOnConversationUpdateParentConversationId: parent_conversation_id=parent_id, ) - # Act - call on_conversation_update directly with dependencies - result = await on_conversation_update( - conversation_info=mock_conversation_info, - sandbox_info=sandbox_info, - existing=existing_conv, - app_conversation_info_service=app_conversation_info_service, - ) + # Act - call on_conversation_update directly with mocked valid_conversation + with patch( + 'openhands.app_server.event_callback.webhook_router.valid_conversation', + return_value=existing_conv, + ): + result = await on_conversation_update( + conversation_info=mock_conversation_info, + sandbox_info=sandbox_info, + app_conversation_info_service=app_conversation_info_service, + ) # Assert assert isinstance(result, Success) @@ -333,9 +334,6 @@ class TestOnConversationUpdateParentConversationId: Assert: - Parent_conversation_id remains unchanged after all updates """ - from openhands.app_server.event_callback.webhook_router import ( - on_conversation_update, - ) # Arrange parent_id = uuid4() @@ -363,12 +361,15 @@ class TestOnConversationUpdateParentConversationId: else: existing = initial_conv - result = await on_conversation_update( - conversation_info=mock_conversation_info, - sandbox_info=sandbox_info, - existing=existing, - app_conversation_info_service=app_conversation_info_service, - ) + with patch( + 'openhands.app_server.event_callback.webhook_router.valid_conversation', + return_value=existing, + ): + result = await on_conversation_update( + conversation_info=mock_conversation_info, + sandbox_info=sandbox_info, + app_conversation_info_service=app_conversation_info_service, + ) assert isinstance(result, Success) # Assert @@ -396,9 +397,6 @@ class TestOnConversationUpdateParentConversationId: Assert: - Function returns early, no updates are made """ - from openhands.app_server.event_callback.webhook_router import ( - on_conversation_update, - ) # Arrange parent_id = uuid4() @@ -420,13 +418,16 @@ class TestOnConversationUpdateParentConversationId: # Set conversation to DELETING status mock_conversation_info.execution_status = ConversationExecutionStatus.DELETING - # Act - call on_conversation_update directly with dependencies - result = await on_conversation_update( - conversation_info=mock_conversation_info, - sandbox_info=sandbox_info, - existing=existing_conv, - app_conversation_info_service=app_conversation_info_service, - ) + # Act - call on_conversation_update directly with mocked valid_conversation + with patch( + 'openhands.app_server.event_callback.webhook_router.valid_conversation', + return_value=existing_conv, + ): + result = await on_conversation_update( + conversation_info=mock_conversation_info, + sandbox_info=sandbox_info, + app_conversation_info_service=app_conversation_info_service, + ) # Assert - Function returns success but doesn't update assert isinstance(result, Success) @@ -456,9 +457,6 @@ class TestOnConversationUpdateParentConversationId: Assert: - Parent_conversation_id is preserved and title is generated """ - from openhands.app_server.event_callback.webhook_router import ( - on_conversation_update, - ) # Arrange parent_id = uuid4() @@ -473,13 +471,16 @@ class TestOnConversationUpdateParentConversationId: parent_conversation_id=parent_id, ) - # Act - call on_conversation_update directly with dependencies - result = await on_conversation_update( - conversation_info=mock_conversation_info, - sandbox_info=sandbox_info, - existing=existing_conv, - app_conversation_info_service=app_conversation_info_service, - ) + # Act - call on_conversation_update directly with mocked valid_conversation + with patch( + 'openhands.app_server.event_callback.webhook_router.valid_conversation', + return_value=existing_conv, + ): + result = await on_conversation_update( + conversation_info=mock_conversation_info, + sandbox_info=sandbox_info, + app_conversation_info_service=app_conversation_info_service, + ) # Assert assert isinstance(result, Success)