fix(python): preserve empty tool results - #14364
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The functional change is small, targeted, and covered by a regression test, with only a minor typing-annotation nit noted.
Pull request overview
Fixes a Python ChatHistory.add_tool_message() edge case where an empty-string tool result was treated as “no content”, producing an invalid tool message (no FunctionResultContent) and causing ChatMessageContent.to_dict() to fail.
Changes:
- Adjust tool-message preparation to treat
""as valid content by distinguishingNonefrom empty strings. - Preserve tool-call correlation (
tool_call_id) and function metadata for empty tool results by always creatingFunctionResultContentwhencontent is not None. - Add a focused regression test covering empty-string tool results and successful serialization via
to_dict().
File summaries
| File | Description |
|---|---|
| python/semantic_kernel/contents/chat_history.py | Updates tool-message preparation logic to build FunctionResultContent even when the tool result is an empty string. |
| python/tests/unit/contents/test_chat_history.py | Adds regression coverage ensuring empty tool results preserve correlation and serialize without errors. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -219,7 +219,7 @@ def _prepare_for_add( | |||
| """Prepare a message to be added to the history.""" | |||
| kwargs["role"] = role | |||
|
|
|||
| if role == AuthorRole.TOOL and content and not items: | |||
| if role == AuthorRole.TOOL and content is not None and not items: | |||
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: No findings
Scope: full PR (1 commit(s)): 6de01064685f
Model: claude-opus-4.8
Overview
This PR changes a single guard in ChatHistory._prepare_for_add from a truthiness
test (content and not items) to an identity test (content is not None and not items) for AuthorRole.TOOL, so an empty-string tool result now builds a
FunctionResultContent(result="") instead of producing an item-less message that
crashed ChatMessageContent.to_dict() with IndexError (issue #14359). The change
is precisely scoped: None still skips the branch (behavior unchanged), non-empty
strings were already handled, the string overload's required tool_call_id guard
(chat_history.py:176-180) still runs first, and a focused regression test locks in
the content-object fields and the previously-crashing serialization path. Reviewers
empirically reproduced both the pre-fix crash and the post-fix success and could not
construct any PR-introduced defect; the residual concerns are Low-severity and
pre-existing (non-TOOL empty-content asymmetry and an XML round-trip fidelity gap).
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
| def _prepare_for_add( | ||
| self, role: AuthorRole, content: str | None = None, items: list[KernelContent] | None = None, **kwargs: Any | ||
| ) -> dict[str, str]: | ||
| ) -> dict[str, Any]: |
There was a problem hiding this comment.
Addressed in commit f53a8be by changing _prepare_for_add return annotation to dict[str, Any]. This reflects the actual role and items values returned by the helper.
2f89496 to
664e518
Compare
Motivation and Context
Fixes #14359.
ChatHistory.add_tool_message()currently treats an empty string as absent content. This creates a tool message with noFunctionResultContent, loses the supplied tool-call correlation, and makesChatMessageContent.to_dict()raiseIndexError.An empty string is a valid result for a successful command with no output or a no-op tool.
Description
Change the tool-message preparation path to distinguish
Nonefrom an empty string. Empty string results now create the sameFunctionResultContentas other string results, preserving the result,id,call_id, and optional function name. Non-tool messages retain their existing truthiness behavior.Added a focused regression test that verifies the content object fields and successful serialized tool-message dictionary.
This is backward compatible: non-empty tool results and all existing message forms are unchanged, and the fix only makes the documented string input work for the valid empty-string boundary case.
Validation
uv run pytest tests/unit/contents/test_chat_history.py -k empty_tool_message -q --basetemp D:\\project\\tscodex\\github_pr\\tmp\\semantic-kernel-14359-pytest— passed (1 test)uv run pytest tests/unit/contents/test_chat_history.py -q --basetemp D:\\project\\tscodex\\github_pr\\tmp\\semantic-kernel-14359— passed (56 tests)uv run ruff check semantic_kernel/contents/chat_history.py tests/unit/contents/test_chat_history.py— passeduv run ruff format --check semantic_kernel/contents/chat_history.py tests/unit/contents/test_chat_history.py— passeduv run mypy semantic_kernel/contents/chat_history.py— passeduv run pre-commit run --files semantic_kernel/contents/chat_history.py tests/unit/contents/test_chat_history.py— passedgit diff --check— passedThe full Python unit suite was not run because this is a localized content-model change; no model, provider, credentials, database, or external service is required.
Contribution Checklist
AI Disclosure
This contribution was prepared with AI assistance and reviewed against the repository source, issue reproduction, and local test results.