Skip to content

feat(tendlc): band tendlc brand + vetting end to end (PR 3 of 5) - #33

Open
kshahbw wants to merge 20 commits into
feat/customer-profilesfrom
feat/tendlc-brands
Open

feat(tendlc): band tendlc brand + vetting end to end (PR 3 of 5)#33
kshahbw wants to merge 20 commits into
feat/customer-profilesfrom
feat/tendlc-brands

Conversation

@kshahbw

@kshahbw kshahbw commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

What this adds

band tendlc brand (9 commands) and band tendlc vetting (3 commands), end to end — service writes, the per-brandType validation matrix, lossless read-modify-write updates, --confirm fee gates, and --wait polling.

PR 3 of 5. Stacked on #32 (feat/customer-profiles) — review that first; this diff is only the brand/vetting layer.

brand create refresh list get update delete reverify resend-2fa history
vetting list request import

The old tendlc campaigns / numbers / number commands are untouched. The command-tree cutover is PR 5, so both trees coexist for one release as a real deprecation window.

The guard worth reviewing first

POST /brands silently discards an invalid customerProfileId and returns 202, creating an orphan brand with no profile association — a typo produces a billable, permanently-unverifiable brand and a success receipt. I hit this by accident while probing.

brand create now pre-flights the profile. Verified live: the exact input that produced the orphan brand now exits 3 and creates nothing (brand count 10 before, 10 after). The check degrades rather than blocks — Customer Profiles Access is a separate role from Campaign Management, so a 403 on the pre-flight warns and proceeds rather than locking out someone entitled to create brands.

Two defects live testing caught that the stub suite could not

A registering brand is indistinguishable from a rejected one. brandIdentityStatus reads UNVERIFIED for the entire registration window; REGISTERING is never returned on the read path despite being in the published enum. Two brands with byte-identical payloads: one VERIFIED at 46s, one still UNVERIFIED at 275s with no TCR response at all. Our classifier called UNVERIFIED terminal, so brand create --wait would have exited 4 — "identity could not be confirmed" — within seconds for every brand, including every one that goes on to verify. It now keeps polling and surfaces the last-seen status at timeout.

I rejected the clever fix: you can separate the two cases by grepping brand history for a BRAND_IDENTITY_STATUS_UPDATE entry, but that message is free text, undocumented, and unversioned. Coupling poll control flow to its wording would be worse than the latency.

We were telling users something untrue about an irreversible operation. The API docs say deleting a brand deletes its customer profile. It does not — both test profiles survived with softDeleted: false. The --confirm message repeated that claim; it now says the profile survives and must be deleted separately.

Design decisions worth a second opinion

  • update returns an acceptance, not the resource, and takes ~5 minutes to apply. Measured. The receipt now says so. This independently confirmed the earlier decision to withhold --wait from update — polling would have returned instantly against pre-update state and reported success.
  • No --bandwidth-id filter on list. bandwidthId[eq] is accepted and silently ignored, returning every brand. Handing back wrong data with no error is worse than not offering the flag.
  • --class RCS is supported despite being absent from the published enum, because production accepts it.
  • No --country flag — the API derives country from countryCodeA3. This corrects an earlier finding of our own that said both were required.
  • SOLE_PROPRIETOR gets common-field validation only. Its rules are account-gated and unobservable on any account we have; inventing them would reject requests the API accepts.
  • brand create is billable but ungated — the fee lands ~6s after the 202, before verification resolves, and is charged even when the brand never verifies. Create needs ~13 explicit flags and can't happen by accident, so it stays ungated, but that exception is now documented rather than silent.

Testing

Ten tasks, each with an independent review and fix loop, then a whole-branch review that returned six Important findings — all fixed here.

Live-verified against 9900000: every read command, all four --confirm refusals (each exit 6, zero write requests), validation aggregation, the pre-flight guard, a full create→verify→delete lifecycle, RMW losslessness (46 keys before and after, zero nulled), and refresh accepting either identifier. All test resources cleaned up; brand count back to its baseline.

Stub-verified only: vetting request (places a real billable order with an external provider) and brand reverify ($4 per call). Their refusal and validation paths were verified live; their happy paths were not.

Follow-ups, not in scope

  • SilenceUsage is never set on the real root, so every error in the whole CLI dumps a usage block — pre-existing and CLI-wide, worth its own issue.
  • The pre-flight checks HTTP status but not softDeleted; the API's behavior with a soft-deleted profile is unmeasured, so blocking on it could refuse creates production would accept. Measure in PR 4.

Live probing added 12 findings to the spec-vs-production log (now 31), including the two above. the internal API-spec tracking ticket has been updated.

kshahbw added 18 commits August 18, 2026 14:57
…cape

Capture and assert on r.URL.EscapedPath() instead of the decoded r.URL.Path to
ensure the test detects when brandPath() stops calling url.PathEscape. The decoded
path is identical whether the ID was escaped or not (both produce /brands/a/b c/history),
so asserting on the decoded path would pass even if escaping was removed.
…bering, correct 12-key projection, label reconstructed receipt
@kshahbw
kshahbw requested review from a team as code owners August 19, 2026 15:35
@bwappsec

bwappsec commented Aug 19, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@kshahbw

kshahbw commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Independent adversarial pass (codex) — 4 Important + 1 Minor, all real

Ran a fresh reviewer over the branch after it had already cleared ten per-task reviews and a whole-branch review. It found five things, and I verified every one against the source before acting. Three were this PR's to fix; two are CLI-wide and are now documented rather than silently papered over.

Fixed

brand delete claimed completion before the delete had happened. The receipt set deleted: true the moment the DELETE was accepted — but deletion takes ~40s to actually take effect, and only a follow-up 404 proves it. Worse, --wait --timeout 0 exited 5 while still printing deleted: true: a receipt contradicting its own exit code. Now deleted stays false with a status: accepted and a note on how to confirm, and flips to true only at the exact moment a 404 proves it.

brand update skipped per-brandType validation that brand create performs. --vertical "" on a PRIVATE_PROFIT brand overlaid null, passed validation, and hit a production 400 as exit 1 — while the identical mistake on create is caught client-side at exit 6. This was the third instance of that same create/update asymmetry in this PR. Update now applies the same per-type tier, still skipping SOLE_PROPRIETOR (rules unobservable) and still aggregating rather than short-circuiting.

A test that could not fail. TestBrandGetAcceptsEitherIdentifier drove both identifiers but asserted only on the second, so an implementation that always requested the Bandwidth ID would have passed. That was the ninth instance of this pattern found on this branch, and the reason every high-value test here now has a recorded break-and-restore proof.

Documented, not fixed — both are CLI-wide and belong in their own change

Ctrl-C during --wait loses the receipt. Execute() wires no signal-cancelled context and there is no signal.Notify anywhere in the repo, so awaitTerminal's cancellation branch — which exists precisely to emit the receipt — can never fire from a real SIGINT. The receipt guarantee in AGENTS.md now says so explicitly, along with the recovery path.

--timeout is checked between polls, not enforced as a deadline. cmdutil.Poll returns on done before testing the deadline and always sleeps a full interval, so a call can overshoot by up to one interval plus the in-flight request — and can even return success after the timeout has expired. AGENTS.md no longer implies precision the implementation doesn't deliver.

Both are pre-existing and shared with number order, call create, and transcription create. Fixing them means touching cmd/root.go and internal/cmdutil/poll.go, which is not this PR's business.

Heads-up on CI

.github/workflows/ci.yml triggers on pull_request: branches: [main], so this PR and #32 run only the three Snyk checks — no test matrix, no lint, no security job. Locally at HEAD: 33 packages ok, vet/gofmt/golangci-lint all clean. Worth deciding whether the trigger should include feat/** so stacked PRs get the real gate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants