fix(telemetry): postinstall — symlink-safe isDirectRun, stdout flush, dead-code#323
Merged
Merged
Conversation
… dead-code
Three small fixes uncovered by manual smoke testing of the npm install
path against a local capture server.
1. isDirectRun symlink bug (macOS, pnpm stores):
`process.argv[1]` keeps the unresolved path (e.g. /tmp/foo.js) while
`import.meta.url` resolves to the realpath (file:///private/tmp/foo.js
on macOS). The naive string comparison failed even though both
referenced the same file, so `node /tmp/.../postinstall.js` was a
silent no-op. Switched to
pathToFileURL(realpathSync(process.argv[1])).href === import.meta.url
which normalizes both sides through realpath. Direct invocation now
triggers main() correctly.
2. stdout opt-out notice was invisible to `npm install --foreground-scripts`:
posthog-node's `shutdown()` await chain left the notice in stdout's
pipe buffer at process exit. npm reaped the script before the buffer
drained. Added `await flushStdout()` (Promise that resolves on the
stream's drain event or one setImmediate tick) so the line is reliably
flushed before the process can exit.
3. Dropped redundant `if (!deps.env.CI)` guard:
`isTelemetryDisabled(deps.env)` already short-circuits on CI=true
(and DO_NOT_TRACK, NGAF_TELEMETRY_DISABLED) by returning early before
either the capture or the write. The second CI check was unreachable
dead code. The README's trust contract correctly says CI = full opt-out;
the previous code matched that contract semantically but had two
inconsistent gates.
Tests: 46 passing (was 45). New test asserts DO_NOT_TRACK is a full
opt-out (no event AND no notice), matching the README contract. The
CI=true test gained a `capturePostinstall.not.toHaveBeenCalled()`
assertion to pin that CI is full opt-out too.
Smoke-verified end-to-end:
- npm install --foreground-scripts now shows the opt-out notice ✓
- node /tmp/.../postinstall.js (direct from /tmp) now triggers main() ✓
- DO_NOT_TRACK=1 npm install: no event, no notice ✓
- CI=true npm install: no event, no notice ✓
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Summary
Three small fixes uncovered while smoke-testing the npm install flow against a local capture server. All confirmed working end-to-end before opening the PR.
Bug 1 —
isDirectRunsymlink-blindness (macOS, pnpm stores)process.argv[1]keeps the unresolved path (e.g./tmp/foo.js), butimport.meta.urlresolves through realpath (file:///private/tmp/foo.json macOS, content-addressed paths under pnpm). The naiveimport.meta.url === \file://${process.argv[1]}`comparison failed even when both pointed at the same file — sonode /tmp/.../postinstall.js` was a silent no-op.Fixed by normalizing both sides through realpath:
```ts
pathToFileURL(realpathSync(process.argv[1])).href === import.meta.url
```
Bug 2 — stdout opt-out notice was invisible to
npm install --foreground-scriptsposthog-node's
shutdown()await chain left the notice in stdout's pipe buffer at process exit. npm reaped the script before the buffer drained, so the user never saw the trust-contract opt-out line. Addedawait flushStdout()(drains via the stream'sdrainevent or onesetImmediatetick) beforemain()returns.Before: install output silent.
After:
```
Bug 3 — Redundant
if (!deps.env.CI)guardisTelemetryDisabled(deps.env)already short-circuits onCI=true,DO_NOT_TRACK, andNGAF_TELEMETRY_DISABLED. The second CI check was unreachable. The README's trust contract correctly says CI = full opt-out; the implementation now has a single source-of-truth gate matching that contract.Tests
capturePostinstall.not.toHaveBeenCalled()assertionEnd-to-end smoke verification (manual)
npm install --foreground-scripts /tmp/ngaf-telemetry-smoke.tgz→ notice visible ✓node /tmp/.../postinstall.js(direct, with /tmp symlink) → main() triggers ✓DO_NOT_TRACK=1 npm install …→ no event, no notice ✓CI=true npm install …→ no event, no notice ✓🤖 Generated with Claude Code