Merged
Conversation
Member
|
Thanks @Nayana-R-Gowda — pagination 422 errors affect Admin UI usability. Targeting 1.0.0. |
Signed-off-by: NAYANA.R <nayana.r7813@gmail.com>
…outers - Revert time_buckets le= back to 100 (visualization param, not pagination) - Apply le=settings.pagination_max_page_size to llm_admin_router and llm_config_router pagination params that were missed Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Verify all modified endpoints use settings.pagination_max_page_size and that time_buckets retains its le=100 visualization bound. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
…ings - Remove unused `patch` import from test_pagination_bounds.py (ruff blocker) - Update 12 observability endpoint docstrings that still referenced old (1-100) or (5-100) ranges after le= was changed to settings.pagination_max_page_size Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
3fced92 to
4b3aeb6
Compare
crivetimihai
previously approved these changes
Mar 17, 2026
Member
crivetimihai
left a comment
There was a problem hiding this comment.
Review Summary
Rebased onto main (clean, no conflicts) and reviewed thoroughly.
Original PR (commit by @Nayana-R-Gowda)
Correctly replaces hardcoded le=100 with le=settings.pagination_max_page_size across 18 locations in admin.py and 2 in teams.py. This fixes the 422 errors when the Admin UI selects per_page=200 or 500.
Review fixes applied (3 additional commits)
-
Reverted
time_bucketsinget_latency_heatmapback tole=100— this is a heatmap visualization granularity parameter, not pagination. Usingpagination_max_page_size(configurable up to 10,000) would allow expensive histogram computations. -
Extended the fix to 4 missed pagination endpoints in
llm_admin_router.py(2 endpoints) andllm_config_router.py(2 endpoints) that still had hardcodedle=100. -
Updated 12 stale docstrings in observability endpoints that still referenced old
(1-100)/(5-100)ranges. -
Added 25 unit tests verifying every modified endpoint's
le=metadata matchessettings.pagination_max_page_size(orle=100fortime_buckets), plus 3 functional tests exercising endpoints withlimit=200.
Security / Performance
- No new attack surface. All endpoints remain behind
require_permission(). - The config bound (
ge=1, le=10000) inconfig.pyprevents unbounded values. - Database queries still apply
LIMITin SQL; only the validation cap changed.
Out of scope
- Filed #3706 for 3 unused Pydantic schemas in
schemas.pywith hardcoded bounds (dead code). - Log search / observability routers intentionally use
le=1000(different domain, not affected by this bug).
All linters pass (black, isort, ruff, flake8). All 1181 relevant tests pass.
…akes LoggingService.initialize() sets root logger to ERROR in CI. Tests using caplog.at_level(WARNING) without a logger name target the root, which gets overridden. Specify the exact logger name so caplog sets the level on the correct logger regardless of root configuration. Fixes 6 flaky CI failures in test_passthrough_headers, test_passthrough_headers_fixed, and test_websocket_transport. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
test_cache_expiry used asyncio.sleep(1.1) to wait for a 1s TTL, which is flaky under CI load. Mock time.time() and advance it deterministically instead. Also eliminates the 1.1s test runtime. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
calculus-ask
pushed a commit
to calculus-ask/mcp-context-forge
that referenced
this pull request
Mar 18, 2026
* ui-pagination-422-response Signed-off-by: NAYANA.R <nayana.r7813@gmail.com> * fix: revert non-pagination time_buckets bound and extend fix to LLM routers - Revert time_buckets le= back to 100 (visualization param, not pagination) - Apply le=settings.pagination_max_page_size to llm_admin_router and llm_config_router pagination params that were missed Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * test: add differential tests for pagination bounds (IBM#3469) Verify all modified endpoints use settings.pagination_max_page_size and that time_buckets retains its le=100 visualization bound. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: address codex review — remove unused import, update stale docstrings - Remove unused `patch` import from test_pagination_bounds.py (ruff blocker) - Update 12 observability endpoint docstrings that still referenced old (1-100) or (5-100) ranges after le= was changed to settings.pagination_max_page_size Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(tests): use explicit logger names in caplog.at_level to fix CI flakes LoggingService.initialize() sets root logger to ERROR in CI. Tests using caplog.at_level(WARNING) without a logger name target the root, which gets overridden. Specify the exact logger name so caplog sets the level on the correct logger regardless of root configuration. Fixes 6 flaky CI failures in test_passthrough_headers, test_passthrough_headers_fixed, and test_websocket_transport. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(tests): replace sleep-based cache expiry test with time mock test_cache_expiry used asyncio.sleep(1.1) to wait for a 1s TTL, which is flaky under CI load. Mock time.time() and advance it deterministically instead. Also eliminates the 1.1s test runtime. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: NAYANA.R <nayana.r7813@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: KRISHNAN, SANTHANA <sk8069@exo.att.com> Signed-off-by: calculus-ask <a.santhana.k@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: NAYANA.R nayana.r7813@gmail.com
closes #3469
💡 Fix Description
Replaced hardcoded le=100/le=100 query constraints in several admin endpoints with settings.pagination_max_page_size.
Fix pagination validation limits causing 422 errors when UI selects per_page=200 or 500.
Previously several endpoints used a hardcoded validation limit:
le=100
which conflicted with Admin UI pagination options (200/500).
This change replaces the hardcoded limits with:
le=settings.pagination_max_page_size
so the API limit is configurable and consistent across endpoints.
🧪 Verification
make lintmake test📐 MCP Compliance (if relevant)
✅ Checklist
make black isort pre-commit)