feat(cli-analytics): event capture pipeline#3893
Closed
DanielVisca wants to merge 1 commit into
Closed
Conversation
The sanitize -> truncate -> beforeSend -> capture pipeline shared by every event: secret redaction, size bounding, $cli_* / $exception property mapping, and the sink that enforces consent and debug mode at a single chokepoint. Generated-By: PostHog Code Task-Id: b56cf1bb-97f1-4daf-a65c-01a2cdb12eaa
This was referenced Jun 18, 2026
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 |
20 tasks
Contributor
|
Contributor
Contributor
|
Size Change: +41.4 kB (+0.24%) Total Size: 17.1 MB
ℹ️ View Unchanged
|
Contributor
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
packages/cli-analytics/src/extensions/posthog-events.ts:127-132
`addCustomProperties` runs last in both `buildCaptureEvent` and `buildExceptionEvent`, so anything in `event.properties` can silently overwrite system-controlled fields. If a CLI forwards arbitrary user-supplied key-value pairs as `event.properties` and a value like `{ $process_person_profile: true }` ends up there, it overwrites the explicit `processPersonProfile: false` consent signal set in `addRoutingProperties`, creating person profiles for anonymous users who should be excluded. Moving `addCustomProperties` first turns it into a base layer that system properties always override. The same reordering is needed in `buildExceptionEvent`.
```suggestion
function buildCaptureEvent(event: CliEvent): PostHogCaptureEvent {
const properties: Record<string, unknown> = { [Prop.Source]: POSTHOG_CLI_ANALYTICS_SOURCE }
addCustomProperties(event, properties)
addRoutingProperties(event, properties)
addContextProperties(event, properties)
addCommandProperties(event, properties)
```
### Issue 2 of 4
packages/cli-analytics/src/extensions/posthog-events.ts:143-159
Same issue as in `buildCaptureEvent`: `addCustomProperties` runs last, meaning a caller can overwrite `$process_person_profile` or `$exception_list` via `event.properties`. Moving it first ensures custom properties form a base layer that structured fields always override.
```suggestion
function buildExceptionEvent(event: CliEvent): PostHogCaptureEvent {
const properties: Record<string, unknown> = { [Prop.Source]: POSTHOG_CLI_ANALYTICS_SOURCE }
addCustomProperties(event, properties)
addRoutingProperties(event, properties)
addContextProperties(event, properties)
if (event.error) {
// Spread the core `$exception_list` / `$exception_level` properties so CLI
// command failures use the same error-tracking contract as every other SDK.
Object.assign(properties, event.error)
}
if (event.command !== undefined) {
properties[Prop.Command] = event.command
}
if (event.subcommand !== undefined) {
properties[Prop.Subcommand] = event.subcommand
}
```
### Issue 3 of 4
packages/cli-analytics/src/__tests__/truncation.test.ts:12-18
Five independent input→output cases bundled into one `it` block. The team convention is to prefer parameterised tests — a single failure here reports as the whole "coerces non-serializable values" test rather than the specific case that broke. The regex-match case for functions needs its own `it` since it can't share the `toBe` matcher.
```suggestion
it.each<[unknown, unknown]>([
[undefined, '[undefined]'],
[Number.NaN, '[NaN]'],
[Number.POSITIVE_INFINITY, '[Infinity]'],
[10n, '[BigInt: 10]'],
])('coerces %s to %s', (input, expected) => {
expect(normalize(input)).toBe(expected)
})
it('coerces functions to a [Function:...] string', () => {
expect(normalize(() => 1)).toMatch(/^\[Function:/)
})
```
### Issue 4 of 4
packages/cli-analytics/src/extensions/sanitization.ts:21
**`auth` term over-matches unrelated keys**
The `auth` alternative has no word boundary, so it matches anywhere the four letters appear: `author`, `authored_by`, `github_auth_url`, etc. A CLI that passes `{ author: 'John Doe' }` in custom properties will have the value silently redacted to `[redacted]`. Since `authorization` is already an explicit alternative in the list and handles the full token-header name, and authentication patterns are typically caught by `credential` or `token`, narrowing this to `\bauth\b` (or removing it in favour of the explicit alternatives) would eliminate the false positives while keeping the intended coverage.
Reviews (1): Last reviewed commit: "feat(cli-analytics): event capture pipel..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+127
to
+132
| function buildCaptureEvent(event: CliEvent): PostHogCaptureEvent { | ||
| const properties: Record<string, unknown> = { [Prop.Source]: POSTHOG_CLI_ANALYTICS_SOURCE } | ||
| addRoutingProperties(event, properties) | ||
| addContextProperties(event, properties) | ||
| addCommandProperties(event, properties) | ||
| addCustomProperties(event, properties) |
Contributor
There was a problem hiding this comment.
addCustomProperties runs last in both buildCaptureEvent and buildExceptionEvent, so anything in event.properties can silently overwrite system-controlled fields. If a CLI forwards arbitrary user-supplied key-value pairs as event.properties and a value like { $process_person_profile: true } ends up there, it overwrites the explicit processPersonProfile: false consent signal set in addRoutingProperties, creating person profiles for anonymous users who should be excluded. Moving addCustomProperties first turns it into a base layer that system properties always override. The same reordering is needed in buildExceptionEvent.
Suggested change
| function buildCaptureEvent(event: CliEvent): PostHogCaptureEvent { | |
| const properties: Record<string, unknown> = { [Prop.Source]: POSTHOG_CLI_ANALYTICS_SOURCE } | |
| addRoutingProperties(event, properties) | |
| addContextProperties(event, properties) | |
| addCommandProperties(event, properties) | |
| addCustomProperties(event, properties) | |
| function buildCaptureEvent(event: CliEvent): PostHogCaptureEvent { | |
| const properties: Record<string, unknown> = { [Prop.Source]: POSTHOG_CLI_ANALYTICS_SOURCE } | |
| addCustomProperties(event, properties) | |
| addRoutingProperties(event, properties) | |
| addContextProperties(event, properties) | |
| addCommandProperties(event, properties) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/extensions/posthog-events.ts
Line: 127-132
Comment:
`addCustomProperties` runs last in both `buildCaptureEvent` and `buildExceptionEvent`, so anything in `event.properties` can silently overwrite system-controlled fields. If a CLI forwards arbitrary user-supplied key-value pairs as `event.properties` and a value like `{ $process_person_profile: true }` ends up there, it overwrites the explicit `processPersonProfile: false` consent signal set in `addRoutingProperties`, creating person profiles for anonymous users who should be excluded. Moving `addCustomProperties` first turns it into a base layer that system properties always override. The same reordering is needed in `buildExceptionEvent`.
```suggestion
function buildCaptureEvent(event: CliEvent): PostHogCaptureEvent {
const properties: Record<string, unknown> = { [Prop.Source]: POSTHOG_CLI_ANALYTICS_SOURCE }
addCustomProperties(event, properties)
addRoutingProperties(event, properties)
addContextProperties(event, properties)
addCommandProperties(event, properties)
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+143
to
+159
| function buildExceptionEvent(event: CliEvent): PostHogCaptureEvent { | ||
| const properties: Record<string, unknown> = { [Prop.Source]: POSTHOG_CLI_ANALYTICS_SOURCE } | ||
| addRoutingProperties(event, properties) | ||
| addContextProperties(event, properties) | ||
|
|
||
| if (event.error) { | ||
| // Spread the core `$exception_list` / `$exception_level` properties so CLI | ||
| // command failures use the same error-tracking contract as every other SDK. | ||
| Object.assign(properties, event.error) | ||
| } | ||
| if (event.command !== undefined) { | ||
| properties[Prop.Command] = event.command | ||
| } | ||
| if (event.subcommand !== undefined) { | ||
| properties[Prop.Subcommand] = event.subcommand | ||
| } | ||
| addCustomProperties(event, properties) |
Contributor
There was a problem hiding this comment.
buildCaptureEvent: addCustomProperties runs last, meaning a caller can overwrite $process_person_profile or $exception_list via event.properties. Moving it first ensures custom properties form a base layer that structured fields always override.
Suggested change
| function buildExceptionEvent(event: CliEvent): PostHogCaptureEvent { | |
| const properties: Record<string, unknown> = { [Prop.Source]: POSTHOG_CLI_ANALYTICS_SOURCE } | |
| addRoutingProperties(event, properties) | |
| addContextProperties(event, properties) | |
| if (event.error) { | |
| // Spread the core `$exception_list` / `$exception_level` properties so CLI | |
| // command failures use the same error-tracking contract as every other SDK. | |
| Object.assign(properties, event.error) | |
| } | |
| if (event.command !== undefined) { | |
| properties[Prop.Command] = event.command | |
| } | |
| if (event.subcommand !== undefined) { | |
| properties[Prop.Subcommand] = event.subcommand | |
| } | |
| addCustomProperties(event, properties) | |
| function buildExceptionEvent(event: CliEvent): PostHogCaptureEvent { | |
| const properties: Record<string, unknown> = { [Prop.Source]: POSTHOG_CLI_ANALYTICS_SOURCE } | |
| addCustomProperties(event, properties) | |
| addRoutingProperties(event, properties) | |
| addContextProperties(event, properties) | |
| if (event.error) { | |
| // Spread the core `$exception_list` / `$exception_level` properties so CLI | |
| // command failures use the same error-tracking contract as every other SDK. | |
| Object.assign(properties, event.error) | |
| } | |
| if (event.command !== undefined) { | |
| properties[Prop.Command] = event.command | |
| } | |
| if (event.subcommand !== undefined) { | |
| properties[Prop.Subcommand] = event.subcommand | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/extensions/posthog-events.ts
Line: 143-159
Comment:
Same issue as in `buildCaptureEvent`: `addCustomProperties` runs last, meaning a caller can overwrite `$process_person_profile` or `$exception_list` via `event.properties`. Moving it first ensures custom properties form a base layer that structured fields always override.
```suggestion
function buildExceptionEvent(event: CliEvent): PostHogCaptureEvent {
const properties: Record<string, unknown> = { [Prop.Source]: POSTHOG_CLI_ANALYTICS_SOURCE }
addCustomProperties(event, properties)
addRoutingProperties(event, properties)
addContextProperties(event, properties)
if (event.error) {
// Spread the core `$exception_list` / `$exception_level` properties so CLI
// command failures use the same error-tracking contract as every other SDK.
Object.assign(properties, event.error)
}
if (event.command !== undefined) {
properties[Prop.Command] = event.command
}
if (event.subcommand !== undefined) {
properties[Prop.Subcommand] = event.subcommand
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+12
to
+18
| it('coerces non-serializable values', () => { | ||
| expect(normalize(undefined)).toBe('[undefined]') | ||
| expect(normalize(Number.NaN)).toBe('[NaN]') | ||
| expect(normalize(Number.POSITIVE_INFINITY)).toBe('[Infinity]') | ||
| expect(normalize(() => 1)).toMatch(/^\[Function:/) | ||
| expect(normalize(10n)).toBe('[BigInt: 10]') | ||
| }) |
Contributor
There was a problem hiding this comment.
it block. The team convention is to prefer parameterised tests — a single failure here reports as the whole "coerces non-serializable values" test rather than the specific case that broke. The regex-match case for functions needs its own it since it can't share the toBe matcher.
Suggested change
| it('coerces non-serializable values', () => { | |
| expect(normalize(undefined)).toBe('[undefined]') | |
| expect(normalize(Number.NaN)).toBe('[NaN]') | |
| expect(normalize(Number.POSITIVE_INFINITY)).toBe('[Infinity]') | |
| expect(normalize(() => 1)).toMatch(/^\[Function:/) | |
| expect(normalize(10n)).toBe('[BigInt: 10]') | |
| }) | |
| it.each<[unknown, unknown]>([ | |
| [undefined, '[undefined]'], | |
| [Number.NaN, '[NaN]'], | |
| [Number.POSITIVE_INFINITY, '[Infinity]'], | |
| [10n, '[BigInt: 10]'], | |
| ])('coerces %s to %s', (input, expected) => { | |
| expect(normalize(input)).toBe(expected) | |
| }) | |
| it('coerces functions to a [Function:...] string', () => { | |
| expect(normalize(() => 1)).toMatch(/^\[Function:/) | |
| }) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/__tests__/truncation.test.ts
Line: 12-18
Comment:
Five independent input→output cases bundled into one `it` block. The team convention is to prefer parameterised tests — a single failure here reports as the whole "coerces non-serializable values" test rather than the specific case that broke. The regex-match case for functions needs its own `it` since it can't share the `toBe` matcher.
```suggestion
it.each<[unknown, unknown]>([
[undefined, '[undefined]'],
[Number.NaN, '[NaN]'],
[Number.POSITIVE_INFINITY, '[Infinity]'],
[10n, '[BigInt: 10]'],
])('coerces %s to %s', (input, expected) => {
expect(normalize(input)).toBe(expected)
})
it('coerces functions to a [Function:...] string', () => {
expect(normalize(() => 1)).toMatch(/^\[Function:/)
})
```
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!
| ] | ||
|
|
||
| /** Keys whose values are redacted wholesale, regardless of content. */ | ||
| const SENSITIVE_KEY = /(password|passwd|secret|token|api[_-]?key|authorization|auth|credential|private[_-]?key)/i |
Contributor
There was a problem hiding this comment.
auth term over-matches unrelated keys
The auth alternative has no word boundary, so it matches anywhere the four letters appear: author, authored_by, github_auth_url, etc. A CLI that passes { author: 'John Doe' } in custom properties will have the value silently redacted to [redacted]. Since authorization is already an explicit alternative in the list and handles the full token-header name, and authentication patterns are typically caught by credential or token, narrowing this to \bauth\b (or removing it in favour of the explicit alternatives) would eliminate the false positives while keeping the intended coverage.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/extensions/sanitization.ts
Line: 21
Comment:
**`auth` term over-matches unrelated keys**
The `auth` alternative has no word boundary, so it matches anywhere the four letters appear: `author`, `authored_by`, `github_auth_url`, etc. A CLI that passes `{ author: 'John Doe' }` in custom properties will have the value silently redacted to `[redacted]`. Since `authorization` is already an explicit alternative in the list and handles the full token-header name, and authentication patterns are typically caught by `credential` or `token`, narrowing this to `\bauth\b` (or removing it in favour of the explicit alternatives) would eliminate the false positives while keeping the intended coverage.
How can I resolve this? If you propose a fix, please make it concise.
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