Repository navigation
#476 Fix result limits across OCI MCP tools - #489
Conversation
oracle-contributor-agreement
Bot
added
the
OCA Verified
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
- Instance Agent — [P1] Update stale result-count and second-page limit assertions.
- Resource Search — [P1] Replace the test expectation that permits a limit overrun.
Additional review notes
- Compute and the remaining affected tools — [P2] Commit pagination boundary regressions.
- All eight affected packages — [P2] Add changelog entries, including Identity's root-slot behavior.
Validation
Lint and six affected server suites passed. Instance Agent and Resource Search each fail one stale test assertion, matching CI.
All eight coverage reports exceed 90%; 115 supplemental pagination and error probes passed.
See the inline comments in this review for evidence, impact, and suggested remediation.
| next_page: str = None | ||
|
|
||
| while has_next_page and (limit is None or len(images) < limit): | ||
| remaining = None if limit is None else limit - len(images) |
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P2] Commit regressions for the pagination boundaries
Evidence: the PR changes 18 loops but changes tests only in Identity. Compute's tool tests contain no limit assertions. Supplemental review probes reproduce the original five-results-for-limit=3 behavior on the base revision and pass on this head, including filtered-image pagination.
Impact: high line coverage currently allows the original total-cap defect to pass undetected, leaving these fixes vulnerable to regression.
Requested change: add package-local regression coverage for the affected tools using short pages followed by continuation, empty pages with continuation, remaining request allowances, exact-cap stopping, and unlimited traversal. Include a filtered-out image page followed by matching images and verify the emitted match cap. The regressions should fail on the original implementation.
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
No blocking changes requested. Both previous P1 test findings are fixed.
Additional review notes
- [P2] Complete the remaining pagination boundary regressions.
- [P2] Add changelog entries for the seven affected packages other than Compute.
Validation
Lint and all eight affected server suites passed: 302 tests, with each package above 90% coverage.
All 115 supplemental pagination/error probes passed. The new filtered-image regression also detects the original defect. CI is green.
See the inline comments in this review for evidence, impact, and suggested remediation.
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
No blocking changes requested. The previous P1 findings remain fixed.
Additional review notes
- [P2] Add total-cap regressions for the six remaining Logging and Resource Search tools.
- [P3] Explicitly document Identity's reserved root slot.
The expanded pagination tests and changelog entries substantially address the previous feedback.
Validation
Lint and all eight affected server suites passed: 319 tests, with every package above 90% coverage.
All 115 supplemental pagination/error probes passed, and CI is green. Original-loop controls confirmed the remaining test gaps.
See the inline comments in this review for evidence, impact, and suggested remediation.
…p + resource search tools
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
No blocking changes requested. The previous P1 findings remain fixed.
Additional review notes
- [P2] Add the remaining total-cap regression for list_resource_types.
The latest changes resolve the Logging and three Search-tool test gaps, and Identity's changelog now explicitly documents the reserved root slot.
Validation
Lint and all eight affected server suites passed: 324 tests, with every package above 90% coverage.
All 115 supplemental pagination/error probes passed, and CI is green. The five new regressions reject the original loops; the list_resource_types control confirms the remaining test gap.
See the inline comment in this review for evidence, impact, and suggested remediation.
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
No blocking changes requested.
Additional review notes
All prior review findings are addressed. The new list_resource_types regression verifies the total cap, reduced SDK request limits, page tokens, and stopping without an extra request.
No additional actionable findings after reviewing all 18 affected pagination paths.
Validation
Lint and all eight affected server suites passed: 325 tests, with every package above 90% coverage.
All 115 supplemental pagination/error probes passed. The new regression detects the original overrun, and CI is green.
Validation used mocked OCI clients; live OCI and HTTP deployment were not exercised.
2 participants
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #476
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: