Uh oh!
There was an error while loading. Please reload this page.
fix(telemetry): the token lookup used a key profiles are not stored under (cli#552) - #553
Conversation
…nder (cli#552) Bugbot's High on the #540 promotion. Two keys for one concept, and the failure was silent by construction. `telemetryToken` took the telemetry LABEL and looked the profile up by it. Profiles are keyed on the RAW `cfg.CurrentEnv`; the label has been through `telemetryEnv`, which lower-cases and trims (via `sessionEnv`) and remaps anything unrecognised onto prod. Whenever those disagreed the lookup did not miss loudly — `Profile()` CREATED an empty profile and returned no token, so delivery took the no-token spool path forever while `authedClient`, reading `cfg.Current()` on the raw key, kept working. The CLI looked signed in and healthy; outcomes simply never arrived. THE PARAMETER WAS THE DEFECT, so it is gone rather than corrected. `telemetryToken()` now reads the current session's token, which is the same thing `authedClient` reads and the same key `SignedIn()` tests. Passing a label in is no longer possible, so the two cannot disagree again — as opposed to agreeing today and drifting the next time `telemetryEnv` gains a case. `config.CurrentToken()` is the reader, and it READS WITHOUT CREATING. `Profile()` storing an absent profile is correct for the write paths it exists for — sign-in mutates the returned pointer, then Saves — and a trap as a lookup: the miss is recorded, so the second call finds a profile and looks like a hit. That is the ticket's separate observation and it is fixed here, not deferred. THE TOKEN STILL GOES WHERE THE SESSION ALREADY TALKS. `api.BaseURL` routes an unrecognised env to prod exactly as `telemetryEnv` does, so the destination the label picks is the one this session's client is already using. That BaseURL does that at all is a real defect — shared with the installer's `_backend_url`, contradicted by client-runtime's controller.py, tracked across three components on backend#2171 — and `telemetryEnv`'s own note says this code must match that behaviour until it changes rather than diverge from it. This fix does not diverge from it. TESTS. There were NONE for `telemetryToken`, which is why this reached a promotion. Added at both levels: * `internal/config` — the reader is keyed on the raw `CurrentEnv` and agrees with `SignedIn()`; it does not create on a miss, with the contrast against `Profile()` asserted in the same test so a refactor cannot route one through the other and still pass; and it is empty on every genuinely-tokenless shape. * `internal/cli` — a table of raw keys that each differ from their resolved label (unrecognised → prod, upper-case, surrounding whitespace, a v1 verbatim `Dev`), plus `prod` as the control that worked before and must still. Each case first asserts the label really does diverge, so the table still means something if `telemetryEnv`'s mapping changes. And the composed behaviour the ticket asks for: a signed-in session whose label was remapped POSTs rather than spools. Real `telemetryToken` over a real on-disk config, real `deliver`, only the URL substituted — `api.BaseURL` has no test seam and would otherwise have sent the test at production. Mutation-proven: the two-key lookup restored 2 failed (incl. the POST-vs-spool test) CurrentToken routed via Profile() 1 failed (the creates-on-miss test) No survivors; green on restore. gofmt, go vet and the full suite clean. Closes#552 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 22, 2026
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9c39b04. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
left a comment
There was a problem hiding this comment.
Good fix, and the right shape of fix — deleting the parameter rather than making the two keys agree. I checked every claim in the description against the source and they all hold:
Profile()does create-on-miss (config.go:65-76), so the old lookup really did record its own miss and then look like a hit.sessionEnvlower-cases and trims (client.go:163-168);telemetryEnvremaps anything!IsKnownEnvonto prod (telemetry.go:130-135). So raw-vs-label divergence is reachable from all four of your table's rows.- The safety claim is the one I most wanted to disprove, and it holds:
api.BaseURL'sdefault:arm returns prod (api/client.go:86-95), andauthedClientbuilds its client fromsessionEnv(cfg)through the sameBaseURL(client.go:166). ForCurrentEnv: "acme"both resolve tohttps://api.tracebloc.io, so the session token goes exactly where that session was already talking. Nothing crosses environments. telemetryTokenhas one production caller (pendingSink), and the only otherProfile()use isauth.go:103-104, which setscfg.CurrentEnv = envand keys the profile with the sameenv— a write path, consistent by construction.
The tests clear the bar. Asserting the divergence first in each case is the part that makes the table durable — without it, a change to telemetryEnv's mapping turns five cases into five copies of the control and nothing fails. Same for pinning Profile()-does-create inside TestCurrentTokenReadsWithoutCreatingAProfile: it's what stops a later refactor routing CurrentToken back through Profile() and still passing. The composed test asserts the header value, not just its presence, which is the right call given an empty token was the whole silent failure.
Verified separately since GitHub reports mergeStateStatus: UNKNOWN on this head: git merge-tree origin/develop 9c39b04 merges clean, one commit behind.
One non-blocking follow-up, in your own idiom rather than against it. SignedIn() and CurrentToken() are now two hand-written copies of the same read, agreeing by test (TestCurrentTokenIsKeyedOnTheRawCurrentEnv) rather than by construction — the exact distinction this PR makes for the env parameter:
func (c*Config) SignedIn() bool { returnc.CurrentToken() !="" }That also gives SignedIn() the nil-receiver guard CurrentToken() has and it doesn't. And while you're there, authedClient's cfg.Current().Token (client.go:167) is the last create-on-read left on that path — cfg.CurrentToken() reads the same value without storing a miss. Both are fine as a separate PR; neither blocks this one.
LukasWodka
commented
Aug 22, 2026
/fr-pass |
Closes#552 — Bugbot's High on the #540 promotion, fixed on
developas the ticket asks rather than on the promotion PR.The defect
telemetryTokentook the telemetry label and looked the profile up by it. Profiles are keyed on the rawcfg.CurrentEnv; the label has been throughtelemetryEnv, which lower-cases and trims (viasessionEnv) and remaps anything unrecognised ontoprod.Whenever those disagreed the lookup didn't miss loudly —
Profile()created an empty profile and returned no token. Delivery then took the no-token spool path forever, whileauthedClient(readingcfg.Current(), the raw key) kept working. So the CLI looked signed in and healthy and outcomes simply never arrived.The fix: the parameter was the defect, so it's gone
telemetryToken()now reads the current session's token — the same thingauthedClientreads, the same keySignedIn()tests. Passing a label in is no longer possible, so the two cannot disagree again, as opposed to agreeing today and drifting the next timetelemetryEnvgains a case.config.CurrentToken()is the new reader, and it reads without creating.Profile()storing an absent profile is right for the write paths it exists for (sign-in mutates the returned pointer, then Saves) and a trap as a lookup — the miss is recorded, so the second call finds a profile and looks like a hit. That's the ticket's separate observation, fixed here rather than deferred.Worth stating explicitly since it's the one thing that could make this dangerous:
api.BaseURLroutes an unrecognised env to prod exactly astelemetryEnvdoes, so the destination the label picks is the one this session's client is already using. Using the session token doesn't send it anywhere new.That
BaseURLdoes this at all is a real defect — shared with the installer's_backend_url, contradicted by client-runtime'scontroller.py(which refuses), and tracked across three components on backend#2171.telemetryEnv's own note says this code must match that behaviour until it changes rather than diverge from it. This fix doesn't diverge.Tests — there were none for
telemetryTokenWhich is why this reached a promotion. Added at both levels:
internal/config— the reader is keyed on the rawCurrentEnvand agrees withSignedIn(); it does not create on a miss, with the contrast againstProfile()asserted in the same test so a refactor can't route one through the other and still pass; and it's empty on every genuinely-tokenless shape.internal/cli— a table of raw keys that each differ from their resolved label:CurrentEnvacmeprodSTGstgsessionEnvlower-cases" dev "devsessionEnvtrimsDevdevmigrateV1stores a v1 env verbatimprodprodEach case first asserts the label really does diverge, so the table still means something if
telemetryEnv's mapping changes rather than silently becoming five copies of the control.And the composed behaviour the acceptance criteria ask for: a signed-in session whose label was remapped POSTs rather than spools. Real
telemetryTokenover a real on-disk config, realdeliver, with only the URL substituted —api.BaseURLhas no test seam and the test would otherwise have sent at production.Acceptance criteria
telemetryTokenresolves the bearer through the same keycfg.Profileis keyed onsessionEnvnormalisation both still find the tokenMutations
CurrentTokenrouted throughProfile()No survivors; green on restore.
Test plan
go test ./...— all packages okgo vet ./...— cleangofmt -l .— clean🤖 Generated with Claude Code
Note
Medium Risk
Touches bearer-token lookup for telemetry POSTs. Destination routing is unchanged, but a wrong token would send credentials or drop outcomes.
Overview
Fixes signed-in CLI outcome events never arriving when the telemetry label (lowercased, trimmed, or remapped to
prod) disagreed with the rawCurrentEnvprofile key.telemetryToken()no longer takes an env. It uses a newConfig.CurrentToken()that readsProfiles[CurrentEnv]without creating an empty profile on miss (unlikeProfile()). Delivery still posts to the same hostapi.BaseURLalready uses.Regression tests cover divergent keys (
acme→prod, case/trim), no-token still spooling, and a remapped session actually POSTing with the session bearer.Reviewed by Cursor Bugbot for commit 9c39b04. Bugbot is set up for automated code reviews on this repo. Configure here.