feat(cli): track config apply/plan/export in PostHog#131
Conversation
Wire the local-file config workflow into the PostHog analytics path that already covers `diagnose` and `payments`. Emits `cli_config_invoked` from `apply`, `plan`, and `export` with subcommand, outcome, flag shape, change counts, and the TOML schema sections that were touched. No SQL, TOML contents, credentials, or env() values cross the wire. Adds a hard opt-out: `INSFORGE_TELEMETRY=0` (also accepts `false`/`no`, case-insensitive). When set, `getClient()` short-circuits before constructing the PostHog client. Covered by `analytics.test.ts`. Docs: README "Analytics" section rewritten with the property allowlist and opt-out instructions; DEVELOPMENT.md notes the convention for new commands. The companion OSS docs page lives in InsForge/insforge. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
WalkthroughThis PR adds a trackConfig helper and instruments config commands (apply, export, plan) to emit analytics events for distinct outcomes, ensures safe analytics shutdown, and updates README to clarify telemetry capture scope as non-sensitive metadata only. ChangesConfig Command Analytics Instrumentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🤖 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 `@README.md`:
- Line 677: Update the README's description of the telemetry "outcome" field to
enumerate all emitted values used by config events (not just "success / error /
aborted"); include "applied", "dry_run", "no_changes", and "all_skipped"
alongside the existing values so the documented list for the outcome field
matches the actual emitted values and avoids telemetry contract confusion.
In `@src/commands/config/apply.ts`:
- Around line 33-35: Move the getProjectConfig() call into the try block so its
exceptions are routed through the same error/finally handling: declare a
variable (e.g., let projectConfig) before the try, then inside the try assign
projectConfig = getProjectConfig() (before or after await requireAuth() as
needed), keeping the rest of the logic intact; this ensures any throw from
getProjectConfig() is caught by handleError(...) and the finally block.
Reference: getProjectConfig(), requireAuth(), handleError, finally.
In `@src/commands/config/export.ts`:
- Around line 24-26: Move the call to getProjectConfig() into the existing try
block so any exception it throws is handled by handleError(...) and the finally
block; specifically, inside the function that declares projectConfig and calls
requireAuth(), remove the top-level getProjectConfig() invocation and instead
call and assign projectConfig within the try (before or after await
requireAuth()), ensuring the same projectConfig variable name is used so the
rest of the export command (and the finally cleanup) still operates correctly.
In `@src/commands/config/plan.ts`:
- Around line 25-27: Move the call to getProjectConfig() into the try block so
exceptions go through the centralized error rendering and finally cleanup:
declare a variable (e.g., let projectConfig;) before the try, then inside the
try assign projectConfig = getProjectConfig() immediately before await
requireAuth(); keep references to projectConfig elsewhere unchanged so
subsequent code uses the initialized variable.
🪄 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: ac64204b-c552-48e7-8002-55545253394c
📒 Files selected for processing (7)
DEVELOPMENT.mdREADME.mdsrc/commands/config/apply.tssrc/commands/config/export.tssrc/commands/config/plan.tssrc/lib/analytics.test.tssrc/lib/analytics.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 7 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/config/plan.ts">
<violation number="1" location="src/commands/config/plan.ts:25">
P2: `getProjectConfig()` is called outside the `try` block, so malformed local project config JSON can throw before command error handling runs.</violation>
</file>
<file name="src/commands/config/export.ts">
<violation number="1" location="src/commands/config/export.ts:24">
P2: `getProjectConfig()` is called outside the command's `try/catch`, so a malformed local `.insforge/project.json` can throw an uncaught error before normal CLI error handling runs.</violation>
</file>
<file name="src/commands/config/apply.ts">
<violation number="1" location="src/commands/config/apply.ts:33">
P2: `getProjectConfig()` is called outside the `try`, so malformed local config can crash with an uncaught exception before `handleError` runs.</violation>
<violation number="2" location="src/commands/config/apply.ts:159">
P2: `shutdownAnalytics()` in this `finally` block will not run on error paths because `handleError()` exits the process first.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Re-trigger cubic
Greptile SummaryThis PR wires PostHog telemetry into the three
Confidence Score: 5/5Additive analytics instrumentation with no changes to command logic; safe to merge. The changes are purely additive: No files require special attention; the one minor outcome-naming inconsistency in Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI Action (apply/plan/export)
participant PH as trackConfig()
participant PHC as PostHog Client (queue)
participant SA as shutdownAnalytics()
CLI->>CLI: getProjectConfig() [inside try]
alt Success path
CLI->>PH: "trackConfig(subcommand, config, {outcome})"
PH->>PHC: captureEvent() enqueue
CLI->>SA: (finally) shutdownAnalytics()
SA->>PHC: client.shutdown() flush
else Early return (dry-run / aborted / no-changes)
CLI->>PH: "trackConfig(subcommand, config, {outcome})"
PH->>PHC: captureEvent() enqueue
CLI->>CLI: return
CLI->>SA: (finally) shutdownAnalytics()
SA->>PHC: client.shutdown() flush
else Error path
CLI->>PH: trackConfig(subcommand, config, outcome error)
PH->>PHC: captureEvent() enqueue
CLI->>SA: (catch) shutdownAnalytics() flush + null client
CLI->>CLI: handleError() process.exit()
CLI->>SA: (finally) shutdownAnalytics() no-op client is null
end
Reviews (3): Last reviewed commit: "review fixes: hoist getProjectConfig + f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The published npm package already ships with the PostHog key baked in at build time, so a runtime env var doesn't actually change the privacy posture for end users in any meaningful way. The build-from-source no-op (missing `POSTHOG_API_KEY`) remains the real opt-out for forks and local dev. Removes: telemetryDisabled() gate in analytics.ts, the analytics.test.ts suite that only covered that gate, the README "how to disable" copy and env var row, the DEVELOPMENT.md bullet. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
tonychang04
changed the title
Addresses the review comments on #131. - Move `getProjectConfig()` inside the `try` block in apply/plan/export so a malformed `.insforge/project.json` flows through `handleError()` instead of crashing with a raw `SyntaxError` from `JSON.parse`. (CodeRabbit, cubic, greptile — same finding across 6 comments.) - Flush analytics before `process.exit()` on error paths: catch path now calls `await shutdownAnalytics()` immediately before `handleError()`. `handleError()` exits synchronously, so the queued `error` event would otherwise never make it to PostHog. The finally block stays as a safety net; `shutdownAnalytics()` is now idempotent (nulls the client after first call) so the double-call is harmless. (cubic.) - Reorder each early-return branch to call `reportCliUsage` before `trackConfig` so a `reportCliUsage` throw doesn't fire a second `cli_config_invoked` (the normal-outcome event + the catch's `error` event). (greptile.) - Add the missing `reportCliUsage('cli.config.export', true)` to the `export` aborted prompt path so it matches the symmetric `apply` aborted path. (greptile.) - README: expand the documented `outcome` value list to match what the config events actually emit (`applied`, `dry_run`, `no_changes`, `all_skipped` alongside the existing values). (CodeRabbit.) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed all the review comments in 606aaad. Summary:
Double Asymmetric README Build clean, 0 lint errors, 373/373 unit tests pass. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
2 participants
Summary
cli_config_invokedfrominsforge config apply/plan/export(subcommand, outcome, flag shape, change counts,sections_changedfrom the TOML schema). Same analytics path asdiagnoseandpayments.README.md; adds the convention toDEVELOPMENT.md. Companion OSS docs page in InsForge/InsForge#1279.What is captured
subcommand,outcome(success/applied/aborted/dry_run/no_changes/error/all_skipped).dry_run,json_mode,auto_approved,force,changes_count,applied_count,skipped_count,sections_changed(TOML schema keys only, e.g.auth.smtp).project_id,project_name,org_id,region,oss_mode.What is never captured
SQL, TOML contents, credentials, secrets,
env()resolved values, file paths, free text. Full allowlist lives insrc/lib/analytics.ts.Test plan
npm run buildcleannpm run lint0 errors (39 pre-existing warnings)npm run test373 pass / 13 skipped / 0 failTracked in Linear: INS-217.
🤖 Generated with Claude Code
Summary by CodeRabbit
Note
Track
config apply,plan, andexportcommands in PostHog analyticstrackConfigto src/lib/analytics.ts that emits acli_config_invokedevent with subcommand name, project context, and run metadata (flags, outcome, change counts, affected sections).config apply,config plan, andconfig exportwith analytics calls on all code paths: success, dry-run, aborted confirmation, and error.shutdownAnalyticsto no-op when no client exists and to null the client before shutdown to prevent double-flush.POSTHOG_API_KEYmakes analytics a no-op.Macroscope summarized 606aaad.