feat(telemetry): the CLI validated events and delivered none (backend#2217)

[LukasWodka]

Half of backend#2217 — the CLI (Go) half. The installer (bash) half is a separate PR in client; the two are independent and share only the wire mapping, which each repo derives from the receiver.

What was wrong

pendingSink() returned nil, so every command outcome was validated and dropped. The comment said "when the ingest endpoint lands this returns the client that posts to it, and nothing else in this file changes" — which understated it, because there was no spool anywhere in the Go tree and Emitter.Emit calls the sink synchronously on the command's exit path.

The shape — option (c), as decided on the ticket

One inline POST attempt, ~1s budget for the whole step (drain included), spool on any failure, drain at most 20 pending on the next invocation. Explicit non-goals, written into the file so nobody adds them believing they were forgotten: no background daemon, no goroutine outliving the process, no retry loop. The next command is the retry.

The deciding argument against plain fire-and-forget, from the ticket: in a short-lived CLI, async delivery is a lie. A goroutine that outlives main() is killed at exit, so (a)'s only honest forms are "always block" or "always drop on failure" — and dropping on failure discards precisely the partition-time events that are the most valuable thing the CLI can report.

Three things worth a reviewer's attention

1. anyValue switches on reflect.Kind, not on concrete type. The emitter's checkAttrValue deliberately accepts every scalar kind via reflection so an idiomatic caller can pass a named type (type Reason string, or time.Duration through telemetry.Duration). A switch v := value.(type) at the seam matches the dynamic type, misses those, and silently drops at delivery exactly the values the layer above went out of its way to admit — a hole one level below the check that permits them. TestNamedScalarTypesSurviveTheSeam fails if anyone rewrites it that way.

2. A 4xx other than 401/403/408/429 DISCARDS the batch. The endpoint answers 400 when a whole batch is unparseable. Re-spooling that would wedge the spool: the same bad batch re-sent by every future command, forever, pushing good records out at the cap. 401/403 retry instead, because they are a credential state rather than a payload verdict — the next tracebloc login makes those records deliverable.

3. The spool keeps drop-OLDEST, and that is not a contradiction of D7. D7's amended row (rfcs#36) says drop-newest because exporterhelper sheds at the entrance and offers nothing else — a platform constraint on the edge Collector, not a preference. This spool is our own code and can do what D7 originally wanted, so it does: the newest records describe the incident in progress. Same reasoning as scripts/lib/telemetry.sh's tail -n trim, which it mirrors (0600, capped, oldest dropped). Called out in the file so it does not get "fixed" into agreement with a row that describes different machinery.

The wire mapping

Derived from the receiver (common/telemetry/otlp.py::parse_export_logs_request), not from recollection of the OTLP spec:

• One resourceLogs entry per event, never one per batch.service.instance.id is fresh per run and service.version legitimately differs between runs, so a drained spool carries genuinely different resources; collapsing them attributes every event to whichever run happened to be first. The receiver's parser calls this out by name.
• int64 as a JSON string ("41230") — the canonical proto3 encoding. The receiver takes both; sending canonical means the payload is also readable by a stock OTLP consumer.
• boolValue never intValue for booleans. The receiver reads bool first precisely because isinstance(True, int); the sender should not make that necessary.
• No timeUnixNano. Per the #2213 decision no client-side timing is sent, so the receiver stamps arrival and "how late" is knowingly invisible. Adding an event clock is a contract change, not a drive-by.
• Conversion happens at the seam: the spool keeps the compact {resource, attributes} shape, so a human reading a spool file gets something readable and older spooled files survive a mapping change.

A defect this PR's own test found

deliver originally computed api.BaseURL(env) internally, which left no seam — and its first test run posted to production. It now takes a resolved URL. That also removes a second failure mode: one resolution point means the record's label and its destination cannot disagree. The first draft also left a t.Setenv("TRACEBLOC_TELEMETRY_URL_FOR_TEST", …) line that was a no-op — a dead assertion, which is the exact class this epic keeps finding; it is gone rather than papered over.

Test plan

make check green — vet, full suite, fmt-check, file-budget, check-style, check-tool-pins. make lint clean — errcheck, ineffassign, misspell, staticcheck (-checks all,-ST1005).

Mutation-proved, 9/9, no survivors and no build-breaks. Each mutation asserts its anchor applied before running, because an inert mutation and real coverage produce identical logs:

mutation	test that must redden
collapse to one resourceLogs for the batch	TestEachEventGetsItsOwnResourceLogsEntry	KILLED
encode int64 as a JSON number	TestIntegersAreEncodedAsStrings	KILLED
swap reflect.Kind for a concrete type switch	TestNamedScalarTypesSurviveTheSeam	KILLED
invert the trim (keep oldest)	TestWriteSpoolKeepsNewestAndDropsOldest	KILLED
retry a 400 instead of discarding	TestClassifyStatus	KILLED
send the legacy Token keyword	TestPostBatchSendsBearerAndJSON	KILLED
widen the spool to 0644	TestSpoolFileIsOwnerOnly	KILLED
drop on retry instead of spooling	TestDeliverSpoolsWhenTheServerIsUnreachable	KILLED
drop when unauthenticated	TestDeliverSpoolsWhenNotSignedIn	KILLED

No test contacts the network: the partition tests aim at a closed listener and a loopback port.

Not in this PR

• The installer (bash) half — separate PR in client, including the false #1906's forwarder reads both. comment the ticket calls out.
• Duplicate skew — accepted and documented per the ticket's decision, no tracebloc.event.id. A POST whose 202 is lost re-sends, which double-counts for a failure rate; nobody computes that rate yet (the succeeded denominator does not exist either). Recorded on the ticket rather than solved with a contract change buying precision for an unconsumed metric.

🤖 Generated with Claude Code

---

Note

Medium Risk
Adds authenticated HTTP delivery and on-disk event queues on every CLI exit path, including token use and env-partitioned files. Failures are silent and bounded, but a bug here can leak events across environments or hang the product.

Overview
CLI command-outcome events are no longer validated-and-dropped. Each invocation now makes one ~1s POST of OTLP/HTTP JSON to /telemetry/v1/records/, then spools failures to disk and drains a bounded backlog on the next run.

Label, spool file, and destination URL are derived from a single resolved env so a stg record cannot be posted to prod. Spools are per-environment (pending-<env>.jsonl, 0600, cap 50, drop-oldest) so a later login --env dev cannot drain prod-labelled events with a dev token.

The compact {resource, attributes} shape stays on disk; conversion to OTLP happens at the seam (one resourceLogs per event, int64 as proto3 JSON strings). Permanent 4xx (except 401/403/408/429) discards the batch so a bad payload cannot wedge the spool. All transport failures stay silent.

^{Reviewed by Cursor Bugbot for commit c71acc4. Bugbot is set up for automated code reviews on this repo. Configure here.}

[saqlainsyed007]

This is a careful PR — the "async delivery is a lie in a short-lived CLI" framing, the reflect.Kind seam (I confirmed anyValue switches on kind so named scalar types survive), the drop-oldest spool with the D7 rationale, and the 4xx-discard-vs-401/403-retry split are all well-reasoned and the mutation table is exactly the right way to prove the guards. But I'm concurring with the open Bugbot finding at telemetry_transport.go:108 — it's a real High defect, verified against the code, and it defeats this PR's own wire-mapping guarantee #2 on the path that matters most:

The spool round-trip recodes integers as doubles.spooledEvent.Attributes is map[string]any (telemetry_otlp.go:37), and readSpool does a plain json.Unmarshal (:106), so every JSON number comes back as float64. anyValue then hits reflect.Float64 → DoubleValue (telemetry_otlp.go:97-99). So:

• the first, in-memory POST sends tracebloc.cli.exit_code / duration_ms as canonical intValue strings (native int), but
• every drained retry — the partition-time events this whole transport exists to preserve — sends them as doubleValue. Same event, two encodings, and the wrong one on the path you built the spool for.

TestIntegersAreEncodedAsStrings operates on the in-memory shape and never goes through writeSpool/readSpool, so it stays green while this breaks — the vacuity the house bar rejects.

Fix: decode spool numbers as json.Number (dec := json.NewDecoder(...); dec.UseNumber(), or unmarshal attributes through a decoder configured that way), then add a json.Number case to anyValue that emits intValue when the number is integral and doubleValue otherwise. That also keeps the spool file human-readable, which is a stated goal. Then make TestIntegersAreEncodedAsStrings (or a sibling) round-trip through the spool so the mutation "recode int as double on read-back" reddens.

Nothing else blocking from what I've read; once the spool preserves int-ness and a test covers the drained path, I'll finish the pass and approve. (CI is green; this and the Bugbot thread are the only things holding it.)

[LukasWodka]

@saqlainsyed007 — fixed in 74fe2f1, and you were right that it defeated the PR's own guarantee #2 on exactly the path the design exists for. Reproduced before fixing:

in-memory : "tracebloc.cli.exit_code":{"intValue":"2"}
round-trip: "tracebloc.cli.exit_code":{"doubleValue":2}

Took your suggested fix as given — dec.UseNumber() in readSpool, a json.Number case in anyValue emitting intValue when integral and doubleValue otherwise. It keeps the spool human-readable too, which was a stated goal.

One thing your suggestion surfaced that I want on the record, because it is the same bug wearing a different type: json.Number is a named string type, so simply adding a case to the existing kind switch would have matched reflect.String and emitted stringValue — an exit code as a string on the drained path. A different wrong answer, reached by fixing the first one carelessly. The branch therefore sits before the switch, and the mutation "let json.Number fall through to the kind switch" is in the table and reddens.

On your vacuity point: agreed, and it is the more useful half of the review. TestIntegersAreEncodedAsStrings never went through writeSpool/readSpool, so it was structurally incapable of seeing this. Two tests now drive the drained path, and the first asserts the two encodings agree rather than that the drained payload looks right on its own — a test checking only the drained side would be satisfied by both paths being wrong together, which is the same trap one level up.

I also got the first draft of that test wrong in a way worth naming: it asserted "no doubleValue anywhere in the drained payload" and failed on correct output, because the fixture legitimately carries sampling_rate: 0.5. Narrowed to per-attribute assertions. An over-broad assertion reported as a code bug is its own defect, and it nearly cost a wrong diagnosis.

13 mutations now, all killed — the original nine plus dropping UseNumber, the fall-through above, routing integral numbers to doubleValue, and truncating reals to integers. make check and make lint green. Bugbot thread answered and resolved. Ready for your second pass.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 74fe2f1. Configure here.}

[saqlainsyed007]

The int→double fix I asked for is done and verified — readSpool now decodes with json.NewDecoder(...).UseNumber() and anyValue has a json.Number case first (correctly, since it's a named string type the kind switch would otherwise catch as String), emitting intValue for integral values and doubleValue otherwise. Spooled exit_code/duration_ms now survive the retry path as canonical ints. That concern is closed.

Holding on the new Bugbot finding at :84, which is real and is this PR's own invariant, one level out: the spool is a single host-wide pending.jsonl and deliver always POSTs the drained batch to the current run's URL/token. So after a login --env (or CLIENT_ENV change) with records queued, an event labelled deployment.environment=stg is delivered to prod's ingest and accepted under prod's token — exactly the "tagged stg, sent to prod" mismatch the in-run single-resolution fix was built to prevent, just across invocations rather than inside one. Telemetry-only blast radius, but it pollutes the wrong backend and mis-attributes.

Fix direction: stamp each spooled record with the env (or resolved URL) it was created under, and on drain either group by env and POST each batch to its own endpoint, or drop records whose env ≠ the current one with a note. If the team decides a queued-telemetry-across-env-switch is an acceptable edge, that's a legitimate call too — but then say so on the thread and resolve it, rather than leaving the invariant half-enforced silently.

CI is green; this Bugbot thread is the only thing between here and approval. Close it (fix or reasoned resolve) and I'll finish the pass.

[LukasWodka]

Second Bugbot finding fixed in c71acc4 — a genuinely good catch, and the same defect one level up from the one I designed against.

deliver takes a resolved URL precisely so a record's label and its destination cannot disagree within an invocation. A single host-wide pending.jsonl reopened exactly that across invocations: tracebloc login --env dev after a failed prod command drained prod-labelled records into dev's endpoint with dev's token. Reproduced before fixing rather than reasoned about.

Spool is now pending-<env>.jsonl, and deliver takes the resolved spool path rather than deriving one — so label, spool and destination are three views of one resolution in pendingSink. Worth noting that is the third reason on that function's doc comment now, and the second and third each bit once; the pattern is that anything deliver derives for itself is something that can disagree with the record.

@saqlainsyed007 this is a second substantive change since your review, so it wants your eyes again rather than just a re-request: 16 mutations all killed, make check and make lint green, both Bugbot threads answered and resolved.

[saqlainsyed007]

Approving — both findings are genuinely closed and verified:

• int→double (my first pass):readSpool decodes with UseNumber() and anyValue handles json.Numberfirst, emitting intValue for integral values — so a drained retry ships canonical ints, not doubles.
• cross-env leak (:84, my re-review): the spool is now per-env — telemetrySpoolPath(env) → pending-<slug>.jsonl — so a record spooled under one env stays in its own file and a login --env switch drains only the new env's file. Records are partitioned, not cross-delivered and not dropped ("still evidence"). That closes the label-vs-destination invariant across invocations, the way it already held within one.

Nicely, the fix is covered by TestTheSpoolDoesNotLeakAcrossEnvironments, and since env now lands in a filename you added TestSpoolEnvSlugRefusesPathTraversal to keep a crafted env from escaping the telemetry dir — good instinct. CI green, no open threads. The whole delivery design (inline-attempt-then-spool, drop-oldest, reflect.Kind seam, 4xx-discard-vs-401/403-retry) is solid. Approving.

[LukasWodka]

/fr-pass