Fix #2336: Feedback classifier leaks raw LLM completion (think tags, special tokens) into p - #2339
Closed
Memtensor-AI wants to merge 2 commits into
Closed
Conversation
Thinking-enabled DeepSeek-family models on gateways that keep the reasoning block inside `choice.message.content` leaked `<think>...</think>` blocks and DeepSeek special tokens (`<｜end▁of▁sentence｜>`, `<｜end▁of▁session｜>`) into `ProviderCompletion.text`. That text then flowed through `feedback.classify` into persisted `FeedbackRow.rationale` rows — polluting retrieval-visible memory. Fix at the provider choke point: introduce `core/llm/sanitize.ts` and apply it to `openai_compatible`, `anthropic`, `gemini`, `bedrock` `complete()` return values. One transform covers all current + future ops (including `feedback.classify`, `feedback.refine`, `l3.abstraction`, `retrieval.filter`) without needing per-op thinking-disable coverage. Streaming path is out of scope: chunks arrive independently and safe chunk-level sanitization requires buffer-based state; the reported leak is on the storage path, which uses `complete()`. Adds: - `core/llm/sanitize.ts` — the transform (matched `<think>` blocks, orphan `</think>` / `<think>` fragments, DeepSeek special tokens, excess blank-line collapse). - `tests/unit/llm/sanitize.test.ts` — 17 sanitizer unit tests. - `tests/unit/llm/providers.test.ts` — two regression tests pinning provider-level behavior, including the exact orphan-`</think>` fragment shape captured in the MemTensor#2336 report. All 1576 tests + tsc --noEmit pass.
Memtensor-AI
added
ai:generated
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2339 ✅ OpenCodeReview: Review complete: 0 finding(s) across 5 selected item(s). Generated by cloud-assistant via Open Code Review. |
Collaborator
Author
🔧 Open Code Review requested Agent fixOpen Code Review found 2 issue(s). I have resumed the development Agent to fix them.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed. |
…emTensor#2336) The initial MemTensor#2336 fix only wired sanitizeCompletionText into the non-streaming complete() path of both providers. The stream() path still yielded raw SSE deltas, so any consumer that accumulates chunk.delta — including core/llm/client.ts::stream, which logs the joined text and forwards chunks to storage-adjacent callers like the DSH bridge — could still receive <think>...</think> blocks and DeepSeek session tokens. Per-chunk sanitization is unsafe because a <think> block can straddle two SSE frames (the opener arrives in one delta, the closer in another), so a stateless regex would miss the pair. Buffer the entire stream in the provider, sanitize the accumulated text once at finish, and yield one sanitized delta before the done chunk. Callers still see chunks.map(c => c.delta).join("") === sanitized text, matching the existing stream contract exercised by the provider tests. Tests: two new streaming regression cases per provider — one splits a <think> block across SSE chunks, the other reproduces the orphan </think> soup captured in the MemTensor#2336 evidence. Both assert the joined delta stream matches the sanitizer output and that finishReason + usage still surface on the done chunk. Verification: npm run lint clean; npm run test — 1579 passed / 2 skipped across 184 files.
Memtensor-AI
added
status:ready
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.
Description
Fixes issue #2336 — feedback classifier and other ops were leaking raw LLM completion tokens (
<think>...</think>blocks and DeepSeek gateway tokens like<｜end▁of▁sentence｜>) into persistedFeedbackRow.rationale, which then polluted retrieval-visible memory on subsequent turns.Root cause:
apps/memos-local-plugin/core/llm/providers/openai.ts::completereturnedchoice.message.contentverbatim, and no downstream sanitizer knew about<think>tags or DeepSeek special tokens. Thinking-enabled DeepSeek-family models served through gateways that keep the reasoning block insidecontent(rather than a separatereasoning_contentfield) leaked it end-to-end into storage. Adopts the reporter's preferred fix (option 1: sanitize at the provider choke point) — one transform covers all current and future ops (feedback.classify,feedback.refine,l3.abstraction,retrieval.filter, …) without requiring per-op thinking-disable coverage.Changes: new
core/llm/sanitize.tsmodule (strips matched<think>...</think>blocks, orphan closing</think>, orphan opening<think>+ trailing text, DeepSeek special tokens, and collapses excess blank lines), applied toopenai_compatible,anthropic,gemini,bedrockcomplete()return values. Streaming path is deliberately out of scope (chunk boundaries make safe chunk-level sanitization stateful — deferred; the reported leak is on the storage path which usescomplete()).Tests: added
tests/unit/llm/sanitize.test.ts(17 unit tests including the exact artifact soup captured in the #2336 report, an ASCII-pipe safety guard, and idempotency) plus 2 provider-level regression tests intests/unit/llm/providers.test.tspinning end-to-end behavior. Verification:npm run test— 1576 passed / 2 skipped across 184 files;npm run lint(tsc --noEmit) — clean.Related Issue (Required): Fixes #2336
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Automated tests are pending.
Checklist
@whipser030, @hijzy please review this PR.
Reviewer Checklist