feat(a2a_agent): add client_config param and deprecate factory#1855
Draft
mkmeral wants to merge 1 commit intostrands-agents:mainfrom
Draft
feat(a2a_agent): add client_config param and deprecate factory#1855mkmeral wants to merge 1 commit intostrands-agents:mainfrom
mkmeral wants to merge 1 commit intostrands-agents:mainfrom
Conversation
…henticated card resolution A2AAgent.get_agent_card() was creating a bare httpx client for card discovery, ignoring the a2a_client_factory parameter. This caused 403 errors on authenticated endpoints (e.g. AgentCore with IAM/OAuth). Add a new client_config: ClientConfig parameter as the recommended way to configure authentication. When provided, its httpx client is used for both card resolution and message sending via ClientFactory.connect(). Deprecate a2a_client_factory with a warning. When the deprecated factory is provided, factory.create() is used for client creation to preserve interceptors, consumers, and custom transports. For card resolution, client_config takes precedence; otherwise the factory's internal config is used as a fallback. Three client creation paths: - client_config only: ClientFactory.connect() with config, client cached - factory (deprecated): factory.create(card), client cached - neither: transient ClientFactory.connect() per call
|
Issue: Consider adding This PR introduces a new public API parameter ( Suggestion: Consider adding the |
|
Issue: Missing Documentation PR section This PR introduces a new public API parameter (
Suggestion: Please add a "Documentation PR" section to the PR description. If documentation updates are needed, create a PR in the docs repository. If not needed, provide a brief justification (e.g., "A2AAgent is not yet documented in the public docs"). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Assessment: Request Changes
Well-designed API change that fixes an authentication bug while maintaining backward compatibility. The implementation follows Option C from your design analysis, which is the right choice for preserving existing integrations while providing a cleaner path forward.
Review Details
- Documentation: Missing "Documentation PR" section in the PR description. This is required for public API changes—please add either a link to a docs PR or a justification for why docs updates aren't needed.
- API Review: Consider adding the
needs-api-reviewlabel per API bar-raising guidelines, since this introduces a new public parameter and deprecates an existing one. - Code Quality: Implementation is clean with proper type annotations, Google-style docstrings, and structured logging.
- Testing: Comprehensive test coverage (30 tests) covering all client creation paths, caching behavior, and deprecation warnings.
The code itself is solid—just need the documentation requirement addressed before merging.
🔴 Adversarial Testing Report for PR #1855Summary: FINDINGS — 12 design observations, 1 potential bug Scope: Migration from Tests written: 23 | Tests passing: 23 | Findings: 12 🚨 CRITICAL: Features LOST When Migrating from Factory to client_configUsers who migrate from 1. Interceptors (Authentication hooks like OAuth/API keys)# Factory path: interceptors WORK (preserved via factory.create())
factory = ClientFactory(config)
client = factory.create(card, interceptors=[MyOAuthInterceptor()])
# client_config path: NO WAY to pass interceptors
agent = A2AAgent(endpoint=url, client_config=config)
# ClientFactory.connect() is called WITHOUT interceptors!Impact: Users who rely on per-request auth interceptors CANNOT migrate. 2. Consumers (Event callbacks)# Factory path: consumers attached via factory._consumers
factory = ClientFactory(config, consumers=[my_consumer])
# client_config path: NO WAY to pass consumers
agent = A2AAgent(endpoint=url, client_config=config)
# ClientFactory.connect() is called WITHOUT consumers!3. Custom Transports (gRPC, etc.)# Factory path: custom transports via factory.register()
factory.register("grpc", custom_grpc_producer)
# client_config path: NO WAY to register extra transports
agent = A2AAgent(endpoint=url, client_config=config)
# ClientFactory.connect() is called WITHOUT extra_transports!Impact: Users on gRPC transport CANNOT migrate to 4. Signature Verifier (Security)# ClientFactory.connect() supports this:
await ClientFactory.connect(url, signature_verifier=verify_fn)
# A2AAgent has NO way to pass signature_verifier
agent = A2AAgent(endpoint=url) # No signature_verifier param!5. Card Resolver Customization# ClientFactory.connect() supports:
await ClientFactory.connect(
url,
relative_card_path="/custom/.well-known/agent.json",
resolver_http_kwargs={"headers": {"X-Custom": "value"}}
)
# A2AAgent has NO way to customize card resolution path or HTTP kwargs
|
| Operation | Uses |
|---|---|
| Card resolution | client_config.httpx_client ✅ |
| Client creation | factory.create() (ignores client_config) |
Confusion: User might expect client_config to be used consistently for both.
🔄 factory.create() Only Gets Card
Finding 13: Limited factory.create() call
# Current:
self._a2a_client = self._a2a_client_factory.create(agent_card)
# But factory.create() signature supports:
factory.create(card, consumers=None, interceptors=None, extensions=None)Even with factory path, users can't pass per-call interceptors/consumers because A2AAgent doesn't expose those params.
⏱️ Transient Client Inefficiency
Finding 15: Card resolved on every call
Without client_config or factory, ClientFactory.connect() is called on every _get_or_create_client() invocation:
# No caching without config:
async def _get_or_create_client(self):
...
return await ClientFactory.connect(self.endpoint) # Every time!Impact: Multiple network roundtrips for card resolution on each message send.
🔒 No Card Refresh Mechanism
Finding 14: Stale card cache
Once _agent_card is cached, there's no public method to refresh it. If the remote agent updates its card (new skills, capabilities), the local cache is stale forever.
Missing: agent.refresh_card() or agent.clear_cache() method.
⏰ Timeout Inconsistency
Finding 12: Timeout only used for transient httpx
# Agent's timeout:
agent = A2AAgent(endpoint=url, timeout=30, client_config=config)
# But when config has httpx_client, timeout is IGNORED:
if config.httpx_client is not None:
resolver = A2ACardResolver(httpx_client=config.httpx_client, ...) # No timeout!User's timeout=30 is silently ignored when they provide client_config.httpx_client.
📊 Summary Table
| # | Finding | Severity | Migration Impact |
|---|---|---|---|
| 1 | Interceptors lost | 🔴 High | CANNOT migrate |
| 2 | Consumers lost | 🔴 High | CANNOT migrate |
| 3 | Custom transports lost | 🔴 High | CANNOT migrate |
| 4 | Private attr fragility | 🟡 Medium | Silent auth failures |
| 5 | Empty string bug | 🟡 Medium | Incorrect name/desc |
| 6 | Precedence confusion | 🟡 Medium | Unexpected behavior |
| 7 | Stale card cache | 🟡 Medium | No refresh possible |
| 8 | Transient inefficiency | 🟡 Medium | Performance hit |
| 9 | Timeout inconsistency | 🟢 Low | Surprising behavior |
| 10 | Signature verifier lost | 🟡 Medium | Security feature gap |
| 11 | Card resolver kwargs lost | 🟢 Low | Customization limited |
| 12 | factory.create() limited | 🟢 Low | Per-call flexibility |
🎯 Recommendation
The PR correctly identifies that card resolution was broken with factory (main fix). However, the migration path documentation should clearly state:
⚠️ Users who rely on interceptors, consumers, or custom transports CANNOT fully migrate toclient_config. They must continue using the deprecateda2a_client_factoryuntil support for these features is added toA2AAgent.
Suggested future enhancement:
class A2AAgent:
def __init__(
self,
endpoint: str,
*,
client_config: ClientConfig | None = None,
interceptors: list[ClientCallInterceptor] | None = None, # NEW
consumers: list[Consumer] | None = None, # NEW
extra_transports: dict | None = None, # NEW
signature_verifier: Callable | None = None, # NEW
...
):Artifacts: Tests available at tests/strands/agent/test_adversarial_a2a_agent.py (23 tests)
📌 Follow-up Required@mkmeral requested a follow-up on this PR. Adversarial Testing Summary (click to expand)Result: FINDINGS — 12 design observations, 1 bug Key Items to Address:
Comment left by strands-coder for @mkmeral to follow up later. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🔍 Comprehensive Code Review: PR #1855Overall Assessment: ✅ Strong implementation with thoughtful API design and excellent test coverage. The solution correctly addresses the authenticated card resolution bug while maintaining backward compatibility. However, there are several areas where clarity, documentation, and API design could be improved. 1. 🎯 Code Quality✅ Strengths
|
| Name | Pros | Cons | Verdict |
|---|---|---|---|
client_config |
Short, matches A2A SDK naming | Too generic, ambiguous | 😐 |
a2a_config |
Clear it's A2A-specific | Slightly longer | ✅ Better |
auth_config |
Emphasizes main use case | Misleading (can configure non-auth too) | ❌ |
transport_config |
Accurate but technical | Not obvious | ❌ |
a2a_client_config |
Most explicit | Verbose | ✅ Best |
Recommendation: Rename to a2a_client_config for clarity.
def __init__(
self,
endpoint: str,
*,
a2a_client_config: ClientConfig | None = None,
a2a_client_factory: ClientFactory | None = None, # deprecated
):Issue 9: Deprecation Warnings - Actionability
Current Warning: ✅ Good stacklevel
warnings.warn(
"a2a_client_factory is deprecated, use client_config instead. "
"a2a_client_factory will be removed in a future version.",
DeprecationWarning,
stacklevel=2, # ✅ Correct - points to caller, not __init__
)Missing:
- Version number: "future version" is vague → should be "v2.0.0"
- Link to migration guide: No URL provided
- Feature parity warning: Doesn't mention lost features
Enhanced Warning:
warnings.warn(
"a2a_client_factory is deprecated and will be removed in v2.0.0. "
f"Migrate to client_config for authentication: "
f"A2AAgent(endpoint='{self.endpoint}', client_config=ClientConfig(...)). "
"Note: interceptors, consumers, and custom transports are not yet "
"supported with client_config. If you need these features, continue "
"using a2a_client_factory until v2.0.0. "
"See: https://github.com/strands-agents/sdk-python/wiki/A2AAgent-Migration",
DeprecationWarning,
stacklevel=2,
)4. 🧪 Testing
✅ Strengths
- Comprehensive Coverage: 30 tests covering all code paths
- Clear Test Names:
test_get_agent_card_client_config_takes_precedence_over_factory - Proper Mocking: AsyncMock usage for async operations
- Edge Cases: Tests for missing
_configattribute, caching, etc.
⚠️ Missing Test Cases
Test 1: Verify Auth Bug is Actually Fixed
Current Tests: Mock everything, don't verify the actual bug
Missing: Integration test that proves 403s are resolved
@pytest.mark.asyncio
async def test_authenticated_card_resolution_with_client_config():
"""Integration test: Verify client_config auth is used for card resolution.
This test verifies the bug fix: previously, a bare httpx client was created
for card resolution, ignoring any auth configured via factory. Now, when
client_config is provided, its authenticated httpx client is used.
"""
# Create a mock httpx client that tracks if it was used
mock_auth_client = AsyncMock(spec=httpx.AsyncClient)
auth_used = False
# Mock the resolver to track which client was used
original_resolver_init = A2ACardResolver.__init__
def tracked_init(self, *, httpx_client, base_url):
nonlocal auth_used
if httpx_client is mock_auth_client:
auth_used = True
return original_resolver_init(self, httpx_client=httpx_client, base_url=base_url)
with patch.object(A2ACardResolver, '__init__', tracked_init):
with patch.object(A2ACardResolver, 'get_agent_card', AsyncMock(return_value=mock_agent_card)):
agent = A2AAgent(
endpoint="http://localhost:8000",
client_config=ClientConfig(httpx_client=mock_auth_client)
)
await agent.get_agent_card()
# Verify the authenticated client was used
assert auth_used, "Bug not fixed: authenticated httpx client was not used for card resolution"Test 2: Verify Client Cleanup
@pytest.mark.asyncio
async def test_cached_client_cleanup():
"""Test that cached client can be properly cleaned up."""
config = ClientConfig()
agent = A2AAgent(endpoint="http://localhost:8000", client_config=config)
# Create and cache a client
mock_client = AsyncMock()
mock_client.close = AsyncMock()
with patch("strands.agent.a2a_agent.ClientFactory.connect", return_value=mock_client):
client1 = await agent._get_or_create_client()
assert agent._a2a_client is mock_client
# If cleanup method exists, verify it works
if hasattr(agent, 'close'):
await agent.close()
assert agent._a2a_client is None
if hasattr(mock_client, 'close'):
mock_client.close.assert_called_once()Test 3: Feature Parity with Factory
@pytest.mark.asyncio
async def test_factory_preserves_interceptors_and_consumers():
"""Verify that factory.create() preserves interceptors and consumers.
This documents what features are LOST when migrating to client_config.
"""
mock_interceptor = MagicMock()
mock_consumer = MagicMock()
mock_factory = MagicMock()
mock_factory._config = MagicMock(spec=ClientConfig)
mock_factory._config.httpx_client = None
# Track what factory.create() was called with
created_with_card = None
def track_create(card):
nonlocal created_with_card
created_with_card = card
return MagicMock() # Return mock client
mock_factory.create = track_create
with warnings.catch_warnings():
warnings.simplefilter("ignore", DeprecationWarning)
agent = A2AAgent(endpoint="http://localhost:8000", a2a_client_factory=mock_factory)
mock_card = MagicMock(spec=AgentCard)
mock_card.name = "test"
mock_card.description = "test"
agent._agent_card = mock_card
client = await agent._get_or_create_client()
# Verify factory.create() was called (preserves interceptors/consumers)
assert created_with_card is mock_card
# Document limitation: client_config path cannot preserve these
# TODO: Add support for interceptors/consumers with client_config before v2.0.0Test 4: Transient Client Behavior
@pytest.mark.asyncio
async def test_transient_client_creates_new_instance_per_call():
"""Verify transient clients are created per-call without caching."""
agent = A2AAgent(endpoint="http://localhost:8000") # No config or factory
mock_client_1 = AsyncMock()
mock_client_2 = AsyncMock()
with patch("strands.agent.a2a_agent.ClientFactory.connect", side_effect=[mock_client_1, mock_client_2]):
client1 = await agent._get_or_create_client()
client2 = await agent._get_or_create_client()
# Should create new clients each time (no caching)
assert client1 is mock_client_1
assert client2 is mock_client_2
assert client1 is not client2
# Verify no client was cached
assert agent._a2a_client is None5. 📚 Documentation
⚠️ Issues to Address
Issue 10: Docstrings - Missing Key Information
Current __init__ docstring: Good foundation but missing:
- Return type for methods: Some methods lack
Returns:section - Raises section:
_resolve_client_config()could raise AttributeError - Examples: No inline examples for common use cases
Enhanced Docstring:
def __init__(
self,
endpoint: str,
*,
name: str | None = None,
description: str | None = None,
timeout: int = _DEFAULT_TIMEOUT,
client_config: ClientConfig | None = None,
a2a_client_factory: ClientFactory | None = None,
):
"""Initialize A2A agent for remote agent communication.
Args:
endpoint: The base URL of the remote A2A agent (e.g., "https://agent.example.com").
name: Optional agent name override. If not provided, populated from agent card.
description: Optional description override. If not provided, populated from agent card.
timeout: HTTP operation timeout in seconds (default: 300).
client_config: A2A client configuration for authentication and transport settings.
Recommended for most use cases. Supports SigV4, OAuth, bearer tokens, etc.
The httpx client configured here is used for both card discovery and message sending.
a2a_client_factory: **Deprecated (will be removed in v2.0.0)**.
Use client_config instead unless you need interceptors, consumers, or custom transports
(these features are not yet supported with client_config).
Warnings:
DeprecationWarning: Emitted when a2a_client_factory is provided.
Examples:
Basic usage (unauthenticated):
>>> agent = A2AAgent(endpoint="https://public-agent.example.com")
>>> result = await agent.invoke_async("Hello")
With authentication (recommended):
>>> import httpx
>>> from a2a.client import ClientConfig
>>>
>>> auth_client = httpx.AsyncClient(
... headers={"Authorization": "Bearer token123"}
... )
>>> agent = A2AAgent(
... endpoint="https://secure-agent.example.com",
... client_config=ClientConfig(httpx_client=auth_client)
... )
>>> result = await agent.invoke_async("Hello")
Migration from deprecated factory:
>>> # Before (deprecated)
>>> factory = ClientFactory(client_config=ClientConfig(httpx_client=auth_client))
>>> agent = A2AAgent(endpoint=url, a2a_client_factory=factory)
>>>
>>> # After (recommended)
>>> agent = A2AAgent(endpoint=url, client_config=ClientConfig(httpx_client=auth_client))
"""Issue 11: Missing Migration Guide
Problem: PR description has excellent migration examples, but they're not in code documentation.
Suggestion: Add a module-level docstring with migration guide:
"""A2A Agent client for Strands Agents.
This module provides the A2AAgent class, which acts as a client wrapper for remote A2A agents,
allowing them to be used standalone or as part of multi-agent patterns.
Migration Guide (a2a_client_factory → client_config)
-----------------------------------------------------
Version 1.x introduced client_config as a cleaner way to configure A2A authentication.
The legacy a2a_client_factory parameter will be removed in v2.0.0.
Simple Authentication (SigV4, OAuth, Bearer Tokens)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Before::
from a2a.client import ClientFactory, ClientConfig
import httpx
auth_client = httpx.AsyncClient(headers={"Authorization": "Bearer token"})
factory = ClientFactory(client_config=ClientConfig(httpx_client=auth_client))
agent = A2AAgent(endpoint="https://agent.example.com", a2a_client_factory=factory)
After::
from a2a.client import ClientConfig
import httpx
auth_client = httpx.AsyncClient(headers={"Authorization": "Bearer token"})
agent = A2AAgent(
endpoint="https://agent.example.com",
client_config=ClientConfig(httpx_client=auth_client)
)
Advanced Features (Interceptors, Consumers, Custom Transports)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
If you use factory-only features, continue using a2a_client_factory until v2.0.0::
# Interceptors (e.g., OAuth refresh, API key injection)
factory = ClientFactory(config)
client = factory.create(card, interceptors=[MyOAuthInterceptor()])
# Consumers (e.g., event callbacks)
factory = ClientFactory(config, consumers=[my_event_consumer])
# Custom transports (e.g., gRPC)
factory.register("grpc", custom_grpc_producer)
**These features are not yet supported with client_config.** Track progress:
https://github.com/strands-agents/sdk-python/issues/XXXX
"""Issue 12: Error Conditions Not Documented
Missing from docstrings:
async def get_agent_card(self) -> AgentCard:
"""Fetch and return the remote agent's card.
...
Raises:
httpx.HTTPStatusError: If the agent card endpoint returns an error status (4xx, 5xx).
httpx.TimeoutException: If the request exceeds the configured timeout.
httpx.ConnectError: If unable to connect to the agent endpoint.
"""📊 Summary Table
| # | Issue | Severity | Action Required | Blocks Merge? |
|---|---|---|---|---|
| 1 | client_config naming ambiguity |
🟡 Medium | Rename to a2a_client_config |
No |
| 2 | Empty string bug | 🟡 Medium | Fix falsy checks | Yes |
| 3 | Private _config access |
🟡 Medium | Document or file a2a-sdk issue | No |
| 4 | Precedence documentation | 🟡 Medium | Enhance docstring | No |
| 5 | _resolve_client_config() |
✅ Good | None | No |
| 6 | Cached client cleanup | 🟡 Medium | Add close() method |
No |
| 7 | Two-parameter confusion | 🟢 Low | Enhance warning message | No |
| 8 | Parameter naming | 🟡 Medium | Consider a2a_client_config |
No |
| 9 | Deprecation actionability | 🟡 Medium | Add version, URL, features warning | No |
| 10 | Docstring completeness | 🟢 Low | Add examples, raises, returns | No |
| 11 | Migration guide missing | 🟡 Medium | Add module-level migration docs | No |
| 12 | Error conditions | 🟢 Low | Document exceptions | No |
Missing Tests
| Test | Priority | Blocks Merge? |
|---|---|---|
| Auth bug verification (integration) | 🔴 High | Yes |
| Client cleanup | 🟡 Medium | No |
| Feature parity with factory | 🟢 Low | No |
| Transient client behavior | 🟢 Low | No |
🎯 Recommendations
Must Fix Before Merge
- ✅ Fix empty string bug (Issue ci: update mypy requirement from <1.0.0,>=0.981 to >=0.981,<2.0.0 #2)
- ✅ Add integration test for auth bug (Test ci: update sphinx requirement from <6.0.0,>=5.0.0 to >=5.0.0,<9.0.0 #1)
Should Fix Before Merge
- 🟡 Enhance deprecation warning with version and feature parity notes (Issue models - anthropic #9)
- 🟡 Improve precedence documentation in docstring (Issue ci: update pre-commit requirement from <4.2.0,>=3.2.0 to >=3.2.0,<4.3.0 #4)
- 🟡 Add cleanup method for cached client (Issue ci: update ruff requirement from <0.5.0,>=0.4.4 to >=0.4.4,<0.12.0 #6)
Nice to Have (Post-Merge)
- 🟢 Rename
client_configtoa2a_client_config(breaking change, wait for v2.0) - 🟢 Add module-level migration guide (Issue feat: add Llama API as model provider #11)
- 🟢 File issue in a2a-sdk for public
get_config()method (Issue ci: update sphinx-rtd-theme requirement from <2.0.0,>=1.0.0 to >=1.0.0,<4.0.0 #3) - 🟢 Document error conditions in docstrings (Issue models - anthropic #12)
🏆 Conclusion
This is a well-designed solution that correctly balances backward compatibility with API improvement. The implementation is clean, well-tested, and thoughtfully documented.
The core fix is sound: authenticated card resolution now works correctly.
Main areas for improvement:
- Correctness: Empty string bug (must fix)
- Testing: Add integration test proving the bug is fixed (must have)
- Documentation: Enhance warnings and migration guidance (should have)
- API Clarity: Consider parameter renaming and cleanup methods (nice to have)
Overall Grade: A- (would be A+ with the two must-fix items addressed)
Great work on the design analysis in the PR description - that level of thorough option evaluation is exemplary! 🎉
🐛 Code Fix: Empty String Handling BugCurrent Code (Buggy)# src/strands/agent/a2a_agent.py, lines ~198-203
# Populate name from card if not set
if self.name is None and self._agent_card.name:
self.name = self._agent_card.name
# Populate description from card if not set
if self.description is None and self._agent_card.description:
self.description = self._agent_card.descriptionProblemIf the agent card returns # When self._agent_card.name == ""
if self.name is None and self._agent_card.name: # "" is falsy, condition fails
self.name = self._agent_card.name # Never executed
# Result: self.name stays None instead of being set to ""Fixed Code# Populate name from card if not set
if self.name is None and self._agent_card.name is not None:
self.name = self._agent_card.name
# Populate description from card if not set
if self.description is None and self._agent_card.description is not None:
self.description = self._agent_card.descriptionTest Case# tests/strands/agent/test_a2a_agent.py
@pytest.mark.asyncio
async def test_get_agent_card_handles_empty_string_name_and_description(mock_httpx_client):
"""Test that empty string name/description from card are preserved."""
# Create agent card with empty strings (not None)
mock_card = MagicMock(spec=AgentCard)
mock_card.name = "" # Empty string, not None
mock_card.description = "" # Empty string, not None
agent = A2AAgent(endpoint="http://localhost:8000")
with patch("strands.agent.a2a_agent.httpx.AsyncClient", return_value=mock_httpx_client):
with patch("strands.agent.a2a_agent.A2ACardResolver") as mock_resolver_class:
mock_resolver = AsyncMock()
mock_resolver.get_agent_card = AsyncMock(return_value=mock_card)
mock_resolver_class.return_value = mock_resolver
await agent.get_agent_card()
# Bug: agent.name should be "" but it's None
assert agent.name == "", f"Expected empty string, got {agent.name!r}"
assert agent.description == "", f"Expected empty string, got {agent.description!r}"
@pytest.mark.asyncio
async def test_get_agent_card_handles_none_vs_empty_string():
"""Test explicit None vs empty string handling."""
# Test matrix:
# | agent.name | card.name | Expected |
# | None | None | None |
# | None | "" | "" |
# | None | "foo" | "foo" |
# | "custom" | "foo" | "custom" | (no override)
test_cases = [
(None, None, None),
(None, "", ""),
(None, "card-name", "card-name"),
("custom-name", "card-name", "custom-name"),
]
for agent_name, card_name, expected in test_cases:
mock_card = MagicMock(spec=AgentCard)
mock_card.name = card_name
mock_card.description = None
agent = A2AAgent(endpoint="http://localhost:8000", name=agent_name)
mock_httpx = AsyncMock()
mock_httpx.__aenter__.return_value = mock_httpx
mock_httpx.__aexit__.return_value = None
with patch("strands.agent.a2a_agent.httpx.AsyncClient", return_value=mock_httpx):
with patch("strands.agent.a2a_agent.A2ACardResolver") as mock_resolver_class:
mock_resolver = AsyncMock()
mock_resolver.get_agent_card = AsyncMock(return_value=mock_card)
mock_resolver_class.return_value = mock_resolver
await agent.get_agent_card()
assert agent.name == expected, (
f"Test failed: agent_name={agent_name!r}, card_name={card_name!r}, "
f"expected={expected!r}, got={agent.name!r}"
)RationaleIn Python, empty strings are falsy but semantically distinct from None:
The current code treats |
✅ Integration Test: Verify Auth Bug FixThe Bug You FixedBefore this PR: After this PR: When Missing TestYour current tests mock everything - they don't actually verify that the authenticated client is used for card resolution. They verify the structure is correct, but not the behavior. Proposed Integration Test# tests/strands/agent/test_a2a_agent.py
import pytest
from unittest.mock import AsyncMock, MagicMock, patch, call
import httpx
from a2a.client import A2ACardResolver, ClientConfig
from a2a.types import AgentCard
from strands.agent.a2a_agent import A2AAgent
@pytest.mark.asyncio
async def test_client_config_httpx_client_is_used_for_card_resolution():
"""Integration test: Verify client_config's httpx_client is used for card resolution.
This is the PRIMARY test for the bug fix. Before this PR, card resolution
used a bare httpx.AsyncClient, ignoring any authentication. Now it should
use the httpx client from client_config.
Scenario:
1. User provides an authenticated httpx client via client_config
2. Agent resolves the card
3. Verify that the AUTHENTICATED client was used (not a bare one)
"""
# Create a mock authenticated httpx client
mock_auth_client = MagicMock(spec=httpx.AsyncClient)
mock_auth_client.headers = {"Authorization": "Bearer secret-token"}
# Create ClientConfig with the authenticated client
config = ClientConfig(httpx_client=mock_auth_client)
# Create mock agent card
mock_card = AgentCard(
name="test-agent",
description="Test",
url="http://localhost:8000",
version="1.0.0",
capabilities={},
default_input_modes=["text/plain"],
default_output_modes=["text/plain"],
skills=[],
)
# Track which httpx client was passed to A2ACardResolver
resolver_httpx_client = None
def track_resolver_init(self, *, httpx_client, base_url, **kwargs):
"""Track which httpx_client was used to create the resolver."""
nonlocal resolver_httpx_client
resolver_httpx_client = httpx_client
# Don't actually initialize (we'll mock get_agent_card)
with patch.object(A2ACardResolver, '__init__', track_resolver_init):
with patch.object(A2ACardResolver, 'get_agent_card', AsyncMock(return_value=mock_card)):
agent = A2AAgent(
endpoint="http://localhost:8000",
client_config=config
)
card = await agent.get_agent_card()
# CRITICAL ASSERTION: Verify the authenticated client was used
assert resolver_httpx_client is mock_auth_client, (
f"BUG NOT FIXED: Expected authenticated httpx client {mock_auth_client}, "
f"but resolver was initialized with {resolver_httpx_client}. "
f"This means card resolution is still using a bare httpx client, "
f"which will cause 403 errors on authenticated endpoints."
)
assert card == mock_card
@pytest.mark.asyncio
async def test_factory_config_fallback_for_card_resolution():
"""Test that factory's _config is used as fallback for card resolution.
When only a2a_client_factory is provided (deprecated path), the factory's
internal _config should be extracted and used for card resolution.
"""
# Create mock authenticated client
mock_auth_client = MagicMock(spec=httpx.AsyncClient)
mock_auth_client.headers = {"Authorization": "Bearer factory-token"}
# Create mock factory with config
mock_factory_config = MagicMock(spec=ClientConfig)
mock_factory_config.httpx_client = mock_auth_client
mock_factory = MagicMock()
mock_factory._config = mock_factory_config
mock_card = AgentCard(
name="test-agent",
description="Test",
url="http://localhost:8000",
version="1.0.0",
capabilities={},
default_input_modes=["text/plain"],
default_output_modes=["text/plain"],
skills=[],
)
# Track which httpx client was passed to A2ACardResolver
resolver_httpx_client = None
def track_resolver_init(self, *, httpx_client, base_url, **kwargs):
nonlocal resolver_httpx_client
resolver_httpx_client = httpx_client
with patch.object(A2ACardResolver, '__init__', track_resolver_init):
with patch.object(A2ACardResolver, 'get_agent_card', AsyncMock(return_value=mock_card)):
import warnings
with warnings.catch_warnings():
warnings.simplefilter("ignore", DeprecationWarning)
agent = A2AAgent(
endpoint="http://localhost:8000",
a2a_client_factory=mock_factory
)
card = await agent.get_agent_card()
# Verify factory's authenticated client was used for card resolution
assert resolver_httpx_client is mock_auth_client, (
f"Expected factory's httpx client {mock_auth_client}, "
f"but got {resolver_httpx_client}"
)
@pytest.mark.asyncio
async def test_client_config_precedence_over_factory_for_card_resolution():
"""Test that client_config takes precedence over factory for card resolution.
When BOTH client_config and a2a_client_factory are provided, client_config
should be used for card resolution (not the factory's config).
"""
# Two different authenticated clients
client_config_httpx = MagicMock(spec=httpx.AsyncClient)
client_config_httpx.name = "client_config_httpx" # For debugging
factory_config_httpx = MagicMock(spec=httpx.AsyncClient)
factory_config_httpx.name = "factory_config_httpx" # For debugging
# Create client_config
explicit_config = ClientConfig(httpx_client=client_config_httpx)
# Create factory with different config
factory_config = MagicMock(spec=ClientConfig)
factory_config.httpx_client = factory_config_httpx
mock_factory = MagicMock()
mock_factory._config = factory_config
mock_card = AgentCard(
name="test-agent",
description="Test",
url="http://localhost:8000",
version="1.0.0",
capabilities={},
default_input_modes=["text/plain"],
default_output_modes=["text/plain"],
skills=[],
)
# Track which httpx client was used
resolver_httpx_client = None
def track_resolver_init(self, *, httpx_client, base_url, **kwargs):
nonlocal resolver_httpx_client
resolver_httpx_client = httpx_client
with patch.object(A2ACardResolver, '__init__', track_resolver_init):
with patch.object(A2ACardResolver, 'get_agent_card', AsyncMock(return_value=mock_card)):
import warnings
with warnings.catch_warnings():
warnings.simplefilter("ignore", DeprecationWarning)
agent = A2AAgent(
endpoint="http://localhost:8000",
client_config=explicit_config,
a2a_client_factory=mock_factory
)
card = await agent.get_agent_card()
# CRITICAL: client_config should win, not factory
assert resolver_httpx_client is client_config_httpx, (
f"PRECEDENCE BUG: Expected client_config's httpx client, "
f"but got {'factory httpx' if resolver_httpx_client is factory_config_httpx else 'unknown'}. "
f"When both client_config and factory are provided, client_config should take precedence."
)
@pytest.mark.asyncio
async def test_bare_httpx_client_used_when_no_config():
"""Test that a transient bare httpx client is used when no config provided.
When neither client_config nor a2a_client_factory is provided, a transient
httpx.AsyncClient should be created for card resolution.
"""
mock_card = AgentCard(
name="test-agent",
description="Test",
url="http://localhost:8000",
version="1.0.0",
capabilities={},
default_input_modes=["text/plain"],
default_output_modes=["text/plain"],
skills=[],
)
# Track httpx.AsyncClient creation
created_clients = []
class TrackedAsyncClient(httpx.AsyncClient):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
created_clients.append(self)
with patch('strands.agent.a2a_agent.httpx.AsyncClient', TrackedAsyncClient):
with patch.object(A2ACardResolver, 'get_agent_card', AsyncMock(return_value=mock_card)):
agent = A2AAgent(endpoint="http://localhost:8000")
card = await agent.get_agent_card()
# Verify a transient httpx client was created
assert len(created_clients) > 0, "No httpx.AsyncClient was created"
assert card == mock_cardWhy This Test MattersThis test directly verifies the bug is fixed by:
Without this test, you're only verifying that the mocks are called correctly, not that the actual authentication flow works. Alternative: Real HTTP TestIf you want an even stronger test (requires test server): @pytest.mark.asyncio
@pytest.mark.integration # Mark as integration test
async def test_authenticated_card_resolution_with_real_http():
"""Real HTTP test: Verify auth headers are sent in card resolution request.
Requires a test HTTP server that returns 403 without auth, 200 with auth.
"""
from aioresponses import aioresponses
endpoint = "http://localhost:8000"
card_url = f"{endpoint}/.well-known/agent.json"
mock_card_json = {
"name": "test-agent",
"description": "Test",
"url": endpoint,
"version": "1.0.0",
"capabilities": {},
"default_input_modes": ["text/plain"],
"default_output_modes": ["text/plain"],
"skills": [],
}
with aioresponses() as m:
# Mock server: returns 403 without auth header
m.get(
card_url,
status=403,
callback=lambda url, **kwargs: (
mock_card_json if 'Authorization' in kwargs.get('headers', {}) else None
)
)
# Create authenticated client
auth_client = httpx.AsyncClient(
headers={"Authorization": "Bearer secret-token"}
)
agent = A2AAgent(
endpoint=endpoint,
client_config=ClientConfig(httpx_client=auth_client)
)
# Should succeed (auth header sent)
card = await agent.get_agent_card()
assert card.name == "test-agent"
# Verify the request included auth header
# (aioresponses tracks all requests)Choose the first approach (mock-based) for fast unit tests, or the second (real HTTP) for stronger integration testing. |
🚨 Deprecation Warning EnhancementCurrent Warning (Too Generic)if a2a_client_factory is not None:
warnings.warn(
"a2a_client_factory is deprecated, use client_config instead. "
"a2a_client_factory will be removed in a future version.",
DeprecationWarning,
stacklevel=2,
)Problems
The RealityNot all users can migrate from
Users who rely on interceptors, consumers, or custom transports cannot migrate until these features are added to Enhanced Warning (Actionable)if a2a_client_factory is not None:
warnings.warn(
"a2a_client_factory is deprecated and will be removed in v2.0.0.\n\n"
"Migration Path:\n"
" 1. If you ONLY use authentication (httpx_client), migrate to client_config:\n"
f" A2AAgent(endpoint='{endpoint}', client_config=ClientConfig(httpx_client=...))\n\n"
" 2. If you use interceptors, consumers, or custom transports:\n"
" Continue using a2a_client_factory until v2.0.0 adds support for these features.\n"
" Track progress: https://github.com/strands-agents/sdk-python/issues/XXXX\n\n"
"See migration guide: https://github.com/strands-agents/sdk-python/wiki/A2AAgent-Migration",
DeprecationWarning,
stacklevel=2,
)Benefits
Alternative: Conditional Warning Based on UsageEven better - detect what features the user is actually using and tailor the warning: if a2a_client_factory is not None:
# Detect advanced feature usage
has_consumers = bool(getattr(a2a_client_factory, '_consumers', []))
has_custom_transports = len(getattr(a2a_client_factory, '_transports', {})) > 1 # More than default
if has_consumers or has_custom_transports:
# User relies on advanced features - cannot migrate yet
warnings.warn(
f"a2a_client_factory is deprecated and will be removed in v2.0.0.\n\n"
f"Your factory uses advanced features (consumers={has_consumers}, "
f"custom_transports={has_custom_transports}) that are not yet supported "
f"with client_config. Continue using a2a_client_factory until v2.0.0 "
f"adds support. Track progress: https://github.com/strands-agents/sdk-python/issues/XXXX",
DeprecationWarning,
stacklevel=2,
)
else:
# Simple auth case - can migrate
warnings.warn(
f"a2a_client_factory is deprecated and will be removed in v2.0.0.\n\n"
f"Migrate to client_config:\n"
f" A2AAgent(endpoint='{endpoint}', client_config=ClientConfig(httpx_client=...))\n\n"
f"See: https://github.com/strands-agents/sdk-python/wiki/A2AAgent-Migration",
DeprecationWarning,
stacklevel=2,
)This customized warning helps users understand:
Documentation LinkCreate a wiki page or issue with detailed migration guide: https://github.com/strands-agents/sdk-python/wiki/A2AAgent-Migration # A2AAgent Migration Guide: a2a_client_factory → client_config
## Can I Migrate?
### ✅ You CAN migrate if:
- You only use `a2a_client_factory` for authentication (httpx_client)
- You don't use interceptors, consumers, or custom transports
### ❌ You CANNOT migrate yet if:
- You use interceptors (e.g., OAuth token refresh, API key injection)
- You use consumers (event callbacks)
- You use custom transports (e.g., gRPC)
## Migration Steps (Simple Auth)
### Before
```python
from a2a.client import ClientFactory, ClientConfig
import httpx
auth_client = httpx.AsyncClient(
headers={"Authorization": "Bearer token123"}
)
factory = ClientFactory(client_config=ClientConfig(httpx_client=auth_client))
agent = A2AAgent(
endpoint="https://protected-agent.example.com",
a2a_client_factory=factory
)Afterfrom a2a.client import ClientConfig
import httpx
auth_client = httpx.AsyncClient(
headers={"Authorization": "Bearer token123"}
)
agent = A2AAgent(
endpoint="https://protected-agent.example.com",
client_config=ClientConfig(httpx_client=auth_client)
)Advanced Features (Cannot Migrate Yet)If you use these features, continue using a2a_client_factory until v2.0.0: Interceptors# NOT YET SUPPORTED with client_config
factory = ClientFactory(config)
client = factory.create(card, interceptors=[MyOAuthInterceptor()])Consumers# NOT YET SUPPORTED with client_config
factory = ClientFactory(config, consumers=[my_event_consumer])Custom Transports# NOT YET SUPPORTED with client_config
factory.register("grpc", custom_grpc_producer)Track progress: Issue #XXXX - Add interceptor/consumer support to A2AAgent Timeline
|
🔴 ADVERSARIAL TEST REPORTI performed comprehensive adversarial testing on this PR to find bugs, race conditions, edge cases, and security holes. Here are the results: 🐛 BUGS FOUND1.
|
| Category | Pass | Fail | Notes |
|---|---|---|---|
| Race Conditions | 3 | 0 | All detected |
| Memory Leaks | 2 | 0 | Documented |
| Edge Cases | 5 | 0 | All handled |
| Security | 2 | 0 | Timing noted |
| Error Handling | 4 | 1 | Pydantic prevents invalid cards |
| Transient Clients | 2 | 0 | Leak documented |
| TOTAL | 18 | 1 | 95% pass rate |
🎯 RECOMMENDATIONS
Must Fix (Before Merge)
- ✅ Add
asyncio.Locktoget_agent_card()- Fix race condition - ✅ Add
aclose()method - Fix memory leak
Should Fix (Follow-up PR)
- Add timing attack mitigation
- Document transient client lifecycle
Nice to Have
- Add context manager support for auto-cleanup
- Add integration test with real httpx client
📝 ADVERSARIAL TEST SUITE
I've created a comprehensive test suite (test_a2a_agent_adversarial.py, 700+ lines) covering:
- 3 race condition tests
- 2 memory leak tests
- 5 edge case tests
- 2 security tests
- 5 error handling tests
- 2 transient client tests
All tests pass except one Pydantic validation test (expected behavior).
🔥 IMPACT ASSESSMENT
| Issue | Likelihood | Impact | Risk |
|---|---|---|---|
| Race condition | High | Medium | HIGH |
| Memory leak | Medium | High | HIGH |
| Timing attack | Low | Low | LOW |
| Transient leak | High | Low | MEDIUM |
✍️ CONCLUSION
The implementation is functionally correct and the deprecation path works as designed. However, critical concurrency and resource management issues were found that should be fixed before production use.
Recommendation: Fix the race condition (#1) and memory leak (#2) before merging.
🧹 Resource Cleanup: Cached Client LifecycleCurrent Stateclass A2AAgent(AgentBase):
def __init__(self, ...):
self._a2a_client: Client | None = None # Cached, but never cleaned up
async def _get_or_create_client(self) -> Client:
# Factory path: caches client
if self._a2a_client_factory is not None:
if self._a2a_client is not None:
return self._a2a_client
# ...
self._a2a_client = self._a2a_client_factory.create(agent_card)
return self._a2a_client
# client_config path: caches client
if self._client_config is not None:
if self._a2a_client is not None:
return self._a2a_client
self._a2a_client = await ClientFactory.connect(...)
return self._a2a_client
# Transient path: no caching
return await ClientFactory.connect(self.endpoint)ProblemCached clients are never closed, which can lead to:
Analysis by Path
The
Solution: Add Cleanup Methodclass A2AAgent(AgentBase):
"""Client wrapper for remote A2A agents.
Usage:
# Option 1: Manual cleanup
agent = A2AAgent(endpoint=url, client_config=config)
try:
result = await agent.invoke_async("Hello")
finally:
await agent.close() # Clean up cached client
# Option 2: Context manager (recommended)
async with A2AAgent(endpoint=url, client_config=config) as agent:
result = await agent.invoke_async("Hello")
# Client automatically closed
"""
async def close(self) -> None:
"""Close the cached A2A client and release resources.
This method should be called when the agent is no longer needed,
especially when using the client_config parameter which caches
the created client.
When using a2a_client_factory (deprecated), the caller is responsible
for managing the httpx client lifecycle, so this method is a no-op.
Example:
>>> agent = A2AAgent(endpoint=url, client_config=config)
>>> try:
... result = await agent.invoke_async("Hello")
... finally:
... await agent.close()
"""
if self._a2a_client is not None:
# Only close if we created it (client_config path)
# Factory path: user manages lifecycle
if self._a2a_client_factory is None:
# Check if the client has a close method
if hasattr(self._a2a_client, 'close'):
await self._a2a_client.close()
elif hasattr(self._a2a_client, 'aclose'):
await self._a2a_client.aclose()
self._a2a_client = None
logger.debug(
"agent=<%s>, endpoint=<%s> | closed cached A2A client",
self.name,
self.endpoint
)
async def __aenter__(self) -> "A2AAgent":
"""Enter async context manager."""
return self
async def __aexit__(
self,
exc_type: type[BaseException] | None,
exc_val: BaseException | None,
exc_tb: Any,
) -> None:
"""Exit async context manager, cleaning up resources."""
await self.close()Usage ExamplesBefore (Resource Leak)# Cached client is never closed!
agent = A2AAgent(
endpoint="https://agent.example.com",
client_config=ClientConfig(httpx_client=auth_client)
)
for i in range(1000):
result = await agent.invoke_async(f"Message {i}")
# httpx connection stays open foreverAfter: Manual Cleanupagent = A2AAgent(
endpoint="https://agent.example.com",
client_config=ClientConfig(httpx_client=auth_client)
)
try:
for i in range(1000):
result = await agent.invoke_async(f"Message {i}")
finally:
await agent.close() # Clean upAfter: Context Manager (Recommended)async with A2AAgent(
endpoint="https://agent.example.com",
client_config=ClientConfig(httpx_client=auth_client)
) as agent:
for i in range(1000):
result = await agent.invoke_async(f"Message {i}")
# Client automatically closedTest Case@pytest.mark.asyncio
async def test_close_method_cleans_up_cached_client():
"""Test that close() cleans up cached client in client_config path."""
config = ClientConfig()
agent = A2AAgent(endpoint="http://localhost:8000", client_config=config)
# Create mock client with close method
mock_client = AsyncMock()
mock_client.close = AsyncMock()
with patch("strands.agent.a2a_agent.ClientFactory.connect", return_value=mock_client):
# Cache the client
client = await agent._get_or_create_client()
assert agent._a2a_client is mock_client
# Close should clean up
await agent.close()
# Verify cleanup
assert agent._a2a_client is None
mock_client.close.assert_called_once()
@pytest.mark.asyncio
async def test_close_with_factory_does_not_close_client():
"""Test that close() does NOT close client when using factory (user manages it)."""
mock_factory = MagicMock()
mock_factory._config = MagicMock(spec=ClientConfig)
mock_factory._config.httpx_client = None
mock_client = MagicMock()
mock_client.close = MagicMock()
mock_factory.create.return_value = mock_client
with warnings.catch_warnings():
warnings.simplefilter("ignore", DeprecationWarning)
agent = A2AAgent(endpoint="http://localhost:8000", a2a_client_factory=mock_factory)
mock_card = MagicMock(spec=AgentCard)
mock_card.name = "test"
mock_card.description = "test"
agent._agent_card = mock_card
# Cache the client
client = await agent._get_or_create_client()
assert agent._a2a_client is mock_client
# Close should NOT close the client (user owns it)
await agent.close()
# Verify client was cleared but NOT closed
assert agent._a2a_client is None
mock_client.close.assert_not_called() # User manages lifecycle
@pytest.mark.asyncio
async def test_context_manager_cleans_up_automatically():
"""Test that context manager automatically closes client."""
config = ClientConfig()
mock_client = AsyncMock()
mock_client.close = AsyncMock()
with patch("strands.agent.a2a_agent.ClientFactory.connect", return_value=mock_client):
async with A2AAgent(endpoint="http://localhost:8000", client_config=config) as agent:
# Cache the client
client = await agent._get_or_create_client()
assert agent._a2a_client is mock_client
# Context manager should have closed it
assert agent._a2a_client is None
mock_client.close.assert_called_once()Benefits
RecommendationAdd |
✅ Action Items ChecklistBased on the comprehensive code review, here's a prioritized checklist for this PR: 🔴 Must Fix Before Merge (Blocking)
🟡 Should Fix Before Merge (High Priority)
🟢 Nice to Have (Post-Merge)
📊 Test Coverage Additions
📝 Documentation Updates
📦 Suggested Commit OrderIf you want to split this into multiple commits:
This way, the core functionality can be reviewed/merged first, with enhancements following. ✅ When is this ready to merge?Minimum requirements:
Strongly recommended: Let me know if you need clarification on any of these items or want me to help implement any of them! |
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
2 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.
Description
Motivation
A2AAgent.get_agent_card()creates a barehttpx.AsyncClientfor card discovery, ignoring any authentication configured viaa2a_client_factory. This causes 403 errors when the agent card endpoint requires authentication (e.g., AgentCore with IAM SigV4 or OAuth bearer tokens).Since the card resolution path shipped broken, no working authenticated card resolution code exists in the wild. However, the factory's other features (interceptors, consumers, custom transports) do work for
send_messagecalls viafactory.create(agent_card), so removing the factory parameter entirely would break existing integrations.Public API Changes
New
client_config: ClientConfigparameter onA2AAgent.__init__for configuring authentication and transport settings. This is the recommended way to pass an authenticated httpx client for both card resolution and message sending.a2a_client_factoryis deprecated with aDeprecationWarningand will be removed in a future version.When
a2a_client_factoryis still provided (deprecated path),factory.create()is used for client creation to preserve interceptors, consumers, and custom transports. Card resolution usesclient_configif provided, falling back to the factory's internal config.Breaking Changes
No breaking changes.
a2a_client_factorycontinues to work with a deprecation warning.Migration
Design options considered (click to expand)
A2AAgent API Options: Fixing Authenticated Card Resolution
Context
A2AAgent.get_agent_card()was creating a bare httpx client for card discovery, ignoring thea2a_client_factoryparameter. This caused 403 errors on authenticated endpoints. The cardresolution path shipped broken — but the factory's other features (interceptors, consumers,
custom transports) did work for
send_messagecalls since the old code calledfactory.create(agent_card)which properly wired those up.What was broken vs. what worked
get_agent_card()created bare httpx, ignored factoryfactory.create()passed config through to transportfactory.create()accepted interceptorscreate()create()This means users with public card endpoints but factory-configured interceptors/consumers/transports
for message sending have working integrations today. Removing
a2a_client_factorywould break them.What does
ClientFactorycarry beyondClientConfig?ClientConfigClientFactoryClientFactory.connect()paramshttpx_clientfield_configclient_configparam_consumerslistconsumersparamregister()methodextra_transportsparamcreate()timeinterceptorsparamKey insight:
ClientFactory.connect()accepts all of these as separate parameters, so you don'tneed the factory object itself to access any of these features.
Option A: Current implementation — extract config from factory, use
connect()for everythingExtract
_configfrom the factory viagetattr, pass it toClientFactory.connect()for both card resolution and client creation. Cache the resulting client.Pros:
Cons:
_configattribute — breaks if a2a-sdk refactors internalsClientConfig()if_configdisappears (403s return)factory.create()entirely — loses interceptors, consumers, custom transportsBreaks backwards compat: No (parameter unchanged). But silently drops factory features.
Option B: Replace
a2a_client_factorywithclient_config: ClientConfigDrop the factory param. Accept just the config.
Pros:
ClientConfigis the a2a-sdk's public, stable typeCons:
Breaks backwards compat: Yes.
Option C: Add
client_config, deprecatea2a_client_factory, usefactory.create()when present ✅Accept both params. New users use
client_config. Existing factory users keep working with a deprecation warning. When factory is provided, usefactory.create()(public API) for client creation to preserve interceptors/consumers/transports.Pros:
factory.create()(public API) preserves interceptors, consumers, transportsclient_configpath is clean with zero private attr accessclient_confighandles card auth while factory handles client creationCons:
client_config) still falls back togetattrfor card resolutionBreaks backwards compat: No. Deprecates but doesn't remove.
Option D: Keep only
a2a_client_factory, fix card resolution viafactory.create()Keep the factory parameter as-is. Extract config for card resolution (private attr), then use
factory.create(card)for client creation.Pros:
Cons:
getattr(factory, "_config", None)for card resolutionBreaks backwards compat: No.
Option E: Replace
a2a_client_factorywithclient_config+ exposeconnect()kwargsAccept
ClientConfigplus interceptors, consumers, etc. as separate params.Pros:
Cons:
ClientFactory.connect()signatureBreaks backwards compat: Yes.
Comparison Matrix
Related Issues
Type of Change
Bug fix
Testing
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.