From 4305d55108d551a7d7a94a061211c262eb64f29c Mon Sep 17 00:00:00 2001 From: knqiufan Date: Sun, 21 Jun 2026 18:04:57 +0800 Subject: [PATCH 1/2] fix(gateway): use cooked-mode readline for approval prompts on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows console + `terminal: true` readline leaks keypress listeners across prompts (`rl.close()` does not reliably remove the listener on Windows). At the Nth prompt stdin has N accumulated listeners, which (a) synthesize spurious empty-line submits from focus/IME/mode-switch events and (b) echo a single keystroke N times via `_ttyWrite`. Result: prompts auto-advance after ~30s idle, and a single `y` becomes `yy`/`yyy`, which fails the decision parser and deadlocks the session. Switch both `promptForToolApproval` and `promptForUserClarification` to a factory that creates the readline interface with `terminal: false` (cooked mode). The OS handles line editing and delivers complete lines only on Enter — no raw mode, no keypress listener, no spurious events. Trade-off: no in-line editing (backspace, arrows) inside the prompt; acceptable for y/n approval where the user types one char + Enter. `disposeInteractiveReadline` also runs `input.pause()`, `setRawMode(false)`, and removes any stray `keypress` listeners as defensive cleanup for Windows hosts where residue was observed even with `terminal: false`. Closes #76 Co-Authored-By: Claude Opus 4.7 --- src/gateway/interactive-readline.test.ts | 73 ++++++++++++++++++ src/gateway/interactive-readline.ts | 96 ++++++++++++++++++++++++ src/gateway/runtime.ts | 24 +++--- vitest.config.ts | 1 + 4 files changed, 182 insertions(+), 12 deletions(-) create mode 100644 src/gateway/interactive-readline.test.ts create mode 100644 src/gateway/interactive-readline.ts diff --git a/src/gateway/interactive-readline.test.ts b/src/gateway/interactive-readline.test.ts new file mode 100644 index 0000000..a190005 --- /dev/null +++ b/src/gateway/interactive-readline.test.ts @@ -0,0 +1,73 @@ +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("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..ba50193 --- /dev/null +++ b/src/gateway/interactive-readline.ts @@ -0,0 +1,96 @@ +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; + +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; + logApprovalReadlineDebug( + `create invocation=${promptInvocationCount} terminal=false`, + ); + return readline.createInterface({ + input, + output, + terminal: false, + }); +} + +export function disposeInteractiveReadline(rl: readline.Interface): void { + logApprovalReadlineDebug( + `dispose invocation=${promptInvocationCount} keypressListeners=${input.listenerCount("keypress")}`, + ); + + try { + rl.close(); + } catch { + // ignore — may already be closed + } + + // Windows defensive cleanup. With `terminal: false` this is mostly + // belt-and-suspenders (no raw mode, no keypress listener expected), but + // we have observed residue in some console hosts. + try { + if (typeof input.setRawMode === "function") { + input.setRawMode(false); + } + } catch { + // ignore — stdin may already be destroyed + } + + try { + input.pause(); + } catch { + // ignore — stdin may already be destroyed + } + + const keypressListeners = input.listeners("keypress") as (() => void)[]; + for (const listener of keypressListeners) { + input.removeListener("keypress", listener); + } +} + +/** 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", From 7beb47df4800507d2730cd7c2719d8d8e103e604 Mon Sep 17 00:00:00 2001 From: knqiufan Date: Sat, 18 Jul 2026 02:25:26 +0800 Subject: [PATCH 2/2] fix(gateway): preserve REPL keypress listeners --- src/gateway/interactive-readline.test.ts | 20 ++++++++++ src/gateway/interactive-readline.ts | 48 ++++++++++++++---------- 2 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/gateway/interactive-readline.test.ts b/src/gateway/interactive-readline.test.ts index a190005..e904f4f 100644 --- a/src/gateway/interactive-readline.test.ts +++ b/src/gateway/interactive-readline.test.ts @@ -46,6 +46,26 @@ describe("interactive-readline factory", () => { 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(); diff --git a/src/gateway/interactive-readline.ts b/src/gateway/interactive-readline.ts index ba50193..4595814 100644 --- a/src/gateway/interactive-readline.ts +++ b/src/gateway/interactive-readline.ts @@ -28,6 +28,10 @@ import { appendStderrDevLog } from "../runtime/stderr-dev-log.js"; * 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; @@ -41,17 +45,22 @@ function logApprovalReadlineDebug(message: string): void { export function createInteractiveReadline(): readline.Interface { promptInvocationCount += 1; + const keypressListenersBeforeCreate = input.listeners("keypress"); logApprovalReadlineDebug( `create invocation=${promptInvocationCount} terminal=false`, ); - return readline.createInterface({ + 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")}`, ); @@ -59,30 +68,29 @@ export function disposeInteractiveReadline(rl: readline.Interface): void { try { rl.close(); } catch { - // ignore — may already be closed + // Ignore: may already be closed. } - // Windows defensive cleanup. With `terminal: false` this is mostly - // belt-and-suspenders (no raw mode, no keypress listener expected), but - // we have observed residue in some console hosts. - try { - if (typeof input.setRawMode === "function") { - input.setRawMode(false); - } - } catch { - // ignore — stdin may already be destroyed - } - - try { - input.pause(); - } catch { - // ignore — stdin may already be destroyed + // 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, + ); } - - const keypressListeners = input.listeners("keypress") as (() => void)[]; - for (const listener of keypressListeners) { + 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. */