[APIE-1608] Wire agentdetect into the usage-report path - #3487
Draft
David Adams (davidadas) wants to merge 21 commits into
Draft
[APIE-1608] Wire agentdetect into the usage-report path#3487David Adams (davidadas) wants to merge 21 commits into
David Adams (davidadas) wants to merge 21 commits into
Conversation
Remove references to internal-only artifacts (friction logs, ticket IDs like FF-9295, the signals-comparison memo, "proof of concept" framing, LaunchDarkly, and capture dates) from comments and test descriptions, and neutralize the agentdetect-poc fixture string. Purely comment/description changes plus one non-asserted test fixture value; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The " helper" suffix is an Electron packaging convention, not an editor one, so classifyEditorHelper previously tagged any Electron app's helper (Slack, Discord, Hyper — itself a terminal) as kindIDEHost with an empty vendor, polluting the IDEHost signal. Gate classification on the product matching a known kindIDEHost row; unrecognized Electron apps now fall through to kindUnknown. Trades away recognizing an unseen VS Code fork purely from its helper (theoretical, since its main basename would be unknown too) for not misclassifying non-editor Electron apps. Splits the fork-generalization test into known-editor and non-editor-Electron cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A helper-matched IDE host previously resolved to an empty key, so its
identity was dropped from the wire (optional("") is nil) even though the
editor was known — a VS Code chain whose nearest editor process is a
"code helper (plugin)" emitted ide_host: nil. Resolve the helper to its
editor's table key ("code helper (plugin)" -> "code"), the same way the
version-suffix rule resolves "python3.13" -> "python", so ide_host now
carries the editor while still only ever recording table vocabulary,
never the observed helper basename.
Applies to the Electron editors (code, cursor, windsurf); zed and
JetBrains have no helper subprocesses and were already keyed directly.
Also tightens surrounding comments.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code review of the agentdetect package found the privacy invariant (only fixed-table keys, never raw process text, reach AncestorMatch/WrapperMatch) was enforced only by discipline and a runtime test. Introduce fpKey, a private string type returned only by the fingerprint resolvers, so a future assignment of raw process data into one of these fields is a compile error. Also close a gap where walk()'s own budget check is cooperative and never runs while a single ProcSource.Info call is blocked — on darwin several gopsutil calls ignore their context, so a slow syscall could stall past the configured budget. boundedWalk now enforces a hard wall-clock ceiling above that budget and gives up on the walk if it's exceeded. Also clarifies the maxIdentityArgs doc comment and notes the follow-up-PR export need on vendorForProcKey/vendorForArgvPattern.
childStart, the walk's pid-reuse baseline, was only ever set from a descendant already walked, so depth 1 (StartPid) had no prior read to compare against and went unchecked — a recycled pid one hop above the CLI could fabricate an ancestor undetected. Add Options.SelfPid (defaulting to os.Getpid()) to seed childStart with the CLI's own start time before the walk begins; a lookup failure just disables the depth-1 check, same as any other unreadable start time.
ChainShape doc claimed a completed walk always ends in 'n' or 'r', but a walk that reaches PID 1 without an init/remote ancestor produces a valid shape like "sit" — drop the false claim. Also fixes "Detect method" (it's a function) and the field-probe test's file header, which said it's outside the automated suite even though go test discovers and skips it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull request overview
Wires the pkg/agentdetect signal collection into the Cloud usage-reporting path, mapping the derived agentdetect.Attributes() onto new CliV1Usage telemetry fields (pending an SDK schema update), and expands/adjusts the agent detection implementation and tests to be safe, bounded, and privacy-preserving.
Changes:
- Invoke agent detection during usage reporting (
internal/reportUsage→Usage.CollectAgentDetect) and map results onto new usage payload fields. - Implement/extend agent detection (process ancestry + env markers) with a defined wire-format projection (
Attributes) and extensive synthetic + live tests. - Add
gopsutil/v4dependency for cross-platform process inspection and ignore the debug binary output in git.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/command.go |
Hooks CollectAgentDetect() into the existing usage-report path. |
pkg/usage/usage.go |
Adds CollectAgentDetect() and helper functions to map agentdetect.Attributes() onto usage fields. |
pkg/usage/usage_test.go |
Adds a basic unit test asserting the agent-detect mapping ran. |
pkg/agentdetect/doc.go |
Package-level documentation describing signals, privacy constraints, and bounds. |
pkg/agentdetect/detect.go |
Core detection logic: bounded ancestry walk, env/CI/TTY signals, redaction utilities. |
pkg/agentdetect/attributes.go |
Defines the telemetry wire projection (Attributes) and mapping from Result. |
pkg/agentdetect/fingerprints.go |
Adds/updates fingerprint tables and resolution helpers for vendors/kinds/patterns. |
pkg/agentdetect/proc.go |
Defines ProcInfo/ProcSource abstraction for process tree lookup. |
pkg/agentdetect/proc_gopsutil.go |
Implements ProcSource via gopsutil with timeouts and best-effort fields. |
pkg/agentdetect/scenarios_test.go |
Adds synthetic scenario fixtures for regression/failure-mode coverage. |
pkg/agentdetect/ide_surfaces_test.go |
Adds modeled/captured IDE surface fixtures and assertions. |
pkg/agentdetect/detect_test.go |
Large synthetic test suite covering detection behavior, bounds, and privacy invariants. |
pkg/agentdetect/attributes_test.go |
Tests for wire-format correctness (keys vs vendors, omissions, invariants). |
pkg/agentdetect/proc_gopsutil_test.go |
Live-process-tree smoke tests for the real gopsutil source. |
pkg/agentdetect/field_probe_test.go |
Opt-in manual probe test for local field debugging (AGENTDETECT_PROBE). |
go.mod |
Adds github.com/shirou/gopsutil/v4 and related indirect deps. |
go.sum |
Records checksums for newly introduced dependencies. |
.gitignore |
Ignores agentdetect-debug binary produced for manual testing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+51
to
+55
| defer func() { | ||
| if r := recover(); r != nil { | ||
| log.CliLogger.Tracef("agent detection panicked: %v", r) | ||
| } | ||
| }() |
Comment on lines
+32
to
+35
| // StartTime gates our pid-reuse guard & a zero value disables it, so it fails rather than tolerating. | ||
| if info.StartTime == 0 { | ||
| t.Error("StartTime is 0 — the pid-reuse guard is disabled on this platform") | ||
| } |
Comment on lines
+321
to
+340
| func boundedWalk(opts Options) walkResult { | ||
| done := make(chan walkResult, 1) | ||
| go func() { | ||
| // walk touches live OS process data through ProcSource, so a panic here | ||
| // must degrade to an empty result rather than crash the process | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| done <- walkResult{meta: WalkMeta{StoppedAt: "panic", Truncated: true}} | ||
| } | ||
| }() | ||
| done <- walk(opts) | ||
| }() | ||
|
|
||
| select { | ||
| case w := <-done: | ||
| return w | ||
| case <-time.After(opts.Budget + hardTimeoutSlack): | ||
| return walkResult{meta: WalkMeta{StoppedAt: "hard_timeout", Truncated: true}} | ||
| } | ||
| } |
Comment on lines
+80
to
+85
| func optionalStrings(s []string) *[]string { | ||
| if len(s) == 0 { | ||
| return nil | ||
| } | ||
| return &s | ||
| } |
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.
Draft — opened for visibility/tracking, not ready for review.
Follow-up to #3472 (
pkg/agentdetect, merged). This wiresagentdetect.Detect()into the real usage-report path and assigns the results ontoCliV1Usage's new agent-detect fields.Status: does not compile yet
This branch assigns onto 9
CliV1Usagefields (AgentEnv,AgentProc,AgentArgv,IdeHost,Interactive,ChainShape,Wrappers,Ci,AgentTables) that don't exist inccloud-sdk-go-v2/cliyet (currently pinned to v0.3.0 ingo.mod, unchanged on this branch). This is expected — per the branch's own commit messages, it's waiting on the SDK schema update (APIE-1607) beforego buildwill pass.The server side of APIE-1607 is up for review now: confluentinc/cc-cli-service#1079 adds the matching 9 fields to
cli_service.v1.Usage. Once that (or an equivalent) merges andccloud-sdk-go-v2/cliregenerates with the new fields, this branch should just need ago.modbump and a rebase onto currentmain(it's a bit behind — some unrelated renames likeusm→unifiedstreammanagerlanded after this branch was cut).What's already done here
internal/command.go: callsUsage.CollectAgentDetect()from the existing usage-report path.pkg/usage/usage.go: mapsagentdetect.Attributes()onto the 9 new fields, with nil-vs-empty-string handling matching the existingFlags/StackFramesconvention.pkg/agentdetect/detect.go: moves panic recovery into the walk's own goroutine, since adefer recover()in the caller can't catch a panic from a separate goroutine — this part is a real, independent fix over what's onmain.Note
The mapping is explicitly flagged in-code as
UNVERIFIED AGAINST THE FINAL SCHEMA— field names/types should be re-checked once the generated SDK type exists, in case anything drifted from the design doc during implementation of either side.