fix: leave empty-string tool results in context instead of offloading them - #12585
Conversation
What changed:
- ToolResultOffloadHook._maybe_offload now tests the raw tool result for
emptiness before a string result is wrapped in a TextContent block, so ""
is left in context exactly like [] already was
- Dropped the now-unreachable `if not content_blocks` guard: past the new
check the result is truthy, and both arms of
`str | Sequence[TextContent | ImageContent | FileContent]` are truthy only
when they yield at least one block
- Parametrized test_empty_result_is_not_offloaded over "" and []
- Added a release note
Why:
_maybe_offload documents that a message is left as-is "when the result is
empty", but the guard ran after a str result had already been wrapped into
[TextContent("")], so only the empty-sequence shape ever reached it. An
empty-string result was therefore written to the store as a zero-byte entry
and replaced in the conversation by a pointer ending in a dangling
"Preview: " (no ellipsis is emitted because len("") is not greater than
preview_chars). The entry is never read again, so the run paid a store write
and handed the model a pointer to nothing. Checking the raw result restores
the documented behavior for both shapes an empty result can take.
Fixes deepset-ai#12583
Author: harisahmed510.00@gmail.com
|
@TheDeveloperDoctor is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @TheDeveloperDoctor, thanks for your interest in contributing to Haystack! 🙏 This is an automated message to help us keep the review queue healthy. |
|
|
|
Closing as a duplicate of #12584, which was opened first and carries the same patch (the test diff is byte-identical). I've left my verification notes and one adjacent-shape finding over there: #12584 (comment) |
2 participants
Related Issues
Proposed Changes:
ToolResultOffloadHook._maybe_offloaddocuments that a tool result is left in context "when the result is empty", but the emptiness guard ran after astrresult had already been wrapped into a single-element list:ToolCallResultContentTisstr | Sequence[TextContent | ImageContent | FileContent], so an empty result has two shapes — and only one of them reached the guard:result.resultcontent_blocks[][]""[TextContent("")]The empty-string case was written to the store as a zero-byte entry and replaced in the conversation by a pointer ending in a dangling
Preview:— no ellipsis is emitted, becauselen("") > preview_charsis false and the slice itself is empty:That entry is never read again, so the run pays a store write and hands the model a pointer to nothing.
This is reachable from a normal Agent run, not only from a hand-built message: a tool returning
""passes through_result_to_string, which returns strings as-is, intoChatMessage.from_tool, which stores the value verbatim — soresult.resultkeeps itsstrshape all the way to the hook.The fix tests the raw result before the wrapping, so both empty shapes take the same path:
The
if not content_blockscheck is dropped as unreachable: past the new guard the result is truthy, and both arms of the union are truthy only when they yield at least one block.No API change, and no behavior change for non-empty results.
How did you test it?
Extended the existing
test_empty_result_is_not_offloadedinto a parametrized test over""and[], asserting both that the result is unchanged in state and that nothing was written to the store.Checked against the pre-fix code so the new case is a real regression test — it reproduces the reported symptom exactly:
With the change,
test/hooks/tool_result_offloading/passes in full (37 passed), as doruff format --check,ruff check, andmypyon the package.Notes for the reviewer
One adjacent shape is deliberately left alone: a non-empty list whose blocks are all empty text (
[TextContent(text="")]) still offloads, and still produces the danglingPreview:. Covering it would needall(isinstance(b, TextContent) and not b.text for b in content_blocks), which reads further from the docstring's "the result is empty" than the raw-value check does. Happy to widen it if you would rather the symptom be gone in every shape.Checklist