runtime: append newline to non-last steer messages on multi-drain#2518
Merged
simonferquel merged 4 commits intodocker:mainfrom Apr 27, 2026
Merged
Conversation
When multiple steer messages are drained in one batch they were injected back-to-back with no separator, causing some chat templates that concatenate consecutive user messages before tokenisation to glue adjacent word fragments together. Fix: introduce drainAndEmitSteered() which appends "\n" to every non-last message's content before calling appendSteerAndEmit. For multi-content messages the "\n" is appended to the last text part (or a new text part is added when none exists). The original QueuedMessage slice is never mutated — appendNewlineToQueuedMessage works on a shallow copy. The three inline steerQueue.Drain() + for-loop sites in loop.go are replaced by calls to the new helper. Tests added: - TestAppendNewlineToQueuedMessage: unit-tests the pure helper across plain-text, multi-content-with-text-part, multi-content-no-text-part, and no-mutation cases. - TestDrainAndEmitSteered_MultipleMessages: verifies three plain-text messages are kept as separate session messages with \n on non-last ones. - TestDrainAndEmitSteered_MultiContent: verifies the same for multi-content messages. Fixes docker#2517 Assisted-By: docker-agent
Replace make([]T, len)+copy with append(nil, src...) to avoid appending to a non-zero-initialized slice, which triggers the makezero linter. Assisted-By: docker-agent
| } | ||
| } | ||
| // No text part found — append a new one. | ||
| parts = append(parts, chat.MessagePart{Type: chat.MessagePartTypeText, Text: "\n"}) |
Contributor
There was a problem hiding this comment.
This is not useful:
- pure whitespace parts will be skipped down the line anyway
- non text parts have a layout that makes them immune to the issue (they have an envelope that acts as a separator)
S1: add NOTE comment in drainAndEmitSteered documenting that the
appended \n is persisted in session messages and UserMessageEvent.
S2: appendNewlineToQueuedMessage now returns sm unchanged when
MultiContent has no text part (e.g. image-only) instead of
appending a synthetic whitespace text part. Non-text parts carry
their own envelope that acts as a separator, and pure whitespace
parts are skipped downstream anyway.
S3: drainAndEmitSteered now returns (bool, int) where int is the
pre-drain message count; callers no longer need a separate
len(sess.GetAllMessages()) call before draining.
N3: add plain-text no-mutation subtest in TestAppendNewlineToQueuedMessage.
N4: reword appendNewlineToQueuedMessage doc comment.
Assisted-By: docker-agent
Contributor
Author
|
Addressed in the latest commit. For the no-text-part branch (e.g. image-only |
| // Shallow-copy the slice so we don't mutate the original. | ||
| parts := append([]chat.MessagePart(nil), sm.MultiContent...) | ||
| // Find the last text part and append \n to it. | ||
| for i := len(parts) - 1; i >= 0; i-- { |
Contributor
There was a problem hiding this comment.
Why are we iterating here ? Only do append \n on the latest part, and only if it is of text type.
Instead of iterating to find the last text part, only inspect sm.MultiContent[last]: if it is a text part, append \n; otherwise return sm unchanged. Non-text parts carry their own provider envelope that acts as a separator, so no \n is needed regardless of what precedes them. Update tests to match: replace the 3-part (text/image/text) subtest with explicit last-part-text and last-part-non-text cases. Assisted-By: docker-agent
Contributor
Author
|
Simplified in the latest commit. The loop is gone — |
trungutt
approved these changes
Apr 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2517
Problem
When the runtime drains multiple queued steer messages in one batch, each was injected into the session back-to-back with no separator. Some chat templates concatenate consecutive
role=usermessages before tokenisation, causing adjacent word fragments from different messages to be glued together.Fix
drainAndEmitSteered()helper replaces the three inlinesteerQueue.Drain()+ for-loop sites inloop.go.appendNewlineToQueuedMessage()appends\nto every non-last message:\nappended toContent.\nappended to the lasttextpart'sTextfield; if no text part exists, a new one is appended.QueuedMessageis never mutated (shallow-copy of the parts slice).role=usersession message.Tests
TestAppendNewlineToQueuedMessage: 4 subtests covering plain-text, multi-content with text part, multi-content without text part, and no-mutation of the original.TestDrainAndEmitSteered_MultipleMessages: 3 plain-text messages → 3 separate session messages, non-last have\n, last does not.TestDrainAndEmitSteered_MultiContent: same for multi-content messages.