From 78ab5064822323cf59e6ee632632f62e5110caf8 Mon Sep 17 00:00:00 2001 From: Evan Mattson Date: Fri, 27 Feb 2026 15:05:37 +0900 Subject: [PATCH 1/3] Fix #4305: Handle dict chat_options in _update_conversation_id _update_conversation_id assumed chat_options had attribute access, but ChatOptions is a TypedDict (dict). When a dict was passed, setting .conversation_id raised AttributeError. Now checks isinstance(dict) and uses key access for dicts, falling back to attribute access for objects. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../packages/core/agent_framework/_tools.py | 6 +- .../core/test_function_invocation_logic.py | 67 +++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/python/packages/core/agent_framework/_tools.py b/python/packages/core/agent_framework/_tools.py index 3ec167d4f7..3e0dca0321 100644 --- a/python/packages/core/agent_framework/_tools.py +++ b/python/packages/core/agent_framework/_tools.py @@ -1669,7 +1669,11 @@ def _update_conversation_id( if conversation_id is None: return if "chat_options" in kwargs: - kwargs["chat_options"].conversation_id = conversation_id + chat_opts = kwargs["chat_options"] + if isinstance(chat_opts, dict): + chat_opts["conversation_id"] = conversation_id + else: + chat_opts.conversation_id = conversation_id else: kwargs["conversation_id"] = conversation_id diff --git a/python/packages/core/tests/core/test_function_invocation_logic.py b/python/packages/core/tests/core/test_function_invocation_logic.py index 319d35f152..3452478b85 100644 --- a/python/packages/core/tests/core/test_function_invocation_logic.py +++ b/python/packages/core/tests/core/test_function_invocation_logic.py @@ -3449,3 +3449,70 @@ def search_func(query: str) -> str: reasoning_contents = [c for msg in response.messages for c in msg.contents if c.type == "text_reasoning"] assert len(reasoning_contents) >= 1 assert reasoning_contents[0].id == "rs_test123" + + +# region _update_conversation_id unit tests + + +class TestUpdateConversationId: + """Tests for _update_conversation_id handling dict and object chat_options.""" + + def test_chat_options_as_dict(self): + """When chat_options is a plain dict, conversation_id should be set via key access.""" + from agent_framework._tools import _update_conversation_id + + kwargs: dict[str, Any] = {"chat_options": {}} + _update_conversation_id(kwargs, "conv_1") + assert kwargs["chat_options"]["conversation_id"] == "conv_1" + + def test_chat_options_as_typed_dict(self): + """When chat_options is a ChatOptions TypedDict, conversation_id should be set via key access.""" + from agent_framework import ChatOptions + from agent_framework._tools import _update_conversation_id + + opts: ChatOptions = {"temperature": 0.5} + kwargs: dict[str, Any] = {"chat_options": opts} + _update_conversation_id(kwargs, "conv_2") + assert kwargs["chat_options"]["conversation_id"] == "conv_2" + + def test_chat_options_as_object_with_attribute(self): + """When chat_options is an object with attribute access, conversation_id should be set as attribute.""" + from agent_framework._tools import _update_conversation_id + + class ObjOptions: + conversation_id: str | None = None + + obj = ObjOptions() + kwargs: dict[str, Any] = {"chat_options": obj} + _update_conversation_id(kwargs, "conv_3") + assert obj.conversation_id == "conv_3" + + def test_no_chat_options_falls_back_to_kwargs(self): + """When chat_options is absent, conversation_id should be set directly on kwargs.""" + from agent_framework._tools import _update_conversation_id + + kwargs: dict[str, Any] = {} + _update_conversation_id(kwargs, "conv_4") + assert kwargs["conversation_id"] == "conv_4" + + def test_none_conversation_id_is_noop(self): + """When conversation_id is None, kwargs should not be modified.""" + from agent_framework._tools import _update_conversation_id + + kwargs: dict[str, Any] = {"chat_options": {}} + _update_conversation_id(kwargs, None) + assert "conversation_id" not in kwargs["chat_options"] + assert "conversation_id" not in kwargs + + def test_options_dict_also_updated(self): + """The optional options dict should also receive conversation_id.""" + from agent_framework._tools import _update_conversation_id + + kwargs: dict[str, Any] = {"chat_options": {}} + options: dict[str, Any] = {} + _update_conversation_id(kwargs, "conv_5", options) + assert kwargs["chat_options"]["conversation_id"] == "conv_5" + assert options["conversation_id"] == "conv_5" + + +# endregion From deb828760f7c6462a26b4ab8aed1aa75f9057ea0 Mon Sep 17 00:00:00 2001 From: Evan Mattson Date: Fri, 27 Feb 2026 15:11:14 +0900 Subject: [PATCH 2/3] Address PR feedback: use Mapping ABC and add missing tests (#4305) - Use collections.abc.Mapping instead of dict for isinstance check in _update_conversation_id, making it more robust for non-dict mapping types. - Add test for object-style chat_options with optional options dict parameter. - Add test verifying existing conversation_id gets overwritten (idempotent). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../packages/core/agent_framework/_tools.py | 2 +- .../core/test_function_invocation_logic.py | 22 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/python/packages/core/agent_framework/_tools.py b/python/packages/core/agent_framework/_tools.py index 3e0dca0321..616b02315e 100644 --- a/python/packages/core/agent_framework/_tools.py +++ b/python/packages/core/agent_framework/_tools.py @@ -1670,7 +1670,7 @@ def _update_conversation_id( return if "chat_options" in kwargs: chat_opts = kwargs["chat_options"] - if isinstance(chat_opts, dict): + if isinstance(chat_opts, Mapping): chat_opts["conversation_id"] = conversation_id else: chat_opts.conversation_id = conversation_id diff --git a/python/packages/core/tests/core/test_function_invocation_logic.py b/python/packages/core/tests/core/test_function_invocation_logic.py index 3452478b85..f8fda4df46 100644 --- a/python/packages/core/tests/core/test_function_invocation_logic.py +++ b/python/packages/core/tests/core/test_function_invocation_logic.py @@ -3514,5 +3514,27 @@ def test_options_dict_also_updated(self): assert kwargs["chat_options"]["conversation_id"] == "conv_5" assert options["conversation_id"] == "conv_5" + def test_options_dict_also_updated_with_object_chat_options(self): + """The optional options dict should also receive conversation_id when chat_options is object-style.""" + from agent_framework._tools import _update_conversation_id + + class ObjOptions: + conversation_id: str | None = None + + obj = ObjOptions() + kwargs: dict[str, Any] = {"chat_options": obj} + options: dict[str, Any] = {} + _update_conversation_id(kwargs, "conv_6", options) + assert obj.conversation_id == "conv_6" + assert options["conversation_id"] == "conv_6" + + def test_dict_overwrites_existing_conversation_id(self): + """When a dict already has a conversation_id, it should be overwritten.""" + from agent_framework._tools import _update_conversation_id + + kwargs: dict[str, Any] = {"chat_options": {"conversation_id": "old_id"}} + _update_conversation_id(kwargs, "new_id") + assert kwargs["chat_options"]["conversation_id"] == "new_id" + # endregion From 8fde5fbb4d3dee42780ec11c976085878bd69451 Mon Sep 17 00:00:00 2001 From: Evan Mattson Date: Wed, 4 Mar 2026 14:48:36 +0900 Subject: [PATCH 3/3] Remove unnecessary Mapping check in _update_conversation_id (#4305) chat_options is always a dict, so the isinstance(chat_opts, Mapping) check and the else branch for attribute-style access are dead code. Simplify to direct dict key assignment and remove object-style tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../packages/core/agent_framework/_tools.py | 6 +--- .../core/test_function_invocation_logic.py | 28 +------------------ 2 files changed, 2 insertions(+), 32 deletions(-) diff --git a/python/packages/core/agent_framework/_tools.py b/python/packages/core/agent_framework/_tools.py index 616b02315e..728d62490e 100644 --- a/python/packages/core/agent_framework/_tools.py +++ b/python/packages/core/agent_framework/_tools.py @@ -1669,11 +1669,7 @@ def _update_conversation_id( if conversation_id is None: return if "chat_options" in kwargs: - chat_opts = kwargs["chat_options"] - if isinstance(chat_opts, Mapping): - chat_opts["conversation_id"] = conversation_id - else: - chat_opts.conversation_id = conversation_id + kwargs["chat_options"]["conversation_id"] = conversation_id else: kwargs["conversation_id"] = conversation_id diff --git a/python/packages/core/tests/core/test_function_invocation_logic.py b/python/packages/core/tests/core/test_function_invocation_logic.py index f8fda4df46..7f0eda62fc 100644 --- a/python/packages/core/tests/core/test_function_invocation_logic.py +++ b/python/packages/core/tests/core/test_function_invocation_logic.py @@ -3455,7 +3455,7 @@ def search_func(query: str) -> str: class TestUpdateConversationId: - """Tests for _update_conversation_id handling dict and object chat_options.""" + """Tests for _update_conversation_id handling dict chat_options.""" def test_chat_options_as_dict(self): """When chat_options is a plain dict, conversation_id should be set via key access.""" @@ -3475,18 +3475,6 @@ def test_chat_options_as_typed_dict(self): _update_conversation_id(kwargs, "conv_2") assert kwargs["chat_options"]["conversation_id"] == "conv_2" - def test_chat_options_as_object_with_attribute(self): - """When chat_options is an object with attribute access, conversation_id should be set as attribute.""" - from agent_framework._tools import _update_conversation_id - - class ObjOptions: - conversation_id: str | None = None - - obj = ObjOptions() - kwargs: dict[str, Any] = {"chat_options": obj} - _update_conversation_id(kwargs, "conv_3") - assert obj.conversation_id == "conv_3" - def test_no_chat_options_falls_back_to_kwargs(self): """When chat_options is absent, conversation_id should be set directly on kwargs.""" from agent_framework._tools import _update_conversation_id @@ -3514,20 +3502,6 @@ def test_options_dict_also_updated(self): assert kwargs["chat_options"]["conversation_id"] == "conv_5" assert options["conversation_id"] == "conv_5" - def test_options_dict_also_updated_with_object_chat_options(self): - """The optional options dict should also receive conversation_id when chat_options is object-style.""" - from agent_framework._tools import _update_conversation_id - - class ObjOptions: - conversation_id: str | None = None - - obj = ObjOptions() - kwargs: dict[str, Any] = {"chat_options": obj} - options: dict[str, Any] = {} - _update_conversation_id(kwargs, "conv_6", options) - assert obj.conversation_id == "conv_6" - assert options["conversation_id"] == "conv_6" - def test_dict_overwrites_existing_conversation_id(self): """When a dict already has a conversation_id, it should be overwritten.""" from agent_framework._tools import _update_conversation_id