Repository navigation
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.
|
|
||
| page_limit = limit | ||
| if limit is not None and include_root: | ||
| page_limit = limit - 1 |
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P2] Document the result-cap fixes and reserved root slot
Evidence: Identity now reserves one slot for the root, so include_root=True with limit=1 returns only the root. This PR also changes emitted-result behavior in seven other packages. Each affected package has a CHANGELOG.md, but none is updated, despite the root changelog requirement for user-visible changes.
Impact: users reading release notes will miss the corrected caps and Identity's changed root/limit behavior.
Requested change: add an Unreleased / Fixed entry to each affected package's existing changelog. Explicitly state in Identity's entry that the root counts toward the total limit and occupies the sole slot when limit=1.
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.
|
|
||
| # Continue after the filtered-out page and pass the remaining allowance. | ||
| calls = mock_client.list_images.call_args_list | ||
| assert [call.kwargs["limit"] for call in calls] == [2, 2] |
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P2] Complete the remaining pagination boundary regressions
Evidence: this new test usefully verifies continuation after a filtered-out page and local append bounding. Because the first page emits zero matches, the request limits remain [2, 2], so it does not exercise a decreasing allowance. The second page has no continuation, so it also does not prove that reaching the cap prevents another request. Compute's list_instances and list_vnic_attachments still lack committed limit regressions.
Impact: several acceptance criteria from #476 remain protected only by supplemental review probes, leaving gaps in regression protection.
Requested change: add a short-page fixture that emits some results and asserts a reduced limit on the next request, plus a response with continuation at the exact cap that must not trigger another request. Add equivalent regressions for the remaining uncovered tools, including empty-page continuation and unlimited traversal where those cases are absent.
|
|
||
| page_limit = limit | ||
| if limit is not None and include_root: | ||
| page_limit = limit - 1 |
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P2] Finish the package changelog updates
Evidence: the new Compute changelog entry addresses that package, but the existing changelogs for Instance Agent, Identity, Logging, Migration, Network Load Balancer, Registry, and Resource Search still omit this user-visible fix. Identity reserves one slot for the root and returns only the root when include_root=True and limit=1.
Impact: users of those packages will not see the corrected result caps or Identity's root/limit behavior in their release notes. The repository requires changelog entries for user-visible changes in each affected package.
Requested change: add concise Unreleased / Fixed entries to the other seven existing changelogs. In Identity, explicitly state that the root counts toward the total limit and occupies the sole slot when limit=1.
This branch has not been deployed
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: