Repository navigation
fix(python-sdk): align ReasoningMessageStartEvent.role with TypeScript SDK - #1193
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@ag-ui/a2a-middleware
@ag-ui/a2ui-middleware
@ag-ui/mcp-apps-middleware
@ag-ui/middleware-starter
@ag-ui/a2a
@ag-ui/adk
@ag-ui/ag2
@ag-ui/agno
@ag-ui/aws-strands
@ag-ui/claude-agent-sdk
@ag-ui/crewai
@ag-ui/langchain
@ag-ui/langgraph
@ag-ui/langroid
@ag-ui/llamaindex
@ag-ui/mastra
@ag-ui/pydantic-ai
@ag-ui/server-starter
@ag-ui/server-starter-all-features
@ag-ui/vercel-ai-sdk
create-ag-ui-app
@ag-ui/client
@ag-ui/core
@ag-ui/encoder
@ag-ui/proto
commit: |
11e7bf0 to
f51b2f5
Compare
NathanTarbert
left a comment
NathanTarbert
left a comment
There was a problem hiding this comment.
Code Review — PR #1193
Verdict: Looks good to merge
This is a clean, minimal, and correct fix for the cross-SDK wire-incompatibility described in issue #1169.
Root Cause Confirmation
The issue is a straightforward type literal mismatch. The Python SDK defined ReasoningMessageRole = Literal["assistant"] while:
- The TypeScript SDK uses
z.literal("reasoning")forReasoningMessageStartEventSchema.role - Both SDKs already agree that the
ReasoningMessagetype usesrole: "reasoning"
So the Python event definition was the clear outlier. This PR correctly changes Literal["assistant"] to Literal["reasoning"], bringing the Python SDK in line with the TypeScript SDK and with the Python SDK's own ReasoningMessage type in types.py (line 144).
What's Good
- Minimal and scoped fix — exactly one line changed in production code (
events.pyL284), which is the right scope for this bug. - Comment updated — the old comment
# Text message role for reasoning messages (only assistant can reason)is replaced with a neutral# Text message role for reasoning messages, which is appropriate since the semantic meaning changed. - Tests are well-structured:
test_reasoning_message_start_event_role_is_reasoningcovers creation, serialization (model_dump), and deserialization (viaTypeAdapterwith camelCase JSON, simulating the TypeScript wire format). This is thorough.test_reasoning_message_start_event_rejects_assistant_roleis a good negative/regression test ensuring the old value is now properly rejected.
- PR description is accurate and complete — it clearly states the root cause, the change, and the test plan.
Minor Observations (non-blocking)
-
No default value set:
ReasoningMessageStartEvent.rolehas no default, unlikeTextMessageStartEventwhich defaults to"assistant". SinceLiteral["reasoning"]has only one possible value, settingrole: ReasoningMessageRole = "reasoning"as a default would be a nice ergonomic improvement (users wouldn't need to passrole="reasoning"explicitly every time). However, this matches the current behavior (no default was set before either), so it is not a regression and could be a separate enhancement. -
CI status: The
pythoncheck passes. There are two failing checks (buildanddojo / langgraph-typescript) which appear to be pre-existing/unrelated failures (thebuildfailure is in the TypeScript build pipeline, andlanggraph-typescriptis an integration test — neither should be affected by a one-line Python SDK type literal change).
Conclusion
The fix directly addresses the root cause, is minimal, introduces no regressions, and includes comprehensive tests. Ready to merge.
|
@NathanTarbert AGUI protocol defines What is the source of truth for |
|
@danilagamma the docs were wrong, the role is reasoning |
f51b2f5 to
7a5ad14
Compare
…nt" to "reasoning"
The Python SDK defined ReasoningMessageRole as Literal["assistant"] while
the TypeScript SDK used z.literal("reasoning"), making the two SDKs
wire-incompatible. A Python backend emitting REASONING_MESSAGE_START with
role "assistant" would be rejected by the TypeScript client's Zod parser.
Both SDKs already agree that ReasoningMessage.role should be "reasoning"
(types.py L144, types.ts L104), so the Python event definition was the
one out of sync.
Fixes #1169
Python Preview PackagesVersion
Install with uvAdd the TestPyPI index to your [[tool.uv.index]]
name = "testpypi"
url = "https://test.pypi.org/simple/"
explicit = trueThen install the packages you need: # Core SDK
uv add 'ag-ui-protocol==0.0.0.dev1775127364' --index testpypi
# Integrations (each already depends on the matching ag-ui-protocol preview)
uv add 'ag-ui-langgraph==0.0.0.dev1775127364' --index testpypi
uv add 'ag-ui-crewai==0.0.0.dev1775127364' --index testpypi
# NOTE: ag-ui-agent-spec depends on pyagentspec (git-only, not on PyPI).
# You will need to install pyagentspec separately from its git repo.
uv add 'ag-ui-agent-spec==0.0.0.dev1775127364' --index testpypi
uv add 'ag_ui_adk==0.0.0.dev1775127364' --index testpypi
uv add 'ag_ui_strands==0.0.0.dev1775127364' --index testpypiInstall with pippip install \
--index-url https://test.pypi.org/simple/ \
--extra-index-url https://pypi.org/simple/ \
ag-ui-protocol==0.0.0.dev1775127364
Commit: 2ed2416 |
Summary
Fixes #1169
ReasoningMessageRolefromLiteral["assistant"]toLiteral["reasoning"]in the Python SDK'sevents.pyz.literal("reasoning")for the same fieldrole: "reasoning"in theirReasoningMessagetype definitions — only the Python event definition was out of syncRoot Cause
The Python SDK's
ReasoningMessageStartEventdefinedroleasLiteral["assistant"](line 284 ofevents.py), while the TypeScript SDK defined it asz.literal("reasoning")(line 255 ofevents.ts). This made the two SDKs wire-incompatible: a Python backend emittingREASONING_MESSAGE_STARTwithrole="assistant"would be rejected by the TypeScript client's Zod parser, which expects"reasoning".Changes
sdks/python/ag_ui/core/events.py(line 284): ChangedReasoningMessageRole = Literal["assistant"]toReasoningMessageRole = Literal["reasoning"]sdks/python/tests/test_events.py: Added two regression tests:test_reasoning_message_start_event_role_is_reasoning— verifies creation, serialization, and deserialization withrole="reasoning"test_reasoning_message_start_event_rejects_assistant_role— verifies thatrole="assistant"is now rejected with aValidationErrorTest plan
role="reasoning"rejected byLiteral["assistant"])