feat: paginate list past the API's page cap - #108
Conversation
joelorzet
left a comment
There was a problem hiding this comment.
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.
joelorzet
left a comment
There was a problem hiding this comment.
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.
What this changes
kh workflow listmade exactly one request toGET /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--limitabove 200 just failed with an opaqueinvalid_inputerror. 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 carryingdetail,hint, andrequest_idalongside the bare error code, none of which the CLI was using.kh workflow listnow pages through the endpoint internally via&offset=until--limitis satisfied or the API reports the list is exhausted, so--limitabove 200 no longer errors; it just costs more requests. A new--allflag drops the default--limitof 30 and fetches every matching workflow, and can be combined with--project/--tag; an explicit--limitpassed alongside--allstill bounds the result to that count, same as without--all. When the loop stops because--limitwas 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.gonow readsdetail,hint, andrequest_idfrom the API's error envelope and includes them in the surfaced error message, so a 400 likelimit must be <= 200is visible instead of collapsing to the bareinvalid_inputcode; this also changes the text of every CLI error that hits this path, not justworkflow list's.Scope
These two pieces are independently revertible:
list.go's pagination has no functional dependency onerrors.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.gocases 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--limitexactly covers everything, which would false-positive on a naive "full page" heuristic);--allpaginating to exhaustion;--allignoring the default--limitof 30 while still respecting an explicit one (a bug caught during development, where the default silently re-truncated--all's merged result); and--allcombined with--project. Addedinternal/http/errors_test.gocases coveringdetail/hint/request_idextraction and the fallback to the bareerrorfield whendetailis absent. Each of these fails against the pre-change code.Also verified live against KeeperHub's staging environment: confirmed via
curlthatoffset=1&limit=2returns the correct second workflow, thatkh workflow list --limit 2emits the "more workflows exist" note against real data, thatkh workflow list --allaggregates every workflow in a live org, and fed the exact 400 body staging returns for an over-200 request through the realNewAPIErrorcode path to confirm it renders asHTTP 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 lintandmake testpassed.go generate ./docs/ran; it applied no updates.