diff --git a/extensions/cli/src/services/ChatHistoryService.ts b/extensions/cli/src/services/ChatHistoryService.ts index b4a1c56a72f..5fdf18a75dc 100644 --- a/extensions/cli/src/services/ChatHistoryService.ts +++ b/extensions/cli/src/services/ChatHistoryService.ts @@ -2,6 +2,7 @@ import type { ChatHistoryItem, ToolStatus } from "core/index.js"; import { createHistoryItem } from "core/util/messageConversion.js"; import { loadSessionById, updateSessionHistory } from "../session.js"; +import type { ChatHistoryItemWithType } from "../uiNotices.js"; import { logger } from "../util/logger.js"; import { BaseService } from "./BaseService.js"; @@ -205,13 +206,23 @@ export class ChatHistoryService extends BaseService { } /** - * Add a system message to the history + * Add a system message to the history. + * + * Everything that reaches this method is a notice addressed to the user — + * "Remote environment is shutting down…", `WARNING: Tool … requires + * permission`, command output, diffs — so the item is marked as a UI notice + * and is dropped before the history is converted to wire format. Callers that + * ever need to add a real system *instruction* should build the item + * themselves rather than reach for this. */ addSystemMessage(content: string): ChatHistoryItem { - const newMessage = createHistoryItem({ - role: "system", - content, - }); + const newMessage: ChatHistoryItemWithType = { + ...createHistoryItem({ + role: "system", + content, + }), + messageType: "system", + }; const newHistory = [...this.currentState.history, newMessage]; this.setHistoryInternal(newHistory); diff --git a/extensions/cli/src/stream/streamChatResponse.autoCompaction.ts b/extensions/cli/src/stream/streamChatResponse.autoCompaction.ts index 156e9fa3526..bd3e8cf0ef4 100644 --- a/extensions/cli/src/stream/streamChatResponse.autoCompaction.ts +++ b/extensions/cli/src/stream/streamChatResponse.autoCompaction.ts @@ -10,6 +10,7 @@ import { shouldAutoCompact, } from "../compaction.js"; import { updateSessionHistory } from "../session.js"; +import { uiNotice } from "../uiNotices.js"; import { formatError } from "../util/formatError.js"; import { logger } from "../util/logger.js"; @@ -74,13 +75,7 @@ function handleCompactionSuccess( callbacks.setCompactionIndex(result.compactionIndex); callbacks.setMessages((prev: ChatHistoryItem[]) => [ ...prev, - { - message: { - role: "system", - content: successMessage, - }, - contextItems: [], - }, + uiNotice(successMessage), ]); } } @@ -105,13 +100,7 @@ function handleCompactionError( } else if (callbacks?.setMessages) { callbacks.setMessages((prev: ChatHistoryItem[]) => [ ...prev, - { - message: { - role: "system", - content: warningMessage, - }, - contextItems: [], - }, + uiNotice(warningMessage), ]); } } diff --git a/extensions/cli/src/stream/streamChatResponse.systemMessage.test.ts b/extensions/cli/src/stream/streamChatResponse.systemMessage.test.ts index cd20f734c8c..9cb4d26dc54 100644 --- a/extensions/cli/src/stream/streamChatResponse.systemMessage.test.ts +++ b/extensions/cli/src/stream/streamChatResponse.systemMessage.test.ts @@ -4,6 +4,7 @@ import type { ChatHistoryItem } from "core/index.js"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { services } from "../services/index.js"; +import { uiNotice } from "../uiNotices.js"; import { processStreamingResponse } from "./streamChatResponse.js"; @@ -160,6 +161,112 @@ describe("streamChatResponse system message validation", () => { expect(services.systemMessage.getSystemMessage).not.toHaveBeenCalled(); }); + it("should not send UI notices to the provider as extra system messages", async () => { + const model = { + model: "test-model", + defaultCompletionOptions: { + contextLength: 1000, + maxTokens: 100, + }, + } as ModelConfig; + + const systemMessage = "System instructions"; + + // A `/model` notice recorded in the history. It is role:"system" for + // rendering, and carries the messageType discriminator from + // spec/wire-format.md marking it as display-only. + const chatHistory: ChatHistoryItem[] = [ + { message: { role: "user", content: "hello" }, contextItems: [] }, + { + message: { role: "system", content: "Switched to model: gpt-4o" }, + contextItems: [], + messageType: "system", + } as ChatHistoryItem, + { message: { role: "user", content: "and now?" }, contextItems: [] }, + ]; + + const mockStream = { + [Symbol.asyncIterator]: async function* () { + yield { choices: [{ delta: { content: "ok" } }] }; + }, + }; + mockLlmApi.chatCompletionStream = vi.fn().mockResolvedValue(mockStream); + + await processStreamingResponse({ + chatHistory, + model, + llmApi: mockLlmApi, + abortController, + systemMessage, + }); + + const sent = vi.mocked(mockLlmApi.chatCompletionStream).mock.calls[0]?.[0]; + const systemMessages = sent!.messages.filter((m) => m.role === "system"); + + // Exactly one system message, and it is first — otherwise providers reject + // the request with "System message must be at the beginning". + expect(systemMessages).toHaveLength(1); + expect(sent!.messages[0]?.role).toBe("system"); + expect(sent!.messages[0]?.content).toBe(systemMessage); + expect( + sent!.messages.some((m) => + typeof m.content === "string" + ? m.content.includes("Switched to model") + : false, + ), + ).toBe(false); + }); + + it("should not send notices from the non-selector sites either", async () => { + const model = { + model: "test-model", + defaultCompletionOptions: { contextLength: 1000, maxTokens: 100 }, + } as ModelConfig; + + const systemMessage = "System instructions"; + + // The shape reported in #13026: an error banner sitting next to a /model + // notice. The banner comes from useChat.ts, a site that does not go through + // the selector hooks, so it is only filtered because every notice site now + // builds its item with uiNotice(). + const chatHistory: ChatHistoryItem[] = [ + { message: { role: "user", content: "hello" }, contextItems: [] }, + uiNotice("Switched to model: qwen/qwen3.6-35b-a3b") as ChatHistoryItem, + uiNotice("Error: Connection error.") as ChatHistoryItem, + uiNotice( + "[Tool canceled - please tell Continue what to do differently]", + ) as ChatHistoryItem, + uiNotice("Chat history compacted successfully.") as ChatHistoryItem, + uiNotice("Session exported to /tmp/x.json") as ChatHistoryItem, + { message: { role: "user", content: "and now?" }, contextItems: [] }, + ]; + + const mockStream = { + [Symbol.asyncIterator]: async function* () { + yield { choices: [{ delta: { content: "ok" } }] }; + }, + }; + mockLlmApi.chatCompletionStream = vi.fn().mockResolvedValue(mockStream); + + await processStreamingResponse({ + chatHistory, + model, + llmApi: mockLlmApi, + abortController, + systemMessage, + }); + + const sent = vi.mocked(mockLlmApi.chatCompletionStream).mock.calls[0]?.[0]; + + expect(sent!.messages.filter((m) => m.role === "system")).toHaveLength(1); + expect(sent!.messages[0]?.content).toBe(systemMessage); + expect(sent!.messages.map((m) => m.content)).toEqual([ + systemMessage, + "hello", + "and now?", + ]); + }); + it("should use safety buffer in validation", async () => { const model = { model: "test-model", diff --git a/extensions/cli/src/stream/streamChatResponse.ts b/extensions/cli/src/stream/streamChatResponse.ts index 20156663381..13e264eca49 100644 --- a/extensions/cli/src/stream/streamChatResponse.ts +++ b/extensions/cli/src/stream/streamChatResponse.ts @@ -13,6 +13,7 @@ import { services } from "../services/index.js"; import { telemetryService } from "../telemetry/telemetryService.js"; import { applyChatCompletionToolOverrides } from "../tools/applyToolOverrides.js"; import { ToolCall } from "../tools/index.js"; +import { withoutUiNotices } from "../uiNotices.js"; import { chatCompletionStreamWithBackoff, isContextLengthError, @@ -249,9 +250,12 @@ export async function processStreamingResponse( throw new Error(`Context length validation failed: ${validation.error}`); } - // Create OpenAI format history with validated system message + // Create OpenAI format history with validated system message. UI notices are + // dropped first: they are `role: "system"` items the TUI added for display + // only, and sending them would put a second system message at a non-zero + // index, which providers reject. const openaiChatHistory = convertFromUnifiedHistoryWithSystemMessage( - chatHistory, + withoutUiNotices(chatHistory), systemMessage, ) as ChatCompletionMessageParam[]; const requestStartTime = Date.now(); diff --git a/extensions/cli/src/ui/TUIChat.tsx b/extensions/cli/src/ui/TUIChat.tsx index 6eaca42cc34..20c6fbdad75 100644 --- a/extensions/cli/src/ui/TUIChat.tsx +++ b/extensions/cli/src/ui/TUIChat.tsx @@ -19,6 +19,7 @@ import { UpdateServiceState, } from "../services/types.js"; import { getTotalSessionCost } from "../session.js"; +import { uiNotice } from "../uiNotices.js"; import { bashToolEvents } from "../util/cli.js"; import { logger } from "../util/logger.js"; @@ -302,13 +303,7 @@ const TUIChat: React.FC = ({ if (!session) { setChatHistory((prev) => [ ...prev, - { - message: { - role: "system", - content: `Failed to export: Session ${sessionId} not found`, - }, - contextItems: [], - }, + uiNotice(`Failed to export: Session ${sessionId} not found`), ]); closeCurrentScreen(); return; @@ -329,25 +324,13 @@ const TUIChat: React.FC = ({ setChatHistory((prev) => [ ...prev, - { - message: { - role: "system", - content: `Session exported to ${defaultPath}`, - }, - contextItems: [], - }, + uiNotice(`Session exported to ${defaultPath}`), ]); closeCurrentScreen(); } catch (error: any) { setChatHistory((prev) => [ ...prev, - { - message: { - role: "system", - content: `Failed to export session: ${error.message}`, - }, - contextItems: [], - }, + uiNotice(`Failed to export session: ${error.message}`), ]); closeCurrentScreen(); } diff --git a/extensions/cli/src/ui/hooks/useChat.clear.test.ts b/extensions/cli/src/ui/hooks/useChat.clear.test.ts index 7056a62b63c..f86ea42b9e7 100644 --- a/extensions/cli/src/ui/hooks/useChat.clear.test.ts +++ b/extensions/cli/src/ui/hooks/useChat.clear.test.ts @@ -59,7 +59,8 @@ describe("useChat clear command", () => { chatHistory[0], // The system message ChatHistoryItem ]); - // Second call should be with the "Chat history cleared" message + // Second call should be with the "Chat history cleared" message, marked as + // a UI notice so it is shown to the user but not sent to the model. expect(mockSetChatHistory).toHaveBeenLastCalledWith([ { message: { @@ -67,6 +68,7 @@ describe("useChat clear command", () => { content: "Chat history cleared", }, contextItems: [], + messageType: "system", }, ]); }); diff --git a/extensions/cli/src/ui/hooks/useChat.compaction.ts b/extensions/cli/src/ui/hooks/useChat.compaction.ts index 1df84cfc3a1..306048ee6f8 100644 --- a/extensions/cli/src/ui/hooks/useChat.compaction.ts +++ b/extensions/cli/src/ui/hooks/useChat.compaction.ts @@ -3,6 +3,7 @@ import type { ChatHistoryItem, Session } from "core/index.js"; import { compactChatHistory } from "../../compaction.js"; import { updateSessionHistory } from "../../session.js"; import { handleAutoCompaction as coreHandleAutoCompaction } from "../../stream/streamChatResponse.autoCompaction.js"; +import { uiNotice } from "../../uiNotices.js"; import { formatError } from "../../util/formatError.js"; import { logger } from "../../util/logger.js"; @@ -68,39 +69,18 @@ export async function handleCompactCommand({ // Add success message to chat setChatHistory((prev) => [ ...prev, - { - message: { - role: "system", - content: "Chat history compacted successfully.", - }, - contextItems: [], - }, + uiNotice("Chat history compacted successfully."), ]); } catch (error) { // Check if the error was due to abortion if (compactionController.signal.aborted) { logger.info("Manual compaction was cancelled by user"); - setChatHistory((prev) => [ - ...prev, - { - message: { - role: "system", - content: "Compaction cancelled.", - }, - contextItems: [], - }, - ]); + setChatHistory((prev) => [...prev, uiNotice("Compaction cancelled.")]); } else { logger.error("Compaction failed:", error); setChatHistory((prev) => [ ...prev, - { - message: { - role: "system", - content: `Compaction failed: ${formatError(error)}`, - }, - contextItems: [], - }, + uiNotice(`Compaction failed: ${formatError(error)}`), ]); } } finally { diff --git a/extensions/cli/src/ui/hooks/useChat.helpers.ts b/extensions/cli/src/ui/hooks/useChat.helpers.ts index a90a4d6b4fa..a178af79dc4 100644 --- a/extensions/cli/src/ui/hooks/useChat.helpers.ts +++ b/extensions/cli/src/ui/hooks/useChat.helpers.ts @@ -9,6 +9,7 @@ import { logger } from "src/util/logger.js"; import { DEFAULT_SESSION_TITLE } from "../../constants/session.js"; import { loadSession, startNewSession } from "../../session.js"; import { telemetryService } from "../../telemetry/telemetryService.js"; +import { uiNotice } from "../../uiNotices.js"; import { processImagePlaceholder } from "./useChat.imageProcessing.js"; import { SlashCommandResult } from "./useChat.types.js"; @@ -124,15 +125,7 @@ export function processSlashCommandResult({ } if (result.output) { - setChatHistory([ - { - message: { - role: "system", - content: result.output, - }, - contextItems: [], - }, - ]); + setChatHistory([uiNotice(result.output)]); } return null; } @@ -140,26 +133,11 @@ export function processSlashCommandResult({ if (result.diffContent) { setChatHistory((prev) => [ ...prev, - { - message: { - role: "system", - content: `Diff:\n${result.diffContent}`, - }, - contextItems: [], - }, + uiNotice(`Diff:\n${result.diffContent}`), ]); return null; } else if (result.output) { - setChatHistory((prev) => [ - ...prev, - { - message: { - role: "system", - content: result.output || "", - }, - contextItems: [], - }, - ]); + setChatHistory((prev) => [...prev, uiNotice(result.output || "")]); return null; } diff --git a/extensions/cli/src/ui/hooks/useChat.stream.helpers.ts b/extensions/cli/src/ui/hooks/useChat.stream.helpers.ts index a11550ccfb2..538849865bc 100644 --- a/extensions/cli/src/ui/hooks/useChat.stream.helpers.ts +++ b/extensions/cli/src/ui/hooks/useChat.stream.helpers.ts @@ -5,6 +5,7 @@ import { logger } from "src/util/logger.js"; import { services } from "../../services/index.js"; import { getCurrentSession, updateSessionTitle } from "../../session.js"; +import { uiNotice } from "../../uiNotices.js"; import { generateSessionTitle } from "./useChat.helpers.js"; @@ -185,13 +186,7 @@ export function createStreamCallbacks( } } else { // General error message - newHistory.push({ - message: { - role: "system", - content: error, - }, - contextItems: [], - }); + newHistory.push(uiNotice(error)); } return newHistory; @@ -225,17 +220,9 @@ export function createStreamCallbacks( return; } } catch {} - // Fallback to local state if service unavailable - setChatHistory((prev) => [ - ...prev, - { - message: { - role: "system", - content: message, - }, - contextItems: [], - }, - ]); + // Fallback to local state if service unavailable. Marked the same way + // ChatHistoryService.addSystemMessage does, so the two paths agree. + setChatHistory((prev) => [...prev, uiNotice(message)]); }, }; } diff --git a/extensions/cli/src/ui/hooks/useChat.ts b/extensions/cli/src/ui/hooks/useChat.ts index 4bd2bc5c5df..c9339afa306 100644 --- a/extensions/cli/src/ui/hooks/useChat.ts +++ b/extensions/cli/src/ui/hooks/useChat.ts @@ -15,6 +15,7 @@ import { import { handleSlashCommands } from "../../slashCommands.js"; import { messageQueue, QueuedMessage } from "../../stream/messageQueue.js"; import { telemetryService } from "../../telemetry/telemetryService.js"; +import { uiNotice } from "../../uiNotices.js"; import { formatError } from "../../util/formatError.js"; import { logger } from "../../util/logger.js"; @@ -342,16 +343,7 @@ export function useChat({ }); } catch (error: any) { const errorMessage = `Error: ${formatError(error)}`; - setChatHistory((prev) => [ - ...prev, - { - message: { - role: "system", - content: errorMessage, - }, - contextItems: [], - }, - ]); + setChatHistory((prev) => [...prev, uiNotice(errorMessage)]); } finally { // Stop active time tracking telemetryService.stopActiveTime(); @@ -834,16 +826,14 @@ export function useChat({ // If this is a "stop stream" rejection, abort the current request if (stopStream && abortController && isWaitingForResponse) { abortController.abort(); + // Addressed to the user ("please tell Continue …"), not an instruction + // to the model, so it stays out of the wire format. This is the notice + // #12963 reported. setChatHistory((prev) => [ ...prev, - { - message: { - role: "system", - content: - "[Tool canceled - please tell Continue what to do differently]", - }, - contextItems: [], - }, + uiNotice( + "[Tool canceled - please tell Continue what to do differently]", + ), ]); setIsWaitingForResponse(false); setResponseStartTime(null); diff --git a/extensions/cli/src/ui/hooks/useTUIChatHooks.ts b/extensions/cli/src/ui/hooks/useTUIChatHooks.ts index ff63b8e93e5..7fed74588b0 100644 --- a/extensions/cli/src/ui/hooks/useTUIChatHooks.ts +++ b/extensions/cli/src/ui/hooks/useTUIChatHooks.ts @@ -133,12 +133,14 @@ export function useSelectors( const { handleConfigSelect } = useConfigSelector({ configPath, onMessage: (message) => { - // Convert message to ChatHistoryItem format + // Convert message to ChatHistoryItem format, keeping the messageType the + // hook set so the notice is not mistaken for a system instruction. setChatHistory((prev) => [ ...prev, { message: { role: message.role, content: message.content }, contextItems: [], + messageType: message.messageType, }, ]); }, @@ -147,12 +149,14 @@ export function useSelectors( const { handleModelSelect } = useModelSelector({ onMessage: (message) => { - // Convert message to ChatHistoryItem format + // Convert message to ChatHistoryItem format, keeping the messageType the + // hook set so the notice is not mistaken for a system instruction. setChatHistory((prev) => [ ...prev, { message: { role: "system", content: message.content }, contextItems: [], + messageType: message.messageType, }, ]); }, diff --git a/extensions/cli/src/uiNotices.test.ts b/extensions/cli/src/uiNotices.test.ts new file mode 100644 index 00000000000..015289f61ae --- /dev/null +++ b/extensions/cli/src/uiNotices.test.ts @@ -0,0 +1,116 @@ +import type { ChatHistoryItem } from "core/index.js"; +import { convertFromUnifiedHistoryWithSystemMessage } from "core/util/messageConversion.js"; + +import { + type ChatHistoryItemWithType, + isUiNotice, + uiNotice, + withoutUiNotices, +} from "./uiNotices.js"; + +function item( + role: "user" | "assistant" | "system", + content: string, + messageType?: "system", +): ChatHistoryItem { + const historyItem: ChatHistoryItemWithType = { + message: { role, content }, + contextItems: [], + messageType, + }; + return historyItem; +} + +describe("isUiNotice", () => { + it("identifies a notice by messageType, not by role", () => { + expect( + isUiNotice(item("system", "Switched to model: gpt-4o", "system")), + ).toBe(true); + }); + + it("does not treat a real system message as a notice", () => { + // Same role, no discriminator — this is an instruction for the model. + expect(isUiNotice(item("system", "You are a helpful assistant."))).toBe( + false, + ); + }); + + it("does not treat user or assistant messages as notices", () => { + expect(isUiNotice(item("user", "hello"))).toBe(false); + expect(isUiNotice(item("assistant", "hi"))).toBe(false); + }); +}); + +describe("withoutUiNotices", () => { + it("drops notices and keeps everything else in order", () => { + const history = [ + item("user", "hello"), + item("system", "Switched to model: gpt-4o", "system"), + item("assistant", "hi"), + item("system", "Failed to switch model: boom", "system"), + item("user", "still there?"), + ]; + + const result = withoutUiNotices(history); + + expect(result.map((i) => i.message.content)).toEqual([ + "hello", + "hi", + "still there?", + ]); + }); + + it("keeps a real system message", () => { + const history = [item("system", "You are a helpful assistant.")]; + expect(withoutUiNotices(history)).toHaveLength(1); + }); + + it("leaves a history with no notices untouched", () => { + const history = [item("user", "hello"), item("assistant", "hi")]; + expect(withoutUiNotices(history)).toEqual(history); + }); +}); + +describe("uiNotice", () => { + it("produces an item that isUiNotice recognises and withoutUiNotices drops", () => { + const notice = uiNotice("Error: Connection error."); + + expect(notice.message).toEqual({ + role: "system", + content: "Error: Connection error.", + }); + expect(notice.messageType).toBe("system"); + expect(isUiNotice(notice)).toBe(true); + expect(withoutUiNotices([notice])).toHaveLength(0); + }); +}); + +describe("conversion after filtering", () => { + it("no longer emits a system message at a non-zero index", () => { + // Reproduces the shape that made providers return + // "System message must be at the beginning": a /model notice recorded in the + // history becomes a second role:"system" entry after the user turn. + const history = [ + item("user", "hello"), + item("system", "Switched to model: gpt-4o", "system"), + item("user", "and now?"), + ]; + + const unfiltered = convertFromUnifiedHistoryWithSystemMessage( + history, + "real system prompt", + ); + const offendingIndexes = unfiltered + .map((m, i) => (m.role === "system" ? i : -1)) + .filter((i) => i > 0); + expect(offendingIndexes.length).toBeGreaterThan(0); + + const filtered = convertFromUnifiedHistoryWithSystemMessage( + withoutUiNotices(history), + "real system prompt", + ); + expect(filtered.filter((m) => m.role === "system")).toHaveLength(1); + expect(filtered[0]?.role).toBe("system"); + expect(filtered[0]?.content).toBe("real system prompt"); + }); +}); diff --git a/extensions/cli/src/uiNotices.ts b/extensions/cli/src/uiNotices.ts new file mode 100644 index 00000000000..abe446ed7be --- /dev/null +++ b/extensions/cli/src/uiNotices.ts @@ -0,0 +1,69 @@ +import type { ChatHistoryItem } from "core/index.js"; + +/** + * Messages the TUI appends to the history purely to tell the user something + * happened — "Switched to model: …", "Failed to switch model: …", config + * notices. They are given `role: "system"` because that is what the renderer + * keys off, but they are not system instructions for the model. + * + * `spec/wire-format.md` already defines a `messageType` discriminator for + * exactly this, and the selector hooks already set `messageType: "system"`. + * Nothing read it, so once a notice was in the history it was indistinguishable + * from a real system message, and the conversion to wire format sent it to the + * provider as a second `role: "system"` at a non-zero index — which providers + * reject with "System message must be at the beginning". + */ +export type UiMessageType = + | "tool-start" + | "tool-result" + | "tool-error" + | "system"; + +/** + * A history item that carries the `messageType` discriminator from + * `spec/wire-format.md`. The field is CLI-local: `ChatHistoryItem` is a core + * type shared with the extensions, and this classification only means anything + * to the TUI. + */ +export type ChatHistoryItemWithType = ChatHistoryItem & { + messageType?: UiMessageType; +}; + +/** + * True when the item is a UI notice rather than part of the conversation. + * + * Only `messageType` is consulted, never `role`: a real system message and a + * notice both use `role: "system"`, which is precisely why the discriminator + * exists. + */ +export function isUiNotice(item: ChatHistoryItem): boolean { + return (item as ChatHistoryItemWithType).messageType === "system"; +} + +/** + * Drop UI notices before the history is converted to wire format, so they stay + * visible in the transcript but are never sent to the model. + */ +export function withoutUiNotices( + history: ChatHistoryItem[], +): ChatHistoryItem[] { + return history.filter((item) => !isUiNotice(item)); +} + +/** + * Builds a history item for a message shown to the user but not sent to the + * model — a model/config switch, an error banner, command output, a compaction + * or export result. + * + * Every notice site should go through this rather than writing the object + * literal, so that adding one cannot accidentally omit the discriminator: an + * omission is silent, and the item then reaches the provider as a second + * `role: "system"` message. + */ +export function uiNotice(content: string): ChatHistoryItemWithType { + return { + message: { role: "system", content }, + contextItems: [], + messageType: "system", + }; +}