Repository navigation
Python: fix(python): preserve empty tool results in chat history - #14472
Reginald Alfret V (reginaldalfret) wants to merge 2 commits into
Conversation
…soft#14359) Signed-off-by: Reginald Alfret <reginaldalfret@gmail.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The fix and regression test address the empty tool-result serialization failure.
Review effort: Lite
Findings: None
What changed in this PR
Fixes preservation and serialization of empty-string tool results in Python chat history.
Changes:
- Preserves empty tool results and their tool call IDs.
- Adds regression coverage for serialization.
| File | Description |
|---|---|
python/semantic_kernel/contents/chat_history.py |
Preserves empty tool results as FunctionResultContent. |
python/tests/unit/contents/test_chat_history.py |
Adds regression coverage for empty-result serialization. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Community review. I am not a maintainer, so this does not clear the merge gate.
I read the full diff and ChatHistory._prepare_for_add / add_tool_message in python/semantic_kernel/contents/chat_history.py, then ran the change (python/ in a venv, Python 3.13).
The bug on main: _prepare_for_add used role == AuthorRole.TOOL and content and not items, so add_tool_message("", tool_call_id=...) skipped the FunctionResultContent branch. I ran it on origin/main: it raises IndexError: list index out of range, which says nothing about the cause. With the PR (content is not None), the same call returns a message with one FunctionResultContent item and to_dict() gives {'role': 'tool', 'content': '', 'tool_call_id': 'call_1'}.
What I ran:
tests/unit/contentson the PR head: 395 passed.tests/unit/contents/test_chat_history.pywithchat_history.pytaken from origin/main: the newtest_add_tool_message_empty_content_to_dict_succeedsfails and the other tests in that file pass.- A probe of
add_tool_messagewith"","ok"and"0": the last two behave the same on main and the PR. With[]it raisesIndexErroron both; withNoneit raisesNotImplementedErroron both (thesingledispatchmethodhas no handler for it). I did not change or test anything else for those two, and the PR does not claim to cover them. add_user_message(""),add_assistant_message(""),add_system_message("")andadd_developer_message("")on the PR head all succeed and produce a message with no items andcontent=''. So the tool role was the one role where an empty string failed, which matches what the PR fixes.
Why it matters in practice: a tool or function that legitimately returns an empty string is a normal case (for example an empty search result), and the manual path of adding the result with add_tool_message crashed. I grepped semantic_kernel for callers of add_tool_message outside chat_history.py and found none (the two azure_chat_completion.py hits are a different method), so within the library this only affects callers that add tool results themselves.
I did not run the connector tests, any model call, or CI. I have no blocking concerns.
|
Thank you PRABHU KIRAN VANDRANKI (@VANDRANKI) for the community review and verifying that the unit tests pass cleanly. The PR addresses #14359 by ensuring empty string results in �dd_tool_message are preserved as FunctionResultContent when content is not None rather than dropped by truthiness checks. Requesting maintainer approval to run the GitHub Actions workflows (Python Unit Tests, Python Code Quality Checks, etc.) and for official maintainer review. |
Motivation and Context
Fixes #14359
When a tool function returns an empty string
"",ChatHistory.add_tool_message(content="", tool_call_id="...")previously evaluatedcontentas falsy in_prepare_for_add():Because
contentwas"", this check failed and fell through without creating aFunctionResultContentitem and without attachingtool_call_id. The resultingChatMessageContenthad roletoolbut an emptyitemslist. Subsequently callingChatMessageContent.to_dict()failed withIndexError: list index out of rangewhen attempting to accessself.items[0]to retrieve the tool call ID.Description
python/semantic_kernel/contents/chat_history.py, updated_prepare_for_addto checkcontent is not Noneinstead of truthycontent:python/tests/unit/contents/test_chat_history.py, added a unit testtest_add_tool_message_empty_content_to_dict_succeedsverifying that callingadd_tool_message("", tool_call_id="call_123")correctly populates aFunctionResultContentwithresult=""andid="call_123", and that.to_dict()executes cleanly without raisingIndexError.Contribution Checklist