From 0429051202131f8585e3d0a142df620603f8625a Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Wed, 24 Jun 2026 19:31:29 +0500 Subject: [PATCH 1/2] fix(api): error on unparseable pagination next, don't silently truncate (#106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ListClients followed DRF `next` via nextPath, which returned "" for BOTH an empty link (end of pages) and an unparseable one — so a non-empty `next` the server sends that url.Parse rejects silently ended the loop, and ListClients returned only the pages seen so far with a nil error. list / `use` / namespace-collision checks would then miss clients with no signal. nextPath now returns (string, error): "" + nil for an empty link, an error for a non-empty link that won't parse. Trigger is unlikely (DRF emits well-formed URLs, url.Parse is lenient), but the failure mode — silent partial list — is the wrong one for a correctness-sensitive call. Tests: pagination still followed end-to-end (page 1 → 2 → done); an unparseable next link is now a hard error, not a truncation. Bugbot: 8dadb5c2-804a-48ed-bc81-eb14e6317be1 (v0.4.0 RC, #107) Co-Authored-By: Claude Opus 4.8 --- internal/api/client.go | 22 ++++++++++++-------- internal/api/client_test.go | 40 +++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/internal/api/client.go b/internal/api/client.go index fbd97bb6..5ff51cc9 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -346,26 +346,32 @@ func (c *Client) ListClients(ctx context.Context) ([]ProvisionedClient, error) { return nil, fmt.Errorf("decoding client list: %w", err) } all = append(all, body.Results...) - path = nextPath(body.Next) + next, perr := nextPath(body.Next) + if perr != nil { + return nil, perr + } + path = next } return all, nil } // nextPath reduces a DRF `next` link (an absolute URL) to the path+query this -// client appends to BaseURL. Returns "" for an empty/unparseable link, which -// ends the pagination loop. -func nextPath(next string) string { +// client appends to BaseURL. An empty link returns ("", nil) — the normal +// end of pages. A non-empty link that won't parse is an error, NOT a silent +// "", so the loop never quietly stops mid-list and returns only the pages +// seen so far (list / `use` / collision checks must see every client). +func nextPath(next string) (string, error) { if next == "" { - return "" + return "", nil } u, err := url.Parse(next) if err != nil { - return "" + return "", fmt.Errorf("client list: unparseable pagination link %q: %w", next, err) } if u.RawQuery != "" { - return u.Path + "?" + u.RawQuery + return u.Path + "?" + u.RawQuery, nil } - return u.Path + return u.Path, nil } // ListClientAdmins returns who in the account can provision (the ask-an-admin diff --git a/internal/api/client_test.go b/internal/api/client_test.go index f5fbdcd1..8cad599f 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -194,3 +194,43 @@ func TestCreateClientConflict(t *testing.T) { t.Errorf("want APIError 409, got %v", err) } } + +// TestListClients_FollowsPagination guards that DRF pagination is still +// followed end-to-end after the nextPath refactor (page 1 → page 2 → done). +func TestListClients_FollowsPagination(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Query().Get("page") == "2" { + _, _ = w.Write([]byte(`{"next":"","results":[{"id":2,"first_name":"b","namespace":"b"}]}`)) + return + } + // DRF emits an absolute `next`; nextPath keeps only path+query. + _, _ = w.Write([]byte(`{"next":"http://x/edge-device/?page=2","results":[{"id":1,"first_name":"a","namespace":"a"}]}`)) + })) + defer srv.Close() + c := New("prod") + c.BaseURL = srv.URL + got, err := c.ListClients(context.Background()) + if err != nil { + t.Fatal(err) + } + if len(got) != 2 || got[0].ID != 1 || got[1].ID != 2 { + t.Fatalf("want 2 clients [1,2], got %+v", got) + } +} + +// TestListClients_UnparseableNextLink_IsError pins the Bugbot fix (v0.4.0 RC): +// a non-empty `next` the server sends that url.Parse rejects must be a hard +// error, never a silent truncation to the pages seen so far — otherwise list / +// `use` / namespace-collision checks would miss clients without any error. +func TestListClients_UnparseableNextLink_IsError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // `next` carries a control byte (\u007f) → url.Parse fails. + _, _ = w.Write([]byte(`{"next":"http://x/\u007f","results":[{"id":1,"first_name":"a","namespace":"a"}]}`)) + })) + defer srv.Close() + c := New("prod") + c.BaseURL = srv.URL + if _, err := c.ListClients(context.Background()); err == nil { + t.Fatal("expected an error on an unparseable next link, got nil (silent truncation)") + } +} From e14685f3043a3722fe2e7a764d37191657b623b9 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Wed, 24 Jun 2026 19:31:30 +0500 Subject: [PATCH 2/2] fix(cli): route all known-but-unsupported categories to the pending note (#106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dataset-push category gate only special-cased known-but-unsupported *image* categories (`case push.IsImage`), so a registry-known non-image category that isn't CLI-supported yet — `causal_language_modeling` (FamilyText, CLISupported:false, with a real UnsupportedNote) — fell to the default branch and was reported as "isn't a recognized task category". It IS recognized; it's pending support. Swap the gate to `case push.IsKnown`: supported categories are already caught by the prior case, so IsKnown here means known-but-unsupported (image or text), all routed through the registry's per-category pending-support note. The default branch is left for genuinely unknown/typo'd categories. Message-only (exit code was already 2). Test: causal_language_modeling now gets the pending-support note, not the unrecognized-category message. (The existing exit-2 test didn't assert the message, which is how this slipped through.) Bugbot: 16f5b945-5d67-4201-8bc6-1f6baf633672 (v0.4.0 RC, #107) Co-Authored-By: Claude Opus 4.8 --- internal/cli/dataset.go | 11 +++++++---- internal/cli/dataset_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/internal/cli/dataset.go b/internal/cli/dataset.go index 8cb40baf..012fd2cf 100644 --- a/internal/cli/dataset.go +++ b/internal/cli/dataset.go @@ -423,10 +423,13 @@ contributors train against it without ever seeing the raw files.`)) // "category is required" error downstream. case push.IsCLISupported(a.Spec.Category): // supported - case push.IsImage(a.Spec.Category): - // A known image category dataset push doesn't implement yet - // (semantic_segmentation / instance_segmentation). The per-category - // reason + the supported list both come from the registry. + case push.IsKnown(a.Spec.Category): + // A recognized category dataset push doesn't implement yet — image + // (semantic_segmentation / instance_segmentation) or text + // (causal_language_modeling). Routed here (not the default branch) so the + // user gets the registry's per-category pending-support reason, not a + // misleading "unrecognized category". Supported categories were already + // caught above, so IsKnown here means known-but-unsupported. spec, _ := push.Lookup(a.Spec.Category) return &exitError{code: 2, err: fmt.Errorf( "category %q isn't supported by the CLI yet (%s). Supported categories: %s.", diff --git a/internal/cli/dataset_test.go b/internal/cli/dataset_test.go index 5d5cfcfd..b8415b5c 100644 --- a/internal/cli/dataset_test.go +++ b/internal/cli/dataset_test.go @@ -93,6 +93,37 @@ func TestDatasetPush_UnsupportedCategory_ExitsTwo(t *testing.T) { } } +// TestDatasetPush_KnownUnsupportedCategory_PendingNote pins the Bugbot fix +// (v0.4.0 RC): a registry-known but CLI-unsupported NON-image category +// (causal_language_modeling) must get the registry's pending-support note, not +// the misleading "isn't a recognized task category" message. execDatasetPush +// discards the error and SilenceErrors swallows it, so run the command here and +// inspect the returned error directly. +func TestDatasetPush_KnownUnsupportedCategory_PendingNote(t *testing.T) { + root := imgcLayout(t) + rootCmd := NewRootCmd(BuildInfo{Version: "test"}) + rootCmd.SetOut(&bytes.Buffer{}) + rootCmd.SetErr(&bytes.Buffer{}) + rootCmd.SetArgs([]string{"dataset", "push", + "--kubeconfig=/tmp/tracebloc-cli-test-nonexistent-" + t.Name(), + root, "--table=t1", "--category=causal_language_modeling", + "--intent=train", "--label-column=label"}) + err := rootCmd.Execute() + if err == nil { + t.Fatal("expected an error for a known-but-unsupported category") + } + if got := ExitCodeFromError(err); got != 2 { + t.Fatalf("exit code = %d, want 2", got) + } + msg := err.Error() + if strings.Contains(msg, "isn't a recognized task category") { + t.Errorf("known category misrouted to the unrecognized-category branch:\n%s", msg) + } + if !strings.Contains(msg, "isn't supported by the CLI yet") { + t.Errorf("want the registry pending-support note, got:\n%s", msg) + } +} + // TestDatasetPush_TraversalTableName_ExitsTwo is the security // regression pin at the CLI layer. --table=../../etc must be // rejected with exit 2 BEFORE any spec synthesis or cluster work —