From d126f7e08c0d208135460b334da5e976acd15349 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 23:07:09 +0000 Subject: [PATCH 1/3] fix(init): harden /dev/tty teardown and adopt Symbol.dispose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hardening on top of #802. Promotes the explicit try/finally for `closeFreshTtyForwarding()` to a `using` declaration so teardown is guaranteed by the compiler on every exit path (success, early return, throw, unknown). Correctness fixes: - Capture the pre-install `process.stdin.isTTY` value and restore it verbatim instead of hardcoding `undefined` — future- proofs against concurrent writers. - Restore termios (`fresh.setRawMode(false)`) before destroying the fresh stream so a mid-prompt crash doesn't leave the user's shell in raw mode with no echo. - Keep the original `error` listener attached across `destroy()` — Bun can asynchronously emit EBADF after the fd closes, which would otherwise crash the process. API refactor: - `forwardFreshTtyToStdin` now accepts an optional `TtyDeps` object (`{ openTty, isTty }`) for dependency injection in tests. Production callers pass no args. - Returns a `Disposable` (`TtyForwardingHandle`) unconditionally so `using tty = forwardFreshTtyToStdin()` works without null checks. No-install cases return a disposable that routes through `closeFreshTtyForwarding` (a documented no-op in that state) which preserves test observability of the lifecycle call. - Secondary callers (already-installed) get a pure no-op disposable so they don't accidentally tear down the primary. - `runWizardInner` merges back into `runWizard`; the outer try/finally disappears in favor of `using _tty`. Tests: - 11 new tests covering install/teardown round trip, isTTY backfill branches, re-install after teardown, secondary-caller no-op semantics, termios restoration, `using`-scope lifetime, and `using`-on-throw teardown. Uses `/dev/ptmx` as a pseudo- TTY fixture so the install path runs in piped-stdin `bun test` environments. Skips gracefully on platforms without `/dev/ptmx`. - New wizard-runner test covers `WizardError` rethrow spinner- stop side effect. - Existing early-return tests (preflight null, git-safety fail) now assert `closeFreshTtyForwardingSpy` fires once via `using` teardown. --- src/lib/init/stdin-reopen.ts | 150 ++++++++++-- src/lib/init/wizard-runner.ts | 37 ++- test/lib/init/stdin-reopen.test.ts | 339 +++++++++++++++++++++++++++- test/lib/init/wizard-runner.test.ts | 32 +++ 4 files changed, 509 insertions(+), 49 deletions(-) diff --git a/src/lib/init/stdin-reopen.ts b/src/lib/init/stdin-reopen.ts index 2441ed664..b0a7406e2 100644 --- a/src/lib/init/stdin-reopen.ts +++ b/src/lib/init/stdin-reopen.ts @@ -44,36 +44,116 @@ type InstalledState = { 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. + * + * 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. * - * 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. + * @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,11 +165,14 @@ 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). + const previousIsTty = process.stdin.isTTY; let backfilledIsTty = false; if (process.stdin.isTTY === undefined) { (process.stdin as { isTTY?: boolean }).isTTY = true; @@ -144,10 +227,11 @@ export function forwardFreshTtyToStdin(): boolean { dataListener, errorListener, original, + previousIsTty, backfilledIsTty, }; - return true; + return makeHandle(); } /** @@ -157,20 +241,38 @@ 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. Keep the original `errorListener` + // attached across destroy — Bun may asynchronously emit `'error'` (EBADF) + // after destroy when the underlying fd closes, and an unhandled error on + // the stream crashes the process. fresh.pause(); fresh.destroy(); @@ -181,6 +283,6 @@ export function closeFreshTtyForwarding(): void { stdinHandle._read = original.read; if (backfilledIsTty) { - (process.stdin as { isTTY?: boolean }).isTTY = undefined; + (process.stdin as { isTTY?: boolean }).isTTY = previousIsTty; } } diff --git a/src/lib/init/wizard-runner.ts b/src/lib/init/wizard-runner.ts index 8c1f2ec00..be81314c1 100644 --- a/src/lib/init/wizard-runner.ts +++ b/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 { - // Bun's compiled binaries don't deliver keystrokes through TTY fds - // inherited via shell redirection (e.g. `curl | bash` → - // `exec sentry init { - 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 { const { directory, yes, dryRun, features } = initialOptions; if (!(await preamble(directory, yes, dryRun))) { diff --git a/test/lib/init/stdin-reopen.test.ts b/test/lib/init/stdin-reopen.test.ts index a071da6ba..aad21961e 100644 --- a/test/lib/init/stdin-reopen.test.ts +++ b/test/lib/init/stdin-reopen.test.ts @@ -1,7 +1,97 @@ -import { describe, expect, test } from "bun:test"; -import { closeFreshTtyForwarding } from "../../../src/lib/init/stdin-reopen.js"; +/** + * Tests for the `/dev/tty` forwarding install→teardown lifecycle. + * + * The production code opens a real `/dev/tty` fd via `openSync` and checks + * `isatty(0)` for the environment gate. To exercise the state transitions + * deterministically we pass `TtyDeps` that: + * + * - override `isTty` to return `true` (bun test runs with piped stdin, so + * the default predicate short-circuits the install path), + * - override `openTty` to return a `/dev/ptmx` fd — a pseudo-TTY master + * that `new ReadStream(fd)` accepts, with no keyboard side effects. + * + * `/dev/ptmx` is Linux-specific. Tests skip gracefully on platforms where it + * isn't available. + */ -describe("closeFreshTtyForwarding", () => { +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { existsSync, openSync } from "node:fs"; +import { + closeFreshTtyForwarding, + forwardFreshTtyToStdin, +} from "../../../src/lib/init/stdin-reopen.js"; + +const HAS_PTMX = existsSync("/dev/ptmx"); + +/** + * Open a fresh `/dev/ptmx` fd for use as a pseudo-TTY fixture. The returned + * fd is owned by the `ReadStream` that the test attaches it to — + * `fresh.destroy()` inside teardown closes it. No explicit close needed + * from the test body. + */ +function makePtmxFd(): { fd: number } { + const fd = openSync("/dev/ptmx", "r+"); + return { fd }; +} + +/** + * `bun test` runs with piped stdin, so `process.stdin.setRawMode` is + * typically `undefined` (the method only exists on real TTY streams). + * The production code captures these methods at install time via + * `stdinHandle.setRawMode` etc., so we stub them on `process.stdin` before + * each test and restore after. This matches what the real Bun binary would + * provide when stdin is actually a TTY. + */ +type StdinStubState = { + restore: () => void; +}; + +function stubStdinTtyMethods(): StdinStubState { + const stdin = process.stdin as unknown as Record; + const keys = ["setRawMode", "pause", "resume", "_read"] as const; + const originals: Record = {}; + const hadKey: Record = {}; + for (const key of keys) { + hadKey[key] = key in stdin; + originals[key] = stdin[key]; + } + // Install test stubs. Each stub returns process.stdin so chainable-style + // callers (clack's setRawMode, etc.) behave sensibly. + stdin.setRawMode = (_mode: boolean) => process.stdin; + stdin.pause = () => process.stdin; + stdin.resume = () => process.stdin; + stdin._read = (_size: number) => { + // intentionally empty — test stub + }; + + return { + restore: () => { + for (const key of keys) { + if (hadKey[key]) { + stdin[key] = originals[key]; + } else { + delete stdin[key]; + } + } + }, + }; +} + +let stdinStub: StdinStubState | undefined; + +beforeEach(() => { + stdinStub = stubStdinTtyMethods(); +}); + +afterEach(() => { + // Defense-in-depth: if a test installs forwarding and throws before + // tearing down, reset the module state before the next test runs. + closeFreshTtyForwarding(); + stdinStub?.restore(); + stdinStub = undefined; +}); + +describe("closeFreshTtyForwarding (null-state paths)", () => { test("is a no-op when forwarding was never installed", () => { expect(() => closeFreshTtyForwarding()).not.toThrow(); }); @@ -14,3 +104,246 @@ describe("closeFreshTtyForwarding", () => { }).not.toThrow(); }); }); + +describe("forwardFreshTtyToStdin no-install paths", () => { + test("does not patch stdin methods when isTty predicate is false", () => { + const originalSetRawMode = process.stdin.setRawMode; + const handle = forwardFreshTtyToStdin({ isTty: () => false }); + // Handle is a Disposable but install was skipped — stdin untouched. + expect(handle).toBeDefined(); + expect(process.stdin.setRawMode).toBe(originalSetRawMode); + // Disposing the no-install handle is a safe no-op. + expect(() => handle[Symbol.dispose]()).not.toThrow(); + expect(process.stdin.setRawMode).toBe(originalSetRawMode); + }); + + test("does not patch stdin methods when the openTty factory throws", () => { + const originalSetRawMode = process.stdin.setRawMode; + const openTty = mock(() => { + throw new Error("fake /dev/tty unavailable"); + }); + const handle = forwardFreshTtyToStdin({ isTty: () => true, openTty }); + expect(handle).toBeDefined(); + expect(openTty).toHaveBeenCalledTimes(1); + expect(process.stdin.setRawMode).toBe(originalSetRawMode); + }); +}); + +describe("forwardFreshTtyToStdin → closeFreshTtyForwarding round trip", () => { + if (!HAS_PTMX) { + test("skipped: /dev/ptmx unavailable on this platform", () => { + expect(HAS_PTMX).toBe(false); + }); + return; + } + + test("install captures and teardown restores stdin methods", () => { + const { fd } = makePtmxFd(); + const openTty = mock(() => fd); + + const originalSetRawMode = process.stdin.setRawMode; + const originalPause = process.stdin.pause; + const originalResume = process.stdin.resume; + + const handle = forwardFreshTtyToStdin({ isTty: () => true, openTty }); + expect(handle).toBeDefined(); + expect(openTty).toHaveBeenCalledTimes(1); + + // After install, process.stdin methods are patched — they no longer + // match the pre-install references. + expect(process.stdin.setRawMode).not.toBe(originalSetRawMode); + expect(process.stdin.pause).not.toBe(originalPause); + expect(process.stdin.resume).not.toBe(originalResume); + + closeFreshTtyForwarding(); + + // After teardown the originals are restored by reference equality. + expect(process.stdin.setRawMode).toBe(originalSetRawMode); + expect(process.stdin.pause).toBe(originalPause); + expect(process.stdin.resume).toBe(originalResume); + }); + + test("isTTY restored to its pre-install value (backfill branch)", () => { + const { fd } = makePtmxFd(); + const stdin = process.stdin as { isTTY?: boolean }; + const previousIsTty = stdin.isTTY; + + // Force the backfill branch by clearing isTTY up-front. + stdin.isTTY = undefined; + + try { + const handle = forwardFreshTtyToStdin({ + isTty: () => true, + openTty: () => fd, + }); + expect(handle).toBeDefined(); + // Install backfilled isTTY to true. + expect(stdin.isTTY).toBe(true); + + closeFreshTtyForwarding(); + // Teardown restored to the pre-install value (undefined). + expect(stdin.isTTY).toBeUndefined(); + } finally { + stdin.isTTY = previousIsTty; + } + }); + + test("isTTY untouched when already set (no-backfill branch)", () => { + const { fd } = makePtmxFd(); + const stdin = process.stdin as { isTTY?: boolean }; + const previousIsTty = stdin.isTTY; + stdin.isTTY = true; + + try { + const handle = forwardFreshTtyToStdin({ + isTty: () => true, + openTty: () => fd, + }); + expect(handle).toBeDefined(); + expect(stdin.isTTY).toBe(true); + + closeFreshTtyForwarding(); + // backfilledIsTty was false, so teardown must not touch isTTY. + expect(stdin.isTTY).toBe(true); + } finally { + stdin.isTTY = previousIsTty; + } + }); + + test("re-install after teardown succeeds with fresh state", () => { + const first = makePtmxFd(); + const second = makePtmxFd(); + + const h1 = forwardFreshTtyToStdin({ + isTty: () => true, + openTty: () => first.fd, + }); + expect(h1).toBeDefined(); + closeFreshTtyForwarding(); + + // Second install observes the newly-restored original methods as its + // capture target, so the cycle completes cleanly. + const h2 = forwardFreshTtyToStdin({ + isTty: () => true, + openTty: () => second.fd, + }); + expect(h2).toBeDefined(); + closeFreshTtyForwarding(); + }); + + test("secondary forward call returns a no-op disposable", () => { + const { fd } = makePtmxFd(); + const stdin = process.stdin as { isTTY?: boolean }; + const previousIsTty = stdin.isTTY; + + try { + const h1 = forwardFreshTtyToStdin({ + isTty: () => true, + openTty: () => fd, + }); + expect(h1).toBeDefined(); + + // Second call — already installed. Factory NOT called again. + const secondaryFactory = mock(() => { + throw new Error("should not be invoked"); + }); + const h2 = forwardFreshTtyToStdin({ + isTty: () => true, + openTty: secondaryFactory, + }); + expect(h2).toBeDefined(); + expect(secondaryFactory).not.toHaveBeenCalled(); + + // Disposing the secondary handle must NOT tear down the primary's + // state. stdin methods stay patched until the PRIMARY disposable fires. + const patchedSetRawMode = process.stdin.setRawMode; + h2[Symbol.dispose](); + expect(process.stdin.setRawMode).toBe(patchedSetRawMode); + + // Primary disposal now actually tears down. + h1[Symbol.dispose](); + } finally { + stdin.isTTY = previousIsTty; + } + }); + + test("teardown tolerates double-close of the same install", () => { + const { fd } = makePtmxFd(); + + const handle = forwardFreshTtyToStdin({ + isTty: () => true, + openTty: () => fd, + }); + expect(handle).toBeDefined(); + + // First close tears down; second must be a no-op guarded by the + // installedState === null check at the top of closeFreshTtyForwarding. + closeFreshTtyForwarding(); + expect(() => closeFreshTtyForwarding()).not.toThrow(); + }); + + test("teardown tolerates raw mode being set before close", () => { + const { fd } = makePtmxFd(); + + const handle = forwardFreshTtyToStdin({ + isTty: () => true, + openTty: () => fd, + }); + expect(handle).toBeDefined(); + + // Simulate clack calling setRawMode(true) mid-prompt, then the + // wizard throwing before clack's matching setRawMode(false) fires. + // The patched setRawMode routes to fresh.setRawMode on the real + // ptmx fd — exercising the ioctl path. + process.stdin.setRawMode(true); + + // Teardown should call fresh.setRawMode(false) before destroy. + // We can't observe the call directly without a deeper seam, but we + // verify teardown completes without throwing even after raw mode + // was set (covers the termios-restore try/catch). + expect(() => closeFreshTtyForwarding()).not.toThrow(); + }); +}); + +describe("using-declaration semantics", () => { + if (!HAS_PTMX) { + test("skipped: /dev/ptmx unavailable on this platform", () => { + expect(HAS_PTMX).toBe(false); + }); + return; + } + + test("`using` scope releases forwarding when the block exits", () => { + const { fd } = makePtmxFd(); + const originalSetRawMode = process.stdin.setRawMode; + + { + using tty = forwardFreshTtyToStdin({ + isTty: () => true, + openTty: () => fd, + }); + expect(tty).toBeDefined(); + // Inside the block, setRawMode is patched. + expect(process.stdin.setRawMode).not.toBe(originalSetRawMode); + } + // Block exited → disposable fired → originals restored. + expect(process.stdin.setRawMode).toBe(originalSetRawMode); + }); + + test("`using` teardown fires even when the block throws", () => { + const { fd } = makePtmxFd(); + const originalSetRawMode = process.stdin.setRawMode; + + const run = (): void => { + using tty = forwardFreshTtyToStdin({ + isTty: () => true, + openTty: () => fd, + }); + expect(tty).toBeDefined(); + throw new Error("boom"); + }; + expect(run).toThrow("boom"); + // Throw unwound the `using` scope → disposable fired. + expect(process.stdin.setRawMode).toBe(originalSetRawMode); + }); +}); diff --git a/test/lib/init/wizard-runner.test.ts b/test/lib/init/wizard-runner.test.ts index f607568db..e73bc339e 100644 --- a/test/lib/init/wizard-runner.test.ts +++ b/test/lib/init/wizard-runner.test.ts @@ -246,6 +246,8 @@ describe("runWizard", () => { expect(getWorkflowSpy).not.toHaveBeenCalled(); expect(formatResultSpy).not.toHaveBeenCalled(); + // Early-return paths must still tear down TTY forwarding via `using`. + expect(closeFreshTtyForwardingSpy).toHaveBeenCalledTimes(1); }); test("aborts cleanly when git safety check fails", async () => { @@ -255,6 +257,8 @@ describe("runWizard", () => { expect(cancelSpy).toHaveBeenCalledWith("Setup cancelled."); expect(getWorkflowSpy).not.toHaveBeenCalled(); + // Early-return paths must still tear down TTY forwarding via `using`. + expect(closeFreshTtyForwardingSpy).toHaveBeenCalledTimes(1); }); test("dispatches tool payloads through the registry", async () => { @@ -407,6 +411,34 @@ describe("runWizard", () => { expect(closeFreshTtyForwardingSpy).toHaveBeenCalledTimes(1); }); + test("tears down forwarding when a WizardError is rethrown from a tool", async () => { + // The reordered catch block stops the spinner BEFORE the WizardError + // rethrow branch, so any WizardError thrown from handleSuspendedStep + // (e.g. tool handlers, malformed payloads) must still release the TTY + // handle via `using` teardown. + const payload: ToolPayload = { + type: "tool", + operation: "run-commands", + cwd: "/tmp/test", + params: { commands: ["npm install @sentry/node"] }, + }; + mockStartResult = { + status: "suspended", + suspended: [["install-deps"]], + steps: { + "install-deps": { suspendPayload: payload }, + }, + }; + executeToolSpy.mockRejectedValue( + new WizardError("tool rejected by server") + ); + + await expect(runWizard(makeOptions())).rejects.toThrow(WizardError); + + expect(spinnerMock.stop).toHaveBeenCalledWith("Error", 1); + expect(closeFreshTtyForwardingSpy).toHaveBeenCalledTimes(1); + }); + test("shows a multiline tree while reading files and then analyzing them", async () => { mockStartResult = { status: "suspended", From 65a5b2887b542bbc828dfa5e9065df576bac4f3b Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 23 Apr 2026 09:09:10 +0000 Subject: [PATCH 2/3] fix(init): use Object.defineProperty for isTTY assignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On CI and some Node/Bun runtimes, `process.stdin.isTTY` is defined as a non-writable property, so bare `stdin.isTTY = …` throws `TypeError: Attempted to assign to readonly property`. `Object.defineProperty` with `writable: true, configurable: true` overrides the existing descriptor in-place, working on every runtime. Affects both: - Production code — the `isTTY` backfill when forwarding is installed, and the restore step in teardown. - Tests — the `setIsTTY` helper used by the backfill-branch and already-set tests. Lives on `main` pre-#802 because the previous code only ran when `isTTY === undefined` AND `isatty(0) === true`, which doesn't hit on piped-stdin CI. The new test suite forces `isTty: () => true` to exercise the backfill path, surfacing the latent bug. --- src/lib/init/stdin-reopen.ts | 17 +++++++++++++++-- test/lib/init/stdin-reopen.test.ts | 25 ++++++++++++++++++++----- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/lib/init/stdin-reopen.ts b/src/lib/init/stdin-reopen.ts index b0a7406e2..685f12577 100644 --- a/src/lib/init/stdin-reopen.ts +++ b/src/lib/init/stdin-reopen.ts @@ -172,10 +172,19 @@ export function forwardFreshTtyToStdin( // 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; } @@ -283,6 +292,10 @@ export function closeFreshTtyForwarding(): void { stdinHandle._read = original.read; if (backfilledIsTty) { - (process.stdin as { isTTY?: boolean }).isTTY = previousIsTty; + Object.defineProperty(process.stdin, "isTTY", { + value: previousIsTty, + writable: true, + configurable: true, + }); } } diff --git a/test/lib/init/stdin-reopen.test.ts b/test/lib/init/stdin-reopen.test.ts index aad21961e..89a5bee88 100644 --- a/test/lib/init/stdin-reopen.test.ts +++ b/test/lib/init/stdin-reopen.test.ts @@ -34,6 +34,21 @@ function makePtmxFd(): { fd: number } { return { fd }; } +/** + * Assign `process.stdin.isTTY` via `Object.defineProperty` so the test + * works regardless of whether the runtime defined `isTTY` as writable or + * readonly. On CI (Node/Bun with piped stdin), `isTTY` is a non-writable + * property and bare assignment throws `TypeError: Attempted to assign to + * readonly property`. `defineProperty` overrides the descriptor in-place. + */ +function setIsTTY(value: boolean | undefined): void { + Object.defineProperty(process.stdin, "isTTY", { + value, + writable: true, + configurable: true, + }); +} + /** * `bun test` runs with piped stdin, so `process.stdin.setRawMode` is * typically `undefined` (the method only exists on real TTY streams). @@ -169,7 +184,7 @@ describe("forwardFreshTtyToStdin → closeFreshTtyForwarding round trip", () => const previousIsTty = stdin.isTTY; // Force the backfill branch by clearing isTTY up-front. - stdin.isTTY = undefined; + setIsTTY(undefined); try { const handle = forwardFreshTtyToStdin({ @@ -184,7 +199,7 @@ describe("forwardFreshTtyToStdin → closeFreshTtyForwarding round trip", () => // Teardown restored to the pre-install value (undefined). expect(stdin.isTTY).toBeUndefined(); } finally { - stdin.isTTY = previousIsTty; + setIsTTY(previousIsTty); } }); @@ -192,7 +207,7 @@ describe("forwardFreshTtyToStdin → closeFreshTtyForwarding round trip", () => const { fd } = makePtmxFd(); const stdin = process.stdin as { isTTY?: boolean }; const previousIsTty = stdin.isTTY; - stdin.isTTY = true; + setIsTTY(true); try { const handle = forwardFreshTtyToStdin({ @@ -206,7 +221,7 @@ describe("forwardFreshTtyToStdin → closeFreshTtyForwarding round trip", () => // backfilledIsTty was false, so teardown must not touch isTTY. expect(stdin.isTTY).toBe(true); } finally { - stdin.isTTY = previousIsTty; + setIsTTY(previousIsTty); } }); @@ -263,7 +278,7 @@ describe("forwardFreshTtyToStdin → closeFreshTtyForwarding round trip", () => // Primary disposal now actually tears down. h1[Symbol.dispose](); } finally { - stdin.isTTY = previousIsTty; + setIsTTY(previousIsTty); } }); From cef54a13d473baba974a61cb5bbe78a366994675 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 23 Apr 2026 09:18:58 +0000 Subject: [PATCH 3/3] fix(init): remove dead errorListener field from InstalledState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor Bugbot caught that the `errorListener` field on `InstalledState` was stored at install time but never read back in teardown — the old code called `fresh.off('error', errorListener)` which justified storing it, but the post-#802 teardown intentionally keeps the listener attached (see the install-site comment) to absorb Bun's asynchronous EBADF emission after `destroy()`. Inline the listener as an anonymous callback at the install site, drop the field from the type, and update the teardown comment to reference the install-site rationale instead of repeating it. --- src/lib/init/stdin-reopen.ts | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/lib/init/stdin-reopen.ts b/src/lib/init/stdin-reopen.ts index 685f12577..81379ae6c 100644 --- a/src/lib/init/stdin-reopen.ts +++ b/src/lib/init/stdin-reopen.ts @@ -37,7 +37,6 @@ type StdinHandle = { type InstalledState = { fresh: ReadStream; dataListener: (chunk: Buffer) => void; - errorListener: () => void; original: { setRawMode: StdinHandle["setRawMode"]; pause: StdinHandle["pause"]; @@ -199,11 +198,14 @@ export function forwardFreshTtyToStdin( // 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 @@ -234,7 +236,6 @@ export function forwardFreshTtyToStdin( installedState = { fresh, dataListener, - errorListener, original, previousIsTty, backfilledIsTty, @@ -278,10 +279,9 @@ export function closeFreshTtyForwarding(): void { } // Pause before destroy so no queued read callback tries to deliver bytes - // after the stream has been torn down. Keep the original `errorListener` - // attached across destroy — Bun may asynchronously emit `'error'` (EBADF) - // after destroy when the underlying fd closes, and an unhandled error on - // the stream crashes the process. + // 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();