Skip to content

fix(cli): stop sending UI notices to the provider as system messages - #13046

Open
LHMQ878 wants to merge 3 commits into
continuedev:mainfrom
LHMQ878:fix/cli-wire-up-messageType
Open

fix(cli): stop sending UI notices to the provider as system messages#13046
LHMQ878 wants to merge 3 commits into
continuedev:mainfrom
LHMQ878:fix/cli-wire-up-messageType

Conversation

@LHMQ878

@LHMQ878 LHMQ878 commented Jul 28, 2026

Copy link
Copy Markdown

Closes part of #13026 — the classification half. Filed separately from #12988 as discussed on the issue: #12988 rewrites convertFromUnifiedHistoryWithSystemMessage in core/util/messageConversion.ts, and this PR doesn't touch that file at all, so the two don't overlap in the diff.

The dead field

extensions/cli/spec/wire-format.md:28 defines a discriminator:

"messageType": "tool-start" | "tool-result" | "tool-error" | "system",

and :100 describes classifying tool messages by it. The selector hooks already set it — useModelSelector.ts:65,81 and useConfigSelector.ts:61,67,78,97,104, all messageType: "system". But there are zero reads of messageType anywhere under extensions/cli/src. It's declared in the spec and unimplemented.

Why that causes the 400

useTUIChatHooks.ts receives the field and then drops it when building the history item:

onMessage: (message) => {
  setChatHistory((prev) => [
    ...prev,
    {
      message: { role: "system", content: message.content },  // messageType discarded
      contextItems: [],
    },
  ]);
},

So a /model notice becomes an ordinary role: "system" history entry. Once it's in the history nothing can tell it apart from a real system instruction — which is also why the renderer has to fall back to message.role === "system" (MemoizedMessage.tsx:54). On the next turn the conversion emits it as a second role: "system" message at a non-zero index, and providers reject that with "System message must be at the beginning".

The change

Wire the discriminator up rather than delete it:

  • useTUIChatHooks.ts — keep messageType on the history item instead of discarding it.
  • new extensions/cli/src/uiNotices.tsisUiNotice / withoutUiNotices, reading only messageType, never role. A real system message and a notice both use role: "system"; that's precisely why the discriminator exists.
  • streamChatResponse.ts:253 — filter notices out immediately before the conversion, so they stay visible in the transcript but are never sent to the model.

messageType is typed CLI-locally (ChatHistoryItemWithType) rather than added to core's ChatHistoryItem, since the classification only means anything to the TUI.

Verification

Control experiment — reverting only streamChatResponse.ts (the consumer wiring) and re-running:

× should not send UI notices to the provider as extra system messages
  → expected [ { role: 'system', …(1) }, …(1) ] to have a length of 1 but got 2

Two system messages — the exact shape that produces the 400. With the change: one, at index 0.

Tests. The end-to-end one is in the existing streamChatResponse.systemMessage.test.ts, asserting on what actually reaches chatCompletionStream rather than on the helper in isolation — a notice in the history must not appear in the request and must not add a second system message. Plus 7 unit tests in uiNotices.test.ts covering the role-vs-messageType distinction (a real role: "system" message is not filtered), ordering, and the no-notices case.

Suite, against a clean-main baseline (Windows, so there's pre-existing flake here):

baseline on main with this change
extensions/cli full suite 1667 passed, 5 failed / 3 files 1678 passed, 2 failed / 1 file

The 2 remaining failures are in TUIChat.editMessage.test.tsx, which fails on main too in a full parallel run and passes 27/27 when run on its own, on both branches — it's order-dependent, not related to this change. No new failures.

  • tsc --noEmit -p extensions/cli: 43 errors, identical count to baseline, none in the files touched here (all pre-existing core/ moduleResolution errors).
  • prettier --write applied to all changed files.

Note on ordering

If #12988 lands first this rebases cleanly; if this lands first, nothing in #12988 has to change. Worth mentioning that #12988 appears stuck on infrastructure rather than review — its own tests are green (27 success / 2 skipped, only the require-all-checks-to-pass aggregator red), and what's blocking it is 12 Continuous AI: * statuses that exist on that PR and on no other (#13004, #13005, #13037, #13042 all carry only the two snyk contexts).

`spec/wire-format.md:28` defines a `messageType` discriminator
(`"tool-start" | "tool-result" | "tool-error" | "system"`), and the selector
hooks already set `messageType: "system"` on the notices they emit — "Switched
to model: …", "Failed to switch model: …", config notices. Nothing ever read
the field: there were two assignment sites and zero reads under
`extensions/cli/src`.

`useTUIChatHooks` then dropped it when building the history item, so a notice
became an ordinary `role: "system"` entry. Once in the history it was
indistinguishable from a real system instruction, and the conversion to wire
format sent it to the provider as a second system message at a non-zero index,
which providers reject with "System message must be at the beginning".

Wire the discriminator up instead of removing it:

- keep `messageType` when the notice is converted to a history item
- filter notices out in the CLI right before the history is converted, so they
  stay visible in the transcript but are never sent to the model

The filter reads only `messageType`, never `role` — a real system message and a
notice both use `role: "system"`, which is exactly why the discriminator
exists.

Scoped to `extensions/cli`; `core/util/messageConversion.ts` is untouched.

Refs continuedev#13026
The eslint import/order rule requires ../uiNotices.js to come before
../util/exponentialBackoff.js. Fixes the lint failure on this PR.
@LHMQ878

LHMQ878 commented Jul 28, 2026

Copy link
Copy Markdown
Author

Fixed the lint failure

lint was red because of this PR, not a pre-existing issue:

extensions/cli/src/stream/streamChatResponse.ts
  23:1  error  `../uiNotices.js` import should occur before import of `../util/exponentialBackoff.js`  import/order

The new withoutUiNotices import was appended after the ../util/* group; import/order wants it in alphabetical position (../tools/ < ../uiNotices.js < ../util/). Moved it up one group in ef91405 — no behaviour change, imports only.

The two complexity warnings in the same output (streamChatResponse.ts:197, subagent/executor.ts:57) are unused-eslint-disable directives that exist on main and are warnings, not errors — left alone.

jetbrains-tests is a pre-existing failure unrelated to this PR (Autocomplete > testAutocomplete() at Autocomplete.kt:42, failing across unrelated branches including dependency bumps), and require-all-checks-to-pass is its aggregator.

@LHMQ878

LHMQ878 commented Jul 28, 2026

Copy link
Copy Markdown
Author

CI after ef91405

lint is now ✅. The three remaining red checks are all environmental or pre-existing — none is caused by this PR:

Check Cause
e2e/.../GUI.test.js (e2e:ci:run) npm postinstall failure, not a test failure: @vscode/ripgrep postinstall got 403 fetching microsoft/ripgrep-prebuilt release v13.0.0-10 (GitHub API rate limit). This job passed on the previous head of this same PR, so it is flaky infra — a re-run should clear it.
jetbrains-tests Pre-existing: Autocomplete > testAutocomplete() FAILEDAssertionFailedError at Autocomplete.kt:42. Fails across unrelated branches including dependency bumps; this PR touches no autocomplete code.
require-all-checks-to-pass Aggregator of the two above; nothing of its own.

Everything else is green, including core-checks, prettier-check, all packages-checks, and all 12 test jobs across ubuntu/macos/windows × Node 18/20/22/24.

Happy to re-run the GUI e2e job if a maintainer can trigger it — I can't from a fork.

The messageType discriminator was only set by the two selector hooks, so
withoutUiNotices caught 7 of the 23 notice sites and the other 16 still
reached the provider as system messages at a non-zero index. Reported on
continuedev#13026: the error banner from useChat.ts and the tool-cancel notice
(continuedev#12963's trigger) both survived the filter.

Adds a uiNotice() constructor and routes every notice site through it, so
adding a site cannot silently omit the discriminator. ChatHistoryService
.addSystemMessage marks at the sink, covering its 15 callers -- all of
which pass user-facing notices, and none of which could annotate the item
themselves since createHistoryItem builds it internally.

The two genuine system messages are left alone: serve.ts seeding an empty
history with the configured prompt, and autoCompaction.ts building a
throwaway item to count tokens.
@LHMQ878

LHMQ878 commented Jul 29, 2026

Copy link
Copy Markdown
Author

Pushed 7728d35 — covers the remaining notice sites

@garymarkowitz-source pointed out on #13026 that this PR as originally scoped only filtered the two selector hooks, so the error banner from his repro and #12963's tool-cancel notice both still reached the provider. That was correct. This push closes the gap.

What changed. A uiNotice(content) constructor in uiNotices.ts builds the history item with the discriminator set, and every notice site now goes through it instead of writing the object literal. That way adding a site can't silently omit the marker — an omission is invisible until a provider rejects the request.

Sites now marked, by file:

File Sites Notices
ui/hooks/useChat.ts 2 error banner (the one in the reported payload), tool-cancel (#12963's trigger)
ui/hooks/useChat.helpers.ts 3 command output ×2, Diff:…
ui/hooks/useChat.compaction.ts 3 compaction succeeded / cancelled / failed
ui/hooks/useChat.stream.helpers.ts 2 stream error, onSystemMessage local fallback
stream/streamChatResponse.autoCompaction.ts 2 auto-compaction success, auto-compaction warning
ui/TUIChat.tsx 3 export succeeded / session-not-found / export failed
services/ChatHistoryService.ts 1 (sink) covers its 15 callers

ChatHistoryService.addSystemMessage is marked at the sink rather than per-caller, because the item is built inside createHistoryItem and callers have no way to annotate it. Everything reaching that method is user-facing — Remote environment is shutting down…, WARNING: Tool … requires permission, No changes to display, diffs — so marking there is correct and covers all 15 call sites at once.

Two role: "system" sites are deliberately left alone, since they are real system messages and not notices:

  • commands/serve.ts:145 — seeds an empty history with the configured system prompt.
  • stream/streamChatResponse.autoCompaction.ts:170 — builds a throwaway item purely to count tokens; it never enters the history.

I did not widen isUiNotice to infer from role or content. Guessing would eventually drop a genuine system instruction, which fails silently and changes model behaviour — worse than the error this PR fixes.

Testing done

44 passed across uiNotices.test.ts (8), streamChatResponse.systemMessage.test.ts (5), useChat.clear.test.ts (4), and the pre-existing ChatHistoryService.test.ts (27).

New test should not send notices from the non-selector sites either builds the exact shape from #13026 — a /model notice, an error banner, a tool-cancel notice, a compaction notice and an export notice around two user turns — and asserts on what reaches chatCompletionStream:

expect(sent.messages.map((m) => m.content)).toEqual([
  systemMessage,
  "hello",
  "and now?",
]);

Control experiment. Removing only messageType: "system" from uiNotice(), leaving every call site as-is:

× uiNotice > produces an item that isUiNotice recognises and withoutUiNotices drops
  → expected undefined to be 'system'
× should not send notices from the non-selector sites either
  → expected [ { role: 'system', …(1) }, …(5) ] to have a length of 1 but got 6

Six system messages — all five notices plus the real prompt — which is the bug, quantified. Restoring the line returns it to 13/13 on those two files.

One pre-existing test needed updating: useChat.clear.test.ts asserts the "Chat history cleared" item by exact object equality, so it now expects messageType: "system". That message is a notice (it's addressed to the user, and it comes from slashCommands.ts:260 via the result.output path), so marking it is the intended behaviour rather than an accommodation.

Full suite: 1677 passed | 49 skipped. Four files fail when the whole suite runs in parallel (CtrlCProcessHandling, serve.test.ts, TUIChat.editMessage) — all three pass when run on their own, and they also pass on the previous commit of this branch, so that's runner contention rather than anything here.

eslint reports 0 errors on the changed files. prettier --check is clean once line endings are normalised (my checkout is CRLF, which makes prettier flag untouched files like slashCommands.ts too).

Note on repo status

@ryan-adtronics-llc mentioned on #13026 that the repo is read-only. That isn't the case — the API reports archived: false and this push went through. But activity does look stalled: last commit #13005 on 21 Jul, no merges since, and 2 of the last 15 closed PRs merged this month. Flagging so nobody's waiting on a review that may not come; I'd rather leave the fix complete either way.

@garymarkowitz-source — this is the branch to point your LM Studio / Qwen3.6 35b A3b setup at if the offer still stands. A strict Jinja template that raises on a mid-history system message is a better oracle than my mocks, since anything that slips through fails loudly instead of silently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant