Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions openhands-agent-server/openhands/agent_server/bash_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,11 @@ async def search_bash_events(
] = None,
limit: Annotated[
int,
Query(title="The max number of results in the page", gt=0, lte=100),
Query(title="The max number of results in the page", gt=0, le=100),
] = 100,
bash_event_service: BashEventService = Depends(get_bash_event_service),
) -> BashEventPage:
"""Search / List bash event events"""
assert limit > 0
assert limit <= 100

return await bash_event_service.search_bash_events(
kind__eq=kind__eq,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ async def search_conversations(
] = None,
limit: Annotated[
int,
Query(title="The max number of results in the page", gt=0, lte=100),
Query(title="The max number of results in the page", gt=0, le=100),
] = 100,
status: Annotated[
ConversationExecutionStatus | None,
Expand All @@ -110,8 +110,6 @@ async def search_conversations(
conversation_service: ConversationService = Depends(get_conversation_service),
) -> ConversationPage:
"""Search / List conversations"""
assert limit > 0
assert limit <= 100
page = await conversation_service.search_conversations(
page_id, limit, status, sort_order
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ async def search_conversation_events(
] = None,
limit: Annotated[
int,
Query(title="The max number of results in the page", gt=0, lte=100),
Query(title="The max number of results in the page", gt=0, le=100),
] = 100,
kind: Annotated[
str | None,
Expand Down Expand Up @@ -101,8 +101,6 @@ async def search_conversation_events(
event_service: EventService = Depends(get_event_service),
) -> JSONResponse:
"""Search / List local events"""
assert limit > 0
assert limit <= 100

# Normalize timezone-aware datetimes to server timezone
normalized_gte = (
Expand Down
4 changes: 1 addition & 3 deletions openhands-agent-server/openhands/agent_server/file_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ async def search_subdirs(
] = None,
limit: Annotated[
int,
Query(title="The max number of results in the page", gt=0, lte=100),
Query(title="The max number of results in the page", gt=0, le=100),
] = 100,
include_hidden: Annotated[
bool,
Expand All @@ -824,8 +824,6 @@ async def search_subdirs(
the ``next_page_id`` returned by the previous page (the lowercase name of
the first item to include on the next page).
"""
assert limit > 0
assert limit <= 100

target = Path(path)
if not target.is_absolute():
Expand Down
7 changes: 3 additions & 4 deletions tests/agent_server/test_conversation_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,9 @@ def test_search_conversations_limit_validation(client, mock_conversation_service
response = client.get("/api/conversations/search", params={"limit": 0})
assert response.status_code == 422

# Test limit too high - endpoint has FastAPI validation (lte=100) and assertion
# The assertion in the endpoint will cause an AssertionError to be raised
with pytest.raises(AssertionError):
response = client.get("/api/conversations/search", params={"limit": 101})
# Test limit too high - rejected by FastAPI validation (le=100)
response = client.get("/api/conversations/search", params={"limit": 101})
assert response.status_code == 422

# Test valid limit
mock_conversation_service.search_conversations.return_value = ConversationPage(
Expand Down
85 changes: 85 additions & 0 deletions tests/agent_server/test_search_limit_validation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
"""The ``limit`` bound on every paginated search endpoint must be enforced.

These endpoints used to declare ``Query(gt=0, lte=100)``. ``lte`` is not a
FastAPI/Pydantic constraint keyword (``le`` is), so the bound was silently
dropped: it never reached the OpenAPI schema and was never validated. An
oversized ``limit`` then hit a bare ``assert`` in the handler and surfaced as an
unhandled ``AssertionError`` — HTTP 500 for what is a client error.
"""

from unittest.mock import AsyncMock
from uuid import uuid4

import pytest
from fastapi import FastAPI
from fastapi.testclient import TestClient

from openhands.agent_server.bash_router import bash_router
from openhands.agent_server.conversation_router import conversation_router
from openhands.agent_server.dependencies import get_event_service
from openhands.agent_server.event_router import event_router
from openhands.agent_server.event_service import EventService
from openhands.agent_server.file_router import file_discovery_router


SEARCH_ROUTES = [
(conversation_router, "/api/conversations/search"),
(event_router, "/api/conversations/{conversation_id}/events/search"),
(bash_router, "/api/bash/bash_events/search"),
(file_discovery_router, "/api/file/search_subdirs"),
]


def _limit_schema(router, path: str) -> dict:
app = FastAPI()
app.include_router(router, prefix="/api")
parameters = app.openapi()["paths"][path]["get"]["parameters"]
limit = next(p for p in parameters if p["name"] == "limit")
return limit["schema"]


@pytest.mark.parametrize(
"router, path", SEARCH_ROUTES, ids=[path for _, path in SEARCH_ROUTES]
)
def test_search_limit_bounds_are_published_in_the_schema(router, path):
"""The bound has to be in the schema, or generated clients cannot see it."""
schema = _limit_schema(router, path)

assert schema["maximum"] == 100
assert schema["exclusiveMinimum"] == 0
# A misspelled constraint rides along into the schema instead of raising.
assert "lte" not in schema


@pytest.mark.parametrize("limit", [0, 101])
def test_search_events_rejects_out_of_range_limit(limit):
"""An out-of-range ``limit`` is a client error, not a server fault."""
app = FastAPI()
app.include_router(event_router, prefix="/api")
service = AsyncMock(spec=EventService)
service.search_events = AsyncMock(return_value={"items": [], "next_page_id": None})
app.dependency_overrides[get_event_service] = lambda: service

with TestClient(app) as client:
response = client.get(
f"/api/conversations/{uuid4()}/events/search", params={"limit": limit}
)

assert response.status_code == 422
service.search_events.assert_not_called()


def test_search_events_accepts_the_maximum_limit():
app = FastAPI()
app.include_router(event_router, prefix="/api")
service = AsyncMock(spec=EventService)
service.search_events = AsyncMock(return_value={"items": [], "next_page_id": None})
app.dependency_overrides[get_event_service] = lambda: service

with TestClient(app) as client:
response = client.get(
f"/api/conversations/{uuid4()}/events/search", params={"limit": 100}
)

assert response.status_code == 200
service.search_events.assert_called_once()
Loading