feat(cli): anonymous opt-out usage analytics + stash telemetry#644
Merged
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: fcc3047 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
|
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:
📝 WalkthroughWalkthroughAdds anonymous, opt-out telemetry to the ChangesCLI telemetry
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StashCLI
participant TelemetryModule
participant PostHogClient
participant TelemetryProxy
StashCLI->>TelemetryModule: initialize and resolve status
StashCLI->>TelemetryModule: track command_invoked
TelemetryModule->>PostHogClient: capture sanitized event
PostHogClient->>TelemetryProxy: send event
StashCLI->>TelemetryModule: shutdown and flush
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
Symmetric with STASH_POSTHOG_KEY: the host now defaults to the Cloudflare proxy (telemetry.cipherstash.com) but can be overridden for testing against a real PostHog ingestion endpoint before the proxy is deployed, or for self-hosting. Verified the CLI POSTs the event batch to the configured host. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Add a coarse, fixed-enum `caller` property to command telemetry so we can gauge how much stash usage is agent-driven — a lot of it is our own skills being run by a customer's coding agent. resolveCaller() recognises Claude Code (CLAUDECODE / CLAUDE_CODE_ENTRYPOINT), Cursor (CURSOR_TRACE_ID), and Codex (CODEX_SANDBOX) by the env var each sets, falls back to `editor` for a VS Code-family terminal (TERM_PROGRAM=vscode), then to `interactive`/`non-interactive` by stdin TTY. Only the fixed label is emitted, never the raw env value — some carry session/trace ids — so the anonymity contract holds. `caller` is added to the emitter allowlist. Updates the stash-cli skill disclosure and the changeset field list to match. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/stash-cli/SKILL.md (1)
160-163: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid unpinned
npx stashcommands in the new workflow instructions.These commands can download the mutable latest package when no local installation exists. Use the project-local binary after initialization or pin the exact CLI version for one-shot invocations, especially for privacy-sensitive opt-out controls.
🤖 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 `@skills/stash-cli/SKILL.md` around lines 160 - 163, Update the telemetry workflow instructions around the npx stash commands to avoid unpinned invocations: use the project-local CLI binary after initialization, or specify an exact CLI version for one-shot commands. Apply this consistently to telemetry status, enable, and disable examples, preserving the existing opt-out behavior.Source: Linters/SAST tools
🤖 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 `@skills/stash-cli/SKILL.md`:
- Line 150: Update the first-run notice wording in the documentation to specify
that it is shown and persisted only on the first interactive TTY run, rather
than on the first run generally.
- Around line 145-150: Update the telemetry disclosure in the CLI analytics
description to explicitly mention the anonymous salted machine identifier. State
that it is used only to aggregate anonymous events, while preserving the
existing list of excluded sensitive data and first-run notice behavior.
---
Outside diff comments:
In `@skills/stash-cli/SKILL.md`:
- Around line 160-163: Update the telemetry workflow instructions around the npx
stash commands to avoid unpinned invocations: use the project-local CLI binary
after initialization, or specify an exact CLI version for one-shot commands.
Apply this consistently to telemetry status, enable, and disable examples,
preserving the existing opt-out behavior.
🪄 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: 92ab9e58-21ed-44b1-88a5-6921349560fd
📒 Files selected for processing (6)
.changeset/cli-anonymous-telemetry.mdpackages/cli/src/config/__tests__/tty.test.tspackages/cli/src/config/tty.tspackages/cli/src/telemetry/__tests__/telemetry.test.tspackages/cli/src/telemetry/index.tsskills/stash-cli/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/cli-anonymous-telemetry.md
- packages/cli/src/config/tty.ts
- packages/cli/src/telemetry/tests/telemetry.test.ts
- packages/cli/src/telemetry/index.ts
…repo The Cloudflare Worker that terminates telemetry.cipherstash.com is deployed platform infrastructure, not part of the published stash package, so it belongs with the rest of our infra in cipherstash-suite (infra/telemetry-proxy) rather than shipping in this repo. No CLI behaviour changes: the CLI still targets https://telemetry.cipherstash.com and stays dormant until a key is embedded. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Wire up the injection the placeholder always promised. tsup's esbuild `define` replaces the `__STASH_POSTHOG_KEY__` identifier with the key from the STASH_POSTHOG_KEY env var; release.yml feeds it from a public repo *variable* (vars.STASH_POSTHOG_KEY), so ONLY the official release build embeds a key. Dev, fork, and CI builds bake in an empty string and ship telemetry-dormant. Applied to both bundles; turbo.json lists the var on `build` so the cache key tracks it. Also fixes a latent bug the injection would have exposed: resolveStatus compared the resolved key against EMBEDDED_KEY, which once injected holds the real key — so an embedded build with no env override would have read as `unconfigured`. It now compares against a dedicated PLACEHOLDER_KEY sentinel (the un-rewritten string literal), so an injected build is correctly enabled. New tests cover the placeholder-passthrough (dormant) and injected-key (enabled) paths. The go-live switch is setting the repo variable; until then every release stays dormant. Verified both ways: dormant build reports "not active in this build", injected build reports "Telemetry is enabled" with no runtime env override. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Ten findings from the high-effort review, all fixed:
1. subcommand value leak (privacy): trackCommand emitted argv[1] verbatim, so
`stash wizard "<prompt>"` sent the natural-language prompt (table/column
names) to PostHog. New classify-command.ts coerces command/subcommand to the
registry's known verbs before emit; anything else becomes `<other>`.
2. non-TTY never activated: the first-run notice only persisted noticeShownAt on
a TTY, so agents/piped callers stayed firstRun forever and never sent — the
exact population the `caller` dimension measures. Now the disclosure is shown
(to stderr) and persisted on every first run, TTY or not.
3. process.exit bypassed the tracking finally: commands exiting via process.exit
emitted nothing and never flushed. run() now intercepts process.exit with an
ExitSignal sentinel that unwinds through the finally (track + flush) before
the real exit, honouring the exit code. Degrades gracefully if a command
catch swallows it.
4. empty-string key enabled telemetry: STASH_POSTHOG_KEY='' slipped past the
nullish fallback; projectKey() now trims and treats empty as unset.
5. first-run notice could show twice: maybeShowFirstRunNotice didn't reassign the
state cache, so a same-run setTelemetryDisabled clobbered noticeShownAt. Fixed.
6. isCiEnv widening leaked into prompt gating: reverted the shared isCiEnv to its
narrow (CI-only) behaviour used by region/database-url; telemetry uses a new
isCiEnvBroad. Reverted the now-unnecessary test neutralizations.
7. eager cost on every invocation: posthog-node is now a dynamic import inside
getClient (loaded only when an event is actually sent), and the state file
read is lazy — both off the --version/--help fast paths.
8. shutdown could linger: pass requestTimeout to PostHog so a black-holed
endpoint can't keep the socket alive past the flush window.
9. dormant tests depended on ambient STASH_POSTHOG_KEY: clearGateEnv now stubs it.
10. lifecycle path was untested: new telemetry-lifecycle.test.ts covers the
emitter/flush contract (no-op when disabled/firstRun, swallows throws, safe
double-shutdown, non-TTY notice persistence, bounded flush on a hung endpoint)
and classify-command.test.ts covers the value allowlist.
Verified end-to-end against a local capture: a process.exit command is now
tracked with its exit code preserved, and a wizard prompt containing a
table.column is scrubbed to `<other>` (0 leakage). 453 unit tests pass.
Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
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/src/telemetry/__tests__/telemetry-lifecycle.test.ts`:
- Around line 1-4: Add the `dotenv/config` import at the top of the telemetry
lifecycle test file before the existing imports, preserving the current
`CI_ENV_VARS` and `TelemetryState` imports.
🪄 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: ee1d85ca-7e06-4e8c-9821-ced9514a2439
📒 Files selected for processing (7)
packages/cli/src/bin/main.tspackages/cli/src/config/tty.tspackages/cli/src/telemetry/__tests__/classify-command.test.tspackages/cli/src/telemetry/__tests__/telemetry-lifecycle.test.tspackages/cli/src/telemetry/__tests__/telemetry.test.tspackages/cli/src/telemetry/classify-command.tspackages/cli/src/telemetry/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli/src/bin/main.ts
- packages/cli/src/telemetry/tests/telemetry.test.ts
- packages/cli/src/telemetry/index.ts
… harden telemetry per review round 2 The round-1 fix intercepted process.exit globally with a thrown signal. The second review pass proved that unsafe: @clack/core calls process.exit(0) from a keypress handler (Ctrl+C during any spinner became an uncaughtException crash), and five broad catches (init's install-eql, plan, status, impl, the jiti config loader) swallowed the signal — init continued past an explicit cancel, impl skipped the cutover deploy-gate, and commands that recovered by design exited with a stale latched code. New design: no global interception. First-party sites verified to unwind cleanly to run() throw `CliExit(code)` (new cli/exit.ts) — main.ts's own unknown-command/subcommand defaults, requireValue, requireStack, the telemetry command, and the outermost CancelledError handlers of init/plan/impl (cancels are now tracked). Deep process.exit sites revert to pre-branch behaviour: immediate, uninterceptable, untracked — documented as a known measurement gap in the telemetry module doc and packages/cli/AGENTS.md. Also fixed from the review: - shutdownTelemetry: the capture enqueue (containing the dynamic posthog-node import) now sits INSIDE the Promise.race, so the 1500ms bound covers everything that can stall; the flush promise carries its own rejection handler so a timeout win can't leave an unhandledRejection. - First-run notice persists BEFORE printing: an unwritable HOME no longer prints the notice on every run forever — no persist, no print, no telemetry. - pty e2e harness sets STASH_TELEMETRY_DISABLED=1 so an ambient STASH_POSTHOG_KEY can't make tests send real events against the developer's real ~/.cipherstash. - errorType is now a closed vocabulary (classifyErrorType): user-defined error class names from jiti-loaded config code collapse to `<other>`. - Wizard analytics aligned with the CLI's privacy contract: honors DO_NOT_TRACK / STASH_TELEMETRY_DISABLED / CI, random per-session id instead of sha256(username@hostname), geoip off, and error events carry fixed labels / class names instead of raw messages. - Docs de-staled: packages/cli/AGENTS.md telemetry section rewritten (it said the CLI had no telemetry), CI_ENV_VARS/resolveStatus/pty.ts comments now describe the isCiEnv (narrow, prompts) vs isCiEnvBroad (telemetry) split, and the changeset names the value-coercion boundary. - Tests: narrow-vs-broad isCiEnv contract pinned; classifyErrorType covered; notice write-failure covered; stderr.isTTY save/restore no longer creates a read-only own property; smoke e2e covers `telemetry status` and the bogus-subcommand CliExit path. Verified: 460 unit + 57 pty e2e tests pass; local capture shows unknown command and telemetry bogus-sub tracked with correct exit codes, wizard free-text still scrubbed to `<other>` (0 leakage), and a read-only-HOME first run prints nothing. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
CodeRabbit flagged that the SKILL.md disclosure lists event fields but never mentions the distinct_id. Add it — with accurate wording: it is a locally generated random UUID stored in ~/.cipherstash/telemetry.json, used only to de-duplicate events in aggregate, and NOT a salted/derived machine id (the review's suggested phrasing described a mechanism we deliberately don't use). Mirrored in the changeset so the release notes carry the same disclosure. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Contributor
Author
|
Comprehensive human and AI review. Multiple agents: Copilot, Coderabbit and Claude (Fable) - all findings addressed. |
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
stash telemetry status|enable|disablefor managing analytics preferences.Documentation
Tests