fix(reflect): fail on unusable tool calls instead of salvaging leaked text - #3013
Merged
Conversation
nicoloboschi
force-pushed
the
fix/reflect-fail-on-no-tool-call
branch
from
July 28, 2026 09:47
3796908 to
7419c64
Compare
… 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.
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.
nicoloboschi
force-pushed
the
fix/reflect-fail-on-no-tool-call
branch
from
July 28, 2026 11:14
f21f1fb to
3b3511d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Reflect is driven entirely by structured tool calls. Some provider transports don't actually support function calling and silently strip the tool definitions from the request — most notably litellm's Vertex AI gpt-oss MaaS path, whose transformer drops
tools/tool_choicewhen the model is flaggedsupports_function_calling=False. The model, never given the tools, answers in free text that mimics adone()payload, which lands inmessage.contentwith emptytool_calls.The old reflect loop served that raw text as the answer. To cope, it grew an ever-expanding pile of regex/JSON "strippers" (
_clean_done_answer,_unwrap_leaked_done_arguments,_strip_trailing_id_json_object,_clean_answer_text,_LEAKED_JSON_SUFFIX,_TRAILING_IDS_PATTERN, …) to claw the leakedmemory_ids/observation_ids/directive_compliancesiblings back out of the user-facing answer — each new provider quirk = another pattern (#2222, #2345, #2757, and the pending #2972).Verification
Reproduced against real
gpt-oss:120b-cloud:donetool call parses cleanly every time — structuredtool_calls, empty content, no leak.vertex_ai/vertex_ai_partner_models/gpt_oss/transformation.pystrips the tool params for gpt-oss MaaS. It's not a model limitation — the same model tool-calls perfectly on a transport that supports it.What changed
Fail loudly instead of salvaging untooled text:
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).donetool._process_done_toolnow trustsargs["answer"]verbatim — a parsed tool call can't bleed its sibling id fields into the answer string._clean_answer_text/_DONE_CALL_PATTERN. The forced-final paths return the model's prose directly — tools are disabled there, so there's no tool syntax to strip.No static
supports_function_callinggate — reflect just tries and fails. It's reliable because iteration 0 forces a named tool choice, so any compliant provider yields a tool call and never trips the guard; only a transport that strips tools fails, fast, on turn 0.Note for downstream clients
Reflect can now return HTTP 500 for a model/transport that can't drive tool calls, where it previously returned 200 with leaked or hollow text. No route/request/response schema changed, so no OpenAPI/client regeneration is required.
Checks
ruff check/ruff format --check/ty check— cleanpytest tests/test_reflect_agent.py(non-LLM) — 39 passedtest_no_tool_call_ever_raises_tool_call_errorandtest_stop_after_evidence_uses_forced_final_synthesistest_directive_not_echoed_on_empty_bankerrors locally on a shared-pg0 port conflict (pre-existing, environment), unrelated to this changeSupersedes the answer-salvage approach in #2972.