diff --git a/scripts/prompt-ownership-pty.fixture.ts b/scripts/prompt-ownership-pty.fixture.ts new file mode 100644 index 00000000..3251b322 --- /dev/null +++ b/scripts/prompt-ownership-pty.fixture.ts @@ -0,0 +1,107 @@ +/** Offline synthetic installer only. Driven by prompt-ownership-pty.py. */ +import { mock } from 'bun:test'; +import assert from 'node:assert/strict'; +import { appendFile } from 'node:fs/promises'; + +const [mode, scenario, evidence] = process.argv.slice(2); +assert(['cli', 'tui'].includes(mode)); +assert(['answers', 'cancel', 'stop'].includes(scenario)); +assert(evidence && process.stdin.isTTY && process.stdout.isTTY); + +// Fail closed if a future adapter change tries to access credentials/network. +const forbidden = () => { + throw new Error('Credential/network access is forbidden in this fixture'); +}; +mock.module('../src/lib/config-store.js', () => ({ + getActiveEnvironment: forbidden, + isUnclaimedEnvironment: forbidden, + profileEnvironmentLabel: forbidden, +})); +mock.module('@napi-rs/keyring', () => ({ + Entry: class { + constructor() { + forbidden(); + } + }, +})); +mock.module('../src/lib/darwin-keychain.js', () => ({ + DarwinSecurityEntry: class { + constructor() { + forbidden(); + } + }, +})); +// Reject asynchronously like fetch does, allowing Yoga's embedded-WASM fallback. +globalThis.fetch = async () => forbidden(); + +const { createInstallerEventEmitter } = await import('../src/lib/events.js'); +const { CLIAdapter } = await import('../src/lib/adapters/cli-adapter.js'); +const { TuiAdapter } = await import('../src/lib/adapters/tui-adapter.js'); +const { getUiHost } = await import('../src/utils/ui.js'); +const emitter = createInstallerEventEmitter(); +const events: string[] = []; +const report = (stage: string) => appendFile(evidence, `${JSON.stringify({ stage, events })}\n`); +const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); +let finished!: () => void; +const done = new Promise((resolve) => (finished = resolve)); +const inputBefore = process.stdin.listenerCount('readable'); +const config = { + emitter, + sendEvent(event: { type: string }) { + events.push(event.type); + if (event.type === 'GIT_CANCELLED') { + emitter.emit('complete', { success: false, summary: 'Synthetic cancellation' }); + finished(); + } + if (event.type === 'PR_APPROVED') finished(); + }, +}; +const adapter = + mode === 'tui' + ? new TuiAdapter({ ...config, installDir: process.cwd(), tipIntervalMs: 60_000 }) + : new CLIAdapter(config); + +// Driver enforces its own process-group deadline too. +const timeout = setTimeout(() => { + throw new Error('PTY fixture timed out'); +}, 10_000); +try { + await adapter.start(); + // Ink's restore-cursor dependency deliberately keeps a process-exit hook; + // check the adapter's own SIGINT listener, not that shared library hook. + const cliSigint = process.listeners('SIGINT').find((listener) => listener.name === 'handleSigInt'); + assert(cliSigint); + emitter.emit('agent:start', {}); + if (scenario === 'cancel') { + emitter.emit('git:dirty', { files: ['synthetic.rb'] }); + emitter.emit('branch:prompt', { branch: 'main' }); + } else { + emitter.emit('postinstall:commit:prompt', {}); + emitter.emit('postinstall:pr:prompt', {}); + } + await delay(350); + emitter.emit('agent:success', { summary: 'Synthetic Rails/no-validation completion' }); + emitter.emit('postinstall:commit:generating', {}); + emitter.emit('postinstall:pr:generating', {}); + emitter.emit('postinstall:pr:pushing', {}); + emitter.emit('agent:tool', { kind: 'command', detail: 'synthetic-no-op' }); + await report('replaced'); + if (scenario === 'stop') await delay(400); + else await done; + await adapter.stop(); + await report('stopped'); + assert.equal(getUiHost(), null); + assert.equal(process.stdin.isRaw, false); + assert(!process.listeners('SIGINT').includes(cliSigint)); + assert.equal(process.stdin.listenerCount('readable'), inputBefore); + assert.equal(emitter.listenerCount('agent:start'), 0); + if (scenario === 'answers') assert.deepEqual(events, ['COMMIT_DECLINED', 'PR_APPROVED']); + if (scenario === 'cancel') assert(events.includes('GIT_CANCELLED')); + // Leave the process alive long enough to detect orphaned redraw intervals. + await delay(400); + await report('clean'); +} finally { + clearTimeout(timeout); + await adapter.stop(); + process.stdin.pause(); +} diff --git a/scripts/prompt-ownership-pty.md b/scripts/prompt-ownership-pty.md new file mode 100644 index 00000000..bfc0d08e --- /dev/null +++ b/scripts/prompt-ownership-pty.md @@ -0,0 +1,121 @@ +# AUTH-6732 verification and integration notes + +## Provenance + +- Branch: `riker/13-complete-auth-6732-by-fixing-current-wor` +- Base: `2f199267d93fa5b966e677b77923663ccfee1919` +- Ownership/cancellation change: `bb5102c` +- Logging ownership follow-up: `2a72ab2` +- Adapted from Nick Nisi's [PR #216](https://github.com/workos/cli/pull/216), original commit `1953bed4131f0f1fd532e4291d7d4d0a47a7dfd7`. The actual four-file diff was read before implementation. Attribution is also in the first commit message. + +## Changes + +The UI facade has one spinner owner across terminal and hosted rendering. Replacement, stop, clear, and host teardown retire handles permanently. Only an actually drawn spinner line can be erased. Queued prompt callers reserve the terminal before awaiting input; existing and newly started spinners remain suspended until all questions settle. Handback resumes the current owner, never the entry handle. Terminal completion/error lines wait behind input; hosted warnings/errors remain immediately visible. + +Prompt requests retain their host and are cancelled on host teardown rather than opening a late inquirer question. A one-turn queued handoff lets the installer process cancellation before the next question opens. The adapter counts queued prompt callers, supplies its cancellation signal to every prompt, and drains its handlers before normal hosted teardown. Exit/signal teardown also detaches CLI handlers. + +Agent success ends the spinner without requiring validation (Rails/`--no-validate`); failure, completion, cancellation and stop retire animation. Logging no longer stops/recreates adapter spinners: the facade borrows the current owner's line. This prevents a stale adapter handle from reclaiming an obsolete phase. Hosted logs are delivered in their original phase to preserve transcript filtering. + +No prompt widgets, UI libraries, selection/headless policy, auth recovery, or commit/PR policy were replaced or removed. + +## Automated evidence (Bun 1.4.2, macOS arm64) + +### Deterministic timers and controlled input promises + +`src/utils/ui.spec.ts` uses fake timers and controllable inquirer/host promises. It covers retired timers, inert stale operations (including start), multiple 80ms ticks during input, replacement/update/stop/clear during queued input, current-owner resume, rejection, pre-abort, hosted status ownership/teardown, and JSON/non-TTY guards. + +`src/lib/adapters/cli-adapter.coordination.spec.ts` uses the real facade and real adapter, mocking only the input transport. Synthetic events cover Rails/no-validation and validation completion, queued answer routing, buffered file/tool logs, newer external ownership, cancellation before a queued sibling opens, failure, completion, SIGINT and adapter stop. Tests assert terminal writes and timer counts, not spinner mock call counts. + +Two regressions were observed red before fixing: stale handles erased/restarted terminal output; tool logging resurrected an obsolete adapter phase over a newer facade owner. + +### Fake Ink streams (not PTYs) + +`src/lib/adapters/tui-adapter.spec.ts` uses the existing fake terminal streams and real Ink/CLI/facade code. New tests assert visible questions and resulting installer events during phase changes, password masking, transcript filtering, warning visibility, queued teardown, exit-hook cleanup, raw-mode restoration and removal of input listeners. + +Targeted command: **9 files / 250 tests passed**. + +```sh +bun run test src/utils/ui.spec.ts \ + src/lib/adapters/cli-adapter.spec.ts \ + src/lib/adapters/cli-adapter.coordination.spec.ts \ + src/lib/adapters/tui-adapter.spec.ts \ + src/lib/adapters/headless-adapter.spec.ts \ + src/lib/adapters/select-adapter.spec.ts src/tui +``` + +### Real PTYs + +```sh +python3 scripts/prompt-ownership-pty.py +``` + +**6/6 passed**, separately from the fake-stream tests: + +- CLI/inquirer: queued No/Yes answers, Ctrl-C with a queued sibling, stop during unanswered input. +- Hosted Ink: the same three flows, using the normal live renderer. + +Each subprocess runs at 100×30, keeps a real question open across synthetic agent completion and spinner replacement, and verifies silence over multiple spinner ticks. The driver sends actual PTY bytes. The fixture checks resulting installer events, host cleanup, adapter subscriptions and input listeners. The driver checks restored terminal attributes, alternate-screen exit, cursor restoration, no moot queued question and no stale output after stop. + +The harness uses Python's standard-library PTY facilities, ephemeral HOME/config/temp directories inside this worktree, no inherited credentials, forbidden keyring/config access and rejected network fetches. Yoga uses its embedded-WASM fallback. Waits are bounded and subprocess groups are killed on failure. No installer machine, auth/model call, Git operation, provisioning or publication runs. Fixture events named commit/PR only ask questions; their handler records answers without executing actions. + +Ink's `restore-cursor` dependency deliberately keeps a process-exit hook. Its final show-cursor escape is allowed after teardown; text, erasure, animation and prompts are not. + +### Worker-local full checks and build + +Earlier successful worker-local run (not the supervisor's independent result): + +```sh +bun run test && bun run typecheck && bun run lint && bun run format:check +bun run build +``` + +- Tests: **170 files / 3228 tests passed**. +- TypeScript, oxlint, oxfmt: **passed**. +- Standalone Bun build: **passed**, 2117 modules. +- Dependency installation: `bun install --frozen-lockfile`; no lockfile change. Generation remained in this worktree. + +**Baseline flake, not fixed here:** two full-suite attempts failed in `src/doctor/checks/skills-fix.spec.ts:180/183`, seeing either `null` or only `workos` instead of `workos` plus `workos-widgets`. The test passed in isolation. An untouched archive of the base commit, created and run entirely inside this worktree with its own temp directory, reproduced the missing-widget failure (3202 passed / 1 failed). A subsequent final check on this branch passed in full. Riker may encounter this existing parallel skills-extraction test flake. The temporary baseline archive was removed. + +### Supervisor typecheck timeout investigation + +**Independent verification remains unresolved.** The supervisor reported 3228 passing tests, successful generation, then no completion after `$ tsc --noEmit` before its **600-second overall timeout**. That failure is authoritative for the independent check. The compiler's own exit status is unknown; a timeout is not evidence that tsc returned a nonzero status. No timestamp, raw failed-run log, captured environment or stalled process sample was available from the supervisor. The earlier worker-local passes above do not supersede this result. + +One bounded local reproduction was run at **2026-09-28 14:57:17 -05:00** on unchanged implementation commit `16c103d`. It used the supervisor launch shape reported by Riker from source: `/bin/sh -c`, detached session, stdin `/dev/null`, stdout and stderr in separate pipes, and inherited environment. This matches the reported launcher shape, not a verified deployed supervisor environment. The exact command was unchanged: + +```sh +bun run test && bun run typecheck && bun run lint && bun run format:check +``` + +A temporary Python observer drained both pipes concurrently, timestamped stage markers, sampled only the launched process group, and imposed the same **600-second ceiling**. It did not inject environment overrides, change compiler flags, skip stages, or kill unrelated processes. Only one reproduction was run; no timeout increase or retry loop was used. + +| Local stage | Exit status | Observed wall duration | +| ----------------------------------------- | ----------- | ---------------------- | +| `bun run test`, including generation | 0 | 8.458s | +| `bun run typecheck`, including generation | 0 | 2.727s | +| `bun run lint` | 0 | 0.108s | +| `bun run format:check` | 0 | 0.541s | +| Entire shell chain and pipe EOF | **0** | **11.834s** | + +Stage boundaries were observed from Bun's stderr launch markers, so durations include small observer/scheduling overhead rather than being compiler profiler measurements. Individual successful statuses follow from advancement through `&&`; the final shell status was collected directly. Vitest reported **170 files / 3228 tests passed**, with its internal duration **7.81s**. TypeScript, oxlint and oxfmt all completed. + +Concrete local diagnostics: + +- Bun **1.4.2** resolved to `/Users/nicknisi/.local/share/mise/installs/bun/latest/bin/bun`. +- Node **v24.19.0** resolved to `/Users/nicknisi/.local/share/mise/installs/node/24.19.0/bin/node`. +- Local `node_modules/.bin/tsc` resolves to `node_modules/typescript/bin/tsc`, TypeScript **5.9.3**. `package.json`, `bun.lock`, `tsconfig.json` and `vitest.config.ts` are unchanged from the task base. +- Only the named non-secret environment details were inspected: `NODE_OPTIONS`, `BUN_OPTIONS`, and `CI` unset; `SHELL=/opt/homebrew/bin/zsh`; `TERM=tmux-256color`; inherited PATH resolves the executables above. The actual launched shell was explicitly `/bin/sh`, not `$SHELL`. The supervisor's corresponding values are unknown. +- Shell PID **89055** launched Bun PID **90307**, which launched compiler PID **90334**: `node /Users/nicknisi/.riker/worktrees/13/node_modules/.bin/tsc --noEmit`. +- The compiler command marker appeared at **+8.801s**; lint's marker appeared at **+11.185s**. Compiler samples showed running state, CPU time progressing from **0.64s to 4.79s**, and maximum sampled RSS **623920 KiB**. This was an actively executing compiler, not an observed child/pipe stall. +- `lsof` on that compiler confirmed this worktree as cwd, fd 0 `/dev/null`, and fds 1/2 as pipes. Both pipes reached EOF. The launched process group was empty at **+11.915s**; a subsequent PID check found neither shell nor compiler alive. +- No identifiable stalled tsc process or supervisor log was present when this investigation began. The machine's earlier load averages were **6.58 / 6.39 / 8.39**, but there is no corresponding snapshot from the supervisor failure; this does not establish resource contention as its cause. + +**Conclusion: local reproduction passed; supervisor timeout cause unestablished.** No reproducible job-local defect was identified, so no implementation, dependency, compiler or global configuration change was made. A diagnosis of the independent timeout still needs its raw timestamped output, resolved runtimes/selected environment, and compiler/parent process state and pipe status during the actual stall. Do not infer a compiler, child-process, resource-contention, environment or transport cause from the timeout alone. This is distinct from the separately reproduced skills-test flake above. + +This follow-up changes verification documentation only. The original PR216 attribution, implementation commits, deterministic/fake-stream tests and six successful real-PTY scenarios remain unchanged. The temporary observer and logs were removed after recording these diagnostics. + +## Limits and integration + +- PTY evidence is macOS arm64, Bun 1.4.2, one supported terminal size. Linux/Windows terminals, resize/wrapping, SSH/multiplexer behavior and human visual inspection were not verified by this harness. No live Rails/AI install was run. +- AUTH-6733/6735 may overlap `src/utils/ui.ts` and `src/lib/adapters/cli-adapter.ts`. Preserve facade-owned retirement and log pause/resume; do not reintroduce adapter stop/restart around logs or unconditional erasure before a new phase. +- Keep cancellation signals on every prompt and count queued callers, not a boolean. Preserve awaited CLI stop before hosted teardown. Auth fallback still clears its phase before manual credentials; post-install commit/PR questions and their policy remain intact. +- TUI changes are limited to exit/signal cleanup and tests; its model, inline prompt components, content, transcript masking and selection policy remain in place. diff --git a/scripts/prompt-ownership-pty.py b/scripts/prompt-ownership-pty.py new file mode 100644 index 00000000..a0407857 --- /dev/null +++ b/scripts/prompt-ownership-pty.py @@ -0,0 +1,120 @@ +#!/usr/bin/env python3 +"""Offline real-PTY smoke: python3 scripts/prompt-ownership-pty.py (macOS/Linux). + +Runs only synthetic installer events, never the installer machine or services. +Uses stdlib PTYs, isolated HOME/config/temp paths, bounded waits and group cleanup. +""" +import errno +import fcntl +import json +import os +from pathlib import Path +import pty +import select +import shutil +import signal +import struct +import subprocess +import tempfile +import termios +import time + +ROOT = Path(__file__).resolve().parent.parent +BUN = shutil.which("bun") +assert BUN, "Bun is required" + + +def run(mode, scenario, home): + evidence = home / f"{mode}-{scenario}.jsonl" + master, slave = pty.openpty() + fcntl.ioctl(slave, termios.TIOCSWINSZ, struct.pack("HHHH", 30, 100, 0, 0)) + original_termios = termios.tcgetattr(slave) + env = { + "PATH": os.environ.get("PATH", ""), + "HOME": str(home), + "TMPDIR": str(home), + "XDG_CONFIG_HOME": str(home), + "TERM": "xterm-256color", + "LANG": "en_US.UTF-8", + "DO_NOT_TRACK": "1", + "WORKOS_TELEMETRY_DISABLED": "1", + } + child = subprocess.Popen( + [BUN, str(ROOT / "scripts/prompt-ownership-pty.fixture.ts"), mode, scenario, str(evidence)], + cwd=ROOT, env=env, stdin=slave, stdout=slave, stderr=slave, start_new_session=True, + ) + output = bytearray() + deadline = time.monotonic() + 12 + + def pump(seconds=0.02): + if select.select([master], [], [], seconds)[0]: + try: + output.extend(os.read(master, 65536)) + except OSError as error: + if error.errno != errno.EIO: + raise + + def stages(): + if not evidence.exists(): + return [] + return [json.loads(line)["stage"] for line in evidence.read_text().splitlines()] + + def wait(check): + while not check(): + assert time.monotonic() < deadline, f"timeout: {mode}/{scenario} {output[-3000:]!r}" + assert child.poll() is None, f"early exit: {mode}/{scenario} {output[-3000:]!r}" + pump() + + def hold(seconds): + end = time.monotonic() + seconds + while time.monotonic() < end: + pump() + + try: + question = b"Continue anyway?" if scenario == "cancel" else b"Commit the changes?" + wait(lambda: question in output) + wait(lambda: "replaced" in stages()) + hold(0.1) # Let Ink's batched frame finish, then hold input open for 3 ticks. + if mode == "tui": + assert b"\x1b[?1049h" in output, "missing alternate-screen entry" + assert question in output[-6000:], "question missing from recent Ink frame" + before = len(output) + hold(0.24) + assert len(output) == before, f"output while awaiting input: {output[before:]!r}" + if scenario == "answers": + os.write(master, b"n" if mode == "tui" else b"n\r") + wait(lambda: b"Create a pull request?" in output[before:]) + hold(0.1) + os.write(master, b"y" if mode == "tui" else b"y\r") + elif scenario == "cancel": + os.write(master, b"\x03") + wait(lambda: "stopped" in stages()) + hold(0.1) + after_stop = len(output) + wait(lambda: "clean" in stages()) + hold(0.05) + # restore-cursor's process-exit hook may show the cursor once more; + # there must be no text, erasure, animation or prompt after teardown. + assert not output[after_stop:].replace(b"\x1b[?25h", b""), f"stale output after stop: {output[after_stop:]!r}" + assert child.wait(timeout=2) == 0, output[-3000:] + assert termios.tcgetattr(slave) == original_termios, "terminal attributes not restored" + if mode == "tui": + assert output.count(b"\x1b[?1049l") == 1, "alternate screen not restored exactly once" + assert b"\x1b[?25h" in output, "cursor not restored" + if scenario == "cancel": + assert b"Create a feature branch?" not in output, "moot queued question opened" + if scenario == "stop": + assert b"Create a pull request?" not in output, "queued question escaped teardown" + print(f"PASS {mode}/{scenario}: real PTY 100x30; input, quiet timers, cleanup verified") + finally: + if child.poll() is None: + os.killpg(child.pid, signal.SIGKILL) + child.wait(timeout=2) + os.close(master) + os.close(slave) + + +with tempfile.TemporaryDirectory(prefix=".prompt-pty-", dir=ROOT) as directory: + for adapter in ("cli", "tui"): + for scenario in ("answers", "cancel", "stop"): + run(adapter, scenario, Path(directory)) diff --git a/src/lib/adapters/cli-adapter.coordination.spec.ts b/src/lib/adapters/cli-adapter.coordination.spec.ts new file mode 100644 index 00000000..36ef1140 --- /dev/null +++ b/src/lib/adapters/cli-adapter.coordination.spec.ts @@ -0,0 +1,185 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { createInstallerEventEmitter } from '../events.js'; +import { CLIAdapter } from './cli-adapter.js'; +import ui from '../../utils/ui.js'; + +// Only the input transport is fake: adapter, facade, spinner timers, queuing, +// cancellation and installer event delivery are real. +vi.mock('@inquirer/prompts', () => ({ confirm: vi.fn(), select: vi.fn(), input: vi.fn(), password: vi.fn() })); +vi.mock('../settings.js', () => ({ getConfig: () => ({ branding: { showAsciiArt: false } }) })); +const inquirer = await import('@inquirer/prompts'); + +let emitter: ReturnType; +let adapter: CLIAdapter; +let sendEvent: ReturnType; +let write: ReturnType; +let log: ReturnType; +let stdinTty: PropertyDescriptor | undefined; +let stdoutTty: PropertyDescriptor | undefined; +let questions: Array<{ message: string; answer: (value: never) => void }>; +const drain = () => vi.advanceTimersByTimeAsync(1); +const output = () => log.mock.calls.map(([chunk]) => String(chunk)).join('\n'); +const frames = () => write.mock.calls.map(([chunk]) => String(chunk)).join(''); + +beforeEach(async () => { + vi.useFakeTimers(); + questions = []; + stdinTty = Object.getOwnPropertyDescriptor(process.stdin, 'isTTY'); + stdoutTty = Object.getOwnPropertyDescriptor(process.stdout, 'isTTY'); + Object.defineProperty(process.stdin, 'isTTY', { configurable: true, value: true }); + Object.defineProperty(process.stdout, 'isTTY', { configurable: true, value: true }); + write = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + log = vi.spyOn(console, 'log').mockImplementation(() => {}); + for (const prompt of [inquirer.confirm, inquirer.select, inquirer.input, inquirer.password]) { + vi.mocked(prompt).mockImplementation( + (options, context) => + new Promise((resolve, reject) => { + const signal = context?.signal; + const abort = () => reject(Object.assign(new Error('aborted'), { name: 'AbortPromptError' })); + signal?.addEventListener('abort', abort, { once: true }); + questions.push({ + message: options.message, + answer: (value) => { + signal?.removeEventListener('abort', abort); + resolve(value); + }, + }); + }), + ); + } + emitter = createInstallerEventEmitter(); + sendEvent = vi.fn(); + adapter = new CLIAdapter({ emitter, sendEvent }); + await adapter.start(); + log.mockClear(); +}); + +afterEach(async () => { + const stopped = adapter.stop(); + await drain(); + await stopped; + expect(vi.getTimerCount()).toBe(0); + vi.useRealTimers(); + vi.restoreAllMocks(); + vi.clearAllMocks(); + if (stdinTty) Object.defineProperty(process.stdin, 'isTTY', stdinTty); + else delete (process.stdin as { isTTY?: boolean }).isTTY; + if (stdoutTty) Object.defineProperty(process.stdout, 'isTTY', stdoutTty); + else delete (process.stdout as { isTTY?: boolean }).isTTY; +}); + +describe('CLI adapter with real UI coordination', () => { + it.each(['rails', 'no-validate', 'validation'])( + 'ends the agent spinner without waiting for post-install (%s)', + async (path) => { + emitter.emit('agent:start', {}); + if (path === 'validation') emitter.emit('validation:start', { framework: 'nextjs' }); + emitter.emit('agent:success', { summary: 'Synthetic success' }); + expect(output().match(/Agent completed/g)).toHaveLength(1); + write.mockClear(); + await vi.advanceTimersByTimeAsync(800); + expect(write).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(0); + }, + ); + + it('keeps two questions isolated through spinner replacement, logs and phase completion', async () => { + emitter.emit('agent:start', {}); + emitter.emit('postinstall:commit:prompt', {}); + emitter.emit('postinstall:pr:prompt', {}); + await drain(); + expect(questions.map((q) => q.message)).toEqual(['Commit the changes?']); + write.mockClear(); + log.mockClear(); + emitter.emit('agent:tool', { kind: 'command', detail: 'synthetic tool' }); + emitter.emit('file:write', { path: '/fixture/callback.rb' }); + emitter.emit('agent:success', {}); + emitter.emit('postinstall:commit:generating', {}); + emitter.emit('postinstall:commit:success', { message: 'fixture only' }); + emitter.emit('postinstall:pr:generating', {}); + emitter.emit('postinstall:pr:pushing', {}); + await vi.advanceTimersByTimeAsync(800); + expect(write).not.toHaveBeenCalled(); + expect(log).not.toHaveBeenCalled(); + + questions[0].answer(false as never); + await drain(); + expect(sendEvent).toHaveBeenCalledWith({ type: 'COMMIT_DECLINED' }); + expect(questions.map((q) => q.message)).toEqual(['Commit the changes?', 'Create a pull request?']); + emitter.emit('agent:tool', { kind: 'command', detail: 'second synthetic tool' }); + await vi.advanceTimersByTimeAsync(800); + expect(write).not.toHaveBeenCalled(); + expect(log).not.toHaveBeenCalled(); + + questions[1].answer(true as never); + await drain(); + expect(sendEvent).toHaveBeenCalledWith({ type: 'PR_APPROVED' }); + expect(output()).toContain('Agent completed'); + expect(output()).toContain('fixture only'); + expect(output()).toContain('callback.rb'); + expect(output()).toContain('second synthetic tool'); + write.mockClear(); + await vi.advanceTimersByTimeAsync(800); + expect(frames()).toContain('Pushing to remote...'); + expect(frames()).not.toMatch(/Running AI|Generating commit/); + expect(vi.getTimerCount()).toBe(1); + }); + + it('logging preserves a newer facade owner instead of restarting a stale adapter phase', async () => { + emitter.emit('agent:start', {}); + const newer = ui.spinner(); + try { + newer.start('newer owner'); + emitter.emit('agent:tool', { kind: 'command', detail: 'synthetic log' }); + write.mockClear(); + await vi.advanceTimersByTimeAsync(240); + expect(frames()).toContain('newer owner'); + expect(frames()).not.toContain('Running AI agent'); + expect(output()).toContain('synthetic log'); + expect(output()).not.toContain('✓'); + expect(vi.getTimerCount()).toBe(1); + } finally { + newer.clear(); + } + }); + + it('cancelling the first question never opens the now-moot queued sibling', async () => { + sendEvent.mockImplementation((event) => { + if (event.type === 'GIT_CANCELLED') emitter.emit('complete', { success: false, summary: 'Cancelled' }); + }); + emitter.emit('agent:start', {}); + emitter.emit('git:dirty', { files: ['fixture.rb'] }); + emitter.emit('branch:prompt', { branch: 'main' }); + await drain(); + questions[0].answer(false as never); + await drain(); + expect(questions.map((q) => q.message)).toEqual(['Continue anyway?']); + expect(output()).toContain('Cancelled'); + expect(vi.getTimerCount()).toBe(0); + }); + + it.each(['failure', 'error', 'complete', 'stop', 'sigint'] as const)( + '%s aborts open/queued prompts and retires all animation', + async (end) => { + emitter.emit('agent:start', {}); + emitter.emit('postinstall:commit:prompt', {}); + emitter.emit('postinstall:pr:prompt', {}); + await drain(); + write.mockClear(); + let stopped: Promise | undefined; + if (end === 'failure') emitter.emit('agent:failure', { message: 'Synthetic failure' }); + if (end === 'error') emitter.emit('error', { message: 'Synthetic failure' }); + if (end === 'complete') emitter.emit('complete', { success: true, summary: 'Synthetic completion' }); + if (end === 'stop') stopped = adapter.stop(); + if (end === 'sigint') process.emit('SIGINT'); + await vi.advanceTimersByTimeAsync(800); + await stopped; + expect(questions).toHaveLength(1); + expect(write).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(0); + if (end === 'failure') expect(output()).toContain('Agent failed'); + if (end === 'error') expect(output()).toContain('Synthetic failure'); + if (end === 'complete') expect(output()).toContain('Synthetic completion'); + }, + ); +}); diff --git a/src/lib/adapters/cli-adapter.spec.ts b/src/lib/adapters/cli-adapter.spec.ts index 82b26eeb..f1780aca 100644 --- a/src/lib/adapters/cli-adapter.spec.ts +++ b/src/lib/adapters/cli-adapter.spec.ts @@ -7,6 +7,7 @@ const mockConsoleLog = vi.spyOn(console, 'log').mockImplementation(() => {}); // Mock the UI facade vi.mock('../../utils/ui.js', () => ({ + getUiHost: vi.fn(() => null), default: { intro: vi.fn(), log: { @@ -317,7 +318,7 @@ describe('CLIAdapter', () => { } }); - it('restarts the spinner on the last phase message after logging a file op', async () => { + it('leaves spinner ownership to the facade when logging a file op', async () => { await adapter.start(); const ui = await import('../../utils/ui.js'); const spinnerMock = { start: vi.fn(), stop: vi.fn(), message: vi.fn(), clear: vi.fn() }; @@ -327,8 +328,10 @@ describe('CLIAdapter', () => { emitter.emit('agent:progress', { step: 'Configuring middleware' }); emitter.emit('file:write', { path: '/proj/src/auth.ts', content: 'x' }); - expect(spinnerMock.stop).toHaveBeenCalled(); - expect(spinnerMock.start).toHaveBeenCalledWith('Configuring middleware'); + expect(spinnerMock.stop).not.toHaveBeenCalled(); + expect(spinnerMock.start).toHaveBeenCalledTimes(1); + expect(spinnerMock.message).toHaveBeenCalledWith('Configuring middleware'); + expect(ui.default.log.step).toHaveBeenCalledWith(expect.stringContaining('src/auth.ts')); }); it('renders Bash tool calls as step lines (agent:tool)', async () => { diff --git a/src/lib/adapters/cli-adapter.ts b/src/lib/adapters/cli-adapter.ts index 2c10c615..23461d68 100644 --- a/src/lib/adapters/cli-adapter.ts +++ b/src/lib/adapters/cli-adapter.ts @@ -1,7 +1,7 @@ import type { InstallerAdapter, AdapterConfig } from './types.js'; import type { InstallerEventEmitter, InstallerEvents } from '../events.js'; import { relative } from 'node:path'; -import ui, { PromptUnavailableError } from '../../utils/ui.js'; +import ui, { getUiHost, PromptUnavailableError } from '../../utils/ui.js'; import chalk from 'chalk'; import { getConfig } from '../settings.js'; import { getActiveEnvironment, isUnclaimedEnvironment, profileEnvironmentLabel } from '../config-store.js'; @@ -31,7 +31,8 @@ export class CLIAdapter implements InstallerAdapter { private handlers = new Map void>(); // Queue for logs while prompt is active (parallel state issue) - private isPromptActive = false; + private pendingPrompts = 0; + private pendingHandlers = new Set>(); private pendingLogs: Array<() => void> = []; // SIGINT handler for cleanup @@ -43,7 +44,7 @@ export class CLIAdapter implements InstallerAdapter { // stops the queued sibling from opening a now-moot question. private promptAbort: AbortController | null = null; - // Last phase message shown on the agent spinner, restored after logging above it. + // Last phase message shown on the agent spinner. private lastAgentMessage = 'Running AI agent...'; // Last file path rendered as a step line, to dedupe consecutive same-path ops. private lastFileOp: string | null = null; @@ -58,8 +59,15 @@ export class CLIAdapter implements InstallerAdapter { * Queue a log call if a prompt is active, otherwise execute immediately. */ private queueableLog(logFn: () => void): void { - if (this.isPromptActive) { - this.pendingLogs.push(logFn); + // Hosted lines go to the transcript, not through input. Deliver them now + // so the TUI can still classify agent play-by-play in its original phase. + if (this.pendingPrompts > 0 && !getUiHost()) { + const host = getUiHost(); + this.pendingLogs.push(() => { + // Normal stop drains before host teardown. An exit/signal hook cannot + // await it: never leak its late logs onto the restored terminal. + if (getUiHost() === host) logFn(); + }); } else { logFn(); } @@ -80,12 +88,12 @@ export class CLIAdapter implements InstallerAdapter { * prompt handler so the active/flush lifecycle lives in one place. */ private async withPromptActive(run: () => Promise): Promise { - this.isPromptActive = true; + this.pendingPrompts++; try { return await run(); } finally { - this.isPromptActive = false; - this.flushPendingLogs(); + this.pendingPrompts--; + if (this.pendingPrompts === 0) this.flushPendingLogs(); } } @@ -142,6 +150,8 @@ export class CLIAdapter implements InstallerAdapter { this.subscribe('config:complete', this.handleConfigComplete); this.subscribe('agent:start', this.handleAgentStart); this.subscribe('agent:progress', this.handleAgentProgress); + this.subscribe('agent:success', this.handleAgentSuccess); + this.subscribe('agent:failure', this.handleAgentFailure); // Persistent, append-only log of file operations + tool calls above the spinner. this.subscribe('file:write', this.handleFileWrite); this.subscribe('file:edit', this.handleFileEdit); @@ -183,7 +193,6 @@ export class CLIAdapter implements InstallerAdapter { // Abort any in-flight/queued prompt so a cancelled run can't leave a // now-moot sibling question open (e.g. the branch prompt after git-cancel). this.promptAbort?.abort(); - this.promptAbort = null; // Remove SIGINT handler if (this.sigIntHandler) { @@ -198,10 +207,13 @@ export class CLIAdapter implements InstallerAdapter { this.handlers.clear(); // Stop any active spinner - this.spinner?.stop(); + this.spinner?.clear(); this.spinner = null; this.isStarted = false; + // Let aborted handlers flush their buffered output while the TUI host and + // console capture still exist. No question may escape into plain stdin. + await Promise.all(this.pendingHandlers); } private stopSpinner(message: string, code = 0): void { @@ -211,10 +223,16 @@ export class CLIAdapter implements InstallerAdapter { } } + /** ui owns replacement/retirement, including while a question is open. */ + private startSpinner(message: string): void { + this.spinner = ui.spinner(); + this.spinner.start(message); + } + /** Debug logging - only outputs when debug mode is enabled */ private debugLog = (message: string): void => { if (this.debug) { - console.log(chalk.dim(`[debug] ${message}`)); + this.queueableLog(() => console.log(chalk.dim(`[debug] ${message}`))); } }; @@ -233,7 +251,10 @@ export class CLIAdapter implements InstallerAdapter { const safeHandler = (payload: InstallerEvents[K]): void => { try { const result = boundHandler(payload); - if (result instanceof Promise) result.catch((err) => this.onHandlerError(err)); + if (result instanceof Promise) { + this.pendingHandlers.add(result); + void result.catch((err) => this.onHandlerError(err)).finally(() => this.pendingHandlers.delete(result)); + } } catch (err) { this.onHandlerError(err); } @@ -332,6 +353,7 @@ export class CLIAdapter implements InstallerAdapter { ui.confirm({ message: `Found ${fileList}. Check for existing WorkOS credentials?`, initialValue: true, + signal: this.promptAbort?.signal, }), ); @@ -342,11 +364,12 @@ export class CLIAdapter implements InstallerAdapter { private handleDeviceStarted = ({ verificationUri, userCode }: InstallerEvents['device:started']): void => { ui.log.info(`\nOpen this URL in your browser:\n`); - console.log(` ${chalk.cyan(verificationUri)}`); - console.log(`\nEnter code: ${chalk.bold(userCode)}\n`); + this.queueableLog(() => { + console.log(` ${chalk.cyan(verificationUri)}`); + console.log(`\nEnter code: ${chalk.bold(userCode)}\n`); + }); - this.spinner = ui.spinner(); - this.spinner.start('Waiting for authentication...'); + this.startSpinner('Waiting for authentication...'); }; private handleDeviceSuccess = (): void => { @@ -354,11 +377,8 @@ export class CLIAdapter implements InstallerAdapter { }; private handleStagingFetching = (): void => { - if (this.spinner) { - this.spinner.stop('Authenticated'); - } - this.spinner = ui.spinner(); - this.spinner.start('Fetching your WorkOS credentials...'); + this.stopSpinner('Authenticated'); + this.startSpinner('Fetching your WorkOS credentials...'); }; private handleStagingSuccess = ({ source, credentials }: InstallerEvents['staging:success']): void => { @@ -457,19 +477,22 @@ export class CLIAdapter implements InstallerAdapter { ui.log.step(`Get your credentials from ${chalk.cyan('https://dashboard.workos.com')}`); - const clientId = await ui.text({ - message: 'Enter your WorkOS Client ID:', - placeholder: 'client_...', - validate: (value) => { - if (!value || value.trim().length === 0) { - return 'Client ID is required'; - } - if (!value.startsWith('client_')) { - return 'Client ID should start with "client_"'; - } - return undefined; - }, - }); + const clientId = await this.withPromptActive(() => + ui.text({ + signal: this.promptAbort?.signal, + message: 'Enter your WorkOS Client ID:', + placeholder: 'client_...', + validate: (value) => { + if (!value || value.trim().length === 0) { + return 'Client ID is required'; + } + if (!value.startsWith('client_')) { + return 'Client ID should start with "client_"'; + } + return undefined; + }, + }), + ); if (ui.isCancel(clientId)) { this.sendEvent({ type: 'CANCEL' }); @@ -479,18 +502,21 @@ export class CLIAdapter implements InstallerAdapter { let apiKey = ''; if (requiresApiKey) { ui.log.info(chalk.dim('ℹ️ Your API key will be hidden for security and saved to .env.local')); - const apiKeyResult = await ui.password({ - message: 'Enter your WorkOS API Key:', - validate: (value) => { - if (!value || value.trim().length === 0) { - return 'API Key is required'; - } - if (!value.startsWith('sk_')) { - return 'API Key should start with "sk_"'; - } - return undefined; - }, - }); + const apiKeyResult = await this.withPromptActive(() => + ui.password({ + signal: this.promptAbort?.signal, + message: 'Enter your WorkOS API Key:', + validate: (value) => { + if (!value || value.trim().length === 0) { + return 'API Key is required'; + } + if (!value.startsWith('sk_')) { + return 'API Key should start with "sk_"'; + } + return undefined; + }, + }), + ); if (ui.isCancel(apiKeyResult)) { this.sendEvent({ type: 'CANCEL' }); @@ -513,39 +539,32 @@ export class CLIAdapter implements InstallerAdapter { }; private handleAgentStart = (): void => { - this.spinner = ui.spinner(); - this.spinner.start(this.lastAgentMessage); + this.startSpinner(this.lastAgentMessage); // No setInterval: ui animates its own frames, and the old 2s reset // clobbered the current phase text set by handleAgentProgress. }; + // Rails and --no-validate never emit validation:start. + private handleAgentSuccess = (): void => { + this.stopSpinner('Agent completed'); + }; + + private handleAgentFailure = (): void => { + this.promptAbort?.abort(); + this.stopSpinner('Agent failed', 1); + }; + private handleAgentProgress = ({ step, detail }: InstallerEvents['agent:progress']): void => { const message = detail ? `${step}: ${detail}` : step; this.lastAgentMessage = message; this.spinner?.message(message); }; - /** - * Render a persistent line above the running spinner: stop the spinner to - * finalize its line, emit the log, then restart it on the last phase message. - * Mirrors the existing stop→log and stop→start-new-spinner precedents. - */ - private logAboveSpinner(render: () => void): void { - const wasRunning = this.spinner !== null; - this.spinner?.stop(); - this.spinner = null; - render(); - if (wasRunning) { - this.spinner = ui.spinner(); - this.spinner.start(this.lastAgentMessage); - } - } - private logFileOp(verb: 'Creating' | 'Editing', path: string): void { if (path === this.lastFileOp) return; // dedupe consecutive same-path ops this.lastFileOp = path; const rel = relative(process.cwd(), path); - this.logAboveSpinner(() => ui.log.step(`${verb} ${chalk.dim(rel)}`)); + this.queueableLog(() => ui.log.step(`${verb} ${chalk.dim(rel)}`)); } private handleFileWrite = ({ path }: InstallerEvents['file:write']): void => { @@ -558,7 +577,7 @@ export class CLIAdapter implements InstallerAdapter { private handleAgentTool = ({ detail }: InstallerEvents['agent:tool']): void => { const cmd = detail.length > 80 ? `${detail.slice(0, 77)}…` : detail; - this.logAboveSpinner(() => ui.log.step(`Running ${chalk.dim(cmd)}`)); + this.queueableLog(() => ui.log.step(`Running ${chalk.dim(cmd)}`)); }; private handleValidationStart = (): void => { @@ -596,9 +615,11 @@ export class CLIAdapter implements InstallerAdapter { this.stopSpinner(success ? 'Done' : 'Failed'); - console.log(''); - console.log(renderCompletionSummary(success, summary, completion)); - console.log(''); + this.queueableLog(() => { + console.log(''); + console.log(renderCompletionSummary(success, summary, completion)); + console.log(''); + }); // When we scaffolded a fresh app, the install ran in the current dir, so // point the user straight at the dev server. @@ -608,6 +629,7 @@ export class CLIAdapter implements InstallerAdapter { }; private handleError = ({ message, stack, code }: InstallerEvents['error']): void => { + this.promptAbort?.abort(); // A structured decline (e.g. unsupported framework version) already // printed its guidance via the integration — don't restyle it as a // generic failure. @@ -642,6 +664,7 @@ export class CLIAdapter implements InstallerAdapter { ui.confirm({ message: 'This directory is empty. Scaffold a new Next.js app with AuthKit here?', initialValue: true, + signal: this.promptAbort?.signal, }), ); @@ -652,8 +675,7 @@ export class CLIAdapter implements InstallerAdapter { private handleScaffoldStart = ({ packageManager }: InstallerEvents['scaffold:start']): void => { this.scaffoldPackageManager = packageManager; - this.spinner = ui.spinner(); - this.spinner.start(`Scaffolding a new Next.js app with ${packageManager} (this can take a minute)...`); + this.startSpinner(`Scaffolding a new Next.js app with ${packageManager} (this can take a minute)...`); }; // create-next-app output is verbose; surface it only under --debug and keep @@ -712,6 +734,7 @@ export class CLIAdapter implements InstallerAdapter { ui.confirm({ message: 'Commit the changes?', initialValue: true, + signal: this.promptAbort?.signal, }), ); @@ -721,8 +744,7 @@ export class CLIAdapter implements InstallerAdapter { }; private handleCommitGenerating = (): void => { - this.spinner = ui.spinner(); - this.spinner.start('Generating commit message...'); + this.startSpinner('Generating commit message...'); }; private handleCommitSuccess = ({ message }: InstallerEvents['postinstall:commit:success']): void => { @@ -740,6 +762,7 @@ export class CLIAdapter implements InstallerAdapter { ui.confirm({ message: 'Create a pull request?', initialValue: true, + signal: this.promptAbort?.signal, }), ); @@ -749,16 +772,14 @@ export class CLIAdapter implements InstallerAdapter { }; private handlePrGenerating = (): void => { - this.spinner = ui.spinner(); - this.spinner.start('Generating PR description...'); + this.startSpinner('Generating PR description...'); }; private handlePrPushing = (): void => { if (this.spinner) { this.spinner.message('Pushing to remote...'); } else { - this.spinner = ui.spinner(); - this.spinner.start('Pushing to remote...'); + this.startSpinner('Pushing to remote...'); } }; @@ -779,6 +800,6 @@ export class CLIAdapter implements InstallerAdapter { private handleManualInstructions = ({ instructions }: InstallerEvents['postinstall:manual']): void => { ui.log.info('GitHub CLI not found. Manual steps:'); - console.log(chalk.dim(instructions)); + this.queueableLog(() => console.log(chalk.dim(instructions))); }; } diff --git a/src/lib/adapters/tui-adapter.spec.ts b/src/lib/adapters/tui-adapter.spec.ts index f1a57341..eeb2422c 100644 --- a/src/lib/adapters/tui-adapter.spec.ts +++ b/src/lib/adapters/tui-adapter.spec.ts @@ -307,6 +307,108 @@ describe('TuiAdapter', () => { expect(afterExit()).toContain('✗ Still there? cancelled'); }); + it('keeps queued questions visible and answerable through phase/status replacement', async () => { + await adapter.start(); + emitter.emit('agent:start', {}); + emitter.emit('postinstall:commit:prompt', {}); + emitter.emit('postinstall:pr:prompt', {}); + await waitFor(() => expect(frame()).toContain('? Commit the changes?')); + emitter.emit('agent:tool', { kind: 'command', detail: 'hidden agent play-by-play' }); + emitter.emit('agent:success', { summary: 'Rails fixture: no validation' }); + emitter.emit('postinstall:commit:generating', {}); + emitter.emit('postinstall:commit:success', { message: 'fixture commit' }); + emitter.emit('postinstall:pr:generating', {}); + emitter.emit('postinstall:pr:pushing', {}); + emitter.emit('agent:tool', { kind: 'command', detail: 'synthetic tool log' }); + // The status bar deliberately yields to prompt key hints while input is open. + await new Promise((resolve) => setTimeout(resolve, 240)); + expect(frame()).toContain('? Commit the changes?'); + stdin.press('n'); + await waitFor(() => expect(sendEvent).toHaveBeenCalledWith({ type: 'COMMIT_DECLINED' })); + await waitFor(() => expect(frame()).toContain('? Create a pull request?')); + emitter.emit('postinstall:pr:failed', { error: 'synthetic warning' }); + await waitFor(() => { + expect(frame()).toContain('? Create a pull request?'); + expect(frame()).toContain('synthetic warning'); + }); + stdin.press('y'); + await waitFor(() => expect(sendEvent).toHaveBeenCalledWith({ type: 'PR_APPROVED' })); + await adapter.stop(); + expect(afterExit()).toContain('Agent completed'); + expect(afterExit()).not.toContain('hidden agent play-by-play'); + expect(afterExit()).toContain("The agent's step-by-step log is in the installer log"); + expect(afterExit()).toContain('✔ Commit the changes? No'); + expect(afterExit()).toContain('✔ Create a pull request? Yes'); + expect(afterExit()).toContain('synthetic warning'); + }); + + it('tears down open and queued questions without late terminal output or input listeners', async () => { + const write = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + const listeners = stdin.listenerCount('readable'); + await adapter.start(); + emitter.emit('agent:start', {}); + emitter.emit('postinstall:commit:prompt', {}); + emitter.emit('postinstall:pr:prompt', {}); + await waitFor(() => expect(frame()).toContain('? Commit the changes?')); + emitter.emit('agent:tool', { kind: 'command', detail: 'buffered before stop' }); + emitter.emit('postinstall:commit:generating', {}); + await adapter.stop(); + const stoppedOutput = stdout.output(); + await new Promise((resolve) => setTimeout(resolve, 240)); + expect(stdout.output()).toBe(stoppedOutput); + expect(write).not.toHaveBeenCalled(); + expect(stdout.output()).not.toContain('? Create a pull request?'); + expect(afterExit()).toContain('✗ Commit the changes? cancelled'); + expect(afterExit()).not.toContain('buffered before stop'); + expect(afterExit()).toContain("The agent's step-by-step log is in the installer log"); + expect(stdin.listenerCount('readable')).toBe(listeners); + expect(stdin.rawMode).toBe(false); + expect(getUiHost()).toBeNull(); + await adapter.stop(); + expect(stdout.output()).toBe(stoppedOutput); + }); + + it('the synchronous exit hook also detaches CLI handlers and cancels queued input', async () => { + const sigintListeners = process.listenerCount('SIGINT'); + const write = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + await adapter.start(); + emitter.emit('agent:start', {}); + emitter.emit('postinstall:commit:prompt', {}); + emitter.emit('postinstall:pr:prompt', {}); + await waitFor(() => expect(frame()).toContain('? Commit the changes?')); + emitter.emit('agent:tool', { kind: 'command', detail: 'late tool log' }); + const hook = process.listeners('exit').at(-1) as () => void; + hook(); + await new Promise((resolve) => setTimeout(resolve, 240)); + expect(write).not.toHaveBeenCalled(); + expect(stdout.output()).not.toContain('? Create a pull request?'); + expect(process.listenerCount('SIGINT')).toBe(sigintListeners); + expect(emitter.listenerCount('agent:start')).toBe(0); + expect(stdin.rawMode).toBe(false); + expect(getUiHost()).toBeNull(); + }); + + it('keeps password answers masked across status replacement and teardown', async () => { + await adapter.start(); + const old = ui.spinner(); + old.start('old phase'); + const answer = ui.password({ message: 'Synthetic password?' }); + await waitFor(() => expect(frame()).toContain('Synthetic password?')); + const current = ui.spinner(); + current.start('new phase'); + old.message('stale'); + old.clear(); + old.stop('stale'); + await new Promise((resolve) => setTimeout(resolve, 240)); + expect(frame()).toContain('Synthetic password?'); + stdin.press('fake-secret\r'); + expect(await answer).toBe('fake-secret'); + await waitFor(() => expect(frame()).toContain('new phase')); + await adapter.stop(); + expect(stdout.output()).not.toContain('fake-secret'); + expect(afterExit()).toContain('********'); + }); + it('answers CANCEL when the question is aborted by its signal', async () => { await adapter.start(); const controller = new AbortController(); diff --git a/src/lib/adapters/tui-adapter.ts b/src/lib/adapters/tui-adapter.ts index 9de4c6c0..0d928d41 100644 --- a/src/lib/adapters/tui-adapter.ts +++ b/src/lib/adapters/tui-adapter.ts @@ -171,6 +171,9 @@ export class TuiAdapter implements InstallerAdapter { process.off('exit', this.teardown); for (const signal of TERMINATING_SIGNALS) process.off(signal, this.terminated); + // stop() normally already awaited this. Exit/signal hooks must also abort + // queued questions and detach the CLI subscriptions, synchronously. + void this.cli.stop(); // A prompt nobody will answer now must not leave its caller hanging. this.settle(CANCEL); try { diff --git a/src/utils/ui.spec.ts b/src/utils/ui.spec.ts index cbd2bbe1..d9e37db7 100644 --- a/src/utils/ui.spec.ts +++ b/src/utils/ui.spec.ts @@ -199,6 +199,198 @@ describe('prompt coordination (withPrompt)', () => { }); }); +describe('spinner ownership (AUTH-6732)', () => { + let write: ReturnType; + let log: ReturnType; + let stdoutTty: PropertyDescriptor | undefined; + const handles: ReturnType[] = []; + const start = (message: string) => { + const handle = ui.spinner(); + handles.push(handle); + handle.start(message); + return handle; + }; + const output = () => write.mock.calls.map(([chunk]) => String(chunk)).join(''); + const drain = async () => { + await vi.advanceTimersByTimeAsync(0); + }; + + beforeEach(() => { + vi.useFakeTimers(); + stdoutTty = Object.getOwnPropertyDescriptor(process.stdout, 'isTTY'); + Object.defineProperty(process.stdout, 'isTTY', { value: true, configurable: true }); + write = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + log = vi.spyOn(console, 'log').mockImplementation(() => {}); + }); + afterEach(() => { + handles.splice(0).forEach((handle) => handle.clear()); + setUiHost(null); + vi.useRealTimers(); + vi.restoreAllMocks(); + if (stdoutTty) Object.defineProperty(process.stdout, 'isTTY', stdoutTty); + else delete (process.stdout as { isTTY?: boolean }).isTTY; + }); + + it('retires replaced timers and makes all stale operations inert', () => { + const old = start('old'); + const current = start('current'); + write.mockClear(); + old.message('stale'); + old.stop('stale'); + old.clear(); + old.start('stale'); + expect(write).not.toHaveBeenCalled(); + expect(log).not.toHaveBeenCalled(); + vi.advanceTimersByTime(400); + expect(output()).toContain('current'); + expect(output()).not.toMatch(/old|stale/); + expect(vi.getTimerCount()).toBe(1); + current.clear(); + expect(vi.getTimerCount()).toBe(0); + }); + + it.each(['resume', 'stop', 'clear'] as const)( + 'suspends new and replaced spinners through queued prompts, then %s', + async (ending) => { + let answer!: (value: boolean) => void; + vi.mocked(inquirer.confirm).mockImplementation(() => new Promise((resolve) => (answer = resolve))); + const old = start('old'); + const first = ui.confirm({ message: 'first' }); + const second = ui.confirm({ message: 'second' }); + await drain(); + write.mockClear(); + vi.advanceTimersByTime(400); + const current = start('next'); + old.clear(); + current.message('updated'); + if (ending === 'stop') current.stop('finished'); + if (ending === 'clear') current.clear(); + vi.advanceTimersByTime(400); + expect(write).not.toHaveBeenCalled(); + expect(log).not.toHaveBeenCalled(); + answer(true); + await first; + await drain(); + expect(inquirer.confirm).toHaveBeenCalledTimes(2); + vi.advanceTimersByTime(400); + expect(write).not.toHaveBeenCalled(); + expect(log).not.toHaveBeenCalled(); + answer(false); + expect(await second).toBe(false); + write.mockClear(); + vi.advanceTimersByTime(400); + expect(output()).not.toContain('old'); + if (ending === 'resume') expect(output()).toContain('updated'); + else expect(write).not.toHaveBeenCalled(); + if (ending === 'stop') expect(log).toHaveBeenCalledWith(expect.stringContaining('finished')); + }, + ); + + it('pauses an existing owner for all ticks and resumes it after the answer', async () => { + let answer!: (value: boolean) => void; + vi.mocked(inquirer.confirm).mockImplementationOnce(() => new Promise((resolve) => (answer = resolve))); + start('working'); + const prompt = ui.confirm({ message: 'question' }); + await drain(); + write.mockClear(); + vi.advanceTimersByTime(800); + expect(write).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(0); + answer(true); + await prompt; + vi.advanceTimersByTime(240); + expect(output()).toContain('working'); + expect(vi.getTimerCount()).toBe(1); + }); + + it('host activation retires terminal animation and host replacement retires status', () => { + const terminal = start('terminal'); + const oldHost = { line: vi.fn(), status: vi.fn(), prompt: vi.fn() }; + const newHost = { line: vi.fn(), status: vi.fn(), prompt: vi.fn() }; + setUiHost(oldHost); + expect(vi.getTimerCount()).toBe(0); + const hosted = start('hosted'); + setUiHost(newHost); + expect(oldHost.status).toHaveBeenLastCalledWith(null); + write.mockClear(); + terminal.start('stale terminal'); + hosted.stop('stale host'); + hosted.message('stale host'); + vi.advanceTimersByTime(800); + expect(write).not.toHaveBeenCalled(); + expect(newHost.status).not.toHaveBeenCalled(); + expect(newHost.line).not.toHaveBeenCalled(); + }); + + it('does not animate on non-TTY output or emit any spinner output in JSON mode', async () => { + Object.defineProperty(process.stdout, 'isTTY', { value: false, configurable: true }); + const nonTty = start('non-TTY'); + nonTty.stop('done'); + expect(write).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(0); + const { setOutputMode } = await import('./output.js'); + log.mockClear(); + setOutputMode('json'); + try { + const json = start('json'); + json.message('json update'); + json.stop('json done'); + json.clear(); + expect(log).not.toHaveBeenCalled(); + expect(write).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(0); + } finally { + setOutputMode('human'); + } + }); + + it('hands back to the current spinner after a rejected prompt', async () => { + start('working'); + vi.mocked(inquirer.confirm).mockRejectedValueOnce(new Error('broken')); + await expect(ui.confirm({ message: 'question' })).rejects.toThrow('broken'); + write.mockClear(); + vi.advanceTimersByTime(400); + expect(output()).toContain('working'); + }); + + it('does not open pre-aborted or cancelled queued prompts', async () => { + const controller = new AbortController(); + controller.abort(); + expect(await ui.confirm({ message: 'moot', signal: controller.signal })).toBe(CANCEL); + expect(inquirer.confirm).not.toHaveBeenCalled(); + }); + + it('protects hosted status from stale handles and retires it at teardown', async () => { + const status = vi.fn(); + let answer!: (value: unknown) => void; + setUiHost({ line: vi.fn(), status, prompt: () => new Promise((resolve) => (answer = resolve)) }); + const old = start('old'); + const first = ui.confirm({ message: 'first' }); + const queued = ui.confirm({ message: 'queued' }); + await drain(); + const current = start('current'); + status.mockClear(); + old.stop('stale'); + old.clear(); + old.message('stale'); + old.start('stale'); + expect(status).not.toHaveBeenCalled(); + setUiHost(null); + answer(CANCEL); + expect(await first).toBe(CANCEL); + await drain(); + expect(await queued).toBe(CANCEL); + current.message('late'); + current.stop('late'); + current.start('late'); + vi.advanceTimersByTime(400); + expect(write).not.toHaveBeenCalled(); + expect(log).not.toHaveBeenCalled(); + expect(inquirer.confirm).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(0); + }); +}); + describe('UI host', () => { let logSpy: ReturnType; let errorSpy: ReturnType; @@ -317,7 +509,8 @@ describe('UI host', () => { await expect(ui.text({ message: 'Name', placeholder: 'client_...', validate })).resolves.toBe('typed'); await expect(ui.password({ message: 'Key', validate })).resolves.toBe('typed'); - expect(requests).toEqual([ + expect(requests.every((request) => request.signal instanceof AbortSignal)).toBe(true); + expect(requests.map(({ signal: _signal, ...request }) => request)).toEqual([ { kind: 'confirm', message: 'Continue?', initialValue: true }, { kind: 'select', diff --git a/src/utils/ui.ts b/src/utils/ui.ts index 0b520c55..512b5bae 100644 --- a/src/utils/ui.ts +++ b/src/utils/ui.ts @@ -69,9 +69,15 @@ export interface UiHost { } let uiHost: UiHost | null = null; +let hostLifetime = new AbortController(); /** Route all ui output and prompts to `host`, or back to the terminal with null. */ export function setUiHost(host: UiHost | null): void { + if (host === uiHost) return; + activeSpinner?.retire(); + hostLifetime.abort(); + hostLifetime = new AbortController(); + recentLines = []; uiHost = host; } @@ -105,7 +111,15 @@ function emit(kind: UiLineKind, message: string, rendered: string, terminal = IN uiHost.line(line); return; } + if (pendingPrompts > 0) { + pendingOutput.push(() => emit(kind, message, rendered, terminal)); + return; + } + // Logs borrow the spinner line without ending/restarting its phase. Keep + // ownership here: adapters may still hold handles retired by another caller. + activeSpinner?.pause(); console.log(terminal); + activeSpinner?.resume(); } // Callers print what a question is about, then ask, in one synchronous run @@ -231,11 +245,12 @@ export interface Spinner { /** * The currently-running spinner, if any. A prompt pauses it before opening so * the 80ms redraw interval can't overwrite the question (see withPrompt). - * Internal — not part of the public Spinner surface. + * Replacement retires the owner, including hosted status. Internal only. */ interface PausableSpinner { pause: () => void; resume: () => void; + retire: () => void; } let activeSpinner: PausableSpinner | null = null; @@ -249,84 +264,59 @@ function spinner(): Spinner { let timer: ReturnType | undefined; let frame = 0; let text = ''; - let hosted = false; + let retired = false; + let host: UiHost | null = null; + let visible = false; const isTty = Boolean(process.stdout.isTTY); + const ownsStatus = () => activeSpinner === handle && !retired; const render = () => { + if (!ownsStatus() || pendingPrompts > 0 || host || isJsonMode()) return; + visible = true; process.stdout.write(`\r${INDENT}${dim(SPINNER_FRAMES[(frame = (frame + 1) % SPINNER_FRAMES.length)])} ${text}`); }; - const clearLine = () => { - if (isTty) process.stdout.write('\r\x1b[2K'); + const pause = () => { + if (timer) clearInterval(timer); + timer = undefined; + // Only erase a line this handle actually drew, never a prompt's line. + if (visible) process.stdout.write('\r\x1b[2K'); + visible = false; }; - const tick = () => { - if (isTty && !timer) { - render(); - timer = setInterval(render, 80); - } + const retire = () => { + if (!ownsStatus()) return; + pause(); + host?.status(null); + retired = true; + activeSpinner = null; }; const handle: Spinner & PausableSpinner = { start(message = '') { + if (retired || isJsonMode()) return; + if (activeSpinner !== handle) activeSpinner?.retire(); + activeSpinner = handle; + host = uiHost; text = message; - if (uiHost) { - hosted = true; - uiHost.status(text); - return; - } - if (isTty) { - tick(); - activeSpinner = handle; - } else { - line(`${dim('…')} ${text}`); - } + if (host) host.status(text); + else if (isTty) handle.resume(); + else line(`${dim('…')} ${text}`); }, message(message: string) { + if (!ownsStatus()) return; text = message; - if (hosted) uiHost?.status(text); + host?.status(text); }, stop(message?: string, code = 0) { - if (timer) { - clearInterval(timer); - timer = undefined; - } - if (activeSpinner === handle) activeSpinner = null; - if (hosted) { - hosted = false; - uiHost?.status(null); - } else { - clearLine(); - } + if (!ownsStatus()) return; + retire(); const final = message ?? text; emit(code === 0 ? 'success' : 'error', final, `${code === 0 ? green('✓') : red('✗')} ${final}`); }, - // Halt + erase without printing a final line (e.g. an orphaned spinner from - // a failed step being cleared before a prompt), and deregister so a prompt - // doesn't resume it. - clear() { - if (timer) { - clearInterval(timer); - timer = undefined; - } - if (activeSpinner === handle) activeSpinner = null; - if (hosted) { - hosted = false; - uiHost?.status(null); - return; - } - clearLine(); - }, - // Pause/resume let a prompt borrow the terminal: pause clears the spinner - // line and halts the redraw interval; resume restarts it. stop() is NOT - // called, so activeSpinner stays registered across the prompt. - pause() { - if (timer) { - clearInterval(timer); - timer = undefined; - } - clearLine(); - }, + clear: retire, + retire, + pause, resume() { - // Only resume if this handle is still the active spinner — never resurrect - // a spinner that was stopped or cleared while the prompt was open. - if (activeSpinner === handle) tick(); + if (!ownsStatus() || host || pendingPrompts > 0 || !isTty || isJsonMode() || timer) return; + render(); + timer = setInterval(render, 80); }, }; return handle; @@ -390,6 +380,9 @@ export class PromptUnavailableError extends Error { * the 80ms redraw interval can't overwrite the question. */ let promptChain: Promise = Promise.resolve(); +// Count queued callers too: no spinner flash or log flush between questions. +let pendingPrompts = 0; +const pendingOutput: Array<() => void> = []; /** Hand a prompt to the UI host, short-circuiting one whose signal already aborted. */ async function hostPrompt(host: UiHost, request: UiPromptRequest): Promise { @@ -397,7 +390,10 @@ async function hostPrompt(host: UiHost, request: UiPromptRequest): Promise(run: () => Promise): Promise { +async function withPrompt( + run: (host: UiHost | null, signal?: AbortSignal) => Promise, + signal?: AbortSignal, +): Promise { if (isJsonMode()) { throw new PromptUnavailableError( 'json', @@ -410,19 +406,29 @@ async function withPrompt(run: () => Promise): Promise { 'This step needs an interactive terminal. Re-run in a terminal, or pass the required flags to run non-interactively.', ); } + const host = uiHost; + if (host) signal = signal ? AbortSignal.any([signal, hostLifetime.signal]) : hostLifetime.signal; const prior = promptChain.catch(() => undefined); let release!: () => void; promptChain = new Promise((resolve) => { release = resolve; }); + activeSpinner?.pause(); + pendingPrompts++; await prior; - const spinner = activeSpinner; - spinner?.pause(); try { - return await run(); + if (signal?.aborted) return CANCEL; + return await run(host, signal); } finally { - spinner?.resume(); - release(); + pendingPrompts--; + if (pendingPrompts === 0) { + pendingOutput.splice(0).forEach((print) => print()); + activeSpinner?.resume(); + } + // Let the caller deliver cancellation to the installer before a queued + // sibling checks its signal. The chain still owns stdin during handoff. + if (pendingPrompts > 0) setTimeout(release, 0); + else release(); } } @@ -451,8 +457,9 @@ export interface ConfirmOptions { } async function confirm(options: ConfirmOptions): Promise { const context = takePromptContext(); - return withPrompt(async () => { - if (uiHost) return hostPrompt(uiHost, { kind: 'confirm', ...options, ...context }); + return withPrompt(async (host, signal) => { + options = { ...options, signal }; + if (host) return hostPrompt(host, { kind: 'confirm', ...options, ...context }); const { confirm: inquirerConfirm } = await import('@inquirer/prompts'); try { return await inquirerConfirm( @@ -463,7 +470,7 @@ async function confirm(options: ConfirmOptions): Promise { if (isCancelError(error)) return CANCEL; throw error; } - }); + }, options.signal); } export interface SelectOption { @@ -482,8 +489,9 @@ export interface SelectOptions { } async function select(options: SelectOptions): Promise { const context = takePromptContext(); - return withPrompt(async () => { - if (uiHost) return hostPrompt(uiHost, { kind: 'select', ...(options as SelectOptions), ...context }); + return withPrompt(async (host, signal) => { + options = { ...options, signal }; + if (host) return hostPrompt(host, { kind: 'select', ...(options as SelectOptions), ...context }); const { select: inquirerSelect } = await import('@inquirer/prompts'); try { return await inquirerSelect( @@ -504,7 +512,7 @@ async function select(options: SelectOptions): Promise { if (isCancelError(error)) return CANCEL; throw error; } - }); + }, options.signal); } export interface TextOptions { @@ -517,8 +525,9 @@ export interface TextOptions { } async function text(options: TextOptions): Promise { const context = takePromptContext(); - return withPrompt(async () => { - if (uiHost) return hostPrompt(uiHost, { kind: 'text', ...options, ...context }); + return withPrompt(async (host, signal) => { + options = { ...options, signal }; + if (host) return hostPrompt(host, { kind: 'text', ...options, ...context }); // @inquirer/input has no placeholder concept, and mapping it to `default` // would auto-submit the hint as the real value on an empty enter. Fold it // into the message so the hint survives (rendered as ghost text previously). @@ -537,7 +546,7 @@ async function text(options: TextOptions): Promise { if (isCancelError(error)) return CANCEL; throw error; } - }); + }, options.signal); } export interface PasswordOptions { @@ -547,8 +556,9 @@ export interface PasswordOptions { } async function password(options: PasswordOptions): Promise { const context = takePromptContext(); - return withPrompt(async () => { - if (uiHost) return hostPrompt(uiHost, { kind: 'password', ...options, ...context }); + return withPrompt(async (host, signal) => { + options = { ...options, signal }; + if (host) return hostPrompt(host, { kind: 'password', ...options, ...context }); const { password: inquirerPassword } = await import('@inquirer/prompts'); try { return await inquirerPassword( @@ -559,7 +569,7 @@ async function password(options: PasswordOptions): Promise { if (isCancelError(error)) return CANCEL; throw error; } - }); + }, options.signal); } // ── Default export (the `ui` facade) ────────────────────────────────────────