From 986855481b5d462626788b4d74a5b1e0fbd1e1c1 Mon Sep 17 00:00:00 2001 From: hylin Date: Thu, 1 Oct 2026 03:04:46 +0800 Subject: [PATCH] fix(worker): fall back to thinking field; strip JSON fences before reflection parse (#38) Bug 1: ollama_chat only fell back to 'reasoning' when 'response' was empty, but glm-5.x-family models return chain-of-thought under 'thinking' (verified on Ollama Cloud). With a low num_predict every token went to thinking, response stayed empty, and reflection jobs failed even though the model completed fine. Bug 2: reflect_on_memories called json.loads(response) directly. Most current chat models wrap JSON in markdown fences even when asked for raw JSON, so json.loads failed and the reflection was stored as {'raw': ...} - the structured fields (patterns/connections/insights/ actions) were lost and the point degraded to an opaque blob. Extract _extract_response() (response -> reasoning -> thinking) and _strip_json_fences() helpers and use them at both call sites. New pytest module under docker/worker/tests covers both helpers plus a regression test proving a fenced response now lands as structured JSON instead of a raw blob. --- docker/worker/services/llm.py | 24 ++++-- docker/worker/tasks/reflection.py | 23 +++++- docker/worker/tests/test_llm_reflection.py | 92 ++++++++++++++++++++++ 3 files changed, 131 insertions(+), 8 deletions(-) create mode 100644 docker/worker/tests/test_llm_reflection.py diff --git a/docker/worker/services/llm.py b/docker/worker/services/llm.py index 8baf1b7..9a9874f 100644 --- a/docker/worker/services/llm.py +++ b/docker/worker/services/llm.py @@ -48,10 +48,20 @@ async def ollama_chat(prompt: str, model: str | None = None, timeout: int = 120) resp.raise_for_status() data = resp.json() - # DeepSeek v4 flash: reasoning can consume tokens, leaving response empty - # Return reasoning if content is empty - response = data.get("response", "") - if not response and "reasoning" in data: - response = data["reasoning"] - - return response + return _extract_response(data) + + +def _extract_response(data: dict) -> str: + """Response body -> reply text. + + Reasoning models can consume the whole budget on chain-of-thought, leaving + `response` empty. DeepSeek puts that under `reasoning`; glm-5.x-family + models name it `thinking` (verified on Ollama Cloud). Either is better than + returning nothing — the caller would otherwise fail even though the model + completed fine (#38). + """ + return ( + data.get("response", "") + or data.get("reasoning", "") + or data.get("thinking", "") + ) diff --git a/docker/worker/tasks/reflection.py b/docker/worker/tasks/reflection.py index 69e0a26..0a22b7d 100644 --- a/docker/worker/tasks/reflection.py +++ b/docker/worker/tasks/reflection.py @@ -34,6 +34,27 @@ """ + + +def _strip_json_fences(response: str) -> str: + """Strip one markdown code fence around a JSON payload. + + Most current chat models wrap JSON in ```json fences even when the prompt + asks for raw JSON. json.loads fails on the opening backticks, the + JSONDecodeError branch fires, and the reflection is stored as {"raw": ...} + — the structured fields (patterns/connections/insights/actions) are lost + and the point degrades to an opaque blob (#38). + """ + stripped = response.strip() + if stripped.startswith("```"): + first_newline = stripped.find("\n") + if first_newline != -1: + stripped = stripped[first_newline + 1:] + if stripped.rstrip().endswith("```"): + stripped = stripped.rstrip()[:-3] + return stripped + + async def reflect_on_memories(qdrant: AsyncQdrantClient) -> dict: """ Runs a reflection cycle on unreflected or old memories. @@ -74,7 +95,7 @@ async def reflect_on_memories(qdrant: AsyncQdrantClient) -> dict: try: response = await ollama_chat(prompt) - reflection_data = json.loads(response) + reflection_data = json.loads(_strip_json_fences(response)) except json.JSONDecodeError: logger.warning("Reflection returned invalid JSON, saving raw") reflection_data = {"raw": response} diff --git a/docker/worker/tests/test_llm_reflection.py b/docker/worker/tests/test_llm_reflection.py new file mode 100644 index 0000000..84b55f5 --- /dev/null +++ b/docker/worker/tests/test_llm_reflection.py @@ -0,0 +1,92 @@ +"""Unit tests for the #38 fixes: thinking-fallback in the Ollama client and +markdown-fence stripping before reflection JSON parsing. + +The docker worker has no pytest infrastructure (the repo uses script-style +_test_*.py files), so this file adds one deliberately tiny pytest module +scoped to the two changed functions. +""" +from __future__ import annotations + +import json +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from services.llm import _extract_response +from tasks.reflection import _strip_json_fences, reflect_on_memories + + +def test_extract_response_prefers_content(): + data = {"response": "hello", "reasoning": "chain", "thinking": "thoughts"} + assert _extract_response(data) == "hello" + + +def test_extract_response_falls_back_to_reasoning(): + data = {"response": "", "reasoning": "chain-of-thought"} + assert _extract_response(data) == "chain-of-thought" + + +def test_extract_response_falls_back_to_thinking(): + """glm-5.x-style models return chain-of-thought under `thinking`, with + `response` empty — the case #38 reports on Ollama Cloud.""" + data = {"response": "", "thinking": "thoughts", "done_reason": "length"} + assert _extract_response(data) == "thoughts" + + +def test_extract_response_empty_when_nothing_useful(): + assert _extract_response({}) == "" + assert _extract_response({"response": "", "reasoning": None}) == "" + + +def test_strip_json_fences_passthrough_plain_json(): + body = '{"patterns": []}' + assert _strip_json_fences(body) == '{"patterns": []}' + + +def test_strip_json_fences_language_tagged(): + body = '```json\n{"patterns": ["a"]}\n```' + assert json.loads(_strip_json_fences(body)) == {"patterns": ["a"]} + + +def test_strip_json_fences_bare_fence_and_trailing_space(): + body = '```\n{"insights": ["x"]}\n``` ' + assert json.loads(_strip_json_fences(body)) == {"insights": ["x"]} + + +@pytest.mark.asyncio +async def test_reflection_parses_fenced_json_instead_of_raw_blob(monkeypatch): + """Regression for #38 Bug 2: before the fix, a fenced response failed + json.loads and the reflection was stored as {"raw": ...}, losing the + structured fields.""" + captured = {} + + class FakeClient: + async def scroll(self, **kwargs): + class Point: + id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + payload = {"source": "chat", "text": "user said hello"} + return ([Point()], None) + + async def upsert(self, collection_name, points, **kwargs): + captured["point"] = points[0] + + async def set_payload(self, **kwargs): + return None + + async def fake_ollama(prompt): + return '```json\n{"patterns": ["greetings"], "insights": ["hello"], "actions": [], "connections": []}\n```' + + async def fake_embedding(text): + return [0.1, 0.2, 0.3] + + monkeypatch.setattr("tasks.reflection.ollama_chat", fake_ollama) + monkeypatch.setattr("tasks.reflection.get_embedding", fake_embedding) + + await reflect_on_memories(FakeClient()) + + payload = captured["point"].payload + assert "patterns" in payload["text"], "reflection degraded to a raw blob" + assert payload["source"] == "reflection"