Skip to content

fix(sdk/python): coerce complex pydantic type hints in reasoner args - #1035

Open
7vignesh wants to merge 4 commits into
Agent-Field:mainfrom
7vignesh:fix/1034-pydantic-nested-union-coercion
Open

fix(sdk/python): coerce complex pydantic type hints in reasoner args#1035
7vignesh wants to merge 4 commits into
Agent-Field:mainfrom
7vignesh:fix/1034-pydantic-nested-union-coercion

Conversation

@7vignesh

@7vignesh 7vignesh commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #1034. Reasoner/skill argument coercion in the Python SDK only handled a bare Pydantic model or a 2-arg Optional[model]. Complex type hints fell through and the caller received raw dicts instead of model instances.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Reproduction

Before this change, these all left the value as a raw dict / list of dicts:

def r(item: M1 | M2 | None = None): ...              # -> dict, not M1
def r(items: list[M1] | None = None): ...            # -> [dict, dict]
def r(items: list[M1] | list[M2] | None = None): ... # -> [dict]
def r(seq: Sequence[M1 | None] = ()): ...            # -> [dict, None]

Root cause

pydantic_utils.py detected a model only via is_pydantic_model(actual_type) after unwrapping a strict 2-arg Optional. A 3+ arg union, or a container whose element is a model, never matched, so convert_dict_to_model was never called. Separately, convert_function_args wrapped Pydantic v2 ValidationError with keyword args its v2 constructor rejects, so the except Exception fallback swallowed real validation failures and returned the raw dict.

Fix

  • type_hint_involves_model() recurses through Union / list / Sequence / tuple / dict args to detect a model anywhere in a hint.
  • _convert_with_type_hint() validates via pydantic.TypeAdapter(hint).validate_python(value), which resolves any nested / union / optional / container shape losslessly. Hints with no model are returned untouched, so plain int / str / dict params still pass through unchanged.
  • convert_function_args now propagates validation errors (wrapped with the parameter name) instead of hiding them; non-validation errors (e.g. unresolved forward refs) still fall back to the original args.
  • convert_dict_to_model uses model_class.model_validate (the correct v2 API).
  • should_convert_args uses the same recursive detection so conversion triggers for the new shapes.

Test plan

  • cd sdk/python && python -m pytest tests/test_pydantic_utils.py - 15 passed
  • python -m pytest tests/test_skill_pydantic_models.py tests/test_decorators.py - pass
  • Full unit suite: 2316 passed (remaining failures are litellm-not-installed and live-server environmental issues, pre-existing)
  • python -m ruff check and ruff format --check on the changed files - clean

New tests cover: multi-arg union of models, list[model], union of list[model], Sequence[model | None], nested-model roundtrip, container validation-error propagation, and non-model pass-through for complex hints.

Checklist

Related issues / PRs

Fixes #1034

…gent-Field#1034)

Argument coercion only handled a bare model or a 2-arg Optional[model].
Complex hints fell through as raw dicts:
- unions of 3+ (M1 | M2 | None)
- containers of models (list[M], Sequence[M | None])
- unions of container types (list[M1] | list[M2] | None)

It also silently swallowed validation errors due to a Pydantic v2
ValidationError constructor mismatch, returning the raw dict instead of
surfacing the failure.

Fix:
- type_hint_involves_model(): recurses through Union/list/Sequence/tuple/
  dict args to detect a model anywhere in the hint.
- _convert_with_type_hint(): validates via pydantic TypeAdapter, which
  handles any nested/union/optional/container shape losslessly; hints with
  no model are returned untouched (preserves plain int/str/dict pass-through).
- convert_function_args now propagates validation errors (wrapped with the
  parameter name) instead of hiding them; non-validation errors still fall
  back to original args for backward compatibility.
- convert_dict_to_model uses model_class.model_validate (correct v2 API).
- should_convert_args uses the same recursive detection so conversion
  triggers for the new shapes.

Adds tests for each reported case plus non-model pass-through and
validation-error propagation.
@7vignesh
7vignesh requested review from a team and AbirAbbas as code owners September 1, 2026 21:28
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Performance

SDK Memory Δ Latency Δ Tests Status
Python 9.0 KB - 0.31 µs -11%

✓ No regressions detected

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

📊 Coverage gate

Thresholds from .coverage-gate.toml: per-surface ≥ 84%, aggregate ≥ 85%, max per-surface regression ≤ 1.0 pp, max aggregate regression ≤ 0.50 pp.

Surface Current Baseline Δ
control-plane 87.80% 87.40% ↑ +0.40 pp 🟡
sdk-go 93.10% 92.00% ↑ +1.10 pp 🟢
sdk-python 94.72% 93.73% ↑ +0.99 pp 🟢
sdk-typescript 91.72% 90.42% ↑ +1.30 pp 🟢
web-ui 84.76% 84.79% ↓ -0.03 pp 🟡
aggregate 85.88% 85.75% ↑ +0.13 pp 🟡

✅ Gate passed

No surface regressed past the allowed threshold and the aggregate stayed above the floor.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

📐 Patch coverage gate

Threshold: 80% on lines this PR touches vs origin/main (from .coverage-gate.toml:thresholds.min_patch).

Surface Touched lines Patch coverage Status
control-plane 0 ➖ no changes
sdk-go 0 ➖ no changes
sdk-python 0 ➖ no changes
sdk-typescript 0 ➖ no changes
web-ui 0 ➖ no changes

✅ Patch gate passed

Every surface whose lines were touched by this PR has patch coverage at or above the threshold.

@santoshkumarradha santoshkumarradha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for chasing the nested-type coercion gaps here. I found one behavior regression that needs to be fixed before this is safe to merge: invalid model payloads now get rethrown as ValueError, but the current SDK call sites still only intercept pydantic.ValidationError when they convert reasoner and skill inputs. That means a bad payload can now miss the existing safe-validation path and either fall back to the raw dict or surface through the wrong error path. If you preserve ValidationError here, or update the callers in the same PR, this looks close.

Comment thread sdk/python/agentfield/pydantic_utils.py Outdated
except ValidationError as e:
# Add parameter context to the error, preserving the original
# ValidationError as the cause.
raise ValueError(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This changes the surfaced exception type from ValidationError to ValueError, but the existing callers in agent.py and decorators.py only catch ValidationError around convert_function_args(). With this change, invalid model payloads can skip the current safe-input handling path. Can we preserve ValidationError here, or update those callers in the same PR?

7vignesh and others added 3 commits September 2, 2026 19:26
…eld#1034 review)

Addresses review feedback on PR Agent-Field#1035. The rewrite wrapped validation
failures as ValueError, but the reasoner/skill call sites in agent.py and
decorators.py intercept pydantic.ValidationError specifically to route bad
payloads through their safe-validation path (_HandlerInputError, avoiding
stack-trace exposure in 422s). Wrapping as ValueError let a bad payload miss
that handler and fall back to the raw dict.

Let the ValidationError from TypeAdapter propagate unchanged so the existing
callers keep intercepting it. Tests now assert ValidationError explicitly.
@7vignesh

7vignesh commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Good catch - fixed in 656bbd9.

You're right: wrapping the failure as ValueError bypassed the except ValidationError handlers in agent.py (reasoner + skill) and decorators.py that convert a bad payload into a safe _HandlerInputError, so it would have fallen through to the raw-dict fallback.

I went with option 1 (preserve ValidationError): convert_function_args now lets the ValidationError raised by TypeAdapter.validate_python propagate unchanged instead of re-wrapping it, and the top-level except re-raises ValidationError rather than the previous (ValidationError, ValueError). No caller changes needed - all three call sites keep intercepting it exactly as before.

Verified end to end:

def f(m: MyModel): ...
convert_function_args(f, (), {"m": {"x": "bad"}})   # -> raises pydantic.ValidationError

def g(items: list[MyModel]): ...
convert_function_args(g, (), {"items": [{"x": "bad"}]})  # -> raises pydantic.ValidationError

Both the direct-model and nested-container error paths now surface ValidationError. Updated the two tests to assert pytest.raises(ValidationError) explicitly. test_pydantic_utils.py, test_skill_pydantic_models.py, and test_decorators.py all pass; ruff check/format clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Python SDK] Trouble with cross agent pydantic (un)marshalling

2 participants