From dc213d47b869f1706aa35d7b4f40332094919aac Mon Sep 17 00:00:00 2001 From: Frank Song Date: Thu, 14 May 2026 07:13:34 +0800 Subject: [PATCH] fix: preserve literal thinking tags --- CHANGELOG.md | 2 ++ api/streaming.py | 9 ++++--- static/ui.js | 14 +++++----- tests/test_issue607.py | 30 +++++++++++++++++++++ tests/test_sprint38.py | 61 +++++++++++++++++++++++++++--------------- 5 files changed, 84 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 48e58a6e..b1967d0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Fixed +- **Issue #2152** — Literal discussions of reasoning tags such as `` and `` no longer disappear from saved or re-rendered assistant messages. WebUI now treats `...`, MiniMax `<|channel>thought...`, and Gemma 4 `<|turn|>thinking...` blocks as hidden reasoning metadata only when the wrapper is the first non-whitespace content in the response; provider wrappers with leading whitespace still strip as before. + - **PR #2191** by @lucasrc (auth refactor 1/3) — Thread-safe login rate limiter (new `_LOGIN_ATTEMPTS_LOCK`) + PBKDF2 key separation (new `_pbkdf2_key()` reading `.pbkdf2_key` separately from `_signing_key()` reading `.signing_key` — previously both shared `.signing_key`, a key-reuse anti-pattern across HMAC and PBKDF2 primitives) + transparent migration in `verify_password()` that re-salts legacy hashes with the new key on next successful login. 241-line regression suite covering the lock + migration paths. Split from earlier #2167 per maintainer review request. - **PR #2192** by @lucasrc (auth refactor 2/3, depends on #2191) — Invalidate password-hash cache when password changes via the Settings panel. The PR #2191 cache lives for the process lifetime, but `save_settings({'_set_password': ...})` could mutate `settings.json.password_hash` without telling the auth module — leaving the cache stale and verifying against the old password until restart. Now `save_settings()` calls `_invalidate_password_hash_cache()` on both `_set_password` and `_clear_password` paths. 52-line regression suite + `verify_password()` simplified to rely on the new hook instead of doing the invalidation itself. diff --git a/api/streaming.py b/api/streaming.py index 91dbc738..bf473d90 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -689,9 +689,12 @@ def _strip_thinking_markup(text: str) -> str: if not text: return '' s = str(text) - s = re.sub(r'.*?', ' ', s, flags=re.IGNORECASE | re.DOTALL) - s = re.sub(r'<\|channel\|>thought.*?', ' ', s, flags=re.IGNORECASE | re.DOTALL) - s = re.sub(r'<\|turn\|>thinking\n.*?', ' ', s, flags=re.IGNORECASE | re.DOTALL) # Gemma 4 + # Treat provider thinking wrappers as metadata only when they lead the + # response. Literal discussion of these tags later in normal prose should + # stay visible (#2152). + s = re.sub(r'^\s*.*?\s*', ' ', s, flags=re.IGNORECASE | re.DOTALL) + s = re.sub(r'^\s*<\|channel\|?>thought\n?.*?\s*', ' ', s, flags=re.IGNORECASE | re.DOTALL) + s = re.sub(r'^\s*<\|turn\|>thinking\n.*?\s*', ' ', s, flags=re.IGNORECASE | re.DOTALL) # Gemma 4 s = re.sub(r'^\s*(the|ther)\s+user\s+is\s+asking.*$', ' ', s, flags=re.IGNORECASE | re.MULTILINE) # Strip plain-text thinking preambles from models that don't use tags (e.g. Qwen3). # These appear as the very first sentence of the assistant response and are not useful as titles. diff --git a/static/ui.js b/static/ui.js index d0f27aa8..4814092b 100644 --- a/static/ui.js +++ b/static/ui.js @@ -4236,7 +4236,7 @@ function _messageHasReasoningPayload(m){ if(!m||m.role!=='assistant') return false; if(m.reasoning) return true; if(Array.isArray(m.content)) return m.content.some(p=>p&&(p.type==='thinking'||p.type==='reasoning')); - return /[\s\S]*?<\/think>|<\|channel>thought\n[\s\S]*?|<\|turn\|>thinking\n[\s\S]*?/.test(String(m.content||'')); + return /^\s*(?:[\s\S]*?<\/think>|<\|channel\|?>thought\n?[\s\S]*?|<\|turn\|>thinking\n[\s\S]*?)/.test(String(m.content||'')); } function _formatTurnTps(value){ const n=Number(value); @@ -5101,25 +5101,25 @@ function renderMessages(options){ } if(!thinkingText && m.reasoning) thinkingText=m.reasoning; if(!thinkingText && typeof content==='string'){ - const thinkMatch=content.match(/([\s\S]*?)<\/think>/); + const thinkMatch=content.match(/^\s*([\s\S]*?)<\/think>\s*/); if(thinkMatch){ thinkingText=thinkMatch[1].trim(); - content=content.replace(/[\s\S]*?<\/think>\s*/,'').trimStart(); + content=content.replace(/^\s*[\s\S]*?<\/think>\s*/,'').trimStart(); } if(!thinkingText){ // Historical name "gemmaMatch" refers to MiniMax <|channel>thought format. - const gemmaMatch=content.match(/<\|channel>thought\n([\s\S]*?)/); + const gemmaMatch=content.match(/^\s*<\|channel\|?>thought\n?([\s\S]*?)\s*/); if(gemmaMatch){ thinkingText=gemmaMatch[1].trim(); - content=content.replace(/<\|channel>thought\n[\s\S]*?\s*/,'').trimStart(); + content=content.replace(/^\s*<\|channel\|?>thought\n?[\s\S]*?\s*/,'').trimStart(); } } if(!thinkingText){ // Gemma 4 uses asymmetric <|turn|>thinking\n... delimiters. - const gemmaTurnMatch=content.match(/<\|turn\|>thinking\n([\s\S]*?)/); + const gemmaTurnMatch=content.match(/^\s*<\|turn\|>thinking\n([\s\S]*?)\s*/); if(gemmaTurnMatch){ thinkingText=gemmaTurnMatch[1].trim(); - content=content.replace(/<\|turn\|>thinking\n[\s\S]*?\s*/,'').trimStart(); + content=content.replace(/^\s*<\|turn\|>thinking\n[\s\S]*?\s*/,'').trimStart(); } } } diff --git a/tests/test_issue607.py b/tests/test_issue607.py index 15d0c4e4..0e950c3d 100644 --- a/tests/test_issue607.py +++ b/tests/test_issue607.py @@ -64,6 +64,36 @@ class TestGemma4ThinkingTokenStrip: result = _strip_thinking_markup(raw) assert result == "Answer" + def test_mid_sentence_think_tags_are_preserved(self): + """Literal discussion of tags in visible prose must survive (#2152).""" + raw = "The literal tags and describe reasoning markup." + result = _strip_thinking_markup(raw) + assert result == raw + + def test_mid_sentence_closed_think_block_is_preserved(self): + """Only leading provider wrappers are stripped; later tag-looking text stays visible.""" + raw = "Use scratchpad as an example in the answer." + result = _strip_thinking_markup(raw) + assert result == raw + + def test_mid_sentence_channel_tokens_are_preserved(self): + """MiniMax-style channel markers later in prose are visible content, not metadata.""" + raw = "Use <|channel>thought to start reasoning and to finish." + result = _strip_thinking_markup(raw) + assert result == raw + + def test_mid_sentence_gemma4_tokens_are_preserved(self): + """Gemma 4 thinking markers later in prose are visible content, not metadata.""" + raw = "Use <|turn|>thinking to start reasoning and to finish." + result = _strip_thinking_markup(raw) + assert result == raw + + def test_minimax_channel_without_second_pipe_still_stripped_at_start(self): + """The client-facing MiniMax <|channel>thought shape is stripped when leading.""" + raw = "<|channel>thought\nSome reasoningAnswer" + result = _strip_thinking_markup(raw) + assert result == "Answer" + class TestGemma4TitleLeakDetection: """Verify _looks_invalid_generated_title catches Gemma 4 leak.""" diff --git a/tests/test_sprint38.py b/tests/test_sprint38.py index 020574f3..880a812e 100644 --- a/tests/test_sprint38.py +++ b/tests/test_sprint38.py @@ -5,7 +5,6 @@ Covers the static render path (ui.js regex logic, verified against the JS source and the streaming render path (messages.js _streamDisplay logic). """ import pathlib -import re REPO_ROOT = pathlib.Path(__file__).parent.parent UI_JS = (REPO_ROOT / "static" / "ui.js").read_text() @@ -14,24 +13,25 @@ MSG_JS = (REPO_ROOT / "static" / "messages.js").read_text() # ── ui.js: static render path ──────────────────────────────────────────────── -def test_think_regex_has_no_anchor(): - """The regex in ui.js must not use a ^ anchor so leading whitespace is allowed.""" +def test_think_regex_is_leading_only_after_optional_whitespace(): + """The regex in ui.js must anchor after optional whitespace.""" # Find the thinkMatch line by locating the .match( call on that line idx = UI_JS.find("const thinkMatch=content.match(") assert idx >= 0, "thinkMatch line not found in ui.js" line = UI_JS[idx:idx+100] - # The regex must NOT start with ^ right after the opening / - assert "/^" not in line and "(/^" not in line, \ - f"thinkMatch regex must not use ^ anchor — found: {line.strip()}" + assert "/^\\s*" in line, \ + f"thinkMatch regex must only match leading blocks after whitespace — found: {line.strip()}" + assert "/^" not in line, \ + f"thinkMatch regex must still allow leading whitespace — found: {line.strip()}" -def test_gemma_regex_has_no_anchor(): - """The Gemma channel-token regex in ui.js must not use a ^ anchor.""" - match = re.search(r'const gemmaMatch=content\.match\((/[^/]+/)\)', UI_JS) - assert match, "gemmaMatch line not found in ui.js" - pattern = match.group(1) - assert not pattern.startswith('/^'), \ - f"gemmaMatch regex must not use ^ anchor — got {pattern}" +def test_gemma_regex_is_leading_only_after_optional_whitespace(): + """The MiniMax channel-token regex in ui.js must anchor after optional whitespace.""" + idx = UI_JS.find("const gemmaMatch=content.match(") + assert idx >= 0, "gemmaMatch line not found in ui.js" + line = UI_JS[idx:idx+140] + assert "/^\\s*<\\|channel\\|?>thought\\n?" in line, \ + f"gemmaMatch regex must only match leading channel blocks after whitespace — found: {line.strip()}" def test_think_content_removal_uses_replace_not_slice(): @@ -42,6 +42,8 @@ def test_think_content_removal_uses_replace_not_slice(): block = UI_JS[idx:idx+200] assert "content.replace(" in block, \ "ui.js must use content.replace() to remove block (not .slice())" + assert "content.replace(/^\\s*" in block, \ + "ui.js must remove only leading blocks after optional whitespace" assert ".trimStart()" in block, \ "ui.js must call .trimStart() on content after removing the block" @@ -53,6 +55,8 @@ def test_gemma_content_removal_uses_replace_not_slice(): block = UI_JS[idx:idx+200] assert "content.replace(" in block, \ "ui.js must use content.replace() to remove Gemma channel block (not .slice())" + assert "content.replace(/^\\s*<\\|channel\\|?>thought\\n?" in block, \ + "ui.js must remove only leading Gemma channel blocks after optional whitespace" assert ".trimStart()" in block, \ "ui.js must call .trimStart() on content after removing the Gemma channel block" @@ -65,11 +69,11 @@ def test_gemma_turn_regex_in_ui_js(): " (note: double-pipe: <|turn|> not <|turn>)" ) # Extraction block - match = re.search(r'const gemmaTurnMatch=content\.match\((/[^/]+/)\)', UI_JS) - assert match, "gemmaTurnMatch line not found in ui.js" - pattern = match.group(1) - assert not pattern.startswith('/^'), ( - f"gemmaTurnMatch regex must not use ^ anchor — got {pattern}" + idx = UI_JS.find("const gemmaTurnMatch=content.match(") + assert idx >= 0, "gemmaTurnMatch line not found in ui.js" + line = UI_JS[idx:idx+140] + assert "/^\\s*<\\|turn\\|>thinking\\n" in line, ( + f"gemmaTurnMatch regex must only match leading Gemma 4 blocks after whitespace — found: {line.strip()}" ) @@ -81,11 +85,24 @@ def test_gemma_turn_content_removal_uses_replace_not_slice(): assert "content.replace(" in block, ( "ui.js must use content.replace() to remove Gemma 4 turn block (not .slice())" ) + assert "content.replace(/^\\s*<\\|turn\\|>thinking\\n" in block, ( + "ui.js must remove only leading Gemma 4 turn blocks after optional whitespace" + ) assert ".trimStart()" in block, ( "ui.js must call .trimStart() on content after removing the Gemma 4 turn block" ) +def test_message_reasoning_payload_detection_is_leading_only(): + """Persisted literal tag discussion later in content must not create a thinking card.""" + idx = UI_JS.find("function _messageHasReasoningPayload(m)") + assert idx >= 0, "_messageHasReasoningPayload function not found in ui.js" + block = UI_JS[idx:idx+500] + assert "return /^\\s*(?:" in block, ( + "_messageHasReasoningPayload must only detect leading provider thinking wrappers" + ) + + # ── messages.js: streaming render path ─────────────────────────────────────── def test_stream_display_trims_before_startswith(): @@ -128,13 +145,13 @@ def test_stream_display_trims_return_after_close(): # ── Regression: existing anchored patterns must be gone ────────────────────── -def test_no_anchored_think_regex_in_ui_js(): - """The old anchored regex /^/ must not exist in ui.js.""" +def test_no_strictly_anchored_think_regex_in_ui_js(): + """The old /^/ shape must not return; leading whitespace remains supported.""" assert "/^" not in UI_JS, \ "Old anchored /^/ regex still present in ui.js — fix not applied" -def test_no_anchored_gemma_regex_in_ui_js(): - """The old anchored Gemma regex must not exist in ui.js.""" +def test_no_strictly_anchored_gemma_regex_in_ui_js(): + """The old /^<|channel>/ shape must not return; leading whitespace remains supported.""" assert "/^<|channel>" not in UI_JS, \ "Old anchored /^<|channel>/ regex still present in ui.js — fix not applied"