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/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. 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):