Repository navigation
fix(agent-server): enforce the paginated search limit - #4991
Conversation
…rting
The four paginated search endpoints declared their page-size bound as
`Query(gt=0, lte=100)`. `lte` is not a FastAPI/Pydantic constraint keyword
(`le` is) and an unknown key does not raise — it rides along into the JSON
schema and is never enforced. `limit=101` therefore passed validation and
reached a bare `assert limit <= 100` in the handler, which surfaced as an
unhandled AssertionError: HTTP 500 for what is a client error.
Two consequences: the cap never appeared in the OpenAPI document, so a
generated client could not see it, and exceeding it looked like a server
fault rather than a 422.
Use `le=100` and drop the now-redundant asserts. The bound is unchanged, so
requests that worked before still work; oversized ones now get 422 instead
of 500.
An existing test asserted the AssertionError, so it is updated to expect
the 422.
Affected: conversations/search, conversations/{id}/events/search,
bash/bash_events/search, file/search_subdirs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
enyst
left a comment
There was a problem hiding this comment.
@mkuri thank you for the contribution! Looks good to me actually, sorry for reading through when it’s still a draft! Do you want it out of draft? You may want to fill in the “HUMAN:” section as well with your short description or whatever you’d like to tell :)
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
…imit test The search_subdirs route lives on file_discovery_router, not file_router. The test previously imported file_router, so /api/file/search_subdirs was absent from its OpenAPI schema and the schema assertion raised KeyError. Co-authored-by: openhands <openhands@all-hands.dev>
|
Fixed the failing Root cause: the new test Fix: import
This comment was created by an AI agent (OpenHands) on behalf of the user. |
Co-authored-by: Engel Nyst <engel.nyst@gmail.com> Co-authored-by: openhands <openhands@all-hands.dev> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…uter Upstream OpenHands#4991's test targets file_discovery_router, a discovery surface the fork does not have; the fork serves /file/search_subdirs from file_router. Co-authored-by: openhands <openhands@all-hands.dev>
HUMAN:
This PR proposes a fix for a typo in endpoint definitions (
leinstead oflte), which will allow to enforce limits for search queriesAGENT:
Why
The four paginated search endpoints declare their page-size bound as
Query(gt=0, lte=100).lteis not a FastAPI/Pydantic constraint keyword — the correct spelling isle— and an unknown key does not raise. It rides along into the JSON schema and is never enforced, solimit=101passes validation and reaches the handler's bareassert limit <= 100, which surfaces as an unhandledAssertionError.Two consequences:
clients/typescript) cannot see it.Found while building an external scheduler on top of the Agent Server: paging events with
limit=200returned a 500 with no indication that the request was at fault.Summary
lte=100→le=100onlimitinconversation_router,event_router,bash_routerandfile_router.assert limit > 0/assert limit <= 100from those four handlers — FastAPI rejects both cases with 422 before the handler runs.tests/agent_server/test_search_limit_validation.py, covering all four endpoints: the bound must be published in the schema, and out-of-range values must be 422 rather than an exception.test_search_conversations_limit_validation, which asserted the old behaviour (pytest.raises(AssertionError)forlimit=101).The bound itself is unchanged, so every request that worked before still works. The only behaviour change is that an oversized
limitnow returns 422 instead of 500.Issue Number
Fixes #5041.
How to Test
Start the agent server from this branch and call each search endpoint:
Before (released 1.47.0, same request):
After (this branch, live server, not a test double):
The other three endpoints on the same running server:
And the published schema now carries the bound on all four:
Unit tests:
uv run pytest tests/agent_server/test_search_limit_validation.py \ tests/agent_server/test_conversation_router.py \ tests/agent_server/test_event_router.py \ tests/agent_server/test_file_router.py \ tests/agent_server/test_bash_service.py # 215 passedThe new test file fails on
main(5 of 7 cases) and passes here, so it does catch the regression it describes.make format,make lintanduv run pre-commit run --files <changed files>are clean. The one pre-commit hook that fails —check-forbidden-dynamic-attributes, flaggingstream_context.pyandllm/utils/telemetry.py— fails identically on an unmodified checkout ofmainand is unrelated to this change.Video/Screenshots
Not applicable — no GUI surface; the terminal output above is the evidence.
Design Doc
Not applicable: a one-keyword bug fix, which CONTRIBUTING.md lists as the kind of trivial PR where a design doc is noise.
Type
Notes
Behaviour is additive for well-behaved clients: the accepted range is unchanged, the schema simply becomes truthful. Any caller currently relying on a 500 for an oversized
limitwould now get a 422, which is the documented meaning of the response.A repo-wide grep found no other
lte=/gte=misuse inQuery(...); the remainingtimestamp__gte=occurrences are keyword argument names, not constraints.