From 87c04e28579900f8d760ab02c14759de23d993da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Boschi?= Date: Tue, 28 Jul 2026 10:51:49 +0200 Subject: [PATCH 1/2] fix(reflect): fail on unusable tool calls instead of salvaging leaked text Reflect is driven by structured tool calls. Some provider transports don't actually support function calling and silently strip the tool definitions from the request (e.g. litellm's Vertex AI gpt-oss MaaS path drops tools/tool_choice when the model is flagged unsupported). The model then answers in free text that mimics a done() payload, which landed in message.content with empty tool_calls. The old code served that raw text as the answer, so a growing pile of regex/JSON "strippers" tried to claw the leaked memory_ids/observation_ids/directive_compliance siblings back out of the user-facing answer. Instead of salvaging untooled text, fail loudly: - Track whether the model ever produced a tool call reflect could parse. If it never does (the stripped-tools case), raise ReflectToolCallError -> HTTP 500 (the request is valid; the server's configured model can't do the job) with a clear message (provider, model, response snippet). - Keep the done tool; _process_done_tool now trusts args["answer"] verbatim. A parsed tool call can't bleed its sibling id fields into the answer string. - A model that DID tool-call and later stops with text is a legitimate stop and still routes through the clean forced-final synthesis path. - Delete the entire strip zoo: _clean_done_answer, _unwrap_leaked_done_arguments, _strip_trailing_id_json_object, _clean_answer_text, _DONE_CALL_PATTERN, and the leaked-JSON regexes/key-sets. The forced-final paths return the model's prose directly (tools are disabled there, so there is no tool syntax to strip). No static supports_function_calling gate -- reflect just tries and fails. Supersedes the answer-salvage approach in #2972. --- hindsight-api-slim/hindsight_api/api/http.py | 7 + .../hindsight_api/engine/reflect/__init__.py | 3 +- .../hindsight_api/engine/reflect/agent.py | 276 ++++------------- .../tests/test_reflect_agent.py | 286 ++++-------------- 4 files changed, 120 insertions(+), 452 deletions(-) diff --git a/hindsight-api-slim/hindsight_api/api/http.py b/hindsight-api-slim/hindsight_api/api/http.py index 711b7ead6a..1e4dd23d1f 100644 --- a/hindsight-api-slim/hindsight_api/api/http.py +++ b/hindsight-api-slim/hindsight_api/api/http.py @@ -157,6 +157,7 @@ def FieldWithDefault(default_factory: Callable, **kwargs) -> Any: _get_tiktoken_encoding, ) from hindsight_api.engine.providers.none_llm import LLMNotAvailableError +from hindsight_api.engine.reflect import ReflectToolCallError from hindsight_api.engine.response_models import ( VALID_RECALL_FACT_TYPES, DryRunExtractionResult, @@ -4431,6 +4432,12 @@ async def api_reflect( raise except LLMNotAvailableError as e: raise HTTPException(status_code=400, detail=str(e)) + except ReflectToolCallError as e: + # The configured model/transport can't drive reflect's tool-calling loop. + # The request itself is fine, so this is a server-side (500) failure, not a + # 4xx -- but log at warning, not error: it's a misconfiguration, not a bug. + logger.warning("Reflect tool-calling failure in bank %s: %s", bank_id, e) + raise HTTPException(status_code=500, detail=str(e)) except TimeoutError as e: logger.error("Timeout in /v1/default/banks/%s/reflect: %s", bank_id, e) raise HTTPException( diff --git a/hindsight-api-slim/hindsight_api/engine/reflect/__init__.py b/hindsight-api-slim/hindsight_api/engine/reflect/__init__.py index 52680a2af1..83147ee9e2 100644 --- a/hindsight-api-slim/hindsight_api/engine/reflect/__init__.py +++ b/hindsight-api-slim/hindsight_api/engine/reflect/__init__.py @@ -7,12 +7,13 @@ 3. Expand memories (get chunk/document context) """ -from .agent import ReflectAgentResult, run_reflect_agent +from .agent import ReflectAgentResult, ReflectToolCallError, run_reflect_agent from .models import ReflectAction, ReflectActionBatch __all__ = [ "run_reflect_agent", "ReflectAgentResult", + "ReflectToolCallError", "ReflectAction", "ReflectActionBatch", ] diff --git a/hindsight-api-slim/hindsight_api/engine/reflect/agent.py b/hindsight-api-slim/hindsight_api/engine/reflect/agent.py index f67185a787..11421b5abc 100644 --- a/hindsight-api-slim/hindsight_api/engine/reflect/agent.py +++ b/hindsight-api-slim/hindsight_api/engine/reflect/agent.py @@ -10,7 +10,6 @@ import asyncio import json import logging -import re import time from typing import TYPE_CHECKING, Any, Awaitable, Callable @@ -56,6 +55,20 @@ def _build_directives_applied(directives: list[dict[str, Any]] | None) -> list[D NO_ANSWER_TEXT = "No answer provided." +class ReflectToolCallError(RuntimeError): + """The model never produced a tool call reflect could understand. + + Reflect is driven entirely by structured tool calls (``recall``, ``expand``, + ``done`` ...). Some provider transports do not actually support function + calling and silently drop the tool definitions from the request (e.g. litellm's + Vertex AI gpt-oss MaaS path strips ``tools``/``tool_choice`` when the model is + flagged as not supporting them). The model then answers in free text that may + mimic a ``done`` payload. Rather than salvage that untooled text -- and risk + surfacing raw tool-call JSON as the answer -- we fail loudly so the caller can + switch to a tool-calling-capable model/transport. + """ + + def _normalize_tool_name(name: str) -> str: """Normalize tool name from various LLM output formats. @@ -88,143 +101,6 @@ def _is_done_tool(name: str) -> bool: return _normalize_tool_name(name) == "done" -# Pattern to match done() call as text - handles done({...}) with nested JSON -_DONE_CALL_PATTERN = re.compile(r"done\s*\(\s*\{.*$", re.DOTALL) - -# Patterns for leaked structured output in the answer field -_LEAKED_JSON_SUFFIX = re.compile( - r'\s*```(?:json)?\s*\{[^}]*(?:"(?:observation_ids|memory_ids|mental_model_ids)"|\})\s*```\s*$', - re.DOTALL | re.IGNORECASE, -) -_TRAILING_IDS_PATTERN = re.compile( - r"\s*(?:observation_ids|memory_ids|mental_model_ids)\s*[=:]\s*\[.*?\]\s*$", re.DOTALL | re.IGNORECASE -) -_JSON_CODE_FENCE_PATTERN = re.compile(r"^\s*```(?:json)?\s*(\{.*\})\s*```\s*$", re.DOTALL | re.IGNORECASE) - -_DONE_ARGUMENT_KEYS = frozenset( - { - "answer", - "directive_compliance", - "memory_ids", - "mental_model_ids", - "observation_ids", - "model_ids", - } -) -_DONE_ARGUMENT_MARKER_KEYS = _DONE_ARGUMENT_KEYS - {"answer"} -_LEAKED_JSON_ID_KEYS = frozenset({"memory_ids", "mental_model_ids", "observation_ids", "model_ids"}) - - -def _unwrap_leaked_done_arguments(text: str) -> str | None: - """Return the answer when a done tool call was rendered as JSON text. - - Some providers leak the done tool's argument object instead of surfacing it - as a native tool call, e.g. {"answer": "...", "memory_ids": [...]}. Only - unwrap objects that match the done argument shape so normal JSON answers - stay intact. - """ - candidate = text.strip() - if not candidate: - return None - - fenced = _JSON_CODE_FENCE_PATTERN.match(candidate) - if fenced: - candidate = fenced.group(1).strip() - - try: - payload = json.loads(candidate) - except json.JSONDecodeError: - return None - - if not isinstance(payload, dict): - return None - answer = payload.get("answer") - if not isinstance(answer, str) or not answer.strip(): - return None - - keys = set(payload) - if not keys.intersection(_DONE_ARGUMENT_MARKER_KEYS): - return None - if not keys.issubset(_DONE_ARGUMENT_KEYS): - return None - - for key in ("memory_ids", "mental_model_ids", "observation_ids", "model_ids"): - value = payload.get(key) - if value is not None and not isinstance(value, list): - return None - - return answer.strip() - - -def _strip_trailing_id_json_object(text: str) -> str: - stripped = text.rstrip() - if not stripped.endswith("}"): - return text.strip() - - start = stripped.rfind("{") - if start < 0: - return text.strip() - - try: - payload = json.loads(stripped[start:]) - except json.JSONDecodeError: - return text.strip() - - if not isinstance(payload, dict) or not payload: - return text.strip() - keys = set(payload) - if not keys.issubset(_LEAKED_JSON_ID_KEYS): - return text.strip() - - return stripped[:start].strip() - - -def _clean_answer_text(text: str) -> str: - """Clean up answer text by removing any done() tool call syntax. - - Some LLMs output the done() call as text instead of a proper tool call. - This strips out patterns like: done({"answer": "...", ...}) - """ - unwrapped = _unwrap_leaked_done_arguments(text) - if unwrapped is not None: - return unwrapped - - # Remove done() call pattern from the end of the text - cleaned = _DONE_CALL_PATTERN.sub("", text).strip() - return cleaned if cleaned else text - - -def _clean_done_answer(text: str) -> str: - """Clean up the answer field from a done() tool call. - - Some LLMs leak structured output patterns into the answer text, such as: - - JSON code blocks with observation_ids/memory_ids at the end - - Raw JSON objects with these fields - - Plain text like "observation_ids: [...]" - - This cleans those patterns while preserving the actual answer content. - """ - if not text: - return text - - unwrapped = _unwrap_leaked_done_arguments(text) - if unwrapped is not None: - return unwrapped - - cleaned = text - - # Remove leaked JSON in code blocks at the end - cleaned = _LEAKED_JSON_SUFFIX.sub("", cleaned).strip() - - # Remove leaked raw JSON objects at the end - cleaned = _strip_trailing_id_json_object(cleaned) - - # Remove trailing ID patterns - cleaned = _TRAILING_IDS_PATTERN.sub("", cleaned).strip() - - return cleaned if cleaned else text - - async def _generate_structured_output( answer: str, response_schema: dict, @@ -679,6 +555,10 @@ def _schedule_cache(upto: int) -> None: # Tracking total_tools_called = 0 + # Whether the model has ever produced a tool call reflect could understand. + # Stays False when a transport silently strips tool support (the model then + # only ever returns free text) -- that case fails via ReflectToolCallError. + saw_tool_call = False tool_trace: list[ToolCall] = [] tool_trace_summary: list[dict[str, Any]] = [] llm_trace: list[dict[str, Any]] = [] @@ -792,7 +672,7 @@ def _log_completion(answer: str, iterations: int, forced: bool = False): "output_tokens": usage.output_tokens, } ) - answer = _clean_answer_text(response.strip()) + answer = response.strip() # Generate structured output if schema provided structured_output = None @@ -857,7 +737,7 @@ def _log_completion(answer: str, iterations: int, forced: bool = False): "output_tokens": usage.output_tokens, } ) - answer = _clean_answer_text(response.strip()) + answer = response.strip() structured_output = None if response_schema and answer: @@ -1000,7 +880,7 @@ def _log_completion(answer: str, iterations: int, forced: bool = False): "output_tokens": usage.output_tokens, } ) - answer = _clean_answer_text(response.strip()) + answer = response.strip() # Generate structured output if schema provided structured_output = None @@ -1024,85 +904,29 @@ def _log_completion(answer: str, iterations: int, forced: bool = False): directives_applied=directives_applied, ) - # No tool calls - LLM wants to respond with text + # No tool calls this turn. if not result.tool_calls: - # When directives are present but no evidence has been gathered, - # the LLM tends to echo directive content verbatim as its answer. - # Fall through to the final-prompt path which doesn't include - # directives and handles "no data" gracefully. - has_gathered_evidence = ( - bool(available_memory_ids) or bool(available_mental_model_ids) or bool(available_observation_ids) - ) - directive_leak_risk = directives and not has_gathered_evidence - if result.content and not directive_leak_risk: - answer = _clean_answer_text(result.content.strip()) - - # The call_with_tools call above is intentionally uncapped so the - # LLM has headroom to emit tool-call JSON plus any intermediate - # reasoning. But when the LLM short-circuits and returns text - # directly, that text becomes the user-visible final answer and - # must respect max_tokens like the forced-final paths do. If it - # overshoots, run one extra capped call to rewrite it within - # the cap. - if max_tokens is not None and count_cl100k_tokens(answer) > max_tokens: - rewrite_start = time.time() - rewritten, rewrite_usage = await llm_config.call( - messages=[ - { - "role": "system", - "content": ( - "Rewrite the user's text so it fits within the requested token " - "budget. Preserve the key facts and structure; drop lower-priority " - "detail. Respond with the rewritten text only, no preamble." - ), - }, - { - "role": "user", - "content": f"Target budget: {max_tokens} tokens.\n\nText to rewrite:\n{answer}", - }, - ], - scope="reflect", - max_completion_tokens=max_tokens, - return_usage=True, - ) - total_input_tokens += rewrite_usage.input_tokens - total_output_tokens += rewrite_usage.output_tokens - total_cached_tokens += getattr(rewrite_usage, "cached_tokens", 0) or 0 - total_thoughts_tokens += getattr(rewrite_usage, "thoughts_tokens", 0) or 0 - llm_trace.append( - { - "scope": "final_rewrite", - "duration_ms": int((time.time() - rewrite_start) * 1000), - "input_tokens": rewrite_usage.input_tokens, - "output_tokens": rewrite_usage.output_tokens, - } - ) - answer = _clean_answer_text(rewritten.strip()) - - # Generate structured output if schema provided - structured_output = None - if response_schema and answer: - struct = await _generate_structured_output( - answer, response_schema, llm_config, reflect_id, max_tokens - ) - structured_output = struct.structured_output - total_input_tokens += struct.input_tokens - total_output_tokens += struct.output_tokens - total_cached_tokens += struct.cached_tokens - total_thoughts_tokens += struct.thoughts_tokens - - _log_completion(answer, iteration + 1) - return ReflectAgentResult( - text=answer, - structured_output=structured_output, - iterations=iteration + 1, - tools_called=total_tools_called, - tool_trace=tool_trace, - llm_trace=_get_llm_trace(), - usage=_get_usage(), - directives_applied=directives_applied, + # Reflect is driven by structured tool calls. A turn with no tool call + # means one of two things: + # * the model already gathered evidence via earlier tool calls and is + # now stopping -- fine, synthesize a clean final answer below; + # * the transport can't produce tool calls at all, so it only ever + # returns free text (e.g. litellm strips tools on the Vertex gpt-oss + # MaaS path). In that case ``saw_tool_call`` is still False. + # We no longer salvage that free text as the answer -- it can be a raw + # done()-payload with sibling id fields leaking into user-visible text. + # Fail loudly instead so the caller picks a tool-calling-capable model. + if not saw_tool_call: + snippet = (result.content or "").strip() + if len(snippet) > 500: + snippet = snippet[:500] + "..." + detail = f" Response: {snippet!r}" if snippet else " The model returned no content." + raise ReflectToolCallError( + f"Reflect requires a tool-calling model, but {llm_config.provider}/{llm_config.model} " + f"produced no usable tool call (the transport may not support function calling)." + detail ) - # Empty response, force final + # Model tool-called earlier and is now stopping: fall through to a clean + # forced final synthesis (tools disabled, prose expected). prompt = build_final_prompt( query, context_history, bank_profile, context, max_context_tokens=max_context_tokens ) @@ -1134,7 +958,7 @@ def _log_completion(answer: str, iterations: int, forced: bool = False): "output_tokens": usage.output_tokens, } ) - answer = _clean_answer_text(response.strip()) + answer = response.strip() # Generate structured output if schema provided structured_output = None @@ -1158,6 +982,11 @@ def _log_completion(answer: str, iterations: int, forced: bool = False): directives_applied=directives_applied, ) + # The model produced at least one tool call reflect could parse: it can + # drive the loop, so a later text-only turn is a legitimate stop, not a + # broken transport. + saw_tool_call = True + # Check for done tool call (handle various LLM output formats) done_call = next((tc for tc in result.tool_calls if _is_done_tool(tc.name)), None) if done_call: @@ -1433,9 +1262,10 @@ async def _process_done_tool( """Process the done tool call and return the result.""" args = done_call.arguments - # Extract and clean the answer - some LLMs leak structured output into the answer text - raw_answer = args.get("answer", "").strip() - answer = _clean_done_answer(raw_answer) if raw_answer else "" + # ``done`` is a structured tool call: trust its ``answer`` field verbatim. + # Sibling id fields (memory_ids, ...) live in their own arguments and are + # validated separately below -- they can't bleed into a parsed answer string. + answer = args.get("answer", "").strip() if not answer: answer = NO_ANSWER_TEXT @@ -1461,7 +1291,7 @@ async def _process_done_tool( max_completion_tokens=max_tokens, return_usage=True, ) - answer = _clean_answer_text(rewritten.strip()) + answer = rewritten.strip() final_usage = TokenUsageSummary( input_tokens=usage.input_tokens + rewrite_usage.input_tokens, output_tokens=usage.output_tokens + rewrite_usage.output_tokens, diff --git a/hindsight-api-slim/tests/test_reflect_agent.py b/hindsight-api-slim/tests/test_reflect_agent.py index 9f072b2466..b8bdaa1f38 100644 --- a/hindsight-api-slim/tests/test_reflect_agent.py +++ b/hindsight-api-slim/tests/test_reflect_agent.py @@ -15,10 +15,9 @@ from hindsight_api.engine.llm_interface import LLM_TOOL_CHOICE_AUTO, LLMToolChoice from hindsight_api.engine.reflect.agent import ( + ReflectToolCallError, _all_mental_models_are_usable_and_fresh, _cache_cleanup_tasks, - _clean_answer_text, - _clean_done_answer, _count_messages_tokens, _generate_structured_output, _is_context_overflow_error, @@ -30,171 +29,6 @@ from tests.llm_judge import assert_meets_criteria -class TestCleanAnswerText: - """Test cleanup of answer text that includes done() tool call syntax.""" - - def test_clean_text_with_done_call(self): - """Text ending with done() call should have it stripped.""" - text = """The team's OKRs focus on performance.done({"answer":"The team's OKRs","memory_ids":[]})""" - cleaned = _clean_answer_text(text) - assert cleaned == "The team's OKRs focus on performance." - assert "done(" not in cleaned - - def test_clean_text_with_done_call_and_whitespace(self): - """done() call with whitespace should be stripped.""" - text = """Answer text here. done( {"answer": "short", "memory_ids": []} )""" - cleaned = _clean_answer_text(text) - assert cleaned == "Answer text here." - - def test_clean_text_without_done_call(self): - """Text without done() call should be unchanged.""" - text = "This is a normal answer without any tool calls." - cleaned = _clean_answer_text(text) - assert cleaned == text - - def test_clean_text_with_done_word_in_content(self): - """The word 'done' in regular text should not be stripped.""" - text = "The task is done and completed successfully." - cleaned = _clean_answer_text(text) - assert cleaned == text - - def test_clean_empty_text(self): - """Empty text should return empty.""" - assert _clean_answer_text("") == "" - - def test_clean_text_multiline_done(self): - """done() call spanning multiple lines should be stripped.""" - text = """Summary of findings.done({ - "answer": "Summary", - "memory_ids": ["id1", "id2"] - })""" - cleaned = _clean_answer_text(text) - assert cleaned == "Summary of findings." - - def test_clean_text_recovers_leaked_done_arguments(self): - """A done tool-call argument object rendered as text should keep only answer.""" - text = """{ - "answer": "Use the inbound table for API consumers.", - "directive_compliance": "Directive 1 followed.", - "memory_ids": ["mem-1"], - "mental_model_ids": [], - "observation_ids": [] - }""" - cleaned = _clean_answer_text(text) - assert cleaned == "Use the inbound table for API consumers." - assert "directive_compliance" not in cleaned - - def test_clean_text_leaves_non_done_json_answer_unchanged(self): - """Plain JSON answers are valid user-visible content.""" - text = '{"status": "ok", "items": [1, 2]}' - cleaned = _clean_answer_text(text) - assert cleaned == text - - -class TestCleanDoneAnswer: - """Test cleanup of answer field from done() tool call that leaks structured output.""" - - def test_clean_answer_with_leaked_json_code_block(self): - """Answer with leaked JSON code block at the end should be cleaned.""" - text = """The user's favorite color is blue. - -```json -{"observation_ids": ["obs-1", "obs-2"]} -```""" - cleaned = _clean_done_answer(text) - assert cleaned == "The user's favorite color is blue." - assert "observation_ids" not in cleaned - - def test_clean_answer_with_memory_ids_code_block(self): - """Answer with leaked memory_ids JSON code block should be cleaned.""" - text = """Here is the answer. - -```json -{"memory_ids": ["mem-1"]} -```""" - cleaned = _clean_done_answer(text) - assert cleaned == "Here is the answer." - - def test_clean_answer_with_raw_json_object(self): - """Answer with raw JSON object containing IDs at the end should be cleaned.""" - text = 'The answer is 42. {"observation_ids": ["obs-1"]}' - cleaned = _clean_done_answer(text) - assert cleaned == "The answer is 42." - - def test_clean_answer_with_trailing_ids_pattern(self): - """Answer with 'observation_ids: [...]' pattern at the end should be cleaned.""" - text = 'This is the answer.\n\nobservation_ids: ["obs-1", "obs-2"]' - cleaned = _clean_done_answer(text) - assert cleaned == "This is the answer." - - def test_clean_answer_with_memory_ids_equals(self): - """Answer with 'memory_ids = [...]' pattern at the end should be cleaned.""" - text = 'Answer text here.\nmemory_ids = ["mem-1"]' - cleaned = _clean_done_answer(text) - assert cleaned == "Answer text here." - - def test_clean_normal_answer_unchanged(self): - """Normal answer without leaked output should be unchanged.""" - text = "This is a normal answer about observation strategies." - cleaned = _clean_done_answer(text) - assert cleaned == text - - def test_clean_empty_answer(self): - """Empty answer should return empty.""" - assert _clean_done_answer("") == "" - - def test_clean_answer_with_observation_word_in_content(self): - """The word 'observation' in regular text should not be stripped.""" - text = "Based on my observation, the user prefers dark mode." - cleaned = _clean_done_answer(text) - assert cleaned == text - - def test_clean_answer_multiline_with_markdown(self): - """Answer with markdown and leaked JSON at end should clean only the leak.""" - text = """Summary: -- Point 1 -- Point 2 - -```json -{"mental_model_ids": ["mm-1"]} -```""" - cleaned = _clean_done_answer(text) - assert "Point 1" in cleaned - assert "Point 2" in cleaned - assert "mental_model_ids" not in cleaned - - def test_clean_answer_recovers_leaked_done_arguments(self): - """A done answer that contains leaked done arguments should keep answer content.""" - text = """{ - "answer": "Render two markdown tables: inbound and outbound.", - "directive_compliance": "All directives followed.", - "memory_ids": ["mem-1", "mem-2"], - "mental_model_ids": [], - "observation_ids": ["obs-1"] - }""" - cleaned = _clean_done_answer(text) - assert cleaned == "Render two markdown tables: inbound and outbound." - - def test_clean_answer_recovers_fenced_leaked_done_arguments(self): - """Some providers put leaked done arguments in a JSON code fence.""" - text = """```json -{ - "answer": "The current interface is HTTP only.", - "memory_ids": [], - "mental_model_ids": [], - "observation_ids": [] -} -```""" - cleaned = _clean_done_answer(text) - assert cleaned == "The current interface is HTTP only." - - def test_clean_answer_rejects_done_arguments_with_unexpected_keys(self): - """Avoid rewriting user-requested JSON that happens to contain answer.""" - text = '{"answer": "yes", "payload": {"format": "json"}, "memory_ids": []}' - cleaned = _clean_done_answer(text) - assert cleaned == text - - class TestToolNameNormalization: """Test tool name normalization for various LLM output formats.""" @@ -484,6 +318,40 @@ async def test_done_tool_answer_respects_max_tokens(self, mock_llm, mock_functio assert result.usage.total_tokens == 150 assert result.llm_trace[-1].scope == "final_rewrite" + @pytest.mark.asyncio + async def test_no_tool_call_ever_raises_tool_call_error(self, mock_llm, mock_functions): + """A transport that strips tool support (never yields a tool call) fails loudly. + + This is the harmony/gpt-oss-via-Vertex-MaaS case: the model returns free text + that mimics a done() payload with sibling id fields. We must NOT salvage it as + the answer -- reflect raises ReflectToolCallError instead. + """ + mock_llm.provider = "litellm" + mock_llm.model = "vertex_ai/openai/gpt-oss-120b-maas" + leaked = '{"answer": "The user has a cat named Luna.", "memory_ids": ["mem-1"], "observation_ids": []}' + mock_llm.call_with_tools.side_effect = [ + LLMToolCallResult(content=leaked, tool_calls=[], finish_reason="stop"), + ] + + with pytest.raises(ReflectToolCallError) as exc_info: + await run_reflect_agent( + llm_config=mock_llm, + bank_id="test-bank", + query="what pets does the user have?", + bank_profile={"name": "Test", "mission": "Testing"}, + has_mental_models=True, + budget="low", + max_iterations=5, + **mock_functions, + ) + + msg = str(exc_info.value) + assert "vertex_ai/openai/gpt-oss-120b-maas" in msg + assert "no usable tool call" in msg + # The forced-final fallback (mock_llm.call) must NOT have run: we fail fast + # rather than synthesizing a hollow answer from zero evidence. + mock_llm.call.assert_not_called() + @pytest.mark.asyncio async def test_short_circuited_agent_may_still_retrieve_under_auto(self, mock_llm, mock_functions): """After release, the agent can still choose to retrieve deeper itself (its own query).""" @@ -842,82 +710,44 @@ async def test_normalizes_tool_names_in_other_tools(self, mock_llm, mock_functio mock_functions["recall_fn"].assert_called_once() @pytest.mark.asyncio - async def test_short_circuit_answer_is_capped_by_max_tokens(self, mock_llm, mock_functions): - """When the LLM short-circuits (returns text without calling a tool) and the text - exceeds max_tokens, the agent must rewrite it through a capped call so the final - user-visible answer respects the configured limit. + async def test_stop_after_evidence_uses_forced_final_synthesis(self, mock_llm, mock_functions): + """A model that tool-called at least once and then stops (no tool call) is a + legitimate completion: reflect does a clean forced final-synthesis call (tools + disabled) rather than salvaging free text or raising ReflectToolCallError. """ - # Build a long response that's well over the cap in cl100k_base tokens. - long_answer = " ".join( - ["This is a detailed paragraph about the team, their roles, and their recurring meetings."] * 80 - ) - # The short-circuit path: tool_calls empty, content populated. - mock_llm.call_with_tools.return_value = LLMToolCallResult( - tool_calls=[], - content=long_answer, - finish_reason="stop", - input_tokens=10, - output_tokens=500, - ) + mock_functions["search_mental_models_fn"].return_value = { + "mental_models": [{"id": "mm-1", "name": "Prefs", "content": "Fresh content.", "is_stale": False}] + } + mock_llm.call_with_tools.side_effect = [ + # Turn 0: a real tool call -> saw_tool_call becomes True. + self._mm_call(), + # Turn 1: model stops with plain text and no tool call. + LLMToolCallResult(tool_calls=[], content="I have enough to answer.", finish_reason="stop"), + ] mock_llm.call = AsyncMock( return_value=( - "Short rewritten answer.", - TokenUsage(input_tokens=50, output_tokens=10, total_tokens=60), + "Synthesized final answer.", + TokenUsage(input_tokens=40, output_tokens=12, total_tokens=52), ) ) - cap = 50 + cap = 64 result = await run_reflect_agent( llm_config=mock_llm, bank_id="test-bank", query="test query", bank_profile={"name": "Test", "mission": "Testing"}, + has_mental_models=True, + budget="low", max_tokens=cap, **mock_functions, ) - # The rewrite call must have been made, and it must carry the cap. - assert mock_llm.call.await_count == 1, ( - f"expected exactly one capped rewrite call, got {mock_llm.call.await_count}" - ) - rewrite_kwargs = mock_llm.call.await_args.kwargs - assert rewrite_kwargs.get("max_completion_tokens") == cap, ( - f"rewrite call should use max_completion_tokens={cap}, got {rewrite_kwargs.get('max_completion_tokens')}" - ) - - # The final answer is the rewritten text, not the oversized original. - assert result.text == "Short rewritten answer." - - # The trace records the rewrite step so we can see it was invoked. - assert any(entry.scope == "final_rewrite" for entry in result.llm_trace), ( - f"llm_trace should include a final_rewrite entry, got {result.llm_trace}" - ) - - @pytest.mark.asyncio - async def test_short_circuit_answer_under_cap_is_not_rewritten(self, mock_llm, mock_functions): - """If the short-circuit answer already fits within max_tokens, no extra rewrite - call should happen — we don't want to pay for a second LLM call in the common case. - """ - short_answer = "Small answer that already fits." - mock_llm.call_with_tools.return_value = LLMToolCallResult( - tool_calls=[], - content=short_answer, - finish_reason="stop", - input_tokens=10, - output_tokens=8, - ) - - result = await run_reflect_agent( - llm_config=mock_llm, - bank_id="test-bank", - query="test query", - bank_profile={"name": "Test", "mission": "Testing"}, - max_tokens=200, - **mock_functions, - ) - - assert result.text == short_answer - mock_llm.call.assert_not_called() + # Answer comes from the clean forced-final call, not the turn-1 free text. + assert result.text == "Synthesized final answer." + assert mock_llm.call.await_count == 1 + # The forced-final synthesis respects the token cap directly on the call. + assert mock_llm.call.await_args.kwargs["max_completion_tokens"] == cap @pytest.mark.asyncio async def test_max_iterations_reached(self, mock_llm, mock_functions): From 3b3511dcf9f4db6df55cc1f83767cf94407879e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Boschi?= Date: Tue, 28 Jul 2026 12:58:02 +0200 Subject: [PATCH 2/2] test(mock): drive the reflect loop via tool calls, not bare prose The reflect agent now rejects a turn that yields no usable tool call (ReflectToolCallError). MockLLM's default call_with_tools returned bare "mock response" content with no tool calls, which the old salvage path served as the answer -- so ~15 reflect integration tests (empty-bank, tracing, based_on, tags, think) started failing with 500 under the new guard. Make MockLLM simulate a compliant tool-calling provider in its default path: honor a forced retrieval tool_choice (so recall/search actually run and populate based_on), and otherwise finish via the done tool. Tests that script their own turns via _response_callback / _mock_response are unaffected. --- .../engine/providers/mock_llm.py | 59 ++++++++++++++++++- 1 file changed, 57 insertions(+), 2 deletions(-) diff --git a/hindsight-api-slim/hindsight_api/engine/providers/mock_llm.py b/hindsight-api-slim/hindsight_api/engine/providers/mock_llm.py index aee5645071..740d49ad1c 100644 --- a/hindsight-api-slim/hindsight_api/engine/providers/mock_llm.py +++ b/hindsight-api-slim/hindsight_api/engine/providers/mock_llm.py @@ -9,7 +9,7 @@ from collections.abc import Callable from typing import Any -from ..llm_interface import LLM_TOOL_CHOICE_AUTO, LLMInterface, LLMToolChoice +from ..llm_interface import LLM_TOOL_CHOICE_AUTO, LLMInterface, LLMToolChoice, LLMToolChoiceMode from ..response_models import LLMToolCall, LLMToolCallResult, TokenUsage logger = logging.getLogger(__name__) @@ -266,7 +266,7 @@ async def call_with_tools( else: result = LLMToolCallResult(content="mock response", finish_reason="stop") else: - result = LLMToolCallResult(content="mock response", finish_reason="stop") + result = self._compliant_tool_call(tools, tool_choice, messages) # Set mock token usage on result if not already set if result.input_tokens == 0: @@ -297,6 +297,61 @@ async def call_with_tools( return result + @staticmethod + def _compliant_tool_call( + tools: list[dict[str, Any]], + tool_choice: LLMToolChoice, + messages: list[dict[str, Any]], + ) -> LLMToolCallResult: + """Default tool response: simulate a compliant tool-calling model. + + Real providers drive the reflect loop entirely through tool calls -- they + honor a forced tool choice, then finish via ``done`` -- and the reflect + agent now rejects a turn that yields no tool call at all (a transport that + can't tool-call raises ReflectToolCallError). So the mock must behave like a + working provider here rather than returning bare "mock response" prose, + which used to be salvaged as the answer. Only this default path is affected; + tests that script turns via ``_response_callback`` / ``_mock_response`` are not. + """ + tool_names = {t.get("function", {}).get("name") for t in tools} + + def _mock_query() -> str: + for message in reversed(messages): + content = message.get("content") + if message.get("role") == "user" and isinstance(content, str) and content.strip(): + return content[:200] + return "mock query" + + # Honor a forced retrieval tool so the loop actually runs recall/search and + # gathers evidence (populates based_on for tests that assert on it). + if tool_choice.mode is LLMToolChoiceMode.NAMED and tool_choice.function_name in { + "search_mental_models", + "search_observations", + "recall", + }: + return LLMToolCallResult( + tool_calls=[ + LLMToolCall( + id="mock_forced", + name=tool_choice.function_name, + arguments={"reason": "mock", "query": _mock_query()}, + ) + ], + finish_reason="tool_calls", + ) + + # Auto turn: finish via the done tool, mirroring a model that has gathered + # enough. The reflect evidence guardrail handles the empty-bank case (no + # evidence -> forced text synthesis on the final iteration). + if "done" in tool_names: + return LLMToolCallResult( + tool_calls=[LLMToolCall(id="mock_done", name="done", arguments={"answer": "mock response"})], + finish_reason="tool_calls", + ) + + # No done tool offered (non-reflect tool call): fall back to plain text. + return LLMToolCallResult(content="mock response", finish_reason="stop") + @staticmethod def _build_mock_facts(messages: list[dict]) -> dict: """Build a canned fact extraction response from the user message text.