Skip to content

Commit d9eff68

Browse files
bokelleyclaude
andauthored
fix: correct tool name in list_creative_formats method (#10)
Fixed critical copy-paste bug where list_creative_formats() was calling adapter.call_tool("update_media_buy", ...) instead of the correct "list_creative_formats" tool name. Bug Impact: - Method was completely non-functional, calling wrong tool - Activity logs showed incorrect task_type - Webhooks and tracking would fail for this method Root Cause: - Copy-paste error from method template - Over-mocked tests didn't verify tool name argument - Tests passed because mocks returned success regardless of tool name Test Improvements: - Added parameterized test verifying ALL methods call correct tool names - Fixed test_get_products to verify tool name argument - Fixed test_multi_agent_parallel_execution to actually test execution - Enhanced protocol tests to verify HTTP/MCP request details - Removed non-functional stub tests from test_cli.py All 68 tests now pass, including the new test that would have caught this bug immediately. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
1 parent bf4561f commit d9eff68

File tree

4 files changed

+147
-40
lines changed

4 files changed

+147
-40
lines changed

src/adcp/client.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -159,19 +159,19 @@ async def list_creative_formats(
159159
type=ActivityType.PROTOCOL_REQUEST,
160160
operation_id=operation_id,
161161
agent_id=self.agent_config.id,
162-
task_type="update_media_buy",
162+
task_type="list_creative_formats",
163163
timestamp=datetime.now(timezone.utc).isoformat(),
164164
)
165165
)
166166

167-
result = await self.adapter.call_tool("update_media_buy", params)
167+
result = await self.adapter.call_tool("list_creative_formats", params)
168168

169169
self._emit_activity(
170170
Activity(
171171
type=ActivityType.PROTOCOL_RESPONSE,
172172
operation_id=operation_id,
173173
agent_id=self.agent_config.id,
174-
task_type="update_media_buy",
174+
task_type="list_creative_formats",
175175
status=result.status,
176176
timestamp=datetime.now(timezone.utc).isoformat(),
177177
)

tests/test_cli.py

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -252,34 +252,6 @@ def test_invalid_protocol(self, tmp_path, monkeypatch):
252252
class TestCLIIntegration:
253253
"""Integration tests for CLI (with mocked network calls)."""
254254

255-
@pytest.mark.asyncio
256-
async def test_tool_execution_flow(self, tmp_path, monkeypatch):
257-
"""Test complete tool execution flow (mocked)."""
258-
# Setup config
259-
config_file = tmp_path / "config.json"
260-
config_data = {
261-
"agents": {
262-
"test": {
263-
"id": "test",
264-
"agent_uri": "https://test.com",
265-
"protocol": "mcp",
266-
}
267-
}
268-
}
269-
config_file.write_text(json.dumps(config_data))
270-
271-
import adcp.config
272-
monkeypatch.setattr(adcp.config, "CONFIG_FILE", config_file)
273-
274-
# This is an integration test concept - would need actual mocking
275-
# of ADCPClient to fully test. Showing the pattern here.
276-
# In practice, you'd mock the client's call_tool method.
277-
278-
def test_json_output_format(self):
279-
"""Test that --json flag produces valid JSON output."""
280-
# Would require mocking the actual tool call
281-
# Conceptual test showing what we'd verify
282-
pass
283255

284256

285257
class TestSpecialCharactersInPayload:

tests/test_client.py

Lines changed: 83 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,8 @@ async def test_get_products():
9797
request = GetProductsRequest(brief="test campaign")
9898
result = await client.get_products(request)
9999

100-
mock_call.assert_called_once()
100+
# Verify correct tool name is called
101+
mock_call.assert_called_once_with("get_products", {"brief": "test campaign"})
101102
assert result.success is True
102103
assert result.status == TaskStatus.COMPLETED
103104
assert "products" in result.data
@@ -126,11 +127,76 @@ async def test_all_client_methods():
126127
assert hasattr(client, "provide_performance_feedback")
127128

128129

130+
@pytest.mark.parametrize(
131+
"method_name,request_class,request_data",
132+
[
133+
("get_products", "GetProductsRequest", {}),
134+
("list_creative_formats", "ListCreativeFormatsRequest", {}),
135+
("sync_creatives", "SyncCreativesRequest", {"creatives": []}),
136+
("list_creatives", "ListCreativesRequest", {}),
137+
("get_media_buy_delivery", "GetMediaBuyDeliveryRequest", {}),
138+
("list_authorized_properties", "ListAuthorizedPropertiesRequest", {}),
139+
("get_signals", "GetSignalsRequest", {"signal_spec": "test", "deliver_to": {}}),
140+
(
141+
"activate_signal",
142+
"ActivateSignalRequest",
143+
{"signal_agent_segment_id": "test", "platform": "test"},
144+
),
145+
(
146+
"provide_performance_feedback",
147+
"ProvidePerformanceFeedbackRequest",
148+
{"media_buy_id": "test", "measurement_period": {}, "performance_index": 0.5},
149+
),
150+
],
151+
)
152+
@pytest.mark.asyncio
153+
async def test_method_calls_correct_tool_name(method_name, request_class, request_data):
154+
"""Test that each method calls adapter.call_tool with the correct tool name.
155+
156+
This test prevents copy-paste bugs where method bodies are copied but
157+
tool names aren't updated to match the method name.
158+
"""
159+
from unittest.mock import patch
160+
from adcp.types.core import TaskResult, TaskStatus
161+
import adcp.types.generated as gen
162+
163+
config = AgentConfig(
164+
id="test_agent",
165+
agent_uri="https://test.example.com",
166+
protocol=Protocol.A2A,
167+
)
168+
169+
client = ADCPClient(config)
170+
171+
# Create request instance with required fields
172+
request_cls = getattr(gen, request_class)
173+
request = request_cls(**request_data)
174+
175+
mock_result = TaskResult(
176+
status=TaskStatus.COMPLETED,
177+
data={},
178+
success=True,
179+
)
180+
181+
with patch.object(client.adapter, "call_tool", return_value=mock_result) as mock_call:
182+
method = getattr(client, method_name)
183+
await method(request)
184+
185+
# CRITICAL: Verify the tool name matches the method name
186+
mock_call.assert_called_once()
187+
actual_tool_name = mock_call.call_args[0][0]
188+
assert actual_tool_name == method_name, (
189+
f"Method {method_name} called tool '{actual_tool_name}' instead of '{method_name}'. "
190+
f"This is likely a copy-paste bug."
191+
)
192+
193+
129194
@pytest.mark.asyncio
130195
async def test_multi_agent_parallel_execution():
131196
"""Test parallel execution across multiple agents."""
132197
from unittest.mock import patch
133198
from adcp.types.core import TaskResult, TaskStatus
199+
from adcp.types.generated import GetProductsRequest
134200

135201
agents = [
136202
AgentConfig(
@@ -153,11 +219,19 @@ async def test_multi_agent_parallel_execution():
153219
success=True,
154220
)
155221

156-
# Mock both agents' adapters
157-
for agent_client in client.agents.values():
158-
with patch.object(agent_client.adapter, "call_tool", return_value=mock_result):
159-
pass
160-
161-
# Test that get_products can be called on multi-agent client
162-
# (actual execution would require proper mocking of asyncio.gather)
163-
assert callable(client.get_products)
222+
# Mock both agents' adapters - keep context active during execution
223+
with patch.object(
224+
client.agents["agent1"].adapter, "call_tool", return_value=mock_result
225+
) as mock1, patch.object(
226+
client.agents["agent2"].adapter, "call_tool", return_value=mock_result
227+
) as mock2:
228+
request = GetProductsRequest(brief="test")
229+
results = await client.get_products(request)
230+
231+
# Verify both agents were called with correct tool name
232+
mock1.assert_called_once_with("get_products", {"brief": "test"})
233+
mock2.assert_called_once_with("get_products", {"brief": "test"})
234+
235+
# Verify results from both agents
236+
assert len(results) == 2
237+
assert all(r.success for r in results)

tests/test_protocols.py

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,26 @@ async def test_call_tool_success(self, a2a_config):
5555
with patch.object(adapter, "_get_client", return_value=mock_client):
5656
result = await adapter.call_tool("get_products", {"brief": "test"})
5757

58+
# Verify the adapter logic - check HTTP request details
59+
mock_client.post.assert_called_once()
60+
call_args = mock_client.post.call_args
61+
62+
# Verify URL includes /message/send endpoint
63+
assert call_args[0][0] == "https://a2a.example.com/message/send"
64+
65+
# Verify headers include auth token (default auth_type is "token", not "bearer")
66+
headers = call_args[1]["headers"]
67+
assert "x-adcp-auth" in headers
68+
assert headers["x-adcp-auth"] == "test_token"
69+
70+
# Verify request body structure matches A2A spec
71+
json_body = call_args[1]["json"]
72+
assert "message" in json_body
73+
assert json_body["message"]["role"] == "user"
74+
assert "parts" in json_body["message"]
75+
assert "context_id" in json_body
76+
77+
# Verify result parsing
5878
assert result.success is True
5979
assert result.status == TaskStatus.COMPLETED
6080
assert result.data == {"result": "success"}
@@ -78,6 +98,14 @@ async def test_call_tool_failure(self, a2a_config):
7898
with patch.object(adapter, "_get_client", return_value=mock_client):
7999
result = await adapter.call_tool("get_products", {"brief": "test"})
80100

101+
# Verify HTTP request was made with correct parameters
102+
mock_client.post.assert_called_once()
103+
call_args = mock_client.post.call_args
104+
assert call_args[0][0] == "https://a2a.example.com/message/send"
105+
assert call_args[1]["headers"]["x-adcp-auth"] == "test_token"
106+
assert "message" in call_args[1]["json"]
107+
108+
# Verify failure handling
81109
assert result.success is False
82110
assert result.status == TaskStatus.FAILED
83111

@@ -103,9 +131,22 @@ async def test_list_tools(self, a2a_config):
103131
with patch.object(adapter, "_get_client", return_value=mock_client):
104132
tools = await adapter.list_tools()
105133

134+
# Verify agent card URL construction (A2A spec uses agent.json)
135+
mock_client.get.assert_called_once()
136+
call_args = mock_client.get.call_args
137+
expected_url = "https://a2a.example.com/.well-known/agent.json"
138+
assert call_args[0][0] == expected_url
139+
140+
# Verify auth headers are included (default auth_type is "token")
141+
headers = call_args[1]["headers"]
142+
assert "x-adcp-auth" in headers
143+
assert headers["x-adcp-auth"] == "test_token"
144+
145+
# Verify tool list parsing
106146
assert len(tools) == 3
107147
assert "get_products" in tools
108148
assert "create_media_buy" in tools
149+
assert "list_creative_formats" in tools
109150

110151

111152
class TestMCPAdapter:
@@ -125,6 +166,15 @@ async def test_call_tool_success(self, mcp_config):
125166
with patch.object(adapter, "_get_session", return_value=mock_session):
126167
result = await adapter.call_tool("get_products", {"brief": "test"})
127168

169+
# Verify MCP protocol details - tool name and arguments
170+
mock_session.call_tool.assert_called_once()
171+
call_args = mock_session.call_tool.call_args
172+
173+
# Verify tool name and params are passed as positional args
174+
assert call_args[0][0] == "get_products"
175+
assert call_args[0][1] == {"brief": "test"}
176+
177+
# Verify result parsing
128178
assert result.success is True
129179
assert result.status == TaskStatus.COMPLETED
130180
assert result.data == [{"type": "text", "text": "Success"}]
@@ -140,6 +190,13 @@ async def test_call_tool_error(self, mcp_config):
140190
with patch.object(adapter, "_get_session", return_value=mock_session):
141191
result = await adapter.call_tool("get_products", {"brief": "test"})
142192

193+
# Verify call_tool was attempted with correct parameters (positional args)
194+
mock_session.call_tool.assert_called_once()
195+
call_args = mock_session.call_tool.call_args
196+
assert call_args[0][0] == "get_products"
197+
assert call_args[0][1] == {"brief": "test"}
198+
199+
# Verify error handling
143200
assert result.success is False
144201
assert result.status == TaskStatus.FAILED
145202
assert "Connection failed" in result.error
@@ -161,6 +218,10 @@ async def test_list_tools(self, mcp_config):
161218
with patch.object(adapter, "_get_session", return_value=mock_session):
162219
tools = await adapter.list_tools()
163220

221+
# Verify list_tools was called on the session
222+
mock_session.list_tools.assert_called_once()
223+
224+
# Verify adapter correctly extracts tool names from MCP response
164225
assert len(tools) == 2
165226
assert "get_products" in tools
166227
assert "create_media_buy" in tools

0 commit comments

Comments
 (0)