feat(cli): anonymous opt-out usage analytics + stash telemetry#644
Open
coderdan wants to merge 3 commits into
Open
feat(cli): anonymous opt-out usage analytics + stash telemetry#644coderdan wants to merge 3 commits into
stash telemetry#644coderdan wants to merge 3 commits into
Conversation
Add anonymous, opt-out usage analytics to the stash CLI, plus a `stash telemetry [status|enable|disable]` command to manage it. - Coarse events only (command, version, OS/arch, success, duration); a property allowlist at the emitter boundary makes it impossible for plaintext, schema, table/column names, connection strings, or argument values to leave the machine. - Opt-out by default, Homebrew-style: nothing is sent until the one-time first-run notice has been shown once. Four opt-out gates in precedence order: DO_NOT_TRACK, STASH_TELEMETRY_DISABLED, CI auto-detect, and the persisted `stash telemetry disable` flag in ~/.cipherstash/telemetry.json. - Events go through a first-party Cloudflare proxy (infra/telemetry-proxy), so a future US->EU PostHog move is a proxy-target change with no CLI re-release. posthog-node, events-only (no person profiles), lazy client, flush bounded by a timeout so it never blocks or slows the CLI. - Ships dormant: no events are sent until a real PostHog key is embedded at release (STASH_POSTHOG_KEY overrides for testing). Updates the stash-cli skill and adds a changeset. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
🦋 Changeset detectedLatest commit: 9048fc6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
📝 WalkthroughWalkthroughAdds anonymous, opt-out CLI telemetry with persisted settings, CI-aware gating, allowlisted event fields, ChangesCLI telemetry
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an anonymous, opt-out telemetry system to the stash CLI (with a persisted machine-level state file), introduces a stash telemetry [status|enable|disable] command to manage the saved preference, and includes a Cloudflare Worker reverse-proxy so the CLI reports to a first-party endpoint that can be retargeted without re-releasing the CLI.
Changes:
- Implement CLI telemetry capture with opt-out gating, a one-time first-run notice, property allowlisting, and bounded shutdown flushing.
- Add
stash telemetrysubcommand + registry/docs wiring. - Add
infra/telemetry-proxy/Cloudflare Worker + deploy docs, plus a changeset for the CLI change.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/stash-cli/SKILL.md | Documents the new telemetry command and opt-out mechanisms in the published CLI skill. |
| packages/cli/src/telemetry/state.ts | Implements persisted telemetry state (~/.cipherstash/telemetry.json) with corruption tolerance and 0600 permissions. |
| packages/cli/src/telemetry/index.ts | Telemetry core: gating, allowlist sanitization, PostHog client creation, first-run notice, capture + shutdown flush. |
| packages/cli/src/telemetry/tests/telemetry.test.ts | Adds tests for gate precedence and property allowlist behavior. |
| packages/cli/src/telemetry/tests/state.test.ts | Adds tests for telemetry state persistence and corruption recovery. |
| packages/cli/src/messages.ts | Adds the first-run telemetry notice and stash telemetry UX strings. |
| packages/cli/src/commands/telemetry/index.ts | Implements `stash telemetry [status |
| packages/cli/src/commands/index.ts | Exports the new telemetry command. |
| packages/cli/src/cli/registry.ts | Registers telemetry in the CLI command registry/help surface. |
| packages/cli/src/bin/main.ts | Wires telemetry init/notice/tracking/shutdown into the main CLI dispatch path. |
| infra/telemetry-proxy/wrangler.toml | Adds Worker config for telemetry.cipherstash.com and POSTHOG_HOST target var. |
| infra/telemetry-proxy/worker.js | Implements allowlisted-path reverse proxy to PostHog ingestion endpoints. |
| infra/telemetry-proxy/README.md | Adds deploy + verification instructions for the proxy Worker. |
| .changeset/cli-anonymous-telemetry.md | Records the CLI user-facing feature addition for release notes/versioning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review findings: - shutdownTelemetry: clear the flush-timeout timer so a fast flush no longer leaves a dangling setTimeout that hangs the process for the full timeout; also set fetchRetryCount:0 so an unreachable endpoint fails fast instead of retrying with backoff (which kept internal timers alive and hung the CLI for seconds). Enabled commands now exit in ~140ms even when the endpoint is down. - First-run notice: persist noticeShownAt ONLY when the notice was actually displayed (stderr is a TTY), so a non-interactive first run no longer consumes the freebie and starts sending without the user ever seeing the disclosure. - Broaden DO_NOT_TRACK / STASH_TELEMETRY_DISABLED to opt out on any non-empty value except 0/false (matches the DO_NOT_TRACK convention). - Reuse the shared isCiEnv() from config/tty.ts instead of a second, divergent CI detector; widen isCiEnv() with provider markers (GITHUB_ACTIONS, GITLAB_CI, Azure TF_BUILD, Jenkins, TeamCity, …) so telemetry never fires in CI even when a provider doesn't set CI=true. Export CI_ENV_VARS and make the region / database-url tests hermetic against those ambient vars. - Drop the dead resolveStatus() re-resolve after writing the notice. Copilot: - Sanitize the combined event+base props so the allowlist is the single enforced boundary (a future baseProps field can't bypass it). - Make the first-run opt-out hint runner-aware (npx stash …) so it's actionable before stash is on PATH. - Restore process.env.HOME in the state test so it can't leak across test files. - Worker returns 500 when POSTHOG_HOST is unset instead of forwarding to an invalid host. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Contributor
Author
|
Addressed the review + Copilot feedback in ad77675. Copilot comments
Review findings also fixed
425 tests pass; Biome clean. |
Widening isCiEnv() with provider markers (GITHUB_ACTIONS, GITLAB_CI, …) made the pty e2e harness leak this repo's own CI env into the spawned CLI: the harness defaults CI=true and interactive tests override with `CI: ''`, but that no longer fully de-CIs the environment once GITHUB_ACTIONS=true is also present, so `auth login` skipped the region prompt and the test timed out. Strip every CI signal (CI_ENV_VARS) from the child env before applying the harness's CI default and per-test overrides, so CI detection is fully controlled by the harness — matching how the unit tests were made hermetic. Verified the full e2e suite passes with GITHUB_ACTIONS=true CI=true exported. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/bin/main.ts (1)
537-540: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid
process.exit()in these unknown-command branches — it short-circuitsrun()’sfinally, so the telemetry event/flush never runs on typo/error exits. Throw instead, or flush before exiting, inpackages/cli/src/bin/main.ts#L537-L540andpackages/cli/src/commands/telemetry/index.ts#L60-L63.🤖 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/src/bin/main.ts` around lines 537 - 540, Replace the direct process.exit() calls in the unknown-command branches of packages/cli/src/bin/main.ts lines 537-540 and packages/cli/src/commands/telemetry/index.ts lines 60-63 with thrown errors so run() reaches its finally block and telemetry flushes; preserve the existing error message and help output.
🧹 Nitpick comments (3)
packages/cli/src/__tests__/database-url.test.ts (1)
43-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract duplicate test helpers to a shared module.
These test files contain identical logic for managing provider CI environment variables. Extracting this setup into a shared helper (e.g., in
packages/cli/tests/helpers/) will keep the test suite DRY and easier to maintain.
packages/cli/src/__tests__/database-url.test.ts#L43-L66: extract the duplicatedsavedProviderCi,neutralizeProviderCi, andrestoreProviderCilogic into a shared test helper.packages/cli/src/commands/auth/__tests__/region.test.ts#L33-L56: replace this duplicated logic with an import from the new shared test helper.🤖 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/src/__tests__/database-url.test.ts` around lines 43 - 66, Extract the duplicated savedProviderCi, neutralizeProviderCi, and restoreProviderCi logic into a shared test helper under packages/cli/tests/helpers/, preserving exact environment restoration and CI handling. In packages/cli/src/__tests__/database-url.test.ts:43-66, replace the local state and functions with imports from the helper; apply the same replacement in packages/cli/src/commands/auth/__tests__/region.test.ts:33-56.packages/cli/src/telemetry/__tests__/telemetry.test.ts (1)
1-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding lifecycle coverage for
trackCommand/maybeShowFirstRunNotice/shutdownTelemetry.This suite thoroughly covers
resolveStatus/sanitize(both pure), but the side-effecting lifecycle functions that depend on module-levelstate/status/firstRunare untested here. Since those variables are captured once at import time, exercising them would needvi.resetModules()+ dynamic re-import per test — worth doing given they encode the "freebie first run" and "no-op when disabled" guarantees the PR relies on.🤖 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/src/telemetry/__tests__/telemetry.test.ts` around lines 1 - 169, Extend the telemetry test suite with lifecycle coverage for trackCommand, maybeShowFirstRunNotice, and shutdownTelemetry using vi.resetModules() and dynamic imports per test so module-level state, status, and firstRun are reinitialized. Verify the enabled first-run/freebie behavior and that disabled telemetry paths are no-ops, reusing the existing environment setup and mocks where needed.packages/cli/src/commands/telemetry/index.ts (1)
10-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize these status strings in
messages.tsfor consistency.
enabled/disabled/unknownSubcommandalready pull frommessages.telemetry, butreasonText's per-reason strings andprintStatus's info lines are inlined here instead. If any E2E test ever asserts on this copy, it'll need to track two places instead of one.As per path instructions: "Keep assertion-stable user-facing strings in
src/messages.tsand update the constants there when E2E tests assert on copy such as cancellation text, 'Unknown auth command', or thedb migratestub warning."🤖 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/src/commands/telemetry/index.ts` around lines 10 - 38, Move the user-facing telemetry status and reason strings used by reasonText and printStatus into the existing telemetry section of messages.ts, alongside enabled, disabled, and unknownSubcommand. Update these functions to reference the centralized message constants while preserving the current wording and status-specific behavior.Source: Path instructions
🤖 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 `@infra/telemetry-proxy/worker.js`:
- Around line 48-49: Update the forwarded request construction in the proxy
handler to remove IP-revealing headers, including CF-Connecting-IP and
X-Forwarded-For, after copying the incoming request headers and before
forwarding to PostHog. Preserve the existing Host header assignment while
ensuring these anonymity-sensitive headers cannot reach the target.
---
Outside diff comments:
In `@packages/cli/src/bin/main.ts`:
- Around line 537-540: Replace the direct process.exit() calls in the
unknown-command branches of packages/cli/src/bin/main.ts lines 537-540 and
packages/cli/src/commands/telemetry/index.ts lines 60-63 with thrown errors so
run() reaches its finally block and telemetry flushes; preserve the existing
error message and help output.
---
Nitpick comments:
In `@packages/cli/src/__tests__/database-url.test.ts`:
- Around line 43-66: Extract the duplicated savedProviderCi,
neutralizeProviderCi, and restoreProviderCi logic into a shared test helper
under packages/cli/tests/helpers/, preserving exact environment restoration and
CI handling. In packages/cli/src/__tests__/database-url.test.ts:43-66, replace
the local state and functions with imports from the helper; apply the same
replacement in packages/cli/src/commands/auth/__tests__/region.test.ts:33-56.
In `@packages/cli/src/commands/telemetry/index.ts`:
- Around line 10-38: Move the user-facing telemetry status and reason strings
used by reasonText and printStatus into the existing telemetry section of
messages.ts, alongside enabled, disabled, and unknownSubcommand. Update these
functions to reference the centralized message constants while preserving the
current wording and status-specific behavior.
In `@packages/cli/src/telemetry/__tests__/telemetry.test.ts`:
- Around line 1-169: Extend the telemetry test suite with lifecycle coverage for
trackCommand, maybeShowFirstRunNotice, and shutdownTelemetry using
vi.resetModules() and dynamic imports per test so module-level state, status,
and firstRun are reinitialized. Verify the enabled first-run/freebie behavior
and that disabled telemetry paths are no-ops, reusing the existing environment
setup and mocks where needed.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97e29ac0-3f73-4457-b803-3e488d1cd2f0
📒 Files selected for processing (18)
.changeset/cli-anonymous-telemetry.mdinfra/telemetry-proxy/README.mdinfra/telemetry-proxy/worker.jsinfra/telemetry-proxy/wrangler.tomlpackages/cli/src/__tests__/database-url.test.tspackages/cli/src/bin/main.tspackages/cli/src/cli/registry.tspackages/cli/src/commands/auth/__tests__/region.test.tspackages/cli/src/commands/index.tspackages/cli/src/commands/telemetry/index.tspackages/cli/src/config/tty.tspackages/cli/src/messages.tspackages/cli/src/telemetry/__tests__/state.test.tspackages/cli/src/telemetry/__tests__/telemetry.test.tspackages/cli/src/telemetry/index.tspackages/cli/src/telemetry/state.tspackages/cli/tests/helpers/pty.tsskills/stash-cli/SKILL.md
Comment on lines
+48
to
+49
| const forwarded = new Request(url, request) | ||
| forwarded.headers.set('Host', target) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Strip IP-revealing headers to ensure strict anonymity.
When constructing the forwarded request via new Request(url, request), all headers from the incoming request are copied. This includes headers injected by Cloudflare at the edge, such as CF-Connecting-IP and X-Forwarded-For. Even though the CLI uses disableGeoip: true, forwarding these headers exposes the user's raw IP address to PostHog, which undermines the guarantee of anonymous analytics.
Delete these headers from the forwarded request to ensure the telemetry remains fully anonymized at the proxy boundary.
🛡️ Proposed fix to strip IP headers
const forwarded = new Request(url, request)
forwarded.headers.set('Host', target)
+
+ // Ensure strict anonymity by stripping IP-revealing headers injected by Cloudflare or intermediaries
+ forwarded.headers.delete('cf-connecting-ip')
+ forwarded.headers.delete('x-forwarded-for')
+ forwarded.headers.delete('x-real-ip')
+ forwarded.headers.delete('true-client-ip')📝 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.
Suggested change
| const forwarded = new Request(url, request) | |
| forwarded.headers.set('Host', target) | |
| const forwarded = new Request(url, request) | |
| forwarded.headers.set('Host', target) | |
| // Ensure strict anonymity by stripping IP-revealing headers injected by Cloudflare or intermediaries | |
| forwarded.headers.delete('cf-connecting-ip') | |
| forwarded.headers.delete('x-forwarded-for') | |
| forwarded.headers.delete('x-real-ip') | |
| forwarded.headers.delete('true-client-ip') |
🤖 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 `@infra/telemetry-proxy/worker.js` around lines 48 - 49, Update the forwarded
request construction in the proxy handler to remove IP-revealing headers,
including CF-Connecting-IP and X-Forwarded-For, after copying the incoming
request headers and before forwarding to PostHog. Preserve the existing Host
header assignment while ensuring these anonymity-sensitive headers cannot reach
the target.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds anonymous, opt-out usage analytics to the
stashCLI, astash telemetry [status|enable|disable]command to manage it, and the Cloudflare reverse proxy the CLI reports through. Stacked on the EQL v3 RC (feat/eql-v3-text-search-schema, #535).What it collects
Coarse events only: command name, CLI version, OS/arch, success/failure, duration, and the error class on failure. A property allowlist enforced at the emitter boundary (
sanitizeintelemetry/index.ts) makes it structurally impossible for plaintext, schema, table/column names, connection strings, or argument values to leave the machine — this is the load-bearing guarantee for a zero-knowledge product, and it has direct tests.Behaviour
DO_NOT_TRACK=1(cross-tool standard),STASH_TELEMETRY_DISABLED=1, CI auto-detect, and persistedstash telemetry disable.posthog-nodeevents-only, flush bounded by a 1.5s timeout, all failures swallowed.~/.cipherstash/telemetry.json— a non-secret file distinct from the auth credentials.Proxy (
infra/telemetry-proxy/)A Cloudflare Worker forwards
telemetry.cipherstash.com→ PostHog, with the target host in aPOSTHOG_HOSTvariable. This makes the deferred US→EU PostHog migration a var-change + redeploy with no CLI re-release — every installedstashfollows the proxy. It's an allowlisted forwarder, not an open proxy.Ships dormant
The embedded PostHog key is a placeholder, so telemetry is fully inert (
resolveStatus→unconfigured, no banner, no sends) until the real public write-only key is embedded at release. Safe to merge before the infra is live.Tests
423 CLI tests pass, including 16 new ones covering gate precedence, CI detection, the allowlist (asserts
table/databaseUrl/plaintextetc. are dropped), and state roundtrip/corruption recovery.Follow-ups (outside this PR)
infra/telemetry-proxy/README.md) and embed the real PostHog key at release.cipherstash.com/docs/reference/clipage the first-run notice links to.Notes
posthog-nodewas already a vetted dependency; it's newly used here.skills/stash-cli/SKILL.md(new command + Telemetry section) — hence thestashchangeset.https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Summary by CodeRabbit
New Features
stashCLI.stash telemetry status|enable|disablefor managing analytics preferences.Documentation