mirror of
https://github.com/Crosstalk-Solutions/project-nomad.git
synced 2026-10-17 05:14:19 +02:00
fix(AI): stop <think> output leaking into titles, chips, and the RAG query (#1254)
ThinkTagSplitter was wired into both streaming paths but neither non-streaming one, and every ancillary call goes through non-streaming chat(). So a reasoning tasks model put its reasoning transcript into the sidebar title, into the suggestion chips, and — worst — into the string that gets embedded and sent to Qdrant, silently corrupting retrieval for the rest of the conversation. splitThinkTags() already existed and was already unit tested; it just had no production callers. Fixed in three layers: 1. Strip at the transport. New normalizeNonStreamed() in think_stream.ts merges structured reasoning (native `thinking`, /v1 `reasoning`) with anything the splitter pulls out of inline tags, and both _chatNative and _chatCompat now return through it. That covers every caller at once, including the legacy controller path and the eval harness. 2. Suppress at the source. _chatNative and _chatStreamNative forwarded `think` only when truthy, so `think: false` was never sent and Ollama left reasoning on for capable models. Guard is now definedness. This also fixes the ai.autoThinking=off setting, which had no effect on the native streaming transport. 3. Guard the call sites. Title, suggestions, and query rewrite now pass think:false plus thinkingCapable (from the memoized capability check, so no extra round-trip), and handle an empty-after-strip result. The rewrite matters most: QUERY_REWRITE_MAX_TOKENS caps it at 120, which a reasoning model burns entirely on thinking, so stripping alone would have turned a corrupt query into an empty one. It now falls back to the raw user message instead of embedding "". Verified: think_stream.spec.ts 17/17 (6 new cases, including the truncated-mid-thought case that drives the fallback), typecheck clean, full test:unit failures byte-identical to the pre-change baseline.
This commit is contained in:
parent
9a95f7b2b8
commit
5c97bf605a
|
|
@ -73,6 +73,12 @@ export class ChatService {
|
|||
|
||||
const model = (await this.resolveTasksModel(chosen.name, models)) ?? chosen.name
|
||||
|
||||
// Suggestions are a formatting task, not a reasoning one. A reasoning tasks model
|
||||
// would spend the whole response thinking and leave nothing to parse, so suppress
|
||||
// it at the source; `thinkingCapable` is what lets the compat transport pick
|
||||
// reasoning_effort:'none' rather than sending nothing. Memoized per model name.
|
||||
const thinkingCapable = await this.ollamaService.checkModelHasThinking(model)
|
||||
|
||||
const response = await this.ollamaService.chat({
|
||||
model,
|
||||
messages: [
|
||||
|
|
@ -82,6 +88,8 @@ export class ChatService {
|
|||
}
|
||||
],
|
||||
stream: false,
|
||||
think: false,
|
||||
thinkingCapable,
|
||||
})
|
||||
|
||||
if (response && response.message && response.message.content) {
|
||||
|
|
@ -112,6 +120,9 @@ export class ChatService {
|
|||
|
||||
return filtered.map((s) => toTitleCase(s))
|
||||
} else {
|
||||
// Empty content after the <think> split means the model produced reasoning and
|
||||
// nothing else. Log it rather than silently returning no chips.
|
||||
logger.warn(`[ChatService] Model "${model}" returned no usable suggestion text`)
|
||||
return []
|
||||
}
|
||||
} catch (error) {
|
||||
|
|
@ -268,6 +279,9 @@ export class ChatService {
|
|||
// configured rather than the chat model that just answered.
|
||||
const titleModel = (await this.resolveTasksModel(model)) ?? model
|
||||
|
||||
// Naming a chat needs no reasoning; see the note in getChatSuggestions.
|
||||
const thinkingCapable = await this.ollamaService.checkModelHasThinking(titleModel)
|
||||
|
||||
const response = await this.ollamaService.chat({
|
||||
model: titleModel,
|
||||
messages: [
|
||||
|
|
@ -275,10 +289,16 @@ export class ChatService {
|
|||
{ role: 'user', content: userMessage },
|
||||
{ role: 'assistant', content: assistantMessage },
|
||||
],
|
||||
think: false,
|
||||
thinkingCapable,
|
||||
})
|
||||
|
||||
title = response?.message?.content?.trim()
|
||||
if (!title) {
|
||||
// Nothing left once reasoning was split out — fall back to the user's own words.
|
||||
logger.warn(
|
||||
`[ChatService] Model "${titleModel}" returned no usable title text; using the user message`
|
||||
)
|
||||
title = userMessage.slice(0, 57) + (userMessage.length > 57 ? '...' : '')
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@ import OpenAI from 'openai'
|
|||
import type { ChatCompletionChunk, ChatCompletionMessageParam } from 'openai/resources/chat/completions.js'
|
||||
import type { Stream } from 'openai/streaming.js'
|
||||
import { Ollama } from 'ollama'
|
||||
import { ThinkTagSplitter } from '../utils/think_stream.js'
|
||||
import { ThinkTagSplitter, normalizeNonStreamed } from '../utils/think_stream.js'
|
||||
import { readContextLength, readModelfileNumCtx } from '../utils/context_window.js'
|
||||
import { NomadOllamaModel } from '../../types/ollama.js'
|
||||
import { EMBEDDING_MODEL_NAME, FALLBACK_RECOMMENDED_OLLAMA_MODELS } from '../../constants/ollama.js'
|
||||
|
|
@ -511,15 +511,25 @@ export class OllamaService {
|
|||
model: chatRequest.model,
|
||||
messages: chatRequest.messages,
|
||||
stream: false,
|
||||
...(chatRequest.think ? { think: chatRequest.think } : {}),
|
||||
// Definedness, not truthiness: `think: false` has to reach Ollama, which
|
||||
// otherwise leaves thinking ON for a capable model. Unset still sends
|
||||
// nothing, so non-thinking models and other backends are unaffected.
|
||||
...(chatRequest.think !== undefined ? { think: chatRequest.think } : {}),
|
||||
...(chatRequest.keepAlive !== undefined ? { keep_alive: chatRequest.keepAlive } : {}),
|
||||
options: this._nativeOptions(chatRequest),
|
||||
})
|
||||
|
||||
// A model can report reasoning on the structured field and still emit literal
|
||||
// <think> tags in content; the splitter is a no-op on text that has none.
|
||||
const split = normalizeNonStreamed(
|
||||
response.message?.content ?? '',
|
||||
response.message?.thinking ?? ''
|
||||
)
|
||||
|
||||
return {
|
||||
message: {
|
||||
content: response.message?.content ?? '',
|
||||
thinking: response.message?.thinking ?? undefined,
|
||||
content: split.content,
|
||||
thinking: split.thinking || undefined,
|
||||
},
|
||||
done: true,
|
||||
model: response.model,
|
||||
|
|
@ -540,12 +550,18 @@ export class OllamaService {
|
|||
const response = await this.openai.chat.completions.create(params, { signal: chatRequest.signal })
|
||||
const choice = response.choices[0]
|
||||
|
||||
// Ollama's OpenAI-compat endpoint (/v1) emits thinking as `reasoning`; its native
|
||||
// shape uses `thinking`. Read both so thinking is never silently dropped (#1065).
|
||||
// Backends that emit tags inline instead (LM Studio, llama.cpp) are handled by the split.
|
||||
const split = normalizeNonStreamed(
|
||||
choice.message.content ?? '',
|
||||
(choice.message as any).thinking ?? (choice.message as any).reasoning ?? ''
|
||||
)
|
||||
|
||||
return {
|
||||
message: {
|
||||
content: choice.message.content ?? '',
|
||||
// Ollama's OpenAI-compat endpoint (/v1) emits thinking as `reasoning`; its native
|
||||
// shape uses `thinking`. Read both so thinking is never silently dropped (#1065).
|
||||
thinking: (choice.message as any).thinking ?? (choice.message as any).reasoning ?? undefined,
|
||||
content: split.content,
|
||||
thinking: split.thinking || undefined,
|
||||
},
|
||||
done: true,
|
||||
model: response.model,
|
||||
|
|
@ -575,7 +591,9 @@ export class OllamaService {
|
|||
model: chatRequest.model,
|
||||
messages: chatRequest.messages,
|
||||
stream: true,
|
||||
...(chatRequest.think ? { think: chatRequest.think } : {}),
|
||||
// See _chatNative: `think: false` must be sent explicitly, or a thinking-capable
|
||||
// model keeps reasoning regardless of the user's ai.autoThinking preference.
|
||||
...(chatRequest.think !== undefined ? { think: chatRequest.think } : {}),
|
||||
...(chatRequest.keepAlive !== undefined ? { keep_alive: chatRequest.keepAlive } : {}),
|
||||
options: this._nativeOptions(chatRequest),
|
||||
})
|
||||
|
|
|
|||
|
|
@ -255,6 +255,13 @@ export class RagPipelineService {
|
|||
// ordering. A small dedicated model keeps the chat model's cache intact.
|
||||
const rewriteModel = (await resolveTasksModel(this.ollamaService, model, undefined, '[RAG]')) ?? model
|
||||
|
||||
// A rewrite is a mechanical transformation, and QUERY_REWRITE_MAX_TOKENS is far
|
||||
// too small a budget for a reasoning model to think and then answer — it would
|
||||
// spend the whole cap thinking and get truncated with no query at all. Suppress
|
||||
// reasoning at the source; `thinkingCapable` is what lets the compat transport
|
||||
// send reasoning_effort:'none' instead of nothing. Memoized per model name.
|
||||
const thinkingCapable = await this.ollamaService.checkModelHasThinking(rewriteModel)
|
||||
|
||||
const response = await this.ollamaService.chat({
|
||||
model: rewriteModel,
|
||||
messages: [
|
||||
|
|
@ -269,9 +276,21 @@ export class RagPipelineService {
|
|||
// pin the sampler so the same conversation rewrites the same way.
|
||||
numPredict: QUERY_REWRITE_MAX_TOKENS,
|
||||
temperature: 0,
|
||||
think: false,
|
||||
thinkingCapable,
|
||||
})
|
||||
|
||||
const rewrittenQuery = response.message.content.trim()
|
||||
// Empty means the response was reasoning and nothing else (or was truncated
|
||||
// mid-thought). Embedding an empty string would search the corpus for nothing
|
||||
// and quietly poison retrieval for the rest of the conversation, so fall back
|
||||
// to the raw message — the same shape as the first-turn skip above.
|
||||
if (!rewrittenQuery) {
|
||||
logger.warn(
|
||||
`[RAG] Model "${rewriteModel}" produced no query text; falling back to the user message`
|
||||
)
|
||||
return { query: lastUserMessage?.content ?? null, didRewrite: false }
|
||||
}
|
||||
logger.info(`[RAG] Query rewritten: "${rewrittenQuery}"`)
|
||||
return { query: rewrittenQuery, didRewrite: true }
|
||||
} catch (error) {
|
||||
|
|
|
|||
|
|
@ -103,3 +103,20 @@ export function splitThinkTags(raw: string): ThinkSplit {
|
|||
thinking: streamed.thinking + tail.thinking,
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Non-streaming counterpart to the merge the streaming normalizers do inline.
|
||||
*
|
||||
* A backend may report reasoning structurally (native `message.thinking`, or
|
||||
* `reasoning` on the /v1 shim), inline as `<think>` tags, or both at once —
|
||||
* and the ancillary calls (title, suggestions, query rewrite) all go through
|
||||
* the non-streaming path. Without this the tags reached the sidebar title and,
|
||||
* worse, the string that gets embedded and sent to Qdrant.
|
||||
*/
|
||||
export function normalizeNonStreamed(rawContent: string, nativeThinking?: string): ThinkSplit {
|
||||
const split = splitThinkTags(rawContent ?? '')
|
||||
return {
|
||||
content: split.content,
|
||||
thinking: (nativeThinking ?? '') + split.thinking,
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,6 +1,11 @@
|
|||
import { test } from 'node:test'
|
||||
import assert from 'node:assert/strict'
|
||||
import { ThinkTagSplitter, partialTagSuffix, splitThinkTags } from '../../app/utils/think_stream.js'
|
||||
import {
|
||||
ThinkTagSplitter,
|
||||
normalizeNonStreamed,
|
||||
partialTagSuffix,
|
||||
splitThinkTags,
|
||||
} from '../../app/utils/think_stream.js'
|
||||
|
||||
/**
|
||||
* Characterization tests for the <think> tag splitter extracted out of
|
||||
|
|
@ -95,3 +100,55 @@ test('splitThinkTags handles the non-streaming case', () => {
|
|||
})
|
||||
assert.deepEqual(splitThinkTags('no tags here'), { content: 'no tags here', thinking: '' })
|
||||
})
|
||||
|
||||
// --- normalizeNonStreamed ----------------------------------------------------
|
||||
//
|
||||
// The non-streaming path feeds every ancillary call: chat titles, suggestion
|
||||
// chips, and — the one that matters — the query that gets embedded and sent to
|
||||
// Qdrant. Reasoning reaching any of those is the bug this exists to close.
|
||||
|
||||
test('normalizeNonStreamed: inline tags are split out of content', () => {
|
||||
assert.deepEqual(normalizeNonStreamed('<think>weighing it up</think>Boiling Water'), {
|
||||
content: 'Boiling Water',
|
||||
thinking: 'weighing it up',
|
||||
})
|
||||
})
|
||||
|
||||
test('normalizeNonStreamed: a structured thinking field passes through untouched', () => {
|
||||
assert.deepEqual(normalizeNonStreamed('Boiling Water', 'weighing it up'), {
|
||||
content: 'Boiling Water',
|
||||
thinking: 'weighing it up',
|
||||
})
|
||||
})
|
||||
|
||||
test('normalizeNonStreamed: structured and inline reasoning merge into one channel', () => {
|
||||
// Ollama reports reasoning natively AND the model emits literal tags in content.
|
||||
assert.deepEqual(normalizeNonStreamed('<think>then this</think>Answer', 'first this '), {
|
||||
content: 'Answer',
|
||||
thinking: 'first this then this',
|
||||
})
|
||||
})
|
||||
|
||||
test('normalizeNonStreamed: reasoning truncated mid-thought leaves no content', () => {
|
||||
// What a reasoning model does under QUERY_REWRITE_MAX_TOKENS: it spends the whole
|
||||
// budget thinking and never closes the tag. Callers must treat this as "no answer"
|
||||
// rather than embedding the empty string.
|
||||
assert.deepEqual(normalizeNonStreamed('<think>still working on it'), {
|
||||
content: '',
|
||||
thinking: 'still working on it',
|
||||
})
|
||||
})
|
||||
|
||||
test('normalizeNonStreamed: text with no tags is passed through exactly', () => {
|
||||
assert.deepEqual(normalizeNonStreamed(' How do I purify water? '), {
|
||||
content: ' How do I purify water? ',
|
||||
thinking: '',
|
||||
})
|
||||
})
|
||||
|
||||
test('normalizeNonStreamed: tolerates a missing content field', () => {
|
||||
assert.deepEqual(normalizeNonStreamed(undefined as unknown as string), {
|
||||
content: '',
|
||||
thinking: '',
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user