feat(cli-analytics): instrument() handle and commander adapter#3894
Draft
DanielVisca wants to merge 1 commit into
Draft
feat(cli-analytics): instrument() handle and commander adapter#3894DanielVisca wants to merge 1 commit into
DanielVisca wants to merge 1 commit into
Conversation
Ties the SDK together: instrument(posthog, options) returns a handle for capturing command runs and custom events with exit-safe flushing, plus a zero-dependency Commander adapter and the public entry point. Generated-By: PostHog Code Task-Id: b56cf1bb-97f1-4daf-a65c-01a2cdb12eaa
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 |
Contributor
|
Contributor
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/adapters/commander.ts:40-41
**`opts()` returns all options, not just user-supplied ones**
Commander's `opts()` returns every defined option with its current value — including those that were never passed on the command line but have a default (e.g. `{ verbose: false, force: false, dryRun: false }`). `Object.keys` over that will include ALL flag names even when the user typed nothing. The `FakeCommand` in the test hides this because it only stores explicitly-passed options, so the test passes while real programs would emit inflated `$cli_flags` arrays.
To capture only explicitly-set flags Commander exposes `getOptionValueSource(name): 'cli' | 'env' | 'default' | 'implied' | undefined`. Adding that method to `CommanderCommandLike` (as optional) and filtering to `'cli'` entries would give the intended behaviour. Alternatively, filtering `Object.entries(command.opts())` to only truthy values is a workable approximation for boolean flags, though it would silently drop numeric `0` or empty-string values.
### Issue 2 of 4
packages/cli-analytics/src/extensions/instrumentation.ts:132
The `this.agent.isAgent ||` sub-expression is redundant. When `options.agent === false` the constructor assigns `{ isAgent: false, source: null }` to `this.agent`, so `this.agent.isAgent` is always `false` in that branch — meaning the overall condition reduces to `false || false` → `false` regardless. For every other value of `options.agent` the right-hand `this.options.agent !== false` is already `true`, so the left-hand side adds nothing.
```suggestion
agent: this.options.agent !== false ? this.agent : undefined,
```
### Issue 3 of 4
packages/cli-analytics/src/__tests__/instrument.test.ts:153-162
**Opt-out test should be parameterised over both env vars**
`isTelemetryEnabled` (in `consent.ts`) honours two independent opt-out variables — `DO_NOT_TRACK` and `POSTHOG_CLI_TELEMETRY_DISABLED` — but only `DO_NOT_TRACK` is exercised here. Per the team's preference for parameterised tests, both variables should be covered with a single `it.each` (or `test.each`) block to prevent a regression where one path stops working silently.
### Issue 4 of 4
packages/cli-analytics/src/extensions/instrumentation.ts:42
**`enableExceptionAutocapture` is always `true` — superfluous field**
The field is assigned `true` in the constructor and is never read from elsewhere or toggled anywhere in the codebase. Storing it as a named field rather than inlining the literal at its single use site adds indirection without meaning. If it is intended as a future extension point, a TODO comment at the call site would communicate that more clearly than a private field.
```suggestion
// TODO: make configurable via CliAnalyticsOptions when needed
private readonly enableExceptionAutocapture = true
```
Reviews (1): Last reviewed commit: "feat(cli-analytics): instrument() handle..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+40
to
+41
| // `opts()` keys are the flag NAMES the user supplied — values are never read. | ||
| flags: Object.keys(command.opts()), |
Contributor
There was a problem hiding this comment.
opts() returns all options, not just user-supplied ones
Commander's opts() returns every defined option with its current value — including those that were never passed on the command line but have a default (e.g. { verbose: false, force: false, dryRun: false }). Object.keys over that will include ALL flag names even when the user typed nothing. The FakeCommand in the test hides this because it only stores explicitly-passed options, so the test passes while real programs would emit inflated $cli_flags arrays.
To capture only explicitly-set flags Commander exposes getOptionValueSource(name): 'cli' | 'env' | 'default' | 'implied' | undefined. Adding that method to CommanderCommandLike (as optional) and filtering to 'cli' entries would give the intended behaviour. Alternatively, filtering Object.entries(command.opts()) to only truthy values is a workable approximation for boolean flags, though it would silently drop numeric 0 or empty-string values.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/adapters/commander.ts
Line: 40-41
Comment:
**`opts()` returns all options, not just user-supplied ones**
Commander's `opts()` returns every defined option with its current value — including those that were never passed on the command line but have a default (e.g. `{ verbose: false, force: false, dryRun: false }`). `Object.keys` over that will include ALL flag names even when the user typed nothing. The `FakeCommand` in the test hides this because it only stores explicitly-passed options, so the test passes while real programs would emit inflated `$cli_flags` arrays.
To capture only explicitly-set flags Commander exposes `getOptionValueSource(name): 'cli' | 'env' | 'default' | 'implied' | undefined`. Adding that method to `CommanderCommandLike` (as optional) and filtering to `'cli'` entries would give the intended behaviour. Alternatively, filtering `Object.entries(command.opts())` to only truthy values is a workable approximation for boolean flags, though it would silently drop numeric `0` or empty-string values.
How can I resolve this? If you propose a fix, please make it concise.| processPersonProfile: partial.processPersonProfile ?? identity.processPersonProfile, | ||
| cli: this.options.cli, | ||
| environment: this.environment, | ||
| agent: this.agent.isAgent || this.options.agent !== false ? this.agent : undefined, |
Contributor
There was a problem hiding this comment.
this.agent.isAgent || sub-expression is redundant. When options.agent === false the constructor assigns { isAgent: false, source: null } to this.agent, so this.agent.isAgent is always false in that branch — meaning the overall condition reduces to false || false → false regardless. For every other value of options.agent the right-hand this.options.agent !== false is already true, so the left-hand side adds nothing.
Suggested change
| agent: this.agent.isAgent || this.options.agent !== false ? this.agent : undefined, | |
| agent: this.options.agent !== false ? this.agent : undefined, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/extensions/instrumentation.ts
Line: 132
Comment:
The `this.agent.isAgent ||` sub-expression is redundant. When `options.agent === false` the constructor assigns `{ isAgent: false, source: null }` to `this.agent`, so `this.agent.isAgent` is always `false` in that branch — meaning the overall condition reduces to `false || false` → `false` regardless. For every other value of `options.agent` the right-hand `this.options.agent !== false` is already `true`, so the left-hand side adds nothing.
```suggestion
agent: this.options.agent !== false ? this.agent : undefined,
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+153
to
+162
| const analytics = instrument(fake.asPostHog(), options()) | ||
| analytics.trackCommand({ command: 'deploy' }) | ||
| analytics.track('feedback', {}) | ||
| await analytics.flush() | ||
| expect(fake.captures).toHaveLength(0) | ||
| }) | ||
|
|
||
| it('flush awaits pending captures before flushing the client', async () => { | ||
| const fake = new FakePostHog() | ||
| const analytics = instrument( |
Contributor
There was a problem hiding this comment.
isTelemetryEnabled (in consent.ts) honours two independent opt-out variables — DO_NOT_TRACK and POSTHOG_CLI_TELEMETRY_DISABLED — but only DO_NOT_TRACK is exercised here. Per the team's preference for parameterised tests, both variables should be covered with a single it.each (or test.each) block to prevent a regression where one path stops working silently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/__tests__/instrument.test.ts
Line: 153-162
Comment:
**Opt-out test should be parameterised over both env vars**
`isTelemetryEnabled` (in `consent.ts`) honours two independent opt-out variables — `DO_NOT_TRACK` and `POSTHOG_CLI_TELEMETRY_DISABLED` — but only `DO_NOT_TRACK` is exercised here. Per the team's preference for parameterised tests, both variables should be covered with a single `it.each` (or `test.each`) block to prevent a regression where one path stops working silently.
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!
| private readonly sink: CliEventSink | ||
| private readonly sessionId: string | ||
| private readonly environment: EnvironmentInfo | ||
| private readonly enableExceptionAutocapture: boolean |
Contributor
There was a problem hiding this comment.
enableExceptionAutocapture is always true — superfluous field
The field is assigned true in the constructor and is never read from elsewhere or toggled anywhere in the codebase. Storing it as a named field rather than inlining the literal at its single use site adds indirection without meaning. If it is intended as a future extension point, a TODO comment at the call site would communicate that more clearly than a private field.
Suggested change
| private readonly enableExceptionAutocapture: boolean | |
| // TODO: make configurable via CliAnalyticsOptions when needed | |
| private readonly enableExceptionAutocapture = true |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/extensions/instrumentation.ts
Line: 42
Comment:
**`enableExceptionAutocapture` is always `true` — superfluous field**
The field is assigned `true` in the constructor and is never read from elsewhere or toggled anywhere in the codebase. Storing it as a named field rather than inlining the literal at its single use site adds indirection without meaning. If it is intended as a future extension point, a TODO comment at the call site would communicate that more clearly than a private field.
```suggestion
// TODO: make configurable via CliAnalyticsOptions when needed
private readonly enableExceptionAutocapture = true
```
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
|
Size Change: +25 kB (+0.15%) Total Size: 17.1 MB
ℹ️ View Unchanged
|
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