Conversation
…n handling and response model validation
There was a problem hiding this comment.
Pull request overview
This PR adds optional pagination support to the framework’s script-backed plugin listing route (GET /) by threading TaskExecutionApp.pagination into derive_script_routes. Script-backed apps keep the current plain-list behavior by default, and switch to a PaginatedResponse envelope (with client-side slicing) when a pagination dependency is provided.
Changes:
- Extend
derive_script_routes(...)with an optionalpagination_depthat toggles between a plainlist[...]response and aPaginatedResponseenvelope. - Thread
TaskExecutionApp.paginationinto the script-source router build path so script-backed apps can opt in without custom wiring. - Add test coverage for both behaviors (plain list vs paginated envelope), including slicing and OpenAPI response-model shape.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/sep/apps/framework/api.py | Adds optional pagination_dep to script-route derivation and registers either a plain list handler or a paginated handler. |
| app/sep/apps/framework/apps.py | Passes self.pagination into derive_script_routes for script-backed apps. |
| tests/app/sep/apps/framework/test_api.py | Adds isolated route-introspection tests verifying response_model changes with pagination enabled/disabled. |
| tests/app/sep/apps/framework/test_script_source.py | Adds HTTP-level tests covering plain-list default behavior and paginated envelope + slicing behavior. |
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
yyyyyyyan
left a comment
There was a problem hiding this comment.
@peter-o-addo — clean, tightly-scoped work. The new paginated branch of derive_script_routes mirrors the model-first _register_list_route pagination seam closely, the unpaginated default reproduces the prior route exactly (so opt-out apps are untouched), and the slicing is correct: total is taken over the full script set before pagination.slice(...), and only the page rows are projected through source.list_response. The app-spine threading reuses the existing self.pagination field, one branch down from the model-first derive_crud_routes(..., pagination_dep=self.pagination) call — no new app-level field, exactly as the ticket asks.
Test coverage is thorough: test_paginated_list_slices_by_offset_and_limit pins offset/limit against an exact total, test_paginated_list_untyped_without_response_model covers the list_response_model is None envelope at both the router and OpenAPI layers, and test_list_returns_plain_array_without_pagination locks in the back-compatible default.
One small non-blocking nit inline — the cast in the paginated branch looks redundant, since from_pagination already returns a PaginatedResponse (unlike the model-first _list_paginated, there's nothing to narrow). Left it as a one-click suggestion.
Approving — nothing blocking.



Summary
Add optional pagination to the framework script-listing route so script-backed apps can opt into
PaginatedResponseonGET /via the existingTaskExecutionApp.paginationfield.app/sep/apps/framework/api.py,app/sep/apps/framework/apps.py: add optionalpagination_deptoderive_script_routes(plainlist[...]by default; client-sidePaginatedResponseslice when set) and threadself.paginationthrough the script-sourcebuild_routerpathtests/app/sep/apps/framework/test_api.py,tests/app/sep/apps/framework/test_script_source.py: cover paginated envelope shape, offset/limit slicing, exact total, plain-list default, and untypedPaginatedResponsecasesTested
GET http://localhost:8000/api/apps/snippets/returns200and a top-level JSON array of snippet rows (not a{ items, total, offset, limit }envelope).GET http://localhost:8000/api/apps/snippets/?offset=0&limit=1returns200with aPaginatedResponseenvelope whereitemshas one snippet,offsetis0,limitis1, andtotalequals the full discovered snippet count.GET http://localhost:8000/api/apps/snippets/?offset=1&limit=1returns200with a differentitems[0].filenamethan the previous request whiletotalstays the same andoffset/limitecho1/1.Checklist
make test)make run-pre-commit)make makemigrations)changelog.d/if the change is user-facing (make changelog-add), or confirmed N/A (internal-only change, or a same-release-cycle fix for an unreleased sibling ticket)