diff --git a/openspec/changes/graceful-streaming-interruption/.openspec.yaml b/openspec/changes/graceful-streaming-interruption/.openspec.yaml new file mode 100644 index 0000000..de73b34 --- /dev/null +++ b/openspec/changes/graceful-streaming-interruption/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-06-25 diff --git a/openspec/changes/graceful-streaming-interruption/design.md b/openspec/changes/graceful-streaming-interruption/design.md new file mode 100644 index 0000000..b2294da --- /dev/null +++ b/openspec/changes/graceful-streaming-interruption/design.md @@ -0,0 +1,56 @@ +## Context + +The TUI chat interface streams agent responses in real-time. When a user sends a new message while a response is streaming, the system must gracefully cancel the current stream and begin processing the new message. Currently, this interruption causes an error message to be displayed to the user because: + +1. `handleChat()` and `handleInterrupt()` both await the same `dispatchPromise` +2. When the abort signal triggers, the error thrown may not have `name === "AbortError"` +3. `handleChat()`'s catch block treats unrecognized errors as real errors and displays them + +The relevant code paths: +- `src/tui/app.js:904-927` — `handleChat()` catch block checks `err.name === "AbortError"` +- `src/tui/app.js:947-985` — `handleInterrupt()` aborts and awaits `dispatchPromise` +- `src/agent/react.js:263-276` — Abort signal check inside `for await` loop + +## Goals / Non-Goals + +**Goals:** +- Ensure intentional interruptions (user sends new message during streaming) never show an error to the user +- Recognize interruptions regardless of the error type thrown by the abort signal +- Preserve all existing cleanup behavior (tool-call removal, message clearing, flag reset) + +**Non-Goals:** +- Changing the streaming protocol or LLM provider integration +- Modifying session management or compaction logic +- Handling network errors or LLM API errors during streaming (those should still show errors) + +## Decisions + +### Decision 1: Use a shared ref flag instead of error type detection +**Choice:** Introduce `isIntentionalAbort` ref in `src/tui/app.js` +**Rationale:** Error types vary across Node.js environments (DOMException, custom LangChain errors, AbortError). Checking error names is fragile and environment-dependent. A shared ref is explicit, deterministic, and doesn't rely on error type matching. +**Alternatives considered:** +- Checking multiple error names (`"AbortError"`, `"Canceled"`, etc.) — fragile, may miss new error types +- Wrapping the abort in a custom error class — still requires error type propagation through async boundaries +- Using a Promise-based signaling mechanism — over-engineered for a single boolean state + +### Decision 2: Set the flag before aborting, not after +**Choice:** Set `isIntentionalAbort.current = true` before calling `abortController.abort()` +**Rationale:** Ensures the flag is set even if `abort()` throws (unlikely but possible). The flag is reset in `handleChat()`'s `finally` block, guaranteeing cleanup regardless of execution path. +**Alternatives considered:** +- Setting after abort — race condition if abort throws +- Using a separate signaling Promise — adds complexity without benefit + +### Decision 3: Add early abort check in react.js +**Choice:** Check `signal.aborted` at function entry in `callReactAgentStreaming()` and throw a named `AbortError` if already aborted +**Rationale:** Provides a second layer of defense — even if the ref flag isn't checked for some reason, the error thrown will have the correct name. This is a belt-and-suspenders approach. +**Alternatives considered:** +- Relying solely on the ref flag — sufficient but less defensive +- Checking only inside the for-await loop — misses errors thrown during stream initialization + +## Risks / Trade-offs + +[Risk] The ref flag approach adds state that must be carefully managed across async boundaries → [Mitigation] The flag is set in a synchronous code path (handleInterrupt) and checked in the catch block of the same async function (handleChat), minimizing race conditions. Reset in finally ensures cleanup. + +[Risk] Adding an early abort check in react.js could mask legitimate errors if the signal is aborted for unexpected reasons → [Mitigation] The ref flag provides the primary decision path; the named AbortError is secondary. The ref flag is only set by handleInterrupt(), which is the intentional abort path. + +[Risk] The change is tightly coupled to the current TUI architecture → [Mitigation] The fix is localized to two files and doesn't change the streaming protocol or provider interface. If the TUI architecture changes, the fix can be adapted without affecting other systems. \ No newline at end of file diff --git a/openspec/changes/graceful-streaming-interruption/proposal.md b/openspec/changes/graceful-streaming-interruption/proposal.md new file mode 100644 index 0000000..ce77f58 --- /dev/null +++ b/openspec/changes/graceful-streaming-interruption/proposal.md @@ -0,0 +1,24 @@ +## Why + +When a user sends a new message while the agent is streaming a response, an error message is displayed to the user instead of a clean "Interrupted." status. The root cause is a race condition between `handleChat()` and `handleInterrupt()` — both await the same `dispatchPromise`, and when the abort signal triggers, the error thrown doesn't always have `name === "AbortError"`, causing `handleChat()`'s catch block to treat it as a real error and display it to the user. + +## What Changes + +- Introduce a shared `isIntentionalAbort` ref in `src/tui/app.js` that `handleInterrupt()` sets to `true` before aborting, allowing `handleChat()`'s catch block to recognize intentional interruptions regardless of error type +- Add early abort signal check at function entry in `src/agent/react.js` to throw a named `AbortError` if the signal is already aborted when entering the streaming function +- Preserve all existing cleanup logic — the flag only affects error display, not cleanup behavior + +## Capabilities + +### New Capabilities +- `streaming-interruption`: Graceful handling of user-initiated streaming interruptions without error message display + +### Modified Capabilities + + +## Impact + +- `src/tui/app.js` — `handleChat()` and `handleInterrupt()` functions, new ref +- `src/agent/react.js` — `callReactAgentStreaming()` function, early abort check +- No API changes, no dependency changes +- No breaking changes \ No newline at end of file diff --git a/openspec/changes/graceful-streaming-interruption/specs/streaming-interruption/spec.md b/openspec/changes/graceful-streaming-interruption/specs/streaming-interruption/spec.md new file mode 100644 index 0000000..7701213 --- /dev/null +++ b/openspec/changes/graceful-streaming-interruption/specs/streaming-interruption/spec.md @@ -0,0 +1,44 @@ +## ADDED Requirements + +### Requirement: Intentional streaming interruptions SHALL not display error messages to the user + +When a user sends a new message while the agent is streaming a response, the system SHALL handle the interruption gracefully by displaying a clean "Interrupted." status message instead of an error message. + +#### Scenario: User interrupts streaming with new message +- **WHEN** the agent is streaming a response and the user sends a new message +- **THEN** the system SHALL abort the current stream and display "Interrupted." status +- **THEN** the system SHALL NOT display an error message to the user + +#### Scenario: Intentional interrupt preserves cleanup behavior +- **WHEN** the user interrupts streaming +- **THEN** the system SHALL remove any orphaned tool-call messages from session state +- **THEN** the system SHALL clear the partial streaming assistant message from the UI +- **THEN** the system SHALL reset the abort controller and streaming flags + +### Requirement: The system SHALL recognize intentional interruptions regardless of error type + +The system SHALL use an explicit intent signal to distinguish intentional interruptions from unexpected errors, rather than relying on error type detection. + +#### Scenario: Ref flag prevents error display for non-AbortError interruptions +- **WHEN** the abort signal triggers and throws an error with a non-standard name (e.g., DOMException) +- **THEN** the system SHALL check the intentional abort flag before deciding to display an error +- **THEN** the system SHALL suppress the error message if the flag indicates intentional interruption + +#### Scenario: Error messages are shown for non-intentional failures +- **WHEN** a streaming response fails due to a network error or LLM API error (not user-initiated) +- **THEN** the system SHALL display an error message to the user +- **THEN** the intentional abort flag SHALL be false, allowing the error path to execute + +### Requirement: The streaming function SHALL throw a named AbortError when the signal is already aborted + +When `callReactAgentStreaming()` is entered with an already-aborted signal, it SHALL throw an error with `name === "AbortError"` to provide a consistent error type for downstream handlers. + +#### Scenario: Early abort check throws named AbortError +- **WHEN** `callReactAgentStreaming()` is called with an already-aborted signal +- **THEN** the function SHALL throw an error with `name === "AbortError"` +- **THEN** the error SHALL include a descriptive message indicating the stream was interrupted + +#### Scenario: Normal abort signal check inside stream loop +- **WHEN** the abort signal is triggered during streaming (after stream initialization) +- **THEN** the function SHALL check `signal.aborted` at the start of each iteration +- **THEN** the function SHALL clean up any pending tool calls and return early \ No newline at end of file diff --git a/openspec/changes/graceful-streaming-interruption/tasks.md b/openspec/changes/graceful-streaming-interruption/tasks.md new file mode 100644 index 0000000..8c8d84f --- /dev/null +++ b/openspec/changes/graceful-streaming-interruption/tasks.md @@ -0,0 +1,31 @@ +## 1. Add intentional abort signaling to TUI + +- [ ] 1.1 Add `isIntentionalAbort` ref to `src/tui/app.js` alongside existing refs +- [ ] 1.2 Set `isIntentionalAbort.current = true` in `handleInterrupt()` before calling `abortController.abort()` +- [ ] 1.3 Reset `isIntentionalAbort.current = false` in `handleChat()`'s `finally` block + +## 2. Update handleChat() catch block to check abort intent + +- [ ] 2.1 Modify `handleChat()`'s catch block to check `isIntentionalAbort.current` before error type detection +- [ ] 2.2 If `isIntentionalAbort.current` is true, execute the clean interruption path regardless of error name +- [ ] 2.3 Preserve existing cleanup logic (tool-call removal, message clearing, flag reset) in the intentional abort path + +## 3. Add early abort check in streaming function + +- [ ] 3.1 Add early `signal.aborted` check at function entry in `callReactAgentStreaming()` in `src/agent/react.js` +- [ ] 3.2 Throw a named `AbortError` if the signal is already aborted when entering the function +- [ ] 3.3 Preserve existing abort signal check inside the `for await` loop + +## 4. Write tests + +- [ ] 4.1 Add test for intentional interrupt suppressing error message +- [ ] 4.2 Add test for non-intentional errors still showing error message +- [ ] 4.3 Add test for early abort check throwing named AbortError +- [ ] 4.4 Add test for cleanup behavior during intentional interrupt + +## 5. Verify and commit + +- [ ] 5.1 Run `npm run test` and verify all tests pass +- [ ] 5.2 Run `npm run lint` and verify no lint errors +- [ ] 5.3 Run `npm start` and verify application starts without crashing +- [ ] 5.4 Manually verify: send message, interrupt during streaming, confirm "Interrupted." status appears \ No newline at end of file diff --git a/src/agent/react.js b/src/agent/react.js index 0f27e1d..756f878 100644 --- a/src/agent/react.js +++ b/src/agent/react.js @@ -191,6 +191,15 @@ async function callReactAgentStreaming( ...(recursionLimit !== null && { recursionLimit }), }; + // Early abort check — if the signal is already aborted when entering, + // throw a named AbortError immediately. This ensures the TUI catch block + // recognizes the error as a clean interruption regardless of environment. + if (signal?.aborted) { + const abortError = new Error("Stream interrupted before start"); + abortError.name = "AbortError"; + throw abortError; + } + // If an abort signal is provided, listen for it and break the stream loop if (signal) { signal.throwIfAborted(); diff --git a/src/tui/app.js b/src/tui/app.js index d19961e..968e6b2 100644 --- a/src/tui/app.js +++ b/src/tui/app.js @@ -48,6 +48,7 @@ export default function App({ const dispatchPromiseRef = useRef(null); const autoContinueCountRef = useRef(0); const isAutoContinuingRef = useRef(false); + const isIntentionalAbortRef = useRef(false); const { exit } = useApp(); const exitRef = useRef(exit); exitRef.current = exit; @@ -902,16 +903,20 @@ export default function App({ gcManager?.(); setStatusMessage("Received response"); } catch (err) { - // Abort is a normal interruption, not an error - if (err.name === "AbortError") { - // Clean up: remove the assistant's tool-call message (if any) and the user message. - // The assistant's tool-call message is removed first to prevent orphaned tool_calls - // from corrupting the conversation history sent to the LLM API on resume. + // Intentional abort from handleInterrupt() — clean interruption, not an error + if (isIntentionalAbortRef.current) { + if (sessionState) { + sessionState.removeLastAssistantToolCallMessage(); + sessionState.popExchange(); + } + setMessages((prev) => prev.filter((msg) => !isStreamingMessage(msg))); + setStatusMessage("Interrupted."); + } else if (err.name === "AbortError") { + // Abort is a normal interruption, not an error if (sessionState) { sessionState.removeLastAssistantToolCallMessage(); sessionState.popExchange(); } - // Clear the partial streaming assistant message from UI setMessages((prev) => prev.filter((msg) => !isStreamingMessage(msg))); setStatusMessage("Interrupted."); } else { @@ -926,9 +931,10 @@ export default function App({ }); } } finally { - // Reset abort controller and streaming flag + // Reset abort controller, streaming flag, and intentional abort signal abortControllerRef.current = null; isStreamingRef.current = false; + isIntentionalAbortRef.current = false; } gcManager?.(); }; @@ -945,6 +951,9 @@ export default function App({ * processing the abort and completed its cleanup. */ const handleInterrupt = async () => { + // Signal to handleChat() that this is an intentional interruption + // so it doesn't display an error message regardless of error type + isIntentionalAbortRef.current = true; if (abortControllerRef.current) { abortControllerRef.current.abort(); abortControllerRef.current = null; diff --git a/tests/unit/react_agent.test.js b/tests/unit/react_agent.test.js index a7172d5..2a27691 100644 --- a/tests/unit/react_agent.test.js +++ b/tests/unit/react_agent.test.js @@ -634,6 +634,48 @@ describe("callReactAgent", () => { assert.strictEqual(err.name, "AbortError"); }); + it("throws AbortError with descriptive message when signal is already aborted", async () => { + const controller = new AbortController(); + controller.abort(); + + const agentMock = createMock([]); + + let err = null; + try { + await callReactAgent(agentMock, "hello", null, null, () => {}, { + signal: controller.signal, + }); + } catch (e) { + err = e; + } + + assert.ok(err instanceof Error); + assert.strictEqual(err.name, "AbortError"); + assert.ok( + err.message.toLowerCase().includes("interrupt"), + "error message should indicate interruption", + ); + }); + + it("does not throw when signal is not aborted", async () => { + const controller = new AbortController(); + // Signal is NOT aborted + + const agentMock = createMock([]); + + let err = null; + try { + await callReactAgent(agentMock, "hello", null, null, () => {}, { + signal: controller.signal, + }); + } catch (e) { + err = e; + } + + // Should not throw AbortError — empty stream returns original message + assert.notStrictEqual(err?.name, "AbortError"); + }); + it("emits tool_end for pending tools on abort", async () => { const events = [ {