github/OpenHands
1
0
Fork 0
mirror of https://github.com/OpenHands/OpenHands.git synced 2026-03-22 13:47:19 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: Revert on_conversation_update to load conversation inside method (#13368)

Browse Source 
Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
Tim O'Farrell
2026-03-12 19:08:04 -06:00
committed by GitHub
parent c1328f512d
commit 8b8ed5be96
3 changed files with 164 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

91
tests/unit/app_server/test_webhook_router_auth.py
View File

@@ -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'

141
tests/unit/app_server/test_webhook_router_parent_conversation.py
View File

@@ -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)