feat(telemetry): CLI usage telemetry - #410
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…omes, disclosure wording Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 6b9fe1c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
CLERK_TELEMETRY_DISABLED=yes silently keeping telemetry on is the worst failure mode for a privacy control; only "0"/"false" now keep it enabled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…check, --verbose payload dump The first human run only shows the notice (and says nothing was sent); finalize re-checks enablement against persisted config so `clerk telemetry disable` itself never emits an event; the full payload is visible with --verbose before the POST. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sisted opt-out status reports the winning reason (env var > clerk telemetry disable > dev-build guard); a run of `clerk telemetry disable` never sends an event. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…app ids honestly Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
djgould
changed the title
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CLI now records command paths, supplied flag names, outcomes, durations, runtime metadata, and environment signals through configurable telemetry. Persisted configuration stores a machine UUID, disclosure state, and opt-out state. New Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli-core/src/cli-program.ts`:
- Line 145: Move startCommandTelemetry(actionCommand) in the command execution
flow before the mode validation that can call throwUsageError, ensuring invalid
--mode runs still create telemetry context for finalizeAndSendTelemetry(). Add
an integration test covering an invalid --mode value and assert exactly one
CLI_COMMAND_EXECUTED event is emitted.
In `@packages/cli-core/src/lib/config.ts`:
- Around line 217-249: Serialize the read-modify-write operations in
ensureMachineUuid, markTelemetryNoticeShown, and setTelemetryDisabled so
concurrent processes cannot overwrite unrelated configuration changes. Use the
project’s cross-process locking mechanism if available, or an atomic
merge/update strategy that preserves fields written by interleaved operations;
ensure a concurrent ensureMachineUuid and setTelemetryDisabled(true) retains
both the UUID and opt-out. Add an interleaving test covering that scenario.
In `@packages/cli-core/src/lib/env-signals.ts`:
- Around line 34-42: Normalize telemetry-bound environment values against
controlled allowlists: update detectTerminalProgram in
packages/cli-core/src/lib/env-signals.ts (lines 34-42) to return only recognized
terminal keys, using an empty string or "unknown" for arbitrary LC_TERMINAL and
TERM_PROGRAM values; update the CLERK_INSTALL_METHOD handling in
packages/cli-core/src/lib/env-signals.ts (lines 50-64) to allow only defined
installation-method keys. In packages/cli-core/src/lib/env-signals.test.ts
(lines 36-59), replace verbatim terminal-value expectations with recognized and
unknown-value cases, and add arbitrary CLERK_INSTALL_METHOD rejection coverage
at lines 62-114.
In `@packages/cli-core/src/lib/telemetry.ts`:
- Around line 158-172: Update finalizeAndSendTelemetry to start a single
TELEMETRY_TIMEOUT_MS deadline at function entry and apply it across status
checks, notice handling, UUID/profile resolution, and the eventual send
workflow, rather than starting the timeout near the fetch. Preserve handling for
failures from late or timed-out telemetry work, and add a test that blocks the
pre-fetch telemetry steps and verifies command completion remains bounded.
In `@README.md`:
- Around line 63-68: Qualify the first-run telemetry disclosure wording so the
no-send behavior is stated only when the first-run notice is shown, accounting
for CI suppression. Apply the same wording update in README.md lines 63-68 and
.changeset/usage-telemetry.md line 5; no other telemetry behavior changes are
needed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef9ab852-9e71-4f44-9c3d-35f965fab935
📒 Files selected for processing (15)
.changeset/usage-telemetry.mdREADME.mdpackages/cli-core/src/cli-program.tspackages/cli-core/src/commands/telemetry/README.mdpackages/cli-core/src/commands/telemetry/index.tspackages/cli-core/src/lib/config.test.tspackages/cli-core/src/lib/config.tspackages/cli-core/src/lib/constants.tspackages/cli-core/src/lib/env-signals.test.tspackages/cli-core/src/lib/env-signals.tspackages/cli-core/src/lib/telemetry.test.tspackages/cli-core/src/lib/telemetry.tspackages/cli-core/src/lib/user-agent.test.tspackages/cli-core/src/lib/user-agent.tspackages/cli-core/src/test/integration/telemetry.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)
| /** Persistent random machine id for telemetry. Generated on first use. */ | ||
| export async function ensureMachineUuid(): Promise<string> { | ||
| const config = await readConfig(); | ||
| if (config.machineUuid) return config.machineUuid; | ||
| config.machineUuid = crypto.randomUUID(); | ||
| await writeConfig(config); | ||
| return config.machineUuid; | ||
| } | ||
|
|
||
| /** Flip the one-time telemetry notice flag. Returns true only on the transition. */ | ||
| export async function markTelemetryNoticeShown(): Promise<boolean> { | ||
| const config = await readConfig(); | ||
| if (config.telemetryNoticeShown) return false; | ||
| config.telemetryNoticeShown = true; | ||
| await writeConfig(config); | ||
| return true; | ||
| } | ||
|
|
||
| /** Persisted telemetry opt-out, set via `clerk telemetry disable`. */ | ||
| export async function getTelemetryDisabled(): Promise<boolean> { | ||
| const config = await readConfig(); | ||
| return config.telemetryDisabled === true; | ||
| } | ||
|
|
||
| export async function setTelemetryDisabled(disabled: boolean): Promise<void> { | ||
| const config = await readConfig(); | ||
| if (disabled) { | ||
| config.telemetryDisabled = true; | ||
| } else { | ||
| delete config.telemetryDisabled; | ||
| } | ||
| await writeConfig(config); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/cli-core/src/lib/config.ts --items all
rg -n -C 10 'function writeConfig|function readConfig|ensureMachineUuid|setTelemetryDisabled|setEnvironment' \
packages/cli-core/src/lib/config.tsRepository: clerk/cli
Length of output: 6573
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- config implementation ---'
sed -n '1,155p' packages/cli-core/src/lib/config.ts
printf '%s\n' '--- config tests and callers ---'
rg -n -C 6 'readConfig|writeConfig|ensureMachineUuid|setTelemetryDisabled|telemetryDisabled|machineUuid' \
packages --glob '*.{ts,tsx,js,jsx}' --glob '*.{test,spec}.{ts,tsx,js,jsx}' || true
printf '%s\n' '--- filesystem access wrapper ---'
rg -n -C 12 'withHomeFsAccess|lock|mutex|semaphore|atomic|rename|Bun\.write' \
packages/cli-core/src --glob '*.{ts,tsx,js,jsx}' || trueRepository: clerk/cli
Length of output: 50366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("packages/cli-core/src/lib/config.ts").read_text()
start = source.index("export async function ensureMachineUuid")
end = source.index("type ResolvedVia", start)
section = source[start:end]
required = [
"const config = await readConfig();",
"await writeConfig(config);",
]
print("read-modify-write functions:", section.count("const config = await readConfig();"))
print("writes:", section.count("await writeConfig(config);"))
print("has lock primitive in section:", any(x in section.lower() for x in ("lock", "mutex", "semaphore")))
print("has merge/re-read in section:", "readConfig()" in section[section.index("await writeConfig(config);"):] or
"Object.assign" in section or "merge" in section.lower())
# Demonstrate the lost-update interleaving using the fields in the source.
initial = {}
uuid_snapshot = dict(initial)
optout_snapshot = dict(initial)
uuid_snapshot["machineUuid"] = "generated-uuid"
optout_snapshot["telemetryDisabled"] = True
# UUID writer completes after the opt-out writer.
final = uuid_snapshot
print("interleaving final config:", final)
print("telemetryDisabled preserved:", final.get("telemetryDisabled") is True)
PYRepository: clerk/cli
Length of output: 356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- config.ts ---'
sed -n '1,150p' packages/cli-core/src/lib/config.ts
printf '%s\n' '--- host-execution files ---'
fd -i 'host-execution|config' packages/cli-core/src packages/cli-core/test packages/cli-core/tests 2>/dev/null \
| head -80
printf '%s\n' '--- access wrapper definitions and uses ---'
rg -l 'withHomeFsAccess' packages/cli-core/src | sort
rg -n -C 12 '^export .*withHomeFsAccess|function withHomeFsAccess|withHomeFsAccess' \
packages/cli-core/src/lib/host-execution.ts packages/cli-core/src/lib 2>/dev/null
printf '%s\n' '--- telemetry flows ---'
rg -n -C 8 'getTelemetryDisabled|ensureMachineUuid|telemetryDisabled|telemetry disable' \
packages/cli-core/src --glob '*.ts'Repository: clerk/cli
Length of output: 45773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source = Path("packages/cli-core/src/lib/config.ts").read_text()
section = source[source.index("export async function ensureMachineUuid"):
source.index("type ResolvedVia", source.index("export async function ensureMachineUuid"))]
for name in ("ensureMachineUuid", "setTelemetryDisabled"):
match = re.search(
rf"export async function {name}\b.*?(?=\nexport async function |\ntype ResolvedVia)",
section,
re.S,
)
print(f"--- {name} ---")
print(match.group(0) if match else "not found")
print("--- writeConfig implementation facts ---")
write = re.search(r"export async function writeConfig\(.*?(?=\nexport async function )", source, re.S)
print("direct Bun.write:", bool(write and "Bun.write" in write.group(0)))
print("read before write in writeConfig:", bool(write and "readConfig" in write.group(0)))
print("lock terms in writeConfig:", bool(write and re.search(r"\b(lock|mutex|semaphore)\b", write.group(0), re.I)))
# Deterministic lost-update model for the exact two-field mutation pattern.
initial = {}
uuid_snapshot = dict(initial)
disable_snapshot = dict(initial)
uuid_snapshot["machineUuid"] = "uuid"
disable_snapshot["telemetryDisabled"] = True
final = uuid_snapshot # UUID write occurs after disable write.
assert final.get("telemetryDisabled") is None
print("lost-update interleaving reproduces:", True)
PYRepository: clerk/cli
Length of output: 732
Serialize configuration updates across processes. readConfig followed by writeConfig is not atomic, and withHomeFsAccess does not provide locking. A UUID write can overwrite telemetryDisabled: true with an older snapshot. A later command can then send telemetry after the user opted out. Add a cross-process lock or atomic merge-based update, plus an interleaving test for ensureMachineUuid() and setTelemetryDisabled(true).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli-core/src/lib/config.ts` around lines 217 - 249, Serialize the
read-modify-write operations in ensureMachineUuid, markTelemetryNoticeShown, and
setTelemetryDisabled so concurrent processes cannot overwrite unrelated
configuration changes. Use the project’s cross-process locking mechanism if
available, or an atomic merge/update strategy that preserves fields written by
interleaved operations; ensure a concurrent ensureMachineUuid and
setTelemetryDisabled(true) retains both the UUID and opt-out. Add an
interleaving test covering that scenario.
There was a problem hiding this comment.
Declining in this PR: the unlocked read-modify-write is this file's established pattern for every field (auth, profiles, relay), so locking belongs to the config layer as its own change — follow-up queued for atomic config writes.
There was a problem hiding this comment.
@djgould, acknowledged. The race remains a config-layer concern, and an atomic config-write follow-up is the appropriate scope for it. I will not require the locking change in this PR.
You are interacting with an AI system.
wyattjoh
left a comment
wyattjoh
left a comment
There was a problem hiding this comment.
Solid implementation overall, and the mid-PR docs revision to drop "anonymous" once workspace/app IDs entered the payload was the right call. My concerns are concentrated in one place: the send is awaited on the critical path. I ran the CLI to check rather than just reading it, and the measurements are in the inline comments. Worth noting cli-program.ts is this repo's highest-churn file and its top bug hotspot, so the runProgram control-flow change gets extra scrutiny.
| exitCode: softExitCode, | ||
| }); | ||
| } catch (error) { | ||
| await finalizeAndSendTelemetry(telemetryResultForError(error)); |
There was a problem hiding this comment.
await finalizeAndSendTelemetry(...) is the first statement in the catch block, before any error rendering. The user waits on a network POST before learning their command failed.
Measured with CLERK_TELEMETRY_URL pointed at a blackholed address, on a purely local ~0.1s failure:
$ bun packages/cli-core/src/cli.ts env pull
[+1.08s] {"error":{"code":"not_linked",...}}
A full second before the "not linked" message appears. Rendering the error first and finalizing afterwards (or from a finally after the output block) fixes it without changing what gets collected.
There was a problem hiding this comment.
Fixed in 3a4da8d — the send starts at catch-top and overlaps rendering; awaited only right before each exit. Your repro now shows the error at local speed.
|
|
||
| const url = process.env.CLERK_TELEMETRY_URL ?? DEFAULT_TELEMETRY_ENDPOINT; | ||
| const controller = new AbortController(); | ||
| const timer = setTimeout(() => controller.abort(), TELEMETRY_TIMEOUT_MS); |
There was a problem hiding this comment.
Two issues here.
First, the cost is real on the success path too. Measured on clerk telemetry status with a hanging endpoint: 0.105s → 1.120s. That is a uniform latency tax on every invocation.
Second, the AbortController only covers the loggedFetch below it. getTelemetryStatus(), ensureMachineUuid() (config read plus a first-run write), and resolveProfile() (config read plus up to two git subprocess spawns for commands that never otherwise touch git) all run before this timer starts and are unbounded. So "failures never affect the command (1s cap)" is stronger than what the code guarantees.
Either detach the send from the exit path, or lower TELEMETRY_TIMEOUT_MS and put the entire body inside the abort budget rather than just the request.
There was a problem hiding this comment.
Fixed in d75fa06 — one deadline at finalize entry covers config I/O, resolveProfile, and the POST, and holds even if the work ignores the abort signal. The ~100ms healthy-endpoint tail on success stays by design: outcome/duration require send-at-end.
| const controller = new AbortController(); | ||
| const timer = setTimeout(() => controller.abort(), TELEMETRY_TIMEOUT_MS); | ||
| try { | ||
| await loggedFetch(url, { |
There was a problem hiding this comment.
loggedFetch wraps the request in withNetworkAccess, whose observeHostCapabilityFailure calls warnAboutSandbox() on a permission-like failure in agent mode. Since telemetry is now the last network call on every command, a sandboxed run whose other work was purely local can surface:
Host-only Clerk state or capabilities may be unavailable in agent mode (possible sandboxed run)…
after an otherwise successful command. The try/catch here swallows the error, but the log.warn has already been emitted — which cuts against this module's own "Telemetry must never affect the command" docstring, and misattributes a telemetry failure to the user's actual command.
Since .claude/rules/debug-logging.md rules out calling fetch directly, an opt-out flag on loggedFetch that skips capability observation seems like the cleanest fix.
There was a problem hiding this comment.
Fixed in 720e16a — the telemetry POST is best-effort; permission-shaped failures no longer surface the host-capability hint.
| * classified by executable path. | ||
| */ | ||
| export function detectInstallMethod(env: EnvLike, execPath: string): string { | ||
| if (env.CLERK_INSTALL_METHOD) return env.CLERK_INSTALL_METHOD; |
There was a problem hiding this comment.
CLERK_INSTALL_METHOD here, plus LC_TERMINAL (line 35) and TERM_PROGRAM (line 42), are returned unmodified and land in the payload. Every other signal in this module is a closed enum, which is what makes these stand out — they are unbounded-cardinality analytics dimensions and an arbitrary-string path into the pipeline.
It also puts pressure on the README's "never collects command arguments, option values, file paths" claim: CLERK_INSTALL_METHOD can hold anything, including a path.
Truncating to a small fixed length and either allowlisting known values or slugging them (lowercase, non-alphanumerics collapsed) would keep the signal without the open end.
There was a problem hiding this comment.
Fixed in d9e8033 — terminal fallbacks slugged + 32-char cap, CLERK_INSTALL_METHOD allowlisted.
| log.data(JSON.stringify(status)); | ||
| return; | ||
| } | ||
| log.data(`Telemetry is ${status.enabled ? "enabled" : "disabled"}`); |
There was a problem hiding this comment.
log.data("Telemetry is enabled") writes UI prose to stdout. Per .claude/rules/logging.md, log.data() is reserved for pipeable machine-readable output and human status text belongs on stderr via log.info. The agent-mode JSON branch just above is correct as-is.
Switching the human branch to log.info needs a matching update to the integration test that asserts enabled.stdout.
There was a problem hiding this comment.
Fixed in 76167b1 — bare enabled/disabled on stdout (whoami pattern), reason on stderr.
The send starts at catch-top and overlaps with error rendering; it is awaited only immediately before process.exit, so a slow endpoint can no longer delay the error output (~1s worst case previously). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 1s cap now covers config I/O and the git profile lookup, not just the fetch, and holds even if the underlying work ignores the abort signal — the event is dropped on timeout instead of stalling the exit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| return config.telemetryDisabled === true; | ||
| } | ||
|
|
||
| export async function setTelemetryDisabled(disabled: boolean): Promise<void> { |
There was a problem hiding this comment.
setTelemetryDisabled(true) clears nothing else, so opting out and later back in resumes the same machineUuid. Clearing it on disable would make the opt-out a cleaner privacy boundary — the user gets a fresh identifier if they return, rather than being rejoined to their prior history.
There was a problem hiding this comment.
Fixed in 08f747c — disable sheds the uuid; re-enable starts a fresh, unlinkable identity.
The disclosure notice and its no-send grace run now apply to human and agent runs alike; only CI is exempt — ephemeral CI machines are always on their first run, so a grace there would mean CI never sends. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Terminal fallbacks are slugged (lowercase, safe charset, 32-char cap) and the CLERK_INSTALL_METHOD override only accepts known enum values — no unbounded env string can reach the payload. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Start the telemetry context first in the preAction hook so validation and env-init throws still produce an event. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… on stderr Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-enabling telemetry starts a fresh, unlinkable machine identity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli-core/src/test/integration/telemetry.test.ts`:
- Around line 261-264: Update the test for `clerk telemetry status` to assert
that stderr includes the dev-build guard explanation, while preserving the
existing stdout assertion for `disabled`.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ea2e43c-7d72-4447-8a68-5bd47c3eb87a
📒 Files selected for processing (12)
.changeset/usage-telemetry.mdREADME.mdpackages/cli-core/src/cli-program.tspackages/cli-core/src/commands/telemetry/README.mdpackages/cli-core/src/commands/telemetry/index.tspackages/cli-core/src/lib/config.test.tspackages/cli-core/src/lib/config.tspackages/cli-core/src/lib/env-signals.test.tspackages/cli-core/src/lib/env-signals.tspackages/cli-core/src/lib/telemetry.test.tspackages/cli-core/src/lib/telemetry.tspackages/cli-core/src/test/integration/telemetry.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)
🚧 Files skipped from review as they are similar to previous changes (8)
- README.md
- packages/cli-core/src/commands/telemetry/README.md
- .changeset/usage-telemetry.md
- packages/cli-core/src/lib/config.test.ts
- packages/cli-core/src/cli-program.ts
- packages/cli-core/src/commands/telemetry/index.ts
- packages/cli-core/src/lib/telemetry.ts
- packages/cli-core/src/lib/env-signals.test.ts
| test("`clerk telemetry status` explains the dev-build guard", async () => { | ||
| http.mock(); // dev build without the URL escape hatch: no network at all | ||
| const result = await clerk("telemetry", "status"); | ||
| expect(result.stdout.trim()).toBe("disabled"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the dev-build disable reason on stderr.
This test only verifies disabled on stdout. It does not verify the dev-build explanation that the test name and status-output contract require. Add an assertion for the dev-build guard reason on stderr.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli-core/src/test/integration/telemetry.test.ts` around lines 261 -
264, Update the test for `clerk telemetry status` to assert that stderr includes
the dev-build guard explanation, while preserving the existing stdout assertion
for `disabled`.
The POST is marked best-effort so a permission-shaped failure in agent mode stays debug-only instead of surfacing the host-capability hint after otherwise-successful commands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Wyatt Johnson <accounts+github@wyattjoh.ca>
… the opt-out The segment is analytics-purposed, so the fetch layer resolves the User-Agent once per process against the env and persisted opt-outs and omits the segment when telemetry is off (failing toward omission). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli-core/src/lib/fetch.test.ts`:
- Around line 63-81: Update the fetch test setup around beforeEach and afterEach
to capture the original values of CLERK_TELEMETRY_DISABLED and DO_NOT_TRACK,
then restore each value during cleanup instead of always deleting them. Preserve
deletion only when the variable was originally undefined, matching the existing
CLAUDECODE restoration behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d80bd49a-7846-4b25-9e45-f07b8e6ff051
📒 Files selected for processing (9)
packages/cli-core/src/commands/users/interactive/instance-context.test.tspackages/cli-core/src/commands/webhooks/listen.test.tspackages/cli-core/src/lib/env-signals.tspackages/cli-core/src/lib/fetch.test.tspackages/cli-core/src/lib/fetch.tspackages/cli-core/src/lib/telemetry.tspackages/cli-core/src/lib/user-agent.test.tspackages/cli-core/src/lib/user-agent.tspackages/cli-core/src/test/lib/stubs.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli-core/src/lib/user-agent.ts
| beforeEach(async () => { | ||
| configDir = await mkdtemp(join(tmpdir(), "clerk-fetch-test-")); | ||
| _setConfigDir(configDir); | ||
| _resetUserAgentCache(); | ||
| originalClaudecode = process.env.CLAUDECODE; | ||
| process.env.CLAUDECODE = "1"; | ||
| delete process.env.CLERK_TELEMETRY_DISABLED; | ||
| delete process.env.DO_NOT_TRACK; | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| globalThis.fetch = originalFetch; | ||
| _setConfigDir(undefined); | ||
| _resetUserAgentCache(); | ||
| if (originalClaudecode === undefined) delete process.env.CLAUDECODE; | ||
| else process.env.CLAUDECODE = originalClaudecode; | ||
| delete process.env.CLERK_TELEMETRY_DISABLED; | ||
| delete process.env.DO_NOT_TRACK; | ||
| await rm(configDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the original telemetry opt-out variables.
Lines 69-70 delete caller-provided values. Lines 79-80 do not restore them. This can change later tests when the test process starts with either opt-out variable set.
Proposed fix
let configDir: string;
let originalClaudecode: string | undefined;
+ let originalClerkTelemetryDisabled: string | undefined;
+ let originalDoNotTrack: string | undefined;
beforeEach(async () => {
configDir = await mkdtemp(join(tmpdir(), "clerk-fetch-test-"));
_setConfigDir(configDir);
_resetUserAgentCache();
originalClaudecode = process.env.CLAUDECODE;
+ originalClerkTelemetryDisabled = process.env.CLERK_TELEMETRY_DISABLED;
+ originalDoNotTrack = process.env.DO_NOT_TRACK;
process.env.CLAUDECODE = "1";
delete process.env.CLERK_TELEMETRY_DISABLED;
delete process.env.DO_NOT_TRACK;
});
afterEach(async () => {
// ...
- delete process.env.CLERK_TELEMETRY_DISABLED;
- delete process.env.DO_NOT_TRACK;
+ if (originalClerkTelemetryDisabled === undefined) delete process.env.CLERK_TELEMETRY_DISABLED;
+ else process.env.CLERK_TELEMETRY_DISABLED = originalClerkTelemetryDisabled;
+ if (originalDoNotTrack === undefined) delete process.env.DO_NOT_TRACK;
+ else process.env.DO_NOT_TRACK = originalDoNotTrack;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| beforeEach(async () => { | |
| configDir = await mkdtemp(join(tmpdir(), "clerk-fetch-test-")); | |
| _setConfigDir(configDir); | |
| _resetUserAgentCache(); | |
| originalClaudecode = process.env.CLAUDECODE; | |
| process.env.CLAUDECODE = "1"; | |
| delete process.env.CLERK_TELEMETRY_DISABLED; | |
| delete process.env.DO_NOT_TRACK; | |
| }); | |
| afterEach(async () => { | |
| globalThis.fetch = originalFetch; | |
| _setConfigDir(undefined); | |
| _resetUserAgentCache(); | |
| if (originalClaudecode === undefined) delete process.env.CLAUDECODE; | |
| else process.env.CLAUDECODE = originalClaudecode; | |
| delete process.env.CLERK_TELEMETRY_DISABLED; | |
| delete process.env.DO_NOT_TRACK; | |
| await rm(configDir, { recursive: true, force: true }); | |
| let configDir: string; | |
| let originalClaudecode: string | undefined; | |
| let originalClerkTelemetryDisabled: string | undefined; | |
| let originalDoNotTrack: string | undefined; | |
| beforeEach(async () => { | |
| configDir = await mkdtemp(join(tmpdir(), "clerk-fetch-test-")); | |
| _setConfigDir(configDir); | |
| _resetUserAgentCache(); | |
| originalClaudecode = process.env.CLAUDECODE; | |
| originalClerkTelemetryDisabled = process.env.CLERK_TELEMETRY_DISABLED; | |
| originalDoNotTrack = process.env.DO_NOT_TRACK; | |
| process.env.CLAUDECODE = "1"; | |
| delete process.env.CLERK_TELEMETRY_DISABLED; | |
| delete process.env.DO_NOT_TRACK; | |
| }); | |
| afterEach(async () => { | |
| globalThis.fetch = originalFetch; | |
| _setConfigDir(undefined); | |
| _resetUserAgentCache(); | |
| if (originalClaudecode === undefined) delete process.env.CLAUDECODE; | |
| else process.env.CLAUDECODE = originalClaudecode; | |
| if (originalClerkTelemetryDisabled === undefined) delete process.env.CLERK_TELEMETRY_DISABLED; | |
| else process.env.CLERK_TELEMETRY_DISABLED = originalClerkTelemetryDisabled; | |
| if (originalDoNotTrack === undefined) delete process.env.DO_NOT_TRACK; | |
| else process.env.DO_NOT_TRACK = originalDoNotTrack; | |
| await rm(configDir, { recursive: true, force: true }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli-core/src/lib/fetch.test.ts` around lines 63 - 81, Update the
fetch test setup around beforeEach and afterEach to capture the original values
of CLERK_TELEMETRY_DISABLED and DO_NOT_TRACK, then restore each value during
cleanup instead of always deleting them. Preserve deletion only when the
variable was originally undefined, matching the existing CLAUDECODE restoration
behavior.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The catch block in `runProgram` repeated `await pendingTelemetry; process.exit(...)` across five branches, so every new error branch had to remember to flush telemetry before exiting. Extract the rendering and exit-code decision into `reportError` and keep a single exit in `runProgram`, which makes the flush structural rather than a convention. Correct the hoisted-telemetry comment while here. The send does not overlap with the error output — `reportError` is synchronous, so nothing progresses between starting the send and awaiting it. The real reason to hoist is ordering: the message reaches the terminal before the process blocks on a possibly-slow endpoint. Cover the cascade directly. It previously had no tests: only a generic `Error` and a usage error were reached incidentally through the telemetry integration harness, leaving aborts, the whole `ApiError` branch, and the non-Error fallback unverified on the file with the highest fix-commit density in the repo. `isPromptExitError` had no coverage anywhere. The final case pins `reportError`'s exit code to `telemetryResultForError`'s, which is the invariant that keeps telemetry from reporting an exit code the user never saw.
| // The AIAgent segment exists purely for analytics classification, so it | ||
| // honors the telemetry opt-outs (env vars and `clerk telemetry disable`). | ||
| userAgentPromise ??= (async () => { | ||
| const optedOut = |
There was a problem hiding this comment.
The AIAgent segment honors both opt-outs but not the first-run grace. loggedFetch backs every API call, so a brand new user's very first request already carries AIAgent/claude_code before the disclosure notice has ever been printed, while the changeset promises the first run sends nothing. Gating on the notice state, with CI exempt to match maybeShowTelemetryNotice, keeps the header consistent with that promise:
// config.ts: read-only peek, no mutation
export async function getTelemetryNoticeShown(): Promise<boolean> {
return (await readConfig()).telemetryNoticeShown === true;
}
// fetch.ts
const graceRun =
!process.env.CI && !(await getTelemetryNoticeShown().catch(() => false));
const optedOut =
optOutEnvVar(process.env) !== null ||
graceRun ||
(await getTelemetryDisabled().catch(() => true));| if (!status.enabled && status.reason === "env") { | ||
| log.warn(`\`${status.envVar}\` is still set — telemetry stays disabled until it is unset.`); | ||
| } |
There was a problem hiding this comment.
Only the env reason gets a warning here. On a dev build, clerk telemetry enable prints an unqualified "Telemetry enabled." while clerk telemetry status immediately reports disabled for the dev-build guard, the same gotcha the env branch protects against. reason === "config" is impossible right after setTelemetryDisabled(false), so covering dev-build closes the set.
| if (!status.enabled && status.reason === "env") { | |
| log.warn(`\`${status.envVar}\` is still set — telemetry stays disabled until it is unset.`); | |
| } | |
| if (status.enabled) return; | |
| if (status.reason === "env") { | |
| log.warn(`\`${status.envVar}\` is still set, telemetry stays disabled until it is unset.`); | |
| return; | |
| } | |
| if (status.reason === "dev-build") { | |
| log.warn("This is a dev build (0.0.0-dev), telemetry stays disabled regardless."); | |
| } |
3 participants
Adds usage telemetry to the CLI (GROW-1200): one
CLI_COMMAND_EXECUTEDevent per command run, sent toclerk-telemetry.com(BigQuery,sdk = 'clerk-cli').What an event contains
commandusers list— subcommand names only, never argumentsflagsjson,limit— flag names only, never valuesoutcome/exit_code/error_codeerror/1/not_linkedduration_ms412machine_uuidinstall_methodnpm_global,homebrew,npx, …ai_agentclaude_code,cursor,gemini_cli, … — empty for humansterminal_programghostty,warp, …modehuman/agent(TTY detection)os/arch/ci/in_tmux/in_screendarwin/arm64/false/ …envproduction— which Clerk API the CLI targets (clerk switch-env); non-production means Clerk-internal usage, so analysts can filter itworkspace_id/app_idnullwhen unlinkedNever collected: command arguments, option values, file paths, error messages, or personal data.
Controls
clerk telemetry status|disable|enable— persisted opt-out; adisablerun sends nothing itselfCLERK_TELEMETRY_DISABLED/DO_NOT_TRACKenv vars (any non-false value)--verboseprints each event before the POST; failures never affect the command (1s cap)AIAgent/<agent>segment so API logs can classify agent traffic — omitted when telemetry is opted outRelease gate: telemetry-service drops iid-less events (
workers/telemetry-service/src/index.ts:91) — needs the one-linesdk === "clerk-cli"exemption in cloudflare-workers before this ships, then an end-to-end BigQuery check.🤖 Generated with Claude Code