Skip to content

CLI telemetry and the signup-funnel measurement gap - #501

Open
alexeyzimarev wants to merge 46 commits into
mainfrom
cli-telemetry-signup-funnel
Open

CLI telemetry and the signup-funnel measurement gap#501
alexeyzimarev wants to merge 46 commits into
mainfrom
cli-telemetry-signup-funnel

Conversation

@alexeyzimarev

Copy link
Copy Markdown
Member

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 runs kcap 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_none minus cli_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 — no server_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 from machine.json (which is an auth identifier sent to the server). Where a workspace is known, events join the server's existing organization group — but SaaS only, because the Helm chart guarantees Tenant__Name == slug for {slug}.kcap.ai and 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 kcap again, so anything deferred to a later invocation is lost. Everything else flushes once from a ProcessExit handler 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 hook runs 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_TELEMETRY outranks DO_NOT_TRACK in both directions — it is the kcap-specific deliberate statement and the only way someone with a blanket DO_NOT_TRACK can opt back in. That will surprise people, so it is documented explicitly. kcap config show reports the effective state and which setting decided it. A one-time notice prints to stderr on first run.

The telemetry key 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, because RunWithLiveAuthAsync does signin, enumeration and provisioning internally and its exit code is non-zero for declined offers and the retarget path.

Notes for the reviewer

  • McpReviewContextServer is 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.
  • The unit suite is red on this branch and on main. 54 failures here, 52 at merge-base 5e84bed2d, with the set churning in both directions. AgentOrchestratorVendorTests in isolation fails 3/207 here and 5/207 at merge-base — worse on main. All are timing-sensitive orchestrator/PTY/teardown tests in files this branch never touches. See AI-1815.
  • Companion change needed in kcap-web: the privacy policy describes web and server collection only and needs a CLI paragraph.

🤖 Generated with Claude Code

alexeyzimarev and others added 30 commits August 8, 2026 12:10
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>
alexeyzimarev and others added 14 commits August 8, 2026 18:13
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>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add anonymous CLI telemetry to close the setup signup-funnel measurement gap

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Add opt-outable CLI telemetry shipped directly to PostHog with strict redaction rules.
• Instrument kcap setup funnel steps and MCP tool calls, excluding hooks and machine-driven verbs.
• Add durable device state + bounded spool, plus extensive unit tests and documentation.
Diagram

graph TD
  A["src/Capacitor.Cli/Program.cs"] --> B["CliTelemetry facade"] --> C["TelemetryClient"] --> D{{"PostHog ingest\nphog.kurrent.io"}}
  B --> E[("telemetry.json\n(device+consent)")]
  C --> F[("telemetry-spool.jsonl\n(drop-oldest)")]
  G["Setup flow\n(SetupCommand/WorkOSDiscovery/Provisioner)"] --> H["SetupFunnel events"] --> B
  I["MCP servers\n(Mcp*Server)"] --> J["McpTelemetry"] --> B
  subgraph Legend
    direction LR
    _app["Process/module"] ~~~ _file[("On-disk file")] ~~~ _ext{{"External service"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Route telemetry via the user’s Capacitor server
  • ➕ Centralizes telemetry and avoids embedding an ingest token
  • ➕ Could join CLI events to server identity more directly
  • ➖ Cannot observe the pre-server signup funnel (no server_url/tenant/token yet)
  • ➖ Adds server availability/latency as a dependency on CLI command paths
2. Adopt OpenTelemetry + collector/exporter to PostHog
  • ➕ Standardizes instrumentation and could unify future metrics/traces/logs
  • ➕ Potentially better tooling for sampling/aggregation
  • ➖ Heavier dependency and complexity for a NativeAOT CLI
  • ➖ Still needs strong redaction boundaries; doesn’t reduce privacy risk by itself
3. Make telemetry opt-in only
  • ➕ Lower surprise factor for privacy-sensitive users
  • ➕ Reduces default data collection
  • ➖ Biases funnel measurement hardest where drop-off is highest (setup abandoners)
  • ➖ Undercuts the primary goal of closing the measurement gap

Recommendation: The PR’s approach (direct-to-PostHog, anonymous device id, SaaS-only org grouping, eager flush for funnel steps, and strict allow-by-exception redaction) is the best fit for measuring the pre-server funnel without increasing command-path risk. The key tradeoffs (embedded public ingest token and default-on) are mitigated by explicit first-run notice, layered opt-out precedence, and comprehensive tests around redaction and persistence.

Files changed (42) +5389 / -11

Enhancement (24) +1214 / -10
WorkOSDiscovery.csEmit setup funnel sign-in and tenant-none telemetry for WorkOS +15/-0

Emit setup funnel sign-in and tenant-none telemetry for WorkOS

• Adds 'SetupFunnel.SigninCompleted'/'SigninFailed' at the correct boundary (auth result), and emits 'TenantNone' before the headless/provisioner fork so the funnel denominator is accurate.

src/Capacitor.Cli.Core/Auth/WorkOSDiscovery.cs

CliTelemetry.csAdd never-throw telemetry facade with shared properties and flush behavior +174/-0

Add never-throw telemetry facade with shared properties and flush behavior

• Introduces the single telemetry surface used by call sites, including initialization, event capture, eager capture for funnel steps, and a ProcessExit flush under a 1.5s budget. Implements first-run disclosure, debug logging, device id acquisition, and SaaS-only org grouping.

src/Capacitor.Cli.Core/Telemetry/CliTelemetry.cs

CommandEvents.csImplement redaction boundary for reportable commands, subcommands, and flags +98/-0

Implement redaction boundary for reportable commands, subcommands, and flags

• Defines denylisted machine-driven verbs, known verb allowlist, per-verb subcommand allowlists, and a shape+length rule for safe flag-name capture. Ensures unknown or sensitive argv tokens are dropped or normalized to prevent leaking paths/URLs/IDs.

src/Capacitor.Cli.Core/Telemetry/CommandEvents.cs

CommandTiming.csAdd stopwatch-based duration helper with non-negative clamp +14/-0

Add stopwatch-based duration helper with non-negative clamp

• Provides a small utility to compute wall-clock duration from 'Stopwatch' ticks and clamp negative results to zero.

src/Capacitor.Cli.Core/Telemetry/CommandTiming.cs

McpTelemetry.csAdd per-tool-call MCP telemetry helper with safe tool-name parsing +46/-0

Add per-tool-call MCP telemetry helper with safe tool-name parsing

• Implements MCP tool-call event capture ('mcp_tool_called') and periodic flushing for long-lived MCP servers. Adds defensive JSON parsing to avoid exceptions on malformed requests.

src/Capacitor.Cli.Core/Telemetry/McpTelemetry.cs

PostHogPayload.csBuild PostHog /batch payload with GeoIP suppression and SaaS org grouping +75/-0

Build PostHog /batch payload with GeoIP suppression and SaaS org grouping

• Constructs the JSON payload for PostHog batch ingestion, injects 'distinct_id', and disables GeoIP enrichment ('$ip: null' + '$geoip_disable: true'). Derives and attaches 'organization' group + 'org' property only for '*.kcap.ai' server URLs.

src/Capacitor.Cli.Core/Telemetry/PostHogPayload.cs

SetupFunnel.csDefine setup signup-funnel event API with eager flushing +77/-0

Define setup signup-funnel event API with eager flushing

• Adds strongly-named funnel events for 'kcap setup' (signin, tenant-none, workspace offer/request/outcome, and success). Flushes most steps immediately to capture abandonment, while deferring terminal provisioning outcomes to avoid blocking UI callbacks.

src/Capacitor.Cli.Core/Telemetry/SetupFunnel.cs

TelemetryClient.csAdd budgeted batch sender with spill-to-spool failure handling +68/-0

Add budgeted batch sender with spill-to-spool failure handling

• Implements an in-memory queue and a flush method that posts queued + previously spooled events to PostHog within a time budget. On any failure, spills queued events to disk for replay rather than retrying inline.

src/Capacitor.Cli.Core/Telemetry/TelemetryClient.cs

TelemetryEvent.csAdd telemetry event record type +8/-0

Add telemetry event record type

• Introduces a simple event container with name, JsonObject properties, and timestamp, designed for AOT-safe serialization via JsonNode.

src/Capacitor.Cli.Core/Telemetry/TelemetryEvent.cs

TelemetrySettings.csAdd opt-out precedence resolution with reason reporting +56/-0

Add opt-out precedence resolution with reason reporting

• Implements deterministic precedence for telemetry enablement: 'KCAP_TELEMETRY' > 'DO_NOT_TRACK' > persisted config > default-on. Returns both effective value and the source that decided it for user-facing diagnostics.

src/Capacitor.Cli.Core/Telemetry/TelemetrySettings.cs

TelemetrySpool.csAdd bounded on-disk spool for failed flushes +102/-0

Add bounded on-disk spool for failed flushes

• Implements append-only JSONL spooling with a maximum event cap and drop-oldest trimming. Provides drain and clear operations and broad exception swallowing to meet the never-throw constraint under NativeAOT.

src/Capacitor.Cli.Core/Telemetry/TelemetrySpool.cs

TelemetryState.csPersist device id, enable flag, and first-run notice marker safely +138/-0

Persist device id, enable flag, and first-run notice marker safely

• Adds 'telemetry.json' state management including cross-process locking, read-modify-write updates, and best-effort fallbacks when locking fails. Ensures no device id is minted while telemetry is disabled and that corruption degrades to defaults.

src/Capacitor.Cli.Core/Telemetry/TelemetryState.cs

ConfigCommand.csAdd machine-wide telemetry config key and show effective decision +42/-0

Add machine-wide telemetry config key and show effective decision

• Implements 'kcap config set telemetry on/off' as a machine-scoped setting stored outside profiles. Updates 'config show' to print effective telemetry state plus the deciding source (env vs config vs default).

src/Capacitor.Cli/Commands/ConfigCommand.cs

McpAnalyticsServer.csInstrument MCP tool calls and initialize telemetry for long-lived server +27/-1

Instrument MCP tool calls and initialize telemetry for long-lived server

• Initializes telemetry under the pseudo-command 'mcp-server' and wraps 'tools/call' dispatch to measure per-tool duration and success. Uses safe tool-name extraction and periodic flushing via 'McpTelemetry'.

src/Capacitor.Cli/Commands/McpAnalyticsServer.cs

McpFlowResultServer.csInstrument MCP flow-result server tool calls +27/-1

Instrument MCP flow-result server tool calls

• Adds telemetry initialization and per-tool-call timing/reporting for the flow-result MCP server by wrapping 'tools/call' dispatch.

src/Capacitor.Cli/Commands/McpFlowResultServer.cs

McpFlowsServer.csInstrument MCP flows server tool calls +27/-1

Instrument MCP flows server tool calls

• Adds telemetry initialization and per-tool-call timing/reporting for the flows MCP server by wrapping 'tools/call' dispatch.

src/Capacitor.Cli/Commands/McpFlowsServer.cs

McpJudgeServer.csInstrument MCP judge server tool calls +28/-1

Instrument MCP judge server tool calls

• Adds telemetry initialization and wraps tool-call handling with timing and success/failure reporting for the judge MCP server.

src/Capacitor.Cli/Commands/McpJudgeServer.cs

McpMemoryServer.csInstrument MCP memory server tool calls +27/-1

Instrument MCP memory server tool calls

• Adds telemetry initialization and per-tool-call timing/reporting for the memory MCP server by wrapping 'tools/call' dispatch.

src/Capacitor.Cli/Commands/McpMemoryServer.cs

McpReviewServer.csInstrument MCP review server tool calls +28/-1

Instrument MCP review server tool calls

• Adds telemetry initialization and per-tool-call timing/reporting for the review MCP server by wrapping 'tools/call' dispatch.

src/Capacitor.Cli/Commands/McpReviewServer.cs

McpSessionsServer.csInstrument MCP sessions server tool calls +27/-1

Instrument MCP sessions server tool calls

• Adds telemetry initialization and per-tool-call timing/reporting for the sessions MCP server by wrapping 'tools/call' dispatch.

src/Capacitor.Cli/Commands/McpSessionsServer.cs

McpWorkItemsServer.csInstrument MCP workitems server tool calls +27/-1

Instrument MCP workitems server tool calls

• Adds telemetry initialization and per-tool-call timing/reporting for the workitems MCP server by wrapping 'tools/call' dispatch.

src/Capacitor.Cli/Commands/McpWorkItemsServer.cs

SetupCommand.csEmit setup funnel telemetry across setup flow and auth branches +36/-1

Emit setup funnel telemetry across setup flow and auth branches

• Adds funnel instrumentation at setup start, sign-in open/completion/failure, tenant-none detection, and success (including a count of configured agents). Ensures GitHub token denial and WorkOS/headless distinctions are captured without leaking user data.

src/Capacitor.Cli/Commands/SetupCommand.cs

SpectreTenantProvisioner.csInstrument workspace offer/request/outcome steps during provisioning +22/-1

Instrument workspace offer/request/outcome steps during provisioning

• Emits funnel events for workspace offered/declined/redirected/requested and provisioning outcomes (success/failure reasons), matching the signup-funnel measurement model.

src/Capacitor.Cli/Commands/SpectreTenantProvisioner.cs

Program.csInitialize telemetry, record cli_command, and flush on ProcessExit +25/-0

Initialize telemetry, record cli_command, and flush on ProcessExit

• Adds telemetry initialization after resolving server URL, captures 'logged_in' from local token presence, records command exit code and duration, and flushes under ProcessExit. Avoids work for denylisted commands and keeps telemetry failures from impacting command execution.

src/Capacitor.Cli/Program.cs

Tests (12) +1361 / -0
CliTelemetryTests.csAdd facade behavior tests (shared props, first-run notice, redaction) +161/-0

Add facade behavior tests (shared props, first-run notice, redaction)

• Covers capture merging, command event emission, denylist suppression, inert behavior before initialization, and first-run notice semantics (including the 'mcp-server' exception). Uses a test sink and isolated telemetry state paths.

test/Capacitor.Cli.Tests.Unit/Telemetry/CliTelemetryTests.cs

CommandEventsTests.csAdd tests for command/flag allowlists and redaction boundaries +155/-0

Add tests for command/flag allowlists and redaction boundaries

• Verifies denylisted verbs are not reportable, unknown verbs are redacted to 'unknown', subcommand allowlists prevent leaking positionals, and flag shape/length rules exclude GUIDs/paths/URLs. Confirms sorting, dedupe, and max flag count behavior.

test/Capacitor.Cli.Tests.Unit/Telemetry/CommandEventsTests.cs

CommandTimingTests.csAdd tests for duration measurement and non-negative clamp +35/-0

Add tests for duration measurement and non-negative clamp

• Validates elapsed time is derived from stopwatch ticks, never negative, and near-zero without sleep to prevent stubbed implementations.

test/Capacitor.Cli.Tests.Unit/Telemetry/CommandTimingTests.cs

ConfigSetTelemetryCompositionTests.csAdd composition tests for 'config set telemetry' integration +92/-0

Add composition tests for 'config set telemetry' integration

• Drives 'kcap config set telemetry off' through the real command handler to ensure the telemetry branch returns early and never mutates or creates profile config files. Includes cleanup patterns for shared config directory constraints.

test/Capacitor.Cli.Tests.Unit/Telemetry/ConfigSetTelemetryCompositionTests.cs

ConfigTelemetryKeyTests.csAdd unit tests for telemetry config key parsing and persistence +64/-0

Add unit tests for telemetry config key parsing and persistence

• Validates on/off parsing, persistence into telemetry state, non-claiming of other keys, and actionable error messages for invalid values. Ensures telemetry is not a profile-scoped config key.

test/Capacitor.Cli.Tests.Unit/Telemetry/ConfigTelemetryKeyTests.cs

McpTelemetryTests.csAdd tests for MCP tool-call telemetry and safe tool-name parsing +96/-0

Add tests for MCP tool-call telemetry and safe tool-name parsing

• Verifies per-call properties, failure reporting, and that no argument data leaks via unexpected properties. Tests defensive parsing paths for malformed JSON-RPC tool call payloads.

test/Capacitor.Cli.Tests.Unit/Telemetry/McpTelemetryTests.cs

PostHogPayloadTests.csAdd tests for PostHog batch payload and SaaS-only org grouping +111/-0

Add tests for PostHog batch payload and SaaS-only org grouping

• Validates payload structure, distinct_id injection, GeoIP suppression, and that org group/property are attached together only when provided. Confirms org slug derivation for '*.kcap.ai' and immutability of source events.

test/Capacitor.Cli.Tests.Unit/Telemetry/PostHogPayloadTests.cs

SetupFunnelTests.csAdd tests for setup funnel sequences and key call-site wiring +156/-0

Add tests for setup funnel sequences and key call-site wiring

• Asserts ordered funnel sequences for happy/abandoned/failure paths, ensures no event-name collisions with server events, and verifies WorkOS discovery emits signin_completed before tenant_none for zero-tenant runs.

test/Capacitor.Cli.Tests.Unit/Telemetry/SetupFunnelTests.cs

TelemetryClientTests.csAdd tests for flush behavior, spooling, and replay ordering +141/-0

Add tests for flush behavior, spooling, and replay ordering

• Verifies no-op flush, successful POST batching, spill-to-spool on HTTP/network failures, replay ordering (spooled before queued), and no duplication across repeated failures. Confirms org group reaches payload.

test/Capacitor.Cli.Tests.Unit/Telemetry/TelemetryClientTests.cs

TelemetrySettingsTests.csAdd tests for opt-out precedence and reason reporting +83/-0

Add tests for opt-out precedence and reason reporting

• Covers default-on behavior, KCAP_TELEMETRY and DO_NOT_TRACK precedence (including opt-back-in), persisted config fallback, ignored blanks, and reason string selection.

test/Capacitor.Cli.Tests.Unit/Telemetry/TelemetrySettingsTests.cs

TelemetrySpoolTests.csAdd tests for spool persistence, trimming, and never-throw behavior +117/-0

Add tests for spool persistence, trimming, and never-throw behavior

• Validates JSONL round-tripping, accumulation across instances, clearing, skipping corrupt/type-mismatched lines, drop-oldest trimming, and graceful degradation on unusable paths.

test/Capacitor.Cli.Tests.Unit/Telemetry/TelemetrySpoolTests.cs

TelemetryStateTests.csAdd tests for telemetry.json state (device id, enable flag, notice) +150/-0

Add tests for telemetry.json state (device id, enable flag, notice)

• Covers default reads, stable GUID generation, avoiding ID creation while disabled, persistence of enable + notice markers, non-rewrite behavior, and corrupt-file healing. Uses PathOverride with shared parallelization lock keys.

test/Capacitor.Cli.Tests.Unit/Telemetry/TelemetryStateTests.cs

Documentation (5) +2812 / -1
README.mdDocument new telemetry behavior and opt-out controls +29/-1

Document new telemetry behavior and opt-out controls

• Adds a Telemetry section describing what is collected, what is never collected, workspace grouping rules, and opt-out precedence. Updates the README table of contents and the product overview to reference telemetry.

README.md

2026-08-08-cli-telemetry-signup-funnel.mdAdd detailed implementation plan for CLI telemetry +2453/-0

Add detailed implementation plan for CLI telemetry

• Introduces a step-by-step plan covering settings resolution, persistence, payload construction, flushing/spooling strategy, instrumentation points, and AOT constraints. Serves as an execution checklist for implementing the feature safely.

docs/superpowers/plans/2026-08-08-cli-telemetry-signup-funnel.md

2026-08-08-cli-telemetry-signup-funnel-design.mdAdd design spec for CLI telemetry and signup funnel +320/-0

Add design spec for CLI telemetry and signup funnel

• Documents the measurement gap, design decisions (direct ingest, anonymous device id, SaaS-only org grouping), event catalog, privacy/redaction guarantees, and delivery strategy (eager funnel flush + exit flush).

docs/superpowers/specs/2026-08-08-cli-telemetry-signup-funnel-design.md

help-config.txtExpose telemetry config key in CLI help text +1/-0

Expose telemetry config key in CLI help text

• Adds 'telemetry' to the list of config keys with machine-wide semantics.

src/Capacitor.Cli.Core/Resources/help-config.txt

McpReviewContextServer.csDocument why review-context sidecar remains uninstrumented +9/-0

Document why review-context sidecar remains uninstrumented

• Adds a detailed comment explaining why telemetry must not be wired into this sidecar due to sandboxed egress and no-config-write guarantees, enforced by integration tests.

src/Capacitor.Cli/Commands/McpReviewContextServer.cs

Other (1) +2 / -0
Models.csRegister TelemetryStateFile for System.Text.Json source generation +2/-0

Register TelemetryStateFile for System.Text.Json source generation

• Adds 'TelemetryStateFile' to the JSON source-gen context so telemetry state serialization remains AOT-safe.

src/Capacitor.Cli.Core/Models.cs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Comment on lines +41 to +42
_deviceId = TelemetryState.GetOrCreateDeviceId();
if (_deviceId is null) { Enabled = false; return; }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Comment on lines +69 to +72
try {
var response = await DispatchToolCallAsync(callId, callRequest);
ok = true;
return response;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@qodo-code-review

qodo-code-review Bot commented Aug 9, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Flush budget excludes spool/serialization work ✓ Resolved 🐞 Bug ➹ Performance
Description
TelemetryClient.FlushAsync's budget parameter only bounds the HttpClient timeout and
CancellationTokenSource, but spool.DrainAll() (file read of up to 2000 lines) and
PostHogPayload.Build (JSON serialization of the whole pending batch) run before the timer starts, so
the documented 1.5s wall-clock budget is not actually enforced end-to-end. Since
CliTelemetry.CaptureNow and Program's ProcessExit handler block synchronously on FlushAndClose, slow
disk I/O on a large spool can stall setup steps or process exit well beyond the intended budget.
Code

src/Capacitor.Cli.Core/Telemetry/TelemetryClient.cs[R37-51]

+            var spooled = spool.DrainAll();
+            var pending = new List<TelemetryEvent>(spooled.Count + queued.Count);
+            pending.AddRange(spooled);   // spool first: previously-failed events keep their place in the funnel
+            pending.AddRange(queued);
+
+            if (pending.Count == 0) return true;
+
+            var body = PostHogPayload.Build(pending, token, distinctId, orgGroup);
+
+            using var http = new HttpClient(handler, disposeHandler: false) { Timeout = budget };
+            using var cts  = new CancellationTokenSource(budget);
+            using var content = new StringContent(body, Encoding.UTF8);
+            content.Headers.ContentType = new MediaTypeHeaderValue("application/json");
+
+            var response = await http.PostAsync($"{endpoint.TrimEnd('/')}/batch/", content, cts.Token);
Evidence
TelemetryClient.cs class doc states FlushAsync ships events 'under a wall-clock budget', but the
budget TimeSpan is only used to construct the CancellationTokenSource/HttpClient at lines 46-47,
after spool.DrainAll() (line 37) and PostHogPayload.Build (line 44) already ran unbounded.
CliTelemetry.CaptureNow (CliTelemetry.cs:101-104) is called synchronously from every SetupFunnel
eager event (SetupFunnel.cs Emit), and Program.cs's ProcessExit handler (Program.cs:100-103) also
blocks synchronously on FlushAndClose — both rely on the 1.5s FlushBudget to keep the command path
responsive, but that guarantee doesn't hold if DrainAll/Build are slow (e.g. large spool near the
2000-event cap).

src/Capacitor.Cli.Core/Telemetry/TelemetryClient.cs[28-51]
src/Capacitor.Cli.Core/Telemetry/CliTelemetry.cs[101-104]
src/Capacitor.Cli/Program.cs[100-103]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `budget` parameter to TelemetryClient.FlushAsync is documented as a wall-clock budget for the whole flush operation, but it is only applied to the HTTP request phase (HttpClient.Timeout / CancellationTokenSource). Reading the spool file and serializing the full batch happen first and are unbounded, so a large spool or slow disk can make the actual flush take much longer than the 1.5s budget, blocking the synchronous callers (CaptureNow, ProcessExit).

## Issue Context
CliTelemetry.CaptureNow is called synchronously by every eager SetupFunnel event, and Program.cs's ProcessExit handler blocks synchronously on FlushAndClose for every command — both depend on a bounded flush to avoid stalling the user.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Telemetry/TelemetryClient.cs[21-51]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Telemetry opt-out race enables telemetry ✓ Resolved 🐞 Bug ☼ Reliability
Description
TelemetryState.Read() reads telemetry.json without acquiring the ConfigFileLock used by mutations,
while writes use non-atomic File.WriteAllText (truncate+rewrite). A concurrent Read() during a write
can observe torn JSON, hit the catch clause, and return default(Enabled=null), which
TelemetrySettings.Resolve treats as "default → enabled", so a process can initialize telemetry as on
even though the user just ran kcap config set telemetry off.
Code

src/Capacitor.Cli.Core/Telemetry/TelemetryState.cs[R29-39]

+    public static TelemetryStateFile Read() {
+        var path = Path;
+        if (!File.Exists(path)) return default;
+
+        try {
+            var json = File.ReadAllText(path);
+            return JsonSerializer.Deserialize(json, CapacitorJsonContext.Default.TelemetryStateFile);
+        } catch (Exception e) when (e is JsonException or IOException or UnauthorizedAccessException) {
+            return default;   // corrupt or transiently locked → defaults, never throw
+        }
+    }
Evidence
Read() (TelemetryState.cs:29-39) is not guarded by ConfigFileLock.Acquire, unlike Mutate()
(TelemetryState.cs:79-99) which acquires the lock for every write. WriteLocked
(TelemetryState.cs:130-137) uses File.WriteAllText, which truncates then rewrites — not an atomic
rename — so a concurrent unlocked Read can see a partial/corrupt file, hit the catch and return
default (Enabled=null). CliTelemetry.Initialize (CliTelemetry.cs:34-38) feeds
TelemetryState.PersistedEnabled() into TelemetrySettings.Resolve, which for persisted=null returns
Enabled=true (TelemetrySettings.cs:23-25, 'default' reason) — i.e. a transient read race right after
config set telemetry off can cause a concurrently-running kcap process to send telemetry despite
the opt-out.

src/Capacitor.Cli.Core/Telemetry/TelemetryState.cs[29-39]
src/Capacitor.Cli.Core/Telemetry/TelemetryState.cs[79-137]
src/Capacitor.Cli.Core/Telemetry/TelemetrySettings.cs[17-25]
src/Capacitor.Cli.Core/Telemetry/CliTelemetry.cs[34-42]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
TelemetryState.Read() does not take the same ConfigFileLock that TelemetryState.Mutate() uses, and writes are done via File.WriteAllText which is not atomic. A reader can observe a torn/partial file mid-write, treat it as corrupt, and fall back to defaults (Enabled=null), which TelemetrySettings.Resolve interprets as telemetry-enabled-by-default — silently bypassing a just-applied opt-out for the racing process.

## Issue Context
TelemetryState.json is the machine-scoped persisted telemetry consent flag. CliTelemetry.Initialize() calls TelemetryState.PersistedEnabled() (which calls Read()) on every command startup, while ConfigCommand's `telemetry` key setter calls TelemetryState.SetEnabled(), which goes through the locked Mutate()/WriteLocked() path using File.WriteAllText.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Telemetry/TelemetryState.cs[29-39]
- src/Capacitor.Cli.Core/Telemetry/TelemetryState.cs[130-137]
- src/Capacitor.Cli.Core/Telemetry/TelemetryState.cs[79-99]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread src/Capacitor.Cli.Core/Telemetry/TelemetryState.cs
Comment thread src/Capacitor.Cli.Core/Telemetry/TelemetryClient.cs
alexeyzimarev and others added 2 commits August 9, 2026 12:36
- 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>
@alexeyzimarev

Copy link
Copy Markdown
Member Author

Thanks — worked through all six findings. Five were real and are fixed in 804f777d; one had a correct diagnosis but an incorrect remedy.

Fixed

  • P1, opt-out transmitted telemetry. Confirmed: on a fresh machine kcap config set telemetry off initialised telemetry before persisting false, so it minted a device id, printed the notice, and flushed cli_first_run + cli_command. The documented opt-out phoned home. Now short-circuited before Initialize, with DiscardAndDisable() covering the case where a live facade must be torn down mid-command, and SetEnabled(false) deletes the device id — which also closes a known gap, since the spec justified a file separate from machine.json on the grounds that opt-out could delete the id outright.
  • P2, KCAP_TELEMETRY=1 couldn't override a persisted off. GetOrCreateDeviceId was re-deciding precedence that TelemetrySettings.Resolve already owns. Guard removed; Initialize's gate remains the single enforcement point.
  • P2, MCP ok always true. Correct — dispatch catches its own exceptions and returns isError: true, so the false branch was unreachable and success rates were systematically wrong. Now derived from the result.
  • Torn read could re-enable telemetry. Fixed by making the write atomic (temp + File.Move(overwrite: true)) rather than locking every read, which would put a cross-process mutex on every command startup.
  • Flush budget bounded only the HTTP phase. Now timed from entry, with the remaining budget passed to the POST and an exhausted budget spilling to the spool instead of starting a doomed request.

Diagnosis right, remedy wrong

  • GitHub tenant_none counting discovery failures is real, but the suggested fix — gating on ErrorMessage — would have suppressed the event entirely: TenantDiscovery.RunAsync:29-31 sets ErrorMessage for the genuine zero-tenant case too. Fixed instead by adding a discriminator to DiscoveryOutcome so only the real zero-tenant outcome emits.

For the record, the equivalent WorkOS path was checked and already correct — WorkOSDiscovery.cs:81-90 returns early on DiscoveryError, before the zero-tenant check — so the primary funnel denominator was never affected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI telemetry to close the signup-funnel measurement gap

1 participant