Skip to content

Commit 3a0246f

Browse files
fix: use safe dual-access pattern in CancelledError handler (#6)
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>
1 parent aa37f4e commit 3a0246f

File tree

2 files changed

+108
-2
lines changed

2 files changed

+108
-2
lines changed

amplifier_module_loop_basic/__init__.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -533,8 +533,9 @@ async def execute_single_tool(
533533
await context.add_message(
534534
{
535535
"role": "tool",
536-
"tool_call_id": tc.id,
537-
"content": f'{{"error": "Tool execution was cancelled by user", "cancelled": true, "tool": "{tc.name}"}}',
536+
"tool_call_id": getattr(tc, "id", None)
537+
or tc.get("id"),
538+
"content": f'{{"error": "Tool execution was cancelled by user", "cancelled": true, "tool": "{getattr(tc, "name", None) or tc.get("tool")}"}}',
538539
}
539540
)
540541
# Re-raise to let the cancellation propagate
Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
"""Tests for CancelledError handler with dict-based tool_calls.
2+
3+
Regression test for unsafe tc.id / tc.name access at lines 536-537.
4+
The CancelledError handler used bare attribute access on tool_call objects
5+
that may be plain dicts. Every other access site (9 of them) uses the safe
6+
dual-access pattern: getattr(tc, "id", None) or tc.get("id").
7+
"""
8+
9+
import asyncio
10+
11+
import pytest
12+
13+
from amplifier_core.testing import EventRecorder, MockContextManager
14+
15+
from amplifier_module_loop_basic import BasicOrchestrator
16+
17+
18+
class DictToolCallProvider:
19+
"""Provider that returns tool_calls as plain dicts (not ToolCall objects).
20+
21+
Some providers return tool_calls as dicts rather than objects.
22+
The orchestrator explicitly accommodates this with a dual-access pattern.
23+
"""
24+
25+
name = "dict-provider"
26+
27+
async def complete(self, request, **kwargs):
28+
return type(
29+
"Response",
30+
(),
31+
{
32+
"content": "Calling tool",
33+
"tool_calls": [
34+
{"id": "tc1", "tool": "cancel_tool", "arguments": {}}
35+
],
36+
"usage": None,
37+
"content_blocks": None,
38+
"metadata": None,
39+
},
40+
)()
41+
42+
43+
class CancellingTool:
44+
"""Tool that raises CancelledError to simulate immediate cancellation."""
45+
46+
name = "cancel_tool"
47+
description = "tool that simulates cancellation"
48+
input_schema = {"type": "object", "properties": {}}
49+
50+
async def execute(self, args):
51+
raise asyncio.CancelledError()
52+
53+
54+
@pytest.mark.asyncio
55+
async def test_cancelled_error_handler_with_dict_tool_calls():
56+
"""CancelledError handler must not crash when tool_calls are plain dicts.
57+
58+
Without the fix, line 536 (tc.id) raises:
59+
AttributeError: 'dict' object has no attribute 'id'
60+
61+
With the fix, CancelledError propagates cleanly after synthesizing
62+
cancelled tool results into the context.
63+
"""
64+
orchestrator = BasicOrchestrator({})
65+
context = MockContextManager()
66+
hooks = EventRecorder()
67+
68+
with pytest.raises(asyncio.CancelledError):
69+
await orchestrator.execute(
70+
prompt="Test",
71+
context=context,
72+
providers={"default": DictToolCallProvider()},
73+
tools={"cancel_tool": CancellingTool()},
74+
hooks=hooks,
75+
)
76+
77+
78+
@pytest.mark.asyncio
79+
async def test_cancelled_error_synthesizes_messages_for_dict_tool_calls():
80+
"""After fix, cancelled tool results are properly added to context.
81+
82+
Verifies the synthesized cancellation messages contain the correct
83+
tool_call_id and tool name extracted via the safe dual-access pattern.
84+
"""
85+
orchestrator = BasicOrchestrator({})
86+
context = MockContextManager()
87+
hooks = EventRecorder()
88+
89+
with pytest.raises(asyncio.CancelledError):
90+
await orchestrator.execute(
91+
prompt="Test",
92+
context=context,
93+
providers={"default": DictToolCallProvider()},
94+
tools={"cancel_tool": CancellingTool()},
95+
hooks=hooks,
96+
)
97+
98+
# Find the synthesized cancellation message in context
99+
tool_messages = [m for m in context.messages if m.get("role") == "tool"]
100+
assert len(tool_messages) >= 1, "Expected at least one synthesized tool message"
101+
102+
cancel_msg = tool_messages[-1]
103+
assert cancel_msg["tool_call_id"] == "tc1"
104+
assert "cancelled" in cancel_msg["content"]
105+
assert "cancel_tool" in cancel_msg["content"]

0 commit comments

Comments
 (0)