Skip to content

Commit 7632f55

Browse files
fix(core): preserve legitimate falsy values in _clean_empty (a2aproject#713)
# Description Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [x] Follow the [`CONTRIBUTING` Guide](https://github.com/a2aproject/a2a-python/blob/main/CONTRIBUTING.md). - [x] Make your Pull Request title in the <https://www.conventionalcommits.org/> specification. - Important Prefixes for [release-please](https://github.com/googleapis/release-please): - `fix:` which represents bug fixes, and correlates to a [SemVer](https://semver.org/) patch. - `feat:` represents a new feature, and correlates to a SemVer minor. - `feat!:`, or `fix!:`, `refactor!:`, etc., which represent a breaking change (indicated by the `!`) and will result in a SemVer major. - [x] Ensure the tests and linter pass (Run `bash scripts/format.sh` from the repository root to format) - [x] Appropriate docs were updated (if necessary) Fixes a2aproject#692 🦕 ## Problem `_clean_empty` uses a truthiness check (`if v`) that drops legitimate falsy values like `0`, `False`, and `0.0` alongside the intended empties (`''`, `[]`, `{}`). This affects `canonicalize_agent_card()` — the only caller — which produces canonical JSON for signature verification (RFC 8785). An `AgentCard` with e.g. `AgentCapabilities(streaming=False)` would have the field silently stripped, losing the distinction between "explicitly disabled" and "unspecified". ## Fix Restructured `_clean_empty` so that only empty strings are converted to `None` at the leaf level, and empty containers collapse naturally via `or None` after their children are cleaned. The filter becomes `if v is not None`, which only catches values the function itself converted — `0`, `False`, and `0.0` never go through any conversion and flow through unchanged. ### Alternatives considered If maintainers prefer a different style, happy to switch to either: - Inline sentinel: `if v not in (None, '', [], {})` directly in the comprehensions, keeping the original structure with a fixed filter. - Helper predicate: extract an `_is_empty(v)` function and use `if not _is_empty(v)` as the filter in both dict and list comprehensions. ## Tests Parametrized tests covering empty removal, falsy value preservation, mixed cases, non-mutation, and a regression test confirming `streaming=False` survives in `canonicalize_agent_card` output. Happy to trim test cases if seen as excessive. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 parent 9968f9c commit 7632f55

2 files changed

Lines changed: 111 additions & 9 deletions

File tree

src/a2a/utils/helpers.py

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -350,14 +350,20 @@ def are_modalities_compatible(
350350
def _clean_empty(d: Any) -> Any:
351351
"""Recursively remove empty strings, lists and dicts from a dictionary."""
352352
if isinstance(d, dict):
353-
cleaned_dict: dict[Any, Any] = {
354-
k: _clean_empty(v) for k, v in d.items()
353+
cleaned_dict = {
354+
k: cleaned_v
355+
for k, v in d.items()
356+
if (cleaned_v := _clean_empty(v)) is not None
355357
}
356-
return {k: v for k, v in cleaned_dict.items() if v}
358+
return cleaned_dict or None
357359
if isinstance(d, list):
358-
cleaned_list: list[Any] = [_clean_empty(v) for v in d]
359-
return [v for v in cleaned_list if v]
360-
return d if d not in ['', [], {}] else None
360+
cleaned_list = [
361+
cleaned_v for v in d if (cleaned_v := _clean_empty(v)) is not None
362+
]
363+
return cleaned_list or None
364+
if isinstance(d, str) and not d:
365+
return None
366+
return d
361367

362368

363369
def canonicalize_agent_card(agent_card: AgentCard) -> str:

tests/utils/test_helpers.py

Lines changed: 99 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@
66
import pytest
77

88
from a2a.types import (
9-
Artifact,
9+
AgentCapabilities,
1010
AgentCard,
1111
AgentCardSignature,
12-
AgentCapabilities,
1312
AgentSkill,
13+
Artifact,
1414
Message,
1515
MessageSendParams,
1616
Part,
@@ -22,12 +22,13 @@
2222
)
2323
from a2a.utils.errors import ServerError
2424
from a2a.utils.helpers import (
25+
_clean_empty,
2526
append_artifact_to_task,
2627
are_modalities_compatible,
2728
build_text_artifact,
29+
canonicalize_agent_card,
2830
create_task_obj,
2931
validate,
30-
canonicalize_agent_card,
3132
)
3233

3334

@@ -380,3 +381,98 @@ def test_canonicalize_agent_card():
380381
)
381382
result = canonicalize_agent_card(agent_card)
382383
assert result == expected_jcs
384+
385+
386+
def test_canonicalize_agent_card_preserves_false_capability():
387+
"""Regression #692: streaming=False must not be stripped from canonical JSON."""
388+
card = AgentCard(
389+
**{
390+
**SAMPLE_AGENT_CARD,
391+
'capabilities': AgentCapabilities(
392+
streaming=False,
393+
push_notifications=True,
394+
),
395+
}
396+
)
397+
result = canonicalize_agent_card(card)
398+
assert '"streaming":false' in result
399+
400+
401+
@pytest.mark.parametrize(
402+
'input_val',
403+
[
404+
pytest.param({'a': ''}, id='empty-string'),
405+
pytest.param({'a': []}, id='empty-list'),
406+
pytest.param({'a': {}}, id='empty-dict'),
407+
pytest.param({'a': {'b': []}}, id='nested-empty'),
408+
pytest.param({'a': '', 'b': [], 'c': {}}, id='all-empties'),
409+
pytest.param({'a': {'b': {'c': ''}}}, id='deeply-nested'),
410+
],
411+
)
412+
def test_clean_empty_removes_empties(input_val):
413+
"""_clean_empty removes empty strings, lists, and dicts recursively."""
414+
assert _clean_empty(input_val) is None
415+
416+
417+
def test_clean_empty_top_level_list_becomes_none():
418+
"""Top-level list that becomes empty after cleaning should return None."""
419+
assert _clean_empty(['', {}, []]) is None
420+
421+
422+
@pytest.mark.parametrize(
423+
'input_val,expected',
424+
[
425+
pytest.param({'retries': 0}, {'retries': 0}, id='int-zero'),
426+
pytest.param({'enabled': False}, {'enabled': False}, id='bool-false'),
427+
pytest.param({'score': 0.0}, {'score': 0.0}, id='float-zero'),
428+
pytest.param([0, 1, 2], [0, 1, 2], id='zero-in-list'),
429+
pytest.param([False, True], [False, True], id='false-in-list'),
430+
pytest.param(
431+
{'config': {'max_retries': 0, 'name': 'agent'}},
432+
{'config': {'max_retries': 0, 'name': 'agent'}},
433+
id='nested-zero',
434+
),
435+
],
436+
)
437+
def test_clean_empty_preserves_falsy_values(input_val, expected):
438+
"""_clean_empty preserves legitimate falsy values (0, False, 0.0)."""
439+
assert _clean_empty(input_val) == expected
440+
441+
442+
@pytest.mark.parametrize(
443+
'input_val,expected',
444+
[
445+
pytest.param(
446+
{'count': 0, 'label': '', 'items': []},
447+
{'count': 0},
448+
id='falsy-with-empties',
449+
),
450+
pytest.param(
451+
{'a': 0, 'b': 'hello', 'c': False, 'd': ''},
452+
{'a': 0, 'b': 'hello', 'c': False},
453+
id='mixed-types',
454+
),
455+
pytest.param(
456+
{'name': 'agent', 'retries': 0, 'tags': [], 'desc': ''},
457+
{'name': 'agent', 'retries': 0},
458+
id='realistic-mixed',
459+
),
460+
],
461+
)
462+
def test_clean_empty_mixed(input_val, expected):
463+
"""_clean_empty handles mixed empty and falsy values correctly."""
464+
assert _clean_empty(input_val) == expected
465+
466+
467+
def test_clean_empty_does_not_mutate_input():
468+
"""_clean_empty should not mutate the original input object."""
469+
original = {'a': '', 'b': 1, 'c': {'d': ''}}
470+
original_copy = {
471+
'a': '',
472+
'b': 1,
473+
'c': {'d': ''},
474+
}
475+
476+
_clean_empty(original)
477+
478+
assert original == original_copy

0 commit comments

Comments
 (0)