Skip to content

Python: Fix core observability unsafe serialization of function-call arguments containing dataclass/framework objects - #6026

Merged
Evan Mattson (moonbox3) merged 5 commits into
microsoft:mainfrom
moonbox3:agent/fix-5733-1
Jun 1, 2026
Merged

Python: Fix core observability unsafe serialization of function-call arguments containing dataclass/framework objects#6026
Evan Mattson (moonbox3) merged 5 commits into
microsoft:mainfrom
moonbox3:agent/fix-5733-1

Conversation

@moonbox3

Copy link
Copy Markdown
Contributor

Motivation and Context

Core observability in agent_framework/observability.py passed raw function_call arguments directly into json.dumps, which raises TypeError when those arguments contain dataclass or framework objects (e.g. HandoffAgentUserRequest) emitted by workflow request_info / handoff flows. This affected any Foundry-hosted or handoff scenario with sensitive telemetry enabled.

Fixes#5733

Description

The root cause was that _to_otel_part's function_call branch kept content.arguments as-is, with no safe-serialization step, mirroring the already-fixed AG-UI path. The fix lifts a make_json_safe() helper into agent_framework/_serialization.py — which recursively converts dataclasses, Pydantic models, datetimes, and __dict__-bearing objects to JSON-serializable primitives — and calls it on content.arguments before the value is included in the OTel span attribute. Tests covering dataclass payloads and nested non-primitive objects in function_call arguments were added to test_observability.py to prevent regression.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • The PR follows the Contribution Guidelines
  • All unit tests pass, and I have added new tests where possible
  • Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

Note: PR autogenerated by moonbox3's agent

Copilotand others added 2 commits May 22, 2026 05:49
Apply make_json_safe() to content.arguments in _to_otel_part() before
building the otel message dict, so that dataclass/framework payloads
(e.g. workflow request_info events) do not cause a TypeError when
_capture_messages() calls json.dumps().
Lift make_json_safe() into agent_framework._serialization (no new
external deps — dataclasses/datetime only) so the core observability
path can use it without a dependency on the ag-ui adapter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…bility (microsoft#5733)
- Add make_json_safe() helper to recursively convert non-serializable objects
- Use make_json_safe() in _to_otel_part() for function_call arguments
- Fix CustomPayload test class to use @DataClass (resolves B903 lint error)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings May 22, 2026 07:59
@moonbox3Evan Mattson (moonbox3) added the python Usage: [Issues, PRs], Target: Python label May 22, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a runtime failure in Python core observability where OpenTelemetry span attributes could not be serialized when function_call arguments contained dataclasses or framework objects (common in workflow request_info / handoff flows), by introducing a safe-serialization step and adding regression tests.

Changes:

  • Add make_json_safe() helper in core serialization utilities and apply it to function_call arguments in _to_otel_part.
  • Add tests ensuring dataclass and nested-object tool arguments are serializable and _capture_messages doesn’t raise.
  • Minor formatting/import cleanups in Foundry hosting and A2A tests, plus a lockfile metadata normalization.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
FileDescription
python/packages/core/agent_framework/observability.pySerializes function_call arguments via make_json_safe before emitting OTel message parts.
python/packages/core/agent_framework/_serialization.pyIntroduces make_json_safe() recursive conversion helper for JSON-serializable telemetry payloads.
python/packages/core/tests/core/test_observability.pyAdds regression tests for dataclass/nested objects in tool-call arguments and span capture.
python/packages/foundry_hosting/agent_framework_foundry_hosting/_responses.pyConsolidates imports (minor cleanup).
python/packages/foundry_hosting/tests/test_responses.pyAdds spacing to satisfy formatting/lint expectations around region boundaries.
python/packages/a2a/tests/test_a2a_agent.pyFormats a test function signature to a single line.
python/uv.lockNormalizes dependency specifier ordering for github-copilot-sdk.

Comment threadpython/packages/core/agent_framework/_serialization.py Outdated
Comment threadpython/packages/core/agent_framework/_serialization.py Outdated
@moonbox3

Evan Mattson (moonbox3) commented May 22, 2026

Copy link
Copy Markdown
ContributorAuthor

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
packages/core/agent_framework
_serialization.py1501590%517, 534, 544, 612, 637, 646–649, 651–654, 658, 661
packages/core/agent_framework/_workflows
_agent.py3636482%68, 76–82, 118–119, 212, 273, 286, 353, 364, 366, 426, 432, 449–450, 457, 459, 465, 531–532, 541, 580, 615, 690, 720, 737, 765, 776–779, 785, 791, 795–796, 799–805, 809–810, 818, 879, 886, 892–893, 904, 936, 943, 964, 973, 977, 979–981, 988
_functional.py4913193%398–399, 465, 488, 497–499, 502, 532, 713–714, 721, 969, 1001, 1004–1005, 1013–1015, 1090–1091, 1097, 1480–1481, 1483, 1490–1493, 1514, 1539
TOTAL37429435488%

Python Unit Test Overview

TestsSkippedFailuresErrorsTime
745834 💤0 ❌0 🔥1m 58s ⏱️

@moonbox3Evan Mattson (moonbox3) left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Code Review

Reviewers: 2 | Confidence: 92% | Result: All clear

Reviewed: Security Reliability, Design Approach


Automated review by moonbox3's agents

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Code Review

Reviewers: 4 | Confidence: 83% | Result: All clear

Reviewed: Correctness, Security Reliability, Test Coverage, Design Approach


Automated review by moonbox3's agents

…_json_safe (microsoft#5733)
- Use callable(getattr(obj, method, None)) instead of hasattr() so that
non-callable attributes named model_dump/to_dict/dict do not raise
TypeError at runtime.
- Wrap each call in try/except TypeError to handle callables with
mandatory arguments gracefully.
- Convert dict keys to str() so that non-string keys (e.g. datetime,
int) cannot cause json.dumps to raise TypeError.
- Add regression tests for both scenarios.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@moonbox3Evan Mattson (moonbox3) left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Code Review

Reviewers: 4 | Confidence: 86%

✓ Correctness

The PR correctly fixes two bugs: (1) hasattr checks replaced with callable(getattr(..., None)) to skip non-callable attributes named model_dump/to_dict/dict, and (2) dict keys are now stringified so json.dumps never raises TypeError on non-string keys. The try/except TypeError wrappers provide a safe fallback chain. The __dict__ branch retains bare key references but vars(obj) always yields string keys, so no issue there. No correctness problems found.

✓ Security Reliability

The PR correctly fixes two bugs: (1) using callable(getattr(...)) to guard against non-callable attributes named model_dump/to_dict/dict, and (2) converting dict keys to strings so json.dumps never raises TypeError on non-string keys. Both fixes are well-motivated and tested. One reliability concern: the str(key) conversion at line 656 can silently clobber data when a dict contains both an integer key and its string equivalent (e.g., {42: 'a', '42': 'b'}), since iteration order determines which value survives with no warning. This is a silent data-loss scenario not covered by the new tests. The TypeError suppression around serialization method calls is reasonable in scope since TypeError on a zero-argument call is a narrow signal, but it could mask bugs in model_dump/to_dict implementations that raise TypeError internally — the fallback to __dict__ may then silently serialize incorrect/partial state.

✓ Test Coverage

The fix correctly guards against non-callable model_dump/to_dict/dict attributes and converts dict keys to strings. Two test coverage gaps: (1) the assertion in test_make_json_safe_non_callable_method_attribute is trivially true (json.dumps never returns None), masking whether the correct fallback value is returned; (2) the new try/except TypeError path — where a callable model_dump() raises TypeError and falls through — is never exercised by a test.

✓ Design Approach

I did not find a design-approach issue in this bounded pass. The change stays focused on making observability serialization tolerate the two concrete edge cases covered by the diff: non-callable serializer-like attributes and non-string dict keys that would otherwise break JSON encoding.


Automated review by moonbox3's agents

Comment threadpython/packages/core/agent_framework/_serialization.py
Comment threadpython/packages/core/tests/core/test_observability.py Outdated
…icts
# Conflicts:
#	python/packages/foundry_hosting/agent_framework_foundry_hosting/_responses.py
Comment threadpython/packages/core/agent_framework/observability.py Outdated
@moonbox3
Evan Mattson (moonbox3) added this pull request to the merge queueJun 1, 2026
Merged via the queue into microsoft:main with commit f36096cJun 1, 2026
36 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pythonUsage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: Core observability cannot safely serialize workflow request_info payloads

4 participants

@moonbox3@eavanvalkenburg@giles17