VibecoderMcSwaggins commited on
Commit
2b44c5a
·
1 Parent(s): 8615dab

fix: Address CodeRabbit review feedback

Browse files

Changes based on CodeRabbit PR review:

1. docs/bugs/P0_HUGGINGFACE_TOOL_CALLING_BROKEN.md:
- Add 'text' language to fenced code block (MD040)
- Wrap bare GitHub URL in markdown link (MD034)
- Update workaround section to reflect implementation status

2. src/orchestrators/advanced.py:
- Add plain string handling to _extract_text() for WorkflowOutputEvent.data
- Filter repr-style noise from string payloads

3. src/clients/huggingface.py:
- JSON-encode structured tool results instead of str()
- Track call_id -> name mapping for tool result messages
- Add 'name' field to tool messages (required by some APIs)

docs/bugs/P0_HUGGINGFACE_TOOL_CALLING_BROKEN.md CHANGED
@@ -52,7 +52,7 @@ Updated `_convert_messages()` in `src/clients/huggingface.py:71-121` to:
52
 
53
  ### Symptom
54
  `MagenticAgentMessageEvent.message.text` contains:
55
- ```
56
  '<agent_framework._types.ChatMessage object at 0x10c394210>'
57
  ```
58
 
@@ -89,19 +89,19 @@ async def _invoke_agent(self, ctx, ...) -> ChatMessage:
89
  3. Unrelated orchestration issue
90
 
91
  ### Upstream Issue Filed
92
- **GitHub Issue**: https://github.com/microsoft/agent-framework/issues/2562
93
 
94
  **Suggested fixes in issue**:
95
  1. **Minimal**: `text = last.text or ""`
96
  2. **Better UX**: Format tool calls for display
97
  3. **Best**: Add `__str__` to `ChatMessage` class
98
 
99
- ### Workaround (Not Implemented)
100
- We COULD modify `_extract_text()` in `advanced.py` to extract tool call names from `.contents` when text is empty/repr:
101
 
102
  ```python
103
  def _extract_text(self, message: Any) -> str:
104
- # ... existing logic ...
105
 
106
  # Workaround: Extract tool call info when text is repr/empty
107
  if hasattr(message, "contents") and message.contents:
@@ -116,7 +116,7 @@ def _extract_text(self, message: Any) -> str:
116
  return ""
117
  ```
118
 
119
- **Decision**: Not implementing until we confirm whether Bug #2 affects research completion or just display.
120
 
121
  ---
122
 
@@ -141,8 +141,8 @@ def _extract_text(self, message: Any) -> str:
141
  - Added `@use_function_invocation`, `@use_observability`, `@use_chat_middleware` decorators
142
  - Added `__function_invoking_chat_client__ = True` marker
143
 
144
- ### No Changes Needed
145
- - `src/orchestrators/advanced.py` - `_extract_text()` already filters repr strings
146
 
147
  ---
148
 
 
52
 
53
  ### Symptom
54
  `MagenticAgentMessageEvent.message.text` contains:
55
+ ```text
56
  '<agent_framework._types.ChatMessage object at 0x10c394210>'
57
  ```
58
 
 
89
  3. Unrelated orchestration issue
90
 
91
  ### Upstream Issue Filed
92
+ **GitHub Issue**: [microsoft/agent-framework#2562](https://github.com/microsoft/agent-framework/issues/2562)
93
 
94
  **Suggested fixes in issue**:
95
  1. **Minimal**: `text = last.text or ""`
96
  2. **Better UX**: Format tool calls for display
97
  3. **Best**: Add `__str__` to `ChatMessage` class
98
 
99
+ ### Workaround (Implemented in `advanced.py`)
100
+ We modified `_extract_text()` in `advanced.py` to extract tool call names from `.contents` when text is empty or looks like a repr:
101
 
102
  ```python
103
  def _extract_text(self, message: Any) -> str:
104
+ # ... existing logic with repr filtering ...
105
 
106
  # Workaround: Extract tool call info when text is repr/empty
107
  if hasattr(message, "contents") and message.contents:
 
116
  return ""
117
  ```
118
 
119
+ **Decision**: Implemented locally to fix display and logging while we wait for upstream fix.
120
 
121
  ---
122
 
 
141
  - Added `@use_function_invocation`, `@use_observability`, `@use_chat_middleware` decorators
142
  - Added `__function_invoking_chat_client__ = True` marker
143
 
144
+ ### Also Fixed
145
+ - `src/orchestrators/advanced.py` - `_extract_text()` now filters repr strings AND extracts tool call names
146
 
147
  ---
148
 
src/clients/huggingface.py CHANGED
@@ -71,6 +71,11 @@ class HuggingFaceChatClient(BaseChatClient): # type: ignore[misc]
71
  def _convert_messages(self, messages: MutableSequence[ChatMessage]) -> list[dict[str, Any]]:
72
  """Convert framework messages to HuggingFace format."""
73
  hf_messages: list[dict[str, Any]] = []
 
 
 
 
 
74
  for msg in messages:
75
  # msg.role can be string or enum - extract .value for enums
76
  if hasattr(msg.role, "value"):
@@ -81,12 +86,15 @@ class HuggingFaceChatClient(BaseChatClient): # type: ignore[misc]
81
  content_str = msg.text or ""
82
  tool_calls = []
83
  tool_call_id = None
 
84
 
85
  # Process contents for tool calls and results
86
  if msg.contents:
87
  for item in msg.contents:
88
  if isinstance(item, FunctionCallContent):
89
  # This is an assistant message invoking a tool
 
 
90
  tool_calls.append(
91
  {
92
  "id": item.call_id,
@@ -105,8 +113,16 @@ class HuggingFaceChatClient(BaseChatClient): # type: ignore[misc]
105
  # This is a tool result message
106
  role_str = "tool"
107
  tool_call_id = item.call_id
108
- # For tool results, the content is the result string
109
- content_str = str(item.result) if item.result is not None else ""
 
 
 
 
 
 
 
 
110
 
111
  message_dict: dict[str, Any] = {"role": role_str, "content": content_str}
112
 
@@ -115,6 +131,9 @@ class HuggingFaceChatClient(BaseChatClient): # type: ignore[misc]
115
 
116
  if tool_call_id:
117
  message_dict["tool_call_id"] = tool_call_id
 
 
 
118
 
119
  hf_messages.append(message_dict)
120
 
 
71
  def _convert_messages(self, messages: MutableSequence[ChatMessage]) -> list[dict[str, Any]]:
72
  """Convert framework messages to HuggingFace format."""
73
  hf_messages: list[dict[str, Any]] = []
74
+
75
+ # Track call_id -> tool_name mapping for tool result messages
76
+ # Assistant messages with tool_calls come before tool result messages
77
+ call_id_to_name: dict[str, str] = {}
78
+
79
  for msg in messages:
80
  # msg.role can be string or enum - extract .value for enums
81
  if hasattr(msg.role, "value"):
 
86
  content_str = msg.text or ""
87
  tool_calls = []
88
  tool_call_id = None
89
+ tool_name = None
90
 
91
  # Process contents for tool calls and results
92
  if msg.contents:
93
  for item in msg.contents:
94
  if isinstance(item, FunctionCallContent):
95
  # This is an assistant message invoking a tool
96
+ # Track call_id -> name for later tool result messages
97
+ call_id_to_name[item.call_id] = item.name
98
  tool_calls.append(
99
  {
100
  "id": item.call_id,
 
113
  # This is a tool result message
114
  role_str = "tool"
115
  tool_call_id = item.call_id
116
+ # Look up tool name from prior FunctionCallContent
117
+ tool_name = call_id_to_name.get(item.call_id)
118
+ # For tool results, JSON-encode structured data
119
+ # HuggingFace/OpenAI expects string content
120
+ if item.result is None:
121
+ content_str = ""
122
+ elif isinstance(item.result, str):
123
+ content_str = item.result
124
+ else:
125
+ content_str = json.dumps(item.result)
126
 
127
  message_dict: dict[str, Any] = {"role": role_str, "content": content_str}
128
 
 
131
 
132
  if tool_call_id:
133
  message_dict["tool_call_id"] = tool_call_id
134
+ # Add name field if we tracked it (required by some APIs)
135
+ if tool_name:
136
+ message_dict["name"] = tool_name
137
 
138
  hf_messages.append(message_dict)
139
 
src/orchestrators/advanced.py CHANGED
@@ -339,10 +339,18 @@ The final output should be a structured research report."""
339
 
340
  Handles ChatMessage objects from both OpenAI and HuggingFace clients.
341
  ChatMessage has: .text (str), .contents (list of content objects)
 
342
  """
343
  if not message:
344
  return ""
345
 
 
 
 
 
 
 
 
346
  # Priority 1: .text (standard ChatMessage text content)
347
  if hasattr(message, "text") and message.text:
348
  text = message.text
 
339
 
340
  Handles ChatMessage objects from both OpenAI and HuggingFace clients.
341
  ChatMessage has: .text (str), .contents (list of content objects)
342
+ Also handles plain string messages (e.g., WorkflowOutputEvent.data).
343
  """
344
  if not message:
345
  return ""
346
 
347
+ # Priority 0: Handle plain string messages (e.g., WorkflowOutputEvent.data)
348
+ if isinstance(message, str):
349
+ # Filter out obvious repr-style noise
350
+ if not (message.startswith("<") and "object at" in message):
351
+ return message
352
+ return ""
353
+
354
  # Priority 1: .text (standard ChatMessage text content)
355
  if hasattr(message, "text") and message.text:
356
  text = message.text