test(llm): lock in _should_think classifier; drop briefing think overrides
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 <noreply@anthropic.com>
This commit is contained in:
@@ -578,7 +578,6 @@ async def discuss_article(item_id: int):
|
|||||||
buf, history, model,
|
buf, history, model,
|
||||||
uid, conv_id, conv.title or "",
|
uid, conv_id, conv.title or "",
|
||||||
"Please summarize and discuss this article.",
|
"Please summarize and discuss this article.",
|
||||||
think=True,
|
|
||||||
))
|
))
|
||||||
|
|
||||||
return jsonify({"assistant_message_id": assistant_msg.id, "status": "generating"}), 202
|
return jsonify({"assistant_message_id": assistant_msg.id, "status": "generating"}), 202
|
||||||
@@ -677,7 +676,6 @@ async def discuss_topic(topic: str):
|
|||||||
buf, history, model,
|
buf, history, model,
|
||||||
uid, conv_id, conv.title or "",
|
uid, conv_id, conv.title or "",
|
||||||
user_prompt,
|
user_prompt,
|
||||||
think=True,
|
|
||||||
))
|
))
|
||||||
|
|
||||||
return jsonify({
|
return jsonify({
|
||||||
|
|||||||
@@ -67,6 +67,7 @@ _THINK_KEYWORDS: tuple[str, ...] = (
|
|||||||
"review", "critique", "evaluate", "trade-off", "tradeoff", "trade off",
|
"review", "critique", "evaluate", "trade-off", "tradeoff", "trade off",
|
||||||
"pros and cons", "step by step", "walk me through",
|
"pros and cons", "step by step", "walk me through",
|
||||||
"prove", "derive", "figure out", "work 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
|
# Messages shorter than this and without any think-keyword are treated as
|
||||||
|
|||||||
@@ -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
|
||||||
Reference in New Issue
Block a user