From 5229f3e72e4ad64b32fd8b044ade19251b08ccf3 Mon Sep 17 00:00:00 2001 From: Kush Date: Fri, 21 Aug 2026 15:23:17 -0500 Subject: [PATCH 01/15] feat(tendlc): add phone number service methods --- internal/tendlc/numbers.go | 64 ++++++++++++++++++ internal/tendlc/numbers_test.go | 115 ++++++++++++++++++++++++++++++++ 2 files changed, 179 insertions(+) create mode 100644 internal/tendlc/numbers.go create mode 100644 internal/tendlc/numbers_test.go diff --git a/internal/tendlc/numbers.go b/internal/tendlc/numbers.go new file mode 100644 index 0000000..2f44fd9 --- /dev/null +++ b/internal/tendlc/numbers.go @@ -0,0 +1,64 @@ +package tendlc + +import ( + "fmt" + "net/url" + + "github.com/Bandwidth/cli/internal/api" +) + +// ListPhoneNumbers returns the phone numbers on the account. The list +// projection has five keys — createdDate, modifiedDate, nnid, phoneNumber, +// status — notably no campaignId. +// +// filters is accepted only for signature consistency with ListBrands and +// ListCampaigns. Measured against production, filtering does not work on +// this endpoint: status[eq] is silently ignored (an account with 21 SUCCESS +// and 2 FAILURE phone numbers returned all 23 for status[eq]=SUCCESS, +// status[eq]=FAILED, and status[eq]=NOT_A_STATUS alike), and +// campaignId[contains] is evaluated but matches nothing, for a real +// campaign ID and for garbage alike. No caller should pass a filter here, +// and the command layer offers no flags for one. +func (s *Service) ListPhoneNumbers(limit, offset int, filters []api.Filter) (*api.Envelope, error) { + return s.get(s.base() + "/phoneNumbers" + api.EncodeQuery(limit, offset, filters)) +} + +// GetPhoneNumber returns one phone number. +// +// Measured against production: this endpoint returned 404 for all four +// numbers tested, while PhoneNumberHistory on the same path prefix returned +// 200 for all four. The cause is unconfirmed — only one account was +// available to test against, and this API reports authorization failures as +// 403, so a 404 here is not a permissions mask in disguise. The currently +// shipped `band tendlc number ` command already fails the same way. +func (s *Service) GetPhoneNumber(phoneNumber string) (*api.Envelope, error) { + if phoneNumber == "" { + return nil, fmt.Errorf("phone number is required") + } + return s.get(s.phoneNumberPath(phoneNumber)) +} + +// PhoneNumberHistory returns the phone number's activity log: free-text +// {createdDate, message} entries, newest first. As with BrandHistory and +// CampaignHistory there are no versioned snapshots and no per-version fetch. +func (s *Service) PhoneNumberHistory(phoneNumber string, limit, offset int) (*api.Envelope, error) { + if phoneNumber == "" { + return nil, fmt.Errorf("phone number is required") + } + return s.get(s.phoneNumberPath(phoneNumber) + "/history" + api.EncodeQuery(limit, offset, nil)) +} + +// phoneNumberPath builds /phoneNumbers/{tn}. +// +// Phone numbers are E.164 and carry a leading '+'. url.PathEscape leaves '+' +// unescaped — verified directly: url.PathEscape("+15555550100") returns +// "+15555550100", not "%2B15555550100". That is correct: '+' is a valid +// sub-delimiter in a path segment, so this is not a bug to "fix" by +// switching to url.QueryEscape, which would encode it as %2B where it does +// not belong. Measured against production, a raw '+' and a pre-encoded %2B +// reach the server identically. PathEscape is called anyway, for +// consistency with brandPath/campaignPath and because it still needs to +// escape whatever isn't a '+'. +func (s *Service) phoneNumberPath(phoneNumber string) string { + return s.base() + "/phoneNumbers/" + url.PathEscape(phoneNumber) +} diff --git a/internal/tendlc/numbers_test.go b/internal/tendlc/numbers_test.go new file mode 100644 index 0000000..f48925c --- /dev/null +++ b/internal/tendlc/numbers_test.go @@ -0,0 +1,115 @@ +package tendlc + +import "testing" + +func TestListPhoneNumbersEncodesPagination(t *testing.T) { + var got captured + s := stubService(t, 200, `{"data":[],"page":{"totalElements":0}}`, &got) + + if _, err := s.ListPhoneNumbers(10, 20, nil); err != nil { + t.Fatalf("ListPhoneNumbers: %v", err) + } + if got.method != "GET" { + t.Errorf("method = %q, want GET", got.method) + } + if want := "/api/v2/accounts/9901287/tendlc/phoneNumbers"; got.path != want { + t.Errorf("path = %q, want %q", got.path, want) + } + if got.query != "limit=10&offset=20" { + t.Errorf("query = %q, want limit=10&offset=20", got.query) + } +} + +func TestGetPhoneNumberGetsToPhoneNumberPath(t *testing.T) { + var got captured + s := stubService(t, 200, `{"data":{"phoneNumber":"+15555550100"}}`, &got) + + env, err := s.GetPhoneNumber("+15555550100") + if err != nil { + t.Fatalf("GetPhoneNumber: %v", err) + } + if got.method != "GET" { + t.Errorf("method = %q, want GET", got.method) + } + if want := "/api/v2/accounts/9901287/tendlc/phoneNumbers/+15555550100"; got.path != want { + t.Errorf("path = %q, want %q", got.path, want) + } + obj, err := env.Object() + if err != nil { + t.Fatalf("Object: %v", err) + } + if obj["phoneNumber"] != "+15555550100" { + t.Errorf("phoneNumber = %v, want +15555550100", obj["phoneNumber"]) + } +} + +func TestPhoneNumberHistoryEncodesPagination(t *testing.T) { + var got captured + s := stubService(t, 200, `{"data":[],"page":{"totalElements":0}}`, &got) + + if _, err := s.PhoneNumberHistory("+15555550100", 10, 20); err != nil { + t.Fatalf("PhoneNumberHistory: %v", err) + } + if got.method != "GET" { + t.Errorf("method = %q, want GET", got.method) + } + if want := "/api/v2/accounts/9901287/tendlc/phoneNumbers/+15555550100/history"; got.path != want { + t.Errorf("path = %q, want %q", got.path, want) + } + if got.query != "limit=10&offset=20" { + t.Errorf("query = %q, want limit=10&offset=20", got.query) + } +} + +// Every method that takes a phone number must reject an empty one before +// making a request. Without this a caller with an unset variable silently +// hits the collection endpoint — GET on /phoneNumbers rather than +// /phoneNumbers/{tn}. +func TestEmptyPhoneNumbersRejectedWithoutRequest(t *testing.T) { + var got captured + s := stubService(t, 200, `{"data":{}}`, &got) + + calls := map[string]func() error{ + "GetPhoneNumber": func() error { _, err := s.GetPhoneNumber(""); return err }, + "PhoneNumberHistory": func() error { + _, err := s.PhoneNumberHistory("", 10, 0) + return err + }, + } + for name, call := range calls { + t.Run(name, func(t *testing.T) { + got = captured{} + if err := call(); err == nil { + t.Fatal("want an error for an empty phone number, got nil") + } + if got.method != "" { + t.Errorf("a request was made (%s %s); want none", got.method, got.path) + } + }) + } +} + +// A phone number goes into the path, so a value containing a slash or a +// space must be escaped rather than silently changing which endpoint is +// called. The leading '+' in a real E.164 number is left alone by +// url.PathEscape (see phoneNumberPath), so a value with just a space is not +// enough to prove this test has teeth: net/url's EscapedPath derives its own +// canonical encoding from the decoded Path whenever RawPath isn't already in +// that exact form, so an unescaped space gets "corrected" to %20 on the way +// out regardless of whether url.PathEscape ran. A slash does not get that +// treatment — an unescaped '/' is a real path separator (an extra segment), +// while an escaped one is %2F, so only the slash form actually distinguishes +// "escaped" from "not escaped". Assert on got.escapedPath, not got.path: +// net/url decodes Path, so an assertion against it would pass whether or not +// url.PathEscape was called. +func TestPhoneNumberIsPathEscaped(t *testing.T) { + var got captured + s := stubService(t, 200, `{"data":[],"page":{"totalElements":0}}`, &got) + + if _, err := s.PhoneNumberHistory("+1/555 0100", 10, 0); err != nil { + t.Fatalf("PhoneNumberHistory: %v", err) + } + if want := "/api/v2/accounts/9901287/tendlc/phoneNumbers/+1%2F555%200100/history"; got.escapedPath != want { + t.Errorf("escaped path = %q, want %q", got.escapedPath, want) + } +} From 5e805a63428723137793e547b1d886fe747cbc99 Mon Sep 17 00:00:00 2001 From: Kush Date: Fri, 21 Aug 2026 16:46:43 -0500 Subject: [PATCH 02/15] feat(tendlc): add band tendlc number list, get, and history --- cmd/tendlc/number.go | 211 ++++++++++++++++++++++++++ cmd/tendlc/number_test.go | 309 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 520 insertions(+) create mode 100644 cmd/tendlc/number.go create mode 100644 cmd/tendlc/number_test.go diff --git a/cmd/tendlc/number.go b/cmd/tendlc/number.go new file mode 100644 index 0000000..7a6480a --- /dev/null +++ b/cmd/tendlc/number.go @@ -0,0 +1,211 @@ +package tendlc + +import ( + "github.com/spf13/cobra" + + "github.com/Bandwidth/cli/internal/api" + "github.com/Bandwidth/cli/internal/cmdutil" + "github.com/Bandwidth/cli/internal/output" +) + +var ( + numberListLimit int + numberListOffset int + numberListAll bool + + numberHistoryLimit int + numberHistoryOffset int + numberHistoryAll bool +) + +func init() { + lf := numberListCmd.Flags() + lf.IntVar(&numberListLimit, "limit", 50, "Page size") + lf.IntVar(&numberListOffset, "offset", 0, "Pagination offset") + lf.BoolVar(&numberListAll, "all", false, "Fetch every page (cannot be combined with --offset)") + + hf := numberHistoryCmd.Flags() + hf.IntVar(&numberHistoryLimit, "limit", 50, "Page size") + hf.IntVar(&numberHistoryOffset, "offset", 0, "Pagination offset") + hf.BoolVar(&numberHistoryAll, "all", false, "Fetch every page (cannot be combined with --offset)") + + // numberGetCmd (Use: "number ") is declared in numbers.go -- + // the legacy flat `band tendlc number ` command, deleted in Task 3, + // not here. It is already registered under Cmd by numbers.go's own + // init(). A second, sibling command also named "number" would collide: + // cobra's Command.Find matches children by Name() and returns the FIRST + // one added to the parent's command slice, with no tie-break on Args or + // anything else. Go initializes files within a package in filename + // order, so this file's init() would run before numbers.go's, and + // `Cmd.AddCommand(numberCmd)` here would always win the race -- silently + // shadowing the legacy command's `Cmd.AddCommand(numberGetCmd)` call + // below it, so a bare `band tendlc number +15555550100` would resolve to + // this file's parent, find no subcommand named "+15555550100", and print + // this command's help instead of running the legacy lookup. Verified + // against a cobra sandbox before writing this. + // + // Attaching these three subcommands directly onto the existing + // numberGetCmd node avoids that entirely: no second "number" command is + // ever registered. Cobra falls through to a parent's own Args/RunE + // whenever the next token isn't a known child's name, so + // `band tendlc number ` keeps invoking the legacy runNumberGet + // unchanged, while `list`, `get `, and `history ` route to the + // commands below. When Task 3 deletes numbers.go, replace numberGetCmd + // here with a plain `numberCmd` parent (Use: "number") and re-register + // it on Cmd directly. + numberGetCmd.AddCommand(numberListCmd, numberDetailCmd, numberHistoryCmd) +} + +var numberListCmd = &cobra.Command{ + Use: "list", + Short: "List 10DLC registered phone numbers", + Long: `Lists the phone numbers registered for 10DLC traffic on the account. + +There are no filter flags -- not --status, not --campaign-id. None. That is +deliberate and measured, not an oversight: on an account holding 21 SUCCESS +and 2 FAILURE phone numbers, status[eq] was silently ignored and returned +all 23 records for status[eq]=SUCCESS, status[eq]=FAILED, and even +status[eq]=NOT_A_STATUS alike, and campaignId[contains] was evaluated but +matched nothing -- 0 results for a real campaign ID and for deliberate +garbage alike. The API does not support filtering on this endpoint. A +filter that returns every record, or none, with a 200 and no error is worse +than an absent flag, because the caller believes it worked -- the same +reasoning that already removed 'brand list --bandwidth-id' and +'campaign list --usecase'. + +For the campaign-scoped view, which is a different endpoint and does work, +use 'band tendlc campaign numbers ' instead. + +This is a summary projection -- five keys per number: createdDate, +modifiedDate, nnid, phoneNumber, status. Notably absent: campaignId.`, + Example: ` band tendlc number list --plain + band tendlc number list --all --plain`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + // Detected via Changed so that an explicit --offset 0 also conflicts. + if numberListAll && cmd.Flags().Changed("offset") { + return cmdutil.NewFlagError("--all fetches every page, so it cannot be combined with --offset") + } + svc, err := service(cmd) + if err != nil { + return err + } + format, plain := cmdutil.OutputFlags(cmd) + + if !numberListAll { + env, err := svc.ListPhoneNumbers(numberListLimit, numberListOffset, nil) + if err != nil { + return roleGateError(err, "Campaign Management") + } + items, err := env.List() + if err != nil { + return err + } + warnIfTruncated(cmd, env, numberListOffset, len(items), "phone numbers") + return output.StdoutPlainList(format, plain, items) + } + + var all []any + err = api.ForEachPage(func(limit, offset int) (*api.Envelope, error) { + return svc.ListPhoneNumbers(limit, offset, nil) + }, numberListLimit, func(batch []any) error { + all = append(all, batch...) + return nil + }) + if err != nil { + return roleGateError(err, "Campaign Management") + } + if all == nil { + all = []any{} + } + return output.StdoutPlainList(format, plain, all) + }, +} + +// numberDetailCmd implements `band tendlc number get `. Named +// numberDetailCmd, not numberGetCmd, because numbers.go already declares +// numberGetCmd for the legacy flat `number ` command this is attached +// beneath -- see this file's init(). +var numberDetailCmd = &cobra.Command{ + Use: "get ", + Short: "Get 10DLC registration details for a phone number", + Long: `Shows one phone number's 10DLC registration record. + +Shipped plainly, with no special-case error handling: a 404 maps to exit 3 +through the normal path. On the one account this was tested against, this +endpoint returned 404 for every number tried, while 'number history' on the +same path prefix returned 200 for all of them. The cause is unconfirmed and +may be account-specific -- this API reports authorization failures as 403, +so a 404 here is not a permissions mask in disguise, but one account isn't +enough to call it an API defect either. The command starts working +wherever the endpoint does, without a bespoke error message guessing at why.`, + Example: ` band tendlc number get +15555550100 --plain`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + svc, err := service(cmd) + if err != nil { + return err + } + env, err := svc.GetPhoneNumber(args[0]) + if err != nil { + return roleGateError(err, "Campaign Management") + } + obj, err := env.Object() + if err != nil { + return err + } + format, plain := cmdutil.OutputFlags(cmd) + return output.StdoutAuto(format, plain, obj) + }, +} + +var numberHistoryCmd = &cobra.Command{ + Use: "history ", + Short: "Show a phone number's activity log", + Long: `Lists a phone number's activity log: free-text {createdDate, message} +entries, newest first. + +As with brand and campaign history, there are no versioned snapshots and no +per-entry fetch -- this is the only history view for a phone number.`, + Example: ` band tendlc number history +15555550100 --plain + band tendlc number history +15555550100 --all --plain`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if numberHistoryAll && cmd.Flags().Changed("offset") { + return cmdutil.NewFlagError("--all fetches every page, so it cannot be combined with --offset") + } + svc, err := service(cmd) + if err != nil { + return err + } + format, plain := cmdutil.OutputFlags(cmd) + + if !numberHistoryAll { + env, err := svc.PhoneNumberHistory(args[0], numberHistoryLimit, numberHistoryOffset) + if err != nil { + return roleGateError(err, "Campaign Management") + } + items, err := env.List() + if err != nil { + return err + } + warnIfTruncated(cmd, env, numberHistoryOffset, len(items), "history entries") + return output.StdoutPlainList(format, plain, items) + } + + var all []any + err = api.ForEachPage(func(limit, offset int) (*api.Envelope, error) { + return svc.PhoneNumberHistory(args[0], limit, offset) + }, numberHistoryLimit, func(batch []any) error { + all = append(all, batch...) + return nil + }) + if err != nil { + return roleGateError(err, "Campaign Management") + } + if all == nil { + all = []any{} + } + return output.StdoutPlainList(format, plain, all) + }, +} diff --git a/cmd/tendlc/number_test.go b/cmd/tendlc/number_test.go new file mode 100644 index 0000000..8dd4492 --- /dev/null +++ b/cmd/tendlc/number_test.go @@ -0,0 +1,309 @@ +package tendlc + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/Bandwidth/cli/internal/cmdutil" +) + +// stubNumberList answers any request with one phone number on a single, +// non-truncated page. Good enough for tests that just need number list to +// succeed. +func stubNumberList(t *testing.T) *httptest.Server { + return newBrandStub(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"data":[{"phoneNumber":"+15555550100","status":"SUCCESS"}],` + + `"page":{"pageNumber":0,"pageSize":50,"totalElements":1,"totalPages":1}}`)) + }) +} + +// stubNumberListTruncated answers with a page that reports more records +// exist than were returned, so warnIfTruncated fires. +func stubNumberListTruncated(t *testing.T) *httptest.Server { + return newBrandStub(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"data":[{"phoneNumber":"+15555550100"}],` + + `"page":{"pageNumber":0,"pageSize":1,"totalElements":5,"totalPages":5}}`)) + }) +} + +// stubNumberListTwoPages serves genuinely different items on page one +// (offset 0) versus page two (offset 1), keyed off the request's offset +// query param. totalElements=2 with pageSize=1 forces api.ForEachPage to +// fetch both pages under --all --limit 1. +func stubNumberListTwoPages(t *testing.T) *httptest.Server { + return newBrandStub(t, func(w http.ResponseWriter, r *http.Request) { + offset := r.URL.Query().Get("offset") + if offset == "" || offset == "0" { + _, _ = w.Write([]byte(`{"data":[{"phoneNumber":"+15555550100"}],` + + `"page":{"pageNumber":0,"pageSize":1,"totalElements":2,"totalPages":2}}`)) + return + } + _, _ = w.Write([]byte(`{"data":[{"phoneNumber":"+15555550199"}],` + + `"page":{"pageNumber":1,"pageSize":1,"totalElements":2,"totalPages":2}}`)) + }) +} + +// stubNumberGetCapturing records the request path of every request so a test +// can assert the positional phone number is passed through unchanged. +func stubNumberGetCapturing(t *testing.T) (*httptest.Server, *[]string) { + var paths []string + srv := newBrandStub(t, func(w http.ResponseWriter, r *http.Request) { + paths = append(paths, r.URL.Path) + _, _ = w.Write([]byte(`{"data":{"phoneNumber":"+15555550100","status":"SUCCESS"}}`)) + }) + return srv, &paths +} + +// stubNumberHistory answers /phoneNumbers/.../history with one free-text +// entry. +func stubNumberHistory(t *testing.T) *httptest.Server { + return newBrandStub(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"data":[{"createdDate":"2026-01-01T00:00:00Z",` + + `"message":"Successfully registered phone number"}],` + + `"page":{"pageNumber":0,"pageSize":50,"totalElements":1,"totalPages":1}}`)) + }) +} + +// stubNumberHistoryTwoPages is stubNumberListTwoPages's twin for the history +// endpoint: distinct messages per page, keyed off the offset query param. +func stubNumberHistoryTwoPages(t *testing.T) *httptest.Server { + return newBrandStub(t, func(w http.ResponseWriter, r *http.Request) { + offset := r.URL.Query().Get("offset") + if offset == "" || offset == "0" { + _, _ = w.Write([]byte(`{"data":[{"createdDate":"2026-01-02T00:00:00Z",` + + `"message":"page one message"}],` + + `"page":{"pageNumber":0,"pageSize":1,"totalElements":2,"totalPages":2}}`)) + return + } + _, _ = w.Write([]byte(`{"data":[{"createdDate":"2026-01-01T00:00:00Z",` + + `"message":"page two message"}],` + + `"page":{"pageNumber":1,"pageSize":1,"totalElements":2,"totalPages":2}}`)) + }) +} + +func TestNumberListHasNoFilterFlags(t *testing.T) { + // Locks in the deliberate absence of --status and --campaign-id (see + // numberListCmd's Long): the API accepts and silently ignores both, so + // nobody should reintroduce these flags without deleting this test and + // the reasoning it guards. + if f := numberListCmd.Flags().Lookup("status"); f != nil { + t.Errorf("number list must not have a --status flag, found %v", f) + } + if f := numberListCmd.Flags().Lookup("campaign-id"); f != nil { + t.Errorf("number list must not have a --campaign-id flag, found %v", f) + } +} + +func TestNumberListRejectsAllWithOffset(t *testing.T) { + _, _, err := runBrandCmd(t, stubNumberList(t), "number", "list", "--all", "--offset", "0") + if err == nil { + t.Fatal("want an error combining --all with --offset") + } + if code := cmdutil.ExitCodeForError(err); code != cmdutil.ExitFlagError { + t.Errorf("exit code = %d, want %d", code, cmdutil.ExitFlagError) + } +} + +func TestNumberListReturnsNumbers(t *testing.T) { + out, _, err := runBrandCmd(t, stubNumberList(t), "number", "list") + if err != nil { + t.Fatalf("number list: %v", err) + } + if !strings.Contains(out, "+15555550100") { + t.Errorf("stdout should carry the phone number, got %q", out) + } +} + +func TestNumberListWarnsOnTruncationViaStderrOnly(t *testing.T) { + out, errOut, err := runBrandCmd(t, stubNumberListTruncated(t), "number", "list", "--limit", "1") + if err != nil { + t.Fatalf("number list: %v", err) + } + if strings.Contains(out, "pass --all") { + t.Error("truncation warning leaked into stdout; stdout must stay parseable") + } + if !strings.Contains(errOut, "pass --all") { + t.Errorf("stderr should carry the truncation warning, got %q", errOut) + } +} + +// TestNumberListAllWalksEveryPage exercises the ForEachPage accumulation +// branch: the stub serves distinct items per page, and the assertion +// requires BOTH pages' items in stdout, not just a count -- an +// implementation that fetched page one twice (or dropped a page) would fail +// this even though len(all) might coincidentally match. +func TestNumberListAllWalksEveryPage(t *testing.T) { + out, errOut, err := runBrandCmd(t, stubNumberListTwoPages(t), "number", "list", "--all", "--limit", "1", "--plain") + if err != nil { + t.Fatalf("number list --all: %v", err) + } + if !strings.Contains(out, "+15555550100") || !strings.Contains(out, "+15555550199") { + t.Errorf("stdout = %q, want numbers from both pages", out) + } + if strings.Contains(errOut, "pass --all") { + t.Errorf("stderr = %q, want no truncation warning when --all already walked every page", errOut) + } +} + +func TestNumberGetPassesPhoneNumberThrough(t *testing.T) { + srv, paths := stubNumberGetCapturing(t) + if _, _, err := runBrandCmd(t, srv, "number", "get", "+15555550100"); err != nil { + t.Fatalf("number get: %v", err) + } + if len(*paths) != 1 || !strings.HasSuffix((*paths)[0], "/phoneNumbers/+15555550100") { + t.Errorf("paths = %v; get must pass the phone number through unchanged", *paths) + } +} + +// TestNumberGetNotFoundMapsToExitThree covers the "ship it plainly" decision +// in numberDetailCmd's Long: no bespoke handling for a 404, just the normal +// error path, which must still land on exit 3. +func TestNumberGetNotFoundMapsToExitThree(t *testing.T) { + _, _, err := runBrandCmd(t, stubBrandErr(t, 404, `{"errors":[{"description":"not found"}]}`), + "number", "get", "+15555550100") + if err == nil { + t.Fatal("want an error on 404") + } + if code := cmdutil.ExitCodeForError(err); code != cmdutil.ExitNotFound { + t.Errorf("exit code = %d, want %d", code, cmdutil.ExitNotFound) + } +} + +func TestNumberHistoryRejectsAllWithOffset(t *testing.T) { + _, _, err := runBrandCmd(t, stubNumberHistory(t), "number", "history", "+15555550100", "--all", "--offset", "0") + if err == nil { + t.Fatal("want an error combining --all with --offset") + } + if code := cmdutil.ExitCodeForError(err); code != cmdutil.ExitFlagError { + t.Errorf("exit code = %d, want %d", code, cmdutil.ExitFlagError) + } +} + +func TestNumberHistoryReturnsMessageLog(t *testing.T) { + out, _, err := runBrandCmd(t, stubNumberHistory(t), "number", "history", "+15555550100") + if err != nil { + t.Fatalf("number history: %v", err) + } + if !strings.Contains(out, "Successfully registered phone number") { + t.Errorf("stdout should carry history messages, got %q", out) + } +} + +func TestNumberHistoryWarnsOnTruncationViaStderrOnly(t *testing.T) { + out, errOut, err := runBrandCmd(t, stubNumberListTruncated(t), "number", "history", "+15555550100", "--limit", "1") + if err != nil { + t.Fatalf("number history: %v", err) + } + if strings.Contains(out, "pass --all") { + t.Error("truncation warning leaked into stdout; stdout must stay parseable") + } + if !strings.Contains(errOut, "pass --all") { + t.Errorf("stderr should carry the truncation warning, got %q", errOut) + } +} + +// TestNumberHistoryAllWalksEveryPage is TestNumberListAllWalksEveryPage's +// twin for `number history --all`. +func TestNumberHistoryAllWalksEveryPage(t *testing.T) { + out, errOut, err := runBrandCmd(t, stubNumberHistoryTwoPages(t), "number", "history", "+15555550100", + "--all", "--limit", "1", "--plain") + if err != nil { + t.Fatalf("number history --all: %v", err) + } + if !strings.Contains(out, "page one message") || !strings.Contains(out, "page two message") { + t.Errorf("stdout = %q, want messages from both pages", out) + } + if strings.Contains(errOut, "pass --all") { + t.Errorf("stderr = %q, want no truncation warning when --all already walked every page", errOut) + } +} + +func TestNumberCommandsRejectStrayPositionals(t *testing.T) { + // A stray positional on a read is harmless; on a write it is not, and the + // guard belongs on every command so the rule is not a per-command + // judgment call. + cases := [][]string{ + {"number", "list", "STRAY"}, + {"number", "get"}, + {"number", "get", "+15555550100", "STRAY"}, + {"number", "history"}, + {"number", "history", "+15555550100", "STRAY"}, + } + for _, args := range cases { + t.Run(strings.Join(args, " "), func(t *testing.T) { + if _, _, err := runBrandCmd(t, stubNumberList(t), args...); err == nil { + t.Fatal("want an argument error") + } + }) + } +} + +func TestNumberRoleGate403MapsToExitFour(t *testing.T) { + _, _, err := runBrandCmd(t, stubBrandErr(t, 403, + `{"errors":[{"description":"does not have access rights"}]}`), "number", "list") + if err == nil { + t.Fatal("want an error on 403") + } + if code := cmdutil.ExitCodeForError(err); code != cmdutil.ExitConflict { + t.Errorf("exit code = %d, want %d — re-authenticating cannot add a role", code, cmdutil.ExitConflict) + } +} + +// TestNumberCommandTreeCoexistsWithLegacy guards the coexistence decision +// recorded in number.go's init(): the three new subcommands are attached +// onto the existing numberGetCmd node rather than a new sibling "number" +// command, specifically so the legacy flat `band tendlc number ` keeps +// resolving to numberGetCmd (and running the legacy runNumberGet) instead of +// being shadowed by a second command of the same name. This checks cobra's +// routing directly via Find, which needs no stub server or credentials — +// legacy's runNumberGet calls cmdutil.PlatformClient directly rather than +// going through the `service` seam this package's harness swaps, so it +// can't be exercised end-to-end here the way the new subcommands are; the +// full end-to-end legacy check happens by hand against the built binary +// (see the task report). If a future change reintroduces a colliding +// sibling "number" command, the child-count assertion below catches it. +func TestNumberCommandTreeCoexistsWithLegacy(t *testing.T) { + found, _, err := Cmd.Find([]string{"number", "+15555550100"}) + if err != nil { + t.Fatalf("Find(number, ): %v", err) + } + if found != numberGetCmd { + t.Errorf("bare `number ` resolved to %q, want the legacy numberGetCmd", found.CommandPath()) + } + + found, _, err = Cmd.Find([]string{"number", "list"}) + if err != nil { + t.Fatalf("Find(number, list): %v", err) + } + if found != numberListCmd { + t.Errorf("`number list` resolved to %q, want numberListCmd", found.CommandPath()) + } + + found, _, err = Cmd.Find([]string{"number", "get", "+15555550100"}) + if err != nil { + t.Fatalf("Find(number, get, ): %v", err) + } + if found != numberDetailCmd { + t.Errorf("`number get ` resolved to %q, want numberDetailCmd", found.CommandPath()) + } + + found, _, err = Cmd.Find([]string{"number", "history", "+15555550100"}) + if err != nil { + t.Fatalf("Find(number, history, ): %v", err) + } + if found != numberHistoryCmd { + t.Errorf("`number history ` resolved to %q, want numberHistoryCmd", found.CommandPath()) + } + + count := 0 + for _, c := range Cmd.Commands() { + if c.Name() == "number" { + count++ + } + } + if count != 1 { + t.Errorf("Cmd has %d children named %q, want exactly 1", count, "number") + } +} From ede10f6ecba03ce513ac4de744bf3f6ad39dd0b8 Mon Sep 17 00:00:00 2001 From: Kush Date: Fri, 21 Aug 2026 16:53:34 -0500 Subject: [PATCH 03/15] fix(tendlc): add number list --campaign-id-contains, correct filter/projection docs Re-measurement showed campaignId[contains] filters correctly (the earlier zero-match probe used a campaign with no assigned numbers) and the list projection has two shapes, not a fixed five keys (assigned numbers carry three extra fields). status remains confirmed dead under every operator and value, so --status stays absent. --- cmd/tendlc/number.go | 52 ++++++++++++++--------- cmd/tendlc/number_test.go | 87 +++++++++++++++++++++++++++++++++++---- 2 files changed, 113 insertions(+), 26 deletions(-) diff --git a/cmd/tendlc/number.go b/cmd/tendlc/number.go index 7a6480a..e00d803 100644 --- a/cmd/tendlc/number.go +++ b/cmd/tendlc/number.go @@ -9,9 +9,10 @@ import ( ) var ( - numberListLimit int - numberListOffset int - numberListAll bool + numberListLimit int + numberListOffset int + numberListAll bool + numberListCampaignIDContains string numberHistoryLimit int numberHistoryOffset int @@ -23,6 +24,8 @@ func init() { lf.IntVar(&numberListLimit, "limit", 50, "Page size") lf.IntVar(&numberListOffset, "offset", 0, "Pagination offset") lf.BoolVar(&numberListAll, "all", false, "Fetch every page (cannot be combined with --offset)") + lf.StringVar(&numberListCampaignIDContains, "campaign-id-contains", "", + "Filter by campaign ID substring (e.g. CEXMPL1 also matches CEXMPL12); the API has no exact-match operator for this field") hf := numberHistoryCmd.Flags() hf.IntVar(&numberHistoryLimit, "limit", 50, "Page size") @@ -61,25 +64,31 @@ var numberListCmd = &cobra.Command{ Short: "List 10DLC registered phone numbers", Long: `Lists the phone numbers registered for 10DLC traffic on the account. -There are no filter flags -- not --status, not --campaign-id. None. That is -deliberate and measured, not an oversight: on an account holding 21 SUCCESS -and 2 FAILURE phone numbers, status[eq] was silently ignored and returned -all 23 records for status[eq]=SUCCESS, status[eq]=FAILED, and even -status[eq]=NOT_A_STATUS alike, and campaignId[contains] was evaluated but -matched nothing -- 0 results for a real campaign ID and for deliberate -garbage alike. The API does not support filtering on this endpoint. A -filter that returns every record, or none, with a 200 and no error is worse -than an absent flag, because the caller believes it worked -- the same -reasoning that already removed 'brand list --bandwidth-id' and +There is no --status flag. That is deliberate and measured, not an +oversight: status[eq] and status[contains] are both accepted and silently +ignored, for every value tried -- including a value matching nothing at +all -- and every one of them returned every phone number on the account +regardless. A filter that returns every record with a 200 and no error is +worse than an absent flag, because the caller believes it worked -- the +same reasoning that already removed 'brand list --bandwidth-id' and 'campaign list --usecase'. -For the campaign-scoped view, which is a different endpoint and does work, +--campaign-id-contains, by contrast, genuinely narrows results: +campaignId[contains] filters correctly. campaignId[eq] does not -- like +status, it is accepted and silently ignored, returning every number +regardless of value -- which is why the flag is named for what it actually +does (a substring match) rather than implying an exact-match filter the API +cannot perform. For the campaign-scoped view, which is a different endpoint, use 'band tendlc campaign numbers ' instead. -This is a summary projection -- five keys per number: createdDate, -modifiedDate, nnid, phoneNumber, status. Notably absent: campaignId.`, +The list projection has two shapes, not one fixed set of keys: every record +carries createdDate, modifiedDate, nnid, phoneNumber, and status; a number +already assigned to a campaign additionally carries brandId, campaignId, +and customerProfileId. Do not assume every record is missing the campaign +fields -- check for them rather than relying on their absence.`, Example: ` band tendlc number list --plain - band tendlc number list --all --plain`, + band tendlc number list --all --plain + band tendlc number list --campaign-id-contains CEXMPL1 --plain`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { // Detected via Changed so that an explicit --offset 0 also conflicts. @@ -92,8 +101,13 @@ modifiedDate, nnid, phoneNumber, status. Notably absent: campaignId.`, } format, plain := cmdutil.OutputFlags(cmd) + var filters []api.Filter + if numberListCampaignIDContains != "" { + filters = append(filters, api.Filter{Field: "campaignId", Op: api.OpContains, Value: numberListCampaignIDContains}) + } + if !numberListAll { - env, err := svc.ListPhoneNumbers(numberListLimit, numberListOffset, nil) + env, err := svc.ListPhoneNumbers(numberListLimit, numberListOffset, filters) if err != nil { return roleGateError(err, "Campaign Management") } @@ -107,7 +121,7 @@ modifiedDate, nnid, phoneNumber, status. Notably absent: campaignId.`, var all []any err = api.ForEachPage(func(limit, offset int) (*api.Envelope, error) { - return svc.ListPhoneNumbers(limit, offset, nil) + return svc.ListPhoneNumbers(limit, offset, filters) }, numberListLimit, func(batch []any) error { all = append(all, batch...) return nil diff --git a/cmd/tendlc/number_test.go b/cmd/tendlc/number_test.go index 8dd4492..07aeb65 100644 --- a/cmd/tendlc/number_test.go +++ b/cmd/tendlc/number_test.go @@ -19,6 +19,36 @@ func stubNumberList(t *testing.T) *httptest.Server { }) } +// stubNumberListCapturing records the raw query string of every request to +// /phoneNumbers so a test can assert on the deepObject filter encoding. +func stubNumberListCapturing(t *testing.T) (*httptest.Server, *[]string) { + var queries []string + srv := newBrandStub(t, func(w http.ResponseWriter, r *http.Request) { + queries = append(queries, r.URL.RawQuery) + _, _ = w.Write([]byte(`{"data":[],"page":{"pageNumber":0,"pageSize":50,"totalElements":0,"totalPages":0}}`)) + }) + return srv, &queries +} + +// stubNumberListTwoPagesCapturing is stubNumberListTwoPages's twin that also +// records the raw query string of every request, so a test can assert a +// filter survives every page of an --all walk, not just the first request. +func stubNumberListTwoPagesCapturing(t *testing.T) (*httptest.Server, *[]string) { + var queries []string + srv := newBrandStub(t, func(w http.ResponseWriter, r *http.Request) { + queries = append(queries, r.URL.RawQuery) + offset := r.URL.Query().Get("offset") + if offset == "" || offset == "0" { + _, _ = w.Write([]byte(`{"data":[{"phoneNumber":"+15555550100"}],` + + `"page":{"pageNumber":0,"pageSize":1,"totalElements":2,"totalPages":2}}`)) + return + } + _, _ = w.Write([]byte(`{"data":[{"phoneNumber":"+15555550199"}],` + + `"page":{"pageNumber":1,"pageSize":1,"totalElements":2,"totalPages":2}}`)) + }) + return srv, &queries +} + // stubNumberListTruncated answers with a page that reports more records // exist than were returned, so warnIfTruncated fires. func stubNumberListTruncated(t *testing.T) *httptest.Server { @@ -83,16 +113,59 @@ func stubNumberHistoryTwoPages(t *testing.T) *httptest.Server { }) } -func TestNumberListHasNoFilterFlags(t *testing.T) { - // Locks in the deliberate absence of --status and --campaign-id (see - // numberListCmd's Long): the API accepts and silently ignores both, so - // nobody should reintroduce these flags without deleting this test and - // the reasoning it guards. +// TestNumberListHasNoStatusFilterFlag locks in the deliberate absence of +// --status (see numberListCmd's Long): status[eq] and status[contains] are +// both accepted and silently ignored by the server for every value tried, +// including one matching nothing, and every one of them returns every phone +// number on the account regardless. Unlike campaignId, there is no operator +// under which status genuinely filters, so nobody should reintroduce this +// flag without deleting this test and the reasoning it guards. +func TestNumberListHasNoStatusFilterFlag(t *testing.T) { if f := numberListCmd.Flags().Lookup("status"); f != nil { t.Errorf("number list must not have a --status flag, found %v", f) } - if f := numberListCmd.Flags().Lookup("campaign-id"); f != nil { - t.Errorf("number list must not have a --campaign-id flag, found %v", f) +} + +// TestNumberListCampaignIDContainsSendsContains covers the one filter that +// genuinely works on this endpoint: campaignId[contains]. campaignId[eq] is +// accepted and silently ignored by the server (returns every number +// regardless of value) exactly like status -- see numberListCmd's Long -- +// so this locks in that the flag never regresses back to eq, which "looks" +// more correct for an ID filter but would return every number on the +// account. +func TestNumberListCampaignIDContainsSendsContains(t *testing.T) { + srv, queries := stubNumberListCapturing(t) + if _, _, err := runBrandCmd(t, srv, "number", "list", "--campaign-id-contains", "CEXMPL1"); err != nil { + t.Fatalf("number list: %v", err) + } + q := (*queries)[0] + if !strings.Contains(q, "campaignId%5Bcontains%5D=CEXMPL1") { + t.Errorf("query %q missing deepObject contains filter for campaignId", q) + } + if strings.Contains(q, "campaignId%5Beq%5D") { + t.Errorf("query %q uses eq for campaignId, which the API silently ignores", q) + } +} + +// TestNumberListCampaignIDContainsSurvivesAllPagination confirms the filter +// is threaded through every page of an --all walk, not just the first +// request -- an implementation that captured filters once for the first +// call and then paginated with a bare ForEachPage closure ignoring them +// would pass a single-page filter test but silently drop the filter from +// page two onward. +func TestNumberListCampaignIDContainsSurvivesAllPagination(t *testing.T) { + srv, queries := stubNumberListTwoPagesCapturing(t) + if _, _, err := runBrandCmd(t, srv, "number", "list", + "--campaign-id-contains", "CEXMPL1", "--all", "--limit", "1"); err != nil { + t.Fatalf("number list --all: %v", err) + } + if len(*queries) < 2 { + t.Fatalf("got %d requests, want at least 2 (one per page)", len(*queries)) + } + for i, q := range *queries { + if !strings.Contains(q, "campaignId%5Bcontains%5D=CEXMPL1") { + t.Errorf("page %d query %q missing the campaignId filter", i, q) + } } } From 1a22923a4cea5c40f29189b1eb46f2895673cdf1 Mon Sep 17 00:00:00 2001 From: Kush Date: Fri, 21 Aug 2026 16:55:30 -0500 Subject: [PATCH 04/15] docs(tendlc): correct the phone number projection and filter comments Two claims in ListPhoneNumbers' doc comment were wrong, both from probe design rather than API behaviour. The projection is conditional, not fixed: 16 of 23 records carry five keys, the 7 assigned to a campaign carry three more. The original claim came from a single record fetched with limit=1. campaignId[contains] works correctly. The original probe filtered on a campaign with no assigned numbers, got zero results, and read the empty set as a broken filter. Re-tested against a campaign with three numbers, it returns three. status genuinely does not filter under any operator, including a value that matches nothing on the account. --- internal/tendlc/numbers.go | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/internal/tendlc/numbers.go b/internal/tendlc/numbers.go index 2f44fd9..6e0d024 100644 --- a/internal/tendlc/numbers.go +++ b/internal/tendlc/numbers.go @@ -7,18 +7,27 @@ import ( "github.com/Bandwidth/cli/internal/api" ) -// ListPhoneNumbers returns the phone numbers on the account. The list -// projection has five keys — createdDate, modifiedDate, nnid, phoneNumber, -// status — notably no campaignId. +// ListPhoneNumbers returns the phone numbers on the account. // -// filters is accepted only for signature consistency with ListBrands and -// ListCampaigns. Measured against production, filtering does not work on -// this endpoint: status[eq] is silently ignored (an account with 21 SUCCESS -// and 2 FAILURE phone numbers returned all 23 for status[eq]=SUCCESS, -// status[eq]=FAILED, and status[eq]=NOT_A_STATUS alike), and -// campaignId[contains] is evaluated but matches nothing, for a real -// campaign ID and for garbage alike. No caller should pass a filter here, -// and the command layer offers no flags for one. +// The projection is CONDITIONAL, not fixed. Measured across all 23 numbers +// on one account: 16 carried five keys — createdDate, modifiedDate, nnid, +// phoneNumber, status — while the 7 assigned to a campaign carried three +// more: brandId, campaignId, customerProfileId. A client that types this +// response from a single sample drops three fields on every assigned +// number. (An earlier version of this comment claimed a fixed five-key +// projection; that was measured from one record fetched with limit=1.) +// +// Filter support is split, and both halves were measured: +// +// - campaignId[contains] WORKS. A campaign with three assigned numbers +// returns 3; a garbage value returns 0. campaignId[eq] is ignored and +// returns all 23, matching the brand endpoints' behavior in general. +// - status does NOT filter under any operator. An account with 21 SUCCESS +// and 2 FAILURE returned all 23 for status[eq] and status[contains] on +// SUCCESS, on FAILURE, and on a value matching nothing at all. +// +// So the command layer offers --campaign-id-contains and deliberately does +// not offer --status. func (s *Service) ListPhoneNumbers(limit, offset int, filters []api.Filter) (*api.Envelope, error) { return s.get(s.base() + "/phoneNumbers" + api.EncodeQuery(limit, offset, filters)) } From 0cc0bc243cda363e4e2eca9363da69cae53d80cd Mon Sep 17 00:00:00 2001 From: Kush Date: Fri, 21 Aug 2026 17:28:53 -0500 Subject: [PATCH 05/15] feat(tendlc)!: remove the legacy campaigns, numbers, and number commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deletes band tendlc campaigns, campaigns numbers, numbers, and the flat number command, ending the deprecation window from the two previous PRs where the legacy and new command trees coexisted deliberately. No aliases or shims: v0.3.0-beta status and no substantial customer traffic justify a clean break here. number.go's three subcommands (list/get/history) now attach to a plain numberCmd parent registered directly on Cmd, matching brandCmd/campaignCmd, instead of the numberGetCmd node that lived in the now-deleted numbers.go. extractData and filterNumbers in helpers.go existed only to serve the deleted commands and are removed along with their tests. Also folds in a carried-over test assertion: number list must not gain an exact-match --campaign-id flag, since campaignId[eq] is silently ignored by the API and returns every record — the same anti-pattern already avoided by brand list and campaign list. --- cmd/doccontract_test.go | 7 ++ cmd/tendlc/campaign_numbers.go | 53 ------------- cmd/tendlc/campaigns.go | 51 ------------- cmd/tendlc/helpers.go | 47 ------------ cmd/tendlc/number.go | 46 +++++------ cmd/tendlc/number_test.go | 44 +++++++---- cmd/tendlc/numbers.go | 96 ----------------------- cmd/tendlc/tendlc_test.go | 135 --------------------------------- 8 files changed, 53 insertions(+), 426 deletions(-) delete mode 100644 cmd/tendlc/campaign_numbers.go delete mode 100644 cmd/tendlc/campaigns.go delete mode 100644 cmd/tendlc/numbers.go diff --git a/cmd/doccontract_test.go b/cmd/doccontract_test.go index 77d15c0..104bcf6 100644 --- a/cmd/doccontract_test.go +++ b/cmd/doccontract_test.go @@ -14,8 +14,15 @@ import ( // - "location create::subaccount": tracked by the --site→--subaccount rename // spec (docs/superpowers/specs/2026-05-07-subaccount-rename.md). Remove when // that PR lands. +// - "tendlc numbers::campaign-id" and "tendlc numbers::status": the legacy +// `band tendlc numbers` command (and its --campaign-id/--status flags) was +// removed in the 10DLC PR5 cutover (task 3 of +// .superpowers/sdd/2026-08-21-tendlc-pr5-cutover); AGENTS.md's doc sweep is +// task 6 of that same plan. Remove when that PR lands. var knownDrift = map[string]bool{ "location create::subaccount": true, + "tendlc numbers::campaign-id": true, + "tendlc numbers::status": true, } // knownDriftCommands lists command paths documented in tables that are known diff --git a/cmd/tendlc/campaign_numbers.go b/cmd/tendlc/campaign_numbers.go deleted file mode 100644 index e10dc95..0000000 --- a/cmd/tendlc/campaign_numbers.go +++ /dev/null @@ -1,53 +0,0 @@ -package tendlc - -import ( - "fmt" - "net/url" - - "github.com/spf13/cobra" - - "github.com/Bandwidth/cli/internal/cmdutil" - "github.com/Bandwidth/cli/internal/output" -) - -var ( - campaignNumbersLimit int - campaignNumbersOffset int -) - -func init() { - campaignNumbersCmd.Flags().IntVar(&campaignNumbersLimit, "limit", 50, "Page size (max 250)") - campaignNumbersCmd.Flags().IntVar(&campaignNumbersOffset, "offset", 0, "Pagination offset") - campaignsCmd.AddCommand(campaignNumbersCmd) -} - -var campaignNumbersCmd = &cobra.Command{ - Use: "numbers ", - Short: "List phone numbers assigned to a campaign", - Long: "Shows all phone numbers associated with a specific 10DLC campaign, including numbers with provisioning errors.", - Example: ` band tendlc campaigns numbers CR8HFN0`, - Args: cobra.ExactArgs(1), - RunE: runCampaignNumbers, -} - -func runCampaignNumbers(cmd *cobra.Command, args []string) error { - if err := cmdutil.ValidateID(args[0]); err != nil { - return err - } - - client, acctID, err := cmdutil.PlatformClient(cmdutil.AccountIDFlag(cmd)) - if err != nil { - return err - } - - path := fmt.Sprintf("/api/v2/accounts/%s/tendlc/campaigns/%s/phoneNumbers?limit=%d&offset=%d", - acctID, url.PathEscape(args[0]), campaignNumbersLimit, campaignNumbersOffset) - - var result interface{} - if err := client.Get(path, &result); err != nil { - return roleGateError(err, "Campaign Management") - } - - format, plain := cmdutil.OutputFlags(cmd) - return output.StdoutPlainList(format, plain, extractData(result)) -} diff --git a/cmd/tendlc/campaigns.go b/cmd/tendlc/campaigns.go deleted file mode 100644 index d7074d4..0000000 --- a/cmd/tendlc/campaigns.go +++ /dev/null @@ -1,51 +0,0 @@ -package tendlc - -import ( - "fmt" - - "github.com/spf13/cobra" - - "github.com/Bandwidth/cli/internal/cmdutil" - "github.com/Bandwidth/cli/internal/output" -) - -var ( - campaignsLimit int - campaignsOffset int -) - -func init() { - campaignsCmd.Flags().IntVar(&campaignsLimit, "limit", 50, "Page size (max 250)") - campaignsCmd.Flags().IntVar(&campaignsOffset, "offset", 0, "Pagination offset") - Cmd.AddCommand(campaignsCmd) -} - -var campaignsCmd = &cobra.Command{ - Use: "campaigns", - Short: "List 10DLC campaigns on this account", - Long: "Lists all 10DLC campaigns with their registration status, brand, and phone number associations.", - Example: ` # List all campaigns - band tendlc campaigns - - # Paginate through results - band tendlc campaigns --limit 10 --offset 20`, - RunE: runCampaigns, -} - -func runCampaigns(cmd *cobra.Command, args []string) error { - client, acctID, err := cmdutil.PlatformClient(cmdutil.AccountIDFlag(cmd)) - if err != nil { - return err - } - - path := fmt.Sprintf("/api/v2/accounts/%s/tendlc/campaigns?limit=%d&offset=%d", - acctID, campaignsLimit, campaignsOffset) - - var result interface{} - if err := client.Get(path, &result); err != nil { - return roleGateError(err, "Campaign Management") - } - - format, plain := cmdutil.OutputFlags(cmd) - return output.StdoutPlainList(format, plain, extractData(result)) -} diff --git a/cmd/tendlc/helpers.go b/cmd/tendlc/helpers.go index 4f3bd5a..4140216 100644 --- a/cmd/tendlc/helpers.go +++ b/cmd/tendlc/helpers.go @@ -57,53 +57,6 @@ func roleGateError(err error, roleName string) error { } } -// extractData unwraps a paginated response to return just the "data" array. -// If the response doesn't match the expected shape, it's returned as-is. -func extractData(result interface{}) interface{} { - m, ok := result.(map[string]interface{}) - if !ok { - return result - } - if data, exists := m["data"]; exists { - return data - } - return result -} - -// filterNumbers applies client-side filtering on the phone numbers list. -// The phoneNumbers endpoint doesn't support server-side filtering on status -// or campaignId, so we filter after fetching. -func filterNumbers(data interface{}, status, campaignID string) interface{} { - arr, ok := data.([]interface{}) - if !ok { - return data - } - var filtered []interface{} - for _, item := range arr { - m, ok := item.(map[string]interface{}) - if !ok { - continue - } - if status != "" { - s, _ := m["status"].(string) - if !strings.EqualFold(s, status) { - continue - } - } - if campaignID != "" { - c, _ := m["campaignId"].(string) - if !strings.EqualFold(c, campaignID) { - continue - } - } - filtered = append(filtered, item) - } - if filtered == nil { - return []interface{}{} - } - return filtered -} - // isNotFound reports whether err is an API 404. func isNotFound(err error) bool { var apiErr *api.APIError diff --git a/cmd/tendlc/number.go b/cmd/tendlc/number.go index e00d803..6671b38 100644 --- a/cmd/tendlc/number.go +++ b/cmd/tendlc/number.go @@ -32,31 +32,21 @@ func init() { hf.IntVar(&numberHistoryOffset, "offset", 0, "Pagination offset") hf.BoolVar(&numberHistoryAll, "all", false, "Fetch every page (cannot be combined with --offset)") - // numberGetCmd (Use: "number ") is declared in numbers.go -- - // the legacy flat `band tendlc number ` command, deleted in Task 3, - // not here. It is already registered under Cmd by numbers.go's own - // init(). A second, sibling command also named "number" would collide: - // cobra's Command.Find matches children by Name() and returns the FIRST - // one added to the parent's command slice, with no tie-break on Args or - // anything else. Go initializes files within a package in filename - // order, so this file's init() would run before numbers.go's, and - // `Cmd.AddCommand(numberCmd)` here would always win the race -- silently - // shadowing the legacy command's `Cmd.AddCommand(numberGetCmd)` call - // below it, so a bare `band tendlc number +15555550100` would resolve to - // this file's parent, find no subcommand named "+15555550100", and print - // this command's help instead of running the legacy lookup. Verified - // against a cobra sandbox before writing this. - // - // Attaching these three subcommands directly onto the existing - // numberGetCmd node avoids that entirely: no second "number" command is - // ever registered. Cobra falls through to a parent's own Args/RunE - // whenever the next token isn't a known child's name, so - // `band tendlc number ` keeps invoking the legacy runNumberGet - // unchanged, while `list`, `get `, and `history ` route to the - // commands below. When Task 3 deletes numbers.go, replace numberGetCmd - // here with a plain `numberCmd` parent (Use: "number") and re-register - // it on Cmd directly. - numberGetCmd.AddCommand(numberListCmd, numberDetailCmd, numberHistoryCmd) + numberCmd.AddCommand(numberListCmd, numberDetailCmd, numberHistoryCmd) + Cmd.AddCommand(numberCmd) +} + +// numberCmd is the `band tendlc number` parent. The legacy flat +// `band tendlc number ` command (numberGetCmd, declared in numbers.go) +// is gone as of Task 3 -- there is no bare `number ` anymore, only the +// three subcommands below. +var numberCmd = &cobra.Command{ + Use: "number", + Short: "Manage 10DLC phone number registrations", + Long: `View 10DLC phone number registration status. + +Requires the Registration Center feature and the Campaign Management role. +Check with 'band tendlc status --plain'.`, } var numberListCmd = &cobra.Command{ @@ -137,9 +127,9 @@ fields -- check for them rather than relying on their absence.`, } // numberDetailCmd implements `band tendlc number get `. Named -// numberDetailCmd, not numberGetCmd, because numbers.go already declares -// numberGetCmd for the legacy flat `number ` command this is attached -// beneath -- see this file's init(). +// numberDetailCmd, not numberGetCmd, to avoid colliding with the historical +// numberGetCmd identifier that once lived in the now-deleted numbers.go for +// the legacy flat `number ` command. var numberDetailCmd = &cobra.Command{ Use: "get ", Short: "Get 10DLC registration details for a phone number", diff --git a/cmd/tendlc/number_test.go b/cmd/tendlc/number_test.go index 07aeb65..829b345 100644 --- a/cmd/tendlc/number_test.go +++ b/cmd/tendlc/number_test.go @@ -124,6 +124,20 @@ func TestNumberListHasNoStatusFilterFlag(t *testing.T) { if f := numberListCmd.Flags().Lookup("status"); f != nil { t.Errorf("number list must not have a --status flag, found %v", f) } + + // An exact-match --campaign-id (distinct from --campaign-id-contains, + // which does and should exist) is deliberately absent for the same + // reason: campaignId[eq] is silently ignored by the API and returns + // every record regardless of value, so an exact-match flag would + // reintroduce the filter-returns-everything anti-pattern this series + // has now hit three times -- brand list --bandwidth-id, campaign list + // --usecase, and this command's own --status above. + if f := numberListCmd.Flags().Lookup("campaign-id"); f != nil { + t.Errorf("number list must not have an exact-match --campaign-id flag, found %v", f) + } + if f := numberListCmd.Flags().Lookup("campaign-id-contains"); f == nil { + t.Error("number list must have --campaign-id-contains, the flag that genuinely filters") + } } // TestNumberListCampaignIDContainsSendsContains covers the one filter that @@ -324,26 +338,24 @@ func TestNumberRoleGate403MapsToExitFour(t *testing.T) { } } -// TestNumberCommandTreeCoexistsWithLegacy guards the coexistence decision -// recorded in number.go's init(): the three new subcommands are attached -// onto the existing numberGetCmd node rather than a new sibling "number" -// command, specifically so the legacy flat `band tendlc number ` keeps -// resolving to numberGetCmd (and running the legacy runNumberGet) instead of -// being shadowed by a second command of the same name. This checks cobra's -// routing directly via Find, which needs no stub server or credentials — -// legacy's runNumberGet calls cmdutil.PlatformClient directly rather than -// going through the `service` seam this package's harness swaps, so it -// can't be exercised end-to-end here the way the new subcommands are; the -// full end-to-end legacy check happens by hand against the built binary -// (see the task report). If a future change reintroduces a colliding -// sibling "number" command, the child-count assertion below catches it. -func TestNumberCommandTreeCoexistsWithLegacy(t *testing.T) { +// TestNumberCommandTreeHasNoLegacyFlatGet guards Task 3's removal of the +// legacy flat `band tendlc number ` command: numberCmd (Use: "number") +// is now a plain parent, declared the same way as brandCmd and campaignCmd, +// with no RunE of its own. A bare `number ` therefore resolves to +// numberCmd itself with an unconsumed positional, not to a get-style +// command — there is no more shorthand for `number get `. This also +// guards against a regression back to the pre-Task-3 collision risk: if a +// future change ever adds a second sibling command also named "number", +// this test's child-count assertion below would catch it, since cobra's +// Find matches children by name and returns the first match with no +// tie-break. +func TestNumberCommandTreeHasNoLegacyFlatGet(t *testing.T) { found, _, err := Cmd.Find([]string{"number", "+15555550100"}) if err != nil { t.Fatalf("Find(number, ): %v", err) } - if found != numberGetCmd { - t.Errorf("bare `number ` resolved to %q, want the legacy numberGetCmd", found.CommandPath()) + if found != numberCmd { + t.Errorf("bare `number ` resolved to %q, want it to fall through to the numberCmd parent (no legacy get shorthand)", found.CommandPath()) } found, _, err = Cmd.Find([]string{"number", "list"}) diff --git a/cmd/tendlc/numbers.go b/cmd/tendlc/numbers.go deleted file mode 100644 index 70d64a0..0000000 --- a/cmd/tendlc/numbers.go +++ /dev/null @@ -1,96 +0,0 @@ -package tendlc - -import ( - "fmt" - "net/url" - - "github.com/spf13/cobra" - - "github.com/Bandwidth/cli/internal/cmdutil" - "github.com/Bandwidth/cli/internal/output" -) - -var ( - numbersLimit int - numbersOffset int - numbersCampaignID string - numbersStatus string -) - -func init() { - numbersCmd.Flags().IntVar(&numbersLimit, "limit", 50, "Page size (max 250)") - numbersCmd.Flags().IntVar(&numbersOffset, "offset", 0, "Pagination offset") - numbersCmd.Flags().StringVar(&numbersCampaignID, "campaign-id", "", "Filter by campaign ID") - numbersCmd.Flags().StringVar(&numbersStatus, "status", "", "Filter by status: PROCESSING, SUCCESS, FAILURE") - Cmd.AddCommand(numbersCmd) - Cmd.AddCommand(numberGetCmd) -} - -var numbersCmd = &cobra.Command{ - Use: "numbers", - Short: "List 10DLC registered phone numbers", - Long: "Lists all phone numbers registered for A2P 10DLC traffic, with their campaign assignment and registration status.", - Example: ` # List all registered numbers - band tendlc numbers - - # Filter by campaign - band tendlc numbers --campaign-id CR8HFN0 - - # Filter by status - band tendlc numbers --status SUCCESS`, - RunE: runNumbers, -} - -func runNumbers(cmd *cobra.Command, args []string) error { - client, acctID, err := cmdutil.PlatformClient(cmdutil.AccountIDFlag(cmd)) - if err != nil { - return err - } - - path := fmt.Sprintf("/api/v2/accounts/%s/tendlc/phoneNumbers?limit=%d&offset=%d", - acctID, numbersLimit, numbersOffset) - - var result interface{} - if err := client.Get(path, &result); err != nil { - return roleGateError(err, "Campaign Management") - } - - data := extractData(result) - - // Client-side filtering — the phoneNumbers endpoint doesn't support - // server-side filtering on status or campaignId. - if numbersStatus != "" || numbersCampaignID != "" { - data = filterNumbers(data, numbersStatus, numbersCampaignID) - } - - format, plain := cmdutil.OutputFlags(cmd) - return output.StdoutPlainList(format, plain, data) -} - -var numberGetCmd = &cobra.Command{ - Use: "number ", - Short: "Get 10DLC registration details for a phone number", - Long: "Shows the 10DLC registration status, campaign assignment, and brand for a specific phone number.", - Example: ` band tendlc number +19195551234`, - Args: cobra.ExactArgs(1), - RunE: runNumberGet, -} - -func runNumberGet(cmd *cobra.Command, args []string) error { - number := cmdutil.NormalizeNumber(args[0]) - - client, acctID, err := cmdutil.PlatformClient(cmdutil.AccountIDFlag(cmd)) - if err != nil { - return err - } - - path := fmt.Sprintf("/api/v2/accounts/%s/tendlc/phoneNumbers/%s", acctID, url.PathEscape(number)) - - var result interface{} - if err := client.Get(path, &result); err != nil { - return roleGateError(err, "Campaign Management") - } - - format, plain := cmdutil.OutputFlags(cmd) - return output.StdoutAuto(format, plain, result) -} diff --git a/cmd/tendlc/tendlc_test.go b/cmd/tendlc/tendlc_test.go index f52d779..502aece 100644 --- a/cmd/tendlc/tendlc_test.go +++ b/cmd/tendlc/tendlc_test.go @@ -87,141 +87,6 @@ func TestRoleGateError_NonAPIError(t *testing.T) { } } -func TestExtractData(t *testing.T) { - t.Run("standard paginated response", func(t *testing.T) { - resp := map[string]interface{}{ - "data": []interface{}{ - map[string]interface{}{ - "phoneNumber": "+12054443942", - "campaignId": "CA3XKE1", - "status": "SUCCESS", - }, - }, - "page": map[string]interface{}{ - "totalElements": float64(1), - }, - } - data := extractData(resp) - arr, ok := data.([]interface{}) - if !ok { - t.Fatalf("expected []interface{}, got %T", data) - } - if len(arr) != 1 { - t.Fatalf("expected 1 element, got %d", len(arr)) - } - }) - - t.Run("no data key", func(t *testing.T) { - resp := map[string]interface{}{ - "something": "else", - } - data := extractData(resp) - m, ok := data.(map[string]interface{}) - if !ok { - t.Fatalf("expected map, got %T", data) - } - if m["something"] != "else" { - t.Error("expected original response returned as-is") - } - }) - - t.Run("non-map response", func(t *testing.T) { - resp := "just a string" - data := extractData(resp) - if data != resp { - t.Error("expected passthrough for non-map input") - } - }) - - t.Run("nil response", func(t *testing.T) { - data := extractData(nil) - if data != nil { - t.Errorf("expected nil, got %v", data) - } - }) - - t.Run("empty data array", func(t *testing.T) { - resp := map[string]interface{}{ - "data": []interface{}{}, - "page": map[string]interface{}{ - "totalElements": float64(0), - }, - } - data := extractData(resp) - arr, ok := data.([]interface{}) - if !ok { - t.Fatalf("expected []interface{}, got %T", data) - } - if len(arr) != 0 { - t.Errorf("expected empty array, got %d elements", len(arr)) - } - }) -} - -func TestFilterNumbers(t *testing.T) { - numbers := []interface{}{ - map[string]interface{}{"phoneNumber": "+11111111111", "status": "SUCCESS", "campaignId": "C1"}, - map[string]interface{}{"phoneNumber": "+12222222222", "status": "FAILURE", "campaignId": "C1"}, - map[string]interface{}{"phoneNumber": "+13333333333", "status": "SUCCESS", "campaignId": "C2"}, - map[string]interface{}{"phoneNumber": "+14444444444", "status": "PROCESSING"}, - } - - t.Run("filter by status", func(t *testing.T) { - result := filterNumbers(numbers, "FAILURE", "") - arr := result.([]interface{}) - if len(arr) != 1 { - t.Fatalf("expected 1, got %d", len(arr)) - } - m := arr[0].(map[string]interface{}) - if m["phoneNumber"] != "+12222222222" { - t.Errorf("got %v", m["phoneNumber"]) - } - }) - - t.Run("filter by campaign", func(t *testing.T) { - result := filterNumbers(numbers, "", "C2") - arr := result.([]interface{}) - if len(arr) != 1 { - t.Fatalf("expected 1, got %d", len(arr)) - } - }) - - t.Run("filter by both", func(t *testing.T) { - result := filterNumbers(numbers, "SUCCESS", "C1") - arr := result.([]interface{}) - if len(arr) != 1 { - t.Fatalf("expected 1, got %d", len(arr)) - } - m := arr[0].(map[string]interface{}) - if m["phoneNumber"] != "+11111111111" { - t.Errorf("got %v", m["phoneNumber"]) - } - }) - - t.Run("no matches returns empty array", func(t *testing.T) { - result := filterNumbers(numbers, "FAILURE", "C999") - arr := result.([]interface{}) - if len(arr) != 0 { - t.Errorf("expected 0, got %d", len(arr)) - } - }) - - t.Run("case insensitive", func(t *testing.T) { - result := filterNumbers(numbers, "success", "c1") - arr := result.([]interface{}) - if len(arr) != 1 { - t.Fatalf("expected 1, got %d", len(arr)) - } - }) - - t.Run("non-array passthrough", func(t *testing.T) { - result := filterNumbers("not an array", "SUCCESS", "") - if result != "not an array" { - t.Error("expected passthrough") - } - }) -} - func TestStatusCommandRegistered(t *testing.T) { c, _, err := Cmd.Find([]string{"status"}) if err != nil || c.Name() != "status" { From 77c2d0f5f0467030a7e3a20e0a9550a0ac9840ec Mon Sep 17 00:00:00 2001 From: Kush Date: Fri, 21 Aug 2026 17:35:50 -0500 Subject: [PATCH 06/15] fix(tendlc)!: reject stray args on tendlc's dispatcher commands band tendlc campaigns, band tendlc numbers, and band tendlc number -- all removed in the previous commit -- exited 0 with a help dump instead of failing, because cobra checks Runnable() before it ever consults Args, and none of Cmd/brandCmd/campaignCmd/numberCmd/vettingCmd had a RunE. A stray token that used to be a real, now-deleted command was indistinguishable from a successful help request. Gives each of those five dispatcher commands Args: cobra.NoArgs plus a trivial RunE (return cmd.Help()) so NoArgs actually runs: a bare invocation still prints help and exits 0, but a trailing token matching no subcommand now exits non-zero. customer-profile has the same latent shape but is out of scope for this branch. --- cmd/tendlc/brand.go | 12 +++++++ cmd/tendlc/campaign.go | 12 +++++++ cmd/tendlc/number.go | 12 +++++++ cmd/tendlc/tendlc.go | 12 +++++++ cmd/tendlc/tendlc_test.go | 75 +++++++++++++++++++++++++++++++++++++++ cmd/tendlc/vetting.go | 12 +++++++ 6 files changed, 135 insertions(+) diff --git a/cmd/tendlc/brand.go b/cmd/tendlc/brand.go index af0d9fa..4d8286c 100644 --- a/cmd/tendlc/brand.go +++ b/cmd/tendlc/brand.go @@ -19,6 +19,18 @@ and is null until registration completes. Commands here accept either. Requires the Registration Center feature and the Campaign Management role. Check with 'band tendlc status --plain'.`, + Args: cobra.NoArgs, + // A trivial RunE is required, not decorative: cobra's execute() checks + // Runnable() (Run/RunE set) BEFORE it ever calls ValidateArgs, so a + // parent with no RunE always short-circuits to flag.ErrHelp regardless + // of Args -- Args: cobra.NoArgs above would silently never run. Calling + // cmd.Help() here keeps the existing bare-invocation behavior (print + // help, exit 0) while letting a stray positional -- e.g. a deleted + // command's name -- reach NoArgs and fail with a non-zero exit instead + // of being silently swallowed as if it were a help request. + RunE: func(cmd *cobra.Command, args []string) error { + return cmd.Help() + }, } func init() { diff --git a/cmd/tendlc/campaign.go b/cmd/tendlc/campaign.go index b7ace11..9344825 100644 --- a/cmd/tendlc/campaign.go +++ b/cmd/tendlc/campaign.go @@ -15,6 +15,18 @@ or VETTED_VERIFIED before the campaign can carry traffic. Requires the Registration Center feature and the Campaign Management role. Check with 'band tendlc status --plain'.`, + Args: cobra.NoArgs, + // A trivial RunE is required, not decorative: cobra's execute() checks + // Runnable() (Run/RunE set) BEFORE it ever calls ValidateArgs, so a + // parent with no RunE always short-circuits to flag.ErrHelp regardless + // of Args -- Args: cobra.NoArgs above would silently never run. Calling + // cmd.Help() here keeps the existing bare-invocation behavior (print + // help, exit 0) while letting a stray positional -- e.g. a deleted + // command's name -- reach NoArgs and fail with a non-zero exit instead + // of being silently swallowed as if it were a help request. + RunE: func(cmd *cobra.Command, args []string) error { + return cmd.Help() + }, } func init() { diff --git a/cmd/tendlc/number.go b/cmd/tendlc/number.go index 6671b38..f743da3 100644 --- a/cmd/tendlc/number.go +++ b/cmd/tendlc/number.go @@ -47,6 +47,18 @@ var numberCmd = &cobra.Command{ Requires the Registration Center feature and the Campaign Management role. Check with 'band tendlc status --plain'.`, + Args: cobra.NoArgs, + // A trivial RunE is required, not decorative: cobra's execute() checks + // Runnable() (Run/RunE set) BEFORE it ever calls ValidateArgs, so a + // parent with no RunE always short-circuits to flag.ErrHelp regardless + // of Args -- Args: cobra.NoArgs above would silently never run. Calling + // cmd.Help() here keeps the existing bare-invocation behavior (print + // help, exit 0) while letting a stray positional -- e.g. a deleted + // command's name -- reach NoArgs and fail with a non-zero exit instead + // of being silently swallowed as if it were a help request. + RunE: func(cmd *cobra.Command, args []string) error { + return cmd.Help() + }, } var numberListCmd = &cobra.Command{ diff --git a/cmd/tendlc/tendlc.go b/cmd/tendlc/tendlc.go index b9847b3..8b1f837 100644 --- a/cmd/tendlc/tendlc.go +++ b/cmd/tendlc/tendlc.go @@ -10,4 +10,16 @@ var Cmd = &cobra.Command{ Requires the Campaign Management role and the Registration Center feature on your account. If you get a 403 error, contact your Bandwidth account manager to enable access.`, + Args: cobra.NoArgs, + // A trivial RunE is required, not decorative: cobra's execute() checks + // Runnable() (Run/RunE set) BEFORE it ever calls ValidateArgs, so a + // parent with no RunE always short-circuits to flag.ErrHelp regardless + // of Args -- Args: cobra.NoArgs above would silently never run. Calling + // cmd.Help() here keeps the existing bare-invocation behavior (print + // help, exit 0) while letting a stray positional -- e.g. a deleted + // command's name -- reach NoArgs and fail with a non-zero exit instead + // of being silently swallowed as if it were a help request. + RunE: func(cmd *cobra.Command, args []string) error { + return cmd.Help() + }, } diff --git a/cmd/tendlc/tendlc_test.go b/cmd/tendlc/tendlc_test.go index 502aece..cd8052f 100644 --- a/cmd/tendlc/tendlc_test.go +++ b/cmd/tendlc/tendlc_test.go @@ -3,9 +3,13 @@ package tendlc import ( "errors" "fmt" + "strings" "testing" + "github.com/spf13/cobra" + "github.com/Bandwidth/cli/internal/api" + "github.com/Bandwidth/cli/internal/cmdutil" ) func TestRoleGateError_RegistrationCenter(t *testing.T) { @@ -170,6 +174,77 @@ func TestIsNotFound(t *testing.T) { } } +// TestRemovedLegacyCommandsExitNonZero locks in the Task 3 fix: a stray +// token that used to be a real command -- `campaigns`, `numbers`, or a bare +// positional under `number` -- must fail loudly, not print help with exit 0. +// Before this fix, Cmd/brandCmd/campaignCmd/numberCmd/vettingCmd had no RunE, +// so cobra's execute() hit its `if !c.Runnable() { return flag.ErrHelp }` +// short-circuit before ever reaching ValidateArgs, and a deleted command's +// name looked identical to a successful help request: exit 0 either way. A +// caller (especially an agent scripting against this CLI) cannot tell +// "this command doesn't exist" apart from "you asked for help" on exit code +// alone without this fix. +// +// `number +15555550100` is the subtler case of the three: "+15555550100" was +// never a command name to begin with, so this isn't "unknown command", it's +// a stray positional against a parent (numberCmd) that now takes none. Both +// failure shapes are covered here because they go through different cobra +// code paths (unmatched child name vs. a leftover arg after the deepest +// match), and only NoArgs on the matched command catches the latter. +// +// No stub server is passed (srv is nil in every case): all three must fail +// before ever reaching a RunE that would call `service`, so this needs no +// live API call and no credentials. +func TestRemovedLegacyCommandsExitNonZero(t *testing.T) { + cases := [][]string{ + {"campaigns"}, + {"numbers"}, + {"number", "+15555550100"}, + } + for _, args := range cases { + t.Run(strings.Join(args, " "), func(t *testing.T) { + _, _, err := runBrandCmd(t, nil, args...) + if err == nil { + t.Fatalf("band tendlc %s: want a non-zero-exit error, got nil", strings.Join(args, " ")) + } + if code := cmdutil.ExitCodeForError(err); code == cmdutil.ExitOK { + t.Errorf("band tendlc %s: exit code = %d, want non-zero", strings.Join(args, " "), code) + } + }) + } +} + +// TestParentCommandsStillDispatchToRealSubcommands guards the other half of +// the same change: Args: cobra.NoArgs on Cmd/brandCmd/campaignCmd/numberCmd/ +// vettingCmd must only reject a token that matches no subcommand -- cobra +// resolves subcommands via Find before Args is ever consulted, so a real +// subcommand name must keep dispatching exactly as before. Checked directly +// via Cmd.Find rather than execution, since these commands need no stub +// server or credentials to prove routing. +func TestParentCommandsStillDispatchToRealSubcommands(t *testing.T) { + cases := []struct { + path []string + want *cobra.Command + }{ + {[]string{"number", "list"}, numberListCmd}, + {[]string{"brand", "list"}, brandListCmd}, + {[]string{"campaign", "list"}, campaignListCmd}, + {[]string{"vetting", "list"}, vettingListCmd}, + } + for _, tt := range cases { + name := strings.Join(tt.path, " ") + t.Run(name, func(t *testing.T) { + found, _, err := Cmd.Find(tt.path) + if err != nil { + t.Fatalf("Find(%v): %v", tt.path, err) + } + if found != tt.want { + t.Errorf("%s resolved to %q, want %s", name, found.CommandPath(), tt.want.Name()) + } + }) + } +} + func contains(s, sub string) bool { return len(s) >= len(sub) && (s == sub || len(s) > 0 && containsSubstring(s, sub)) } diff --git a/cmd/tendlc/vetting.go b/cmd/tendlc/vetting.go index a34825a..72652b7 100644 --- a/cmd/tendlc/vetting.go +++ b/cmd/tendlc/vetting.go @@ -33,6 +33,18 @@ Every command here accepts a brand ID as its first positional, not a vetting ID. Requires the Registration Center feature and the Campaign Management role.`, + Args: cobra.NoArgs, + // A trivial RunE is required, not decorative: cobra's execute() checks + // Runnable() (Run/RunE set) BEFORE it ever calls ValidateArgs, so a + // parent with no RunE always short-circuits to flag.ErrHelp regardless + // of Args -- Args: cobra.NoArgs above would silently never run. Calling + // cmd.Help() here keeps the existing bare-invocation behavior (print + // help, exit 0) while letting a stray positional -- e.g. a deleted + // command's name -- reach NoArgs and fail with a non-zero exit instead + // of being silently swallowed as if it were a help request. + RunE: func(cmd *cobra.Command, args []string) error { + return cmd.Help() + }, } func init() { From 2ad093345c6b35f63ea08e8bcc57eeb9b94e4bb9 Mon Sep 17 00:00:00 2001 From: Kush Date: Fri, 21 Aug 2026 17:51:59 -0500 Subject: [PATCH 07/15] test: make the doc-contract parser distinguish subcommands from positionals --- cmd/doccontract_test.go | 192 +++++++++++++++++++++++++++++++++++----- 1 file changed, 171 insertions(+), 21 deletions(-) diff --git a/cmd/doccontract_test.go b/cmd/doccontract_test.go index 104bcf6..1c9e72b 100644 --- a/cmd/doccontract_test.go +++ b/cmd/doccontract_test.go @@ -25,10 +25,30 @@ var knownDrift = map[string]bool{ "tendlc numbers::status": true, } -// knownDriftCommands lists command paths documented in tables that are known -// rename-related drift and intentionally not yet reconciled. +// knownDriftCommands lists command paths that are known drift and +// intentionally not yet reconciled — either a documented command path that +// no longer resolves, or (see the last entry) a prose false positive the +// parser can't tell apart from a real one. // REMOVE entries here as the underlying drift is fixed. -var knownDriftCommands = map[string]bool{} +// - "tendlc campaigns", "tendlc numbers", "tendlc campaigns numbers": the +// legacy `band tendlc campaigns`/`numbers`/`campaigns numbers` commands +// were removed in the 10DLC PR5 cutover (task 3 of +// .superpowers/sdd/2026-08-21-tendlc-pr5-cutover). Fixing the parser to +// catch this exact class of drift is task 4 of that plan; the doc sweep +// that removes these references (and these three entries) is task 6. +// - "number list is not": not command drift at all — a false positive from +// AGENTS.md's "# On Bandwidth Build accounts, band number list is not +// available." comment. The line has no backtick/code-span markers, so +// bandUsageRe (which matches "band " anywhere in a line) tokenizes the +// following prose words ("list", "is", "not") as if they were further +// command-path tokens, same as it would a real subcommand name. Task 6 +// rewords this line too; remove this entry alongside it. +var knownDriftCommands = map[string]bool{ + "tendlc campaigns": true, + "tendlc numbers": true, + "tendlc campaigns numbers": true, + "number list is not": true, +} // bandUsageRe captures everything after "band " to end of line (GREEDY — a // non-greedy capture would stop at the first space and truncate multi-word @@ -48,6 +68,13 @@ var backtickBandRe = regexp.MustCompile("`(band [^`]+)`") // so the first non-matching token ends the command path. var commandTokenRe = regexp.MustCompile(`^[a-z][a-z-]*$`) +// usePlaceholderRe matches a "<...>" or "[...]" placeholder in a cobra +// Use string, e.g. "get " or "release [number]". This codebase +// consistently declares positional args this way (verified against every +// `Args: cobra.ExactArgs/MinimumNArgs/MaximumNArgs/RangeArgs` site in cmd/ +// before relying on it here) — see resolvedCommandAbsorbsRemainder below. +var usePlaceholderRe = regexp.MustCompile(`[<\[]`) + // resolveCommand walks rootCmd by the command-path tokens, descending into // subcommands. It returns the deepest command matched and how many leading // tokens were matched as ACTUAL subcommands. matched==0 means the first token @@ -65,6 +92,63 @@ func resolveCommand(path []string) (cmd *cobra.Command, matched int) { return cur, matched } +// resolvedCommandAbsorbsRemainder decides whether path[matched:] — the +// command-shaped tokens (they already passed commandTokenRe) left over after +// resolveCommand stopped — are legitimate positional arguments of the +// resolved command, or evidence that the documented path doesn't actually +// exist (a stale/renamed command reference). +// +// A tightened `matched == len(path)` check (the obvious fix) is wrong: it +// would also reject real docs like `band auth use admin` and +// `band sip realm delete vapi`, where "admin"/"vapi" are positional +// arguments that happen to be lowercase words and so parse as command +// tokens too. cobra.Command.Args is a func, not introspectable data, so we +// can't read an arity off it directly. +// +// Heuristic chosen: trust the resolved command's Use string. If it declares +// a "<...>"/"[...]" placeholder, any leftover tokens are treated as that +// command's positional args (pass). Otherwise leftover tokens are treated as +// an unresolved subcommand reference (fail). This was checked against every +// Args-taking command in cmd/ and the convention holds everywhere. +// +// The alternative considered was "resolved command is a leaf (no +// subcommands)". That also passes the required cases here, but has a wider +// blind spot: it would accept ANY trailing word after a leaf command, even +// one that takes zero args (e.g. a stale `band tendlc number list foo` would +// silently pass because "list" is a leaf, regardless of what "foo" is). The +// Use-string check catches that, at the cost of its own blind spot: if a +// command takes positional args but its Use string forgets to declare a +// placeholder, this will wrongly flag it as stale. +// +// Neither heuristic — nor this whole boundary-based approach — catches +// drift where the documented path fully resolves as a command but is +// invoked with an argument shape that command no longer accepts (e.g. a +// stale `band tendlc number ` after that subtree was restructured to +// require an explicit `get`/`list`/`history` subcommand): matched==len(path) +// there, so there's no remainder for this function to judge at all. That +// class of drift needs a human doc sweep, not this parser. +func resolvedCommandAbsorbsRemainder(cmd *cobra.Command, path []string, matched int) bool { + if matched == len(path) { + return true // fully resolved; no remainder to judge + } + return usePlaceholderRe.MatchString(cmd.Use) +} + +// commandPathTokens splits s into whitespace-separated fields and returns the +// leading run of fields that look like command tokens (per commandTokenRe). +// The first field that isn't command-shaped (a flag, placeholder, ID, phone +// number, etc.) ends the path. +func commandPathTokens(s string) []string { + var path []string + for _, f := range strings.Fields(s) { + if !commandTokenRe.MatchString(f) { + break + } + path = append(path, f) + } + return path +} + func flagExists(c *cobra.Command, name string) bool { if c.Flags().Lookup(name) != nil { return true @@ -75,6 +159,73 @@ func flagExists(c *cobra.Command, name string) bool { return rootCmd.PersistentFlags().Lookup(name) != nil } +// TestParserDistinguishesSubcommandsFromPositionals exercises the boundary +// logic (resolveCommand + resolvedCommandAbsorbsRemainder) directly against +// the real command tree, independent of any doc file. It proves the fix +// actually catches the class of drift this PR creates (a stale reference to +// a deleted multi-word command) without also flagging legitimate positional +// arguments that happen to be lowercase words. +func TestParserDistinguishesSubcommandsFromPositionals(t *testing.T) { + tests := []struct { + name string + commandLine string // everything after "band ", as it appears in docs + wantFlagged bool + }{ + { + name: "deleted `tendlc campaigns` with a trailing subcommand-shaped word", + commandLine: "tendlc campaigns list", + wantFlagged: true, + }, + { + name: "deleted `tendlc numbers`", + commandLine: "tendlc numbers", + wantFlagged: true, + }, + { + name: "wholly bogus top-level command", + commandLine: "notacommand", + wantFlagged: true, + }, + { + name: "auth use admin: admin is a positional profile name, not a command", + commandLine: "auth use admin", + wantFlagged: false, + }, + { + name: "sip realm delete vapi: vapi is a positional realm name, not a command", + commandLine: "sip realm delete vapi", + wantFlagged: false, + }, + { + name: "tendlc brand get BEXMPL1: valid, real subcommand path with a positional", + commandLine: "tendlc brand get BEXMPL1", + wantFlagged: false, + }, + { + name: "tendlc number list: fully valid, real subcommand path, no remainder", + commandLine: "tendlc number list", + wantFlagged: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := commandPathTokens(tt.commandLine) + if len(path) == 0 { + t.Fatalf("commandPathTokens(%q) produced no path tokens", tt.commandLine) + } + cmd, matched := resolveCommand(path) + // Mirrors exactly what TestDocumentedCommandsAndFlagsExist does + // with the result of resolveCommand. + flagged := matched == 0 || !resolvedCommandAbsorbsRemainder(cmd, path, matched) + if flagged != tt.wantFlagged { + t.Errorf("band %s: flagged = %v, want %v (matched=%d, path=%v, resolved=%q)", + tt.commandLine, flagged, tt.wantFlagged, matched, path, cmd.CommandPath()) + } + }) + } +} + func TestDocumentedCommandsAndFlagsExist(t *testing.T) { for _, doc := range []string{"../README.md", "../AGENTS.md"} { raw, err := os.ReadFile(doc) @@ -103,14 +254,7 @@ func TestDocumentedCommandsAndFlagsExist(t *testing.T) { // bm[1] is the content inside the backticks, e.g. "band app list" // Strip the leading "band " and tokenize the command path. rest := strings.TrimPrefix(bm[1], "band ") - fields := strings.Fields(rest) - var path []string - for _, f := range fields { - if !commandTokenRe.MatchString(f) { - break - } - path = append(path, f) - } + path := commandPathTokens(rest) if len(path) == 0 { continue } @@ -118,9 +262,14 @@ func TestDocumentedCommandsAndFlagsExist(t *testing.T) { if knownDriftCommands[cmdName] { continue } - _, matched := resolveCommand(path) + cmd, matched := resolveCommand(path) if matched == 0 { t.Errorf("%s documents `band %s …` but %q is not a command under `band`", doc, cmdName, path[0]) + continue + } + if !resolvedCommandAbsorbsRemainder(cmd, path, matched) { + t.Errorf("%s documents `band %s …` but only `band %s` resolves; %q has no declared positional args to absorb %q", + doc, cmdName, strings.Join(path[:matched], " "), cmd.CommandPath(), strings.Join(path[matched:], " ")) } continue } @@ -140,23 +289,24 @@ func TestDocumentedCommandsAndFlagsExist(t *testing.T) { // Command path = leading command-name tokens before the first // flag/placeholder/arg. - fields := strings.Fields(capture) - var path []string - for _, f := range fields { - if !commandTokenRe.MatchString(f) { - break - } - path = append(path, f) - } + path := commandPathTokens(capture) if len(path) == 0 { continue } - cmd, matched := resolveCommand(path) cmdName := strings.Join(path, " ") + if knownDriftCommands[cmdName] { + continue + } + cmd, matched := resolveCommand(path) if matched == 0 { t.Errorf("%s documents `band %s …` but %q is not a command under `band`", doc, cmdName, path[0]) continue } + if !resolvedCommandAbsorbsRemainder(cmd, path, matched) { + t.Errorf("%s documents `band %s …` but only `band %s` resolves; %q has no declared positional args to absorb %q", + doc, cmdName, strings.Join(path[:matched], " "), cmd.CommandPath(), strings.Join(path[matched:], " ")) + continue + } for _, fm := range flagRe.FindAllStringSubmatch(capture, -1) { flag := fm[1] // Cobra auto-injects --help on every command; skip it. From 1257e5100d137113e319e1a4ec92d764ef81dbd4 Mon Sep 17 00:00:00 2001 From: Kush Date: Sun, 23 Aug 2026 21:11:55 -0500 Subject: [PATCH 08/15] test: close the fully-resolves-but-rejects-args blind spot in the doc-contract parser Adds a third gate that calls the resolved command's real cobra Args validator against the documented arguments, catching stale references that resolve completely (so the Use-string heuristic sees no remainder to judge) but whose command no longer accepts what follows it. --- cmd/doccontract_test.go | 256 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 248 insertions(+), 8 deletions(-) diff --git a/cmd/doccontract_test.go b/cmd/doccontract_test.go index 1c9e72b..406422b 100644 --- a/cmd/doccontract_test.go +++ b/cmd/doccontract_test.go @@ -1,6 +1,7 @@ package cmd import ( + "fmt" "os" "regexp" "strings" @@ -43,11 +44,24 @@ var knownDrift = map[string]bool{ // following prose words ("list", "is", "not") as if they were further // command-path tokens, same as it would a real subcommand name. Task 6 // rewords this line too; remove this entry alongside it. +// - "tendlc number": the legacy bare `band tendlc number ` — a +// *different* command from the still-current `number get ` — was +// also removed in task 3 of the same plan. This one resolves fully +// (`tendlc number` is a real command, the dispatcher parent), so it's +// invisible to the path-boundary checks above; argsGateRejects (added +// when this genuine gap was found during review) is what actually +// catches it, by calling numberCmd's real Args (cobra.NoArgs) against +// the documented phone-number argument and observing it reject. This is +// real drift, not a gate false positive — confirmed by hand (`band +// tendlc number +15555550100` exits 1 against the built binary) — and, +// like the three entries above, deferred to task 6's doc sweep rather +// than fixed here. var knownDriftCommands = map[string]bool{ "tendlc campaigns": true, "tendlc numbers": true, "tendlc campaigns numbers": true, "number list is not": true, + "tendlc number": true, } // bandUsageRe captures everything after "band " to end of line (GREEDY — a @@ -134,6 +148,203 @@ func resolvedCommandAbsorbsRemainder(cmd *cobra.Command, path []string, matched return usePlaceholderRe.MatchString(cmd.Use) } +// shellFields splits s into whitespace-separated fields, but treats content +// inside a matching quote pair ("...", '...') or placeholder pair (<...>, +// [...]) as belonging to a SINGLE field, the way a shell (for quotes) or a +// human reading a placeholder (for brackets) would. This matters because +// Args validators in this codebase only count arguments (see argsGateRejects), +// so `band bxml speak "Thanks for calling. How can we help?"` must count as +// one argument, not seven. +// +// ok is false if a quote or bracket is left unclosed by end of line. That +// happens legitimately and often: a command mentioned inside a single- or +// double-quoted phrase whose OPENING delimiter is prose that occurs before +// "band" — e.g. `Confirm with 'band tendlc brand get WEXAMPLE02' — a 404 +// means it is gone.` — leaves an odd, unmatched quote count from this +// string's point of view (the capture starts at "band ", after the real +// opening quote). Callers must treat ok==false as "can't parse this line +// with confidence" and skip it, not as evidence of drift. +func shellFields(s string) (fields []string, ok bool) { + var b strings.Builder + inField := false + var closing byte + for i := 0; i < len(s); i++ { + c := s[i] + switch { + case closing != 0: + b.WriteByte(c) + if c == closing { + closing = 0 + } + case c == '"' || c == '\'': + closing = c + b.WriteByte(c) + inField = true + case c == '<': + closing = '>' + b.WriteByte(c) + inField = true + case c == '[': + closing = ']' + b.WriteByte(c) + inField = true + case c == ' ' || c == '\t': + if inField { + fields = append(fields, b.String()) + b.Reset() + inField = false + } + default: + b.WriteByte(c) + inField = true + } + } + if closing != 0 { + return nil, false + } + if inField { + fields = append(fields, b.String()) + } + return fields, true +} + +// splitPositionalArgs walks fields (everything after a resolved command) and +// separates flags — and, critically, their VALUES — from genuine positional +// arguments, consulting the resolved command's real flag definitions (the +// same source cobra itself uses) rather than guessing from a "--" prefix +// alone. That distinction matters because flags and positional args +// interleave in real examples: `band bxml speak --voice julie "Press 1 for +// sales."` has exactly one positional argument (the quoted string), but a +// naive scan that stops at the first "--x" token would see zero. +// +// A flag's pflag.Flag.NoOptDefVal is empty only when the flag REQUIRES an +// explicit value (e.g. a string flag): in that case the next field is +// consumed as its value and excluded from the result. Bool flags set +// NoOptDefVal ("true") so `--wait` alone doesn't eat the next token. +// +// ok is false when a flag can't be resolved against the command (unknown to +// Flags/InheritedFlags/the root persistent flags) — in that case we don't +// know whether it consumes the next token, so the caller should not trust +// the result rather than guess. +func splitPositionalArgs(cmd *cobra.Command, fields []string) (args []string, ok bool) { + for i := 0; i < len(fields); i++ { + f := fields[i] + if strings.HasPrefix(f, "#") { + break // trailing shell comment + } + if !strings.HasPrefix(f, "-") || f == "-" { + args = append(args, f) + continue + } + if strings.Contains(f, "=") { + continue // "--flag=value" is one self-contained token + } + name := strings.TrimLeft(f, "-") + fl := cmd.Flags().Lookup(name) + if fl == nil { + fl = cmd.InheritedFlags().Lookup(name) + } + if fl == nil { + fl = rootCmd.PersistentFlags().Lookup(name) + } + if fl == nil { + return nil, false // unresolvable flag; don't guess + } + if fl.NoOptDefVal == "" && i+1 < len(fields) { + i++ // this flag requires an explicit value; skip it too + } + } + return args, true +} + +// argsGateRejects is the second, authoritative gate on top of +// resolvedCommandAbsorbsRemainder. That heuristic only asks "does this +// remainder *look* positional" (via the resolved command's Use string) — +// which is blind to the case where `path` resolves fully (matched == +// len(path), so there's no `path` remainder for the heuristic to examine at +// all) but the resolved command's actual runtime Args validator no longer +// accepts what follows it in the doc line. That's exactly the shape of the +// drift this PR creates: `band tendlc number ` resolves completely to +// the `number` dispatcher (matched == len(path) == 2), yet `number`'s Args +// is cobra.NoArgs as of the 10DLC PR5 cutover (task 3 of +// .superpowers/sdd/2026-08-21-tendlc-pr5-cutover), which restructured it to +// require an explicit get/list/history subcommand instead of a bare +// phone-number argument. +// +// It re-resolves the command from s's raw fields rather than reusing the +// caller's `path`/`matched`: `path` was built from commandTokenRe, a regex +// that (being ignorant of the real command tree) can incorrectly exclude a +// real subcommand name that doesn't fit its lowercase-letters-and-hyphens +// shape — e.g. "resend-2fa" (has a digit) — which would otherwise make this +// gate blame the PARENT command for rejecting what is actually a perfectly +// valid subcommand name. +// +// This deliberately ABSTAINS (returns nil — "no drift found") rather than +// flag, whenever it can't parse the line with confidence: +// - shellFields reports an unterminated quote/bracket (see its doc comment); +// - splitPositionalArgs can't resolve a flag; +// - there's no remainder at all — a bare mention like "`band portin get`", +// naming a command without demonstrating a full invocation, is normal +// technical writing and must not be required to show every argument; +// - a positional token is a literal "..." or contains a "," — both strong +// signals of deliberately elided/abbreviated example text (e.g. `band +// tendlc campaign create --plain`) +// rather than a literal, runnable argument list; +// - a positional token is a lone "\" — a shell line-continuation marker +// from a multi-line example, meaning the real argument is on the next +// line and this test only ever looks at one line at a time. +// +// Abstaining trades a known blind spot (a genuinely bogus example of one of +// these shapes would also slip through unflagged) for not flagging real, +// legitimate documentation — the right side to err on: a gate that cries +// wolf on correct docs gets ignored or its escape hatch gets widened, which +// is strictly worse than a gate with a named, narrow blind spot. +// +// Checked every `Args:` site in cmd/ as of this task (98 positional-taking +// commands): all are cobra.ExactArgs/MinimumNArgs/MaximumNArgs/RangeArgs/ +// NoArgs, which only count arguments, never inspect their values — so this +// gate never needs to worry about a value-inspecting validator (e.g. +// cobra.OnlyValidArgs) rejecting a correct placeholder like "". +// Re-check this claim if such a validator is ever added. +func argsGateRejects(s string) error { + fields, ok := shellFields(s) + if !ok { + return nil + } + cmd, matched := resolveCommand(fields) + if matched == 0 || matched >= len(fields) { + return nil + } + pos, ok := splitPositionalArgs(cmd, fields[matched:]) + if !ok || len(pos) == 0 { + return nil + } + for _, a := range pos { + if a == "..." || a == `\` || strings.Contains(a, ",") { + return nil + } + // A "<...>"/"[...]" placeholder that itself spans multiple words + // (e.g. "", "") isn't a single positional value — + // every genuine one-argument placeholder in this codebase's Use + // strings is one hyphenated word ("", ""). + // A multi-word one is prose shorthand for "insert several flags + // here", not a literal argument; quoted multi-word strings (merged + // above by shellFields) are unaffected since they don't start with + // "<"/"[". + if (strings.HasPrefix(a, "<") || strings.HasPrefix(a, "[")) && strings.ContainsAny(a, " \t") { + return nil + } + } + if cmd.Args == nil { + return nil + } + if err := cmd.Args(cmd, pos); err != nil { + return fmt.Errorf("resolved command %q rejects the documented arguments %v: %w", cmd.CommandPath(), pos, err) + } + return nil +} + // commandPathTokens splits s into whitespace-separated fields and returns the // leading run of fields that look like command tokens (per commandTokenRe). // The first field that isn't command-shaped (a flag, placeholder, ID, phone @@ -159,11 +370,13 @@ func flagExists(c *cobra.Command, name string) bool { return rootCmd.PersistentFlags().Lookup(name) != nil } -// TestParserDistinguishesSubcommandsFromPositionals exercises the boundary -// logic (resolveCommand + resolvedCommandAbsorbsRemainder) directly against -// the real command tree, independent of any doc file. It proves the fix -// actually catches the class of drift this PR creates (a stale reference to -// a deleted multi-word command) without also flagging legitimate positional +// TestParserDistinguishesSubcommandsFromPositionals exercises the full +// three-gate boundary logic (resolveCommand + resolvedCommandAbsorbsRemainder +// + argsGateRejects) directly against the real command tree, independent of +// any doc file. It proves the fix actually catches the class of drift this +// PR creates — both the stale-multi-word-command shape (caught by the first +// two gates) and the fully-resolves-but-rejects-the-args shape (caught only +// by argsGateRejects) — without also flagging legitimate positional // arguments that happen to be lowercase words. func TestParserDistinguishesSubcommandsFromPositionals(t *testing.T) { tests := []struct { @@ -186,6 +399,18 @@ func TestParserDistinguishesSubcommandsFromPositionals(t *testing.T) { commandLine: "notacommand", wantFlagged: true, }, + { + // The legacy bare `number ` (a different command from the + // current `number get `) — resolves fully to the `number` + // dispatcher (matched == len(path)), so the Use-heuristic gate + // alone can't see anything wrong; only argsGateRejects, which + // calls numberCmd's real Args (cobra.NoArgs) against the + // documented phone number, catches it. Reserved-range number + // (555-01xx block), not a real one. + name: "deleted bare `tendlc number `: resolves fully but numberCmd's Args rejects it", + commandLine: "tendlc number +15555550100", + wantFlagged: true, + }, { name: "auth use admin: admin is a positional profile name, not a command", commandLine: "auth use admin", @@ -216,11 +441,18 @@ func TestParserDistinguishesSubcommandsFromPositionals(t *testing.T) { } cmd, matched := resolveCommand(path) // Mirrors exactly what TestDocumentedCommandsAndFlagsExist does - // with the result of resolveCommand. + // with the result of resolveCommand: gate 1 (existence), gate 2 + // (Use-heuristic), then gate 3 (the real Args validator) only if + // gates 1 and 2 both passed. flagged := matched == 0 || !resolvedCommandAbsorbsRemainder(cmd, path, matched) + var argsErr error + if !flagged { + argsErr = argsGateRejects(tt.commandLine) + flagged = argsErr != nil + } if flagged != tt.wantFlagged { - t.Errorf("band %s: flagged = %v, want %v (matched=%d, path=%v, resolved=%q)", - tt.commandLine, flagged, tt.wantFlagged, matched, path, cmd.CommandPath()) + t.Errorf("band %s: flagged = %v, want %v (matched=%d, path=%v, resolved=%q, argsGateRejects=%v)", + tt.commandLine, flagged, tt.wantFlagged, matched, path, cmd.CommandPath(), argsErr) } }) } @@ -270,6 +502,10 @@ func TestDocumentedCommandsAndFlagsExist(t *testing.T) { if !resolvedCommandAbsorbsRemainder(cmd, path, matched) { t.Errorf("%s documents `band %s …` but only `band %s` resolves; %q has no declared positional args to absorb %q", doc, cmdName, strings.Join(path[:matched], " "), cmd.CommandPath(), strings.Join(path[matched:], " ")) + continue + } + if err := argsGateRejects(rest); err != nil { + t.Errorf("%s documents `band %s …` but %v", doc, cmdName, err) } continue } @@ -307,6 +543,10 @@ func TestDocumentedCommandsAndFlagsExist(t *testing.T) { doc, cmdName, strings.Join(path[:matched], " "), cmd.CommandPath(), strings.Join(path[matched:], " ")) continue } + if err := argsGateRejects(capture); err != nil { + t.Errorf("%s documents `band %s …` but %v", doc, cmdName, err) + continue + } for _, fm := range flagRe.FindAllStringSubmatch(capture, -1) { flag := fm[1] // Cobra auto-injects --help on every command; skip it. From 0a287798b73d763eab81e03d10fd5d2d923d329f Mon Sep 17 00:00:00 2001 From: Kush Date: Sun, 23 Aug 2026 21:15:26 -0500 Subject: [PATCH 09/15] docs: retire the deleted commands and drop every doc-contract suppression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parser fix landed with five knownDriftCommands entries: four for real drift the deletion left behind, one for a shell comment that parsed as a command. All five are now unnecessary. The six stale lines are rewritten to the new tree, the numbers block no longer advertises a --status filter the API silently ignores, and the parser skips shell comments inside fenced blocks — prose that mentions a command mid-sentence is not an invocation. knownDriftCommands is empty. Verified by planting three shapes of stale reference and confirming each is caught: a deleted subcommand, a deleted positional form, and a bare argument on a parent that takes none. --- AGENTS.md | 84 ++++++++++++++++++++++------------------- README.md | 26 ++++++------- cmd/doccontract_test.go | 18 +++++---- 3 files changed, 69 insertions(+), 59 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5107553..bff3fbb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -207,7 +207,7 @@ All read operations (gets, lists, deletes) are safe to retry. Use `--wait` to block until completion: ```bash -band number order +19195551234 --subaccount --wait # blocks until number is active (30s default) +band number order +15555550100 --subaccount --wait # blocks until number is active (30s default) band call create --from ... --to ... --wait --timeout 120 # blocks until call completes band transcription create --wait # blocks until transcription ready (60s default) ``` @@ -377,7 +377,7 @@ When `--wait` times out (exit code 5), the operation may have succeeded — the Use when no credentials exist yet. The CLI submits the registration request; the remaining setup happens in the browser. **An agent cannot complete this flow autonomously** — it requires a human (or an agent with web/phone access) to finish. ```bash -band account register --phone +19195551234 --email you@example.com --first-name Jane --last-name Doe --accept-tos +band account register --phone +15555550100 --email you@example.com --first-name Jane --last-name Doe --accept-tos # → registration submitted; remaining steps happen outside the CLI: # 1. Check email for a registration link from Bandwidth # 2. Enter the OTP code sent via SMS to verify the phone number @@ -489,8 +489,8 @@ band vcp list --plain # VCPs? (403 = legacy account, use s For messaging readiness, also check: ```bash -band tendlc campaigns --plain # any 10DLC campaigns? (403 = see note below) -band tendlc number --plain # is a specific number registered? +band tendlc campaign list --plain # any 10DLC campaigns? (403 = see note below) +band tendlc number get --plain # is a specific number registered? band tfv get --plain # toll-free verification status? ``` @@ -561,7 +561,7 @@ band message send --from --to --app-id --text "H |---|---|---| | `"not linked to any location"` | App not assigned to a location | `band app assign --site --location ` | | `"no working callback URL"` | Callback URL is placeholder or missing | `band app update --callback-url ` | -| `"not assigned to any active 10DLC campaign"` | Number not on a campaign | `band tendlc campaigns --plain` to list campaigns; `band tnoption assign --campaign-id ` to assign | +| `"not assigned to any active 10DLC campaign"` | Number not on a campaign | `band tendlc campaign list --plain` to list campaigns; `band tnoption assign --campaign-id ` to assign | | `"toll-free verification status"` | TFV not approved | `band tfv get --plain` to check status | ### Send a message @@ -569,7 +569,7 @@ band message send --from --to --app-id --text "H Once provisioning is set up, sending is straightforward: ```bash -band message send --from +19195551234 --to +15559876543 --app-id abc-123 --text "Hello from the agent" +band message send --from +15555550100 --to +15559876543 --app-id abc-123 --text "Hello from the agent" # → preflight checks pass (app linked, callback URL valid, number on campaign) # → returns JSON with message id, segmentCount, direction ``` @@ -580,30 +580,30 @@ band message send --from +19195551234 --to +15559876543 --app-id abc-123 --text ```bash MEDIA_URL=$(band message media upload image.png) -band message send --from +19195551234 --to +15559876543 --app-id abc-123 --text "Check this out" --media "$MEDIA_URL" +band message send --from +15555550100 --to +15559876543 --app-id abc-123 --text "Check this out" --media "$MEDIA_URL" ``` **Group messaging** uses the same `send` command with multiple recipients: ```bash -band message send --from +19195551234 --to +15551234567,+15552345678 --app-id abc-123 --text "Team update" +band message send --from +15555550100 --to +15551234567,+15552345678 --app-id abc-123 --text "Team update" ``` **Listing messages** requires at least one filter and **millisecond-precision timestamps** (a common agent mistake): ```bash # Correct — milliseconds in the timestamp: -band message list --from +19195551234 --start-date 2024-01-01T00:00:00.000Z --plain +band message list --from +15555550100 --start-date 2024-01-01T00:00:00.000Z --plain # Wrong — this returns a 400: -band message list --from +19195551234 --start-date 2024-01-01T00:00:00Z --plain +band message list --from +15555550100 --start-date 2024-01-01T00:00:00Z --plain ``` ### Make a call ```bash -band number list --plain # → ["+19195551234", ...] +band number list --plain # → ["+15555550100", ...] band app list --plain # → [{"ApplicationId":"abc-123", ...}, ...] -band call create --from +19195551234 --to +15559876543 --app-id abc-123 --answer-url +band call create --from +15555550100 --to +15559876543 --app-id abc-123 --answer-url # → returns JSON with callId # IMPORTANT: always verify the call actually connected @@ -636,7 +636,7 @@ band transcription create --wait --plain # blocks unti **Look up a specific number's VCP:** ```bash -band number get +19195551234 --plain # → shows VCP assignment and voice settings +band number get +15555550100 --plain # → shows VCP assignment and voice settings ``` **List all numbers on a VCP:** @@ -666,13 +666,13 @@ band portin validate-tf +18005551234 --wait --plain ```bash band portin create \ - --numbers +19195551234,+19195551235 \ + --numbers +15555550100,+15555550100 \ --site --peer \ --foc 2026-06-01T15:30:00Z \ --loa-authorizing-person "Jane Doe" \ --loa ./loa.pdf \ --customer-order-id agent-run-42 --if-not-exists --plain -# → {"orderId":"...","status":"DRAFT","numbers":["+19195551234","+19195551235"], ...} +# → {"orderId":"...","status":"DRAFT","numbers":["+15555550100","+15555550100"], ...} ORDER_ID=$(... extract from above ...) band portin submit $ORDER_ID --wait --plain @@ -861,8 +861,8 @@ A 403 from `band tendlc` can mean: credential lacks the Campaign Management role ### Check if a number is registered for 10DLC ```bash -band tendlc number +19195551234 --plain -# → { "phoneNumber": "+19195551234", "campaignId": "CA3XKE1", "status": "SUCCESS", "brandId": "BEXMPL5", ... } +band tendlc number get +15555550100 --plain +# → { "phoneNumber": "+15555550100", "campaignId": "CEXMPL1", "status": "SUCCESS", "brandId": "BEXMPL5", ... } ``` Status values: `SUCCESS` (ready to send), `PROCESSING` (pending), `FAILURE` (registration failed). @@ -870,23 +870,29 @@ Status values: `SUCCESS` (ready to send), `PROCESSING` (pending), `FAILURE` (reg ### List all 10DLC campaigns ```bash -band tendlc campaigns --plain -# → [{ "campaignId": "CA3XKE1", "status": "SUCCESS", "brandId": "BEXMPL5", ... }, ...] +band tendlc campaign list --plain +# → [{ "campaignId": "CEXMPL1", "status": "SUCCESS", "brandId": "BEXMPL5", ... }, ...] ``` -### List all registered numbers (with filters) +### List all registered numbers ```bash -band tendlc numbers --plain # all registered numbers -band tendlc numbers --campaign-id CA3XKE1 --plain # numbers on a specific campaign -band tendlc numbers --status SUCCESS --plain # only successfully registered numbers -band tendlc numbers --status FAILURE --plain # numbers with registration failures +band tendlc number list --plain # all registered numbers +band tendlc number list --campaign-id-contains CEXMPL1 --plain # numbers on a specific campaign +band tendlc number list --all --plain # walk every page ``` +**There is no `--status` filter, and that is deliberate.** The API accepts a `status` +filter and silently ignores it: an account holding 21 `SUCCESS` and 2 `FAILURE` +numbers returns all 23 for `status[eq]=SUCCESS`, for `status[eq]=FAILURE`, and even +for a value matching nothing at all. Offering the flag would hand callers every +record while implying it was filtered. Filter client-side on the `status` field +instead — it is present on every record. + ### List numbers on a specific campaign ```bash -band tendlc campaigns numbers CA3XKE1 --plain +band tendlc campaign numbers CA3XKE1 --plain ``` ### Diagnose messaging send failures @@ -895,13 +901,13 @@ When `message send` fails with "not assigned to any active 10DLC campaign": ```bash # 1. Check the specific number's registration -band tendlc number +19195551234 --plain +band tendlc number get +15555550100 --plain # 2. If not registered, list available campaigns -band tendlc campaigns --plain +band tendlc campaign list --plain # 3. Assign the number to a campaign -band tnoption assign +19195551234 --campaign-id CA3XKE1 --wait +band tnoption assign +15555550100 --campaign-id CA3XKE1 --wait ``` **If `band tendlc` returns 403:** Don't retry — escalate. Tell the user: "Your credential may not have the Campaign Management role, or your account may not have the Registration Center feature enabled. Contact your Bandwidth account manager to check your configuration." @@ -1873,7 +1879,7 @@ band tfv submit +18005551234 \ --contact-first "Jane" \ --contact-last "Doe" \ --contact-email "jane@acme.com" \ - --contact-phone "+19195551234" \ + --contact-phone "+15555550100" \ --message-volume 10000 \ --use-case "2FA" \ --use-case-summary "Two-factor auth codes for user login" \ @@ -1922,14 +1928,14 @@ TN Option Orders assign phone numbers to 10DLC campaigns (and can set other per- ### Assign a number to a campaign ```bash -band tnoption assign +19195551234 --campaign-id CA3XKE1 --wait --plain +band tnoption assign +15555550100 --campaign-id CA3XKE1 --wait --plain # → order completes when status is COMPLETE ``` Multiple numbers in one order: ```bash -band tnoption assign +19195551234 +19195551235 --campaign-id CA3XKE1 --wait +band tnoption assign +15555550100 +15555550100 --campaign-id CA3XKE1 --wait ``` ### Check order status @@ -1944,14 +1950,14 @@ band tnoption get --plain ```bash band tnoption list --plain band tnoption list --status FAILED --plain -band tnoption list --tn +19195551234 --plain +band tnoption list --tn +15555550100 --plain ``` ### Common errors | Error code | Message | Cause | Fix | |---|---|---|---| -| **1022** | "TelephoneNumber is in an invalid format" | Number not in E.164 format | Pass numbers with `+` prefix: `+19195551234` | +| **1022** | "TelephoneNumber is in an invalid format" | Number not in E.164 format | Pass numbers with `+` prefix: `+15555550100` | | **12220** | "Campaign has been rejected by DCA2" | Campaign failed carrier compliance review | Fix campaign compliance in the Bandwidth App, then retry | | **5132** | "SMS attribute should be 'ON' for provisioning A2P" | SMS not enabled on the number's SIP peer/location | Enable SMS on the location in the Bandwidth App | | **5133** | "A2P provisioning requires A2P on corresponding Sip peer" | Location not configured for A2P messaging | Enable A2P on the location in the Bandwidth App | @@ -1960,23 +1966,23 @@ band tnoption list --tn +19195551234 --plain ```bash # 1. Check if number is on a campaign -band tendlc number +19195551234 --plain +band tendlc number get +15555550100 --plain # 2. If not, find an available campaign -band tendlc campaigns --plain +band tendlc campaign list --plain # 3. Assign the number (use full E.164 format with + prefix) -band tnoption assign +19195551234 --campaign-id CA3XKE1 --wait +band tnoption assign +15555550100 --campaign-id CA3XKE1 --wait # 4. If assign fails with 5132/5133, SMS or A2P isn't enabled on the # number's location — this must be fixed in the Bandwidth App before retrying # 5. Verify assignment -band tendlc number +19195551234 --plain +band tendlc number get +15555550100 --plain # → status should be SUCCESS # 6. Now send -band message send --from +19195551234 --to +15559876543 --app-id abc-123 --text "Hello" +band message send --from +15555550100 --to +15559876543 --app-id abc-123 --text "Hello" ``` ### Test SIP Trunking end-to-end @@ -1989,7 +1995,7 @@ Use this workflow to verify that a SIP realm and credential authenticate correct # 1. Pick a from number — must be on your account and voice-capable FROM=$(band number list --plain | jq -r '.[0]') # On Bandwidth Build accounts, band number list is not available. -# Pass the pre-provisioned number manually: FROM=+19195551234 +# Pass the pre-provisioned number manually: FROM=+15555550100 # 2. Create an ephemeral realm (never the default — it cannot be deleted if it is) REALM=$(band sip realm create --name sip-test --default=false --wait --plain) diff --git a/README.md b/README.md index 28c610a..996a9f6 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ Manage phone numbers, voice calls, and messaging from your terminal. No dashboard clicking, no API wrangling — just straightforward commands that get things done. ```sh -band call create --from +19195551234 --to +15559876543 --app-id abc-123 --answer-url https://example.com/answer +band call create --from +15555550100 --to +15559876543 --app-id abc-123 --answer-url https://example.com/answer ``` Built for humans, but agent-native from day one — every command supports `--plain` for flat JSON, `--if-not-exists` for safe retries, and `--wait` for async operations. If you're building an AI agent that provisions phone numbers or makes calls, this is the interface. @@ -83,7 +83,7 @@ No TTY required. Accounts are auto-discovered from the OAuth2 token. You can sign up for a Bandwidth Build trial account from the CLI: ```sh -band account register --phone +19195551234 --email you@example.com --first-name Jane --last-name Doe +band account register --phone +15555550100 --email you@example.com --first-name Jane --last-name Doe ``` You'll be prompted to accept the [Bandwidth Build Terms of Service](https://www.bandwidth.com/legal/build-terms-of-service/) before registration proceeds. For scripted usage, pass `--accept-tos`. @@ -138,7 +138,7 @@ Search for available numbers, then order one: ```sh band number search --area-code 919 --quantity 1 -band number order +19195551234 --subaccount --wait +band number order +15555550100 --subaccount --wait ``` The `--wait` flag blocks until the number is active, so you don't have to poll. @@ -149,7 +149,7 @@ Phone numbers don't do anything on their own — you need to tell Bandwidth how ```sh band vcp create --name "My VCP" --app-id -band vcp assign +19195551234 +band vcp assign +15555550100 ``` Now when someone calls that number, Bandwidth routes the call to your application's callback URL. @@ -164,7 +164,7 @@ If `vcp create` fails with a 403, your account uses the older sub-account model ```sh band call create \ - --from +19195551234 \ + --from +15555550100 \ --to +15559876543 \ --app-id \ --answer-url https://your-server.example.com/answer @@ -196,7 +196,7 @@ The CLI can generate BXML for you locally. No API calls, no auth required — it band bxml speak "Thanks for calling. How can we help?" band bxml speak --voice julie "Press 1 for sales." band bxml gather --url https://example.com/gather --max-digits 1 --prompt "Press a key" -band bxml transfer +19195551234 --caller-id +19195550000 +band bxml transfer +15555550100 --caller-id +19195550000 band bxml record --url https://example.com/done --max-duration 60 band bxml raw 'Hello' # validate and pretty-print XML ``` @@ -276,8 +276,8 @@ If you're sending from a standard 10-digit local number, it must be assigned to You can check registration status with `band tendlc`: ```sh -band tendlc number +19195551234 --plain # check a specific number -band tendlc campaigns --plain # list campaigns on your account +band tendlc number get +15555550100 --plain # check a specific number +band tendlc campaign list --plain # list campaigns on your account ``` These two commands are for **import** customers (accounts that register campaigns through TCR and import them to Bandwidth) — campaign registration for them still happens in the Bandwidth App; see [dev.bandwidth.com](https://dev.bandwidth.com/docs/messaging/campaign-management/) for the full guide. **Direct** customers register brands and campaigns via `band tendlc brand create` and `band tendlc campaign create` (see [AGENTS.md](AGENTS.md#10dlc-campaigns) for the create requirement tree). Once you have a campaign either way, assign numbers to it with `band tnoption assign`. @@ -306,9 +306,9 @@ A fresh UP account typically has one sub-account and one location already create ```sh band number list # list your numbers band number search --area-code 919 --quantity 5 # search available numbers -band number order +19195551234 --subaccount --wait # order (blocks until active) -band number activate +19195551234 --voice-inbound --wait # turn on inbound voice -band number release +19195551234 # release a number +band number order +15555550100 --subaccount --wait # order (blocks until active) +band number activate +15555550100 --voice-inbound --wait # turn on inbound voice +band number release +15555550100 # release a number ``` ### Messaging @@ -334,7 +334,7 @@ band message media upload image.png # prints media URL to stdout ### Calls ```sh -band call create --from +19195551234 --to +15559876543 --app-id abc-123 --answer-url https://example.com/answer +band call create --from +15555550100 --to +15559876543 --app-id abc-123 --answer-url https://example.com/answer band call get # check state band call hangup # hang up band call update --redirect-url # redirect active call @@ -359,7 +359,7 @@ band subaccount create --name "My Subaccount" band location create --subaccount --name "My Location" band app create --name "My Voice App" --type voice --callback-url https://your-server.example.com/callbacks band number search --area-code 919 --quantity 1 -band number order +19195551234 --subaccount --wait +band number order +15555550100 --subaccount --wait ``` Sub-accounts (formerly known as sites) are the top-level container. Locations (formerly known as SIP peers) sit inside sub-accounts and define where numbers get routed. The flow is: sub-account → location → application → number. diff --git a/cmd/doccontract_test.go b/cmd/doccontract_test.go index 406422b..3573dba 100644 --- a/cmd/doccontract_test.go +++ b/cmd/doccontract_test.go @@ -56,13 +56,7 @@ var knownDrift = map[string]bool{ // tendlc number +15555550100` exits 1 against the built binary) — and, // like the three entries above, deferred to task 6's doc sweep rather // than fixed here. -var knownDriftCommands = map[string]bool{ - "tendlc campaigns": true, - "tendlc numbers": true, - "tendlc campaigns numbers": true, - "number list is not": true, - "tendlc number": true, -} +var knownDriftCommands = map[string]bool{} // bandUsageRe captures everything after "band " to end of line (GREEDY — a // non-greedy capture would stop at the first space and truncate multi-word @@ -465,6 +459,16 @@ func TestDocumentedCommandsAndFlagsExist(t *testing.T) { t.Fatalf("reading %s: %v", doc, err) } for _, line := range strings.Split(string(raw), "\n") { + // Shell comments inside fenced blocks are prose, not runnable + // commands. They routinely mention a command mid-sentence — e.g. + // "# On Build accounts, band number list is not available." — and + // parsing them treats the following English words as a command + // path. Skipping them removes a whole class of false positive at + // the cost of not validating commands that appear only inside a + // comment, which are by definition not invocations. + if strings.HasPrefix(strings.TrimSpace(line), "#") { + continue + } // Table rows: only the command in column 1 is validated for existence; // description-column flags are intentionally NOT checked (they're prose // mentions, not usage). This avoids false positives from flags named in From 4be8b49d3c8f41dd9126af86c94107bc03512ffd Mon Sep 17 00:00:00 2001 From: Kush Date: Sun, 23 Aug 2026 21:27:44 -0500 Subject: [PATCH 10/15] feat(tendlc): retry once when a PUT rejects fields we can safely drop brand update and campaign update build a full-replacement PUT body by stripping a known list of read-only keys from the resource the API just returned. That only works because production currently accepts read-only fields it does not use. If that is ever tightened to a 400, both commands break the same day, since the strip lists cannot enumerate every field the API might start rejecting. Add putReplaceWithReadOnlyRetry, shared by UpdateBrand and UpdateCampaign: on a 400 whose error source.POINTER values name top-level fields present in the outgoing body, strip exactly those fields and retry once, noting the drop on stderr. Any other 400, or a retry that also fails, surfaces untouched/original. No loop, no backoff, no mutation of the shared strip lists. --- internal/tendlc/campaignwrite.go | 6 +- internal/tendlc/putretry.go | 132 +++++++++++++++ internal/tendlc/putretry_test.go | 272 +++++++++++++++++++++++++++++++ internal/tendlc/write.go | 6 +- 4 files changed, 414 insertions(+), 2 deletions(-) create mode 100644 internal/tendlc/putretry.go create mode 100644 internal/tendlc/putretry_test.go diff --git a/internal/tendlc/campaignwrite.go b/internal/tendlc/campaignwrite.go index 9c6e4ea..b2191de 100644 --- a/internal/tendlc/campaignwrite.go +++ b/internal/tendlc/campaignwrite.go @@ -27,11 +27,15 @@ func (s *Service) CreateCampaign(body map[string]any) (*api.Envelope, error) { // unchanged: it is not a replacement there. Campaigns carry no version // field, so there is no optimistic-locking check either way: a concurrent // edit between the GET and this PUT is lost silently. +// +// The PUT goes through putReplaceWithReadOnlyRetry, which retries once, with +// the named fields stripped, if the API rejects a 400 on read-only keys +// campaignReadOnlyFields already tried to remove — see that function for why. func (s *Service) UpdateCampaign(campaignID string, body map[string]any) (*api.Envelope, error) { if campaignID == "" { return nil, fmt.Errorf("campaign ID is required") } - raw, err := s.client.PutRawJSON(s.campaignPath(campaignID), body) + raw, err := putReplaceWithReadOnlyRetry(s.client, s.campaignPath(campaignID), body) if err != nil { return nil, err } diff --git a/internal/tendlc/putretry.go b/internal/tendlc/putretry.go new file mode 100644 index 0000000..2286341 --- /dev/null +++ b/internal/tendlc/putretry.go @@ -0,0 +1,132 @@ +package tendlc + +import ( + "encoding/json" + "fmt" + "os" + "sort" + "strings" + + "github.com/Bandwidth/cli/internal/api" +) + +// putReplaceWithReadOnlyRetry PUTs body to path and, on a specific and +// recognizable failure, retries exactly once. +// +// Both `brand update` and `campaign update` build body by copying the +// resource the API just returned and stripping a known list of read-only +// keys (brandReadOnlyFields, campaignReadOnlyFields) before overlaying the +// caller's changes. That only works because production currently ACCEPTS +// read-only fields it does not use — measured, and flagged to the API team as +// a behavior this CLI depends on. If that is ever tightened to a 400 without +// warning, both commands break the same day, because the strip lists cannot +// enumerate every field the API might start rejecting. +// +// The failure is recognizable: a 400 whose error pointers name fields we +// actually sent. On exactly that shape, this drops the named fields from a +// copy of body and PUTs once more. Any other 400 — one naming a field we did +// not send, or with no usable pointers at all — is a genuine validation +// failure and passes straight through. So does every non-400 status; a 409 +// (conflict) or 422 means something other than "the API rejected a +// read-only field", and looping or guessing there would turn a clear error +// into a confusing double-request. +// +// On success after a retry, a note naming the dropped fields goes to stderr: +// a silent self-heal would hide an API change worth knowing about. On a +// retry that also fails, the ORIGINAL error is returned, not the retry's — +// the first response is the one that describes what the caller actually +// sent. +// +// UpdateBrand and UpdateCampaign share this rather than each rolling their +// own copy: both are already identical PUT-then-parse-envelope shapes, and +// this series has already had one bug from a fix landing on one arm and not +// its twin. +func putReplaceWithReadOnlyRetry(client *api.Client, path string, body map[string]any) ([]byte, error) { + raw, err := client.PutRawJSON(path, body) + if err == nil { + return raw, nil + } + + apiErr, ok := err.(*api.APIError) + if !ok || apiErr.StatusCode != 400 { + return nil, err + } + + named := topLevelPointerFields(apiErr.Body) + var drop []string + for _, f := range named { + if _, present := body[f]; present { + drop = append(drop, f) + } + } + if len(drop) == 0 { + // Nothing named in the error is a field we sent: this is a genuine + // validation failure (or names only nested pointers), not the shape + // this retry exists for. + return nil, err + } + + retryBody := make(map[string]any, len(body)) + for k, v := range body { + retryBody[k] = v + } + for _, f := range drop { + delete(retryBody, f) + } + + raw2, err2 := client.PutRawJSON(path, retryBody) + if err2 != nil { + // The retry's own failure is discarded on purpose: the original error + // is the one that describes what the caller did. + return nil, err + } + + sort.Strings(drop) + fmt.Fprintf(os.Stderr, "note: retried after dropping field(s) the API rejected but does not need: %s\n", + strings.Join(drop, ", ")) + return raw2, nil +} + +// topLevelPointerFields parses an API error body shaped like +// {"errors":[{"source":{"POINTER":"/phone"}}], "links":[]} and returns the +// field names named by TOP-LEVEL, single-segment JSON pointers — "/phone" +// contributes "phone". A pointer with more than one segment, e.g. +// "/accounts[0]/customerProfileId", names a field nested inside a structure, +// not a top-level key of the body we sent: dropping the whole top-level key +// on that signal would discard data the caller supplied that has nothing to +// do with the rejected sub-field. Those pointers are excluded here by +// requiring the part after the leading "/" to contain no further "/". +// +// Malformed or unparseable bodies yield no fields, which the caller treats +// as "pass through, no retry" the same as a 400 with no usable pointers. +func topLevelPointerFields(body string) []string { + var parsed struct { + Errors []struct { + Source struct { + Pointer string `json:"POINTER"` + } `json:"source"` + } `json:"errors"` + } + if err := json.Unmarshal([]byte(body), &parsed); err != nil { + return nil + } + + seen := make(map[string]bool) + var out []string + for _, e := range parsed.Errors { + p := e.Source.Pointer + if !strings.HasPrefix(p, "/") { + continue + } + field := p[1:] + if field == "" || strings.Contains(field, "/") { + continue + } + if seen[field] { + continue + } + seen[field] = true + out = append(out, field) + } + return out +} diff --git a/internal/tendlc/putretry_test.go b/internal/tendlc/putretry_test.go new file mode 100644 index 0000000..c99e828 --- /dev/null +++ b/internal/tendlc/putretry_test.go @@ -0,0 +1,272 @@ +package tendlc + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "reflect" + "sort" + "strings" + "testing" + + "github.com/Bandwidth/cli/internal/api" +) + +// retryStubResponse is one canned response in a sequence a stub server hands +// back, one per request received, in order. +type retryStubResponse struct { + status int + body string +} + +// newRetryStub serves responses in sequence and records each request's +// decoded JSON body plus a running count, so tests can assert on what the +// SECOND request actually sent — not just on the error the first one +// returned. +func newRetryStub(t *testing.T, responses []retryStubResponse) (client *api.Client, bodies *[]map[string]any, count *int) { + t.Helper() + var gotBodies []map[string]any + n := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + b, _ := io.ReadAll(r.Body) + var decoded map[string]any + if len(b) > 0 { + _ = json.Unmarshal(b, &decoded) + } + gotBodies = append(gotBodies, decoded) + idx := n + n++ + if idx >= len(responses) { + t.Fatalf("unexpected request #%d; only %d responses configured", idx+1, len(responses)) + } + resp := responses[idx] + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(resp.status) + if resp.body != "" { + _, _ = io.WriteString(w, resp.body) + } + })) + t.Cleanup(srv.Close) + return api.NewClientNoAuth(srv.URL), &gotBodies, &n +} + +// captureStderr redirects os.Stderr for the duration of fn and returns what +// was written to it. Not run in parallel with other tests: os.Stderr is +// process-global. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + orig := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + os.Stderr = w + defer func() { os.Stderr = orig }() + + fn() + + if cerr := w.Close(); cerr != nil { + t.Fatalf("closing pipe writer: %v", cerr) + } + out, _ := io.ReadAll(r) + return string(out) +} + +// errorBodyNaming builds an API error body naming the given source pointers, +// in the shape documented on the brief: {"errors":[{...,"source":{"POINTER": +// "/phone"}}],"links":[]}. +func errorBodyNaming(pointers ...string) string { + var errs []string + for _, p := range pointers { + errs = append(errs, `{"type":"bad request","description":"must not be blank","source":{"POINTER":"`+p+`"}}`) + } + return `{"errors":[` + strings.Join(errs, ",") + `],"links":[]}` +} + +func TestPutRetry_ReadOnlyFieldWeSent_RetriesOnceAndSucceeds(t *testing.T) { + client, bodies, count := newRetryStub(t, []retryStubResponse{ + {status: 400, body: errorBodyNaming("/legacyFlag")}, + {status: 202, body: `{"data":{"bandwidthId":"BEXMPL1"}}`}, + }) + + body := map[string]any{"displayName": "Acme", "legacyFlag": true} + var raw []byte + var err error + stderr := captureStderr(t, func() { + raw, err = putReplaceWithReadOnlyRetry(client, "/thing/1", body) + }) + + if err != nil { + t.Fatalf("putReplaceWithReadOnlyRetry: %v", err) + } + if !strings.Contains(string(raw), "BEXMPL1") { + t.Errorf("raw response = %s, want it to contain the success body", raw) + } + if *count != 2 { + t.Fatalf("request count = %d, want exactly 2", *count) + } + second := (*bodies)[1] + if _, present := second["legacyFlag"]; present { + t.Errorf("second request body = %v, want legacyFlag stripped", second) + } + if second["displayName"] != "Acme" { + t.Errorf("second request body = %v, want displayName preserved", second) + } + if !strings.Contains(stderr, "legacyFlag") { + t.Errorf("stderr = %q, want it to name the dropped field", stderr) + } + + // The retry must not mutate the caller's own body map. + if _, present := body["legacyFlag"]; !present { + t.Error("caller's body map was mutated; legacyFlag should still be present in the original map") + } +} + +func TestPutRetry_FieldNotSent_PassesThroughNoSecondRequest(t *testing.T) { + client, _, count := newRetryStub(t, []retryStubResponse{ + {status: 400, body: errorBodyNaming("/neverSent")}, + }) + + body := map[string]any{"displayName": "Acme"} + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body) + + if err == nil { + t.Fatal("want an error, got nil") + } + apiErr, ok := err.(*api.APIError) + if !ok { + t.Fatalf("err = %T, want *api.APIError", err) + } + if apiErr.StatusCode != 400 { + t.Errorf("StatusCode = %d, want 400", apiErr.StatusCode) + } + if *count != 1 { + t.Errorf("request count = %d, want exactly 1 (no retry)", *count) + } +} + +func TestPutRetry_GenericBadRequest_NoUsablePointers_NoRetry(t *testing.T) { + client, _, count := newRetryStub(t, []retryStubResponse{ + {status: 400, body: `{"errors":[{"type":"bad request","description":"must not be blank"}],"links":[]}`}, + }) + + body := map[string]any{"displayName": ""} + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body) + + if err == nil { + t.Fatal("want an error, got nil") + } + if *count != 1 { + t.Errorf("request count = %d, want exactly 1 (no retry)", *count) + } +} + +func TestPutRetry_RetryAlsoFails_OriginalErrorSurfaces(t *testing.T) { + client, _, count := newRetryStub(t, []retryStubResponse{ + {status: 400, body: errorBodyNaming("/legacyFlag")}, + {status: 400, body: errorBodyNaming("/somethingElse")}, + }) + + body := map[string]any{"displayName": "Acme", "legacyFlag": true} + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body) + + if err == nil { + t.Fatal("want an error, got nil") + } + if !strings.Contains(err.Error(), "legacyFlag") { + t.Errorf("err = %v, want the ORIGINAL error (naming legacyFlag), not the retry's (naming somethingElse)", err) + } + if strings.Contains(err.Error(), "somethingElse") { + t.Errorf("err = %v, want the original error only, not the retry's body", err) + } + if *count != 2 { + t.Errorf("request count = %d, want exactly 2 (one retry, no more)", *count) + } +} + +func TestPutRetry_NestedPointer_NoRetry(t *testing.T) { + client, _, count := newRetryStub(t, []retryStubResponse{ + {status: 400, body: errorBodyNaming("/accounts[0]/customerProfileId")}, + }) + + body := map[string]any{"accounts": []any{map[string]any{"customerProfileId": "CEXMPL1"}}} + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body) + + if err == nil { + t.Fatal("want an error, got nil") + } + if *count != 1 { + t.Errorf("request count = %d, want exactly 1 (dropping the top-level key would lose caller data)", *count) + } +} + +func TestPutRetry_409_NoRetry(t *testing.T) { + client, _, count := newRetryStub(t, []retryStubResponse{ + {status: 409, body: `{"errors":[{"type":"conflict","description":"stale version","source":{"POINTER":"/legacyFlag"}}],"links":[]}`}, + }) + + body := map[string]any{"legacyFlag": true} + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body) + + if err == nil { + t.Fatal("want an error, got nil") + } + apiErr, ok := err.(*api.APIError) + if !ok || apiErr.StatusCode != 409 { + t.Fatalf("err = %v, want a 409 *api.APIError", err) + } + if *count != 1 { + t.Errorf("request count = %d, want exactly 1 (only a 400 triggers a retry)", *count) + } +} + +func TestPutRetry_HappyPath_NoStderrNote(t *testing.T) { + client, _, count := newRetryStub(t, []retryStubResponse{ + {status: 202, body: `{"data":{"bandwidthId":"BEXMPL1"}}`}, + }) + + body := map[string]any{"displayName": "Acme"} + var err error + stderr := captureStderr(t, func() { + _, err = putReplaceWithReadOnlyRetry(client, "/thing/1", body) + }) + + if err != nil { + t.Fatalf("putReplaceWithReadOnlyRetry: %v", err) + } + if *count != 1 { + t.Errorf("request count = %d, want exactly 1", *count) + } + if stderr != "" { + t.Errorf("stderr = %q, want empty on the happy path (no retry fired)", stderr) + } +} + +func TestTopLevelPointerFields(t *testing.T) { + tests := []struct { + name string + body string + want []string + }{ + {"single top-level pointer", errorBodyNaming("/phone"), []string{"phone"}}, + {"nested pointer excluded", errorBodyNaming("/accounts[0]/customerProfileId"), nil}, + {"root pointer excluded", errorBodyNaming("/"), nil}, + {"no source at all", `{"errors":[{"description":"bad"}],"links":[]}`, nil}, + {"malformed json", `not json`, nil}, + {"duplicate pointers deduped", errorBodyNaming("/phone", "/phone"), []string{"phone"}}, + {"multiple distinct pointers", errorBodyNaming("/phone", "/email"), []string{"phone", "email"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := topLevelPointerFields(tt.body) + sort.Strings(got) + want := append([]string(nil), tt.want...) + sort.Strings(want) + if !reflect.DeepEqual(got, want) { + t.Errorf("topLevelPointerFields(%q) = %v, want %v", tt.body, got, want) + } + }) + } +} diff --git a/internal/tendlc/write.go b/internal/tendlc/write.go index 327854a..44ff77b 100644 --- a/internal/tendlc/write.go +++ b/internal/tendlc/write.go @@ -26,11 +26,15 @@ func (s *Service) CreateBrand(body map[string]any) (*api.Envelope, error) { // which starts from the current resource so nothing is dropped. Brands carry // no version field, so there is no optimistic-locking check: a concurrent // edit between the GET and this PUT is lost silently. +// +// The PUT goes through putReplaceWithReadOnlyRetry, which retries once, with +// the named fields stripped, if the API rejects a 400 on read-only keys +// brandReadOnlyFields already tried to remove — see that function for why. func (s *Service) UpdateBrand(brandID string, body map[string]any) (*api.Envelope, error) { if brandID == "" { return nil, fmt.Errorf("brand ID is required") } - raw, err := s.client.PutRawJSON(s.brandPath(brandID), body) + raw, err := putReplaceWithReadOnlyRetry(s.client, s.brandPath(brandID), body) if err != nil { return nil, err } From a636cbf6f3c74855df43c04c82233ebd81650962 Mon Sep 17 00:00:00 2001 From: Kush Date: Mon, 24 Aug 2026 11:02:24 -0400 Subject: [PATCH 11/15] docs: document band tendlc number and finish the command-tree cutover --- AGENTS.md | 124 +++++++++++++++++++++++++++++++++++++++- README.md | 7 ++- cmd/doccontract_test.go | 7 --- 3 files changed, 128 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index bff3fbb..93d0af5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1339,6 +1339,13 @@ decision rather than merely motivating it — a poll here would hit a brand still holding its pre-update state and report success before the change actually took effect. +**A safety net, not a feature.** The read-modify-write PUT above depends on +production accepting read-only fields it doesn't use. If a future field is +ever rejected instead, the CLI drops exactly the field(s) the API's error +names and retries once, printing `note: retried after dropping field(s) the +API rejected but does not need: ` to stderr — it does not fail +outright or retry silently. + ### `list` vs `get`: projection, not nullability `brand list` returns a **12-key summary projection**; `brand get` returns @@ -1759,6 +1766,12 @@ success before the change — or the re-import above — actually took effect. No `--confirm` either: unlike `brand update`, a campaign update carries no fee and no identity reset, so there is no API-justified reason to gate it. +**A safety net, not a feature.** Like `brand update`, this read-modify-write +PUT depends on production accepting read-only fields it doesn't use. If a +field is ever rejected instead, the CLI drops exactly the named field(s) +and retries once, noting the drop on stderr, rather than failing outright +or silently swallowing the retry. + ### `create --wait`: exit codes and the receipt guarantee | Outcome | Exit | What's on stdout | @@ -1854,6 +1867,115 @@ filtering, and it isn't among the accepted query parameters for this endpoint in either spec file. Filter client-side on `--all`'s output instead. +## 10DLC Numbers + +`band tendlc number` looks up 10DLC phone number registration status: +`list`, `get `, and `history `. Unlike `brand`/`vetting`/`campaign`, +these three aren't direct-customer-only — an imported number's registration +record looks the same as a directly-registered one. Requires the same +Registration Center feature and Campaign Management role as the rest of +`tendlc`. + +### `list`: the projection is conditional, not fixed + +``` +$ band tendlc number list --plain --limit 2 +[ + { + "createdDate": "2025-05-30T20:53:26.046Z", + "modifiedDate": "2025-08-01T19:21:43.987Z", + "nnid": "100000", + "phoneNumber": "+15555550100", + "status": "SUCCESS" + }, + { + "createdDate": "2025-05-30T20:53:27.162Z", + "modifiedDate": "2025-11-07T21:03:41.065Z", + "nnid": "100001", + "phoneNumber": "+15555550101", + "status": "SUCCESS" + } +] +``` + +Every record carries those five keys. A number already assigned to a +campaign carries three more — `brandId`, `campaignId`, +`customerProfileId` — rather than the same five with the extra keys +present but empty: + +``` +{ + "brandId": "BEXMPL1", + "campaignId": "CEXMPL1", + "createdDate": "2025-09-19T14:20:15.189Z", + "customerProfileId": "ExampleProfileId000001", + "modifiedDate": "2026-05-08T10:29:45.789Z", + "nnid": "100010", + "phoneNumber": "+15555550110", + "status": "SUCCESS" +} +``` + +Measured across 23 numbers on a test account: 16 carried the base five +keys, and the 7 assigned to a campaign carried all eight. Don't assume a +record is missing the campaign fields — check for them rather than relying +on their absence. + +### Filters: `--campaign-id-contains` works, there is deliberately no `--status` + +`--campaign-id-contains` genuinely narrows results: `campaignId[contains]` +filters correctly against production — a campaign holding three numbers +returned exactly three, and a substring matching no campaign returned zero. +`campaignId[eq]` does not filter at all — like `status` below, it's +accepted and silently ignored, which is why the flag is named for what it +actually does (a substring match) rather than implying an exact match the +API can't perform. + +`status` is a different story, and there is no `--status` flag at all. +The API accepts a `status` filter and silently ignores it under **every** +operator — `eq`, `contains`, even a value matching nothing at all — always +returning every number on the account regardless. A filter that returns +every record with a 200 and no error is worse than an absent flag, because +the caller believes it worked. Filter client-side on `status` instead — +it's present on every record, in both projection shapes. + +For the campaign-scoped view — a different endpoint, not this one with a +filter — use `band tendlc campaign numbers ` (see +[10DLC Campaigns](#10dlc-campaigns)). + +### `get `: may 404 for numbers `list` returns + +``` +$ band tendlc number get +15555550100 --plain +Error: API request failed: API error 404: {"errors":[{"type":"not found","description":"+15555550100"}],"links":[]} +$ echo $? +3 +``` + +On the one account this was tested against, `get` 404s for every number +`list` returns, while `history` on the same phone number returns 200 for +all of them. The cause is unconfirmed and may be account-specific — this +API reports authorization failures as 403, not 404, so this isn't a +permissions mask in disguise, but one account isn't enough to call it a +confirmed API defect either. `list` and `history` both work normally; if +`get` 404s for you too, fall back to `list` (filtered client-side, or with +`--campaign-id-contains`) or `history`. + +### `history ` + +``` +$ band tendlc number history +15555550100 --plain --limit 2 +[ + { + "createdDate": "2025-08-01T19:21:43.987Z", + "message": "Published registration event for the TN for action: will do nothing" + } +] +``` + +As with brand and campaign history, this is a free-text activity log, +newest first, with no versioned snapshots and no per-entry fetch. + ## Toll-Free Verification (TFV) These commands manage toll-free number verification via the Athena v2 API. A 403 means the TFV role isn't enabled on the credential — contact your Bandwidth account manager to enable it. @@ -2116,7 +2238,7 @@ Every error in this table exits **4**, regardless of the HTTP status the API use - **No real-time call control.** The CLI can initiate calls and query state, but cannot receive or respond to mid-call callbacks. Dynamic call control requires a separate callback-handling server. - **No message delivery confirmation.** The CLI verifies your setup is correct before sending (app-location link, callback URL, campaign), but it cannot confirm whether a message was actually delivered. Delivery status (`message-delivered`, `message-failed`) arrives via webhooks on your callback server. The CLI's `message get` and `message list` return metadata only — not delivery status. - **No message content retrieval.** Bandwidth does not store message bodies. After sending, the message text is gone forever. `message get` and `message list` return timestamps, direction, and segment counts only. -- **10DLC: brand, vetting, and campaign registration are all in the CLI for direct customers.** `band tendlc brand`, `band tendlc vetting`, and `band tendlc campaign` register and manage the full chain. The CLI also lists campaigns, checks number registration status, diagnoses failures (`band tendlc`), and assigns numbers to campaigns (`band tnoption assign`), and blocks a `message send` if the source number isn't on an approved campaign. +- **10DLC: brand, vetting, and campaign registration are all in the CLI for direct customers.** `band tendlc brand`, `band tendlc vetting`, and `band tendlc campaign` register and manage the full chain. `band tendlc number` checks phone number registration status (`get`/`list`/`history`) for direct and import customers alike, `band tendlc campaign` lists campaigns and diagnoses failures, and `band tnoption assign` assigns numbers to campaigns; a `message send` is blocked if the source number isn't on an approved campaign. - **TFV is check-and-submit.** The CLI can check toll-free verification status and submit new requests (`band tfv`), but cannot approve or expedite reviews — those happen on the carrier side. - **Porting is port-IN only.** `band portin` covers the six end-to-end flows that complete via the public API: TF validation, on-net domestic, automated off-net (Level 3), TF Phase 1 (gated), bulk, and lifecycle ops (notes, supp, cancel, history, doc upload). Out of scope: port-out (no public API), manual TF, internal TF, NASC manual override, and international ports — these need ops or the Dashboard. `band portin create` exits 4 if the account doesn't have `TOLL_FREE_AUTOMATION_PHASE_1` for a TF order. `band portin supp` defends against the documented Bandwidth API behavior where a supp returns 200 on PUT but error code 7300 on the next GET (Neustar never received it) — exits 1 with a clear message rather than silently succeeding. - **10DLC, TFV, and short code commands are role-gated.** A 403 can mean the credential lacks the required role (Campaign Management, TFV), the account doesn't have the Registration Center feature, or messaging isn't enabled. The CLI provides a diagnostic message — if it says "access denied," escalate to the Bandwidth account manager rather than retrying. diff --git a/README.md b/README.md index 996a9f6..f606472 100644 --- a/README.md +++ b/README.md @@ -477,9 +477,9 @@ Sub-accounts (formerly known as sites) are the top-level container. Locations (f | `band tnoption get ` | Check the status of a TN Option Order | | `band tnoption list` | List TN Option Orders (filter by `--status`, `--tn`) | -### 10DLC brands, vettings, and campaigns (direct customers) +### 10DLC brands, vettings, campaigns, and numbers -`band tendlc brand`, `band tendlc vetting`, and `band tendlc campaign` register and manage 10DLC brands and campaigns for accounts that register directly with TCR (not through import). Requires the Registration Center feature and Campaign Management role — check with `band tendlc status --plain`. A brand needs a customer profile first (`band customer-profile create`); see [AGENTS.md](AGENTS.md#10dlc-brands) for the full flag matrix, `--wait` semantics, and exit codes, and [AGENTS.md](AGENTS.md#10dlc-campaigns) for the campaign create requirement tree, the `imported` update branch, and the operational trap around editing a non-terminal campaign. +`band tendlc brand`, `band tendlc vetting`, and `band tendlc campaign` register and manage 10DLC brands and campaigns for accounts that register directly with TCR (not through import); `band tendlc number` looks up phone number registration status and works for direct and import customers alike. Requires the Registration Center feature and Campaign Management role — check with `band tendlc status --plain`. A brand needs a customer profile first (`band customer-profile create`); see [AGENTS.md](AGENTS.md#10dlc-brands) for the full flag matrix, `--wait` semantics, and exit codes, [AGENTS.md](AGENTS.md#10dlc-campaigns) for the campaign create requirement tree, the `imported` update branch, and the operational trap around editing a non-terminal campaign, and [AGENTS.md](AGENTS.md#10dlc-numbers) for the conditional list projection and the `get` 404 caveat. | Command | What it does | |---------|-------------| @@ -504,6 +504,9 @@ Sub-accounts (formerly known as sites) are the top-level container. Locations (f | `band tendlc campaign update ` | Update a campaign (read-modify-write; imported campaigns accept only `--campaign-name`) | | `band tendlc campaign deactivate ` | Permanently deactivate a campaign (`--confirm` required; irreversible) | | `band tendlc campaign nudge --intent ` | Ask TCR to re-evaluate a campaign (not billable, no `--confirm`) | +| `band tendlc number list` | List 10DLC phone number registrations (filter by `--campaign-id-contains`; no `--status` filter — the API silently ignores it, so filter client-side) | +| `band tendlc number get ` | Get one phone number's registration record (may 404 on some accounts even for numbers `list` returns) | +| `band tendlc number history ` | Show a phone number's activity log | ### SIP trunk authentication diff --git a/cmd/doccontract_test.go b/cmd/doccontract_test.go index 3573dba..348263c 100644 --- a/cmd/doccontract_test.go +++ b/cmd/doccontract_test.go @@ -15,15 +15,8 @@ import ( // - "location create::subaccount": tracked by the --site→--subaccount rename // spec (docs/superpowers/specs/2026-05-07-subaccount-rename.md). Remove when // that PR lands. -// - "tendlc numbers::campaign-id" and "tendlc numbers::status": the legacy -// `band tendlc numbers` command (and its --campaign-id/--status flags) was -// removed in the 10DLC PR5 cutover (task 3 of -// .superpowers/sdd/2026-08-21-tendlc-pr5-cutover); AGENTS.md's doc sweep is -// task 6 of that same plan. Remove when that PR lands. var knownDrift = map[string]bool{ "location create::subaccount": true, - "tendlc numbers::campaign-id": true, - "tendlc numbers::status": true, } // knownDriftCommands lists command paths that are known drift and From c9bdebd4b5bd42fa9800bb55b60028d378131fef Mon Sep 17 00:00:00 2001 From: Kush Date: Mon, 24 Aug 2026 11:04:31 -0400 Subject: [PATCH 12/15] docs: replace two real phone numbers with reserved-range placeholders Pre-existing on main, in the band number list example. This repo is public and the 919 numbers are real Bandwidth TNs; the rest of the docs already use the reserved 555-0100 block. --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 93d0af5..97b9b27 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -230,7 +230,7 @@ CLI will keep trying. **Always use `--plain` when parsing CLI output.** Default JSON reflects Bandwidth's API structure with deep nesting. `--plain` flattens it: ```bash -band number list --plain # → ["+19193554167", "+19198234157", ...] +band number list --plain # → ["+15555550100", "+15555550101", ...] band subaccount list --plain # → [{"Id":"152681","Name":"Subacct"}] band app list --plain # → [{"ApplicationId":"abc-123", ...}, ...] band app get --plain # → {"ApplicationId":"abc-123", "AppName":"My App", ...} From 43c883d180c4842d6687bb0efa9f0334bca882fc Mon Sep 17 00:00:00 2001 From: Kush Date: Mon, 24 Aug 2026 11:27:20 -0400 Subject: [PATCH 13/15] fix(tendlc): never let the PUT retry drop a field the caller set --- AGENTS.md | 2 +- cmd/doccontract_test.go | 33 +------ cmd/message/preflight.go | 6 +- cmd/tendlc/brand_update.go | 2 +- cmd/tendlc/campaign_update.go | 2 +- cmd/tendlc/number_test.go | 15 +++- cmd/tendlc/status.go | 1 + cmd/tendlc/status_test.go | 19 ++++ cmd/tendlc/tendlc_test.go | 10 +++ internal/tendlc/brandupdate.go | 19 ++++ internal/tendlc/campaignupdate.go | 47 ++++++++++ internal/tendlc/campaignwrite.go | 13 ++- internal/tendlc/campaignwrite_test.go | 4 +- internal/tendlc/numbers.go | 4 +- internal/tendlc/putretry.go | 33 ++++++- internal/tendlc/putretry_test.go | 124 +++++++++++++++++++++++--- internal/tendlc/write.go | 9 +- internal/tendlc/write_test.go | 4 +- 18 files changed, 280 insertions(+), 67 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 97b9b27..a5e27c8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2057,7 +2057,7 @@ band tnoption assign +15555550100 --campaign-id CA3XKE1 --wait --plain Multiple numbers in one order: ```bash -band tnoption assign +15555550100 +15555550100 --campaign-id CA3XKE1 --wait +band tnoption assign +15555550100 +15555550101 --campaign-id CA3XKE1 --wait ``` ### Check order status diff --git a/cmd/doccontract_test.go b/cmd/doccontract_test.go index 348263c..9b961f1 100644 --- a/cmd/doccontract_test.go +++ b/cmd/doccontract_test.go @@ -20,35 +20,10 @@ var knownDrift = map[string]bool{ } // knownDriftCommands lists command paths that are known drift and -// intentionally not yet reconciled — either a documented command path that -// no longer resolves, or (see the last entry) a prose false positive the -// parser can't tell apart from a real one. -// REMOVE entries here as the underlying drift is fixed. -// - "tendlc campaigns", "tendlc numbers", "tendlc campaigns numbers": the -// legacy `band tendlc campaigns`/`numbers`/`campaigns numbers` commands -// were removed in the 10DLC PR5 cutover (task 3 of -// .superpowers/sdd/2026-08-21-tendlc-pr5-cutover). Fixing the parser to -// catch this exact class of drift is task 4 of that plan; the doc sweep -// that removes these references (and these three entries) is task 6. -// - "number list is not": not command drift at all — a false positive from -// AGENTS.md's "# On Bandwidth Build accounts, band number list is not -// available." comment. The line has no backtick/code-span markers, so -// bandUsageRe (which matches "band " anywhere in a line) tokenizes the -// following prose words ("list", "is", "not") as if they were further -// command-path tokens, same as it would a real subcommand name. Task 6 -// rewords this line too; remove this entry alongside it. -// - "tendlc number": the legacy bare `band tendlc number ` — a -// *different* command from the still-current `number get ` — was -// also removed in task 3 of the same plan. This one resolves fully -// (`tendlc number` is a real command, the dispatcher parent), so it's -// invisible to the path-boundary checks above; argsGateRejects (added -// when this genuine gap was found during review) is what actually -// catches it, by calling numberCmd's real Args (cobra.NoArgs) against -// the documented phone-number argument and observing it reject. This is -// real drift, not a gate false positive — confirmed by hand (`band -// tendlc number +15555550100` exits 1 against the built binary) — and, -// like the three entries above, deferred to task 6's doc sweep rather -// than fixed here. +// intentionally not yet reconciled. It is intentionally EMPTY: the 10DLC +// PR5 cutover's doc sweep reconciled every entry this map used to carry, and +// it must stay empty — do not re-add a command path here as a shortcut past +// a doc-test failure; fix the doc (or the command) instead. var knownDriftCommands = map[string]bool{} // bandUsageRe captures everything after "band " to end of line (GREEDY — a diff --git a/cmd/message/preflight.go b/cmd/message/preflight.go index 6be2ded..3abc84e 100644 --- a/cmd/message/preflight.go +++ b/cmd/message/preflight.go @@ -165,7 +165,7 @@ func check10DLC(platClient *api.Client, acctID, number string) PreflightResult { if len(campaigns) == 0 { result.Ready = false result.Message = "no 10DLC campaigns found on this account — the number must be assigned to an approved campaign before messages will deliver.\n" + - "Check registration: band tendlc campaigns" + "Check registration: band tendlc campaign list" return result } @@ -198,8 +198,8 @@ func check10DLC(platClient *api.Client, acctID, number string) PreflightResult { // Not found on any campaign result.Ready = false result.Message = fmt.Sprintf("number is not assigned to any active 10DLC campaign — delivery will fail (error 4476).\n"+ - "Check registration status: band tendlc number %s\n"+ - "List campaigns: band tendlc campaigns\n"+ + "Check registration status: band tendlc number get %s\n"+ + "List campaigns: band tendlc campaign list\n"+ "Assign to a campaign: band tnoption assign %s --campaign-id ", number, number) return result } diff --git a/cmd/tendlc/brand_update.go b/cmd/tendlc/brand_update.go index 9995828..5b9f7cb 100644 --- a/cmd/tendlc/brand_update.go +++ b/cmd/tendlc/brand_update.go @@ -132,7 +132,7 @@ PUBLIC_PROFIT brand revokes Auth+ compliance.`, return err } - updated, err := svc.UpdateBrand(args[0], body) + updated, err := svc.UpdateBrand(args[0], body, changed) if err != nil { return brandUpdateConflictHint(args[0], err) } diff --git a/cmd/tendlc/campaign_update.go b/cmd/tendlc/campaign_update.go index 54223fb..640715e 100644 --- a/cmd/tendlc/campaign_update.go +++ b/cmd/tendlc/campaign_update.go @@ -122,7 +122,7 @@ campaigns, so there is no API-justified reason to gate this behind a flag.`, return err } - updated, err := svc.UpdateCampaign(args[0], body) + updated, err := svc.UpdateCampaign(args[0], body, changed) if err != nil { return campaignUpdateConflictHint(args[0], err) } diff --git a/cmd/tendlc/number_test.go b/cmd/tendlc/number_test.go index 829b345..bf46a4d 100644 --- a/cmd/tendlc/number_test.go +++ b/cmd/tendlc/number_test.go @@ -340,10 +340,17 @@ func TestNumberRoleGate403MapsToExitFour(t *testing.T) { // TestNumberCommandTreeHasNoLegacyFlatGet guards Task 3's removal of the // legacy flat `band tendlc number ` command: numberCmd (Use: "number") -// is now a plain parent, declared the same way as brandCmd and campaignCmd, -// with no RunE of its own. A bare `number ` therefore resolves to -// numberCmd itself with an unconsumed positional, not to a get-style -// command — there is no more shorthand for `number get `. This also +// is now a parent with only list/get/history children. Like brandCmd and +// campaignCmd, it DOES have its own RunE (calling cmd.Help()) -- deliberately, +// not decoratively: cobra's execute() checks Runnable() before it ever +// consults Args, so a parent with no RunE always short-circuits to +// flag.ErrHelp regardless of its Args setting, and removing this RunE would +// silently reintroduce the exit-0 bug this command tree was fixed to avoid. +// See numberCmd's own doc comment in number.go for the full explanation; do +// not remove it on the theory that a "plain parent" needs no RunE. A bare +// `number ` resolves to numberCmd itself with an unconsumed positional, +// not to a get-style command — there is no more shorthand for +// `number get `. This also // guards against a regression back to the pre-Task-3 collision risk: if a // future change ever adds a second sibling command also named "number", // this test's child-count assertion below would catch it, since cobra's diff --git a/cmd/tendlc/status.go b/cmd/tendlc/status.go index db94c69..8e95ed4 100644 --- a/cmd/tendlc/status.go +++ b/cmd/tendlc/status.go @@ -78,6 +78,7 @@ import them from TCR — is not probed and cannot be discovered: an account is o or the other, and that is a property of your Bandwidth setup. If you don't know which yours is, ask your Bandwidth account contact rather than guessing.`, Example: ` band tendlc status --plain`, + Args: cobra.NoArgs, // Contract: probe success is coupled to envelope decoding. svc.ListBrands // parses the response body before returning, so a 200 whose body is not // valid JSON (or whose envelope shape has changed) surfaces as an error diff --git a/cmd/tendlc/status_test.go b/cmd/tendlc/status_test.go index 7c000f9..09b9473 100644 --- a/cmd/tendlc/status_test.go +++ b/cmd/tendlc/status_test.go @@ -196,6 +196,25 @@ func TestTendlcStatus_MalformedSuccessBody(t *testing.T) { assertModeUnknown(t, got) } +// TestTendlcStatus_RejectsStrayPositional guards status.go's Args: cobra.NoArgs +// — before this, statusCmd was the only command in the tree with no Args +// guard at all, so `band tendlc status whatever` silently ignored the extra +// token and exited 0 instead of failing on the unrecognized argument. No +// stub service is installed: this must fail argument validation before ever +// calling `service`. +func TestTendlcStatus_RejectsStrayPositional(t *testing.T) { + root := testutil.NewTestRoot(statusCmd) + root.SetArgs([]string{"status", "whatever"}) + + var err error + testutil.CaptureStdout(t, func() { + err = root.Execute() + }) + if err == nil { + t.Fatal("Execute() error = nil, want a non-nil error for a stray positional") + } +} + // TestTendlcStatus_TransportFailure is the regression lock for the bug where // a bare transport error (never wrapped in *api.APIError) produced EMPTY // stdout: RunE fell straight through to roleGateError without emitting diff --git a/cmd/tendlc/tendlc_test.go b/cmd/tendlc/tendlc_test.go index cd8052f..00c96fa 100644 --- a/cmd/tendlc/tendlc_test.go +++ b/cmd/tendlc/tendlc_test.go @@ -195,11 +195,21 @@ func TestIsNotFound(t *testing.T) { // No stub server is passed (srv is nil in every case): all three must fail // before ever reaching a RunE that would call `service`, so this needs no // live API call and no credentials. +// +// {"brand", "STRAY"} and {"vetting", "STRAY"} extend this to the other two +// parents named in the doc comment above (Cmd itself is exercised by +// "campaigns"/"numbers"; numberCmd by "number +15555550100"; campaignCmd by +// "campaign STRAY" below) -- without this, brandCmd or vettingCmd losing its +// RunE would silently regress to exit 0 on a stray positional with nothing +// in this suite catching it. func TestRemovedLegacyCommandsExitNonZero(t *testing.T) { cases := [][]string{ {"campaigns"}, {"numbers"}, {"number", "+15555550100"}, + {"brand", "STRAY"}, + {"campaign", "STRAY"}, + {"vetting", "STRAY"}, } for _, args := range cases { t.Run(strings.Join(args, " "), func(t *testing.T) { diff --git a/internal/tendlc/brandupdate.go b/internal/tendlc/brandupdate.go index 22ce81e..94137e0 100644 --- a/internal/tendlc/brandupdate.go +++ b/internal/tendlc/brandupdate.go @@ -327,6 +327,25 @@ func IdentityFieldsChanged(current map[string]any, changed map[string]bool) []st return out } +// changedBrandJSONFields translates changed — keyed by CLI flag name, the +// same map BuildBrandUpdateRequest takes — into the JSON body keys those +// flags write, via brandFlagToField. UpdateBrand passes the result to +// putReplaceWithReadOnlyRetry as the set of fields the retry must never drop: +// a field the caller just told this call to set is never eligible to be +// silently stripped and re-sent, no matter what an error names. +func changedBrandJSONFields(changed map[string]bool) map[string]bool { + out := make(map[string]bool, len(changed)) + for flag, isChanged := range changed { + if !isChanged { + continue + } + if field, ok := brandFlagToField[flag]; ok { + out[field] = true + } + } + return out +} + // deepCopyBrandMap copies m so the result shares no mutable structure with it. // current is read from an api.Envelope the caller may reuse, so a shallow copy // would leave nested values (brand.accounts is a []any of maps) aliased diff --git a/internal/tendlc/campaignupdate.go b/internal/tendlc/campaignupdate.go index 1a26923..350dc0a 100644 --- a/internal/tendlc/campaignupdate.go +++ b/internal/tendlc/campaignupdate.go @@ -328,6 +328,53 @@ func ImportedCampaignRejectedFlags(changed map[string]bool) []string { return out } +// changedCampaignJSONFields translates changed — keyed by CLI flag name, the +// same map BuildCampaignUpdateRequest takes — into the JSON body keys those +// flags write, via campaignUpdateFlagToField. UpdateCampaign passes the +// result to putReplaceWithReadOnlyRetry (unioned with +// campaignNeverDropFields) as part of the set of fields the retry must never +// drop. +func changedCampaignJSONFields(changed map[string]bool) map[string]bool { + out := make(map[string]bool, len(changed)) + for flag, isChanged := range changed { + if !isChanged { + continue + } + if field, ok := campaignUpdateFlagToField[flag]; ok { + out[field] = true + } + } + return out +} + +// campaignNeverDropFields are response fields that ride along unmodified in +// the read-modify-write body without ever being reachable by any update +// flag: termsAndConditions, subscriberOptin, subscriberOptout, and +// subscriberHelp — the same 4 booleans campaignUpdateBoolFlags's doc comment +// documents as measured NOT editable on update (as opposed to fields in +// campaignReadOnlyFields, which are stripped before every PUT because they +// are genuinely server-owned). +// +// changedCampaignJSONFields alone cannot protect these: because no update +// flag ever reaches them, they can never appear in the changed map, so a +// guard keyed only on "did the caller ask about this field" would leave them +// exactly as droppable as before. But they are not read-only filler either — +// measured against production, a direct campaign holding false for these +// returns 400 "is required" naming exactly these pointers (the same shape +// ValidateCampaignCreate documents for create; see campaignoptions.go). That +// reads exactly like the shape this retry exists to handle — a 400 naming a +// field present in body — so without this explicit list, the retry would +// strip all three compliance attestations off an update that has nothing to +// do with them (e.g. a plain --description change) and report success. This +// list is therefore excluded from the retry's drop set unconditionally, not +// merely when unchanged this call. +var campaignNeverDropFields = map[string]bool{ + "termsAndConditions": true, + "subscriberOptin": true, + "subscriberOptout": true, + "subscriberHelp": true, +} + // deepCopyCampaignMap copies m so the result shares no mutable structure with // it. current is read from an api.Envelope the caller may reuse, so a // shallow copy would leave nested values aliased between the outgoing body diff --git a/internal/tendlc/campaignwrite.go b/internal/tendlc/campaignwrite.go index b2191de..c1701c0 100644 --- a/internal/tendlc/campaignwrite.go +++ b/internal/tendlc/campaignwrite.go @@ -31,11 +31,20 @@ func (s *Service) CreateCampaign(body map[string]any) (*api.Envelope, error) { // The PUT goes through putReplaceWithReadOnlyRetry, which retries once, with // the named fields stripped, if the API rejects a 400 on read-only keys // campaignReadOnlyFields already tried to remove — see that function for why. -func (s *Service) UpdateCampaign(campaignID string, body map[string]any) (*api.Envelope, error) { +// changed is the same caller-set flag map BuildCampaignUpdateRequest was +// built from; it is translated to the JSON keys the retry must never drop, +// and unioned with campaignNeverDropFields (fields no update flag can ever +// reach but that still hold real data), so neither category is ever +// silently stripped and re-sent. +func (s *Service) UpdateCampaign(campaignID string, body map[string]any, changed map[string]bool) (*api.Envelope, error) { if campaignID == "" { return nil, fmt.Errorf("campaign ID is required") } - raw, err := putReplaceWithReadOnlyRetry(s.client, s.campaignPath(campaignID), body) + neverDrop := changedCampaignJSONFields(changed) + for f := range campaignNeverDropFields { + neverDrop[f] = true + } + raw, err := putReplaceWithReadOnlyRetry(s.client, s.campaignPath(campaignID), body, neverDrop) if err != nil { return nil, err } diff --git a/internal/tendlc/campaignwrite_test.go b/internal/tendlc/campaignwrite_test.go index 239138c..1571a23 100644 --- a/internal/tendlc/campaignwrite_test.go +++ b/internal/tendlc/campaignwrite_test.go @@ -50,7 +50,7 @@ func TestUpdateCampaignPutsToCampaignPath(t *testing.T) { var got captured s := stubService(t, 202, `{"data":{"bandwidthId":"CEXMPL1"}}`, &got) - if _, err := s.UpdateCampaign("CEXMPL1", map[string]any{"campaignName": "Acme Alerts"}); err != nil { + if _, err := s.UpdateCampaign("CEXMPL1", map[string]any{"campaignName": "Acme Alerts"}, nil); err != nil { t.Fatalf("UpdateCampaign: %v", err) } if got.method != "PUT" { @@ -138,7 +138,7 @@ func TestEmptyCampaignIDsRejectedWithoutRequest(t *testing.T) { s := stubService(t, 200, `{"data":{}}`, &got) calls := map[string]func() error{ - "UpdateCampaign": func() error { _, err := s.UpdateCampaign("", map[string]any{}); return err }, + "UpdateCampaign": func() error { _, err := s.UpdateCampaign("", map[string]any{}, nil); return err }, "DeactivateCampaign": func() error { return s.DeactivateCampaign("") }, "NudgeCampaign": func() error { return s.NudgeCampaign("", map[string]any{}) }, "CampaignPhoneNumbers": func() error { diff --git a/internal/tendlc/numbers.go b/internal/tendlc/numbers.go index 6e0d024..26c020f 100644 --- a/internal/tendlc/numbers.go +++ b/internal/tendlc/numbers.go @@ -38,8 +38,8 @@ func (s *Service) ListPhoneNumbers(limit, offset int, filters []api.Filter) (*ap // numbers tested, while PhoneNumberHistory on the same path prefix returned // 200 for all four. The cause is unconfirmed — only one account was // available to test against, and this API reports authorization failures as -// 403, so a 404 here is not a permissions mask in disguise. The currently -// shipped `band tendlc number ` command already fails the same way. +// 403, so a 404 here is not a permissions mask in disguise. `band tendlc +// number get `, which calls this, inherits the same 404. func (s *Service) GetPhoneNumber(phoneNumber string) (*api.Envelope, error) { if phoneNumber == "" { return nil, fmt.Errorf("phone number is required") diff --git a/internal/tendlc/putretry.go b/internal/tendlc/putretry.go index 2286341..62690e0 100644 --- a/internal/tendlc/putretry.go +++ b/internal/tendlc/putretry.go @@ -31,6 +31,22 @@ import ( // read-only field", and looping or guessing there would turn a clear error // into a confusing double-request. // +// INVARIANT: the retry may only ever drop a field the caller did not ask +// about. That is the entire case the design contemplates — an API-side +// change to how some field the CLI merely echoes back is handled, never a +// value the caller explicitly asked this call to set. neverDrop is how that +// invariant is enforced: it is the set of JSON body keys this call must never +// remove, regardless of what the error names. UpdateBrand and UpdateCampaign +// each build it from two things: the JSON keys backing whatever flags the +// caller actually passed THIS call (so "brand update --website bad-url" can +// never have website silently dropped and re-sent), and, for campaigns, a +// fixed set of fields that are never reachable by any update flag at all but +// are still real data, not read-only filler (see campaignNeverDropFields). +// Without the second category, a field the CLI never lets the caller touch +// would look, from here, indistinguishable from a genuinely-inert read-only +// field — which is exactly the shape production returns for a direct +// campaign's subscriberOptin/subscriberOptout/subscriberHelp attestations. +// // On success after a retry, a note naming the dropped fields goes to stderr: // a silent self-heal would hide an API change worth knowing about. On a // retry that also fails, the ORIGINAL error is returned, not the retry's — @@ -41,7 +57,7 @@ import ( // own copy: both are already identical PUT-then-parse-envelope shapes, and // this series has already had one bug from a fix landing on one arm and not // its twin. -func putReplaceWithReadOnlyRetry(client *api.Client, path string, body map[string]any) ([]byte, error) { +func putReplaceWithReadOnlyRetry(client *api.Client, path string, body map[string]any, neverDrop map[string]bool) ([]byte, error) { raw, err := client.PutRawJSON(path, body) if err == nil { return raw, nil @@ -55,14 +71,23 @@ func putReplaceWithReadOnlyRetry(client *api.Client, path string, body map[strin named := topLevelPointerFields(apiErr.Body) var drop []string for _, f := range named { + if neverDrop[f] { + // The caller either explicitly asked to set this field this + // call, or it is a field that is never reachable by any flag but + // still holds real data (see neverDrop's callers). Either way, + // dropping it would silently discard something that matters more + // than the retry's convenience. + continue + } if _, present := body[f]; present { drop = append(drop, f) } } if len(drop) == 0 { - // Nothing named in the error is a field we sent: this is a genuine - // validation failure (or names only nested pointers), not the shape - // this retry exists for. + // Nothing named in the error is a droppable field we sent: this is a + // genuine validation failure (a field the caller set, a protected + // field, or a pointer naming only nested data), not the shape this + // retry exists for. return nil, err } diff --git a/internal/tendlc/putretry_test.go b/internal/tendlc/putretry_test.go index c99e828..a888a19 100644 --- a/internal/tendlc/putretry_test.go +++ b/internal/tendlc/putretry_test.go @@ -87,15 +87,26 @@ func errorBodyNaming(pointers ...string) string { func TestPutRetry_ReadOnlyFieldWeSent_RetriesOnceAndSucceeds(t *testing.T) { client, bodies, count := newRetryStub(t, []retryStubResponse{ - {status: 400, body: errorBodyNaming("/legacyFlag")}, + {status: 400, body: errorBodyNaming("/website")}, {status: 202, body: `{"data":{"bandwidthId":"BEXMPL1"}}`}, }) - body := map[string]any{"displayName": "Acme", "legacyFlag": true} + // "website" is deliberately a real, caller-settable brand field (see + // brandFlagToField), not an invented name like the old "legacyFlag" this + // test used to use. Every prior fixture in this file named a field no + // caller could ever actually set, which is exactly why the retry's + // blindness to caller-set fields went uncaught: nothing here exercised + // that shape. This test still passes a nil neverDrop, i.e. it simulates a + // caller who did NOT ask about website this call — some other field was + // changed, and website merely rode along from the read-modify-write and + // happened to be named in the error. Dropping it here is correct; + // TestPutRetry_CallerSetField_NoRetry below is its mirror image, where + // website WAS what the caller asked to set. + body := map[string]any{"displayName": "Acme", "website": "not a url"} var raw []byte var err error stderr := captureStderr(t, func() { - raw, err = putReplaceWithReadOnlyRetry(client, "/thing/1", body) + raw, err = putReplaceWithReadOnlyRetry(client, "/thing/1", body, nil) }) if err != nil { @@ -108,19 +119,104 @@ func TestPutRetry_ReadOnlyFieldWeSent_RetriesOnceAndSucceeds(t *testing.T) { t.Fatalf("request count = %d, want exactly 2", *count) } second := (*bodies)[1] - if _, present := second["legacyFlag"]; present { - t.Errorf("second request body = %v, want legacyFlag stripped", second) + if _, present := second["website"]; present { + t.Errorf("second request body = %v, want website stripped", second) } if second["displayName"] != "Acme" { t.Errorf("second request body = %v, want displayName preserved", second) } - if !strings.Contains(stderr, "legacyFlag") { + if !strings.Contains(stderr, "website") { t.Errorf("stderr = %q, want it to name the dropped field", stderr) } // The retry must not mutate the caller's own body map. - if _, present := body["legacyFlag"]; !present { - t.Error("caller's body map was mutated; legacyFlag should still be present in the original map") + if _, present := body["website"]; !present { + t.Error("caller's body map was mutated; website should still be present in the original map") + } +} + +// TestPutRetry_CallerSetField_NoRetry is the mirror image of +// TestPutRetry_ReadOnlyFieldWeSent_RetriesOnceAndSucceeds: same field name, +// same 400, but this time neverDrop marks "website" as a field the caller +// explicitly asked this call to set (as UpdateBrand would, via +// changedBrandJSONFields). This is the CRITICAL data-loss shape: "band +// tendlc brand update BEXMPL1 --website 'not a url'" must surface the API's +// own rejection of the caller's value, never silently drop --website and +// report success with the brand's site cleared. +func TestPutRetry_CallerSetField_NoRetry(t *testing.T) { + client, _, count := newRetryStub(t, []retryStubResponse{ + {status: 400, body: errorBodyNaming("/website")}, + }) + + body := map[string]any{"displayName": "Acme", "website": "not a url"} + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, map[string]bool{"website": true}) + + if err == nil { + t.Fatal("want an error, got nil") + } + apiErr, ok := err.(*api.APIError) + if !ok || apiErr.StatusCode != 400 { + t.Fatalf("err = %v, want the original 400 *api.APIError", err) + } + if !strings.Contains(apiErr.Body, "website") { + t.Errorf("err body = %q, want it to still name website", apiErr.Body) + } + if *count != 1 { + t.Errorf("request count = %d, want exactly 1 (no retry — the caller set this field)", *count) + } +} + +// TestPutRetry_CampaignCallerSetField_NoRetry is the campaign-path twin of +// TestPutRetry_CallerSetField_NoRetry: "description" is a real, caller- +// settable campaign field (see campaignUpdateFlagToField), and neverDrop +// simulates UpdateCampaign's changedCampaignJSONFields marking it as changed +// this call. +func TestPutRetry_CampaignCallerSetField_NoRetry(t *testing.T) { + client, _, count := newRetryStub(t, []retryStubResponse{ + {status: 400, body: errorBodyNaming("/description")}, + }) + + body := map[string]any{"campaignName": "Acme Alerts", "description": ""} + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, map[string]bool{"description": true}) + + if err == nil { + t.Fatal("want an error, got nil") + } + if *count != 1 { + t.Errorf("request count = %d, want exactly 1 (no retry — the caller set this field)", *count) + } +} + +// TestPutRetry_SubscriberOptin_NoRetry is the measured real-world shape: a +// direct campaign holding false for subscriberOptin returns 400 "is +// required" naming it (see campaignNeverDropFields), even though no update +// flag can ever set it and the caller changed something unrelated +// (description). Before this guard, that 400 named a field present in body +// and was indistinguishable from the retry's intended case, so the retry +// silently stripped the compliance attestation and reported success. +// neverDrop here is exactly what UpdateCampaign builds: changedCampaignJSONFields +// (which cannot include subscriberOptin — no flag ever writes it) unioned +// with campaignNeverDropFields. +func TestPutRetry_SubscriberOptin_NoRetry(t *testing.T) { + client, _, count := newRetryStub(t, []retryStubResponse{ + {status: 400, body: errorBodyNaming("/subscriberOptin")}, + }) + + body := map[string]any{"description": "Updated description", "subscriberOptin": false} + neverDrop := changedCampaignJSONFields(map[string]bool{"description": true}) + for f := range campaignNeverDropFields { + neverDrop[f] = true + } + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, neverDrop) + + if err == nil { + t.Fatal("want an error, got nil") + } + if !strings.Contains(err.Error(), "subscriberOptin") { + t.Errorf("err = %v, want it to still name subscriberOptin", err) + } + if *count != 1 { + t.Errorf("request count = %d, want exactly 1 (no retry — subscriberOptin is never droppable)", *count) } } @@ -130,7 +226,7 @@ func TestPutRetry_FieldNotSent_PassesThroughNoSecondRequest(t *testing.T) { }) body := map[string]any{"displayName": "Acme"} - _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body) + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, nil) if err == nil { t.Fatal("want an error, got nil") @@ -153,7 +249,7 @@ func TestPutRetry_GenericBadRequest_NoUsablePointers_NoRetry(t *testing.T) { }) body := map[string]any{"displayName": ""} - _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body) + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, nil) if err == nil { t.Fatal("want an error, got nil") @@ -170,7 +266,7 @@ func TestPutRetry_RetryAlsoFails_OriginalErrorSurfaces(t *testing.T) { }) body := map[string]any{"displayName": "Acme", "legacyFlag": true} - _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body) + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, nil) if err == nil { t.Fatal("want an error, got nil") @@ -192,7 +288,7 @@ func TestPutRetry_NestedPointer_NoRetry(t *testing.T) { }) body := map[string]any{"accounts": []any{map[string]any{"customerProfileId": "CEXMPL1"}}} - _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body) + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, nil) if err == nil { t.Fatal("want an error, got nil") @@ -208,7 +304,7 @@ func TestPutRetry_409_NoRetry(t *testing.T) { }) body := map[string]any{"legacyFlag": true} - _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body) + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, nil) if err == nil { t.Fatal("want an error, got nil") @@ -230,7 +326,7 @@ func TestPutRetry_HappyPath_NoStderrNote(t *testing.T) { body := map[string]any{"displayName": "Acme"} var err error stderr := captureStderr(t, func() { - _, err = putReplaceWithReadOnlyRetry(client, "/thing/1", body) + _, err = putReplaceWithReadOnlyRetry(client, "/thing/1", body, nil) }) if err != nil { diff --git a/internal/tendlc/write.go b/internal/tendlc/write.go index 44ff77b..f9777fc 100644 --- a/internal/tendlc/write.go +++ b/internal/tendlc/write.go @@ -30,11 +30,16 @@ func (s *Service) CreateBrand(body map[string]any) (*api.Envelope, error) { // The PUT goes through putReplaceWithReadOnlyRetry, which retries once, with // the named fields stripped, if the API rejects a 400 on read-only keys // brandReadOnlyFields already tried to remove — see that function for why. -func (s *Service) UpdateBrand(brandID string, body map[string]any) (*api.Envelope, error) { +// changed is the same caller-set flag map BuildBrandUpdateRequest was built +// from; it is translated to the JSON keys the retry must never drop, so a 400 +// naming a field the caller just asked to set (e.g. --website) surfaces as +// the caller's own validation error instead of being silently stripped and +// re-sent. +func (s *Service) UpdateBrand(brandID string, body map[string]any, changed map[string]bool) (*api.Envelope, error) { if brandID == "" { return nil, fmt.Errorf("brand ID is required") } - raw, err := putReplaceWithReadOnlyRetry(s.client, s.brandPath(brandID), body) + raw, err := putReplaceWithReadOnlyRetry(s.client, s.brandPath(brandID), body, changedBrandJSONFields(changed)) if err != nil { return nil, err } diff --git a/internal/tendlc/write_test.go b/internal/tendlc/write_test.go index b3d0a95..d2171ba 100644 --- a/internal/tendlc/write_test.go +++ b/internal/tendlc/write_test.go @@ -70,7 +70,7 @@ func TestUpdateBrandPutsToBrandPath(t *testing.T) { var got captured s := stubService(t, 202, `{"data":{"bandwidthId":"WABC123"}}`, &got) - if _, err := s.UpdateBrand("BGJR2BA", map[string]any{"displayName": "Acme"}); err != nil { + if _, err := s.UpdateBrand("BGJR2BA", map[string]any{"displayName": "Acme"}, nil); err != nil { t.Fatalf("UpdateBrand: %v", err) } if got.method != "PUT" { @@ -192,7 +192,7 @@ func TestEmptyIDsRejectedWithoutRequest(t *testing.T) { s := stubService(t, 200, `{"data":{}}`, &got) calls := map[string]func() error{ - "UpdateBrand": func() error { _, err := s.UpdateBrand("", map[string]any{}); return err }, + "UpdateBrand": func() error { _, err := s.UpdateBrand("", map[string]any{}, nil); return err }, "DeleteBrand": func() error { return s.DeleteBrand("") }, "ReverifyBrand": func() error { return s.ReverifyBrand("") }, "Resend2FA": func() error { return s.Resend2FA("") }, From 20bfc9024b6c717270ec719114059dc7c1f84d43 Mon Sep 17 00:00:00 2001 From: Kush Date: Mon, 24 Aug 2026 12:13:21 -0400 Subject: [PATCH 14/15] fix(tendlc): the PUT retry may only drop fields the CLI does not model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent adversarial pass found the previous guard insufficient. It protected fields the caller changed in this invocation, but an unchanged field still holds real data: a brand with a stored website that no longer passes validation would have that website dropped from the retry body by an unrelated --display-name update, and a full-replacement PUT nulls it. neverDrop is now built from the entire update flag surface, so the retry can only ever drop a field the CLI does not model at all — which is the only case it was designed for. The invariant is stated in putretry.go. Also: trailing punctuation no longer bypasses the doc-contract parser, so a documented 'band tendlc campaigns,' is caught rather than abstained on; a port-in example shows two distinct numbers again; and a caller-id moves into the reserved range. --- AGENTS.md | 4 +- README.md | 2 +- cmd/doccontract_test.go | 55 +++++++++++-- cmd/tendlc/brand_update.go | 2 +- cmd/tendlc/campaign_update.go | 2 +- internal/tendlc/brandupdate.go | 26 +++--- internal/tendlc/campaignupdate.go | 54 ++++++------- internal/tendlc/campaignwrite.go | 15 ++-- internal/tendlc/campaignwrite_test.go | 4 +- internal/tendlc/putretry.go | 40 ++++++---- internal/tendlc/putretry_test.go | 109 ++++++++++++++------------ internal/tendlc/write.go | 15 ++-- internal/tendlc/write_test.go | 4 +- 13 files changed, 198 insertions(+), 134 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a5e27c8..856bb9b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -666,13 +666,13 @@ band portin validate-tf +18005551234 --wait --plain ```bash band portin create \ - --numbers +15555550100,+15555550100 \ + --numbers +15555550100,+15555550101 \ --site --peer \ --foc 2026-06-01T15:30:00Z \ --loa-authorizing-person "Jane Doe" \ --loa ./loa.pdf \ --customer-order-id agent-run-42 --if-not-exists --plain -# → {"orderId":"...","status":"DRAFT","numbers":["+15555550100","+15555550100"], ...} +# → {"orderId":"...","status":"DRAFT","numbers":["+15555550100","+15555550101"], ...} ORDER_ID=$(... extract from above ...) band portin submit $ORDER_ID --wait --plain diff --git a/README.md b/README.md index f606472..b880ccc 100644 --- a/README.md +++ b/README.md @@ -196,7 +196,7 @@ The CLI can generate BXML for you locally. No API calls, no auth required — it band bxml speak "Thanks for calling. How can we help?" band bxml speak --voice julie "Press 1 for sales." band bxml gather --url https://example.com/gather --max-digits 1 --prompt "Press a key" -band bxml transfer +15555550100 --caller-id +19195550000 +band bxml transfer +15555550100 --caller-id +15555550101 band bxml record --url https://example.com/done --max-duration 60 band bxml raw 'Hello' # validate and pretty-print XML ``` diff --git a/cmd/doccontract_test.go b/cmd/doccontract_test.go index 9b961f1..bbd5b0e 100644 --- a/cmd/doccontract_test.go +++ b/cmd/doccontract_test.go @@ -44,6 +44,18 @@ var backtickBandRe = regexp.MustCompile("`(band [^`]+)`") // so the first non-matching token ends the command path. var commandTokenRe = regexp.MustCompile(`^[a-z][a-z-]*$`) +// trailingPunctRe matches punctuation glued onto the end of a token by +// ordinary prose — a trailing comma, period, semicolon, colon, or +// close-paren, as in "Use `band tendlc campaigns,` then assign the number." +// A real command or subcommand name never itself ends in one of these, so it +// is stripped before a token is judged against commandTokenRe (and before an +// already-resolved remainder is judged for ambiguity in argsGateRejects). +// Without this, a stale command word with a punctuation mark stuck to it — +// exactly the shape a deleted/renamed command reference takes in running +// prose — silently ends the command path one token early instead of being +// evaluated as the word it actually names. +var trailingPunctRe = regexp.MustCompile(`[,.;:)]+$`) + // usePlaceholderRe matches a "<...>" or "[...]" placeholder in a cobra // Use string, e.g. "get " or "release [number]". This codebase // consistently declares positional args this way (verified against every @@ -248,10 +260,17 @@ func splitPositionalArgs(cmd *cobra.Command, fields []string) (args []string, ok // - there's no remainder at all — a bare mention like "`band portin get`", // naming a command without demonstrating a full invocation, is normal // technical writing and must not be required to show every argument; -// - a positional token is a literal "..." or contains a "," — both strong -// signals of deliberately elided/abbreviated example text (e.g. `band -// tendlc campaign create --plain`) -// rather than a literal, runnable argument list; +// - a positional token is a literal "..." — a strong signal of +// deliberately elided example text; +// - a positional token still contains a "," after trailing punctuation is +// stripped (trailingPunctRe) — an INTERNAL comma is a strong signal of +// deliberately elided/abbreviated example text (e.g. `band tendlc +// campaign create --plain`, though +// that particular case is already one merged token via the bracket rule +// below). A comma stuck only to the END of an otherwise-plain word — e.g. +// a stale `campaigns,` in running prose — is NOT ambiguous in this way +// and is evaluated normally, not abstained on: that was exactly the gap +// that let a deleted command slip through undetected; // - a positional token is a lone "\" — a shell line-continuation marker // from a multi-line example, meaning the real argument is on the next // line and this test only ever looks at one line at a time. @@ -282,7 +301,10 @@ func argsGateRejects(s string) error { return nil } for _, a := range pos { - if a == "..." || a == `\` || strings.Contains(a, ",") { + if a == "..." || a == `\` { + return nil + } + if strings.Contains(trailingPunctRe.ReplaceAllString(a, ""), ",") { return nil } // A "<...>"/"[...]" placeholder that itself spans multiple words @@ -311,9 +333,19 @@ func argsGateRejects(s string) error { // leading run of fields that look like command tokens (per commandTokenRe). // The first field that isn't command-shaped (a flag, placeholder, ID, phone // number, etc.) ends the path. +// +// Each field has trailing prose punctuation (trailingPunctRe) stripped +// before the commandTokenRe check, and the STRIPPED form is what goes into +// path — a deleted/renamed command mentioned as "`band tendlc campaigns,` +// then ..." must be evaluated as "campaigns", not silently end the path one +// token early at "tendlc" just because a comma is stuck to the next word. +// This does not risk misclassifying a flag, placeholder, ID, or phone number +// as command-shaped: all of those fail commandTokenRe on their FIRST +// character, which stripping a trailing character never changes. func commandPathTokens(s string) []string { var path []string for _, f := range strings.Fields(s) { + f = trailingPunctRe.ReplaceAllString(f, "") if !commandTokenRe.MatchString(f) { break } @@ -351,6 +383,19 @@ func TestParserDistinguishesSubcommandsFromPositionals(t *testing.T) { commandLine: "tendlc campaigns list", wantFlagged: true, }, + { + // The planted punctuation-bypass shape: "Use `band tendlc + // campaigns,` then assign the number." A comma glued directly onto + // the deleted command word, with no trailing subcommand-shaped + // word at all, used to make commandPathTokens stop at "tendlc" + // (the comma fails commandTokenRe) and made argsGateRejects + // abstain unconditionally on any remainder containing a comma — + // so this slipped through undetected before trailingPunctRe + // stripping was added to both. + name: "deleted `tendlc campaigns,` with a comma glued directly onto the stale word", + commandLine: "tendlc campaigns,", + wantFlagged: true, + }, { name: "deleted `tendlc numbers`", commandLine: "tendlc numbers", diff --git a/cmd/tendlc/brand_update.go b/cmd/tendlc/brand_update.go index 5b9f7cb..9995828 100644 --- a/cmd/tendlc/brand_update.go +++ b/cmd/tendlc/brand_update.go @@ -132,7 +132,7 @@ PUBLIC_PROFIT brand revokes Auth+ compliance.`, return err } - updated, err := svc.UpdateBrand(args[0], body, changed) + updated, err := svc.UpdateBrand(args[0], body) if err != nil { return brandUpdateConflictHint(args[0], err) } diff --git a/cmd/tendlc/campaign_update.go b/cmd/tendlc/campaign_update.go index 640715e..54223fb 100644 --- a/cmd/tendlc/campaign_update.go +++ b/cmd/tendlc/campaign_update.go @@ -122,7 +122,7 @@ campaigns, so there is no API-justified reason to gate this behind a flag.`, return err } - updated, err := svc.UpdateCampaign(args[0], body, changed) + updated, err := svc.UpdateCampaign(args[0], body) if err != nil { return campaignUpdateConflictHint(args[0], err) } diff --git a/internal/tendlc/brandupdate.go b/internal/tendlc/brandupdate.go index 94137e0..eea3fc7 100644 --- a/internal/tendlc/brandupdate.go +++ b/internal/tendlc/brandupdate.go @@ -327,21 +327,17 @@ func IdentityFieldsChanged(current map[string]any, changed map[string]bool) []st return out } -// changedBrandJSONFields translates changed — keyed by CLI flag name, the -// same map BuildBrandUpdateRequest takes — into the JSON body keys those -// flags write, via brandFlagToField. UpdateBrand passes the result to -// putReplaceWithReadOnlyRetry as the set of fields the retry must never drop: -// a field the caller just told this call to set is never eligible to be -// silently stripped and re-sent, no matter what an error names. -func changedBrandJSONFields(changed map[string]bool) map[string]bool { - out := make(map[string]bool, len(changed)) - for flag, isChanged := range changed { - if !isChanged { - continue - } - if field, ok := brandFlagToField[flag]; ok { - out[field] = true - } +// brandNeverDropFields is every JSON body key any `brand update` flag can +// write — i.e. every value in brandFlagToField. UpdateBrand passes the +// result to putReplaceWithReadOnlyRetry as the set of fields the retry must +// never drop: a field the CLI models at all is mutable customer data (see +// putReplaceWithReadOnlyRetry's INVARIANT and the --website example there), +// so it is never eligible to be silently stripped and re-sent — regardless +// of whether the caller's most recent invocation happened to touch it. +func brandNeverDropFields() map[string]bool { + out := make(map[string]bool, len(brandFlagToField)) + for _, field := range brandFlagToField { + out[field] = true } return out } diff --git a/internal/tendlc/campaignupdate.go b/internal/tendlc/campaignupdate.go index 350dc0a..7efa704 100644 --- a/internal/tendlc/campaignupdate.go +++ b/internal/tendlc/campaignupdate.go @@ -328,21 +328,18 @@ func ImportedCampaignRejectedFlags(changed map[string]bool) []string { return out } -// changedCampaignJSONFields translates changed — keyed by CLI flag name, the -// same map BuildCampaignUpdateRequest takes — into the JSON body keys those -// flags write, via campaignUpdateFlagToField. UpdateCampaign passes the -// result to putReplaceWithReadOnlyRetry (unioned with -// campaignNeverDropFields) as part of the set of fields the retry must never -// drop. -func changedCampaignJSONFields(changed map[string]bool) map[string]bool { - out := make(map[string]bool, len(changed)) - for flag, isChanged := range changed { - if !isChanged { - continue - } - if field, ok := campaignUpdateFlagToField[flag]; ok { - out[field] = true - } +// campaignFlagReachableFields is every JSON body key any `campaign update` +// flag can write — i.e. every value in campaignUpdateFlagToField. UpdateCampaign +// unions the result with campaignNeverDropFields and passes the whole set to +// putReplaceWithReadOnlyRetry as the fields the retry must never drop: a +// field the CLI models at all is mutable customer data (see +// putReplaceWithReadOnlyRetry's INVARIANT), so it is never eligible to be +// silently stripped and re-sent — regardless of whether the caller's most +// recent invocation happened to touch it. +func campaignFlagReachableFields() map[string]bool { + out := make(map[string]bool, len(campaignUpdateFlagToField)) + for _, field := range campaignUpdateFlagToField { + out[field] = true } return out } @@ -355,19 +352,20 @@ func changedCampaignJSONFields(changed map[string]bool) map[string]bool { // campaignReadOnlyFields, which are stripped before every PUT because they // are genuinely server-owned). // -// changedCampaignJSONFields alone cannot protect these: because no update -// flag ever reaches them, they can never appear in the changed map, so a -// guard keyed only on "did the caller ask about this field" would leave them -// exactly as droppable as before. But they are not read-only filler either — -// measured against production, a direct campaign holding false for these -// returns 400 "is required" naming exactly these pointers (the same shape -// ValidateCampaignCreate documents for create; see campaignoptions.go). That -// reads exactly like the shape this retry exists to handle — a 400 naming a -// field present in body — so without this explicit list, the retry would -// strip all three compliance attestations off an update that has nothing to -// do with them (e.g. a plain --description change) and report success. This -// list is therefore excluded from the retry's drop set unconditionally, not -// merely when unchanged this call. +// campaignFlagReachableFields alone cannot protect these: because no update +// flag ever writes them, they never appear in campaignUpdateFlagToField's +// values, so a guard keyed only on "is this field reachable by some flag" +// would leave them exactly as droppable as an unmodeled field. But they are +// not read-only filler either — measured against production, a direct +// campaign holding false for these returns 400 "is required" naming exactly +// these pointers (the same shape ValidateCampaignCreate documents for +// create; see campaignoptions.go). That reads exactly like the shape this +// retry exists to handle — a 400 naming a field present in body — so +// without this explicit list, the retry would strip all three compliance +// attestations off an update that has nothing to do with them (e.g. a plain +// --description change) and report success. This list is therefore unioned +// into the retry's neverDrop set unconditionally by UpdateCampaign, not +// merely when reachable by a flag. var campaignNeverDropFields = map[string]bool{ "termsAndConditions": true, "subscriberOptin": true, diff --git a/internal/tendlc/campaignwrite.go b/internal/tendlc/campaignwrite.go index c1701c0..44d0b68 100644 --- a/internal/tendlc/campaignwrite.go +++ b/internal/tendlc/campaignwrite.go @@ -31,16 +31,17 @@ func (s *Service) CreateCampaign(body map[string]any) (*api.Envelope, error) { // The PUT goes through putReplaceWithReadOnlyRetry, which retries once, with // the named fields stripped, if the API rejects a 400 on read-only keys // campaignReadOnlyFields already tried to remove — see that function for why. -// changed is the same caller-set flag map BuildCampaignUpdateRequest was -// built from; it is translated to the JSON keys the retry must never drop, -// and unioned with campaignNeverDropFields (fields no update flag can ever -// reach but that still hold real data), so neither category is ever -// silently stripped and re-sent. -func (s *Service) UpdateCampaign(campaignID string, body map[string]any, changed map[string]bool) (*api.Envelope, error) { +// The retry's neverDrop set is campaignFlagReachableFields() (every JSON key +// any `campaign update` flag can write, not merely the ones changed THIS +// call) unioned with campaignNeverDropFields (fields no update flag can ever +// reach but that still hold real data), so neither category is ever silently +// stripped and re-sent. See putReplaceWithReadOnlyRetry's INVARIANT for why +// "changed this call" is not the right test. +func (s *Service) UpdateCampaign(campaignID string, body map[string]any) (*api.Envelope, error) { if campaignID == "" { return nil, fmt.Errorf("campaign ID is required") } - neverDrop := changedCampaignJSONFields(changed) + neverDrop := campaignFlagReachableFields() for f := range campaignNeverDropFields { neverDrop[f] = true } diff --git a/internal/tendlc/campaignwrite_test.go b/internal/tendlc/campaignwrite_test.go index 1571a23..239138c 100644 --- a/internal/tendlc/campaignwrite_test.go +++ b/internal/tendlc/campaignwrite_test.go @@ -50,7 +50,7 @@ func TestUpdateCampaignPutsToCampaignPath(t *testing.T) { var got captured s := stubService(t, 202, `{"data":{"bandwidthId":"CEXMPL1"}}`, &got) - if _, err := s.UpdateCampaign("CEXMPL1", map[string]any{"campaignName": "Acme Alerts"}, nil); err != nil { + if _, err := s.UpdateCampaign("CEXMPL1", map[string]any{"campaignName": "Acme Alerts"}); err != nil { t.Fatalf("UpdateCampaign: %v", err) } if got.method != "PUT" { @@ -138,7 +138,7 @@ func TestEmptyCampaignIDsRejectedWithoutRequest(t *testing.T) { s := stubService(t, 200, `{"data":{}}`, &got) calls := map[string]func() error{ - "UpdateCampaign": func() error { _, err := s.UpdateCampaign("", map[string]any{}, nil); return err }, + "UpdateCampaign": func() error { _, err := s.UpdateCampaign("", map[string]any{}); return err }, "DeactivateCampaign": func() error { return s.DeactivateCampaign("") }, "NudgeCampaign": func() error { return s.NudgeCampaign("", map[string]any{}) }, "CampaignPhoneNumbers": func() error { diff --git a/internal/tendlc/putretry.go b/internal/tendlc/putretry.go index 62690e0..290f1f8 100644 --- a/internal/tendlc/putretry.go +++ b/internal/tendlc/putretry.go @@ -31,21 +31,31 @@ import ( // read-only field", and looping or guessing there would turn a clear error // into a confusing double-request. // -// INVARIANT: the retry may only ever drop a field the caller did not ask -// about. That is the entire case the design contemplates — an API-side -// change to how some field the CLI merely echoes back is handled, never a -// value the caller explicitly asked this call to set. neverDrop is how that -// invariant is enforced: it is the set of JSON body keys this call must never -// remove, regardless of what the error names. UpdateBrand and UpdateCampaign -// each build it from two things: the JSON keys backing whatever flags the -// caller actually passed THIS call (so "brand update --website bad-url" can -// never have website silently dropped and re-sent), and, for campaigns, a -// fixed set of fields that are never reachable by any update flag at all but -// are still real data, not read-only filler (see campaignNeverDropFields). -// Without the second category, a field the CLI never lets the caller touch -// would look, from here, indistinguishable from a genuinely-inert read-only -// field — which is exactly the shape production returns for a direct -// campaign's subscriberOptin/subscriberOptout/subscriberHelp attestations. +// INVARIANT: the retry may only ever drop a field the CLI does not model at +// all. Anything reachable by an update flag is mutable customer data whether +// or not the caller happened to touch it THIS invocation — an earlier version +// of this invariant was "don't drop what the caller changed this call", and +// that is not sufficient: an unchanged field still holds real, previously-set +// customer data. Concretely: a brand has website "https://example.com", set +// months ago. The caller runs "brand update --display-name 'New Name'", +// touching nothing else. The PUT 400s naming "/website" because the stored +// value no longer passes current validation. website is not read-only, and +// the caller didn't touch it this call — but it is still reachable by +// --website, so it is still real data, and dropping it would silently null +// the brand's site on a request that never mentioned it. The only field that +// is genuinely safe to drop is one the CLI never models at all — the entire +// case this retry was designed for. neverDrop is how the invariant is +// enforced: it is the set of JSON body keys this call must never remove, +// regardless of what the error names. UpdateBrand and UpdateCampaign each +// build it from the WHOLE update flag surface (every JSON key any update flag +// can write, not merely the ones changed this call) and, for campaigns, union +// it with a fixed set of fields that are never reachable by any update flag +// at all but are still real data, not read-only filler (see +// campaignNeverDropFields). Without that second category, a field the CLI +// never lets the caller touch would look, from here, indistinguishable from a +// genuinely-inert read-only field — which is exactly the shape production +// returns for a direct campaign's +// subscriberOptin/subscriberOptout/subscriberHelp attestations. // // On success after a retry, a note naming the dropped fields goes to stderr: // a silent self-heal would hide an API change worth knowing about. On a diff --git a/internal/tendlc/putretry_test.go b/internal/tendlc/putretry_test.go index a888a19..e47df30 100644 --- a/internal/tendlc/putretry_test.go +++ b/internal/tendlc/putretry_test.go @@ -85,28 +85,26 @@ func errorBodyNaming(pointers ...string) string { return `{"errors":[` + strings.Join(errs, ",") + `],"links":[]}` } -func TestPutRetry_ReadOnlyFieldWeSent_RetriesOnceAndSucceeds(t *testing.T) { +// TestPutRetry_UnmodeledField_RetriesOnceAndSucceeds proves the retry still +// fires for the one shape it exists for: a field the CLI does not model at +// all. "someFutureField" appears in neither brandFlagToField nor +// brandReadOnlyFields (see liveBrand's fixture in brandupdate_test.go) — a +// fix that disabled the retry entirely to close the CRITICAL data-loss bug +// (see the tests below) would also be wrong, and this is the regression +// guard for that. neverDrop here is brandNeverDropFields(), the real set +// UpdateBrand builds, so this test exercises the actual production +// neverDrop shape rather than an ad hoc one. +func TestPutRetry_UnmodeledField_RetriesOnceAndSucceeds(t *testing.T) { client, bodies, count := newRetryStub(t, []retryStubResponse{ - {status: 400, body: errorBodyNaming("/website")}, + {status: 400, body: errorBodyNaming("/someFutureField")}, {status: 202, body: `{"data":{"bandwidthId":"BEXMPL1"}}`}, }) - // "website" is deliberately a real, caller-settable brand field (see - // brandFlagToField), not an invented name like the old "legacyFlag" this - // test used to use. Every prior fixture in this file named a field no - // caller could ever actually set, which is exactly why the retry's - // blindness to caller-set fields went uncaught: nothing here exercised - // that shape. This test still passes a nil neverDrop, i.e. it simulates a - // caller who did NOT ask about website this call — some other field was - // changed, and website merely rode along from the read-modify-write and - // happened to be named in the error. Dropping it here is correct; - // TestPutRetry_CallerSetField_NoRetry below is its mirror image, where - // website WAS what the caller asked to set. - body := map[string]any{"displayName": "Acme", "website": "not a url"} + body := map[string]any{"displayName": "Acme", "someFutureField": "keep me usually, drop me here"} var raw []byte var err error stderr := captureStderr(t, func() { - raw, err = putReplaceWithReadOnlyRetry(client, "/thing/1", body, nil) + raw, err = putReplaceWithReadOnlyRetry(client, "/thing/1", body, brandNeverDropFields()) }) if err != nil { @@ -119,37 +117,45 @@ func TestPutRetry_ReadOnlyFieldWeSent_RetriesOnceAndSucceeds(t *testing.T) { t.Fatalf("request count = %d, want exactly 2", *count) } second := (*bodies)[1] - if _, present := second["website"]; present { - t.Errorf("second request body = %v, want website stripped", second) + if _, present := second["someFutureField"]; present { + t.Errorf("second request body = %v, want someFutureField stripped", second) } if second["displayName"] != "Acme" { t.Errorf("second request body = %v, want displayName preserved", second) } - if !strings.Contains(stderr, "website") { + if !strings.Contains(stderr, "someFutureField") { t.Errorf("stderr = %q, want it to name the dropped field", stderr) } // The retry must not mutate the caller's own body map. - if _, present := body["website"]; !present { - t.Error("caller's body map was mutated; website should still be present in the original map") + if _, present := body["someFutureField"]; !present { + t.Error("caller's body map was mutated; someFutureField should still be present in the original map") } } -// TestPutRetry_CallerSetField_NoRetry is the mirror image of -// TestPutRetry_ReadOnlyFieldWeSent_RetriesOnceAndSucceeds: same field name, -// same 400, but this time neverDrop marks "website" as a field the caller -// explicitly asked this call to set (as UpdateBrand would, via -// changedBrandJSONFields). This is the CRITICAL data-loss shape: "band -// tendlc brand update BEXMPL1 --website 'not a url'" must surface the API's -// own rejection of the caller's value, never silently drop --website and -// report success with the brand's site cleared. -func TestPutRetry_CallerSetField_NoRetry(t *testing.T) { +// TestPutRetry_BrandUnchangedFlagReachableField_NoRetry is the CRITICAL +// regression guard for the data-loss bug a prior round of this fix left in +// place: neverDrop used to be built ONLY from fields the caller changed THIS +// call (changedBrandJSONFields), so an unchanged-but-real field like website +// looked exactly like an unmodeled one the moment some OTHER flag was +// changed. Concretely: "band tendlc brand update BEXMPL1 --display-name +// 'New Name'" never mentions --website, but the read-modify-write body still +// carries the brand's real (months-old) website value, and the API can 400 +// on it anyway if stored data no longer passes current validation. Under the +// old invariant that 400 would have triggered a silent drop-and-retry, +// nulling the site and reporting success. neverDrop here is +// brandNeverDropFields() — the actual, whole-flag-surface set UpdateBrand +// builds — with no "changed" input at all, proving the fix does not depend +// on tracking what this call touched. +func TestPutRetry_BrandUnchangedFlagReachableField_NoRetry(t *testing.T) { client, _, count := newRetryStub(t, []retryStubResponse{ {status: 400, body: errorBodyNaming("/website")}, }) - body := map[string]any{"displayName": "Acme", "website": "not a url"} - _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, map[string]bool{"website": true}) + // displayName is what the caller actually changed; website merely rode + // along from the read-modify-write, untouched this call. + body := map[string]any{"displayName": "New Name", "website": "https://example.com"} + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, brandNeverDropFields()) if err == nil { t.Fatal("want an error, got nil") @@ -162,39 +168,46 @@ func TestPutRetry_CallerSetField_NoRetry(t *testing.T) { t.Errorf("err body = %q, want it to still name website", apiErr.Body) } if *count != 1 { - t.Errorf("request count = %d, want exactly 1 (no retry — the caller set this field)", *count) + t.Errorf("request count = %d, want exactly 1 (no retry — website is reachable by --website, changed or not)", *count) } } -// TestPutRetry_CampaignCallerSetField_NoRetry is the campaign-path twin of -// TestPutRetry_CallerSetField_NoRetry: "description" is a real, caller- -// settable campaign field (see campaignUpdateFlagToField), and neverDrop -// simulates UpdateCampaign's changedCampaignJSONFields marking it as changed -// this call. -func TestPutRetry_CampaignCallerSetField_NoRetry(t *testing.T) { +// TestPutRetry_CampaignUnchangedFlagReachableField_NoRetry is the campaign +// twin of TestPutRetry_BrandUnchangedFlagReachableField_NoRetry: "sample2" is +// a real, caller-settable campaign field (see campaignUpdateFlagToField), +// unchanged this call (only description was), and neverDrop is +// campaignFlagReachableFields() unioned with campaignNeverDropFields — the +// real set UpdateCampaign builds, again with no "changed" input. +func TestPutRetry_CampaignUnchangedFlagReachableField_NoRetry(t *testing.T) { client, _, count := newRetryStub(t, []retryStubResponse{ - {status: 400, body: errorBodyNaming("/description")}, + {status: 400, body: errorBodyNaming("/sample2")}, }) - body := map[string]any{"campaignName": "Acme Alerts", "description": ""} - _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, map[string]bool{"description": true}) + neverDrop := campaignFlagReachableFields() + for f := range campaignNeverDropFields { + neverDrop[f] = true + } + body := map[string]any{"description": "Updated description", "sample2": "Reply STOP to opt out"} + _, err := putReplaceWithReadOnlyRetry(client, "/thing/1", body, neverDrop) if err == nil { t.Fatal("want an error, got nil") } + if !strings.Contains(err.Error(), "sample2") { + t.Errorf("err = %v, want it to still name sample2", err) + } if *count != 1 { - t.Errorf("request count = %d, want exactly 1 (no retry — the caller set this field)", *count) + t.Errorf("request count = %d, want exactly 1 (no retry — sample2 is reachable by --sample2, changed or not)", *count) } } // TestPutRetry_SubscriberOptin_NoRetry is the measured real-world shape: a // direct campaign holding false for subscriberOptin returns 400 "is // required" naming it (see campaignNeverDropFields), even though no update -// flag can ever set it and the caller changed something unrelated -// (description). Before this guard, that 400 named a field present in body -// and was indistinguishable from the retry's intended case, so the retry -// silently stripped the compliance attestation and reported success. -// neverDrop here is exactly what UpdateCampaign builds: changedCampaignJSONFields +// flag can ever set it. Before this guard, that 400 named a field present in +// body and was indistinguishable from the retry's intended case, so the +// retry silently stripped the compliance attestation and reported success. +// neverDrop here is exactly what UpdateCampaign builds: campaignFlagReachableFields // (which cannot include subscriberOptin — no flag ever writes it) unioned // with campaignNeverDropFields. func TestPutRetry_SubscriberOptin_NoRetry(t *testing.T) { @@ -203,7 +216,7 @@ func TestPutRetry_SubscriberOptin_NoRetry(t *testing.T) { }) body := map[string]any{"description": "Updated description", "subscriberOptin": false} - neverDrop := changedCampaignJSONFields(map[string]bool{"description": true}) + neverDrop := campaignFlagReachableFields() for f := range campaignNeverDropFields { neverDrop[f] = true } diff --git a/internal/tendlc/write.go b/internal/tendlc/write.go index f9777fc..6827ed6 100644 --- a/internal/tendlc/write.go +++ b/internal/tendlc/write.go @@ -30,16 +30,17 @@ func (s *Service) CreateBrand(body map[string]any) (*api.Envelope, error) { // The PUT goes through putReplaceWithReadOnlyRetry, which retries once, with // the named fields stripped, if the API rejects a 400 on read-only keys // brandReadOnlyFields already tried to remove — see that function for why. -// changed is the same caller-set flag map BuildBrandUpdateRequest was built -// from; it is translated to the JSON keys the retry must never drop, so a 400 -// naming a field the caller just asked to set (e.g. --website) surfaces as -// the caller's own validation error instead of being silently stripped and -// re-sent. -func (s *Service) UpdateBrand(brandID string, body map[string]any, changed map[string]bool) (*api.Envelope, error) { +// The retry's neverDrop set is brandNeverDropFields(): every JSON key any +// `brand update` flag can write, not merely the ones changed THIS call — a +// 400 naming a field reachable by any update flag (e.g. website, even when +// this call never touched it) surfaces as a real validation failure instead +// of being silently stripped and re-sent. See putReplaceWithReadOnlyRetry's +// INVARIANT for why "changed this call" is not the right test. +func (s *Service) UpdateBrand(brandID string, body map[string]any) (*api.Envelope, error) { if brandID == "" { return nil, fmt.Errorf("brand ID is required") } - raw, err := putReplaceWithReadOnlyRetry(s.client, s.brandPath(brandID), body, changedBrandJSONFields(changed)) + raw, err := putReplaceWithReadOnlyRetry(s.client, s.brandPath(brandID), body, brandNeverDropFields()) if err != nil { return nil, err } diff --git a/internal/tendlc/write_test.go b/internal/tendlc/write_test.go index d2171ba..b3d0a95 100644 --- a/internal/tendlc/write_test.go +++ b/internal/tendlc/write_test.go @@ -70,7 +70,7 @@ func TestUpdateBrandPutsToBrandPath(t *testing.T) { var got captured s := stubService(t, 202, `{"data":{"bandwidthId":"WABC123"}}`, &got) - if _, err := s.UpdateBrand("BGJR2BA", map[string]any{"displayName": "Acme"}, nil); err != nil { + if _, err := s.UpdateBrand("BGJR2BA", map[string]any{"displayName": "Acme"}); err != nil { t.Fatalf("UpdateBrand: %v", err) } if got.method != "PUT" { @@ -192,7 +192,7 @@ func TestEmptyIDsRejectedWithoutRequest(t *testing.T) { s := stubService(t, 200, `{"data":{}}`, &got) calls := map[string]func() error{ - "UpdateBrand": func() error { _, err := s.UpdateBrand("", map[string]any{}, nil); return err }, + "UpdateBrand": func() error { _, err := s.UpdateBrand("", map[string]any{}); return err }, "DeleteBrand": func() error { return s.DeleteBrand("") }, "ReverifyBrand": func() error { return s.ReverifyBrand("") }, "Resend2FA": func() error { return s.Resend2FA("") }, From 19f463b0388a95e2527ea5d4bb1f1bcb30f3678a Mon Sep 17 00:00:00 2001 From: Kush Date: Mon, 24 Aug 2026 13:36:37 -0400 Subject: [PATCH 15/15] test(cmd): normalize CRLF before parsing docs in the doc-contract gate The doc-contract parser is line-oriented and splits on "\n". On a Windows checkout the files land with CRLF, so the trailing "\r" survives the split and glues itself to the last token on every line. That broke the gate two ways. A command token became "get\r", which fails commandTokenRe, so the path resolved one token short and the test reported `band tendlc campaign` rejecting "get". And the lone "\" shell line-continuation marker became "\\\r", which no longer matched the documented abstain rule for it, so multi-line examples were parsed as if the continuation backslash were a real positional argument. Four AGENTS.md examples failed this way on windows-latest only. Normalize once at the read site rather than defending against "\r" at each token check downstream. Verified: passes under LF, passes under simulated CRLF, and still catches a planted `band tendlc brandz list` under CRLF -- the normalization does not neuter the gate. --- cmd/doccontract_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cmd/doccontract_test.go b/cmd/doccontract_test.go index bbd5b0e..49e0466 100644 --- a/cmd/doccontract_test.go +++ b/cmd/doccontract_test.go @@ -471,7 +471,14 @@ func TestDocumentedCommandsAndFlagsExist(t *testing.T) { if err != nil { t.Fatalf("reading %s: %v", doc, err) } - for _, line := range strings.Split(string(raw), "\n") { + // A Windows checkout lands these files with CRLF. This parser is + // line-oriented, so a surviving "\r" glues itself to the last token on + // every line -- turning "get" into "get\r" (fails commandTokenRe) and + // the lone "\" continuation marker into "\\\r", which defeats the + // abstain rule for it. Normalize once, here, rather than defending + // against "\r" at each of the token checks downstream. + text := strings.ReplaceAll(string(raw), "\r\n", "\n") + for _, line := range strings.Split(text, "\n") { // Shell comments inside fenced blocks are prose, not runnable // commands. They routinely mention a command mid-sentence — e.g. // "# On Build accounts, band number list is not available." — and