diff --git a/AGENTS.md b/AGENTS.md index 5107553..856bb9b 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) ``` @@ -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", ...} @@ -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,+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":["+19195551234","+19195551235"], ...} +# → {"orderId":"...","status":"DRAFT","numbers":["+15555550100","+15555550101"], ...} 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." @@ -1333,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 @@ -1753,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 | @@ -1848,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. @@ -1873,7 +2001,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 +2050,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 +15555550101 --campaign-id CA3XKE1 --wait ``` ### Check order status @@ -1944,14 +2072,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 +2088,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 +2117,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) @@ -2110,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 28c610a..b880ccc 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 +15555550101 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. @@ -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 77d15c0..49e0466 100644 --- a/cmd/doccontract_test.go +++ b/cmd/doccontract_test.go @@ -1,6 +1,7 @@ package cmd import ( + "fmt" "os" "regexp" "strings" @@ -18,9 +19,11 @@ var knownDrift = map[string]bool{ "location create::subaccount": true, } -// knownDriftCommands lists command paths documented in tables that are known -// rename-related drift and intentionally not yet reconciled. -// REMOVE entries here as the underlying drift is fixed. +// knownDriftCommands lists command paths that are known drift and +// 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 @@ -41,6 +44,25 @@ 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 +// `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 @@ -58,6 +80,280 @@ 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) +} + +// 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 "..." — 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. +// +// 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 == `\` { + return nil + } + if strings.Contains(trailingPunctRe.ReplaceAllString(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 +// 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 + } + path = append(path, f) + } + return path +} + func flagExists(c *cobra.Command, name string) bool { if c.Flags().Lookup(name) != nil { return true @@ -68,13 +364,131 @@ func flagExists(c *cobra.Command, name string) bool { return rootCmd.PersistentFlags().Lookup(name) != nil } +// 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 { + 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, + }, + { + // 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", + wantFlagged: true, + }, + { + name: "wholly bogus top-level command", + 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", + 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: 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, argsGateRejects=%v)", + tt.commandLine, flagged, tt.wantFlagged, matched, path, cmd.CommandPath(), argsErr) + } + }) + } +} + func TestDocumentedCommandsAndFlagsExist(t *testing.T) { for _, doc := range []string{"../README.md", "../AGENTS.md"} { raw, err := os.ReadFile(doc) 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 + // 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 @@ -96,14 +510,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 } @@ -111,9 +518,18 @@ 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 + } + if err := argsGateRejects(rest); err != nil { + t.Errorf("%s documents `band %s …` but %v", doc, cmdName, err) } continue } @@ -133,23 +549,28 @@ 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 + } + 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. 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.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/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 new file mode 100644 index 0000000..f743da3 --- /dev/null +++ b/cmd/tendlc/number.go @@ -0,0 +1,227 @@ +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 + numberListCampaignIDContains string + + 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)") + 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") + hf.IntVar(&numberHistoryOffset, "offset", 0, "Pagination offset") + hf.BoolVar(&numberHistoryAll, "all", false, "Fetch every page (cannot be combined with --offset)") + + 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'.`, + 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{ + Use: "list", + Short: "List 10DLC registered phone numbers", + Long: `Lists the phone numbers registered for 10DLC traffic on the account. + +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'. + +--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. + +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 --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. + 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) + + 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, filters) + 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, filters) + }, 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, 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", + 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..bf46a4d --- /dev/null +++ b/cmd/tendlc/number_test.go @@ -0,0 +1,401 @@ +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}}`)) + }) +} + +// 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 { + 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}}`)) + }) +} + +// 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) + } + + // 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 +// 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) + } + } +} + +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) + } +} + +// TestNumberCommandTreeHasNoLegacyFlatGet guards Task 3's removal of the +// legacy flat `band tendlc number ` command: numberCmd (Use: "number") +// 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 +// 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 != 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"}) + 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") + } +} 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/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.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 f52d779..00c96fa 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) { @@ -87,141 +91,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" { @@ -305,6 +174,87 @@ 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. +// +// {"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) { + _, _, 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() { diff --git a/internal/tendlc/brandupdate.go b/internal/tendlc/brandupdate.go index 22ce81e..eea3fc7 100644 --- a/internal/tendlc/brandupdate.go +++ b/internal/tendlc/brandupdate.go @@ -327,6 +327,21 @@ func IdentityFieldsChanged(current map[string]any, changed map[string]bool) []st return out } +// 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 +} + // 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..7efa704 100644 --- a/internal/tendlc/campaignupdate.go +++ b/internal/tendlc/campaignupdate.go @@ -328,6 +328,51 @@ func ImportedCampaignRejectedFlags(changed map[string]bool) []string { return out } +// 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 +} + +// 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). +// +// 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, + "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 9c6e4ea..44d0b68 100644 --- a/internal/tendlc/campaignwrite.go +++ b/internal/tendlc/campaignwrite.go @@ -27,11 +27,25 @@ 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. +// 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") } - raw, err := s.client.PutRawJSON(s.campaignPath(campaignID), body) + neverDrop := campaignFlagReachableFields() + 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/numbers.go b/internal/tendlc/numbers.go new file mode 100644 index 0000000..26c020f --- /dev/null +++ b/internal/tendlc/numbers.go @@ -0,0 +1,73 @@ +package tendlc + +import ( + "fmt" + "net/url" + + "github.com/Bandwidth/cli/internal/api" +) + +// ListPhoneNumbers returns the phone numbers on the account. +// +// 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)) +} + +// 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. `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") + } + 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) + } +} diff --git a/internal/tendlc/putretry.go b/internal/tendlc/putretry.go new file mode 100644 index 0000000..290f1f8 --- /dev/null +++ b/internal/tendlc/putretry.go @@ -0,0 +1,167 @@ +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. +// +// 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 +// 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, neverDrop map[string]bool) ([]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 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 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 + } + + 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..e47df30 --- /dev/null +++ b/internal/tendlc/putretry_test.go @@ -0,0 +1,381 @@ +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":[]}` +} + +// 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("/someFutureField")}, + {status: 202, body: `{"data":{"bandwidthId":"BEXMPL1"}}`}, + }) + + 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, brandNeverDropFields()) + }) + + 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["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, "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["someFutureField"]; !present { + t.Error("caller's body map was mutated; someFutureField should still be present in the original map") + } +} + +// 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")}, + }) + + // 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") + } + 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 — website is reachable by --website, changed or not)", *count) + } +} + +// 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("/sample2")}, + }) + + 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 — 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. 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) { + client, _, count := newRetryStub(t, []retryStubResponse{ + {status: 400, body: errorBodyNaming("/subscriberOptin")}, + }) + + body := map[string]any{"description": "Updated description", "subscriberOptin": false} + neverDrop := campaignFlagReachableFields() + 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) + } +} + +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, nil) + + 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, nil) + + 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, nil) + + 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, nil) + + 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, nil) + + 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, nil) + }) + + 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..6827ed6 100644 --- a/internal/tendlc/write.go +++ b/internal/tendlc/write.go @@ -26,11 +26,21 @@ 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. +// 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 := s.client.PutRawJSON(s.brandPath(brandID), body) + raw, err := putReplaceWithReadOnlyRetry(s.client, s.brandPath(brandID), body, brandNeverDropFields()) if err != nil { return nil, err }