feat: telemetry opt-out command; remove hidden records commands - #207
feat: telemetry opt-out command; remove hidden records commands#207Fermionic-Lyu wants to merge 1 commit into
Conversation
Telemetry opt-out (usage analytics kill switch): - New `insforge telemetry status|enable|disable` command. `disable` persists telemetry_disabled in ~/.insforge/config.json; status reports the effective state and what decides it (env var, config, default). - isTelemetryDisabled() in src/lib/analytics.ts honors, in order: DO_NOT_TRACK (consoledonottrack.com convention), INSFORGE_TELEMETRY_DISABLED, then the persisted config flag. It gates the PostHog client AND the legacy reportCliUsage path, so one switch covers every usage-tracking emitter. A corrupt config file reads as enabled — telemetry handling must never break the CLI. - The telemetry command itself deliberately emits no analytics events. - Documented in README (command + env vars) and DEVELOPMENT.md §2, which now requires new telemetry emitters to check the kill switch. Records removal: - Delete the hidden `records` command group (list/create/update/delete). It was never supported for direct use; table data goes through `db query`. README note updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughChangesTelemetry opt-out
Records command removal
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryAdds a persistent and environment-controlled telemetry opt-out while removing the hidden records command group.
Confidence Score: 3/5The telemetry opt-out needs correction before merging because a corrupt global config prevents users from disabling analytics and environment-overridden disable output reports the wrong source. The analytics emitters themselves honor the new gate, but the management command does not share its corrupt-config recovery and emits inconsistent machine-readable status under environment overrides. Files Needing Attention: src/commands/telemetry.ts, src/commands/telemetry.test.ts
|
| Filename | Overview |
|---|---|
| src/commands/telemetry.ts | Adds telemetry state management, but corrupt config blocks all subcommands and disable can report the wrong deciding source. |
| src/lib/analytics.ts | Adds a shared opt-out resolver and rechecks it before every PostHog capture. |
| src/lib/skills.ts | Applies the shared telemetry gate to legacy OSS CLI-usage reporting. |
| src/index.ts | Registers the telemetry command and removes the hidden records command group. |
| src/types.ts | Extends global configuration with the optional persistent telemetry flag. |
| src/commands/telemetry.test.ts | Covers standard command behavior but omits corrupt-config command handling and environment-overridden disable output. |
| src/lib/analytics.test.ts | Covers environment precedence, persistent opt-out, PostHog gating, and corrupt-config fail-safe behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Telemetry check] --> B{DO_NOT_TRACK truthy?}
B -->|Yes| X[Disabled by DO_NOT_TRACK]
B -->|No| C{INSFORGE_TELEMETRY_DISABLED truthy?}
C -->|Yes| Y[Disabled by INSFORGE_TELEMETRY_DISABLED]
C -->|No| D{Global config parses?}
D -->|No| E[Enabled by fail-safe]
D -->|Yes| F{telemetry_disabled is true?}
F -->|Yes| Z[Disabled by config]
F -->|No| G[Enabled by default]
Reviews (1): Last reviewed commit: "feat: telemetry opt-out command; remove ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .action((_opts, cmd) => { | ||
| const { json } = getRootOpts(cmd); | ||
| try { | ||
| const config = getGlobalConfig(); |
There was a problem hiding this comment.
When ~/.insforge/config.json contains invalid JSON, getGlobalConfig() throws before telemetry disable, enable, or status can complete, causing all three commands to exit with an error and preventing the user from persisting an opt-out until they manually repair the file. This path needs the same corrupt-config fallback used by isTelemetryDisabled().
Knowledge Base Used: CLI Entrypoint and Authentication
| config.telemetry_disabled = true; | ||
| saveGlobalConfig(config); | ||
| if (json) { | ||
| outputJson({ enabled: false, source: 'config' }); |
There was a problem hiding this comment.
When telemetry disable --json runs with DO_NOT_TRACK or INSFORGE_TELEMETRY_DISABLED set, this hard-coded response reports source: "config" even though the environment variable has precedence, causing its output to disagree with an immediate telemetry status --json result.
Knowledge Base Used: Account Management
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review: telemetry opt-out + records removal
Summary: A clean, well-tested telemetry opt-out implemented as a single kill switch that correctly gates every analytics emitter, plus a tidy removal of the dead hidden records group — no blocking issues.
Requirements context
No matching spec/plan found under docs/ — the repo keeps specs in docs/specs/ (only diagnose and db-migrations design docs exist; there is no docs/superpowers/). Assessed against the PR description, DEVELOPMENT.md §2, and the surrounding code.
Findings
Critical
(none)
Suggestion
-
Software engineering — duplicated falsy-env parsing (
src/commands/telemetry.ts:9-15vssrc/lib/analytics.ts:8-31).telemetry.tsreimplements the "set to anything except an explicit falsy value" rule (envOverride()/flag()) that already lives inanalytics.ts(envFlag()/isTelemetryDisabled()). The command needs the source name (DO_NOT_TRACKvsINSFORGE_TELEMETRY_DISABLED), whichisTelemetryDisabled()doesn't expose, so the duplication is understandable — but two independent copies of the same precedence + falsy rule can drift, and if they do,telemetry statuswould report asourcethat disagrees with what actually gates emission. Consider exporting a single resolver fromanalytics.ts(e.g. one that returns{disabled, source}) and having bothisTelemetryDisabled()and the command consume it. -
Functionality — corrupt config isn't fail-safe in the command path (
src/commands/telemetry.ts:33-38,38-50,52-73).isTelemetryDisabled()deliberately treats a corrupt~/.insforge/config.jsonas "enabled" (try/catch), so telemetry keeps flowing. Butstatus/disable/enablecallgetGlobalConfig()unguarded, andconfig.ts:29-30JSON.parses with no try/catch — so with a corrupt config,telemetry disablethrows and exits instead of opting the user out. Net effect: the one state where telemetry is silently on (corrupt config → fail-safe "enabled") is also the state where the persistent opt-out command fails, leaving only the env var as an escape. Low blast radius, but mirroring the PR's own fail-safe stance (e.g.disablestarting from a default config when the file is unreadable) would be more robust.
Information
-
Performance —
isTelemetryDisabled()re-reads config per emission (src/lib/analytics.ts:23-31,36).getClient()now callsgetGlobalConfig()→ synchronousreadFileSync+JSON.parseon every telemetry event, and the result isn't memoized even though the PostHogclientis cached. Negligible for a one-shot CLI (typically one event per invocation), noting only in case an event ever fires in a loop. -
Security — no concerns. No new user input reaches SQL/shell/HTTP; the command only writes a boolean to config. No secrets/PII newly logged. This PR is net-positive for privacy (adds opt-out) and the
recordsremoval shrinks the OSS-fetch surface. No new dependencies. -
Software engineering — coverage & conventions are solid. 12 new tests exercise env precedence (including explicit-falsy
0/false), config opt-out, corrupt-config fail-safe, PostHog gating, and theenableenv-override error path. Import style (.jssuffixes,node:prefixes), error handling (handleError/CLIError), and the "no self-tracking" note are consistent with the codebase. The kill switch is genuinely centralized — verifiedcommand-telemetry.ts(trackCommandUsage/trackTopLevelUsage) routes throughcaptureEvent → getClient(), so it's gated too, and no orphanedrecordsreferences remain after the deletion.DEVELOPMENT.mdnow documents the opt-out contract for future emitters.
Verdict
approved (no Critical findings). Suggestions above are non-blocking. Note: this is the bot's assessment for reporting — the GitHub green-check approval remains a separate human action.
![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
🤖 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 `@src/lib/analytics.ts`:
- Around line 14-31: Centralize telemetry-state resolution in
src/lib/analytics.ts#L14-L31 by exporting envFlag and adding a shared resolver
that applies the environment and config rules with consistent corrupt-config
handling; preserve the intended fail-safe behavior for unreadable configuration.
In src/commands/telemetry.ts#L9-L29, remove the duplicated envOverride and
direct config lookup from resolveStatus and reuse the shared analytics resolver
so telemetry status and emitters always report the same state.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 788c8adb-dc1f-4812-ba01-bcf30768fc2f
📒 Files selected for processing (13)
DEVELOPMENT.mdREADME.mdsrc/commands/records/create.tssrc/commands/records/delete.tssrc/commands/records/list.tssrc/commands/records/update.tssrc/commands/telemetry.test.tssrc/commands/telemetry.tssrc/index.tssrc/lib/analytics.test.tssrc/lib/analytics.tssrc/lib/skills.tssrc/types.ts
💤 Files with no reviewable changes (4)
- src/commands/records/update.ts
- src/commands/records/delete.ts
- src/commands/records/create.ts
- src/commands/records/list.ts
| /** | ||
| * Usage-tracking kill switch, honored by every telemetry emitter (PostHog | ||
| * here, `reportCliUsage` in skills.ts). Resolution order: | ||
| * 1. `DO_NOT_TRACK` — the cross-tool convention (consoledonottrack.com) | ||
| * 2. `INSFORGE_TELEMETRY_DISABLED` — per-run / CI override | ||
| * 3. `telemetry_disabled` in ~/.insforge/config.json — persistent opt-out | ||
| * managed by `insforge telemetry enable|disable|status` | ||
| * A corrupt config file must never break the CLI, so it reads as "enabled". | ||
| */ | ||
| export function isTelemetryDisabled(): boolean { | ||
| if (envFlag(process.env.DO_NOT_TRACK)) return true; | ||
| if (envFlag(process.env.INSFORGE_TELEMETRY_DISABLED)) return true; | ||
| try { | ||
| return getGlobalConfig().telemetry_disabled === true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Telemetry-state resolution is duplicated between analytics.ts and telemetry.ts, with diverging corrupt-config behavior. analytics.ts owns the real kill switch (isTelemetryDisabled/envFlag) but doesn't export it for reuse; telemetry.ts re-implements the same env checks (envOverride) and its own config lookup (resolveStatus) without the same fail-safe catch, so a corrupt config.json produces an inconsistent outcome: analytics silently stays enabled, while telemetry status throws a hard error. This also means the two implementations can drift out of sync if the flag rules ever change in only one place.
src/lib/analytics.ts#L14-L31: exportenvFlag(and ideally aresolveTelemetryState()helper that also returns the deciding source) sotelemetry.tscan reuse it instead of re-implementing it; also reconsider whether the catch should fail closed (treat unreadable config as still-disabled) rather than silently re-enabling a previously persisted opt-out.src/commands/telemetry.ts#L9-L29: replaceenvOverride()/the config lookup inresolveStatus()with the shared helper fromanalytics.ts, and wrap the config read in the same fail-safe catch sotelemetry statuscan't diverge from the actual kill switch during a corrupt config.
📍 Affects 2 files
src/lib/analytics.ts#L14-L31(this comment)src/commands/telemetry.ts#L9-L29
🤖 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 `@src/lib/analytics.ts` around lines 14 - 31, Centralize telemetry-state
resolution in src/lib/analytics.ts#L14-L31 by exporting envFlag and adding a
shared resolver that applies the environment and config rules with consistent
corrupt-config handling; preserve the intended fail-safe behavior for unreadable
configuration. In src/commands/telemetry.ts#L9-L29, remove the duplicated
envOverride and direct config lookup from resolveStatus and reuse the shared
analytics resolver so telemetry status and emitters always report the same
state.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/telemetry.ts">
<violation number="1" location="src/commands/telemetry.ts:27">
P2: `telemetry status` fails when the global config is malformed, despite the fail-safe policy that corrupt config reads as enabled. Catch the config read here too so status remains usable for users trying to diagnose or recover telemetry settings.</violation>
<violation number="2" location="src/commands/telemetry.ts:70">
P2: The JSON output here hard-codes `source: 'config'` but this disagrees with `telemetry status --json` when an env var like `DO_NOT_TRACK` or `INSFORGE_TELEMETRY_DISABLED` is set. Since env vars take precedence over config in `resolveStatus()`, the disable output should reflect the actual deciding source — consider calling `resolveStatus()` after the save and returning its result, so both commands report a consistent view.</violation>
</file>
<file name="src/lib/analytics.test.ts">
<violation number="1" location="src/lib/analytics.test.ts:60">
P3: Missing precedence test: env vars should win over the persistent config, but no test asserts this. Consider adding a test that sets DO_NOT_TRACK=1 (or INSFORGE_TELEMETRY_DISABLED=true) while getGlobalConfig returns telemetry_disabled: false and verifies isTelemetryDisabled() is still true. This would lock in the resolution order documented in the PR description.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| function resolveStatus(): TelemetryStatus { | ||
| const env = envOverride(); | ||
| if (env) return { enabled: false, source: env }; | ||
| const disabled = getGlobalConfig().telemetry_disabled === true; |
There was a problem hiding this comment.
P2: telemetry status fails when the global config is malformed, despite the fail-safe policy that corrupt config reads as enabled. Catch the config read here too so status remains usable for users trying to diagnose or recover telemetry settings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/telemetry.ts, line 27:
<comment>`telemetry status` fails when the global config is malformed, despite the fail-safe policy that corrupt config reads as enabled. Catch the config read here too so status remains usable for users trying to diagnose or recover telemetry settings.</comment>
<file context>
@@ -0,0 +1,106 @@
+function resolveStatus(): TelemetryStatus {
+ const env = envOverride();
+ if (env) return { enabled: false, source: env };
+ const disabled = getGlobalConfig().telemetry_disabled === true;
+ return { enabled: !disabled, source: disabled ? 'config' : 'default' };
+}
</file context>
| config.telemetry_disabled = true; | ||
| saveGlobalConfig(config); | ||
| if (json) { | ||
| outputJson({ enabled: false, source: 'config' }); |
There was a problem hiding this comment.
P2: The JSON output here hard-codes source: 'config' but this disagrees with telemetry status --json when an env var like DO_NOT_TRACK or INSFORGE_TELEMETRY_DISABLED is set. Since env vars take precedence over config in resolveStatus(), the disable output should reflect the actual deciding source — consider calling resolveStatus() after the save and returning its result, so both commands report a consistent view.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/telemetry.ts, line 70:
<comment>The JSON output here hard-codes `source: 'config'` but this disagrees with `telemetry status --json` when an env var like `DO_NOT_TRACK` or `INSFORGE_TELEMETRY_DISABLED` is set. Since env vars take precedence over config in `resolveStatus()`, the disable output should reflect the actual deciding source — consider calling `resolveStatus()` after the save and returning its result, so both commands report a consistent view.</comment>
<file context>
@@ -0,0 +1,106 @@
+ config.telemetry_disabled = true;
+ saveGlobalConfig(config);
+ if (json) {
+ outputJson({ enabled: false, source: 'config' });
+ } else {
+ outputSuccess('Telemetry disabled. No usage analytics will be sent.');
</file context>
| @@ -0,0 +1,84 @@ | |||
| import { describe, it, expect, vi, beforeEach, afterEach, type Mock } from 'vitest'; | |||
There was a problem hiding this comment.
P3: Missing precedence test: env vars should win over the persistent config, but no test asserts this. Consider adding a test that sets DO_NOT_TRACK=1 (or INSFORGE_TELEMETRY_DISABLED=true) while getGlobalConfig returns telemetry_disabled: false and verifies isTelemetryDisabled() is still true. This would lock in the resolution order documented in the PR description.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/analytics.test.ts, line 60:
<comment>Missing precedence test: env vars should win over the persistent config, but no test asserts this. Consider adding a test that sets DO_NOT_TRACK=1 (or INSFORGE_TELEMETRY_DISABLED=true) while getGlobalConfig returns telemetry_disabled: false and verifies isTelemetryDisabled() is still true. This would lock in the resolution order documented in the PR description.</comment>
<file context>
@@ -0,0 +1,84 @@
+ it('treats explicit falsy env values as not opting out', async () => {
+ vi.stubEnv('DO_NOT_TRACK', '0');
+ vi.stubEnv('INSFORGE_TELEMETRY_DISABLED', 'false');
+ const { isTelemetryDisabled } = await loadAnalytics();
+ expect(isTelemetryDisabled()).toBe(false);
+ });
</file context>
2 participants
Summary
Two changes:
1.
insforge telemetry— usage-analytics opt-outDesign:
isTelemetryDisabled()insrc/lib/analytics.tsgates both the PostHog client and the legacyreportCliUsageOSS path. DEVELOPMENT.md §2 now requires any new telemetry emitter to check it.DO_NOT_TRACK(the cross-tool convention) →INSFORGE_TELEMETRY_DISABLED(per-run/CI) →telemetry_disabledin the global config (persistent, managed by this command). Explicit falsy values (0,false, empty) don't opt out.telemetry enableerrors if an env var still forces telemetry off, instead of claiming it's back on;statusreports the deciding source.reportAgentConnected(dashboard "agent connected" onboarding signal) is deliberately not gated — it drives product behavior, not analytics.2. Remove the hidden
recordscommand groupNever supported for direct use (hidden since introduction); table data goes through
db query. Deletessrc/commands/records/, the registration insrc/index.ts, and updates the README note.Test plan
recordserrors as an unknown command;telemetry status --jsonreportsdefault/INSFORGE_TELEMETRY_DISABLED/configsources correctly; disable→enable round-trip verified against a real~/.insforge/config.json.🤖 Generated with Claude Code
Summary by cubic
Adds
insforge telemetryto manage anonymous usage analytics with a single opt‑out switch. Removes the hiddenrecordscommands; usedb queryfor table data.New Features
DO_NOT_TRACK→INSFORGE_TELEMETRY_DISABLED→telemetry_disabledin~/.insforge/config.json.reportCliUsage. New emitters must check it (see DEVELOPMENT.md).telemetry status|enable|disable;statusexplains the deciding source, andenablewarns if an env var still keeps it off. The command does not self-track.Refactors
recordsgroup (list/create/update/delete) and its registration. Docs now direct table access throughdb query.Written for commit 8b991d6. Summary will update on new commits.
Note
Add
telemetryCLI command with opt-out support and remove hiddenrecordscommandstelemetrycommand group withstatus,enable, anddisablesubcommands that persist opt-out state in the global config via atelemetry_disabledflag inGlobalConfig.isTelemetryDisabled()in analytics.ts as a centralized kill switch that checksDO_NOT_TRACKandINSFORGE_TELEMETRY_DISABLEDenv vars and the persisted config; bothgetClient()andreportCliUsage()now respect it.recordsCRUD command group (create,delete,list,update) from the CLI; users are directed todb queryinstead.Macroscope summarized 8b991d6.
Summary by CodeRabbit
telemetry status,telemetry enable, andtelemetry disableCLI commands.recordsCLI commands for listing, creating, updating, and deleting records. Usedb queryinstead.