From 8ca821de0ee32d10026463f690921bac9911991a Mon Sep 17 00:00:00 2001 From: dbarr5 Date: Wed, 19 Aug 2026 06:54:18 -0400 Subject: [PATCH] fix(chat): thread the abort signal into local turns so Ctrl+C works MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lane SC-A1, slice 1 of the cancellable tool runtime. The REPL builds an AbortController per turn and aborts it on Ctrl+C. runTurn accepted that signal and then dropped it on the local branch: const backend = await resolveBackend(ctx); if (backend === "local") { await runLocalTurn(ctx, prompt); // signal not passed return; } So Ctrl+C did nothing to a local turn. The abort fired and nothing was listening; the turn ran to completion regardless. Only the cloud path was ever cancellable. runLocalTurn now takes the signal and closes the brain on abort. close() is what unblocks a loop parked on a tool result, so an abort arriving mid-turn is observed rather than waiting the turn out. The listener is registered before the loop starts, so a signal that is already aborted is honoured instead of starting work that was cancelled before it began. An aborted turn returns rather than throwing. The user asked for the stop; it is not a failed turn and must not be reported as one. Adds a LocalTurnDeps seam ({ brain, exec }) mirroring the one smoke.ts already uses, so the abort path is testable without an Ollama server, a child process or real tool execution. runLocalTurn becomes exported for the same reason. Tests: 4 added, driving a brain that emits one tool_call and then parks exactly as the real one does while awaiting sendToolResult — abort reaches the brain, an already-aborted signal stops the turn, an aborted turn does not reject, and a turn with no signal still completes. Mutation-checked, and the failure mode is the interesting part: restoring the old behaviour does not fail the tests, it HANGS them. The runner is killed by timeout with exit 124, having produced no TAP summary at all, because the turn never settles. Restored, the same run exits 0. That hang is precisely what a user experienced when they pressed Ctrl+C. Gates at this commit: npm run typecheck exit 0 npm test 926 pass / 0 fail (922 on clean 41a7e261) Scope note. This makes a local turn cancellable BETWEEN steps — during a model request, or while parked awaiting a tool result. It does not interrupt a tool already executing: tool_executor.ts still uses blocking spawnSync with no AbortSignal, so Ctrl+C during a long `run_tests` is still not observed until that command returns, and the command's children are still orphaned on timeout. Fixing that requires spawn() with process-group cleanup, which requires ToolExecutor.run to become async, which requires finalVerify to become async — it is the synchronous ground-truth gate (verify_gate.ts:71, called from code.ts:371, with 13 tests pinning its behaviour). That is a deliberate, separate slice rather than something to graft onto this one. --- src/commands/chat.ts | 32 ++++++++++-- test/chat_local_abort.test.ts | 98 +++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 4 deletions(-) create mode 100644 test/chat_local_abort.test.ts diff --git a/src/commands/chat.ts b/src/commands/chat.ts index 43d2c15..7f04b53 100644 --- a/src/commands/chat.ts +++ b/src/commands/chat.ts @@ -31,6 +31,8 @@ import { loadHistory, appendHistory, historyPath, historyEnabled } from "../core import { VERSION } from "../version.js"; import { chooseBackend, type BackendPath } from "../core/backend.js"; import { OllamaBrain } from "../core/brain_ollama.js"; +import type { Brain } from "../core/brain.js"; +import type { ToolResult } from "../core/tool_executor.js"; import { ToolExecutor } from "../core/tool_executor.js"; import { HostRenderer } from "../ui/host_render.js"; import type { TaskCommand } from "../core/brain.js"; @@ -106,7 +108,9 @@ export async function runTurn( ): Promise { const backend = await resolveBackend(ctx); if (backend === "local") { - await runLocalTurn(ctx, prompt); + // The signal was dropped here, so the REPL's Ctrl+C controller could not + // reach a local turn at all — the abort fired and nothing observed it. + await runLocalTurn(ctx, prompt, signal); return; } await runCloudTurn(ctx, prompt, signal, onFrame, onPulsePaint); @@ -199,10 +203,20 @@ async function runCloudTurn( * each tool_call (one path-guarded ToolExecutor) and replies, the HostRenderer * draws every event. Identical UX to cloud, just an offline brain. */ -async function runLocalTurn(ctx: AppContext, prompt: string): Promise { +export interface LocalTurnDeps { + brain?: Brain; + exec?: { executeAsync(name: string, args: Record): Promise }; +} + +export async function runLocalTurn( + ctx: AppContext, + prompt: string, + signal?: AbortSignal, + deps: LocalTurnDeps = {}, +): Promise { const cwd = ctx.flags.cwd; - const brain = new OllamaBrain(ctx.flags.model ? { model: ctx.flags.model } : {}); - const exec = new ToolExecutor(cwd); + const brain = deps.brain ?? new OllamaBrain(ctx.flags.model ? { model: ctx.flags.model } : {}); + const exec = deps.exec ?? new ToolExecutor(cwd); const renderer = new HostRenderer({ poolGb: 5, json: ctx.flags.json }); const approveTool = async (name: string, args: Record): Promise => { const outcome = decideGate(name, ctx.cfg.permissionMode, ctx.cfg.autoApply, { @@ -226,8 +240,15 @@ async function runLocalTurn(ctx: AppContext, prompt: string): Promise { ...(ctx.flags.model ? { model: ctx.flags.model } : {}), }; let sawError: string | null = null; + // close() is what unblocks a loop parked on a tool result, so an abort that + // arrives mid-turn is observed rather than waiting out the whole turn. + // Registered before the loop starts so an already-aborted signal still fires. + const onAbort = (): void => brain.close(); + if (signal?.aborted) brain.close(); + signal?.addEventListener("abort", onAbort, { once: true }); try { for await (const ev of brain.run(task)) { + if (signal?.aborted) break; renderer.event(ev); if (ev.type === "error") sawError = ev.msg; if (ev.type === "done" && !ev.ok) sawError = ev.result || ev.reason || "turn did not complete"; @@ -241,8 +262,11 @@ async function runLocalTurn(ctx: AppContext, prompt: string): Promise { } } } finally { + signal?.removeEventListener("abort", onAbort); brain.close(); } + // An aborted turn is not a failed one — the user asked for it to stop. + if (signal?.aborted) return; // Mirror runCloudTurn/CONTRACTS.md invariant 5: a streamed error event is a // failed turn, not a silently-successful one — the renderer already painted // it, so callers (cmdChat/run.ts) special-case ChatTurnError to avoid a diff --git a/test/chat_local_abort.test.ts b/test/chat_local_abort.test.ts new file mode 100644 index 0000000..458a6dc --- /dev/null +++ b/test/chat_local_abort.test.ts @@ -0,0 +1,98 @@ +// The REPL builds an AbortController per turn and aborts it on Ctrl+C, but +// runTurn dropped the signal on the local branch — it was accepted as a +// parameter and simply not passed on. Ctrl+C therefore did nothing to a local +// turn: the abort fired and nothing was listening. +// +// These drive runLocalTurn with an injected brain, so no Ollama server, no +// child process and no real tool execution is involved. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { runLocalTurn } from "../src/commands/chat.js"; +import type { AppContext } from "../src/core/context.js"; +import type { Brain, TaskCommand } from "../src/core/brain.js"; +import type { BrainEvent } from "../src/core/brain_protocol.js"; +import type { ToolResult } from "../src/core/tool_executor.js"; + +/** + * A brain that emits one tool_call and then parks forever, exactly like the + * real one waiting on sendToolResult. close() is the only thing that frees it, + * so a turn that ignores the abort signal never returns. + */ +class ParkingBrain implements Brain { + closed = false; + private release: (() => void) | null = null; + + run(_task: TaskCommand): AsyncIterable { + const self = this; + return (async function* (): AsyncGenerator { + yield { type: "tool_call", id: "call-1", name: "read_file", args: { path: "a.ts" } }; + await new Promise((resolve) => { + if (self.closed) resolve(); + else self.release = resolve; + }); + yield { type: "done", ok: true, result: "done", remaining: 0, reason: "" }; + })(); + } + + sendToolResult(_id: string, _result: ToolResult): void {} + control(): void {} + close(): void { + this.closed = true; + this.release?.(); + this.release = null; + } +} + +function ctx(): AppContext { + return { + cfg: { permissionMode: "skip", autoApply: true }, + flags: { cwd: process.cwd(), yes: true, json: true }, + confirm: async () => true, + } as unknown as AppContext; +} + +const noExec = { + executeAsync: async (): Promise => ({ output: "ok", exitCode: 0 }), +}; + +test("aborting a local turn closes the brain instead of waiting the turn out", async () => { + const brain = new ParkingBrain(); + const controller = new AbortController(); + const turn = runLocalTurn(ctx(), "do a thing", controller.signal, { brain, exec: noExec }); + + // Let the turn reach the parked tool call, then Ctrl+C. + await new Promise((resolve) => setTimeout(resolve, 20)); + controller.abort(); + + // Without the signal wired through, this never settles. + await turn; + assert.equal(brain.closed, true, "abort must reach the brain"); +}); + +test("a signal already aborted stops the turn rather than starting it", async () => { + const brain = new ParkingBrain(); + const controller = new AbortController(); + controller.abort(); + await runLocalTurn(ctx(), "do a thing", controller.signal, { brain, exec: noExec }); + assert.equal(brain.closed, true); +}); + +test("an aborted local turn is not reported as a failed turn", async () => { + // A user-requested stop is not an error, and must not surface as one. + const brain = new ParkingBrain(); + const controller = new AbortController(); + const turn = runLocalTurn(ctx(), "do a thing", controller.signal, { brain, exec: noExec }); + await new Promise((resolve) => setTimeout(resolve, 20)); + controller.abort(); + await assert.doesNotReject(() => turn); +}); + +test("without a signal a local turn still completes normally", async () => { + const brain = new ParkingBrain(); + const turn = runLocalTurn(ctx(), "do a thing", undefined, { brain, exec: noExec }); + await new Promise((resolve) => setTimeout(resolve, 20)); + brain.close(); // stand in for the real brain finishing its wait + await turn; + assert.equal(brain.closed, true); +});