Skip to content

Commit bc7df2b

Browse files
bokelleyclaude
andcommitted
refactor: implement code review suggestions for maintainability
Addresses remaining code review feedback from the reviewer agent: 1. CLI Dispatch Refactor (Suggestion #3) - Replace if/elif chain with dict-based TOOL_DISPATCH mapping - Single source of truth for available tools - Lazy initialization of request types to avoid circular imports - Easier to maintain and extend 2. Pydantic Validation Error Handling (Edge Case #6) - Catch ValidationError in _dispatch_tool() - Return user-friendly error messages showing field-level issues - Format: "Invalid request payload for {tool}:\n - field: message" 3. Response Parser Error Context (Suggestion #4) - Add content preview to parse_mcp_content() error messages - Include first 2 items, max 500 chars for debugging - Helps diagnose real-world parsing failures 4. Adapter Response Parsing Edge Case (Edge Case #7) - Fix _parse_response() to explicitly construct TaskResult[T] - Handle success=False or data=None without type: ignore - Provide clear error message when data is missing Benefits: - Maintainability: CLI tool list in one place, easier to add new ADCP methods - User Experience: Clear validation errors instead of Python tracebacks - Debuggability: Content preview helps diagnose parsing issues - Type Safety: Proper typed TaskResult construction without suppressions All tests pass (92/92), linting and type checking clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 4495ebc commit bc7df2b

File tree

3 files changed

+100
-35
lines changed

3 files changed

+100
-35
lines changed

src/adcp/__main__.py

Lines changed: 83 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -80,49 +80,99 @@ async def execute_tool(
8080
print_result(result, json_output)
8181

8282

83+
# Tool dispatch mapping - single source of truth for ADCP methods
84+
# Types are filled at runtime to avoid circular imports
85+
TOOL_DISPATCH: dict[str, tuple[str, type | None]] = {
86+
"get_products": ("get_products", None),
87+
"list_creative_formats": ("list_creative_formats", None),
88+
"sync_creatives": ("sync_creatives", None),
89+
"list_creatives": ("list_creatives", None),
90+
"get_media_buy_delivery": ("get_media_buy_delivery", None),
91+
"list_authorized_properties": ("list_authorized_properties", None),
92+
"get_signals": ("get_signals", None),
93+
"activate_signal": ("activate_signal", None),
94+
"provide_performance_feedback": ("provide_performance_feedback", None),
95+
}
96+
97+
8398
async def _dispatch_tool(client: ADCPClient, tool_name: str, payload: dict[str, Any]) -> Any:
84-
"""Dispatch tool call to appropriate client method."""
99+
"""Dispatch tool call to appropriate client method.
100+
101+
Args:
102+
client: ADCP client instance
103+
tool_name: Name of the tool to invoke
104+
payload: Request payload as dict
105+
106+
Returns:
107+
TaskResult with typed response or error
108+
109+
Raises:
110+
ValidationError: If payload doesn't match request schema (caught and returned as TaskResult)
111+
"""
112+
from pydantic import ValidationError
113+
85114
from adcp.types import generated as gen
86115
from adcp.types.core import TaskResult, TaskStatus
87116

88-
# Dispatch to specific method based on tool name
89-
if tool_name == "get_products":
90-
return await client.get_products(gen.GetProductsRequest(**payload))
91-
elif tool_name == "list_creative_formats":
92-
return await client.list_creative_formats(gen.ListCreativeFormatsRequest(**payload))
93-
elif tool_name == "sync_creatives":
94-
return await client.sync_creatives(gen.SyncCreativesRequest(**payload))
95-
elif tool_name == "list_creatives":
96-
return await client.list_creatives(gen.ListCreativesRequest(**payload))
97-
elif tool_name == "get_media_buy_delivery":
98-
return await client.get_media_buy_delivery(gen.GetMediaBuyDeliveryRequest(**payload))
99-
elif tool_name == "list_authorized_properties":
100-
return await client.list_authorized_properties(
101-
gen.ListAuthorizedPropertiesRequest(**payload)
102-
)
103-
elif tool_name == "get_signals":
104-
return await client.get_signals(gen.GetSignalsRequest(**payload))
105-
elif tool_name == "activate_signal":
106-
return await client.activate_signal(gen.ActivateSignalRequest(**payload))
107-
elif tool_name == "provide_performance_feedback":
108-
return await client.provide_performance_feedback(
109-
gen.ProvidePerformanceFeedbackRequest(**payload)
110-
)
111-
else:
112-
available_tools = [
113-
"get_products",
117+
# Lazy initialization of request types (avoid circular imports)
118+
if TOOL_DISPATCH["get_products"][1] is None:
119+
TOOL_DISPATCH["get_products"] = ("get_products", gen.GetProductsRequest)
120+
TOOL_DISPATCH["list_creative_formats"] = (
114121
"list_creative_formats",
115-
"sync_creatives",
116-
"list_creatives",
122+
gen.ListCreativeFormatsRequest,
123+
)
124+
TOOL_DISPATCH["sync_creatives"] = ("sync_creatives", gen.SyncCreativesRequest)
125+
TOOL_DISPATCH["list_creatives"] = ("list_creatives", gen.ListCreativesRequest)
126+
TOOL_DISPATCH["get_media_buy_delivery"] = (
117127
"get_media_buy_delivery",
128+
gen.GetMediaBuyDeliveryRequest,
129+
)
130+
TOOL_DISPATCH["list_authorized_properties"] = (
118131
"list_authorized_properties",
119-
"get_signals",
120-
"activate_signal",
132+
gen.ListAuthorizedPropertiesRequest,
133+
)
134+
TOOL_DISPATCH["get_signals"] = ("get_signals", gen.GetSignalsRequest)
135+
TOOL_DISPATCH["activate_signal"] = ("activate_signal", gen.ActivateSignalRequest)
136+
TOOL_DISPATCH["provide_performance_feedback"] = (
121137
"provide_performance_feedback",
122-
]
138+
gen.ProvidePerformanceFeedbackRequest,
139+
)
140+
141+
# Check if tool exists
142+
if tool_name not in TOOL_DISPATCH:
143+
available = ", ".join(sorted(TOOL_DISPATCH.keys()))
144+
return TaskResult(
145+
status=TaskStatus.FAILED,
146+
error=f"Unknown tool: {tool_name}. Available tools: {available}",
147+
)
148+
149+
# Get method and request type
150+
method_name, request_type = TOOL_DISPATCH[tool_name]
151+
152+
# Type guard - request_type should be initialized by this point
153+
if request_type is None:
154+
return TaskResult(
155+
status=TaskStatus.FAILED,
156+
error=f"Internal error: {tool_name} request type not initialized",
157+
)
158+
159+
method = getattr(client, method_name)
160+
161+
# Validate and invoke
162+
try:
163+
request = request_type(**payload)
164+
return await method(request)
165+
except ValidationError as e:
166+
# User-friendly error for invalid payloads
167+
error_details = []
168+
for error in e.errors():
169+
field = ".".join(str(loc) for loc in error["loc"])
170+
msg = error["msg"]
171+
error_details.append(f" - {field}: {msg}")
172+
123173
return TaskResult(
124174
status=TaskStatus.FAILED,
125-
error=f"Unknown tool: {tool_name}. Available tools: {', '.join(available_tools)}",
175+
error=f"Invalid request payload for {tool_name}:\n" + "\n".join(error_details),
126176
)
127177

128178

src/adcp/protocols/base.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,17 @@ def _parse_response(self, raw_result: TaskResult[Any], response_type: type[T]) -
4343
Returns:
4444
Typed TaskResult
4545
"""
46+
# Handle failed results or missing data
4647
if not raw_result.success or raw_result.data is None:
47-
return raw_result
48+
# Explicitly construct typed result to satisfy type checker
49+
return TaskResult[T](
50+
status=raw_result.status,
51+
data=None,
52+
success=False,
53+
error=raw_result.error or "No data returned from adapter",
54+
metadata=raw_result.metadata,
55+
debug_info=raw_result.debug_info,
56+
)
4857

4958
try:
5059
# Handle MCP content arrays

src/adcp/utils/response_parser.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,15 @@ def parse_mcp_content(content: list[dict[str, Any]], response_type: type[T]) ->
6464
continue
6565

6666
# If we get here, no content item could be parsed
67+
# Include content preview for debugging (first 2 items, max 500 chars each)
68+
content_preview = json.dumps(content[:2], indent=2, default=str)
69+
if len(content_preview) > 500:
70+
content_preview = content_preview[:500] + "..."
71+
6772
raise ValueError(
6873
f"No valid {response_type.__name__} data found in MCP content. "
69-
f"Content types: {[item.get('type') for item in content]}"
74+
f"Content types: {[item.get('type') for item in content]}. "
75+
f"Content preview:\n{content_preview}"
7076
)
7177

7278

0 commit comments

Comments
 (0)