Skip to content

feat: paginate list past the API's page cap - #108

Merged
sanbotto merged 7 commits into
mainfrom
feat/TECH-77-enable-pagination
Aug 20, 2026
Merged

feat: paginate list past the API's page cap#108
sanbotto merged 7 commits into
mainfrom
feat/TECH-77-enable-pagination

Conversation

@sanbotto

Copy link
Copy Markdown
Contributor

What this changes

kh workflow list made exactly one request to GET /api/workflows, so any org or project with more than the API's 200-result-per-request cap silently lost everything past that point, and a --limit above 200 just failed with an opaque invalid_input error. Reading the KeeperHub API source directly (app/api/workflows/route.ts) confirmed it already supports real offset-based pagination (&offset=, with a short page as the authoritative end-of-list signal) and a structured error envelope carrying detail, hint, and request_id alongside the bare error code, none of which the CLI was using.

kh workflow list now pages through the endpoint internally via &offset= until --limit is satisfied or the API reports the list is exhausted, so --limit above 200 no longer errors; it just costs more requests. A new --all flag drops the default --limit of 30 and fetches every matching workflow, and can be combined with --project/--tag; an explicit --limit passed alongside --all still bounds the result to that count, same as without --all. When the loop stops because --limit was hit rather than because the list ended, a cheap one-row follow-up probe confirms whether more results actually exist, and only then prints a note to stderr, not a guess from a full page. Separately, internal/http/errors.go now reads detail, hint, and request_id from the API's error envelope and includes them in the surfaced error message, so a 400 like limit must be <= 200 is visible instead of collapsing to the bare invalid_input code; this also changes the text of every CLI error that hits this path, not just workflow list's.

Scope

These two pieces are independently revertible: list.go's pagination has no functional dependency on errors.go's change, and vice versa. They're bundled into one PR because both came out of the same investigation into one set of downstream bug reports (a CLI silently truncating workflow lists and swallowing the API's error detail), and splitting them would mean re-deriving that shared context across two reviews for two small fixes. Happy to split into separate PRs if preferred.

How it was verified

Added cmd/workflow/list_test.go cases covering: pagination past the 200 cap to satisfy a larger --limit; the "more workflows exist" note firing only when a follow-up probe confirms it (and staying silent when --limit exactly covers everything, which would false-positive on a naive "full page" heuristic); --all paginating to exhaustion; --all ignoring the default --limit of 30 while still respecting an explicit one (a bug caught during development, where the default silently re-truncated --all's merged result); and --all combined with --project. Added internal/http/errors_test.go cases covering detail/hint/request_id extraction and the fallback to the bare error field when detail is absent. Each of these fails against the pre-change code.

Also verified live against KeeperHub's staging environment: confirmed via curl that offset=1&limit=2 returns the correct second workflow, that kh workflow list --limit 2 emits the "more workflows exist" note against real data, that kh workflow list --all aggregates every workflow in a live org, and fed the exact 400 body staging returns for an over-200 request through the real NewAPIError code path to confirm it renders as HTTP 400: invalid_input: limit must be <= 200 (request_id: ...). No server-side changes were needed or made; both issues were purely in how the CLI used API behavior that already existed.


  • make lint and make test passed.
  • go generate ./docs/ ran; it applied no updates.

@sanbotto
sanbotto requested a review from a team August 20, 2026 14:55
@sanbotto sanbotto self-assigned this Aug 20, 2026
@sanbotto
sanbotto requested review from OleksandrUA, eskp, joelorzet and suisuss and removed request for a team August 20, 2026 14:55
@sanbotto sanbotto added the no-issue-required PR exempt from the issue-first gate label Aug 20, 2026

@joelorzet joelorzet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pagination loop and the error envelope extraction both look right, and the tests cover the cases that matter. Two things need to change before this lands, plus one smaller item.

The clamp in fetchWorkflowPage needs to go. It is currently unreachable, and if it ever becomes reachable it silently truncates the list. Details inline.

--limit 0 and negative limits now mean "fetch everything". limit > 0 is the unlimited sentinel in fetchWorkflows, and nothing validates the flag, so kh wf ls --limit 0 and kh wf ls --limit -5 page through the entire org. Before this PR both were a 400 from the API (limit must be an integer >= 1). A typo turning into a full org scan is a bad default. Reject a limit below 1, or treat it as the flag default.

Help text nit. List with a higher limit (paginates internally past 200 if needed) and "it fetches as many 200-result pages as needed" are not quite what the code does: the final page requests only the remainder, not 200. Harmless, but it is stated as fact in the generated docs/kh_workflow_list.md too.

One thing I checked and it is fine: the field names in errors.go match the envelope in the API repo (error, detail, hint, request_id). It ignores docs, which is optional in the envelope, so skipping it is correct.

Comment thread cmd/workflow/list.go
Comment thread cmd/workflow/list.go Outdated
Comment thread cmd/workflow/list.go

@joelorzet joelorzet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three items are addressed, and the fixes are correct rather than cosmetic.

Clamp removal (b2234e4): fetchWorkflowPage sends limit unchanged now, and the replacement doc comment states the caller contract and the reason for it. Both call sites still satisfy that contract: the loop passes min(maxListPageSize, remaining), the probe passes 1. An over-cap limit can now only come from a future caller, and it surfaces as the API's 400 rather than a silent truncation.

Source-of-truth note (98e5e83): accurate, and it names both drift directions correctly.

Limit validation (400d685): placed before any request goes out, matches how the other commands reject bad flags, and --all without an explicit limit still reaches the unlimited path because validation runs against the flag value rather than the effective one. The test asserting no request is sent for 0 and -5 is the right assertion.

make lint is clean, go test ./... passes, and go generate ./docs/ produces no diff.

One cosmetic item left inline, not worth holding the PR for.

Two things I checked on this pass that are fine: --all on a large org means many sequential requests, but the shared client already retries 429 with backoff, so it will not fall over; and while the errors.go change alters the text of every CLI error on that path, nothing in the codebase branches on APIError.Message or string-matches err.Error() other than a context deadline exceeded check in doctor.

Comment thread cmd/workflow/list.go Outdated
@sanbotto
sanbotto merged commit 99d9d03 into main Aug 20, 2026
4 checks passed
@sanbotto
sanbotto deleted the feat/TECH-77-enable-pagination branch August 20, 2026 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-issue-required PR exempt from the issue-first gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants