From 734ccc337f561d83e1db077186f98a3f2734a1d8 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 13 Apr 2026 01:04:18 -0400 Subject: [PATCH] test(llm): lock in _should_think classifier; drop briefing think overrides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds 38 parametrized tests for the _should_think classifier covering the explicit-override path, empty/whitespace content, short/medium/long length boundaries, case-insensitive keyword matching, and a chatty-message negative set. These pin the content-based semantics so future tweaks to the keyword list or length thresholds surface regressions immediately instead of going unnoticed behind subtle latency changes. Also drops the `think=True` overrides from the briefing /discuss-article and /discuss-topic entry points. With `"discuss"` added to _THINK_KEYWORDS, those canned prompts trip the classifier naturally, so the overrides were redundant — keeping a uniform "classifier is authoritative" rule makes the code easier to reason about. Co-Authored-By: Claude Opus 4.6 --- src/fabledassistant/routes/briefing.py | 2 - .../services/generation_task.py | 1 + tests/test_should_think.py | 127 ++++++++++++++++++ 3 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 tests/test_should_think.py diff --git a/src/fabledassistant/routes/briefing.py b/src/fabledassistant/routes/briefing.py index 9514537..fba4923 100644 --- a/src/fabledassistant/routes/briefing.py +++ b/src/fabledassistant/routes/briefing.py @@ -578,7 +578,6 @@ async def discuss_article(item_id: int): buf, history, model, uid, conv_id, conv.title or "", "Please summarize and discuss this article.", - think=True, )) return jsonify({"assistant_message_id": assistant_msg.id, "status": "generating"}), 202 @@ -677,7 +676,6 @@ async def discuss_topic(topic: str): buf, history, model, uid, conv_id, conv.title or "", user_prompt, - think=True, )) return jsonify({ diff --git a/src/fabledassistant/services/generation_task.py b/src/fabledassistant/services/generation_task.py index 6b7f100..7d60407 100644 --- a/src/fabledassistant/services/generation_task.py +++ b/src/fabledassistant/services/generation_task.py @@ -67,6 +67,7 @@ _THINK_KEYWORDS: tuple[str, ...] = ( "review", "critique", "evaluate", "trade-off", "tradeoff", "trade off", "pros and cons", "step by step", "walk me through", "prove", "derive", "figure out", "work through", + "discuss", # covers briefing /discuss-article + /discuss-topic entry points ) # Messages shorter than this and without any think-keyword are treated as diff --git a/tests/test_should_think.py b/tests/test_should_think.py new file mode 100644 index 0000000..8ca1385 --- /dev/null +++ b/tests/test_should_think.py @@ -0,0 +1,127 @@ +"""Tests for the `_should_think` classifier. + +`_should_think` decides whether qwen3-class models should engage chain-of- +thought for a given chat turn. It is the single source of truth: frontend +callers pass `think_requested=False` by default and defer to this function, +while explicit `think_requested=True` acts as an override for curated +analytical entry points. + +These tests lock in the content-based behavior so future tweaks don't +silently regress the short / long / keyword boundaries. +""" + +import pytest + +from fabledassistant.services.generation_task import ( + _LONG_MESSAGE_CHARS, + _SHORT_MESSAGE_CHARS, + _should_think, +) + + +class TestExplicitOverride: + def test_override_forces_on_for_empty(self): + assert _should_think("", think_requested=True) is True + + def test_override_forces_on_for_short_greeting(self): + assert _should_think("hi", think_requested=True) is True + + def test_override_forces_on_for_medium_no_keyword(self): + text = "just checking in on the status of things for the week" + assert _should_think(text, think_requested=True) is True + + +class TestEmptyAndWhitespace: + def test_empty_string_off(self): + assert _should_think("", think_requested=False) is False + + def test_none_content_off(self): + # _should_think defensively handles None content from upstream callers + assert _should_think(None, think_requested=False) is False # type: ignore[arg-type] + + def test_whitespace_only_off(self): + assert _should_think(" \n\t ", think_requested=False) is False + + +class TestShortMessages: + def test_short_greeting_off(self): + assert _should_think("hi", think_requested=False) is False + + def test_short_thanks_off(self): + assert _should_think("thanks!", think_requested=False) is False + + def test_short_acknowledgement_off(self): + assert _should_think("ok sounds good", think_requested=False) is False + + def test_just_below_short_threshold_off(self): + text = "a" * (_SHORT_MESSAGE_CHARS - 1) + assert _should_think(text, think_requested=False) is False + + +class TestLongMessages: + def test_at_long_threshold_on(self): + text = "a" * _LONG_MESSAGE_CHARS + assert _should_think(text, think_requested=False) is True + + def test_well_above_long_threshold_on(self): + text = "x" * (_LONG_MESSAGE_CHARS * 3) + assert _should_think(text, think_requested=False) is True + + +class TestMediumMessages: + def test_medium_no_keyword_off(self): + # Between the short and long thresholds with no reasoning cue. + text = "a" * ((_SHORT_MESSAGE_CHARS + _LONG_MESSAGE_CHARS) // 2) + assert _should_think(text, think_requested=False) is False + + +class TestKeywordTriggers: + @pytest.mark.parametrize( + "text", + [ + "why is this failing", + "how does caching work here", + "how do i configure this", + "explain the retry logic", + "analyze the latency breakdown", + "compare gemma3 vs qwen3 for tool use", + "please design the schema for X", + "debug this error", + "troubleshoot the connection issue", + "root cause the outage", + "review this PR", + "critique my approach", + "walk me through the flow", + "step by step instructions please", + "pros and cons of each option", + "help me figure out what's wrong", + "discuss this article", # covers briefing /discuss entry points + ], + ) + def test_keyword_forces_on(self, text): + assert _should_think(text, think_requested=False) is True + + def test_keyword_case_insensitive(self): + assert _should_think("WHY does this break?", think_requested=False) is True + + def test_keyword_in_longer_sentence(self): + text = "hey quick one — can you explain what caching does for qwen3" + assert _should_think(text, think_requested=False) is True + + +class TestNonTriggers: + """Messages that look chatty and should NOT trigger thinking.""" + + @pytest.mark.parametrize( + "text", + [ + "hey", + "yep", + "no worries", + "got it, thanks", + "good morning", + "remind me later", # no reasoning keyword, short + ], + ) + def test_chatty_messages_off(self, text): + assert _should_think(text, think_requested=False) is False