diff --git a/src/gateway/interactive-readline.test.ts b/src/gateway/interactive-readline.test.ts new file mode 100644 index 0000000..e904f4f --- /dev/null +++ b/src/gateway/interactive-readline.test.ts @@ -0,0 +1,93 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { + createInteractiveReadline, + disposeInteractiveReadline, + __resetInteractiveReadlineForTests, + __getInteractiveReadlineInvocationCountForTests, +} from "./interactive-readline.js"; + +describe("interactive-readline factory", () => { + beforeEach(() => { + __resetInteractiveReadlineForTests(); + }); + + afterEach(() => { + __resetInteractiveReadlineForTests(); + delete process.env.LOG_LEVEL; + }); + + it("returns a fresh, independent interface per call", () => { + const rl1 = createInteractiveReadline(); + const rl2 = createInteractiveReadline(); + + expect(rl1).not.toBe(rl2); + expect(__getInteractiveReadlineInvocationCountForTests()).toBe(2); + }); + + it("uses terminal:false (cooked mode) to avoid Windows raw-mode issues", () => { + // With terminal:false, readline does NOT attach a keypress listener + // to stdin. We can't directly read the option from the public API, + // but we can assert the side effect: no keypress listener growth. + const before = process.stdin.listenerCount("keypress"); + + const rl = createInteractiveReadline(); + + expect(process.stdin.listenerCount("keypress")).toBe(before); + + disposeInteractiveReadline(rl); + }); + + it("disposeInteractiveReadline brings keypress listener count back down", () => { + const before = process.stdin.listenerCount("keypress"); + + const rl = createInteractiveReadline(); + disposeInteractiveReadline(rl); + + expect(process.stdin.listenerCount("keypress")).toBeLessThanOrEqual(before); + }); + + it("does not remove keypress listeners that existed before the prompt", () => { + const existingListener = () => undefined; + const leakedPromptListener = () => undefined; + process.stdin.on("keypress", existingListener); + + try { + const rl = createInteractiveReadline(); + process.stdin.on("keypress", leakedPromptListener); + disposeInteractiveReadline(rl); + + expect(process.stdin.listeners("keypress")).toContain(existingListener); + expect(process.stdin.listeners("keypress")).not.toContain( + leakedPromptListener, + ); + } finally { + process.stdin.removeListener("keypress", existingListener); + process.stdin.removeListener("keypress", leakedPromptListener); + } + }); + + it("disposeInteractiveReadline is safe to call on an already-closed interface", () => { + const rl = createInteractiveReadline(); + rl.close(); + expect(() => disposeInteractiveReadline(rl)).not.toThrow(); + }); + + it("does not accumulate keypress listeners across create/dispose cycles", () => { + const before = process.stdin.listenerCount("keypress"); + + for (let i = 0; i < 5; i += 1) { + const rl = createInteractiveReadline(); + disposeInteractiveReadline(rl); + } + + expect(process.stdin.listenerCount("keypress")).toBeLessThanOrEqual(before); + }); + + it("does not throw when debug logging is enabled", () => { + process.env.LOG_LEVEL = "debug"; + expect(() => { + const rl = createInteractiveReadline(); + disposeInteractiveReadline(rl); + }).not.toThrow(); + }); +}); diff --git a/src/gateway/interactive-readline.ts b/src/gateway/interactive-readline.ts new file mode 100644 index 0000000..4595814 --- /dev/null +++ b/src/gateway/interactive-readline.ts @@ -0,0 +1,104 @@ +import readline from "node:readline/promises"; +import { stdin as input, stdout as output } from "node:process"; +import { appendStderrDevLog } from "../runtime/stderr-dev-log.js"; + +/** + * Create a readline interface for one-shot prompts (tool approval, user + * clarification). Use `disposeInteractiveReadline` in the prompt's + * `finally` block to tear it down. + * + * Uses `terminal: false` deliberately. The previous `terminal: true` + * configuration put stdin into raw mode and attached a `keypress` listener, + * both of which are unreliable on Windows (ConHost / Windows Terminal): + * + * 1. `rl.close()` does not reliably remove the keypress listener, causing + * listener accumulation across prompts. At the Nth prompt, each leaked + * listener echoed the same keystroke, so pressing `y` once rendered as + * N `y` characters. + * 2. Windows console focus / IME / mode-switch events leak through raw + * mode as spurious empty line events, so `rl.question()` resolved with + * `""` on its own — the "auto-advance" symptom where each prompt + * disappeared without user input. + * + * `terminal: false` keeps stdin in cooked mode: the OS handles line editing + * and delivers complete lines only when the user presses Enter. No raw + * mode, no keypress listener, no spurious events. + * + * Trade-off: no in-line editing (backspace, arrow keys, history). For y/n + * approval prompts that's acceptable; users type one char + Enter. + */ +let promptInvocationCount = 0; +const keypressListenersByReadline = new WeakMap< + readline.Interface, + Function[] +>(); + +function isDebugLogEnabled(): boolean { + const level = process.env.LOG_LEVEL; + return level === "debug" || level === "trace"; +} + +function logApprovalReadlineDebug(message: string): void { + if (!isDebugLogEnabled()) return; + void appendStderrDevLog(`[debug] event=approval-readline ${message}\n`); +} + +export function createInteractiveReadline(): readline.Interface { + promptInvocationCount += 1; + const keypressListenersBeforeCreate = input.listeners("keypress"); + logApprovalReadlineDebug( + `create invocation=${promptInvocationCount} terminal=false`, + ); + const rl = readline.createInterface({ + input, + output, + terminal: false, + }); + keypressListenersByReadline.set(rl, keypressListenersBeforeCreate); + return rl; +} + +export function disposeInteractiveReadline(rl: readline.Interface): void { + const keypressListenersBeforeCreate = + keypressListenersByReadline.get(rl) ?? []; + logApprovalReadlineDebug( + `dispose invocation=${promptInvocationCount} keypressListeners=${input.listenerCount("keypress")}`, + ); + + try { + rl.close(); + } catch { + // Ignore: may already be closed. + } + + // A console host can still leave a keypress listener behind. Remove only + // listeners added after this interface was created, never ones owned by + // the long-lived REPL readline interface. + const retainedListenerCounts = new Map(); + for (const listener of keypressListenersBeforeCreate) { + retainedListenerCounts.set( + listener, + (retainedListenerCounts.get(listener) ?? 0) + 1, + ); + } + for (const listener of input.listeners("keypress")) { + const retainedCount = retainedListenerCounts.get(listener) ?? 0; + if (retainedCount > 0) { + retainedListenerCounts.set(listener, retainedCount - 1); + continue; + } + input.removeListener("keypress", listener); + } + + keypressListenersByReadline.delete(rl); +} + +/** Test-only: reset invocation counter between unit tests. */ +export function __resetInteractiveReadlineForTests(): void { + promptInvocationCount = 0; +} + +/** Test-only: observe invocation count. */ +export function __getInteractiveReadlineInvocationCountForTests(): number { + return promptInvocationCount; +} diff --git a/src/gateway/runtime.ts b/src/gateway/runtime.ts index 8dc4281..d1e7b01 100644 --- a/src/gateway/runtime.ts +++ b/src/gateway/runtime.ts @@ -115,6 +115,10 @@ import { type StepCliInteractiveUi, type StepCliInteractiveUiFactory, } from "./interactive-ui.js"; +import { + createInteractiveReadline, + disposeInteractiveReadline, +} from "./interactive-readline.js"; import { createFilesystemAgentRunArtifactStore } from "./artifacts/run-artifact-store.js"; import { FilesystemConversationTranscriptStore } from "./memory/filesystem-conversation-transcript-store.js"; import { FilesystemFreshAttemptProgressStore } from "./memory/filesystem-fresh-attempt-progress-store.js"; @@ -4606,11 +4610,10 @@ async function promptForToolApproval( : "deny"; } - const rl = readline.createInterface({ - input, - output, - terminal: true, - }); + // Per-call readline in cooked mode (terminal:false). See + // `interactive-readline.ts` for why `terminal:true` is unreliable on + // Windows (listener leak + spurious empty line events). + const rl = createInteractiveReadline(); const preview = request.rawArgs.length > 240 @@ -4692,7 +4695,7 @@ async function promptForToolApproval( output.write(`Unrecognized decision: ${answer}. Type '?' for help.\n`); } } finally { - rl.close(); + disposeInteractiveReadline(rl); } } @@ -4708,11 +4711,8 @@ async function promptForUserClarification( }; } - const rl = readline.createInterface({ - input, - output, - terminal: true, - }); + // Per-call readline in cooked mode. See `interactive-readline.ts`. + const rl = createInteractiveReadline(); const options = Array.isArray(request.options) ? request.options : []; const helpLines = buildClarificationHelpLines(request); @@ -4779,7 +4779,7 @@ async function promptForUserClarification( output.write(`${parsed.message}\n`); } } finally { - rl.close(); + disposeInteractiveReadline(rl); } } diff --git a/vitest.config.ts b/vitest.config.ts index 2ea1608..8bf09f1 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -76,6 +76,7 @@ export default defineConfig({ "src/bootstrap/config/defaults.ts", "src/commands/option-parsers.ts", "src/gateway/storage/layout.ts", + "src/gateway/interactive-readline.ts", "src/gateway/verifier.ts", "src/runtime/runtime-config.ts", "src/runtime/runtime-utils.ts",