feat(tendlc)!: command-tree cutover and number reads (PR 5 of 5) - #41
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Independent adversarial pass (codex) — one Critical, and it was realRan a fresh reviewer over the branch after six per-task reviews, a whole-branch review, a fix wave, and a scoped re-review had all cleared it. It found a data-loss bug every one of them missed. Verified all three findings against the code before acting. The retry still erased unchanged customer dataThe previous round narrowed the retry so it could not drop a field the caller changed in this invocation. That is not the right test, and the framing error was mine — I offered two routes and the narrower one was taken. An unchanged field still holds real data. Concretely: a brand has a Same exposure on
Punctuation bypassed the doc-contract guardThis line passed the check: The comma made Two documentation slipsA port-in example that exists to demonstrate multiple numbers was showing the same number twice — collateral from my own placeholder sanitization sweep. And one caller-id was outside the reserved range. Worth stating plainly: this is a second fix wave, which the review process I was following allows only one of. I took the deviation deliberately rather than park a known data-loss bug behind a process rule. |
5907f7c to
3683456
Compare
3683456 to
406b36c
Compare
406b36c to
1597861
Compare
…rojection docs Re-measurement showed campaignId[contains] filters correctly (the earlier zero-match probe used a campaign with no assigned numbers) and the list projection has two shapes, not a fixed five keys (assigned numbers carry three extra fields). status remains confirmed dead under every operator and value, so --status stays absent.
Two claims in ListPhoneNumbers' doc comment were wrong, both from probe design rather than API behaviour. The projection is conditional, not fixed: 16 of 23 records carry five keys, the 7 assigned to a campaign carry three more. The original claim came from a single record fetched with limit=1. campaignId[contains] works correctly. The original probe filtered on a campaign with no assigned numbers, got zero results, and read the empty set as a broken filter. Re-tested against a campaign with three numbers, it returns three. status genuinely does not filter under any operator, including a value that matches nothing on the account.
Deletes band tendlc campaigns, campaigns numbers, numbers, and the flat number <tn> command, ending the deprecation window from the two previous PRs where the legacy and new command trees coexisted deliberately. No aliases or shims: v0.3.0-beta status and no substantial customer traffic justify a clean break here. number.go's three subcommands (list/get/history) now attach to a plain numberCmd parent registered directly on Cmd, matching brandCmd/campaignCmd, instead of the numberGetCmd node that lived in the now-deleted numbers.go. extractData and filterNumbers in helpers.go existed only to serve the deleted commands and are removed along with their tests. Also folds in a carried-over test assertion: number list must not gain an exact-match --campaign-id flag, since campaignId[eq] is silently ignored by the API and returns every record — the same anti-pattern already avoided by brand list and campaign list.
band tendlc campaigns, band tendlc numbers, and band tendlc number <tn> -- all removed in the previous commit -- exited 0 with a help dump instead of failing, because cobra checks Runnable() before it ever consults Args, and none of Cmd/brandCmd/campaignCmd/numberCmd/vettingCmd had a RunE. A stray token that used to be a real, now-deleted command was indistinguishable from a successful help request. Gives each of those five dispatcher commands Args: cobra.NoArgs plus a trivial RunE (return cmd.Help()) so NoArgs actually runs: a bare invocation still prints help and exits 0, but a trailing token matching no subcommand now exits non-zero. customer-profile has the same latent shape but is out of scope for this branch.
…-contract parser Adds a third gate that calls the resolved command's real cobra Args validator against the documented arguments, catching stale references that resolve completely (so the Use-string heuristic sees no remainder to judge) but whose command no longer accepts what follows it.
…sion The parser fix landed with five knownDriftCommands entries: four for real drift the deletion left behind, one for a shell comment that parsed as a command. All five are now unnecessary. The six stale lines are rewritten to the new tree, the numbers block no longer advertises a --status filter the API silently ignores, and the parser skips shell comments inside fenced blocks — prose that mentions a command mid-sentence is not an invocation. knownDriftCommands is empty. Verified by planting three shapes of stale reference and confirming each is caught: a deleted subcommand, a deleted positional form, and a bare argument on a parent that takes none.
brand update and campaign update build a full-replacement PUT body by stripping a known list of read-only keys from the resource the API just returned. That only works because production currently accepts read-only fields it does not use. If that is ever tightened to a 400, both commands break the same day, since the strip lists cannot enumerate every field the API might start rejecting. Add putReplaceWithReadOnlyRetry, shared by UpdateBrand and UpdateCampaign: on a 400 whose error source.POINTER values name top-level fields present in the outgoing body, strip exactly those fields and retry once, noting the drop on stderr. Any other 400, or a retry that also fails, surfaces untouched/original. No loop, no backoff, no mutation of the shared strip lists.
Pre-existing on main, in the band number list example. This repo is public and the 919 numbers are real Bandwidth TNs; the rest of the docs already use the reserved 555-0100 block.
An independent adversarial pass found the previous guard insufficient. It protected fields the caller changed in this invocation, but an unchanged field still holds real data: a brand with a stored website that no longer passes validation would have that website dropped from the retry body by an unrelated --display-name update, and a full-replacement PUT nulls it. neverDrop is now built from the entire update flag surface, so the retry can only ever drop a field the CLI does not model at all — which is the only case it was designed for. The invariant is stated in putretry.go. Also: trailing punctuation no longer bypasses the doc-contract parser, so a documented 'band tendlc campaigns,' is caught rather than abstained on; a port-in example shows two distinct numbers again; and a caller-id moves into the reserved range.
The doc-contract parser is line-oriented and splits on "\n". On a Windows checkout the files land with CRLF, so the trailing "\r" survives the split and glues itself to the last token on every line. That broke the gate two ways. A command token became "get\r", which fails commandTokenRe, so the path resolved one token short and the test reported `band tendlc campaign` rejecting "get". And the lone "\" shell line-continuation marker became "\\\r", which no longer matched the documented abstain rule for it, so multi-line examples were parsed as if the continuation backslash were a real positional argument. Four AGENTS.md examples failed this way on windows-latest only. Normalize once at the read site rather than defending against "\r" at each token check downstream. Verified: passes under LF, passes under simulated CRLF, and still catches a planted `band tendlc brandz list` under CRLF -- the normalization does not neuter the gate.
1597861 to
19f463b
Compare
4 participants
What this adds
The last PR in the 10DLC series.
band tendlc number(list/get/history), the legacy command tree deleted outright, a doc-contract test that can finally catch stale command references, and a self-healing retry that neutralizes the one API change most likely to break this CLI.PR 5 of 5. Stacked on #40 — review that first.
Paired with an api-specs change that must not merge alone. Branch
docs/tendlc-number-cli-referenceis prepared locally and unpushed. Publishing docs first describes commands the released CLI does not have; publishing the CLI first orphans four reference pages. They ship together or not at all.The breaking change
Deleted with no aliases:
band tendlc campaigns,campaigns numbers,numbers,number <tn>. The deprecation window was PRs 3 and 4, where both trees coexisted deliberately.band tendlcnow lists exactlybrand,campaign,number,status,vetting.Deleting them surfaced a bug worth naming: removed commands exited 0.
band tendlc campaignsprinted help and returned success whileband notacommandcorrectly exits 1 — so for an agent, a deleted command was indistinguishable from a working one. The cause is subtle: cobra'sexecute()checksRunnable()and short-circuits beforeValidateArgsever runs, which makesArgs: cobra.NoArgsdead code on a command with noRunE. Fixed on all five dispatchers.The doc-contract test could never have caught this
It only errored when a documented path matched zero command tokens. Since
tendlcstays valid,band tendlc campaigns listmatched one token and passed — so the test was structurally blind to exactly the drift this PR creates.The obvious tightening is wrong:
commandTokenReis^[a-z][a-z-]*$, so lowercase positionals parse as command tokens and valid lines likeband auth use adminwould fail. The fix resolves as far as possible, checks whether the resolved command'sUsedeclares a placeholder, then calls the command's ownArgsvalidator on the remainder — cobra's real answer rather than another heuristic.knownDriftCommandsis empty. Every suppression was removed rather than deferred, including one that turned out to be a shell comment the parser was reading as a command. Verified by planting three drift shapes — deleted subcommand, deleted positional form, bare argument on a parent — and watching each fail.The retry, and the bug the review caught in it
brand updateandcampaign updatebuild a lossless full-replacement PUT body that includes fields the API currently tolerates. If that is ever tightened to a 400, both break the same day. The retry recognizes that specific 400, drops the named fields, retries once, and says so on stderr.The whole-branch review found it could fire on the wrong 400. Its discriminator was "error pointers naming fields in the body" — but that body holds the caller's own edits too, and all 23 caller-settable brand fields sit outside the read-only list. So
brand update --website "not a url"would have deletedwebsitefrom the retry body and, on a full-replacement PUT, cleared it — exiting 0 with a success receipt.Now the retry can only ever drop a field the caller did not ask about. The implementer also caught that neither route I suggested would protect campaigns' four compliance attestations, which have no update flag at all and so can never be "caller-set" — they needed an explicit never-drop set. Its correction was right and my brief was incomplete.
Testing
Six tasks, each independently reviewed with its own fix loop, then a whole-branch review, a fix wave, a scoped re-review, and an independent adversarial pass.
Worth noting what the reviews caught, because the pattern is consistent: an implementer found a cobra routing collision where a second command named
numberwould have silently shadowed the legacy one; another found its own escaping test passed with the escaping removed, because Go'snet/urlre-derives canonical encoding. Theputretrysuite passed only because every fixture used invented field names no caller could set — the fixtures could not express the bug that was there.Also in here
Two real Bandwidth phone numbers were sitting in
AGENTS.mdexamples, pre-existing onmain. Replaced with reserved-range placeholders. Three references to deleted commands incmd/message/preflight.go— user-facing remediation advice on a failedmessage send— repointed; the doc-contract test never reads Go string literals, which is why nothing caught them.Live probing added findings to the spec-vs-production log, now at 46 across 12 sections, including one retraction: a filter I had reported as broken turned out to work, because my probe had filtered on a campaign with no assigned numbers and I read the empty result as a failure. An empty result proves a filter matched nothing, not that the filter is broken.