From 7de3bbdc7d97935579ed8d2a2ce15f6cf8f6f27a Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 15 May 2026 10:48:01 -0700 Subject: [PATCH] =?UTF-8?q?fix(telemetry):=20postinstall=20=E2=80=94=20sym?= =?UTF-8?q?link-safe=20isDirectRun,=20stdout=20flush,=20dead-code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- libs/telemetry/src/node/postinstall.spec.ts | 14 +++++- libs/telemetry/src/node/postinstall.ts | 49 ++++++++++++++++----- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/libs/telemetry/src/node/postinstall.spec.ts b/libs/telemetry/src/node/postinstall.spec.ts index 71065e4a3..171105ded 100644 --- a/libs/telemetry/src/node/postinstall.spec.ts +++ b/libs/telemetry/src/node/postinstall.spec.ts @@ -35,7 +35,7 @@ describe('postinstall script', () => { expect(stdout.join('')).toMatch(/DO_NOT_TRACK=1/); }); - test('suppresses stdout notice when CI=true', async () => { + test('CI=true is full opt-out: no event sent and no stdout notice', async () => { const stdout: string[] = []; await capturePostinstallScript({ readPackageJson: () => ({ name: '@ngaf/telemetry', version: '0.0.31' }), @@ -43,6 +43,18 @@ describe('postinstall script', () => { env: { ...process.env, CI: 'true' }, }); expect(stdout).toEqual([]); + expect(capturePostinstall).not.toHaveBeenCalled(); + }); + + test('DO_NOT_TRACK=1 is full opt-out: no event sent and no stdout notice', async () => { + const stdout: string[] = []; + await capturePostinstallScript({ + readPackageJson: () => ({ name: '@ngaf/telemetry', version: '0.0.31' }), + write: (s: string) => stdout.push(s), + env: { ...process.env, DO_NOT_TRACK: '1' }, + }); + expect(stdout).toEqual([]); + expect(capturePostinstall).not.toHaveBeenCalled(); }); test('swallows readPackageJson errors silently', async () => { diff --git a/libs/telemetry/src/node/postinstall.ts b/libs/telemetry/src/node/postinstall.ts index 808069385..f83dffc36 100644 --- a/libs/telemetry/src/node/postinstall.ts +++ b/libs/telemetry/src/node/postinstall.ts @@ -1,5 +1,5 @@ -import { readFileSync } from 'node:fs'; -import { fileURLToPath } from 'node:url'; +import { readFileSync, realpathSync } from 'node:fs'; +import { fileURLToPath, pathToFileURL } from 'node:url'; import { dirname, join } from 'node:path'; import { capturePostinstall } from './client.js'; import { isTelemetryDisabled } from '../shared/env.js'; @@ -11,6 +11,9 @@ interface PostinstallDeps { } export async function capturePostinstallScript(deps: PostinstallDeps): Promise { + // Single opt-out gate. DO_NOT_TRACK, NGAF_TELEMETRY_DISABLED, and CI envs + // all funnel through isTelemetryDisabled and return early — no event sent, + // no stdout notice. Matches libs/telemetry/README.md trust contract. if (isTelemetryDisabled(deps.env)) return; let pkg: { name: string; version: string }; try { @@ -20,18 +23,30 @@ export async function capturePostinstallScript(deps: PostinstallDeps): Promise { + return new Promise((resolve) => { + if (process.stdout.writableNeedDrain) { + process.stdout.once('drain', () => resolve()); + } else { + // Yield one tick so any pending write callbacks run before exit. + setImmediate(() => resolve()); + } + }); +} + // Entry point — invoked by package.json scripts.postinstall. async function main(): Promise { await capturePostinstallScript({ @@ -42,9 +57,19 @@ async function main(): Promise { write: (s) => process.stdout.write(s), env: process.env, }); + await flushStdout(); } // Only run as main entry, not when imported by tests. -const isDirectRun = - process.argv[1] && import.meta.url === `file://${process.argv[1]}`; -if (isDirectRun) main(); +// Resolves symlinks on both sides so `/tmp` vs `/private/tmp` on macOS, +// pnpm content-addressed stores, and similar setups all match correctly. +function isDirectRun(): boolean { + const entry = process.argv[1]; + if (!entry) return false; + try { + return pathToFileURL(realpathSync(entry)).href === import.meta.url; + } catch { + return false; + } +} +if (isDirectRun()) main();