test(cli): drain PTY stdout before resolving E2E exit (fixes flaky help test)

packages/cli/src/bin/main.ts

@@ -4,10 +4,16 @@ import { config } from 'dotenv'
 // not overwrite vars that are already set, so loading .env.local first means
 // its values win over .env for the same keys. Users can still set anything in
 // the real environment to override both.
-config({ path: '.env.local' })
-config({ path: '.env.development.local' })
-config({ path: '.env.development' })
-config({ path: '.env' })
+//
+// `quiet: true` suppresses dotenv v17's `injected env (N) from …` banner,
+// which it now prints to stdout on every `config()` call. Without it the CLI
+// emits four noisy, non-deterministic banner lines (with rotating tips) ahead
+// of its own output on every invocation — restoring the silent behaviour of
+// dotenv v16.
+config({ path: '.env.local', quiet: true })
+config({ path: '.env.development.local', quiet: true })
+config({ path: '.env.development', quiet: true })
+config({ path: '.env', quiet: true })
 
 import { readFileSync } from 'node:fs'
 import { dirname, join } from 'node:path'

packages/cli/tests/e2e/runner-aware-help.e2e.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from 'vitest'
-import { render } from '../helpers/pty.js'
+import { run } from '../helpers/run.js'
 
 /**
  * E2E coverage for the runner-aware help rendering. The smoke suite
@@ -29,9 +29,8 @@ describe('--help — runner-aware Usage + Examples', () => {
     ua,
     label,
   }) => {
-    const r = render(['--help'], { env: { npm_config_user_agent: ua } })
-    const { exitCode } = await r.exit
-    expect(exitCode).toBe(0)
+    const r = await run(['--help'], { env: { npm_config_user_agent: ua } })
+    expect(r.exitCode).toBe(0)
     // Usage line must use the right runner. The leader is stable
     // (`messages.cli.usagePrefix === 'Usage: '`) so we assert on the
     // suffix the renderer composes at runtime.
@@ -50,9 +49,8 @@ describe('auth — runner-aware Usage + Examples', () => {
     label,
   }) => {
     // `auth` with no subcommand prints the auth HELP and exits 0.
-    const r = render(['auth'], { env: { npm_config_user_agent: ua } })
-    const { exitCode } = await r.exit
-    expect(exitCode).toBe(0)
+    const r = await run(['auth'], { env: { npm_config_user_agent: ua } })
+    expect(r.exitCode).toBe(0)
     expect(r.output).toContain(`Usage: ${label} stash auth`)
     expect(r.output).toContain(`${label} stash auth login`)
   })

packages/cli/tests/e2e/smoke.e2e.test.ts

@@ -1,9 +1,11 @@
-import { readFileSync } from 'node:fs'
-import { dirname, resolve } from 'node:path'
+import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { dirname, join, resolve } from 'node:path'
 import { fileURLToPath } from 'node:url'
 import { describe, expect, it } from 'vitest'
 import { messages } from '../../src/messages.js'
 import { render } from '../helpers/pty.js'
+import { run } from '../helpers/run.js'
 
 const __dirname = dirname(fileURLToPath(import.meta.url))
 const pkg = JSON.parse(
@@ -21,6 +23,28 @@ describe('stash CLI — non-interactive smoke', () => {
     // copy strings, so they stay inline.
     expect(r.output).toContain('init')
     expect(r.output).toContain('db install')
+    // The dotenv "injected env" banner regression guard lives in the
+    // dedicated test below — this cwd has no .env file, so a bare
+    // `not.toContain('injected env')` here would pass vacuously.
+  })
+
+  it('suppresses dotenv v17\'s "injected env" banner when a .env file exists in cwd', async () => {
+    // dotenv v17 prints an `injected env (N) from …` banner to stdout on
+    // every `config()` call that actually injects a variable, unless
+    // `quiet: true` is passed. The repo has no `.env` anywhere, so exercising
+    // this requires a real .env file in the spawned process's cwd — without
+    // it, `config()` never finds anything to inject and the banner can never
+    // appear regardless of whether `quiet: true` is present in the CLI.
+    const tmpDir = mkdtempSync(join(tmpdir(), 'stash-dotenv-quiet-'))
+    try {
+      writeFileSync(join(tmpDir, '.env'), 'STASH_DOTENV_QUIET_TEST=1\n')
+      const r = await run(['--version'], { cwd: tmpDir })
+      expect(r.exitCode).toBe(0)
+      expect(r.output).toContain(pkg.version)
+      expect(r.output).not.toContain('injected env')
+    } finally {
+      rmSync(tmpDir, { recursive: true, force: true })
+    }
   })
 
   it('--version prints the package version', async () => {
@@ -73,4 +97,30 @@ describe('stash CLI — non-interactive smoke', () => {
     // suffix is the stable assertion target.
     expect(r.output).toContain('stash db migrate" is not yet implemented.')
   })
+
+  // The two cases below exercise `run()` directly rather than `render()`.
+  // Every existing `run()` consumer only asserted `exitCode === 0`
+  // (runner-aware-help.e2e.test.ts) and every exit-1 case above still went
+  // through `render()`, so `run()`'s actual purpose — correctly propagating
+  // a non-zero/null exit code instead of masking it — was untested by
+  // anything that used `run()`.
+  it('run(): surfaces a non-zero exit code + error output for an unknown command', async () => {
+    const r = await run(['definitely-not-a-command'])
+    expect(r.exitCode).toBe(1)
+    expect(r.output).toContain(messages.cli.unknownCommand)
+    expect(r.output).toContain('definitely-not-a-command')
+  })
+
+  it('run(): splits stdout/stderr into independent channels on a failure path', async () => {
+    // The unknown-command error is written via `console.error` (stderr); the
+    // HELP banner that follows it is written via `console.log` (stdout).
+    // Assert on the dedicated `stdout`/`stderr` fields — not just the
+    // combined `output` — so a regression that wired both `data` handlers
+    // to the same buffer would be caught.
+    const r = await run(['definitely-not-a-command'])
+    expect(r.stderr).toContain(messages.cli.unknownCommand)
+    expect(r.stderr).not.toContain(messages.cli.usagePrefix)
+    expect(r.stdout).toContain(messages.cli.usagePrefix)
+    expect(r.stdout).not.toContain(messages.cli.unknownCommand)
+  })
 })

packages/cli/tests/helpers/run.test.ts

@@ -0,0 +1,48 @@
+import { describe, expect, it } from 'vitest'
+import { buildRunResult } from './run.js'
+
+// Unit coverage for the pure `close`-event → RunResult mapping extracted
+// from `run()`. `run()` itself always spawns the built CLI binary and
+// exposes no handle to signal the child, which makes the signal-terminated
+// path (the headline fix in aedfd9d) impractical to exercise deterministically
+// through an E2E test. Testing the extracted pure function directly avoids
+// needing a timeout/SIGKILL escape hatch or other API surface change.
+describe('buildRunResult', () => {
+  it('reports a clean exit', () => {
+    const r = buildRunResult(0, null, 'hello\n', '')
+    expect(r.exitCode).toBe(0)
+    expect(r.signal).toBeNull()
+    expect(r.stdout).toBe('hello\n')
+    expect(r.output).toBe('hello\n')
+  })
+
+  it('reports a non-zero exit code', () => {
+    const r = buildRunResult(1, null, '', 'boom\n')
+    expect(r.exitCode).toBe(1)
+    expect(r.signal).toBeNull()
+    expect(r.stderr).toBe('boom\n')
+  })
+
+  it('keeps exitCode null (never masked to 0) when the child is signal-terminated', () => {
+    // Node guarantees exactly one of code/signal is non-null on 'close'. A
+    // naive `code ?? 0` would collapse this to a "successful" exit 0, hiding
+    // a crash/OOM/SIGTERM kill from callers.
+    const r = buildRunResult(null, 'SIGTERM', '', '')
+    expect(r.exitCode).toBeNull()
+    expect(r.signal).toBe('SIGTERM')
+  })
+
+  it('strips ANSI codes independently from stdout/stderr while combining into output/raw', () => {
+    // Build the ESC control byte at runtime (rather than embedding it
+    // literally in the source) to keep this file free of non-printable
+    // characters.
+    const ESC = String.fromCharCode(27)
+    const green = `${ESC}[32mok${ESC}[0m`
+    const red = `${ESC}[31merr${ESC}[0m`
+    const r = buildRunResult(0, null, green, red)
+    expect(r.stdout).toBe('ok')
+    expect(r.stderr).toBe('err')
+    expect(r.output).toBe('okerr')
+    expect(r.raw).toBe(green + red)
+  })
+})

packages/cli/tests/helpers/run.ts

@@ -0,0 +1,102 @@
+import { spawn } from 'node:child_process'
+import stripAnsi from 'strip-ansi'
+import { STASH_BIN } from './pty.js'
+
+export interface RunOptions {
+  cwd?: string
+  env?: Record<string, string>
+}
+
+export interface RunResult {
+  /**
+   * The process's numeric exit code, or `null` when it was terminated by a
+   * signal (see {@link RunResult.signal}). Never coerced to 0, so a
+   * signal-killed child (crash, SIGKILL) is distinguishable from a clean exit.
+   */
+  exitCode: number | null
+  signal: NodeJS.Signals | null
+  /** ANSI-stripped combined stdout + stderr. */
+  output: string
+  /** Raw combined stdout + stderr including ANSI escapes. */
+  raw: string
+  stdout: string
+  stderr: string
+}
+
+/**
+ * Run the built CLI to completion over plain pipes and capture all output.
+ *
+ * Prefer this over {@link render} for NON-INTERACTIVE commands (`--help`,
+ * `--version`, usage/error paths). A pipe buffers the child's output in the
+ * Node process and delivers every byte before `close`, so large bursts are
+ * never truncated.
+ *
+ * A PTY, by contrast, loses data on Linux: when the child writes a big burst
+ * (the ~5KB `--help` text is a single `console.log`) and exits immediately,
+ * the kernel discards whatever is still unread in the pty buffer when the
+ * slave closes. On a loaded CI runner node-pty can't drain fast enough, so the
+ * tail of the help — the Examples section — goes missing and assertions on it
+ * flake. (macOS ptys keep buffered data on close, which is why it only bites
+ * in CI.) Post-exit "drain" waits can't help because the bytes are gone before
+ * `onData` ever fires. Only interactive tests that must send keystrokes or
+ * exercise TTY-specific rendering need {@link render}.
+ */
+export function run(args: string[], opts: RunOptions = {}): Promise<RunResult> {
+  const env: Record<string, string> = {
+    ...(process.env as Record<string, string>),
+    // Mirror render()'s env so both helpers hit the same CLI code paths.
+    NO_COLOR: '1',
+    FORCE_COLOR: '0',
+    CI: 'true',
+    ...(opts.env ?? {}),
+  }
+
+  const child = spawn(process.execPath, [STASH_BIN, ...args], {
+    cwd: opts.cwd ?? process.cwd(),
+    env,
+    stdio: ['ignore', 'pipe', 'pipe'],
+  })
+
+  let stdout = ''
+  let stderr = ''
+  child.stdout.setEncoding('utf8')
+  child.stderr.setEncoding('utf8')
+  child.stdout.on('data', (d: string) => {
+    stdout += d
+  })
+  child.stderr.on('data', (d: string) => {
+    stderr += d
+  })
+
+  return new Promise<RunResult>((res, rej) => {
+    child.on('error', rej)
+    child.on('close', (code, signal) => {
+      res(buildRunResult(code, signal, stdout, stderr))
+    })
+  })
+}
+
+/**
+ * Pure `close`-event → {@link RunResult} mapping, factored out of `run()` so
+ * the exit-code/signal handling can be unit-tested directly without spawning
+ * a real child process (see `run.test.ts`). Node guarantees exactly one of
+ * `code`/`signal` is non-null on `'close'` — this must never coerce a null
+ * `code` to `0`, or a signal-terminated child (crash, SIGKILL, OOM) would be
+ * misreported as a clean exit.
+ */
+export function buildRunResult(
+  code: number | null,
+  signal: NodeJS.Signals | null,
+  stdout: string,
+  stderr: string,
+): RunResult {
+  const raw = stdout + stderr
+  return {
+    exitCode: code,
+    signal,
+    output: stripAnsi(raw),
+    raw,
+    stdout: stripAnsi(stdout),
+    stderr: stripAnsi(stderr),
+  }
+}