fix: Handle ToolCall Pydantic models correctly#9
Open
marklicata wants to merge 7 commits intomicrosoft:mainfrom
Open
fix: Handle ToolCall Pydantic models correctly#9marklicata wants to merge 7 commits intomicrosoft:mainfrom
marklicata wants to merge 7 commits intomicrosoft:mainfrom
Conversation
When a hook returns action='modify' on tool:post, the orchestrator now detects the modified data by comparing the returned result with the original, and uses it instead of the original get_serialized_output(). This enables tool output truncation, sanitization, and transformation hooks to work correctly. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
) Replace bare tc.id and tc.name attribute access with the safe getattr/dict.get dual-access pattern already used at every other tool_call access site in the file. Prevents AttributeError when providers return tool_calls as plain dicts and the user cancels during tool execution. Adds regression tests verifying CancelledError handler works with dict-based tool_calls. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…lledError in tool execution Harden the orchestrator's result-writing and tool-execution loops against CancelledError to prevent orphaned tool_calls that break the conversation state. When a CancelledError escapes a result-writing loop, the assistant message is committed without the corresponding tool result. The LLM sees a tool_use without a tool_result on the next turn, causing a protocol violation or infinite retry loop. This fix catches CancelledError at two levels: - In the result-writing loop, ensuring partial results are committed - In tool execution, ensuring the tool result is always recorded Rebased on main to pick up the defensive getattr access pattern for tool call attributes. Companion fixes already merged in loop-streaming#14 and loop-events#5. Fixes: microsoft-amplifier/amplifier-support#46 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…d, observability.events (microsoft#7) - Add EXECUTION_START and EXECUTION_END imports from amplifier_core.events - Move asyncio to top-level import (was inline inside execute method) - Emit execution:start after prompt:submit processing (skipped on deny) - Wrap main loop body in try/except/finally to emit execution:end on ALL exit paths - status='completed' on normal completion - status='cancelled' on cancellation paths (loop-start, post-provider, post-tools) - status='cancelled' for asyncio.CancelledError exceptions - status='error' for all other exceptions - Add tool_call_id to TOOL_PRE event payload - Add tool_call_id to TOOL_POST event payload - Add tool_call_id to both TOOL_ERROR event payloads (not-found and exception paths) - Register observability.events in mount() with execution:start and execution:end Tests added in tests/test_lifecycle_events.py: - test_execution_start_fires_with_prompt - test_execution_end_fires_on_normal_completion - test_execution_end_response_matches_return_value - test_execution_start_not_fired_when_prompt_submit_denied - test_execution_end_fires_cancelled_at_loop_start - test_execution_end_fires_cancelled_after_provider - test_execution_end_fires_on_provider_exception - test_execution_end_fires_exactly_once - test_tool_pre_includes_tool_call_id - test_tool_post_includes_tool_call_id - test_tool_error_not_found_includes_tool_call_id - test_tool_error_exception_includes_tool_call_id - test_mount_registers_observability_events
The orchestrator now sets coordinator._tool_dispatch_context = {
"tool_call_id": tool_call_id,
"parallel_group_id": group_id,
} immediately before calling tool.execute() in execute_single_tool,
and clears it in the outer finally block (alongside register_tool_complete).
This lets tools that need framework correlation IDs (e.g. the delegate
tool) read them via:
dispatch_ctx = getattr(self.coordinator, '_tool_dispatch_context', {})
tool_call_id = dispatch_ctx.get('tool_call_id', '')
Uses setattr() to avoid type errors on the dynamic private attribute.
The finally block always clears the context to prevent leaking between
parallel tool executions.
Fixes Check 4: delegate:agent_spawned tool_call_id was always empty
because the delegate tool was reading from tool input (which the LLM
never populates with framework IDs).
…PyPI The [tool.uv.sources] section forced amplifier-core to resolve from git (requiring a Rust toolchain). Removed so that uv resolves from PyPI, matching the standard install path. Also pinned amplifier-core>=1.0.10 in dev dependency-group.
The orchestrator was failing when processing ToolCall objects because it
used getattr(tc, 'id', None) or tc.get('id'), which fails when:
1. tc is a Pydantic model (no .get() method)
2. The attribute exists but is falsy (empty string)
Changed to use hasattr() checks: tc.id if hasattr(tc, 'id') else tc.get('id')
This properly handles both Pydantic models and dictionaries.
Fixes tool execution errors with custom tool modules.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix: Handle ToolCall Pydantic models correctly
Problem
The orchestrator was failing when processing ToolCall objects with the error:
Root Cause
The code used this pattern:
This fails when:
tcis a Pydantic model (has attributes, not dict methods)"").get()which doesn't exist on Pydantic modelsSolution
Use
hasattr()to check if the attribute exists, rather than checking if it's truthy:This properly handles both:
.get()method)Changes
Updated all instances of ToolCall attribute access in:
Testing
Verified with custom tool module (tool-msgraph) that uses Pydantic ToolCall models.
Before: Tool calls failed with AttributeError
After: Tool calls execute successfully