[APIE-1608] Wire agent detection into the CLI usage-report path - #3515
Draft
Noel Cothren (noeldevelops) wants to merge 13 commits into
Draft
Noel Cothren (noeldevelops) wants to merge 13 commits into
Noel Cothren (noeldevelops) wants to merge 13 commits into
Conversation
Runs Detect from reportUsage's existing cloud-login/gov gate. Detect already degrades to empty signals internally (walk timeout, depth cap, lookup failure), so this can't fail the invocation; the recover is a backstop against a panic escaping Detect. Assigning the computed Attributes onto CliV1Usage's new fields is left as a TODO until APIE-1607 lands the schema update in ccloud-sdk-go-v2.
Maps agentdetect.Attributes field-for-field onto the new CliV1Usage flat fields described in the metrics guide (confluentinc.atlassian.net/wiki/spaces/AEGI/pages/6089736699). This will not compile until APIE-1607 lands those fields in ccloud-sdk-go-v2/cli; expected to be picked up by an SDK bump before this PR is ready for review.
Generalize optionalStrings' nil-when-empty rule to the string fields too (via a new optionalString), since attrs.ChainShape can be "" on an ancestry walk that hits pid 1 immediately. Unconditionally wrapping it in *string would send an empty string instead of omitting the field, unlike the doc's stated contract.
A defer recover() in CollectAgentDetect can't catch a panic from boundedWalk's goroutine (a goroutine panic is only recoverable within that same goroutine), so the walk logic that touches live OS process data was the one part of Detect actually unprotected. Move the recover into the goroutine itself so every caller of Detect gets the same degrade-to-empty guarantee, not just this call site. Also swap the unconditional PtrString wraps for a nil-if-empty optionalString helper, since attrs.ChainShape/Interactive/AgentTables can be "" and should be omitted rather than sent empty, and firm up the smoke test to assert the always-set fields actually got assigned.
Adds TestWalkPanicDegradesInsteadOfCrashing: a ProcSource.Info that panics must degrade boundedWalk to a truncated, StoppedAt="panic" result instead of crashing the process. This is the one part of the "must never fail the invocation" guarantee that was previously untested. Also calls out explicitly, in usage.go, that the pointer types assumed for the new CliV1Usage fields are a bet on codegen matching the existing Command/Flags convention, not a confirmed fact — re-check once APIE-1607's generated struct actually exists. Per code review of c704717..acb5154.
CollectPanic builds its own Usage independently of usage.New()/Collect(), so it needs its own call to CollectAgentDetect() — otherwise a panicking invocation reports usage missing all 9 agent-detect fields even after the normal path has them. Flagged in APIE-1607's own description as a second call site that must be updated in step. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ale comment - CollectAgentDetect's recover() logged the panic value itself (%v), which could carry a raw process path/arg surfacing from a dependency like gopsutil. Log only %T (the panic's type) instead — enough to debug from, without the leak risk this package is otherwise careful to avoid. - optionalString/optionalStrings' doc comment claimed to mirror the existing Flags/StackFrames convention, but Collect() actually always sets Flags (even empty), it doesn't nil-check. Corrected the comment to state the actual (intentional) reason these fields differ: an empty agent-detect field means detection produced nothing, a different signal from "ran and found zero," so it's omitted rather than sent empty. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… expire time.After allocates a timer that can't be stopped, so even when walk() finishes well within budget, the timer keeps running until it fires later — wasted allocation and timer churn on a path that runs once per CLI invocation. Switch to time.NewTimer with a deferred Stop(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Time==0 The production contract already tolerates StartTime==0 (it just disables the pid-reuse guard); this test was stricter than the code it exercises, failing outright instead. Some platforms/sandboxes legitimately can't read process creation time, which made this flaky rather than meaningful. Log it instead of failing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… one caller The CollectAgentDetect() call just added directly to CollectPanic() was wrong: internal/command.go's reportUsage() already calls it internally, gated behind cfg.IsCloudLogin() && !cfg.HasGovHostname() (existing wiring from this same branch). Adding it inside CollectPanic() too meant that caller ran detection twice — once ungated (inside CollectPanic, so it'd run even for gov/on-prem panics before any gate had a say) and once gated (inside reportUsage, overwriting the first result before Report()). CollectPanic is shared by two callers; only one of them (reportUsage) already had the call. Move it out of the shared function and into the one caller that was actually missing it: internal/flink/command_shell.go's own reportUsage closure, at the same point relative to Report() as the other one, preserving its existing gate exactly as-is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
Copilot started reviewing on behalf of
Noel Cothren (noeldevelops)
September 15, 2026 18:32
View session
There was a problem hiding this comment.
🟡 Changes recommended
The public SDK release and dependency update remain pending, with unresolved review comments on diagnostics, latency, and test coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Wires agent-detection signals into Cloud CLI usage telemetry and panic-reporting paths.
Changes:
- Maps detection attributes onto usage events.
- Adds timeout and panic handling.
- Adds and updates related tests.
File summaries
| File | Summary |
|---|---|
pkg/usage/usage.go |
Maps agent signals into telemetry. |
pkg/usage/usage_test.go |
Tests usage collection. |
pkg/agentdetect/proc_gopsutil_test.go |
Adjusts platform-dependent testing. |
pkg/agentdetect/detect.go |
Hardens timeout and panic handling. |
pkg/agentdetect/detect_test.go |
Tests panic degradation. |
internal/flink/command_shell.go |
Collects signals in panic reports. |
internal/command.go |
Collects signals in normal reports. |
Review details
Suppressed comments (2)
pkg/usage/usage.go:55
- Recovering the panic keeps telemetry from affecting the command, but logging only
%Tmakes a production detector panic effectively undiagnosable. Keep the panic value redacted while also preserving an actionable stack trace (as the top-level panic handler does) so failures in the OS-walk dependency can be investigated.
log.CliLogger.Tracef("agent detection panicked: %T", r)
pkg/usage/usage.go:59
- This runs synchronously before
Reportand before the CLI returns, so every Cloud invocation now waits forDetect. Its default budget is 50 ms andboundedWalkadds a 200 ms hard-timeout slack, meaning a blocked process lookup can add up to about 250 ms to command completion despite the PR's claim that detection will not slow the command. Please move detection/reporting off the critical path or use a deliberately tighter asynchronous budget.
attrs := agentdetect.Detect(agentdetect.Options{}).Attributes()
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Rename three attributes so they land as new, correctly-typed (repeated) BigQuery columns, and so the names better reflect the data captured: agent_env -> agent_env_vars (env var names) wrappers -> cmd_wrappers (command wrappers) ci -> ci_providers (normalized CI provider ids) Renames the Attributes wire projection (fields + JSON tags) and the CliV1Usage mapping in pkg/usage. The internal Signals struct (local diagnostics only, not sent to the service) is intentionally unchanged. Still blocked on the public SDK bump; the matching field rename is already merged in the SDK/cc-cli-service, just not vendored here yet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
IBM Bob is a VS Code fork (Electron app). Its main process normalizes
to "ibm bob" and its helpers ("IBM Bob Helper (Plugin)", etc.) resolve
to the same key via classifyEditorHelper. Classifying as kindIDEHost,
consistent with "code"/VS Code — agent-capable environment, not a
confirmed agent-initiated call.
Note: Bob does not currently inject env vars (e.g. AI_AGENT) into
terminal sessions spawned by agent plugins, so ide_host is the only
available signal from the CLI side. chain_shape changes from "s??" to
"see" when running inside Bob.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
This PR does not compile against
mainyet.pkg/usage/usage.goassigns new fields ontocliv1.CliV1Usage. Those fields are merged into the API spec, and the rename changes are in progress for the internal SDK (ccloud-sdk-go-v2-internal/cli), but the public SDK has not been updated, regenerated, or released yet.Release Notes
No user-facing changes — internal usage telemetry only (no new/changed commands, flags, or output).
Checklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below.Blast Radiussection below.What
Wires
pkg/agentdetect(merged separately in #3472) into the existing CLI usage-event path so each invocation attaches lower-level agent-detection signals (process ancestry, agent env-var names, TTY/interactivity, CI, wrappers, fingerprint-table revision) to the usage event.Applies to: Confluent Cloud only. Collection is gated behind
cfg.IsCloudLogin() && !cfg.HasGovHostname()— no data from Confluent Platform / on-prem or gov (FedRAMP) users.Key details:
usage.CollectAgentDetect()runsagentdetect.Detectand maps itsAttributesonto the newCliV1Usagefields. Empty results are omitted (omitempty), so "detected nothing" stays distinct from "ran and found zero."reportUsagepath and the panic-report path (internal/flink/command_shell.go), both behind the same existing gate.Blast Radius
Very low. This only adds fields to an existing, Cloud-only, best-effort usage event:
References
cli.v1.Usageagentdetectpackage #3472 — theagentdetectpackageTest & Review
Ran the modified test build against Staging with the internal SDK using Claude Code, Cursor, IBM Bob, and VS Code Copilot chat, all gave the expected results. 🥳
Verified locally against the internal SDK (
ccloud-sdk-go-v2-internal/cli, which already carries the fields) via a throwawaygo.modreplace:go build ./...— passesgo vet ./pkg/usage/...— cleango test ./pkg/usage/... ./pkg/agentdetect/...— passCliV1Usagefield names and types matchusage.goexactly (agent_env/wrappers/ci→*[]string, the other six →*string), confirming the field/type mapping ahead of the public SDK.