fix(init): harden /dev/tty teardown and adopt Symbol.dispose

src/lib/init/stdin-reopen.ts

@@ -37,43 +37,122 @@ type StdinHandle = {
 type InstalledState = {
   fresh: ReadStream;
   dataListener: (chunk: Buffer) => void;
-  errorListener: () => void;
   original: {
     setRawMode: StdinHandle["setRawMode"];
     pause: StdinHandle["pause"];
     resume: StdinHandle["resume"];
     read: StdinHandle["_read"];
   };
+  /**
+   * Value of `process.stdin.isTTY` before we touched it. Teardown restores
+   * exactly this value rather than hardcoding `undefined`, so a concurrent
+   * writer (e.g. another library that also backfills isTTY) doesn't get
+   * silently stomped on.
+   */
+  previousIsTty: boolean | undefined;
+  /** True when we wrote to `process.stdin.isTTY` at install time. */
   backfilledIsTty: boolean;
 };
 
 let installedState: InstalledState | null = null;
 
+/**
+ * Factory that returns a `/dev/tty` file descriptor. Overridable for tests
+ * so we can exercise the install→teardown state transitions without depending
+ * on the host's actual TTY.
+ */
+export type OpenTtyFactory = () => number;
+
+/**
+ * Predicate that reports whether fd 0 is a TTY. Overridable for tests
+ * because `isatty(0)` reads real kernel state we can't mock easily — and
+ * `bun test` runs with piped stdin where the predicate is always false.
+ */
+export type IsTtyPredicate = () => boolean;
+
+/** Bundle of host primitives that tests can override. */
+export type TtyDeps = {
+  openTty?: OpenTtyFactory;
+  isTty?: IsTtyPredicate;
+};
+
+const defaultOpenTty: OpenTtyFactory = () => openSync("/dev/tty", "r");
+const defaultIsTty: IsTtyPredicate = () => isatty(0);
+
+/**
+ * Disposable returned by {@link forwardFreshTtyToStdin}. Calling
+ * `[Symbol.dispose]()` — or equivalently letting a `using` declaration go
+ * out of scope — releases the temporary TTY handle and restores
+ * `process.stdin`. Always returned (never null) so callers don't need to
+ * null-check inside `using` blocks.
+ */
+export type TtyForwardingHandle = Disposable;
+
+/** Shared no-op disposable for the secondary-caller / already-installed case. */
+const NOOP_HANDLE: TtyForwardingHandle = {
+  [Symbol.dispose]: (): void => {
+    // intentionally empty — primary caller owns teardown
+  },
+};
+
+/**
+ * Build a handle that routes disposal through
+ * {@link closeFreshTtyForwarding}. Using the module-level function (rather
+ * than a captured reference) preserves test observability — tests can spy
+ * on `closeFreshTtyForwarding` and see it fire even on branches that didn't
+ * install forwarding, matching the semantics of the pre-`using`
+ * try/finally pattern. The underlying function is a no-op when
+ * `installedState` is null, so extra calls are safe.
+ */
+function makeHandle(): TtyForwardingHandle {
+  return {
+    [Symbol.dispose]: (): void => {
+      closeFreshTtyForwarding();
+    },
+  };
+}
+
 /**
  * Open a fresh `/dev/tty` fd and wire it up to feed `process.stdin`'s event
- * listeners. Returns `true` if the forwarding was installed, `false` if
- * there's no TTY available or `/dev/tty` can't be opened.
+ * listeners.
  *
- * Safe to call unconditionally at interactive-command entry: if `isatty(0)`
- * is false we skip (non-interactive piped input should stay as-is so
- * `--yes`/non-TTY guards keep working). Idempotent — repeated calls after
- * the first successful install are no-ops, so callers don't duplicate the
- * data listener (which would cause clack to receive each keystroke twice)
- * or leak additional `/dev/tty` fds.
+ * Always returns a {@link TtyForwardingHandle} (a `Disposable`) so callers
+ * can use `using tty = forwardFreshTtyToStdin()` to guarantee teardown on
+ * every exit path without null-checking. When no TTY is available or
+ * `/dev/tty` can't be opened the disposable is a no-op by virtue of
+ * `closeFreshTtyForwarding` short-circuiting on un-installed state — the
+ * wizard still runs; non-interactive (`--yes`, piped stdin) flows stay as-is.
+ *
+ * Idempotent: repeated calls after the first successful install return a
+ * pure no-op `Disposable` (the first caller owns teardown). Secondary
+ * callers don't duplicate the data listener (which would cause clack to
+ * receive each keystroke twice) or leak additional `/dev/tty` fds.
+ *
+ * @param deps - Optional dependency injection for tests. `openTty` overrides
+ *   the `/dev/tty` factory; `isTty` overrides the `isatty(0)` predicate.
+ *   Production callers pass no args — the defaults do the right thing.
  */
-export function forwardFreshTtyToStdin(): boolean {
+export function forwardFreshTtyToStdin(
+  deps: TtyDeps = {}
+): TtyForwardingHandle {
+  const { openTty = defaultOpenTty, isTty = defaultIsTty } = deps;
+
   if (installedState) {
-    return true;
+    // Another caller already installed forwarding and owns teardown. Hand
+    // back a pure no-op so disposing the secondary handle does NOT call
+    // `closeFreshTtyForwarding` — which would tear down the primary's
+    // install before the primary's disposable fires.
+    return NOOP_HANDLE;
   }
-  if (!isatty(0)) {
-    return false;
+  if (!isTty()) {
+    return makeHandle();
   }
 
   let fd: number;
   try {
-    fd = openSync("/dev/tty", "r");
+    fd = openTty();
   } catch {
-    return false;
+    return makeHandle();
   }
 
   const fresh = new ReadStream(fd);
@@ -85,14 +164,26 @@ export function forwardFreshTtyToStdin(): boolean {
     read: stdinHandle._read,
   };
 
-  // Bun's compiled binary can leave `process.stdin.isTTY === undefined` on
-  // inherited-via-redirect fds even when `isatty(0)` is true. Clack gates
-  // its internal `setRawMode(true)` call on `input.isTTY`, so without this
-  // backfill the patched setRawMode below is never invoked and the fresh
-  // fd stays in canonical mode (line-buffered, no keypresses).
+  // Capture the current `isTTY` value before touching it so teardown can
+  // restore it verbatim. Bun's compiled binary can leave
+  // `process.stdin.isTTY === undefined` on inherited-via-redirect fds even
+  // when `isatty(0)` is true. Clack gates its internal `setRawMode(true)`
+  // call on `input.isTTY`, so without this backfill the patched setRawMode
+  // below is never invoked and the fresh fd stays in canonical mode
+  // (line-buffered, no keypresses).
+  //
+  // Use `Object.defineProperty` rather than plain assignment because on
+  // some Node/Bun runtimes `process.stdin.isTTY` is defined as a
+  // non-writable property (notably when stdin is not a TTY) — bare
+  // `stdin.isTTY = …` throws a TypeError in strict mode in that case.
+  const previousIsTty = process.stdin.isTTY;
   let backfilledIsTty = false;
   if (process.stdin.isTTY === undefined) {
-    (process.stdin as { isTTY?: boolean }).isTTY = true;
+    Object.defineProperty(process.stdin, "isTTY", {
+      value: true,
+      writable: true,
+      configurable: true,
+    });
     backfilledIsTty = true;
   }
 
@@ -107,11 +198,14 @@ export function forwardFreshTtyToStdin(): boolean {
   // A ReadStream without an `error` listener crashes the process when it
   // emits (e.g. terminal disconnected, SSH dropped). The wizard can't
   // recover from a dead TTY, so silently drop — the next operation that
-  // actually needs input will fail with a more meaningful error.
-  const errorListener = (): void => {
+  // actually needs input will fail with a more meaningful error. The
+  // listener stays attached across teardown intentionally: Bun can
+  // asynchronously emit `'error'` (EBADF) after `destroy()` closes the
+  // underlying fd, and an unhandled error on the stream crashes the
+  // process. Keeping the listener attached absorbs any late emission.
+  fresh.on("error", (): void => {
     // intentionally empty
-  };
-  fresh.on("error", errorListener);
+  });
 
   // setRawMode issues a TCSETS ioctl on the underlying TTY device. The device
   // is shared between the broken fd 0 and the fresh fd, but the broken fd's
@@ -142,12 +236,12 @@ export function forwardFreshTtyToStdin(): boolean {
   installedState = {
     fresh,
     dataListener,
-    errorListener,
     original,
+    previousIsTty,
     backfilledIsTty,
   };
 
-  return true;
+  return makeHandle();
 }
 
 /**
@@ -157,20 +251,37 @@ export function forwardFreshTtyToStdin(): boolean {
  * Must be safe on every wizard exit path, including when forwarding was never
  * installed. Destroying the temporary `ReadStream` releases the TTY handle so
  * the process can exit naturally once the wizard is done.
+ *
+ * Callers who opt into the {@link TtyForwardingHandle} `Disposable` API (via
+ * `using tty = forwardFreshTtyToStdin()`) get this teardown for free — this
+ * function exists for the imperative API and for explicit cleanup in tests.
  */
 export function closeFreshTtyForwarding(): void {
   if (!installedState) {
     return;
   }
 
-  const { fresh, dataListener, errorListener, original, backfilledIsTty } =
+  const { fresh, dataListener, original, previousIsTty, backfilledIsTty } =
     installedState;
   installedState = null;
 
   fresh.off("data", dataListener);
-  fresh.off("error", errorListener);
+
+  // Restore termios before destroying the fresh stream. If the wizard threw
+  // mid-prompt (between clack's `setRawMode(true)` and its matching
+  // `setRawMode(false)`), the TTY is still in raw mode — leaving it there
+  // produces a shell with no echo after a crash. Best-effort: the fresh fd
+  // may already be destroyed from a prior error, so swallow any throw.
+  try {
+    fresh.setRawMode(false);
+  } catch {
+    // intentionally empty — stream already torn down
+  }
+
   // Pause before destroy so no queued read callback tries to deliver bytes
-  // after the stream has been torn down.
+  // after the stream has been torn down. The error listener installed at
+  // setup time stays attached across destroy — see the comment at the
+  // install site for why.
   fresh.pause();
   fresh.destroy();
 
@@ -181,6 +292,10 @@ export function closeFreshTtyForwarding(): void {
   stdinHandle._read = original.read;
 
   if (backfilledIsTty) {
-    (process.stdin as { isTTY?: boolean }).isTTY = undefined;
+    Object.defineProperty(process.stdin, "isTTY", {
+      value: previousIsTty,
+      writable: true,
+      configurable: true,
+    });
   }
 }

src/lib/init/wizard-runner.ts

@@ -38,10 +38,7 @@ import { checkGitStatus } from "./git.js";
 import { handleInteractive } from "./interactive.js";
 import { resolveInitContext } from "./preflight.js";
 import { createWizardSpinner } from "./spinner.js";
-import {
-  closeFreshTtyForwarding,
-  forwardFreshTtyToStdin,
-} from "./stdin-reopen.js";
+import { forwardFreshTtyToStdin } from "./stdin-reopen.js";
 import { describeTool, executeTool } from "./tools/registry.js";
 import type {
   ResolvedInitContext,
@@ -321,14 +318,6 @@ async function preamble(
   yes: boolean,
   dryRun: boolean
 ): Promise<boolean> {
-  // Bun's compiled binaries don't deliver keystrokes through TTY fds
-  // inherited via shell redirection (e.g. `curl | bash` →
-  // `exec sentry init </dev/tty` in install.sh). Open a fresh `/dev/tty` and
-  // forward its data events onto process.stdin so clack's prompts receive
-  // input. Also backfills process.stdin.isTTY when Bun leaves it undefined
-  // so clack's internal `isTTY && setRawMode(true)` gate still fires.
-  forwardFreshTtyToStdin();
-
   if (!(yes || dryRun || process.stdin.isTTY)) {
     throw new WizardError(
       "Interactive mode requires a terminal. Use --yes for non-interactive mode.",
@@ -370,18 +359,22 @@ async function preamble(
   return true;
 }
 
+// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: sequential wizard orchestration with error handling branches
 export async function runWizard(initialOptions: WizardOptions): Promise<void> {
-  try {
-    await runWizardInner(initialOptions);
-  } finally {
-    // The wizard owns the temporary `/dev/tty` forwarding installed in
-    // preamble(), so teardown must run on every exit path.
-    closeFreshTtyForwarding();
-  }
-}
+  // Bun's compiled binaries don't deliver keystrokes through TTY fds
+  // inherited via shell redirection (e.g. `curl | bash` →
+  // `exec sentry init </dev/tty` in install.sh). Open a fresh `/dev/tty` and
+  // forward its data events onto process.stdin so clack's prompts receive
+  // input. Also backfills process.stdin.isTTY when Bun leaves it undefined
+  // so clack's internal `isTTY && setRawMode(true)` gate still fires.
+  //
+  // The `using` declaration guarantees teardown on every exit path (success,
+  // return, throw) via the Disposable returned by forwardFreshTtyToStdin().
+  // No explicit try/finally needed — the compiler inserts the call for us.
+  // `_tty` binds the disposable's lifetime to this function scope; the
+  // leading underscore signals it's lifecycle-only and not read below.
+  using _tty = forwardFreshTtyToStdin();
 
-// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: sequential wizard orchestration with error handling branches
-async function runWizardInner(initialOptions: WizardOptions): Promise<void> {
   const { directory, yes, dryRun, features } = initialOptions;
 
   if (!(await preamble(directory, yes, dryRun))) {