CLI telemetry and the signup-funnel measurement gap - #501
Conversation
Design for CLI-side PostHog telemetry whose primary job is measuring the segment neither web nor server can see: someone who runs `kcap setup`, signs in, finds no workspace, and quits. Ships direct to phog.kurrent.io (the server can't observe the pre-server funnel), keeps the CLI person anonymous with an `organization` group join, excludes hooks entirely, and flushes funnel steps eagerly because the cohort being measured never runs kcap again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Helm chart sets Tenant__Name from the tenant slug, and a SaaS tenant is
served at {slug}.kcap.ai, so the CLI derives the same value from the URL host
label. Self-hosted has no such guarantee (Tenant__Name defaults to "local"),
so the group is attached only for *.kcap.ai; the org property ships everywhere.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Twelve TDD tasks from settings resolution through docs and AOT verification. Also narrows the spec's org handling: the group and its org property are both SaaS-only. Shipping the property unconditionally would have meant emitting a fragment of an internal hostname for self-hosted users -- unjoinable to anything, and against the never-collect list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lization key - Wrap read-modify-write operations with ConfigFileLock to prevent lost updates when concurrent processes modify different fields (opt-out enforcement critical) - On lock timeout/failure, degrade to unlocked write rather than silently drop - Change [NotInParallel] key from class name to resource (TelemetryState.PathOverride) so other test classes in later tasks can share the same lock - Add test verifying MarkNoticeShown() preserves both Id and Enabled
- Move lock acquisition to BEFORE read to make entire RMW atomic - Refactor into Mutate(Func<,>) helper so all three mutators share one correct path - GetOrCreateDeviceId now checks disabled flag inside locked context - Catch all exceptions from lock acquisition broadly (ArgumentException, WaitHandleCannotBeOpenedException, etc.) to prevent NativeAOT abort - Update doc comment to accurately reflect atomicity guarantee - Fallback to unlocked RMW on lock failure rather than silently dropping changes
- Change Mutate delegate to return nullable TelemetryStateFile? - GetOrCreateDeviceId now returns null to signal no-op when ID exists or disabled - Mutate skips write if delegate returns null (both locked and fallback paths) - Eliminates unnecessary lock acquisition and file rewrites on every invocation - Add test verifying no file rewrite when ID already exists - Document MutateUnlocked fallback behavior: no cross-process guard means concurrent processes can mint different IDs (last-writer-wins on disk)
…-checked flags Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An adversarial review probe found the 40-char bound admitted --prefixed GUIDs: a UUID's alphabet is lowercase hex plus hyphen, exactly the pattern's character class, so ~37% of UUIDv4s matched. A GUID token is 38 chars and cannot fit in 30; the longest real kcap flag is 24. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…racters UUIDs use only lowercase hex and hyphen—exactly this pattern's alphabet. A GUID is 36 chars, so with `--` prefix becomes 38 and fits the original 40-char bound. The new 30-char limit rejects GUIDs structurally by length alone, while real kcap flags max at 24 chars. Add regression test documenting the failure mode so future maintainers know why the bound matters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 40->30 change rested on two wrong numbers of mine. The regex max was 31, not 30, and the longest real flag is --skip-antigravity-instructions at 31, not --skip-antigravity-hooks at 24 -- so the bound landed exactly on the longest real flag with zero headroom. Bound is now 37: above the 31-char floor, below the 38-char GUID ceiling. Both edges get regression tests, since either can break silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… ceiling at 38 The window [31, 38) bounds the shape rule: floor is --skip-antigravity-instructions (31 chars, longest real flag); ceiling is GUID tokens (38 chars total). Setting bound to 37 gives 6 characters of headroom. Both edges are regression-tested: longest real flag must match, GUID-shaped tokens must reject. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tests verify that Build() clones event properties before grafting payload fields, preventing accumulation on retry. Includes nested property test to verify DeepClone is actually used. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The nested test was structurally disconnected from the source event and could never fail. JsonNode enforces single-parent invariant, so shallow copy is not constructible anyway — Build throws rather than silently aliases. Only reachable regression is direct mutation, which the remaining test catches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Widen catch filter to catch all exceptions, not just JsonException/FormatException - InvalidOperationException from GetValue<string>() on wrong field type must be caught - Any exception escaping to NativeAOT runtime causes SIGABRT; graceful degradation required - Add regression test for type-mismatched JSON fields - Document drain/clear atomicity and concurrent append behavior in Clear() doc comment Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixed critical defects: - Separate spooled and queued events to avoid duplicating spooled events on repeated failures - Move PostHogPayload.Build inside try block for proper spill on payload errors - Replace enumerated exception filter with catch-all to handle ArgumentOutOfRangeException from budget validation - Add regression test for repeated failures not duplicating spooled events - Strengthen ordering test to verify spooled events precede queued events Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…fety Widen exception filters in TelemetrySpool.Append, DrainAll, Clear, and Trim from enumerated filters to catch-all to prevent ArgumentException and other rare path-validation exceptions from escaping. This is the third instance of enumerated filters missing exception categories in this namespace. Move DrainAll call in TelemetryClient into try block as belt-and-braces, even though TelemetrySpool now catches broadly — additional defensive layer since pathological config paths can produce unexpected exceptions. Add regression test with structurally invalid path (NUL character) to verify graceful degradation rather than propagation. Fixes: escape of ArgumentException on bad path → SIGABRT under NativeAOT Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…catches
Rename Structurally_invalid_path test to Unusable_path_degrades_on_append to
accurately reflect what it tests: File.Exists swallows ArgumentException
internally and returns false, so DrainAll and Clear don't actually exercise
their catches on a NUL path. Only Append throws because Path.GetDirectoryName
returns "" and Directory.CreateDirectory("") throws ArgumentException.
Update comments on DrainAll and Clear catches to document that they are
defence-in-depth for theoretically reachable exceptions (PathTooLongException,
NotSupportedException from pathological KCAP_CONFIG_DIR), but deterministically
triggering read/delete failures across platforms requires filesystem states
a unit test can't reliably create, so these are not unit-tested.
Preserves the production fix (broad catches remain in place) while documenting
the testing boundary honestly rather than overstating coverage.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ollection expressions Verified by publish: both CliTelemetry.cs:110 (string) and PostHogPayload.cs:48 (JsonObject) hit the RequiresDynamicCode generic overload. JsonValue.Create(x) does not help -- exact-type betterness still prefers Add<T>. Only a JsonNode? static type selects the non-generic overload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
JsonArray.Add<T>(T) binds whenever the argument's static type is narrower than JsonNode?, so neither avoiding collection expressions nor JsonValue.Create(f) alone was sufficient — only a JsonNode?-typed local selects the non-generic, AOT-safe Add(JsonNode?) overload. Fixes both the flags-array site in CliTelemetry and the batch-entry site in PostHogPayload (task 4), verified with a real `dotnet publish -c Release` producing zero IL2026/IL3050 output. Also re-targets Denylisted_commands_emit_nothing at a reportable Initialize command so RecordCommand's own IsReportable guard is what the test exercises, rather than being short-circuited by Initialize's Enabled=false — load-bearing once Task 10's long-lived MCP server process initializes once and calls RecordCommand per invocation with varying command strings.
…return RunWithLiveAuthAsync does signin, enumeration and provisioning internally, so anchoring on its return puts signin_completed after tenant_none (breaking any ordered funnel) and keys signin_failed on an ExitCode that is non-zero for declined offers, provisioning failures and the retarget path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r/deferred split Anchors WorkOS signin_completed/signin_failed inside WorkOSDiscovery.RunAsync at the point live auth actually succeeds or fails, instead of on RunWithLiveAuthAsync's overall ExitCode — which put signin_completed after tenant_none/workspace_provisioned in an ordered funnel and mislabelled declined offers, provisioning failures, and the deliberate retarget path as sign-in failures. Also: gives the "I already have a workspace" redirect its own cli_setup_workspace_redirected terminal event instead of pooling into "workspace_offered" with no resolution; moves the provisioning/poll outcome events to the batched Capture path now that WorkspaceRequested means the user is committed and doesn't need an eager flush blocking Spectre's live display; corrects the WorkOS signin_opened mode label (always "browser", never "device"); and tightens three tests that could pass regardless of correctness (unordered sequence assertion, an all-true Started() call, and a collision loop with no count assertion), plus a new WorkOSDiscovery call-site test that would have caught the signin-anchor defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
McpFlowResultServer was missed. Also pins the server label for each file, including the three internal servers with no KcapMcpServers registry entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…context MCP servers auto-spawn per agent session, so on a fresh machine one is plausibly the first kcap process ever run. It would print the once-per-device notice to a stderr no human reads and consume the marker -- silently reproducing the rejected silent-by-default posture. McpReviewContextServer also short-circuits before the ProcessExit flush registration and never reaches the 20-call periodic flush in practice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tice, weak tests - McpReviewContextServer is spawned via Program.cs's early short-circuit and never reaches the ProcessExit-registered flush, so wrap its loop in try/finally and flush on exit — otherwise its telemetry rarely reaches the 20-call periodic threshold and is functionally dead. - CliTelemetry.Initialize no longer runs NoticeAndFirstRun() for the "mcp-server" pseudo-command: an agent-spawned MCP server's stderr is unwatched, and on a fresh machine it can plausibly be the first kcap-family process ever run, which would silently consume the once-per-device first-run notice before any human ever sees it. - McpTelemetryTests: add SafeToolName coverage for missing/wrong-shaped params and missing/non-string name (the defensive paths had no test), and invert No_argument_data_is_carried from a three-name denylist to an allowlist of the only properties an mcp_tool_called event may carry, so a leak under any other key now fails. - CliTelemetryTests: add a regression test pinning that "mcp-server" cannot consume the first-run notice and that the next human-invoked command still gets it.
Covers the composition of TryApplyTelemetry and Set that the per-unit tests can't: a missing `return 0;` after the telemetry branch would persist the flag and then fall through into ApplySet, which throws "Unknown config key" after the opt-out already silently took effect. Verified by temporarily removing that return and confirming this test fails with exactly that trace, then restoring it.
ConfigSetTelemetryCompositionTests set TelemetryState.PathOverride in [Before(Test)] but locked under the TokenStoreProfileTests key (for the shared config dir it genuinely uses), not the dedicated TelemetryState.PathOverride key every other PathOverride-mutating class shares. Under local (non-CI) parallelism this could race TelemetryStateTests/SetupFunnelTests/CliTelemetryTests/ McpTelemetryTests/ConfigTelemetryKeyTests — invisible in CI, which runs --maximum-parallel-tests 1. Drop the PathOverride mutation instead of adding a second lock key: the module initializer already pins KCAP_CONFIG_DIR to the shared test directory before PathHelpers.ConfigDir captures it, so TelemetryState's default path already resolves inside it. Clean up the telemetry.json this leaves behind in [After(Test)] so it can't leak into a later test reading persisted telemetry state.
Adds a Telemetry section under the config command area covering the three opt-outs (kcap config set telemetry off, KCAP_TELEMETRY, DO_NOT_TRACK), the KCAP_TELEMETRY-outranks-DO_NOT_TRACK-in-both-directions precedence surprise, what is/isn't collected, and kcap config show reporting the effective state. Adds the telemetry key to help-config.txt and a one-line pointer in Getting started, per the repo's standing README-sync rule.
Its own code says 'No backend URL or auth here -- never any'. Instrumenting it wrote telemetry.json into the config dir it has no authority over, and the flush added an outbound POST to phog.kurrent.io from a sidecar designed to reach only its 127.0.0.1 capability URL -- which matters under borrowed review's (deny default) sandbox. Caught by Daemon_context_mode_starts_without_backend_and_performs_one_exact_get. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kcap mcp review under KCAP_REVIEW_CONTEXT_MODE has one job: reach a single 127.0.0.1 capability URL and touch nothing else. Instrumenting it broke that two ways at once — a persisted device id written into a config dir it has no authority over, and an outbound POST to the analytics endpoint from a process whose whole point is that borrowed review can run it under a sandbox with no other egress. Verified by two clean integration-suite runs (the previously failing Daemon_context_mode_starts_without_backend_and_performs_one_exact_get now passes both times) and by --treenode-filter runs confirming the other eight instrumented MCP servers are unaffected.
…r are The README, first-run notice, and design spec all said telemetry "never records arguments" or "never collected: argv values" — but CommandEvents.Flags puts flag names (e.g. --no-prompt, --skip-codex-hooks) into the cli_command payload; only their values are stripped. Reworded all three (plus the spec's verbatim quote of the notice) to say what's actually true: command and flag names are collected, argument values never are. help-config.txt's telemetry line makes no such claim and needed no change.
The Privacy section listed only exclusions, so a reader could not tell from it that the SaaS workspace slug is deliberately collected. Names the device id, the org slug (SaaS only), and the environment-shape properties. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- $geoip_disable: true alongside $ip: null — the latter alone does not suppress PostHog's GeoIP enrichment, which falls back to the connecting IP whenever $ip is falsy. - Allowlist known verbs for the cli_command `command` property and report "unknown" for anything else, so a fumbled GUID/path/URL passed as args[0] never reaches PostHog verbatim. - Denylist `uninstall`: the ProcessExit telemetry flush's spool write would otherwise resurrect the config directory uninstall just deleted. - README: state positively what identifies an installation (device id, and the workspace slug for SaaS), not just what's excluded. - Gate the `logged_in` TokenStore read on IsReportable so `hook` (thousands of invocations/day) skips a disk read whose result is never sent. - Correct the design spec: the $geoip finding, the missing cli_setup_workspace_redirected catalog entry and drop-off exclusion, and the spool bound (2000 events, not ~256KB). - TelemetrySpool.Clear's catch comment now states the real failure mode (duplicates on next drain, not lost events). - CommandTimingTests: add a no-sleep near-zero assertion that a hardcoded-constant stub would fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The plan's own open-question list flagged this as 'confirm rather than assume' and nobody confirmed it. The final review found it does not hold: $geoip_disable is the documented switch, and without it every event carried coordinates derived from the user's real IP. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR Summary by QodoAdd anonymous CLI telemetry to close the setup signup-funnel measurement gap
AI Description
Diagram
High-Level Assessment
Files changed (42)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1acd30838
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { loggedIn = await TokenStore.LoadAsync() is not null; } catch { } | ||
| } | ||
|
|
||
| CliTelemetry.Initialize(command, baseUrl, loggedIn); |
There was a problem hiding this comment.
Handle the opt-out command before initializing telemetry
On a fresh installation, running the documented kcap config set telemetry off command initializes telemetry while the persisted setting is still absent, which mints a device ID and queues cli_first_run; the handler then persists false, but CliTelemetry.Enabled and its client remain active, so ProcessExit also queues the command and flushes both events. This makes the explicit opt-out command itself transmit telemetry and violates the stated invariant that opting out before first run must not create an analytics identifier.
Useful? React with 👍 / 👎.
| _deviceId = TelemetryState.GetOrCreateDeviceId(); | ||
| if (_deviceId is null) { Enabled = false; return; } |
There was a problem hiding this comment.
Honor KCAP_TELEMETRY when persisted telemetry is off
When a user previously persisted telemetry = off and later runs with KCAP_TELEMETRY=1, Resolve correctly returns enabled, but GetOrCreateDeviceId() independently sees TelemetryStateFile.Enabled == false and returns null; this line then disables the facade. Consequently the documented higher-precedence environment variable cannot opt back in, even if the state file already contains a device ID.
Useful? React with 👍 / 👎.
| try { | ||
| var response = await DispatchToolCallAsync(callId, callRequest); | ||
| ok = true; | ||
| return response; |
There was a problem hiding this comment.
Derive MCP success from the returned tool result
For an invalid server URL, an unknown tool, an authentication failure, or an exception caught by DispatchToolCallAsync, dispatch returns a normal JSON-RPC tool result with isError: true; it does not throw. Setting ok = true merely because dispatch returned therefore records these failed calls as successful, and because dispatch catches its own exceptions the false outcome is effectively unreachable. The same wrapper pattern is copied across the instrumented MCP servers, so their success-rate telemetry is systematically incorrect.
Useful? React with 👍 / 👎.
| var discovery = new TenantDiscovery(proxyClient, new SpectreTenantPicker()); | ||
| var outcome = await discovery.RunAsync(AuthProxyEndpoint.Url, ghToken); | ||
|
|
||
| if (outcome.Tenants.Length == 0) SetupFunnel.TenantNone(AuthProvider.GitHubApp); |
There was a problem hiding this comment.
Exclude discovery errors from the no-tenant event
When GitHub tenant discovery fails because the proxy is unreachable, the token is rejected, or the upstream service errors, TenantDiscovery.RunAsync returns an empty tenant array together with an error message. This condition emits cli_setup_tenant_none before checking that error, so transient and authentication failures inflate the key “authenticated but has no tenant” denominator; emit it only for the specific successful zero-tenant outcome.
Useful? React with 👍 / 👎.
Code Review by Qodo
1.
|
- Opt-out (`config set telemetry off`) no longer mints a device id or queues cli_first_run for itself: Program.cs pre-applies the flag before CliTelemetry.Initialize runs, and TryApplyTelemetry tears down an already-live facade (CliTelemetry.DiscardAndDisable) for the KCAP_TELEMETRY=1-overrides-persisted-off case. SetEnabled(false) now also deletes the on-disk device id. - KCAP_TELEMETRY=1 can now override a persisted opt-out: GetOrCreateDeviceId no longer re-decides precedence itself. - MCP tool-call telemetry now reads ok from the dispatched result's isError flag (McpTelemetry.ResponseOk) instead of assuming success whenever dispatch returns, across all 8 instrumented MCP servers. - GitHub tenant discovery distinguishes a genuine zero-tenant outcome from a discovery-service failure via DiscoveryOutcome.NoTenantsFound, so cli_setup_tenant_none no longer counts proxy/token/upstream errors. - TelemetryState writes atomically (temp file + rename) so an unlocked concurrent Read() can no longer observe a torn write and silently re-enable telemetry. - TelemetryClient.FlushAsync now times the whole call, not just the HTTP phase, and skips the POST entirely once the budget is already spent draining the spool and building the payload. Addresses automated review feedback on #501. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The temp-file test guards litter from the new temp-then-rename write, which is a real new risk. It does not demonstrate atomicity and would have passed against File.WriteAllText too -- say so, rather than let the comment imply coverage the test does not have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — worked through all six findings. Five were real and are fixed in Fixed
Diagnosis right, remedy wrong
For the record, the equivalent WorkOS path was checked and already correct — |
Closes #500 · AI-1824
Why
We can see a visitor copy
npm install -g @kurrent/kcap && kcap setup, and we can see a workspace appear on the server. We cannot see anything in between — including the person who runskcap setup, signs in, finds they have no workspace, is offered one, and quits. That population is the whole point of the signup funnel and it was invisible from both ends.This adds CLI-side telemetry. The drop-off measure is
cli_setup_tenant_noneminuscli_setup_workspace_provisioned, split by last step reached.Design decisions worth knowing
Direct to
phog.kurrent.io, not via the user's server. During the segment this exists to measure there is no server — noserver_url, no tenant, no token. Anything routed through the server cannot observe the pre-server funnel.The CLI person stays anonymous. An anonymous device id in
telemetry.json, deliberately separate frommachine.json(which is an auth identifier sent to the server). Where a workspace is known, events join the server's existingorganizationgroup — but SaaS only, because the Helm chart guaranteesTenant__Name == slugfor{slug}.kcap.aiand nothing guarantees it self-hosted. Deriving a group there would look joined and not be.Funnel steps flush eagerly, mid-command. The cohort being measured abandons setup and never runs
kcapagain, so anything deferred to a later invocation is lost. Everything else flushes once from aProcessExithandler under a 1.5s budget; a failed flush spills to a bounded drop-oldest spool that the next successful flush replays.Hooks emit nothing.
kcap hookruns on every tool use of every recorded session, inline in the agent's critical path. MCP is instrumented per tool call instead, which is where recap and memory usage actually shows up.Opt-out
KCAP_TELEMETRY>DO_NOT_TRACK>kcap config set telemetry off> enabled.KCAP_TELEMETRYoutranksDO_NOT_TRACKin both directions — it is the kcap-specific deliberate statement and the only way someone with a blanketDO_NOT_TRACKcan opt back in. That will surprise people, so it is documented explicitly.kcap config showreports the effective state and which setting decided it. A one-time notice prints to stderr on first run.The
telemetrykey is machine-scoped, not profile-scoped — a profile switch silently re-enabling reporting would be a dark pattern.Where to look first
CommandEvents.cs— the redaction boundary. Subcommands and verbs come from allowlists; flag names pass a shape rule whose 37-char bound is load-bearing (a GUID token is 38, the longest real flag is 31, and both edges are pinned by tests).PostHogPayload.OrgGroup— the SaaS-only group derivation.WorkOSDiscovery.cs— the signin events are anchored inside discovery, not on its return, becauseRunWithLiveAuthAsyncdoes signin, enumeration and provisioning internally and its exit code is non-zero for declined offers and the retarget path.Notes for the reviewer
McpReviewContextServeris deliberately uninstrumented. Its own code says "No backend URL or auth here — never any"; telemetry made it write config it has no authority over and POST to PostHog from a sidecar that runs under a(deny default)sandbox. An integration test enforces this.main. 54 failures here, 52 at merge-base5e84bed2d, with the set churning in both directions.AgentOrchestratorVendorTestsin isolation fails 3/207 here and 5/207 at merge-base — worse onmain. All are timing-sensitive orchestrator/PTY/teardown tests in files this branch never touches. See AI-1815.kcap-web: the privacy policy describes web and server collection only and needs a CLI paragraph.🤖 Generated with Claude Code