From 1d823b1c7957166d067091fb12f8cf0fab9fc223 Mon Sep 17 00:00:00 2001 From: Charles Kramer Date: Wed, 4 Nov 2026 15:12:34 -0700 Subject: [PATCH 1/2] fix: stop looping on routes whose pagination cursor never advances A confirmed set of public GET list routes never bind page_size/page_token server-side (the API-gateway route lacks a query: block, so the generated decoder reads neither param from the URL): /api/v1/automations, /api/v1/automation_executions, /api/v1/functions, and /api/v1/functions/{id}/commits among them. The server always answers page 1, resealing a fresh-looking nextPageToken on every call, so following the cursor returns page 1 forever instead of reaching the rest of a large result set. Live on a test tenant: with 150 automations, GET /api/v1/automations returned the same 54 rows on every call; automation_executions returned the same 30 of 34. c1i's automations list and automations executions list both looped forever; functions list and functions commits were exposed to the same defect without any guard at all. This is a server defect, reported separately. There is no client-side workaround that produces a complete result (no search/* equivalent is substituted here), so the fix is to detect the stuck cursor and fail loudly instead of hanging or silently returning an incomplete page at exit 0: - paginationCursorGuard (cmd/flags.go, from prior work) now backs automations list, automations executions list, functions list, and functions commits (and functions usage, which also reads /api/v1/automations). It flags a server that sends the same page_token twice, or a non-empty page whose every item id was already emitted under an earlier (possibly different-looking) token. - A tripped guard stops auto-pagination, keeps every row already printed (no duplicates, no re-fetching), prints a stderr warning naming the route and saying results are INCOMPLETE, and now returns a new stuckPaginationCursorError so the process exits 6 ("C1 failed") instead of silently exiting 0. - `c1i api --paginate` (the generic escape hatch, cmd/api.go) gets the same duplicate-page detection, not just its old same-token-twice check: it compares each page's items by "id" when present, falling back to an item's own canonical JSON when it has none, since `api` can target any endpoint and can't assume an id field exists. - cmd/errors.go, CLAUDE.md, .claude/commands/c1i.md, cmd/agents.md (the `c1i docs agents` source), and README.md document the new exit-6 cause and the stuckPaginationCursorError type. Tests: cmd/list_pagination_test.go's shared table gains a stuckCursorRoute field and TestListCommandsStuckCursorReturnsExitServer, covering automations executions list, functions list, functions commits, and functions usage against a server that reseals a new token over an unchanged page; cmd/automations_list_pagination_test.go's three existing stuck-cursor cases (same token, changing token, repeated empty page) are updated to assert exit 6 instead of silent exit 0; cmd/api_test.go gains three cases for `api --paginate` (same token twice, changing token same rows, and the no-"id" canonical-JSON fallback). All were confirmed to fail against the pre-fix code (an unguarded command spins past the test's request-count ceiling; `api --paginate` returns the old generic exit 1) and pass after. Verified live on a test tenant (read-only except ~100 disabled draft automations created and deleted for the reproduction, per the test instructions): with 102 automations, automations list stops at 56 rows, warns, and exits 6; automations executions list stops at 30 of 34 (cf. POST /api/v1/search/automation_executions, which returns all 34); `api --paginate --path /api/v1/automations` shows the same behavior generically. Only the tenant's 2 real automations remain afterward. --- .claude/commands/c1i.md | 5 +- CHANGELOG.md | 18 ++ CLAUDE.md | 11 +- README.md | 17 +- cmd/agents.md | 2 +- cmd/api.go | 50 +++++- cmd/api_test.go | 152 +++++++++++++++++ cmd/automations_executions_list.go | 23 ++- cmd/automations_list.go | 15 ++ cmd/automations_list_pagination_test.go | 214 ++++++++++++++++++++++++ cmd/errors.go | 24 ++- cmd/flags.go | 73 ++++++++ cmd/functions_commits.go | 16 ++ cmd/functions_list.go | 15 ++ cmd/functions_usage.go | 15 ++ cmd/list_pagination_test.go | 130 ++++++++++---- 16 files changed, 725 insertions(+), 55 deletions(-) create mode 100644 cmd/automations_list_pagination_test.go diff --git a/.claude/commands/c1i.md b/.claude/commands/c1i.md index 6ad6d83..1858f4c 100644 --- a/.claude/commands/c1i.md +++ b/.claude/commands/c1i.md @@ -82,7 +82,7 @@ request was sent. | 3 | not authenticated, or API `401`/`403` | | 4 | API `404` (not found) | | 5 | API `429` (rate limited) | -| 6 | C1 failed: API `5xx`, a `200` with a body that isn't JSON, or a redirect loop (`RedirectLoopError`) | +| 6 | C1 failed: API `5xx`, a `200` with a body that isn't JSON, a redirect loop (`RedirectLoopError`), or a list route whose pagination cursor never advances | | 7 | MCP tool call completed but the tool itself reported `isError: true` | | 8 | a system beyond C1, or the MCP protocol layer, failed (includes an unreachable gateway) | @@ -90,7 +90,8 @@ Typed internally and classified in one place, `cmd/errors.go`: from the client, `APIError`, `AuthError`, `PathError`, `RedirectError`, `RedirectLoopError`; from the gateway, `mcpgateway.TransportError` (unreachable); plus package-`cmd` wrappers `usageError`, `toolExecutionError`, `nonJSONResponseError`, -`upstreamError`, and `helperLeftError`. +`stuckPaginationCursorError` (a list route's pagination cursor never +advances), `upstreamError`, and `helperLeftError`. ## Documentation diff --git a/CHANGELOG.md b/CHANGELOG.md index 20b4082..cc8b2a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,24 @@ credentials fails instead of using the working directory. With a relative ### Fixed +- **`automations list`, `automations executions list`, `functions list`, + `functions commits`, and `functions usage` could loop forever**, and + `api --paginate` against the same routes could too: a handful of GET list + routes (`/api/v1/automations`, `/api/v1/automation_executions`, + `/api/v1/functions`, `/api/v1/functions/{id}/commits`) never bind + `page_size`/`page_token` server-side, so a large enough result set makes + the server keep resealing a fresh-looking `nextPageToken` over the exact + same first page forever — a known C1 API server defect, reported + separately, not something a c1i workaround or a different endpoint fixes. + Every affected command, and `api --paginate` generically (via item `id`, + falling back to an item's canonical JSON when it has none), now detects + the stuck cursor — the same token sent twice, or a non-empty page whose + every item was already emitted under an earlier token — stops instead of + looping, prints each row already fetched with no duplicates, warns on + stderr that results are INCOMPLETE and names the route, and exits `6` + ("C1 failed") instead of 0. `c1i docs agents`, the README, and CLAUDE.md + document the new exit-6 cause and the `stuckPaginationCursorError` type. + - **`--debug` and `--max-retries` did nothing on `docs openapi`, `docs endpoints` and `docs endpoint`.** The spec fetch now uses the shared transport, so it traces, refuses a redirect to a different path or host, and diff --git a/CLAUDE.md b/CLAUDE.md index 66c0faf..fb88bf8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -228,10 +228,11 @@ tracked root file outside its allowlist. Stage explicit paths rather than `-A`. and `client.RedirectLoopError` (a same-path chain that never settles in 5 hops); `internal/mcpgateway` adds `TransportError` for an unreachable gateway. `cmd/errors.go` maps them — plus its own - `usageError`, `toolExecutionError`, `nonJSONResponseError`, and - `upstreamError` wrappers, for failures with no natural client-level type, - and `helperLeftError` (a login's undeleted temporary credential, always 1) — - to exit codes: + `usageError`, `toolExecutionError`, `nonJSONResponseError`, + `stuckPaginationCursorError` (a list route's pagination cursor never + advances), and `upstreamError` wrappers, for failures with no natural + client-level type, and `helperLeftError` (a login's undeleted temporary + credential, always 1) — to exit codes: | Code | Meaning | |------|---------| @@ -241,7 +242,7 @@ tracked root file outside its allowlist. Stage explicit paths rather than `-A`. | `3` | auth (`401`/`403`) | | `4` | not-found (`404`) | | `5` | rate-limited (`429`) | - | `6` | C1 failed (API `5xx`, a redirect loop, or a `200` with a non-JSON body) | + | `6` | C1 failed (API `5xx`, a redirect loop, a `200` with a non-JSON body, or a list route whose pagination cursor never advances) | | `7` | tool-execution error (`mcp gateway call` result has `isError: true`) | | `8` | a system beyond C1 or the protocol layer failed (an unreachable gateway, an upstream connector failure, or a protocol-level JSON-RPC error) | diff --git a/README.md b/README.md index 2fb8785..6ee478c 100644 --- a/README.md +++ b/README.md @@ -307,6 +307,15 @@ c1i functions usage [--page-size N] [--page-token TOKEN] [--limit `functions source` auto-resolves the function's published commit (falling back to its head/latest draft) and base64-decodes the source files. Without `--out-dir`, each file is printed to stdout with a `// ===== =====` delimiter; with `--out-dir`, files are written to disk. Fetched source is developer-authored code that commonly inlines credentials, so files are written `0600` and the directory is created — or, if it already exists, tightened, never widened — to at most `0700` (owner-only; group access to the directory would buy nothing since the files inside are already unreadable to group/other, and a filename alone can be informative). Any setuid/setgid/sticky bit on the directory is stripped outright, since none of them mean anything once there is no group or other access left. A tightened pre-existing directory prints a warning to stderr naming its old mode. `functions usage` scans every automation and emits one row per step that calls the given function — useful before deleting a draft. +`functions list`, `functions commits`, and `functions usage` (which reads +`/api/v1/automations`) guard against a known C1 API server defect on their +underlying GET routes: `page_size`/`page_token` are not bound server-side, so +a server stuck on this can keep resealing a fresh-looking `nextPageToken` +over the same page forever. If that happens, the command stops instead of +looping, prints each row it already fetched with no duplicates, warns on +stderr that results are INCOMPLETE and names the route, and exits `6` ("C1 +failed") rather than 0. + ### Automations ```sh @@ -317,6 +326,10 @@ c1i automations executions list [--state done|error|pending|...] [--template-id Each `automations list` row includes `function_ids` (every distinct function the automation invokes), so `--calls-function` can answer "which automations call function X?". `executions list --state` accepts the short forms (`done`, `error`, `pending`, ...) or the full `AUTOMATION_EXECUTION_STATE_*` enum; state and template filtering are applied client-side, so pair a narrow filter with `--limit` to bound the work. +`automations list` and `automations executions list` carry the same +known-server-defect guard described under Functions above, for +`/api/v1/automations` and `/api/v1/automation_executions` respectively. + ### Policies ```sh @@ -843,7 +856,7 @@ file, or `-` for stdin) — the two are mutually exclusive. GET and DELETE refus a body by default (a body on either is more likely a mistake than intent); pass `--allow-delete-body` to lift that for DELETE specifically, for the handful of C1 endpoints that require one. `--query key=value` and -`--header key=value` are both repeatable. When `--paginate` is used, each page's first array-valued field is unwrapped and each item is emitted as a single line of NDJSON — the same format used by list commands. This covers both the canonical `list` key and typed keys like `automationExecutions`; use `--list-key ` to force a specific field. If the server returns the same `nextPageToken` twice in a row, `c1i` aborts with an error rather than looping forever. Without `--paginate`, the full JSON response is pretty-printed — and if that response carries a non-empty `nextPageToken`, `c1i` warns on stderr that the result is partial and names `--paginate`, since a truncated page is otherwise indistinguishable from a complete one at exit 0. stdout is unchanged either way. +`--header key=value` are both repeatable. When `--paginate` is used, each page's first array-valued field is unwrapped and each item is emitted as a single line of NDJSON — the same format used by list commands. This covers both the canonical `list` key and typed keys like `automationExecutions`; use `--list-key ` to force a specific field. A server whose pagination cursor never makes real progress — the exact same `nextPageToken` sent twice, or a non-empty page whose every item was already emitted under an earlier (possibly different-looking) token — is a known C1 API server defect on some routes: `c1i` stops rather than looping forever, prints each row it already fetched with no duplicates, warns on stderr that results are INCOMPLETE and names the route, and exits `6` ("C1 failed"). Without `--paginate`, the full JSON response is pretty-printed — and if that response carries a non-empty `nextPageToken`, `c1i` warns on stderr that the result is partial and names `--paginate`, since a truncated page is otherwise indistinguishable from a complete one at exit 0. stdout is unchanged either way. ### API Discovery & Documentation @@ -1010,7 +1023,7 @@ branch on without parsing text: | `3` | not authenticated, or API returned `401`/`403` | | `4` | API returned `404` (not found) | | `5` | API returned `429` (rate limited — back off and retry) | -| `6` | C1 failed: the API returned `5xx`, a redirect chain never settled, or it answered `200` with a body that isn't JSON | +| `6` | C1 failed: the API returned `5xx`, a redirect chain never settled, it answered `200` with a body that isn't JSON, or a list route's pagination cursor never advanced | | `7` | `mcp gateway call` completed, but the tool itself reported an error (`isError: true` in its result) | | `8` | a system beyond C1, or the MCP protocol layer, failed — an upstream connector was unreachable or errored, the gateway itself was unreachable (DNS failure, refused connection), or the gateway returned a protocol-level JSON-RPC error | diff --git a/cmd/agents.md b/cmd/agents.md index 96f88d7..0ff9559 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -284,7 +284,7 @@ APP_ID=$(c1i apps create --display-name "Example" | jq -r '.app.id') | 3 | not authenticated, or API `401`/`403` | re-authenticate (`c1i auth login`); a scoped credential needs a role that covers the call | | 4 | API `404` | stop — the resource/path doesn't exist | | 5 | API `429` | back off and retry | -| 6 | C1 failed: API `5xx`, a `200` with a body that isn't JSON, or a redirect chain that never settles | retryable, not your fault | +| 6 | C1 failed: API `5xx`, a `200` with a body that isn't JSON, a redirect chain that never settles, or a list route whose pagination cursor never advances | retryable for the first three; a stuck pagination cursor is a persistent server defect on that route — rows already printed are complete as far as they go, but re-running the same command won't reach the rest until C1 fixes it | | 7 | `mcp gateway call` succeeded but the tool reported `isError: true` | inspect the printed result, not just the code | | 8 | a system beyond C1, or the protocol layer, failed — including a gateway that is unreachable (DNS failure, refused connection) | retrying the same call usually repeats it. For a connector failure, check the server with `mcp servers get --app-id ` and `mcp servers test-connection`; the upstream may be unreachable or its credentials expired. For a protocol error, it's a version mismatch or a c1i bug — report it, don't work around it | diff --git a/cmd/api.go b/cmd/api.go index af9bec5..d35ec47 100644 --- a/cmd/api.go +++ b/cmd/api.go @@ -167,10 +167,14 @@ an intended non-GET REST write.`, } enc := newEmitter(cmd) + guard := newPaginationCursorGuard() pageToken := "" - prevToken := "" + route := fmt.Sprintf("%s %s", method, path) for !limitReached(enc.Written(), limit) { + if guard.tokenAlreadyUsed(pageToken) { + return warnStuckCursor(cmd, route) + } reqPath := path var bodyBytes []byte switch method { @@ -250,6 +254,15 @@ an intended non-GET REST write.`, return fmt.Errorf("failed to parse response: %w", err) } + ids := make([]string, len(items)) + for i, item := range items { + ids[i] = itemIdentity(item) + } + if guard.pageIsDuplicate(ids) { + return warnStuckCursor(cmd, route) + } + guard.markIDs(ids) + for _, item := range items { // Encode the raw item, not a decoded any: decoding to any turns // JSON numbers into float64 and corrupts large integer IDs. The @@ -266,12 +279,7 @@ an intended non-GET REST write.`, if nextToken == "" { break } - // Guard against servers that return the same token forever. - // Without this, c1i would loop indefinitely emitting no progress. - if nextToken == prevToken { - return fmt.Errorf("API returned the same nextPageToken twice in a row for %s; the endpoint may not support pagination via the cursor c1i is sending. Drop --paginate and call the endpoint directly, or report this with the path", path) - } - prevToken = nextToken + guard.markTokenUsed(pageToken) pageToken = nextToken } @@ -414,3 +422,31 @@ func extractListAndToken(data []byte, listKey string) ([]json.RawMessage, string return nil, nextPageToken, nil } + +// itemIdentity returns a stable identity for a decoded list item, used by +// the `api --paginate` stuck-cursor guard to detect a page whose rows never +// change even though the server keeps resealing a fresh nextPageToken. Most +// C1 items have a string "id" field; when one is present and non-empty it is +// used verbatim, matching the identity the first-class list commands' own +// guards key off. `api` can hit literally any endpoint, so an item without a +// usable "id" falls back to its own canonical JSON: an unmarshal/remarshal +// round trip through a generic value deterministically sorts object keys, so +// two byte-for-byte-different encodings of the same logical item still +// compare equal. +func itemIdentity(item json.RawMessage) string { + var withID struct { + ID string `json:"id"` + } + if err := json.Unmarshal(item, &withID); err == nil && withID.ID != "" { + return withID.ID + } + var decoded any + if err := json.Unmarshal(item, &decoded); err != nil { + return string(item) + } + canon, err := json.Marshal(decoded) + if err != nil { + return string(item) + } + return string(canon) +} diff --git a/cmd/api_test.go b/cmd/api_test.go index f1261d8..8b275fa 100644 --- a/cmd/api_test.go +++ b/cmd/api_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -811,3 +812,154 @@ func TestAPIPaginateDrainsCursorWithoutWarning(t *testing.T) { } } } + +// TestAPIPaginateSameTokenTwiceReturnsExitServer is the simplest stuck-cursor +// shape: the server sends the exact same nextPageToken forever. `api +// --paginate` must stop after the repeat is detected (not loop), emit the +// one row exactly once, warn on stderr naming the route, and exit 6 ("C1 +// failed") -- this is a known C1 API server defect, not a usage mistake. +func TestAPIPaginateSameTokenTwiceReturnsExitServer(t *testing.T) { + var requestCount int + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + if requestCount > 4 { + t.Fatalf("server received a 5th request; --paginate did not terminate") + } + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, `{"list":[{"id":"a"}],"nextPageToken":"stuck-tok"}`) + })) + defer srv.Close() + + resetAPICmdFlags(t) + stubNewAPIClient(t, srv) + t.Setenv("C1I_URL", srv.URL) + + var out, errOut bytes.Buffer + apiCmd.SetOut(&out) + apiCmd.SetErr(&errOut) + rootCmd.SetArgs([]string{"api", "--path", "/api/v1/automations", "--paginate"}) + + err := rootCmd.ExecuteContext(context.Background()) + if err == nil { + t.Fatalf("ExecuteContext: want an error (stuck cursor -> exit 6), got nil; stdout=%q stderr=%q", out.String(), errOut.String()) + } + if got, want := exitCode(err), exitServer; got != want { + t.Errorf("exitCode(err) = %d, want %d (exitServer); err=%v", got, want, err) + } + if requestCount != 2 { + t.Fatalf("server received %d requests, want exactly 2 (one to discover the stuck page, one to confirm it repeats)", requestCount) + } + rows := decodeNDJSONRows(t, out.String()) + if len(rows) != 1 || rows[0]["id"] != "a" { + t.Fatalf("got rows %v, want exactly one occurrence of id=a", rows) + } + if !strings.Contains(errOut.String(), "INCOMPLETE") || !strings.Contains(errOut.String(), "GET /api/v1/automations") { + t.Errorf("stderr = %q, want it to say results are INCOMPLETE and name GET /api/v1/automations", errOut.String()) + } +} + +// TestAPIPaginateChangingTokenSameRowsReturnsExitServer reproduces the live +// defect exactly: the server reseals a different-looking nextPageToken on +// every call (so a bare seen-token check never fires) while returning the +// exact same row. The guard must key off the duplicate item id instead. +func TestAPIPaginateChangingTokenSameRowsReturnsExitServer(t *testing.T) { + var requestCount int + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + if requestCount > 4 { + t.Fatalf("server received a 5th request; --paginate did not terminate") + } + tok := r.URL.Query().Get("page_token") + w.Header().Set("Content-Type", "application/json") + switch tok { + case "": + _, _ = fmt.Fprint(w, `{"list":[{"id":"a"}],"nextPageToken":"tok-v1"}`) + case "tok-v1": + _, _ = fmt.Fprint(w, `{"list":[{"id":"a"}],"nextPageToken":"tok-v2"}`) + default: + t.Errorf("unexpected page_token: %q", tok) + w.WriteHeader(http.StatusInternalServerError) + } + })) + defer srv.Close() + + resetAPICmdFlags(t) + stubNewAPIClient(t, srv) + t.Setenv("C1I_URL", srv.URL) + + var out, errOut bytes.Buffer + apiCmd.SetOut(&out) + apiCmd.SetErr(&errOut) + rootCmd.SetArgs([]string{"api", "--path", "/api/v1/automations", "--paginate"}) + + err := rootCmd.ExecuteContext(context.Background()) + if err == nil { + t.Fatalf("ExecuteContext: want an error (stuck cursor -> exit 6), got nil; stdout=%q stderr=%q", out.String(), errOut.String()) + } + if got, want := exitCode(err), exitServer; got != want { + t.Errorf("exitCode(err) = %d, want %d (exitServer); err=%v", got, want, err) + } + if requestCount != 2 { + t.Fatalf("server received %d requests, want exactly 2", requestCount) + } + rows := decodeNDJSONRows(t, out.String()) + if len(rows) != 1 || rows[0]["id"] != "a" { + t.Fatalf("got rows %v, want exactly one occurrence of id=a (no duplicate emission)", rows) + } + if !strings.Contains(errOut.String(), "INCOMPLETE") || !strings.Contains(errOut.String(), "GET /api/v1/automations") { + t.Errorf("stderr = %q, want it to say results are INCOMPLETE and name GET /api/v1/automations", errOut.String()) + } +} + +// TestAPIPaginateStuckCursorWithoutIDFallsBackToCanonicalJSON covers an +// endpoint whose items carry no "id" field at all -- `api --path` can target +// any endpoint, so the guard cannot assume one. The duplicate check must +// fall back to the item's own canonical JSON instead of treating every +// idless item as distinct (which would never detect a stuck cursor at all). +func TestAPIPaginateStuckCursorWithoutIDFallsBackToCanonicalJSON(t *testing.T) { + var requestCount int + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + if requestCount > 4 { + t.Fatalf("server received a 5th request; --paginate did not terminate") + } + tok := r.URL.Query().Get("page_token") + w.Header().Set("Content-Type", "application/json") + switch tok { + case "": + _, _ = fmt.Fprint(w, `{"list":[{"name":"n1","value":1}],"nextPageToken":"tok-v1"}`) + case "tok-v1": + // Same logical item, same key order even -- a changing token is + // the only thing that looks different. + _, _ = fmt.Fprint(w, `{"list":[{"name":"n1","value":1}],"nextPageToken":"tok-v2"}`) + default: + t.Errorf("unexpected page_token: %q", tok) + w.WriteHeader(http.StatusInternalServerError) + } + })) + defer srv.Close() + + resetAPICmdFlags(t) + stubNewAPIClient(t, srv) + t.Setenv("C1I_URL", srv.URL) + + var out, errOut bytes.Buffer + apiCmd.SetOut(&out) + apiCmd.SetErr(&errOut) + rootCmd.SetArgs([]string{"api", "--path", "/api/v1/decoys", "--paginate"}) + + err := rootCmd.ExecuteContext(context.Background()) + if err == nil { + t.Fatalf("ExecuteContext: want an error (stuck cursor -> exit 6), got nil; stdout=%q stderr=%q", out.String(), errOut.String()) + } + if got, want := exitCode(err), exitServer; got != want { + t.Errorf("exitCode(err) = %d, want %d (exitServer); err=%v", got, want, err) + } + if requestCount != 2 { + t.Fatalf("server received %d requests, want exactly 2", requestCount) + } + rows := decodeNDJSONRows(t, out.String()) + if len(rows) != 1 || rows[0]["name"] != "n1" { + t.Fatalf("got rows %v, want exactly one occurrence of name=n1 (no duplicate emission)", rows) + } +} diff --git a/cmd/automations_executions_list.go b/cmd/automations_executions_list.go index cea5efa..292ab9b 100644 --- a/cmd/automations_executions_list.go +++ b/cmd/automations_executions_list.go @@ -71,8 +71,12 @@ the server returns. Combine with --limit to bound the work.`, clientFilter := stateFilter != "" || templateID != "" enc := newEmitter(cmd) - prevToken := "" + guard := newPaginationCursorGuard() for !limitReached(enc.Written(), limit) { + if guard.tokenAlreadyUsed(pageToken) { + return warnStuckCursor(cmd, "GET /api/v1/automation_executions") + } + pageSize := requestedPageSize if !clientFilter && !enc.Filtered() { pageSize = effectivePageSize(requestedPageSize, limit, enc.Written()) @@ -97,6 +101,15 @@ the server returns. Combine with --limit to bound the work.`, return fmt.Errorf("failed to parse response: %w", err) } + ids := make([]string, len(resp.AutomationExecutions)) + for i, e := range resp.AutomationExecutions { + ids[i] = e.ID + } + if guard.pageIsDuplicate(ids) { + return warnStuckCursor(cmd, "GET /api/v1/automation_executions") + } + guard.markIDs(ids) + for _, e := range resp.AutomationExecutions { if stateFilter != "" && e.State != stateFilter { continue @@ -113,13 +126,7 @@ the server returns. Combine with --limit to bound the work.`, if resp.NextPageToken == "" || manualPaging { break } - // Some pagination paths on this endpoint have been seen returning - // the same cursor twice. Catch that and abort rather than spin — - // the caller can drop --page-token and retry. - if resp.NextPageToken == prevToken { - return fmt.Errorf("automation_executions returned the same nextPageToken twice in a row; pagination on this endpoint may be flaky for the current cursor — re-run without --page-token or report the issue") - } - prevToken = resp.NextPageToken + guard.markTokenUsed(pageToken) pageToken = resp.NextPageToken } diff --git a/cmd/automations_list.go b/cmd/automations_list.go index 12f75c8..753eb98 100644 --- a/cmd/automations_list.go +++ b/cmd/automations_list.go @@ -101,7 +101,12 @@ var automationsListCmd = &cobra.Command{ clientFilter := enabledOnly || callsFunction != "" enc := newEmitter(cmd) + guard := newPaginationCursorGuard() for !limitReached(enc.Written(), limit) { + if guard.tokenAlreadyUsed(pageToken) { + return warnStuckCursor(cmd, "GET /api/v1/automations") + } + pageSize := requestedPageSize if !clientFilter && !enc.Filtered() { pageSize = effectivePageSize(requestedPageSize, limit, enc.Written()) @@ -126,6 +131,15 @@ var automationsListCmd = &cobra.Command{ return fmt.Errorf("failed to parse response: %w", err) } + ids := make([]string, len(resp.List)) + for i, a := range resp.List { + ids[i] = a.ID + } + if guard.pageIsDuplicate(ids) { + return warnStuckCursor(cmd, "GET /api/v1/automations") + } + guard.markIDs(ids) + for _, a := range resp.List { if enabledOnly && !a.Enabled { continue @@ -143,6 +157,7 @@ var automationsListCmd = &cobra.Command{ if resp.NextPageToken == "" || manualPaging { break } + guard.markTokenUsed(pageToken) pageToken = resp.NextPageToken } diff --git a/cmd/automations_list_pagination_test.go b/cmd/automations_list_pagination_test.go new file mode 100644 index 0000000..1f32066 --- /dev/null +++ b/cmd/automations_list_pagination_test.go @@ -0,0 +1,214 @@ +package cmd + +import ( + "bytes" + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/ConductorOne/c1i/internal/client" + "github.com/spf13/cobra" +) + +// resetAutomationsListCmdFlags restores automationsListCmd's own flags to +// their zero values and clears pflag's per-flag Changed bit, so tests +// sharing the package-level singleton command can't leak flag state into +// each other. Mirrors resetPoliciesListCmdFlags in +// cmd/policies_pagination_test.go. +func resetAutomationsListCmdFlags(t *testing.T) { + t.Helper() + reset := func() { + for name, def := range map[string]string{ + "page-size": "50", + "page-token": "", + "limit": "0", + "enabled-only": "false", + "calls-function": "", + } { + f := automationsListCmd.Flags().Lookup(name) + _ = f.Value.Set(def) + f.Changed = false + } + } + reset() + t.Cleanup(reset) +} + +// runAutomationsListCmd drives automationsListCmd.RunE directly against a +// test server and returns its NDJSON stdout plus anything written to +// stderr (warnings). +func runAutomationsListCmd(t *testing.T, srv *httptest.Server) (stdout, stderr string, err error) { + t.Helper() + orig := newListClient + newListClient = func(_ *cobra.Command, _ string) (*client.Client, error) { + return client.NewForTesting(srv.URL, srv.Client()), nil + } + t.Cleanup(func() { newListClient = orig }) + t.Setenv("C1I_URL", "https://example.invalid") + + var out, errOut bytes.Buffer + automationsListCmd.SetOut(&out) + automationsListCmd.SetErr(&errOut) + automationsListCmd.SetContext(context.Background()) + runErr := automationsListCmd.RunE(automationsListCmd, []string{}) + return out.String(), errOut.String(), runErr +} + +// automationPage builds a GET /api/v1/automations response body holding the +// given ids with the given nextPageToken, matching the "automations list" +// case's flatPage("list","id") shape in cmd/list_pagination_test.go. +func automationPage(ids []string, nextToken string) string { + items := make([]string, 0, len(ids)) + for _, id := range ids { + items = append(items, fmt.Sprintf(`{"id":%q}`, id)) + } + return fmt.Sprintf(`{"list":[%s],"nextPageToken":%q}`, joinJSON(items), nextToken) +} + +func joinJSON(items []string) string { + s := "" + for i, it := range items { + if i > 0 { + s += "," + } + s += it + } + return s +} + +// TestAutomationsListStuckCursorSameTokenTerminates reproduces the live +// defect: a server whose final page carries a non-empty nextPageToken that, +// when followed, returns the SAME page (including the same token) again. +// Without a duplicate-page guard this loops forever, re-emitting the same +// rows; the fix must stop after the repeat is detected and emit each row +// exactly once. +func TestAutomationsListStuckCursorSameTokenTerminates(t *testing.T) { + var requestCount int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + if requestCount > 4 { + t.Fatalf("server received a 5th request; auto-pagination did not terminate") + } + tok := r.URL.Query().Get("page_token") + w.Header().Set("Content-Type", "application/json") + switch tok { + case "": + _, _ = fmt.Fprint(w, automationPage([]string{"auto-a", "auto-b"}, "stuck-tok")) + case "stuck-tok": + // The final page: same rows, same cursor, forever. + _, _ = fmt.Fprint(w, automationPage([]string{"auto-a", "auto-b"}, "stuck-tok")) + default: + t.Errorf("unexpected page_token: %q", tok) + w.WriteHeader(http.StatusInternalServerError) + } + })) + defer srv.Close() + + resetAutomationsListCmdFlags(t) + out, errOut, err := runAutomationsListCmd(t, srv) + if err == nil { + t.Fatalf("RunE: want an error (stuck cursor -> exit 6), got nil; stdout=%q stderr=%q", out, errOut) + } + if got, want := exitCode(err), exitServer; got != want { + t.Errorf("exitCode(err) = %d, want %d (exitServer, \"C1 failed\"); err=%v", got, want, err) + } + + rows := decodeNDJSONRows(t, out) + if len(rows) != 2 || rows[0]["id"] != "auto-a" || rows[1]["id"] != "auto-b" { + t.Fatalf("got rows %v, want exactly one occurrence each of auto-a, auto-b (a stuck cursor must not re-emit them)", rows) + } + if requestCount != 2 { + t.Fatalf("server received %d requests, want exactly 2 (one to discover the stuck page, one to confirm it repeats)", requestCount) + } + if !strings.Contains(errOut, "INCOMPLETE") || !strings.Contains(errOut, "GET /api/v1/automations") { + t.Errorf("stderr warning = %q, want it to say results are INCOMPLETE and name GET /api/v1/automations", errOut) + } +} + +// TestAutomationsListStuckCursorChangingTokenTerminates covers the case +// where the server's cursor string changes on every call (so a bare +// seen-token check never fires) but the page content never advances. The +// guard must key off duplicate item ids, not just the literal token value. +func TestAutomationsListStuckCursorChangingTokenTerminates(t *testing.T) { + var requestCount int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + if requestCount > 4 { + t.Fatalf("server received a 5th request; auto-pagination did not terminate") + } + tok := r.URL.Query().Get("page_token") + w.Header().Set("Content-Type", "application/json") + switch tok { + case "": + _, _ = fmt.Fprint(w, automationPage([]string{"auto-a", "auto-b"}, "tok-v1")) + case "tok-v1": + // Same rows again, but a freshly minted, never-before-seen token. + _, _ = fmt.Fprint(w, automationPage([]string{"auto-a", "auto-b"}, "tok-v2")) + default: + t.Errorf("unexpected page_token: %q", tok) + w.WriteHeader(http.StatusInternalServerError) + } + })) + defer srv.Close() + + resetAutomationsListCmdFlags(t) + out, errOut, err := runAutomationsListCmd(t, srv) + if err == nil { + t.Fatalf("RunE: want an error (stuck cursor -> exit 6), got nil; stdout=%q stderr=%q", out, errOut) + } + if got, want := exitCode(err), exitServer; got != want { + t.Errorf("exitCode(err) = %d, want %d (exitServer, \"C1 failed\"); err=%v", got, want, err) + } + + rows := decodeNDJSONRows(t, out) + if len(rows) != 2 || rows[0]["id"] != "auto-a" || rows[1]["id"] != "auto-b" { + t.Fatalf("got rows %v, want exactly one occurrence each of auto-a, auto-b", rows) + } + if requestCount != 2 { + t.Fatalf("server received %d requests, want exactly 2", requestCount) + } + if !strings.Contains(errOut, "INCOMPLETE") || !strings.Contains(errOut, "GET /api/v1/automations") { + t.Errorf("stderr warning = %q, want it to say results are INCOMPLETE and name GET /api/v1/automations", errOut) + } +} + +// TestAutomationsListStuckCursorEmptyPagesTerminates covers a server that +// never advances past an empty final page but keeps returning the same +// cursor: the duplicate-id check never fires (empty pages are never a +// duplicate, matching TestListCommandsEmptyPageWithTokenDoesNotStopEarly), +// so the repeated-token guard must be the one to stop it. +func TestAutomationsListStuckCursorEmptyPagesTerminates(t *testing.T) { + var requestCount int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + if requestCount > 4 { + t.Fatalf("server received a 5th request; auto-pagination did not terminate") + } + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, automationPage(nil, "forever-tok")) + })) + defer srv.Close() + + resetAutomationsListCmdFlags(t) + out, errOut, err := runAutomationsListCmd(t, srv) + if err == nil { + t.Fatalf("RunE: want an error (stuck cursor -> exit 6), got nil; stdout=%q stderr=%q", out, errOut) + } + if got, want := exitCode(err), exitServer; got != want { + t.Errorf("exitCode(err) = %d, want %d (exitServer, \"C1 failed\"); err=%v", got, want, err) + } + + rows := decodeNDJSONRows(t, out) + if len(rows) != 0 { + t.Fatalf("got %d rows, want 0", len(rows)) + } + if requestCount != 2 { + t.Fatalf("server received %d requests, want exactly 2 (the token repeats on the 2nd response, caught before a 3rd request)", requestCount) + } + if !strings.Contains(errOut, "INCOMPLETE") || !strings.Contains(errOut, "GET /api/v1/automations") { + t.Errorf("stderr warning = %q, want it to say results are INCOMPLETE and name GET /api/v1/automations", errOut) + } +} diff --git a/cmd/errors.go b/cmd/errors.go index c94cc87..755c3b6 100644 --- a/cmd/errors.go +++ b/cmd/errors.go @@ -26,7 +26,7 @@ const ( exitAuth = 3 // not authenticated, or API returned 401/403 exitNotFound = 4 // API returned 404 exitRateLimited = 5 // API returned 429 - exitServer = 6 // C1 failed: 5xx, a redirect loop, or a 200 with a non-JSON body + exitServer = 6 // C1 failed: 5xx, a redirect loop, a 200 with a non-JSON body, or a list route whose pagination cursor never advances exitToolError = 7 // an MCP tool call completed (transport/protocol succeeded) but the tool itself reported isError exitUpstream = 8 // an upstream system, or the protocol layer itself, failed -- distinct from the C1 API failing (e.g. an unreachable MCP connector, or a JSON-RPC protocol-level error) ) @@ -65,6 +65,24 @@ type nonJSONResponseError struct{ err error } func (e *nonJSONResponseError) Error() string { return e.err.Error() } func (e *nonJSONResponseError) Unwrap() error { return e.err } +// stuckPaginationCursorError marks a list route (GET, or any method under +// `c1i api --paginate`) whose nextPageToken never makes real progress: the +// exact same token sent twice, or a non-empty page whose every item was +// already emitted under an earlier -- possibly different-looking -- token. +// This is a known C1 API server defect (the affected route's page_size and +// page_token query/body parameters are never bound server-side, so it keeps +// resealing a fresh-looking cursor over the same first page) rather than a +// client bug or a usage mistake, so it maps to exitServer (6, "C1 failed") +// like any other case of C1 not honoring its own API contract. By the time +// this is returned, a stderr warning naming the route has already been +// printed and every row fetched before the stuck page is already on stdout, +// with no duplicates: only further cursor-following stopped. +type stuckPaginationCursorError struct{ route string } + +func (e *stuckPaginationCursorError) Error() string { + return fmt.Sprintf("results from %s are INCOMPLETE: the server did not advance its pagination cursor", e.route) +} + // upstreamError marks a failure beyond C1, or in the protocol layer, that did // not arrive as a classifiable HTTP status — an upstream connector failing, or // a JSON-RPC protocol error. It maps to exitUpstream (8), distinct from C1 @@ -208,6 +226,10 @@ func exitCode(err error) int { if errors.As(err, &upstreamErr) { return exitUpstream } + var stuckErr *stuckPaginationCursorError + if errors.As(err, &stuckErr) { + return exitServer + } var nonJSONErr *nonJSONResponseError if errors.As(err, &nonJSONErr) { return exitServer diff --git a/cmd/flags.go b/cmd/flags.go index 4cd9c89..4ae4d8c 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -215,6 +215,79 @@ func pageSizeFlag(cmd *cobra.Command) int { return maxSize } +// paginationCursorGuard detects a server that stops making real progress +// during auto-pagination instead of raising nextPageToken to "": either the +// same cursor sent twice, or a non-empty page whose every id was already +// emitted. Both shapes loop forever without it (observed live on +// /api/v1/automations: a final page can carry a non-empty nextPageToken that +// leads straight back to page one). Zero value is unusable; construct with +// newPaginationCursorGuard. +type paginationCursorGuard struct { + seenTokens map[string]bool + seenIDs map[string]bool +} + +func newPaginationCursorGuard() *paginationCursorGuard { + return &paginationCursorGuard{seenTokens: map[string]bool{}, seenIDs: map[string]bool{}} +} + +// tokenAlreadyUsed reports whether token was already sent as a page_token +// earlier in this run. Check it before issuing the request so a repeated +// cursor is caught without re-fetching and re-emitting that page first. +func (g *paginationCursorGuard) tokenAlreadyUsed(token string) bool { + return token != "" && g.seenTokens[token] +} + +// markTokenUsed records token as sent. Call once per request, with the +// token that request just used, after the page has been accepted. +func (g *paginationCursorGuard) markTokenUsed(token string) { + if token != "" { + g.seenTokens[token] = true + } +} + +// pageIsDuplicate reports whether every id in a freshly fetched page was +// already recorded by markIDs for an earlier page — the server repeating +// content under a cursor that doesn't actually advance, even if the cursor +// string itself varies. An empty page is never a duplicate: a live token can +// legitimately precede an empty intermediate page. +func (g *paginationCursorGuard) pageIsDuplicate(ids []string) bool { + if len(ids) == 0 { + return false + } + for _, id := range ids { + if !g.seenIDs[id] { + return false + } + } + return true +} + +// markIDs records a page's item ids once it has been accepted, i.e. after +// pageIsDuplicate returned false for it. +func (g *paginationCursorGuard) markIDs(ids []string) { + for _, id := range ids { + g.seenIDs[id] = true + } +} + +// warnStuckCursor prints the one stderr line every stuck-cursor call site +// must show (automations/functions list commands, and `api --paginate`) +// and returns the error the caller's RunE must return immediately after, so +// Run() exits 6 ("C1 failed") rather than 0. route names the method and +// path for the user/agent to report (e.g. "GET /api/v1/automations"). +// +// This is deliberately not a silent `break` + exit 0: the rows already +// fetched stay on stdout (the caller must not re-fetch or re-emit the stuck +// page), but the overall result is incomplete because of a known C1 API +// server defect -- not something a c1i workaround or a different endpoint +// fixes -- so the exit code has to say so. +func warnStuckCursor(cmd *cobra.Command, route string) error { + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), + "Warning: results from %s are INCOMPLETE: the server did not advance the pagination cursor (repeated or non-advancing nextPageToken). Rows already fetched are printed above with no duplicates; this is a known C1 API server defect, not a c1i bug.\n", route) + return &stuckPaginationCursorError{route: route} +} + // requireNonEmpty errors when any of the named string flags has an empty // value. Cobra's required-flag enforcement only checks that the flag was // set, not that the value is non-empty — so `--app-id ""` silently passes. diff --git a/cmd/functions_commits.go b/cmd/functions_commits.go index 978b5dd..5e42cd6 100644 --- a/cmd/functions_commits.go +++ b/cmd/functions_commits.go @@ -31,7 +31,13 @@ var functionsCommitsCmd = &cobra.Command{ limit := getIntFlag(cmd, "limit") enc := newEmitter(cmd) + guard := newPaginationCursorGuard() + route := fmt.Sprintf("GET /api/v1/functions/%s/commits", functionID) for !limitReached(enc.Written(), limit) { + if guard.tokenAlreadyUsed(pageToken) { + return warnStuckCursor(cmd, route) + } + pageSize := requestedPageSize if !enc.Filtered() { pageSize = effectivePageSize(requestedPageSize, limit, enc.Written()) @@ -62,6 +68,15 @@ var functionsCommitsCmd = &cobra.Command{ return fmt.Errorf("failed to parse response: %w", err) } + ids := make([]string, len(resp.List)) + for i, item := range resp.List { + ids[i] = item.ID + } + if guard.pageIsDuplicate(ids) { + return warnStuckCursor(cmd, route) + } + guard.markIDs(ids) + for _, c := range resp.List { _ = enc.Encode(map[string]string{ "id": c.ID, @@ -78,6 +93,7 @@ var functionsCommitsCmd = &cobra.Command{ if resp.NextPageToken == "" || manualPaging { break } + guard.markTokenUsed(pageToken) pageToken = resp.NextPageToken } diff --git a/cmd/functions_list.go b/cmd/functions_list.go index 0db07ff..75e7381 100644 --- a/cmd/functions_list.go +++ b/cmd/functions_list.go @@ -77,7 +77,12 @@ var functionsListCmd = &cobra.Command{ clientFilter := publishedOnly || draftOnly enc := newEmitter(cmd) + guard := newPaginationCursorGuard() for !limitReached(enc.Written(), limit) { + if guard.tokenAlreadyUsed(pageToken) { + return warnStuckCursor(cmd, "GET /api/v1/functions") + } + pageSize := requestedPageSize if !clientFilter && !enc.Filtered() { pageSize = effectivePageSize(requestedPageSize, limit, enc.Written()) @@ -102,6 +107,15 @@ var functionsListCmd = &cobra.Command{ return fmt.Errorf("failed to parse response: %w", err) } + ids := make([]string, len(resp.List)) + for i, f := range resp.List { + ids[i] = f.ID + } + if guard.pageIsDuplicate(ids) { + return warnStuckCursor(cmd, "GET /api/v1/functions") + } + guard.markIDs(ids) + for _, f := range resp.List { published := functionIsPublished(f) if publishedOnly && !published { @@ -119,6 +133,7 @@ var functionsListCmd = &cobra.Command{ if resp.NextPageToken == "" || manualPaging { break } + guard.markTokenUsed(pageToken) pageToken = resp.NextPageToken } diff --git a/cmd/functions_usage.go b/cmd/functions_usage.go index eb16703..bf9fe01 100644 --- a/cmd/functions_usage.go +++ b/cmd/functions_usage.go @@ -55,7 +55,12 @@ last_executed_at, args.`, // (see emitter.Filtered), so emitted alone would overcount rows // actually written to stdout. emitted := 0 + guard := newPaginationCursorGuard() for !limitReached(enc.Written(), limit) { + if guard.tokenAlreadyUsed(pageToken) { + return warnStuckCursor(cmd, "GET /api/v1/automations") + } + params := map[string]string{"page_size": strconv.Itoa(pageSize)} if pageToken != "" { params["page_token"] = pageToken @@ -86,6 +91,15 @@ last_executed_at, args.`, return fmt.Errorf("failed to parse response: %w", err) } + ids := make([]string, len(resp.List)) + for i, a := range resp.List { + ids[i] = a.ID + } + if guard.pageIsDuplicate(ids) { + return warnStuckCursor(cmd, "GET /api/v1/automations") + } + guard.markIDs(ids) + for _, a := range resp.List { for _, step := range a.AutomationSteps { if step.CallFunction == nil || step.CallFunction.FunctionID != functionID { @@ -113,6 +127,7 @@ last_executed_at, args.`, if resp.NextPageToken == "" || manualPaging { break } + guard.markTokenUsed(pageToken) pageToken = resp.NextPageToken } diff --git a/cmd/list_pagination_test.go b/cmd/list_pagination_test.go index 3dded2b..70c52a6 100644 --- a/cmd/list_pagination_test.go +++ b/cmd/list_pagination_test.go @@ -40,6 +40,15 @@ type listPaginationCase struct { // rowIDs extracts, in emission order, the identifying value each item() // call varied per row from the command's decoded NDJSON output. rowIDs func(rows []map[string]any) []string + // stuckCursorRoute is non-empty only for a command backed by a + // confirmed-broken GET route (see paginationCursorGuard and + // warnStuckCursor): one whose page_size/page_token are not bound + // server-side, so it keeps resealing a fresh-looking nextPageToken over + // the same page forever. TestListCommandsStuckCursorReturnsExitServer + // runs only the cases that set this, asserting auto-pagination stops, + // emits each row exactly once, and exits 6 ("C1 failed") naming this + // route. Commands on correctly-bound routes must leave it empty. + stuckCursorRoute string } // Every case's command is a package-level singleton shared by the whole test @@ -86,7 +95,7 @@ func extractPageToken(t *testing.T, method string, r *http.Request) string { return tok } -func runListPaginationCase(t *testing.T, tc listPaginationCase, srv *httptest.Server) (string, error) { +func runListPaginationCase(t *testing.T, tc listPaginationCase, srv *httptest.Server) (stdout, stderr string, err error) { t.Helper() orig := newListClient newListClient = func(_ *cobra.Command, _ string) (*client.Client, error) { @@ -97,16 +106,17 @@ func runListPaginationCase(t *testing.T, tc listPaginationCase, srv *httptest.Se resetCmdFlags(t, tc.cmd) for name, val := range tc.extraFlags { - if err := tc.cmd.Flags().Set(name, val); err != nil { - t.Fatalf("setting --%s=%s: %v", name, val, err) + if setErr := tc.cmd.Flags().Set(name, val); setErr != nil { + t.Fatalf("setting --%s=%s: %v", name, val, setErr) } } - var out bytes.Buffer + var out, errOut bytes.Buffer tc.cmd.SetOut(&out) + tc.cmd.SetErr(&errOut) tc.cmd.SetContext(context.Background()) - err := tc.cmd.RunE(tc.cmd, tc.args) - return out.String(), err + runErr := tc.cmd.RunE(tc.cmd, tc.args) + return out.String(), errOut.String(), runErr } // jstr renders a Go string as a JSON string literal for hand-built fixture @@ -185,12 +195,13 @@ func listPaginationCases() []listPaginationCase { rowIDs: idRows("id"), }, { - name: "automations executions list", - cmd: automationsExecutionsListCmd, - method: http.MethodGet, - wantPath: "/api/v1/automation_executions", - page: flatPage("automationExecutions", "id"), - rowIDs: idRows("id"), + name: "automations executions list", + cmd: automationsExecutionsListCmd, + method: http.MethodGet, + wantPath: "/api/v1/automation_executions", + page: flatPage("automationExecutions", "id"), + rowIDs: idRows("id"), + stuckCursorRoute: "GET /api/v1/automation_executions", }, { name: "automations list", @@ -234,21 +245,23 @@ func listPaginationCases() []listPaginationCase { rowIDs: idRows("id"), }, { - name: "functions commits", - cmd: functionsCommitsCmd, - method: http.MethodGet, - wantPath: "/api/v1/functions/func1/commits", - args: []string{"func1"}, - page: flatPage("list", "id"), - rowIDs: idRows("id"), + name: "functions commits", + cmd: functionsCommitsCmd, + method: http.MethodGet, + wantPath: "/api/v1/functions/func1/commits", + args: []string{"func1"}, + page: flatPage("list", "id"), + rowIDs: idRows("id"), + stuckCursorRoute: "GET /api/v1/functions/func1/commits", }, { - name: "functions list", - cmd: functionsListCmd, - method: http.MethodGet, - wantPath: "/api/v1/functions", - page: flatPage("list", "id"), - rowIDs: idRows("id"), + name: "functions list", + cmd: functionsListCmd, + method: http.MethodGet, + wantPath: "/api/v1/functions", + page: flatPage("list", "id"), + rowIDs: idRows("id"), + stuckCursorRoute: "GET /api/v1/functions", }, { name: "functions usage", @@ -267,7 +280,8 @@ func listPaginationCases() []listPaginationCase { } return fmt.Sprintf(`{"list":[%s],"nextPageToken":%s}`, strings.Join(items, ","), jstr(next)) }, - rowIDs: idRows("automation_id"), + rowIDs: idRows("automation_id"), + stuckCursorRoute: "GET /api/v1/automations", }, { name: "grants list", @@ -443,7 +457,7 @@ func TestListCommandsPaginateAcrossTwoPages(t *testing.T) { })) defer srv.Close() - out, err := runListPaginationCase(t, tc, srv) + out, _, err := runListPaginationCase(t, tc, srv) if err != nil { t.Fatalf("RunE: %v", err) } @@ -486,7 +500,7 @@ func TestListCommandsEmptyPageWithTokenDoesNotStopEarly(t *testing.T) { })) defer srv.Close() - out, err := runListPaginationCase(t, tc, srv) + out, _, err := runListPaginationCase(t, tc, srv) if err != nil { t.Fatalf("RunE: %v", err) } @@ -524,7 +538,7 @@ func TestListCommandsExplicitPageTokenDisablesAutoPagination(t *testing.T) { tc.extraFlags = map[string]string{} } tc.extraFlags["page-token"] = "manual-tok" - out, err := runListPaginationCase(t, tc, srv) + out, _, err := runListPaginationCase(t, tc, srv) if err != nil { t.Fatalf("RunE: %v", err) } @@ -540,3 +554,61 @@ func TestListCommandsExplicitPageTokenDisablesAutoPagination(t *testing.T) { }) } } + +// TestListCommandsStuckCursorReturnsExitServer covers every command backed +// by a confirmed-broken GET route (stuckCursorRoute set in +// listPaginationCases): a server that reseals a fresh, never-before-seen +// nextPageToken on every call while returning the exact same row. A bare +// seen-token check never fires against that shape; the guard must key off +// the duplicate item id instead. Without it, these commands loop forever, +// re-emitting the same row. The fix must stop after the repeat is detected, +// emit the row exactly once, print a stderr warning naming the route, and +// exit 6 ("C1 failed") -- a known C1 API server defect, not a usage mistake +// and not something a different c1i command or endpoint works around. +func TestListCommandsStuckCursorReturnsExitServer(t *testing.T) { + for _, tc := range listPaginationCases() { + if tc.stuckCursorRoute == "" { + continue + } + t.Run(tc.name, func(t *testing.T) { + var requestCount int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + if requestCount > 4 { + t.Fatalf("server received a 5th request; auto-pagination did not terminate") + } + tok := extractPageToken(t, tc.method, r) + w.Header().Set("Content-Type", "application/json") + switch tok { + case "": + _, _ = fmt.Fprint(w, tc.page([]string{"stuck-item"}, "tok-v1")) + case "tok-v1": + _, _ = fmt.Fprint(w, tc.page([]string{"stuck-item"}, "tok-v2")) + default: + t.Errorf("unexpected page token: %q", tok) + w.WriteHeader(http.StatusInternalServerError) + } + })) + defer srv.Close() + + out, errOut, err := runListPaginationCase(t, tc, srv) + if err == nil { + t.Fatalf("RunE: want an error (stuck cursor -> exit 6), got nil; stdout=%q stderr=%q", out, errOut) + } + if got, want := exitCode(err), exitServer; got != want { + t.Errorf("exitCode(err) = %d, want %d (exitServer, \"C1 failed\"); err=%v", got, want, err) + } + if requestCount != 2 { + t.Fatalf("server received %d requests, want exactly 2 (one to discover the stuck page, one to confirm it repeats)", requestCount) + } + rows := decodeNDJSONRows(t, out) + ids := tc.rowIDs(rows) + if len(ids) != 1 || ids[0] != "stuck-item" { + t.Fatalf("got ids %v, want exactly one occurrence of stuck-item (a stuck cursor must not re-emit it)", ids) + } + if !strings.Contains(errOut, "INCOMPLETE") || !strings.Contains(errOut, tc.stuckCursorRoute) { + t.Errorf("stderr warning = %q, want it to say results are INCOMPLETE and name %s", errOut, tc.stuckCursorRoute) + } + }) + } +} From 35a14098b5cff695fe07f173d6ca38b77cb3b8ea Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Fri, 6 Nov 2026 00:16:30 +0000 Subject: [PATCH 2/2] fix: stop the stuck-cursor guard from failing healthy pagination Review follow-ups to the stuck-cursor guard: - A page whose rows were all seen before no longer stops pagination on its own: a healthy offset-paginated server that gains exactly a page of rows between requests produces one. Such a page is skipped, and only two in a row (or a repeated page_token) mean the cursor is stuck. A seen-id set, not a previous-page comparison, so alternating, reordered, and empty-interleaved stuck pages are still caught. Rows with an empty id are ignored by the check rather than collapsing into one id. - itemIdentity decodes with UseNumber, so id-less items differing only in a large integer keep distinct identities. - Docs, CHANGELOG, exit-code tables, and the warning drop claims the code doesn't back: that no other endpoint gives a complete result, that the output has no duplicates, that every list command exits 6 on a stuck cursor, and that the fault is always the server's. - Tests: handler goroutines no longer call t.Fatalf; new tests cover a single repeated page on a healthy server, cycle shapes (alternation, reorder, empty interleave, --limit), empty ids, and large-integer identity. Each fails with the guard disabled or on the previous code. Verified live on a test tenant with ~88 automations: automations list, api --paginate, and functions usage stop with exit 6 after 53 rows, all distinct; --limit within the first page exits 0; api --paginate on several correctly paginating routes still exits 0 with complete results. Co-Authored-By: Claude Opus 5.5 --- .claude/commands/c1i.md | 6 +- CHANGELOG.md | 25 ++-- CLAUDE.md | 6 +- README.md | 18 +-- cmd/agents.md | 2 +- cmd/api.go | 27 ++-- cmd/api_test.go | 164 ++++++++++++++++++++---- cmd/automations_executions_list.go | 7 +- cmd/automations_list.go | 7 +- cmd/automations_list_pagination_test.go | 91 ++++--------- cmd/errors.go | 19 +-- cmd/flags.go | 80 ++++++------ cmd/flags_test.go | 11 ++ cmd/functions_commits.go | 7 +- cmd/functions_list.go | 7 +- cmd/functions_usage.go | 7 +- cmd/list_pagination_test.go | 35 ++--- 17 files changed, 290 insertions(+), 229 deletions(-) diff --git a/.claude/commands/c1i.md b/.claude/commands/c1i.md index 1858f4c..d09e528 100644 --- a/.claude/commands/c1i.md +++ b/.claude/commands/c1i.md @@ -82,7 +82,7 @@ request was sent. | 3 | not authenticated, or API `401`/`403` | | 4 | API `404` (not found) | | 5 | API `429` (rate limited) | -| 6 | C1 failed: API `5xx`, a `200` with a body that isn't JSON, a redirect loop (`RedirectLoopError`), or a list route whose pagination cursor never advances | +| 6 | C1 failed: API `5xx`, a `200` with a body that isn't JSON, a redirect loop (`RedirectLoopError`), or a stuck pagination cursor (`automations`/`functions` lists, `api --paginate`) | | 7 | MCP tool call completed but the tool itself reported `isError: true` | | 8 | a system beyond C1, or the MCP protocol layer, failed (includes an unreachable gateway) | @@ -90,8 +90,8 @@ Typed internally and classified in one place, `cmd/errors.go`: from the client, `APIError`, `AuthError`, `PathError`, `RedirectError`, `RedirectLoopError`; from the gateway, `mcpgateway.TransportError` (unreachable); plus package-`cmd` wrappers `usageError`, `toolExecutionError`, `nonJSONResponseError`, -`stuckPaginationCursorError` (a list route's pagination cursor never -advances), `upstreamError`, and `helperLeftError`. +`stuckPaginationCursorError` (a guarded list's cursor stopped +advancing), `upstreamError`, and `helperLeftError`. ## Documentation diff --git a/CHANGELOG.md b/CHANGELOG.md index cc8b2a3..560856a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,22 +24,15 @@ credentials fails instead of using the working directory. With a relative ### Fixed - **`automations list`, `automations executions list`, `functions list`, - `functions commits`, and `functions usage` could loop forever**, and - `api --paginate` against the same routes could too: a handful of GET list - routes (`/api/v1/automations`, `/api/v1/automation_executions`, - `/api/v1/functions`, `/api/v1/functions/{id}/commits`) never bind - `page_size`/`page_token` server-side, so a large enough result set makes - the server keep resealing a fresh-looking `nextPageToken` over the exact - same first page forever — a known C1 API server defect, reported - separately, not something a c1i workaround or a different endpoint fixes. - Every affected command, and `api --paginate` generically (via item `id`, - falling back to an item's canonical JSON when it has none), now detects - the stuck cursor — the same token sent twice, or a non-empty page whose - every item was already emitted under an earlier token — stops instead of - looping, prints each row already fetched with no duplicates, warns on - stderr that results are INCOMPLETE and names the route, and exits `6` - ("C1 failed") instead of 0. `c1i docs agents`, the README, and CLAUDE.md - document the new exit-6 cause and the `stuckPaginationCursorError` type. + `functions commits`, `functions usage`, and `api --paginate` could loop + forever**, re-printing the same rows: on a large enough result set, + `/api/v1/automations`, `/api/v1/automation_executions`, + `/api/v1/functions`, and `/api/v1/functions/{id}/commits` ignore + `page_size`/`page_token` and return the first page under a fresh + `nextPageToken` every time. These commands now stop when the cursor stops + advancing (the same token twice, or two pages in a row of rows already + seen), keep the rows already printed, warn on stderr that results are + INCOMPLETE, and exit `6`. - **`--debug` and `--max-retries` did nothing on `docs openapi`, `docs endpoints` and `docs endpoint`.** The spec fetch now uses the shared diff --git a/CLAUDE.md b/CLAUDE.md index fb88bf8..a0d781f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -229,8 +229,8 @@ tracked root file outside its allowlist. Stage explicit paths rather than `-A`. hops); `internal/mcpgateway` adds `TransportError` for an unreachable gateway. `cmd/errors.go` maps them — plus its own `usageError`, `toolExecutionError`, `nonJSONResponseError`, - `stuckPaginationCursorError` (a list route's pagination cursor never - advances), and `upstreamError` wrappers, for failures with no natural + `stuckPaginationCursorError` (a guarded list's cursor stopped advancing), + and `upstreamError` wrappers, for failures with no natural client-level type, and `helperLeftError` (a login's undeleted temporary credential, always 1) — to exit codes: @@ -242,7 +242,7 @@ tracked root file outside its allowlist. Stage explicit paths rather than `-A`. | `3` | auth (`401`/`403`) | | `4` | not-found (`404`) | | `5` | rate-limited (`429`) | - | `6` | C1 failed (API `5xx`, a redirect loop, a `200` with a non-JSON body, or a list route whose pagination cursor never advances) | + | `6` | C1 failed (API `5xx`, a redirect loop, a `200` with a non-JSON body, or a stuck pagination cursor on a guarded list) | | `7` | tool-execution error (`mcp gateway call` result has `isError: true`) | | `8` | a system beyond C1 or the protocol layer failed (an unreachable gateway, an upstream connector failure, or a protocol-level JSON-RPC error) | diff --git a/README.md b/README.md index 6ee478c..8818ef2 100644 --- a/README.md +++ b/README.md @@ -308,13 +308,10 @@ c1i functions usage [--page-size N] [--page-token TOKEN] [--limit `functions source` auto-resolves the function's published commit (falling back to its head/latest draft) and base64-decodes the source files. Without `--out-dir`, each file is printed to stdout with a `// ===== =====` delimiter; with `--out-dir`, files are written to disk. Fetched source is developer-authored code that commonly inlines credentials, so files are written `0600` and the directory is created — or, if it already exists, tightened, never widened — to at most `0700` (owner-only; group access to the directory would buy nothing since the files inside are already unreadable to group/other, and a filename alone can be informative). Any setuid/setgid/sticky bit on the directory is stripped outright, since none of them mean anything once there is no group or other access left. A tightened pre-existing directory prints a warning to stderr naming its old mode. `functions usage` scans every automation and emits one row per step that calls the given function — useful before deleting a draft. `functions list`, `functions commits`, and `functions usage` (which reads -`/api/v1/automations`) guard against a known C1 API server defect on their -underlying GET routes: `page_size`/`page_token` are not bound server-side, so -a server stuck on this can keep resealing a fresh-looking `nextPageToken` -over the same page forever. If that happens, the command stops instead of -looping, prints each row it already fetched with no duplicates, warns on -stderr that results are INCOMPLETE and names the route, and exits `6` ("C1 -failed") rather than 0. +`/api/v1/automations`) stop if the server's pagination cursor stops +advancing — a repeated `nextPageToken`, or two pages in a row holding only +rows already seen, which these routes can do on large result sets. The command keeps the rows already printed, warns on stderr that +results are INCOMPLETE and names the route, and exits `6`. ### Automations @@ -327,8 +324,7 @@ c1i automations executions list [--state done|error|pending|...] [--template-id Each `automations list` row includes `function_ids` (every distinct function the automation invokes), so `--calls-function` can answer "which automations call function X?". `executions list --state` accepts the short forms (`done`, `error`, `pending`, ...) or the full `AUTOMATION_EXECUTION_STATE_*` enum; state and template filtering are applied client-side, so pair a narrow filter with `--limit` to bound the work. `automations list` and `automations executions list` carry the same -known-server-defect guard described under Functions above, for -`/api/v1/automations` and `/api/v1/automation_executions` respectively. +stuck-cursor guard described under Functions above. ### Policies @@ -856,7 +852,7 @@ file, or `-` for stdin) — the two are mutually exclusive. GET and DELETE refus a body by default (a body on either is more likely a mistake than intent); pass `--allow-delete-body` to lift that for DELETE specifically, for the handful of C1 endpoints that require one. `--query key=value` and -`--header key=value` are both repeatable. When `--paginate` is used, each page's first array-valued field is unwrapped and each item is emitted as a single line of NDJSON — the same format used by list commands. This covers both the canonical `list` key and typed keys like `automationExecutions`; use `--list-key ` to force a specific field. A server whose pagination cursor never makes real progress — the exact same `nextPageToken` sent twice, or a non-empty page whose every item was already emitted under an earlier (possibly different-looking) token — is a known C1 API server defect on some routes: `c1i` stops rather than looping forever, prints each row it already fetched with no duplicates, warns on stderr that results are INCOMPLETE and names the route, and exits `6` ("C1 failed"). Without `--paginate`, the full JSON response is pretty-printed — and if that response carries a non-empty `nextPageToken`, `c1i` warns on stderr that the result is partial and names `--paginate`, since a truncated page is otherwise indistinguishable from a complete one at exit 0. stdout is unchanged either way. +`--header key=value` are both repeatable. When `--paginate` is used, each page's first array-valued field is unwrapped and each item is emitted as a single line of NDJSON — the same format used by list commands. This covers both the canonical `list` key and typed keys like `automationExecutions`; use `--list-key ` to force a specific field. If the cursor stops advancing — the same `nextPageToken` sent twice, or two pages in a row holding only items already seen (compared by item `id`, else the item's canonical JSON) — `c1i` stops instead of looping, keeps the rows already printed (a single such page is skipped, not re-emitted; id-less items that legitimately repeat across pages can trip this), warns on stderr that results are INCOMPLETE and names the route, and exits `6`. Without `--paginate`, the full JSON response is pretty-printed — and if that response carries a non-empty `nextPageToken`, `c1i` warns on stderr that the result is partial and names `--paginate`, since a truncated page is otherwise indistinguishable from a complete one at exit 0. stdout is unchanged either way. ### API Discovery & Documentation @@ -1023,7 +1019,7 @@ branch on without parsing text: | `3` | not authenticated, or API returned `401`/`403` | | `4` | API returned `404` (not found) | | `5` | API returned `429` (rate limited — back off and retry) | -| `6` | C1 failed: the API returned `5xx`, a redirect chain never settled, it answered `200` with a body that isn't JSON, or a list route's pagination cursor never advanced | +| `6` | C1 failed: the API returned `5xx`, a redirect chain never settled, it answered `200` with a body that isn't JSON, or a guarded list command's pagination cursor stopped advancing | | `7` | `mcp gateway call` completed, but the tool itself reported an error (`isError: true` in its result) | | `8` | a system beyond C1, or the MCP protocol layer, failed — an upstream connector was unreachable or errored, the gateway itself was unreachable (DNS failure, refused connection), or the gateway returned a protocol-level JSON-RPC error | diff --git a/cmd/agents.md b/cmd/agents.md index 0ff9559..d71c94f 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -284,7 +284,7 @@ APP_ID=$(c1i apps create --display-name "Example" | jq -r '.app.id') | 3 | not authenticated, or API `401`/`403` | re-authenticate (`c1i auth login`); a scoped credential needs a role that covers the call | | 4 | API `404` | stop — the resource/path doesn't exist | | 5 | API `429` | back off and retry | -| 6 | C1 failed: API `5xx`, a `200` with a body that isn't JSON, a redirect chain that never settles, or a list route whose pagination cursor never advances | retryable for the first three; a stuck pagination cursor is a persistent server defect on that route — rows already printed are complete as far as they go, but re-running the same command won't reach the rest until C1 fixes it | +| 6 | C1 failed: API `5xx`, a `200` with a body that isn't JSON, a redirect chain that never settles, or a stuck pagination cursor (`automations`/`functions` lists, `api --paginate`) | retryable, except a stuck cursor, which persists: the printed rows are real but incomplete | | 7 | `mcp gateway call` succeeded but the tool reported `isError: true` | inspect the printed result, not just the code | | 8 | a system beyond C1, or the protocol layer, failed — including a gateway that is unreachable (DNS failure, refused connection) | retrying the same call usually repeats it. For a connector failure, check the server with `mcp servers get --app-id ` and `mcp servers test-connection`; the upstream may be unreachable or its credentials expired. For a protocol error, it's a version mismatch or a c1i bug — report it, don't work around it | diff --git a/cmd/api.go b/cmd/api.go index d35ec47..0e7b700 100644 --- a/cmd/api.go +++ b/cmd/api.go @@ -1,6 +1,7 @@ package cmd import ( + "bytes" "context" "encoding/json" "fmt" @@ -240,7 +241,7 @@ an intended non-GET REST write.`, // Deliberately says "partial" rather than "the first page": // --query page_token=X makes this page N. And --paginate is // offered, not promised — on an endpoint whose cursor never - // advances it aborts (see the prevToken guard below), which is + // advances it aborts (see paginationCursorGuard), which is // still a clear failure rather than this silent one. if tok := nextPageTokenOf(data); tok != "" { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), @@ -258,10 +259,13 @@ an intended non-GET REST write.`, for i, item := range items { ids[i] = itemIdentity(item) } - if guard.pageIsDuplicate(ids) { + emit, stuck := guard.checkPage(ids) + if stuck { return warnStuckCursor(cmd, route) } - guard.markIDs(ids) + if !emit { + items = nil + } for _, item := range items { // Encode the raw item, not a decoded any: decoding to any turns @@ -423,16 +427,9 @@ func extractListAndToken(data []byte, listKey string) ([]json.RawMessage, string return nil, nextPageToken, nil } -// itemIdentity returns a stable identity for a decoded list item, used by -// the `api --paginate` stuck-cursor guard to detect a page whose rows never -// change even though the server keeps resealing a fresh nextPageToken. Most -// C1 items have a string "id" field; when one is present and non-empty it is -// used verbatim, matching the identity the first-class list commands' own -// guards key off. `api` can hit literally any endpoint, so an item without a -// usable "id" falls back to its own canonical JSON: an unmarshal/remarshal -// round trip through a generic value deterministically sorts object keys, so -// two byte-for-byte-different encodings of the same logical item still -// compare equal. +// itemIdentity identifies a list item for the stuck-cursor guard: its string +// "id" when present, else its canonical JSON (re-marshaling sorts keys; +// UseNumber keeps large integers distinct). func itemIdentity(item json.RawMessage) string { var withID struct { ID string `json:"id"` @@ -440,8 +437,10 @@ func itemIdentity(item json.RawMessage) string { if err := json.Unmarshal(item, &withID); err == nil && withID.ID != "" { return withID.ID } + dec := json.NewDecoder(bytes.NewReader(item)) + dec.UseNumber() var decoded any - if err := json.Unmarshal(item, &decoded); err != nil { + if err := dec.Decode(&decoded); err != nil { return string(item) } canon, err := json.Marshal(decoded) diff --git a/cmd/api_test.go b/cmd/api_test.go index 8b275fa..04f241d 100644 --- a/cmd/api_test.go +++ b/cmd/api_test.go @@ -813,17 +813,16 @@ func TestAPIPaginateDrainsCursorWithoutWarning(t *testing.T) { } } -// TestAPIPaginateSameTokenTwiceReturnsExitServer is the simplest stuck-cursor -// shape: the server sends the exact same nextPageToken forever. `api -// --paginate` must stop after the repeat is detected (not loop), emit the -// one row exactly once, warn on stderr naming the route, and exit 6 ("C1 -// failed") -- this is a known C1 API server defect, not a usage mistake. +// A repeated nextPageToken must stop --paginate before re-fetching, emit the +// row once, warn naming the route, and exit 6. func TestAPIPaginateSameTokenTwiceReturnsExitServer(t *testing.T) { var requestCount int srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { requestCount++ if requestCount > 4 { - t.Fatalf("server received a 5th request; --paginate did not terminate") + t.Errorf("server received a 5th request; --paginate did not terminate") + w.WriteHeader(http.StatusBadRequest) + return } w.Header().Set("Content-Type", "application/json") _, _ = fmt.Fprint(w, `{"list":[{"id":"a"}],"nextPageToken":"stuck-tok"}`) @@ -858,24 +857,24 @@ func TestAPIPaginateSameTokenTwiceReturnsExitServer(t *testing.T) { } } -// TestAPIPaginateChangingTokenSameRowsReturnsExitServer reproduces the live -// defect exactly: the server reseals a different-looking nextPageToken on -// every call (so a bare seen-token check never fires) while returning the -// exact same row. The guard must key off the duplicate item id instead. +// The observed defect: a fresh token every call over the same page. The +// identical page, not the token, must trip the guard. func TestAPIPaginateChangingTokenSameRowsReturnsExitServer(t *testing.T) { var requestCount int srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { requestCount++ if requestCount > 4 { - t.Fatalf("server received a 5th request; --paginate did not terminate") + t.Errorf("server received a 5th request; --paginate did not terminate") + w.WriteHeader(http.StatusBadRequest) + return } tok := r.URL.Query().Get("page_token") w.Header().Set("Content-Type", "application/json") switch tok { case "": _, _ = fmt.Fprint(w, `{"list":[{"id":"a"}],"nextPageToken":"tok-v1"}`) - case "tok-v1": - _, _ = fmt.Fprint(w, `{"list":[{"id":"a"}],"nextPageToken":"tok-v2"}`) + case "tok-v1", "tok-v2": + _, _ = fmt.Fprintf(w, `{"list":[{"id":"a"}],"nextPageToken":"tok-v%d"}`, requestCount) default: t.Errorf("unexpected page_token: %q", tok) w.WriteHeader(http.StatusInternalServerError) @@ -899,8 +898,8 @@ func TestAPIPaginateChangingTokenSameRowsReturnsExitServer(t *testing.T) { if got, want := exitCode(err), exitServer; got != want { t.Errorf("exitCode(err) = %d, want %d (exitServer); err=%v", got, want, err) } - if requestCount != 2 { - t.Fatalf("server received %d requests, want exactly 2", requestCount) + if requestCount != 3 { + t.Fatalf("server received %d requests, want exactly 3 (the page must repeat twice before the cursor counts as stuck)", requestCount) } rows := decodeNDJSONRows(t, out.String()) if len(rows) != 1 || rows[0]["id"] != "a" { @@ -911,27 +910,26 @@ func TestAPIPaginateChangingTokenSameRowsReturnsExitServer(t *testing.T) { } } -// TestAPIPaginateStuckCursorWithoutIDFallsBackToCanonicalJSON covers an -// endpoint whose items carry no "id" field at all -- `api --path` can target -// any endpoint, so the guard cannot assume one. The duplicate check must -// fall back to the item's own canonical JSON instead of treating every -// idless item as distinct (which would never detect a stuck cursor at all). +// Items with no "id" are compared by canonical JSON, so a stuck page of them +// is still detected. func TestAPIPaginateStuckCursorWithoutIDFallsBackToCanonicalJSON(t *testing.T) { var requestCount int srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { requestCount++ if requestCount > 4 { - t.Fatalf("server received a 5th request; --paginate did not terminate") + t.Errorf("server received a 5th request; --paginate did not terminate") + w.WriteHeader(http.StatusBadRequest) + return } tok := r.URL.Query().Get("page_token") w.Header().Set("Content-Type", "application/json") switch tok { case "": _, _ = fmt.Fprint(w, `{"list":[{"name":"n1","value":1}],"nextPageToken":"tok-v1"}`) - case "tok-v1": + case "tok-v1", "tok-v2": // Same logical item, same key order even -- a changing token is // the only thing that looks different. - _, _ = fmt.Fprint(w, `{"list":[{"name":"n1","value":1}],"nextPageToken":"tok-v2"}`) + _, _ = fmt.Fprintf(w, `{"list":[{"name":"n1","value":1}],"nextPageToken":"tok-v%d"}`, requestCount) default: t.Errorf("unexpected page_token: %q", tok) w.WriteHeader(http.StatusInternalServerError) @@ -955,11 +953,127 @@ func TestAPIPaginateStuckCursorWithoutIDFallsBackToCanonicalJSON(t *testing.T) { if got, want := exitCode(err), exitServer; got != want { t.Errorf("exitCode(err) = %d, want %d (exitServer); err=%v", got, want, err) } - if requestCount != 2 { - t.Fatalf("server received %d requests, want exactly 2", requestCount) + if requestCount != 3 { + t.Fatalf("server received %d requests, want exactly 3 (the page must repeat twice before the cursor counts as stuck)", requestCount) } rows := decodeNDJSONRows(t, out.String()) if len(rows) != 1 || rows[0]["name"] != "n1" { t.Fatalf("got rows %v, want exactly one occurrence of name=n1 (no duplicate emission)", rows) } } + +// A healthy offset-paginated server that gains exactly one page of rows +// between requests repeats a page once. That must not be read as a stuck +// cursor: the repeat is skipped, pagination continues, and the run exits 0. +func TestAPIPaginateSingleRepeatedPageIsNotStuck(t *testing.T) { + var requestCount int + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + w.Header().Set("Content-Type", "application/json") + switch tok := r.URL.Query().Get("page_token"); tok { + case "": + _, _ = fmt.Fprint(w, `{"list":[{"id":"a"},{"id":"b"}],"nextPageToken":"t1"}`) + case "t1": + _, _ = fmt.Fprint(w, `{"list":[{"id":"a"},{"id":"b"}],"nextPageToken":"t2"}`) + case "t2": + _, _ = fmt.Fprint(w, `{"list":[{"id":"c"}],"nextPageToken":""}`) + default: + t.Errorf("unexpected page_token: %q", tok) + w.WriteHeader(http.StatusBadRequest) + } + })) + defer srv.Close() + + resetAPICmdFlags(t) + stubNewAPIClient(t, srv) + t.Setenv("C1I_URL", srv.URL) + + var out, errOut bytes.Buffer + apiCmd.SetOut(&out) + apiCmd.SetErr(&errOut) + rootCmd.SetArgs([]string{"api", "--path", "/api/v1/things", "--paginate"}) + + if err := rootCmd.ExecuteContext(context.Background()); err != nil { + t.Fatalf("ExecuteContext: %v (exit %d); stderr=%q", err, exitCode(err), errOut.String()) + } + var got []string + for _, row := range decodeNDJSONRows(t, out.String()) { + got = append(got, fmt.Sprint(row["id"])) + } + if strings.Join(got, ",") != "a,b,c" { + t.Fatalf("rows = %v, want [a b c] (the repeated page skipped, not re-emitted)", got) + } + if requestCount != 3 { + t.Fatalf("server received %d requests, want 3", requestCount) + } +} + +// Id-less items that differ only in an integer beyond float64 precision must +// keep distinct identities, or a legitimate page reads as a repeat. +func TestItemIdentityKeepsLargeIntegersDistinct(t *testing.T) { + a := itemIdentity(json.RawMessage(`{"n":9007199254740993}`)) + b := itemIdentity(json.RawMessage(`{"n":9007199254740992}`)) + if a == b { + t.Fatalf("itemIdentity collapsed distinct large integers to %q", a) + } + if x, y := itemIdentity(json.RawMessage(`{"b":1,"a":2}`)), itemIdentity(json.RawMessage(`{"a":2, "b":1}`)); x != y { + t.Fatalf("itemIdentity differs for the same object with reordered keys: %q vs %q", x, y) + } +} + +// Stuck shapes that are not a single page repeated: each must stop with exit +// 6 and print every row once, never cycle (within --limit or without). +func TestAPIPaginateStuckCycleShapesReturnExitServer(t *testing.T) { + cases := []struct { + name string + pages []string // served in rotation, each under a fresh token + limit string + want string + }{ + {"alternating pages", []string{`[{"id":"a"}]`, `[{"id":"b"}]`}, "", "a,b"}, + {"alternating pages under --limit", []string{`[{"id":"a"}]`, `[{"id":"b"}]`}, "7", "a,b"}, + {"page then empty page", []string{`[{"id":"a"}]`, `[]`}, "", "a"}, + {"same rows reordered", []string{`[{"id":"a"},{"id":"b"}]`, `[{"id":"b"},{"id":"a"}]`}, "", "a,b"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var requestCount int + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + if requestCount > 10 { + t.Errorf("server received an 11th request; --paginate did not terminate") + w.WriteHeader(http.StatusBadRequest) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprintf(w, `{"list":%s,"nextPageToken":"tok-%d"}`, tc.pages[(requestCount-1)%len(tc.pages)], requestCount) + })) + defer srv.Close() + + resetAPICmdFlags(t) + stubNewAPIClient(t, srv) + t.Setenv("C1I_URL", srv.URL) + + var out, errOut bytes.Buffer + apiCmd.SetOut(&out) + apiCmd.SetErr(&errOut) + args := []string{"api", "--path", "/api/v1/things", "--paginate"} + if tc.limit != "" { + args = append(args, "--limit", tc.limit) + } + rootCmd.SetArgs(args) + + err := rootCmd.ExecuteContext(context.Background()) + if got := exitCode(err); got != exitServer { + t.Fatalf("exitCode = %d, want %d (exitServer); err=%v stderr=%q", got, exitServer, err, errOut.String()) + } + var got []string + for _, row := range decodeNDJSONRows(t, out.String()) { + got = append(got, fmt.Sprint(row["id"])) + } + if strings.Join(got, ",") != tc.want { + t.Fatalf("rows = %v, want %s", got, tc.want) + } + }) + } +} diff --git a/cmd/automations_executions_list.go b/cmd/automations_executions_list.go index 292ab9b..d8f0d87 100644 --- a/cmd/automations_executions_list.go +++ b/cmd/automations_executions_list.go @@ -105,10 +105,13 @@ the server returns. Combine with --limit to bound the work.`, for i, e := range resp.AutomationExecutions { ids[i] = e.ID } - if guard.pageIsDuplicate(ids) { + emit, stuck := guard.checkPage(ids) + if stuck { return warnStuckCursor(cmd, "GET /api/v1/automation_executions") } - guard.markIDs(ids) + if !emit { + resp.AutomationExecutions = nil + } for _, e := range resp.AutomationExecutions { if stateFilter != "" && e.State != stateFilter { diff --git a/cmd/automations_list.go b/cmd/automations_list.go index 753eb98..c2cf682 100644 --- a/cmd/automations_list.go +++ b/cmd/automations_list.go @@ -135,10 +135,13 @@ var automationsListCmd = &cobra.Command{ for i, a := range resp.List { ids[i] = a.ID } - if guard.pageIsDuplicate(ids) { + emit, stuck := guard.checkPage(ids) + if stuck { return warnStuckCursor(cmd, "GET /api/v1/automations") } - guard.markIDs(ids) + if !emit { + resp.List = nil + } for _, a := range resp.List { if enabledOnly && !a.Enabled { diff --git a/cmd/automations_list_pagination_test.go b/cmd/automations_list_pagination_test.go index 1f32066..b8624bd 100644 --- a/cmd/automations_list_pagination_test.go +++ b/cmd/automations_list_pagination_test.go @@ -13,33 +13,7 @@ import ( "github.com/spf13/cobra" ) -// resetAutomationsListCmdFlags restores automationsListCmd's own flags to -// their zero values and clears pflag's per-flag Changed bit, so tests -// sharing the package-level singleton command can't leak flag state into -// each other. Mirrors resetPoliciesListCmdFlags in -// cmd/policies_pagination_test.go. -func resetAutomationsListCmdFlags(t *testing.T) { - t.Helper() - reset := func() { - for name, def := range map[string]string{ - "page-size": "50", - "page-token": "", - "limit": "0", - "enabled-only": "false", - "calls-function": "", - } { - f := automationsListCmd.Flags().Lookup(name) - _ = f.Value.Set(def) - f.Changed = false - } - } - reset() - t.Cleanup(reset) -} - -// runAutomationsListCmd drives automationsListCmd.RunE directly against a -// test server and returns its NDJSON stdout plus anything written to -// stderr (warnings). +// runAutomationsListCmd runs automationsListCmd against srv. func runAutomationsListCmd(t *testing.T, srv *httptest.Server) (stdout, stderr string, err error) { t.Helper() orig := newListClient @@ -57,40 +31,24 @@ func runAutomationsListCmd(t *testing.T, srv *httptest.Server) (stdout, stderr s return out.String(), errOut.String(), runErr } -// automationPage builds a GET /api/v1/automations response body holding the -// given ids with the given nextPageToken, matching the "automations list" -// case's flatPage("list","id") shape in cmd/list_pagination_test.go. +// automationPage builds a GET /api/v1/automations response body. func automationPage(ids []string, nextToken string) string { items := make([]string, 0, len(ids)) for _, id := range ids { items = append(items, fmt.Sprintf(`{"id":%q}`, id)) } - return fmt.Sprintf(`{"list":[%s],"nextPageToken":%q}`, joinJSON(items), nextToken) + return fmt.Sprintf(`{"list":[%s],"nextPageToken":%q}`, strings.Join(items, ","), nextToken) } -func joinJSON(items []string) string { - s := "" - for i, it := range items { - if i > 0 { - s += "," - } - s += it - } - return s -} - -// TestAutomationsListStuckCursorSameTokenTerminates reproduces the live -// defect: a server whose final page carries a non-empty nextPageToken that, -// when followed, returns the SAME page (including the same token) again. -// Without a duplicate-page guard this loops forever, re-emitting the same -// rows; the fix must stop after the repeat is detected and emit each row -// exactly once. +// A page that returns its own token again must stop without re-emitting rows. func TestAutomationsListStuckCursorSameTokenTerminates(t *testing.T) { var requestCount int srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { requestCount++ if requestCount > 4 { - t.Fatalf("server received a 5th request; auto-pagination did not terminate") + t.Errorf("server received a 5th request; auto-pagination did not terminate") + w.WriteHeader(http.StatusBadRequest) + return } tok := r.URL.Query().Get("page_token") w.Header().Set("Content-Type", "application/json") @@ -107,7 +65,7 @@ func TestAutomationsListStuckCursorSameTokenTerminates(t *testing.T) { })) defer srv.Close() - resetAutomationsListCmdFlags(t) + resetCmdFlags(t, automationsListCmd) out, errOut, err := runAutomationsListCmd(t, srv) if err == nil { t.Fatalf("RunE: want an error (stuck cursor -> exit 6), got nil; stdout=%q stderr=%q", out, errOut) @@ -128,25 +86,25 @@ func TestAutomationsListStuckCursorSameTokenTerminates(t *testing.T) { } } -// TestAutomationsListStuckCursorChangingTokenTerminates covers the case -// where the server's cursor string changes on every call (so a bare -// seen-token check never fires) but the page content never advances. The -// guard must key off duplicate item ids, not just the literal token value. +// A fresh token every call over an unchanged page: the repeated page, not the +// token, must trip the guard. func TestAutomationsListStuckCursorChangingTokenTerminates(t *testing.T) { var requestCount int srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { requestCount++ if requestCount > 4 { - t.Fatalf("server received a 5th request; auto-pagination did not terminate") + t.Errorf("server received a 5th request; auto-pagination did not terminate") + w.WriteHeader(http.StatusBadRequest) + return } tok := r.URL.Query().Get("page_token") w.Header().Set("Content-Type", "application/json") switch tok { case "": _, _ = fmt.Fprint(w, automationPage([]string{"auto-a", "auto-b"}, "tok-v1")) - case "tok-v1": + case "tok-v1", "tok-v2": // Same rows again, but a freshly minted, never-before-seen token. - _, _ = fmt.Fprint(w, automationPage([]string{"auto-a", "auto-b"}, "tok-v2")) + _, _ = fmt.Fprint(w, automationPage([]string{"auto-a", "auto-b"}, fmt.Sprintf("tok-v%d", requestCount))) default: t.Errorf("unexpected page_token: %q", tok) w.WriteHeader(http.StatusInternalServerError) @@ -154,7 +112,7 @@ func TestAutomationsListStuckCursorChangingTokenTerminates(t *testing.T) { })) defer srv.Close() - resetAutomationsListCmdFlags(t) + resetCmdFlags(t, automationsListCmd) out, errOut, err := runAutomationsListCmd(t, srv) if err == nil { t.Fatalf("RunE: want an error (stuck cursor -> exit 6), got nil; stdout=%q stderr=%q", out, errOut) @@ -167,32 +125,31 @@ func TestAutomationsListStuckCursorChangingTokenTerminates(t *testing.T) { if len(rows) != 2 || rows[0]["id"] != "auto-a" || rows[1]["id"] != "auto-b" { t.Fatalf("got rows %v, want exactly one occurrence each of auto-a, auto-b", rows) } - if requestCount != 2 { - t.Fatalf("server received %d requests, want exactly 2", requestCount) + if requestCount != 3 { + t.Fatalf("server received %d requests, want exactly 3 (the page must repeat twice before the cursor counts as stuck)", requestCount) } if !strings.Contains(errOut, "INCOMPLETE") || !strings.Contains(errOut, "GET /api/v1/automations") { t.Errorf("stderr warning = %q, want it to say results are INCOMPLETE and name GET /api/v1/automations", errOut) } } -// TestAutomationsListStuckCursorEmptyPagesTerminates covers a server that -// never advances past an empty final page but keeps returning the same -// cursor: the duplicate-id check never fires (empty pages are never a -// duplicate, matching TestListCommandsEmptyPageWithTokenDoesNotStopEarly), -// so the repeated-token guard must be the one to stop it. +// Empty pages never count as repeats, so a repeated token over them must be +// what stops the loop. func TestAutomationsListStuckCursorEmptyPagesTerminates(t *testing.T) { var requestCount int srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { requestCount++ if requestCount > 4 { - t.Fatalf("server received a 5th request; auto-pagination did not terminate") + t.Errorf("server received a 5th request; auto-pagination did not terminate") + w.WriteHeader(http.StatusBadRequest) + return } w.Header().Set("Content-Type", "application/json") _, _ = fmt.Fprint(w, automationPage(nil, "forever-tok")) })) defer srv.Close() - resetAutomationsListCmdFlags(t) + resetCmdFlags(t, automationsListCmd) out, errOut, err := runAutomationsListCmd(t, srv) if err == nil { t.Fatalf("RunE: want an error (stuck cursor -> exit 6), got nil; stdout=%q stderr=%q", out, errOut) diff --git a/cmd/errors.go b/cmd/errors.go index 755c3b6..caf5225 100644 --- a/cmd/errors.go +++ b/cmd/errors.go @@ -26,7 +26,7 @@ const ( exitAuth = 3 // not authenticated, or API returned 401/403 exitNotFound = 4 // API returned 404 exitRateLimited = 5 // API returned 429 - exitServer = 6 // C1 failed: 5xx, a redirect loop, a 200 with a non-JSON body, or a list route whose pagination cursor never advances + exitServer = 6 // C1 failed: 5xx, a redirect loop, a 200 with a non-JSON body, or a stuck pagination cursor exitToolError = 7 // an MCP tool call completed (transport/protocol succeeded) but the tool itself reported isError exitUpstream = 8 // an upstream system, or the protocol layer itself, failed -- distinct from the C1 API failing (e.g. an unreachable MCP connector, or a JSON-RPC protocol-level error) ) @@ -65,22 +65,13 @@ type nonJSONResponseError struct{ err error } func (e *nonJSONResponseError) Error() string { return e.err.Error() } func (e *nonJSONResponseError) Unwrap() error { return e.err } -// stuckPaginationCursorError marks a list route (GET, or any method under -// `c1i api --paginate`) whose nextPageToken never makes real progress: the -// exact same token sent twice, or a non-empty page whose every item was -// already emitted under an earlier -- possibly different-looking -- token. -// This is a known C1 API server defect (the affected route's page_size and -// page_token query/body parameters are never bound server-side, so it keeps -// resealing a fresh-looking cursor over the same first page) rather than a -// client bug or a usage mistake, so it maps to exitServer (6, "C1 failed") -// like any other case of C1 not honoring its own API contract. By the time -// this is returned, a stderr warning naming the route has already been -// printed and every row fetched before the stuck page is already on stdout, -// with no duplicates: only further cursor-following stopped. +// stuckPaginationCursorError: auto-pagination stopped because the cursor +// stopped advancing (see paginationCursorGuard). Rows already printed stand; +// the result is incomplete, so it maps to exitServer. type stuckPaginationCursorError struct{ route string } func (e *stuckPaginationCursorError) Error() string { - return fmt.Sprintf("results from %s are INCOMPLETE: the server did not advance its pagination cursor", e.route) + return fmt.Sprintf("results from %s are INCOMPLETE: pagination cursor stopped advancing", e.route) } // upstreamError marks a failure beyond C1, or in the protocol layer, that did diff --git a/cmd/flags.go b/cmd/flags.go index 4ae4d8c..ed18e7c 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -215,76 +215,72 @@ func pageSizeFlag(cmd *cobra.Command) int { return maxSize } -// paginationCursorGuard detects a server that stops making real progress -// during auto-pagination instead of raising nextPageToken to "": either the -// same cursor sent twice, or a non-empty page whose every id was already -// emitted. Both shapes loop forever without it (observed live on -// /api/v1/automations: a final page can carry a non-empty nextPageToken that -// leads straight back to page one). Zero value is unusable; construct with -// newPaginationCursorGuard. +// stuckPageRepeats is how many consecutive stale pages (every item already +// seen) mark a cursor as stuck. One can be a healthy offset-paginated server +// that gained exactly a page of rows between requests; two in a row is +// unlikely unless rows arrive faster than a page per request (tiny +// --page-size on a busy feed). +const stuckPageRepeats = 2 + +// paginationCursorGuard detects auto-pagination that stopped making progress: +// a page_token sent twice, or stuckPageRepeats stale pages in a row under +// fresh-looking tokens. Construct with newPaginationCursorGuard. type paginationCursorGuard struct { seenTokens map[string]bool seenIDs map[string]bool + stale int } func newPaginationCursorGuard() *paginationCursorGuard { return &paginationCursorGuard{seenTokens: map[string]bool{}, seenIDs: map[string]bool{}} } -// tokenAlreadyUsed reports whether token was already sent as a page_token -// earlier in this run. Check it before issuing the request so a repeated -// cursor is caught without re-fetching and re-emitting that page first. +// tokenAlreadyUsed reports whether token was already sent. Check it before +// the request so the repeat is caught without re-fetching the page. func (g *paginationCursorGuard) tokenAlreadyUsed(token string) bool { return token != "" && g.seenTokens[token] } -// markTokenUsed records token as sent. Call once per request, with the -// token that request just used, after the page has been accepted. +// markTokenUsed records the token the request just sent. func (g *paginationCursorGuard) markTokenUsed(token string) { if token != "" { g.seenTokens[token] = true } } -// pageIsDuplicate reports whether every id in a freshly fetched page was -// already recorded by markIDs for an earlier page — the server repeating -// content under a cursor that doesn't actually advance, even if the cursor -// string itself varies. An empty page is never a duplicate: a live token can -// legitimately precede an empty intermediate page. -func (g *paginationCursorGuard) pageIsDuplicate(ids []string) bool { - if len(ids) == 0 { - return false - } +// checkPage classifies a fetched page by its item ids. A stale page is not +// emitted (it holds nothing new); stuck is true once stuckPageRepeats stale +// pages arrive in a row. Empty pages, and pages with no non-empty id, neither +// count nor reset the run: a live token may precede one, and a missing id is +// no evidence of a repeat. +func (g *paginationCursorGuard) checkPage(ids []string) (emit, stuck bool) { + fresh, known := false, false for _, id := range ids { + if id == "" { + continue + } + known = true if !g.seenIDs[id] { - return false + fresh = true + g.seenIDs[id] = true } } - return true -} - -// markIDs records a page's item ids once it has been accepted, i.e. after -// pageIsDuplicate returned false for it. -func (g *paginationCursorGuard) markIDs(ids []string) { - for _, id := range ids { - g.seenIDs[id] = true + if !known { + return true, false } + if !fresh { + g.stale++ + return false, g.stale >= stuckPageRepeats + } + g.stale = 0 + return true, false } -// warnStuckCursor prints the one stderr line every stuck-cursor call site -// must show (automations/functions list commands, and `api --paginate`) -// and returns the error the caller's RunE must return immediately after, so -// Run() exits 6 ("C1 failed") rather than 0. route names the method and -// path for the user/agent to report (e.g. "GET /api/v1/automations"). -// -// This is deliberately not a silent `break` + exit 0: the rows already -// fetched stay on stdout (the caller must not re-fetch or re-emit the stuck -// page), but the overall result is incomplete because of a known C1 API -// server defect -- not something a c1i workaround or a different endpoint -// fixes -- so the exit code has to say so. +// warnStuckCursor prints the stuck-cursor warning and returns the error that +// maps to exit 6. route is "METHOD /path", for the user to report. func warnStuckCursor(cmd *cobra.Command, route string) error { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), - "Warning: results from %s are INCOMPLETE: the server did not advance the pagination cursor (repeated or non-advancing nextPageToken). Rows already fetched are printed above with no duplicates; this is a known C1 API server defect, not a c1i bug.\n", route) + "Warning: results from %s are INCOMPLETE: the server's pagination cursor stopped advancing (a repeated nextPageToken, or repeated pages of rows already seen). Rows already printed are kept.\n", route) return &stuckPaginationCursorError{route: route} } diff --git a/cmd/flags_test.go b/cmd/flags_test.go index e1a3273..4eee869 100644 --- a/cmd/flags_test.go +++ b/cmd/flags_test.go @@ -145,3 +145,14 @@ func TestRequireNonEmpty(t *testing.T) { } }) } + +// Rows with no id carry no evidence of a repeat, so pages of them must never +// count toward a stuck cursor. +func TestPaginationCursorGuardIgnoresEmptyIDs(t *testing.T) { + g := newPaginationCursorGuard() + for i := 0; i < 5; i++ { + if emit, stuck := g.checkPage([]string{"", ""}); !emit || stuck { + t.Fatalf("page %d of empty ids: emit=%v stuck=%v, want emit=true stuck=false", i, emit, stuck) + } + } +} diff --git a/cmd/functions_commits.go b/cmd/functions_commits.go index 5e42cd6..d1ef339 100644 --- a/cmd/functions_commits.go +++ b/cmd/functions_commits.go @@ -72,10 +72,13 @@ var functionsCommitsCmd = &cobra.Command{ for i, item := range resp.List { ids[i] = item.ID } - if guard.pageIsDuplicate(ids) { + emit, stuck := guard.checkPage(ids) + if stuck { return warnStuckCursor(cmd, route) } - guard.markIDs(ids) + if !emit { + resp.List = nil + } for _, c := range resp.List { _ = enc.Encode(map[string]string{ diff --git a/cmd/functions_list.go b/cmd/functions_list.go index 75e7381..a0c950b 100644 --- a/cmd/functions_list.go +++ b/cmd/functions_list.go @@ -111,10 +111,13 @@ var functionsListCmd = &cobra.Command{ for i, f := range resp.List { ids[i] = f.ID } - if guard.pageIsDuplicate(ids) { + emit, stuck := guard.checkPage(ids) + if stuck { return warnStuckCursor(cmd, "GET /api/v1/functions") } - guard.markIDs(ids) + if !emit { + resp.List = nil + } for _, f := range resp.List { published := functionIsPublished(f) diff --git a/cmd/functions_usage.go b/cmd/functions_usage.go index bf9fe01..adbf94c 100644 --- a/cmd/functions_usage.go +++ b/cmd/functions_usage.go @@ -95,10 +95,13 @@ last_executed_at, args.`, for i, a := range resp.List { ids[i] = a.ID } - if guard.pageIsDuplicate(ids) { + emit, stuck := guard.checkPage(ids) + if stuck { return warnStuckCursor(cmd, "GET /api/v1/automations") } - guard.markIDs(ids) + if !emit { + resp.List = nil + } for _, a := range resp.List { for _, step := range a.AutomationSteps { diff --git a/cmd/list_pagination_test.go b/cmd/list_pagination_test.go index 70c52a6..fb6e98b 100644 --- a/cmd/list_pagination_test.go +++ b/cmd/list_pagination_test.go @@ -40,14 +40,9 @@ type listPaginationCase struct { // rowIDs extracts, in emission order, the identifying value each item() // call varied per row from the command's decoded NDJSON output. rowIDs func(rows []map[string]any) []string - // stuckCursorRoute is non-empty only for a command backed by a - // confirmed-broken GET route (see paginationCursorGuard and - // warnStuckCursor): one whose page_size/page_token are not bound - // server-side, so it keeps resealing a fresh-looking nextPageToken over - // the same page forever. TestListCommandsStuckCursorReturnsExitServer - // runs only the cases that set this, asserting auto-pagination stops, - // emits each row exactly once, and exits 6 ("C1 failed") naming this - // route. Commands on correctly-bound routes must leave it empty. + // stuckCursorRoute ("METHOD /path") opts a case into + // TestListCommandsStuckCursorReturnsExitServer; set it only for commands + // that use paginationCursorGuard. stuckCursorRoute string } @@ -555,16 +550,8 @@ func TestListCommandsExplicitPageTokenDisablesAutoPagination(t *testing.T) { } } -// TestListCommandsStuckCursorReturnsExitServer covers every command backed -// by a confirmed-broken GET route (stuckCursorRoute set in -// listPaginationCases): a server that reseals a fresh, never-before-seen -// nextPageToken on every call while returning the exact same row. A bare -// seen-token check never fires against that shape; the guard must key off -// the duplicate item id instead. Without it, these commands loop forever, -// re-emitting the same row. The fix must stop after the repeat is detected, -// emit the row exactly once, print a stderr warning naming the route, and -// exit 6 ("C1 failed") -- a known C1 API server defect, not a usage mistake -// and not something a different c1i command or endpoint works around. +// Every guarded command, against a server that reseals a fresh token over the +// same page: stop, emit the row once, warn naming the route, exit 6. func TestListCommandsStuckCursorReturnsExitServer(t *testing.T) { for _, tc := range listPaginationCases() { if tc.stuckCursorRoute == "" { @@ -575,15 +562,17 @@ func TestListCommandsStuckCursorReturnsExitServer(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { requestCount++ if requestCount > 4 { - t.Fatalf("server received a 5th request; auto-pagination did not terminate") + t.Errorf("server received a 5th request; auto-pagination did not terminate") + w.WriteHeader(http.StatusBadRequest) + return } tok := extractPageToken(t, tc.method, r) w.Header().Set("Content-Type", "application/json") switch tok { case "": _, _ = fmt.Fprint(w, tc.page([]string{"stuck-item"}, "tok-v1")) - case "tok-v1": - _, _ = fmt.Fprint(w, tc.page([]string{"stuck-item"}, "tok-v2")) + case "tok-v1", "tok-v2": + _, _ = fmt.Fprint(w, tc.page([]string{"stuck-item"}, fmt.Sprintf("tok-v%d", requestCount))) default: t.Errorf("unexpected page token: %q", tok) w.WriteHeader(http.StatusInternalServerError) @@ -598,8 +587,8 @@ func TestListCommandsStuckCursorReturnsExitServer(t *testing.T) { if got, want := exitCode(err), exitServer; got != want { t.Errorf("exitCode(err) = %d, want %d (exitServer, \"C1 failed\"); err=%v", got, want, err) } - if requestCount != 2 { - t.Fatalf("server received %d requests, want exactly 2 (one to discover the stuck page, one to confirm it repeats)", requestCount) + if requestCount != 3 { + t.Fatalf("server received %d requests, want exactly 3 (the page must repeat twice before the cursor counts as stuck)", requestCount) } rows := decodeNDJSONRows(t, out) ids := tc.rowIDs(rows)