diff --git a/src/handlers/server-requests.ts b/src/handlers/server-requests.ts index b646c8e..dfb56d5 100644 --- a/src/handlers/server-requests.ts +++ b/src/handlers/server-requests.ts @@ -294,8 +294,11 @@ async function handleSinglePermission( undefined, turn, ); + if (acpResp === INTERRUPTED) { + return onInteractionInterrupted(cx, acpSid, toolCallId, turn); + } if (acpResp === null) { - return { action: "decline", reason: "timeout or cancelled" }; + return { action: "decline", reason: "declined or cancelled" }; } return perm ? acpPermissionResponseToZcode(acpResp) @@ -305,7 +308,7 @@ async function handleSinglePermission( /** * AskUserQuestion: sequential per-question, multi-select per-option. * - * Single-select: one popup per question; Skip/cancel/timeout → overall decline. + * Single-select: one popup per question; Skip/cancel → overall decline. * Multi-select: one Include/Skip popup per option; Include picks comma-joined. */ export async function handleAskUserQuestion( @@ -345,7 +348,7 @@ export async function handleAskUserQuestion( } const selected = parseAskUserResponse(resp); if (selected === null) { - warn(` ⚠ AskUserQuestion [${idx + 1}] skip/cancel/timeout, declining`); + warn(` ⚠ AskUserQuestion [${idx + 1}] skip/cancel, declining`); return { action: "decline", reason: "skipped or cancelled" }; } answers[q.question] = selected; @@ -368,12 +371,12 @@ export async function handleAskUserQuestion( acpParams.toolCall.toolCallId = `${toolCallId}_${idx}_${sub}`; const resp = await askOnce(server, cx, acpParams, idx + 1, qs.length, label, turn); // Abort the whole multi-select if the turn was cancelled (user sent a - // new prompt) or the popup timed out — otherwise the remaining options - // keep popping up and block the new task. Mirrors the single-select - // path's null → decline behaviour. + // new prompt) or the popup returned nothing — otherwise the remaining + // options keep popping up and block the new task. Mirrors the + // single-select path's null → decline behaviour. if (turn?.cancelled || resp === null) { - warn(` ⚠ AskUserQuestion [${idx + 1}] multi aborted (cancel/timeout), declining`); - return { action: "decline", reason: "cancelled or timed out" }; + warn(` ⚠ AskUserQuestion [${idx + 1}] multi aborted (cancel/interrupt), declining`); + return { action: "decline", reason: "cancelled or interrupted" }; } if (parseAskUserResponse(resp) === "yes") { picked.push(label); @@ -428,6 +431,9 @@ async function handleAskUserViaElicitation( undefined, turn, ); + if (acpResp === INTERRUPTED) { + return onInteractionInterrupted(cx, acpSid, toolCallId, turn); + } if (acpResp === null) { return { action: "decline", reason: "elicitation failed" }; } @@ -477,7 +483,7 @@ async function askOnce( ): Promise { const acpReqId = _server.nextId(); log(` ⟳ AskUserQuestion forwarding session/request_permission (acp_id=${acpReqId})`); - return requestWithTimeout( + const resp = await requestWithTimeout( cx, "session/request_permission", acpParams, @@ -485,32 +491,67 @@ async function askOnce( undefined, turn, ); + if (resp === INTERRUPTED) { + // Interrupted (connection close / env timeout / cancel): mark the popup's + // tool_call failed and flip turn.cancelled so the outer loop aborts and the + // turn loop stops the backend. Return null so callers' existing null → + // decline branch handles the reply uniformly. + await onInteractionInterrupted(cx, acpParams.sessionId, acpParams.toolCall.toolCallId, turn); + return null; + } + return resp; } // ---------- request helpers ---------- /** - * Interaction request timeout. Python's `_await_client_response` uses 600s and - * this matches it — long enough for slow models, retries, and user deliberation - * (well past the turn loop's 120s no-progress guard), while still bounding a - * dead client so it cannot pin a turn forever (the turn loop's - * `await handleServerRequests` would otherwise block indefinitely and the 120s - * no-progress timer could never re-check). + * Interaction request wait strategy. + * + * By default we wait INDEFINITELY for the user to respond to a confirmation + * popup — this matches every mainstream agent (Claude Code, Gemini CLI, Codex, + * Cursor all wait forever for tool-auth/ExitPlanMode/AskUserQuestion; the ACP + * spec has no timeout on `session/request_permission`). A finite timeout that + * auto-declines is actively harmful: it decides for the user (often the + * opposite of their intent) and then keeps running, which is exactly the bug + * this fixed. + * + * Two things can still break a pending wait: + * 1. `turn.cancelled` — the user pressed stop / sent a new prompt (preempt). + * 2. Connection close (`cx.signal` abort / `cx.closed` resolves) — the editor + * went away. This is the real crash signal and replaces the old timeout. + * + * An explicit timeout is kept as an opt-in escape hatch via the + * `ZCODE_ACP_INTERACTION_TIMEOUT_MS` env var (milliseconds; 0/unset = wait + * forever). On any of these interrupts the caller replies `decline` to unlock + * the backend AND flips `turn.cancelled` so the turn loop stops the backend + * turn instead of auto-continuing. */ -const INTERACTION_TIMEOUT_MS = 600_000; +const INTERACTION_TIMEOUT_MS = parseInteractionTimeout(); + +function parseInteractionTimeout(): number { + const raw = process.env.ZCODE_ACP_INTERACTION_TIMEOUT_MS; + if (!raw) return 0; // 0 = wait indefinitely + const n = Number(raw); + return Number.isFinite(n) && n > 0 ? Math.floor(n) : 0; +} /** - * Send a client-side request with a timeout and turn-cancel awareness. Resolves - * to the client response, or `null` on timeout/error/cancel (callers treat null - * as decline). The underlying `cx.request` promise stays pending after a - * timeout/cancel (the SDK has no abort), but we no longer await it; it settles - * naturally when the client eventually responds or the connection closes. - * - * Turn cancel: while awaiting the client response we poll `turn.cancelled` - * every 100ms (mirrors Python `_await_client_response`, which drains + checks - * cancel every 0.1s). Without this, a user pressing stop during a permission - * popup would be ignored until the client responds or 600s elapses, because the - * turn loop's `await handleServerRequests` blocks here. + * Marker returned when a wait was interrupted (connection close, env timeout, + * or turn cancel) as opposed to the client deliberately returning null. Callers + * flip `turn.cancelled` on this so the backend turn is stopped rather than + * allowed to continue after the decline reply. + */ +const INTERRUPTED = Symbol("interactionInterrupted"); +type InteractionResult = unknown | typeof INTERRUPTED; + +/** + * Send a client-side request and wait for the response. By default waits + * indefinitely (see {@link INTERACTION_TIMEOUT_MS}). Resolves to the client + * response, `null` if the client returned null/errored, or {@link INTERRUPTED} + * if the wait was broken by connection close / env timeout / turn cancel. The + * underlying `cx.request` promise stays pending after an interrupt (the SDK has + * no abort) but is no longer awaited; it settles naturally when the client + * eventually responds or the connection closes. */ async function requestWithTimeout( cx: acp.AgentContext, @@ -519,46 +560,99 @@ async function requestWithTimeout( label: string, timeoutMs = INTERACTION_TIMEOUT_MS, turn?: PendingTurn, -): Promise { - let timer: ReturnType | undefined; - let cancelTimer: ReturnType | undefined; - const timeout = new Promise((resolve) => { - timer = setTimeout(() => { - warn(` ⚠ ${label} timed out after ${timeoutMs}ms`); - resolve(null); - }, timeoutMs); - }); - const racers: Array> = [ +): Promise { + // Build the racers. The primary is the client request itself. + const racers: Array> = [ cx.request(method, params as never).catch((e: unknown) => { warn(` ⚠ ${label} failed: ${e instanceof Error ? e.message : String(e)}`); return null; }), - timeout, ]; - // Race a cancel poll so the user can abort a pending popup. Resolves null on - // cancel; callers already treat null as decline. Only added when a turn is - // available (turn-less callers, e.g. tests, skip cancel awareness). + + // Connection-close detection: replaces the old finite timeout as the crash + // guard. `cx.signal` is an AbortSignal that fires when the stream closes; + // `cx.closed` is the Promise form. Either being present is enough. + const signal = (cx as { signal?: AbortSignal }).signal; + const closed = (cx as { closed?: Promise }).closed; + if (signal || closed) { + racers.push( + new Promise((resolve) => { + let done = false; + const fire = () => { + if (done) return; + done = true; + warn(` ⚠ ${label} aborted (client connection closed)`); + resolve(INTERRUPTED); + }; + signal?.addEventListener("abort", fire); + closed?.then(fire).catch(() => {}); + }), + ); + } + + // Optional env timeout (off by default). + if (timeoutMs > 0) { + racers.push( + new Promise((resolve) => { + const t = setTimeout(() => { + warn(` ⚠ ${label} timed out after ${timeoutMs}ms`); + resolve(INTERRUPTED); + }, timeoutMs); + t.unref?.(); + }), + ); + } + + // Turn-cancel poll: lets the user abort a pending popup via stop/preempt. if (turn) { racers.push( - new Promise((resolve) => { - cancelTimer = setInterval(() => { + new Promise((resolve) => { + const cancelTimer = setInterval(() => { if (turn.cancelled) { warn(` ⚠ ${label} aborted (turn cancelled)`); - resolve(null); + resolve(INTERRUPTED); } }, 100); - // unref so this polling interval cannot keep the event loop (and thus - // the process) alive while awaiting a client response (up to 600s). + // unref so this polling interval cannot keep the event loop alive. cancelTimer.unref?.(); }), ); } - try { - return await Promise.race(racers); - } finally { - if (timer) clearTimeout(timer); - if (cancelTimer) clearInterval(cancelTimer); + + return Promise.race(racers); +} + +/** + * Handle an interrupted interaction wait (connection close, env timeout, or + * turn cancel). Emits a `failed` tool_call_update so the editor shows a clear + * red marker, and flips `turn.cancelled` so the turn loop stops the backend + * turn instead of auto-continuing after the decline reply. + * + * Returns the decline response the caller should send back to zcode. + */ +async function onInteractionInterrupted( + cx: acp.AgentContext, + acpSid: string, + toolCallId: string, + turn?: PendingTurn, +): Promise { + if (toolCallId) { + await sendSessionUpdate(cx, acpSid, { + sessionUpdate: "tool_call_update", + toolCallId, + status: "failed", + content: [ + { + type: "content", + content: { type: "text", text: "交互中断：连接关闭或超时，请重新发起对话。" }, + }, + ], + }).catch(() => { + /* best-effort: editor may already be gone */ + }); } + if (turn) turn.cancelled = true; + return { action: "decline", reason: "interrupted (connection closed or timeout)" }; } // ---------- reply helpers ---------- diff --git a/tests/ask-user-cancel.test.ts b/tests/ask-user-cancel.test.ts index 8a383ae..d8a1699 100644 --- a/tests/ask-user-cancel.test.ts +++ b/tests/ask-user-cancel.test.ts @@ -3,9 +3,9 @@ * * Bug: when an AskUserQuestion carries multiple questions (especially * multi-select ones), the fallback `request_permission` path emitted one - * popup per question / per option in a tight loop with no cancel/timeout - * check. If the user sent a new prompt (preempting the turn) or a popup - * timed out, the remaining popups still fired and blocked the new task. + * popup per question / per option in a tight loop with no cancel/interrupt + * check. If the user sent a new prompt (preempting the turn) or a popup was + * interrupted, the remaining popups still fired and blocked the new task. * * These tests verify the fix: the loop now aborts immediately on * `turn.cancelled` or `resp === null`, returning a single decline instead @@ -86,11 +86,11 @@ function twoSingleSelectParams(): ZcodeInteractionUserInputParams { } describe("AskUserQuestion multi-select fallback: cancel/timeout aborts loop", () => { - it("stops after the first option times out (resp=null) and declines", async () => { + it("stops after the first option returns null (declined) and declines", async () => { const server = makeServer(); - const { cx, callCount } = makeCx([null, null, null]); // all time out + const { cx, callCount } = makeCx([null, null, null]); // all declined const result = await handleAskUserQuestion(server, cx, "s1", multiSelectParams()); - expect(result).toEqual({ action: "decline", reason: "cancelled or timed out" }); + expect(result).toEqual({ action: "decline", reason: "cancelled or interrupted" }); // Only ONE request_permission should have fired (the first option), not 3. expect(callCount()).toBe(1); }); @@ -116,7 +116,7 @@ describe("AskUserQuestion multi-select fallback: cancel/timeout aborts loop", () return r; }) as typeof origRequest; const result = await handleAskUserQuestion(server, cx, "s1", multiSelectParams(), turn); - expect(result).toEqual({ action: "decline", reason: "cancelled or timed out" }); + expect(result).toEqual({ action: "decline", reason: "cancelled or interrupted" }); // Only one popup fired despite 3 options. expect(callCount()).toBe(1); });