feat(cli-analytics): anonymous identity, session, and consent#3892
Closed
DanielVisca wants to merge 1 commit into
Closed
Conversation
Persisted machine-local anonymous id (with salted one-way project hashing), a per-invocation session, intent resolution, and consent — honoring DO_NOT_TRACK and POSTHOG_CLI_TELEMETRY_DISABLED with a stderr debug mode. Generated-By: PostHog Code Task-Id: b56cf1bb-97f1-4daf-a65c-01a2cdb12eaa
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
20 tasks
Contributor
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
20 tasks
Contributor
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
This was referenced Jun 18, 2026
Contributor
|
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/cli-analytics/src/extensions/identity.ts:25-31
An empty string for `XDG_CONFIG_HOME` (or `APPDATA` on Windows) is treated as a valid path by `??`, causing `join("", 'posthog', ...)` to return a relative path like `posthog/acme.cli-analytics.json`. The identity file would then be written under the current working directory rather than the user's config dir, so each run from a different CWD would mint a new ephemeral id. Using `||` instead of `??` makes both `""` and `undefined`/`null` fall back to the default.
```suggestion
/** Resolve the per-user config directory in an OS-appropriate, dependency-free way. */
function configDir(): string {
if (process.platform === 'win32') {
return process.env.APPDATA || join(homedir(), 'AppData', 'Roaming')
}
return process.env.XDG_CONFIG_HOME || join(homedir(), '.config')
}
```
### Issue 2 of 3
packages/cli-analytics/src/__tests__/consent.test.ts:13
Both `DO_NOT_TRACK` and `POSTHOG_CLI_TELEMETRY_DISABLED` route through the same `isTruthy()` helper, so `'yes'` would also disable telemetry via `POSTHOG_CLI_TELEMETRY_DISABLED`. Adding `'yes'` to the second set keeps the test coverage symmetric and guards against any future divergence in the handling of these two variables.
```suggestion
it.each(['1', 'true', 'yes'])('honors POSTHOG_CLI_TELEMETRY_DISABLED=%p', (value) => {
```
### Issue 3 of 3
packages/cli-analytics/src/extensions/intent.ts:1-42
**Missing test coverage for `resolveIntent`**
`consent.ts` and `identity.ts` both have test files, but `intent.ts` has none. The function has branching logic worth covering: explicit intent wins over env intent, `explicitSource` defaults to `'flag'`, whitespace-only strings are treated as absent, and a `null`/`undefined` explicit with a set `POSTHOG_CLI_INTENT` falls through to `source: 'inferred'`. A short parameterised test file (similar to `consent.test.ts`) would bring this in line with the pattern in the rest of the package.
Reviews (1): Last reviewed commit: "feat(cli-analytics): anonymous identity,..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+25
to
+31
| /** Resolve the per-user config directory in an OS-appropriate, dependency-free way. */ | ||
| function configDir(): string { | ||
| if (process.platform === 'win32') { | ||
| return process.env.APPDATA ?? join(homedir(), 'AppData', 'Roaming') | ||
| } | ||
| return process.env.XDG_CONFIG_HOME ?? join(homedir(), '.config') | ||
| } |
Contributor
There was a problem hiding this comment.
XDG_CONFIG_HOME (or APPDATA on Windows) is treated as a valid path by ??, causing join("", 'posthog', ...) to return a relative path like posthog/acme.cli-analytics.json. The identity file would then be written under the current working directory rather than the user's config dir, so each run from a different CWD would mint a new ephemeral id. Using || instead of ?? makes both "" and undefined/null fall back to the default.
Suggested change
| /** Resolve the per-user config directory in an OS-appropriate, dependency-free way. */ | |
| function configDir(): string { | |
| if (process.platform === 'win32') { | |
| return process.env.APPDATA ?? join(homedir(), 'AppData', 'Roaming') | |
| } | |
| return process.env.XDG_CONFIG_HOME ?? join(homedir(), '.config') | |
| } | |
| /** Resolve the per-user config directory in an OS-appropriate, dependency-free way. */ | |
| function configDir(): string { | |
| if (process.platform === 'win32') { | |
| return process.env.APPDATA || join(homedir(), 'AppData', 'Roaming') | |
| } | |
| return process.env.XDG_CONFIG_HOME || join(homedir(), '.config') | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/extensions/identity.ts
Line: 25-31
Comment:
An empty string for `XDG_CONFIG_HOME` (or `APPDATA` on Windows) is treated as a valid path by `??`, causing `join("", 'posthog', ...)` to return a relative path like `posthog/acme.cli-analytics.json`. The identity file would then be written under the current working directory rather than the user's config dir, so each run from a different CWD would mint a new ephemeral id. Using `||` instead of `??` makes both `""` and `undefined`/`null` fall back to the default.
```suggestion
/** Resolve the per-user config directory in an OS-appropriate, dependency-free way. */
function configDir(): string {
if (process.platform === 'win32') {
return process.env.APPDATA || join(homedir(), 'AppData', 'Roaming')
}
return process.env.XDG_CONFIG_HOME || join(homedir(), '.config')
}
```
How can I resolve this? If you propose a fix, please make it concise.| expect(isTelemetryEnabled({ DO_NOT_TRACK: value })).toBe(false) | ||
| }) | ||
|
|
||
| it.each(['1', 'true'])('honors POSTHOG_CLI_TELEMETRY_DISABLED=%p', (value) => { |
Contributor
There was a problem hiding this comment.
DO_NOT_TRACK and POSTHOG_CLI_TELEMETRY_DISABLED route through the same isTruthy() helper, so 'yes' would also disable telemetry via POSTHOG_CLI_TELEMETRY_DISABLED. Adding 'yes' to the second set keeps the test coverage symmetric and guards against any future divergence in the handling of these two variables.
Suggested change
| it.each(['1', 'true'])('honors POSTHOG_CLI_TELEMETRY_DISABLED=%p', (value) => { | |
| it.each(['1', 'true', 'yes'])('honors POSTHOG_CLI_TELEMETRY_DISABLED=%p', (value) => { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/__tests__/consent.test.ts
Line: 13
Comment:
Both `DO_NOT_TRACK` and `POSTHOG_CLI_TELEMETRY_DISABLED` route through the same `isTruthy()` helper, so `'yes'` would also disable telemetry via `POSTHOG_CLI_TELEMETRY_DISABLED`. Adding `'yes'` to the second set keeps the test coverage symmetric and guards against any future divergence in the handling of these two variables.
```suggestion
it.each(['1', 'true', 'yes'])('honors POSTHOG_CLI_TELEMETRY_DISABLED=%p', (value) => {
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+1
to
+42
| import type { CliIntentSource } from '../types' | ||
|
|
||
| type EnvLike = Record<string, string | undefined> | ||
|
|
||
| /** Env var an agent can set to declare its intent for the command it's about to run. */ | ||
| export const INTENT_ENV = 'POSTHOG_CLI_INTENT' | ||
|
|
||
| export interface ResolvedIntent { | ||
| intent: string | ||
| source: CliIntentSource | ||
| } | ||
|
|
||
| function normalize(intent: string | null | undefined): string | null { | ||
| if (typeof intent !== 'string') { | ||
| return null | ||
| } | ||
| const trimmed = intent.trim() | ||
| return trimmed ? trimmed : null | ||
| } | ||
|
|
||
| /** | ||
| * Resolves the intent for a command. An explicitly-provided intent (from a | ||
| * `--intent` flag or the capture API) wins with `source: 'flag'`; otherwise the | ||
| * `POSTHOG_CLI_INTENT` env var is used with `source: 'inferred'`. The source | ||
| * values mirror the MCP SDK's `$mcp_intent_source` so both feed one clustering | ||
| * pipeline. | ||
| */ | ||
| export function resolveIntent( | ||
| explicit: string | undefined, | ||
| explicitSource: CliIntentSource | undefined, | ||
| env: EnvLike = process.env | ||
| ): ResolvedIntent | null { | ||
| const explicitIntent = normalize(explicit) | ||
| if (explicitIntent) { | ||
| return { intent: explicitIntent, source: explicitSource ?? 'flag' } | ||
| } | ||
| const envIntent = normalize(env[INTENT_ENV]) | ||
| if (envIntent) { | ||
| return { intent: envIntent, source: 'inferred' } | ||
| } | ||
| return null | ||
| } |
Contributor
There was a problem hiding this comment.
resolveIntent
consent.ts and identity.ts both have test files, but intent.ts has none. The function has branching logic worth covering: explicit intent wins over env intent, explicitSource defaults to 'flag', whitespace-only strings are treated as absent, and a null/undefined explicit with a set POSTHOG_CLI_INTENT falls through to source: 'inferred'. A short parameterised test file (similar to consent.test.ts) would bring this in line with the pattern in the rest of the package.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/extensions/intent.ts
Line: 1-42
Comment:
**Missing test coverage for `resolveIntent`**
`consent.ts` and `identity.ts` both have test files, but `intent.ts` has none. The function has branching logic worth covering: explicit intent wins over env intent, `explicitSource` defaults to `'flag'`, whitespace-only strings are treated as absent, and a `null`/`undefined` explicit with a set `POSTHOG_CLI_INTENT` falls through to `source: 'inferred'`. A short parameterised test file (similar to `consent.test.ts`) would bring this in line with the pattern in the rest of the package.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Contributor
|
Size Change: +12.4 kB (+0.07%) Total Size: 17 MB
ℹ️ View Unchanged
|
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Contributor
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
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
1 participant
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.
Problem
Changes
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted) — or — Fully autonomous
Created with PostHog Code