Skip to content

Fix two CLI telemetry signals that could not report what they claimed - #531

Merged
alexeyzimarev merged 1 commit into
mainfrom
telemetry-funnel-signal-fixes
Aug 11, 2026
Merged

alexeyzimarev merged 1 commit into
mainfrom
telemetry-funnel-signal-fixes

Conversation

@alexeyzimarev

Copy link
Copy Markdown
Member

Closes #530. Linear auto-imports the GitHub issue, so the imported AI-* issue links back to this PR.

Follow-up to #501 (CLI telemetry and the signup-funnel measurement gap), from reading the first day of live data.

Problem

Two setup-funnel properties cannot report what they claim.

has_existing_profile is true by construction. SetupCommand asked LoadProfileConfig().Profiles.Count > 0, but that method synthesizes a default entry whenever config.json is missing (AppConfig.cs:273-274), so the count is >= 1 on a machine that has never run kcap. It reported true for 14/14 recorded runs. The funnel therefore cannot separate first-time onboarding from a re-run — the split it exists to measure.

is_ci misses most CI providers. Detection checked only CI and GITHUB_ACTIONS; Jenkins, TeamCity and Azure Pipelines export none of those and were counted as human machines. The inverse was broken too — !string.IsNullOrEmpty(...) treats the common opt-out CI=false as CI, tagging exactly the machines asking not to be.

Fix

AppConfig.HasConfiguredProfile(ProfileConfig) defines "configured" as a profile carrying a server URL — what setup actually persists — and SetupCommand calls it instead of the count test.

New TelemetryEnvironment covers 17 CI providers plus the generic flag, presence-based per provider but value-aware for CI (so CI=false opts out). It is pure over an injected environment, the same seam TelemetrySettings uses, so the provider table is testable without mutating the process env.

Also: build_channel

is_ci does not cover the largest source of non-human devices in practice. Local dev loops and Aspire-spawned dev daemons run prerelease builds against throwaway KCAP_CONFIG_DIRs, minting a device id per run while legitimately reporting is_ci=false — they outnumbered real released-build devices 107 to 26 in the first day. Every event now carries build_channel (release / prerelease / unknown), so excluding them is a normal property filter rather than a cli_version NOT LIKE '%alpha%' string match every future insight has to remember.

McpTelemetryTests' property allowlist gains build_channel — that guard doing its job is precisely why a new shared property has to be a deliberate decision.

Tests

TDD throughout: tests written first, watched fail for the right reason, then implemented.

  • AppConfigHasConfiguredProfileTests (5): the synthesized-default case that caused the bug, blank/whitespace server URL, a configured non-active profile, no profiles at all.
  • TelemetryEnvironmentTests (18 + 8): one case per provider variable, CI=false/False/0 opting out, blank values, provider var set to "false" still counting, and build_channel including the trap where build metadata itself contains a hyphen (0.11.17+feature-branch.1 is a release).

Full unit suite: 57 pre-existing failures on both this branch and a clean origin/main baseline (stashed and re-ran to confirm) — the known-flaky PTY/timer/Codex-TOML classes. The one test this change broke, No_argument_data_is_carried, is the property allowlist and is fixed here. Two timing-sensitive tests differed run-to-run; both pass in isolation and touch neither telemetry nor config.

AOT publish clean — no IL2026/IL3050.

Verified against the real AOT binary:

Scenario Before After
Fresh config dir has_existing_profile: true false
Profile with server URL true true
TEAMCITY_VERSION=2024.1 is_ci: false true
CI=false is_ci: true false

Notes

No README change: no CLI surface changed, and build_channel is derived from the already-sent cli_version, so the privacy statement is unaffected. The event catalog in docs/superpowers/specs/2026-08-08-cli-telemetry-signup-funnel-design.md is updated with both corrections.

Existing has_existing_profile data (v0.11.16 through v0.11.18) should be treated as unusable rather than as evidence that everyone re-runs setup.

🤖 Generated with Claude Code

The first day of live setup-funnel data surfaced two properties that carry no
usable information. Both are measurement bugs, not collection bugs.

has_existing_profile was true by construction. SetupCommand asked
`LoadProfileConfig().Profiles.Count > 0`, but that method synthesizes a
`default` entry whenever config.json is missing, so the count is >= 1 on a
machine that has never run kcap. It reported true for 14/14 runs and could not
distinguish first-time onboarding from a re-run — the split the funnel exists
to measure. AppConfig.HasConfiguredProfile now defines "configured" as a
profile carrying a server URL, which is what setup actually persists.

is_ci missed most CI providers. Detection checked only CI and GITHUB_ACTIONS;
Jenkins, TeamCity and Azure Pipelines export none of those and were counted as
human machines. The inverse was broken too: a non-empty check treated the
common opt-out CI=false as CI, tagging exactly the machines asking not to be.
TelemetryEnvironment covers 17 providers plus the generic flag, and is pure
over an injected environment so the table is testable without mutating the
process env — the seam TelemetrySettings already uses.

Also adds build_channel (release/prerelease/unknown), because is_ci does not
cover the largest source of non-human devices in practice. Local dev loops and
Aspire-spawned dev daemons run prerelease builds against throwaway
KCAP_CONFIG_DIRs, minting a device id per run while legitimately reporting
is_ci=false; they outnumbered real released-build devices 107 to 26 in the
first day. The version string could express this, but only via a
`cli_version NOT LIKE '%alpha%'` match every future insight must remember.

McpTelemetryTests' property allowlist gains build_channel — that guard doing
its job is why the new shared property had to be a deliberate decision.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix CLI telemetry funnel signals and add build_channel property

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Fix has_existing_profile to reflect persisted server URL, not synthesized default profile.
• Expand CI detection across common providers and honor CI=false opt-out.
• Add build_channel shared telemetry property and update docs/tests allowlist.
Diagram

graph TD
  setup["SetupCommand"] --> funnel["SetupFunnel"] --> tel["CliTelemetry"] --> ph[("PostHog")]
  setup --> cfg["AppConfig"] --> prof["ProfileConfig"]
  tel --> tenv["TelemetryEnvironment"] --> env{{"Env vars"}}

  subgraph Legend
    direction LR
    _m["Module"] ~~~ _e{{"External"}} ~~~ _s[("Service")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use a dedicated CI-detection library/package
  • ➕ Potentially broader provider coverage maintained upstream
  • ➕ Reduces in-repo provider table maintenance
  • ➖ Adds dependency surface area for a small feature
  • ➖ Harder to keep value-aware semantics for CI=false opt-out consistent with product needs
2. Parse build channel via a SemVer library instead of string heuristics
  • ➕ More robust handling of edge-case version strings
  • ➕ Clearer intent via typed SemVer concepts
  • ➖ Extra dependency/complexity for a single property
  • ➖ Current rules (drop +metadata, hyphen => prerelease) are sufficient for stated version formats

Recommendation: The PR’s approach is appropriate: keep CI detection and build_channel derivation lightweight, value-aware where it matters (CI=false), and pure/testable via injected env dictionaries. If the provider list grows significantly or version formats diversify, revisit using a small SemVer/CI-info dependency.

Files changed (8) +257 / -17

Bug fix (4) +96 / -15
AppConfig.csAdd HasConfiguredProfile helper for real profile persistence +10/-0

Add HasConfiguredProfile helper for real profile persistence

• Introduces 'AppConfig.HasConfiguredProfile(ProfileConfig)' to detect whether any profile has a non-blank 'ServerUrl'. Avoids treating the loader’s synthesized 'default' profile as evidence of prior setup.

src/Capacitor.Cli.Core/Config/AppConfig.cs

CliTelemetry.csAdd build_channel and route CI detection through TelemetryEnvironment +11/-14

Add build_channel and route CI detection through TelemetryEnvironment

• Adds 'build_channel' to shared telemetry properties and computes it from the resolved CLI version. Replaces inline CI detection with 'TelemetryEnvironment.IsCi()' and removes the old simplistic 'CI || GITHUB_ACTIONS' check.

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

TelemetryEnvironment.csCentralize CI-provider detection and build_channel derivation +74/-0

Centralize CI-provider detection and build_channel derivation

• Adds a pure, testable environment abstraction for CI detection across many providers, including value-aware handling of 'CI=false' opt-out. Adds 'BuildChannel(version)' to classify release vs prerelease vs unknown, carefully handling build metadata containing hyphens.

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

SetupCommand.csFix setup funnel has_existing_profile signal +1/-1

Fix setup funnel has_existing_profile signal

• Replaces the incorrect 'Profiles.Count > 0' check with 'AppConfig.HasConfiguredProfile(...)' when emitting 'cli_setup_started'. Restores the funnel’s ability to distinguish first-time setup from reruns.

src/Capacitor.Cli/Commands/SetupCommand.cs

Tests (3) +144 / -1
AppConfigHasConfiguredProfileTests.csAdd unit tests for HasConfiguredProfile edge cases +58/-0

Add unit tests for HasConfiguredProfile edge cases

• Adds coverage for synthesized default profiles, blank/whitespace server URLs, configured non-active profiles, and empty profile sets. Prevents regression of the original measurement bug.

test/Capacitor.Cli.Tests.Unit/AppConfigHasConfiguredProfileTests.cs

McpTelemetryTests.csAllowlist build_channel in shared telemetry properties +2/-1

Allowlist build_channel in shared telemetry properties

• Extends the MCP telemetry property allowlist to include 'build_channel'. Ensures shared properties remain deliberate and constrained.

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

TelemetryEnvironmentTests.csAdd unit tests for CI provider table and build_channel parsing +84/-0

Add unit tests for CI provider table and build_channel parsing

• Adds parameterized tests for each supported CI provider variable and for 'CI=false/0' opt-out semantics. Adds build_channel tests covering prerelease, release, unknown, and build-metadata hyphen edge cases.

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

Documentation (1) +17 / -1
2026-08-08-cli-telemetry-signup-funnel-design.mdDocument build_channel and clarify has_existing_profile semantics +17/-1

Document build_channel and clarify has_existing_profile semantics

• Updates the CLI event catalog to include 'build_channel' on every event and explains why it exists. Adds explicit guidance that 'has_existing_profile' must be based on persisted server URL presence, not a synthesized default profile count.

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

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@alexeyzimarev
alexeyzimarev merged commit 98088d8 into main Aug 11, 2026
14 of 16 checks passed
@alexeyzimarev
alexeyzimarev deleted the telemetry-funnel-signal-fixes branch August 11, 2026 18:15
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: has_existing_profile is always true, and is_ci misses most CI providers

1 participant