fix(claude-agent-sdk): preserve multimodal input - #2609
Conversation
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.dev1788550060' --index testpypi
# Integrations (each already depends on the matching ag-ui-protocol preview)
uv add 'ag-ui-langgraph==0.0.0.dev1788550060' --index testpypi
uv add 'ag-ui-crewai==0.0.0.dev1788550060' --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.dev1788550060' --index testpypi
uv add 'ag_ui_adk==0.0.0.dev1788550060' --index testpypi
uv add 'ag_ui_strands==0.0.0.dev1788550060' --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.dev1788550060
Commit: 99bc2b5 |
@ag-ui/a2a-middleware
@ag-ui/a2ui-middleware
@ag-ui/event-throttle-middleware
@ag-ui/mcp-apps-middleware
@ag-ui/mcp-middleware
@ag-ui/a2a
@ag-ui/adk
@ag-ui/ag2
@ag-ui/agno
@ag-ui/aws-strands
@ag-ui/claude-agent-sdk
@ag-ui/claude-managed-agents
@ag-ui/crewai
@ag-ui/langchain
@ag-ui/langgraph
@ag-ui/llamaindex
@ag-ui/mastra
@ag-ui/pydantic-ai
@ag-ui/vercel-ai-sdk
@ag-ui/watsonx
@ag-ui/a2ui-toolkit
create-ag-ui-app
@ag-ui/client
@ag-ui/core
@ag-ui/encoder
@ag-ui/proto
commit: |
mxmzb
left a comment
mxmzb
left a comment
There was a problem hiding this comment.
Reviewed both runtimes. The core change is right — attachments now survive into the SDK, ordering is preserved, and the block shapes match what the Claude Agent SDK expects on each side (Python's client.query() accepts str | AsyncIterable[dict], TS query({prompt}) accepts string | AsyncIterable<SDKUserMessage>). I re-ran the stated validation locally and it all passes: 117 Python tests, 14 TS tests, typecheck clean.
Two blocking issues, both about what happens when conversion fails rather than when it succeeds. Verified empirically, not read off the diff.
1. Python: one unsupported attachment tears down the thread's live session
process_messages now raises, and it is called at adapter.py:348 inside run()'s main try. The generic except Exception at adapter.py:440 pops and stops the SessionWorker — so a purely client-side validation failure that never reached the SDK destroys the Claude CLI session for that thread along with its conversation history. The user's next message starts a fresh session with no memory of the conversation.
Probe: prime a thread with a plain text run, then send [text, audio] on the same thread_id:
run1 events: [RUN_STARTED, RUN_FINISHED] workers: ['t1']
run2 events: [RUN_ERROR 'content[1] type audio is not supported']
workers after run2: [] worker stopped? [True]
Suggested fix: convert the content before the worker try-block, or catch the conversion error separately, emit RUN_ERROR, and leave the worker untouched. Nothing about a bad content block implies the session is broken.
2. TypeScript: the throw escapes before any AG-UI event is emitted
processMessages(runInput) is called at adapter.ts:156, in the Observable subscribe body — outside translateStream's try/catch. RxJS routes the synchronous throw to subscriber.error, so the consumer sees a bare error notification and zero events — no RUN_STARTED, no RUN_ERROR:
EVENTS: [] ERROR: [ClaudeAdapter] content[1] type audio is not supported COMPLETED: false
Python at least emits a RUN_ERROR on this path, so the two runtimes disagree on the failure contract. Suggested fix: move the processMessages call inside translateStream's try, after RUN_STARTED, so the existing catch produces a proper RUN_ERROR.
3. Worth a second look: is hard-fail the right policy at all?
The test names make it clear this is deliberate (test_rejects_unsupported_media_instead_of_dropping_it), and I agree silently dropping an image is the bug FAC-144 is about. But audio and video are protocol-legal content that Claude simply cannot accept, and the same is true of an opaque binary file id. Under this change, a user who attaches a voice memo alongside a text question gets no answer at all, where before they got an answer to the text.
Consider: convert what is convertible, replace what is not with a text marker ([unsupported attachment: audio/mp4]), and fail the run only when nothing survives. That keeps the loud signal (the model and the user both see the attachment was not read) without discarding the rest of the turn.
4. Empty text blocks now reach the API
[{"type": "text", "text": ""}] converts to {"type": "text", "text": ""} and is sent. The Messages API rejects empty text content blocks, and because has_user_content / hasUserContent is set from blocks.length > 0 rather than from the content itself, the "No user message found" warning no longer fires either. The old if not user_message: guard covered this incidentally.
Verified on the Python side:
process_messages(... content=[{"type": "text", "text": ""}])
-> {'type': 'user', 'message': {'role': 'user', 'content': [{'type': 'text', 'text': ''}]}, ...}
Suggest skipping empty / whitespace-only text blocks, or rejecting them with the same explicit error the other validators use.
5. The Python tests added here never run in CI
unit-python-sdk.yml:15 states it directly: "agent-spec and claude-agent-sdk have no test lane" — only the committed lockfile is verified for this package. So the regression coverage in tests/test_utils.py, tests/test_adapter.py and tests/test_concurrency_integration.py will not execute on any future PR, and the FAC-144 regression can return green.
The TypeScript side is fine: nx run-many -t test at the repo root picks up @ag-ui/claude-agent-sdk:test, and I confirmed the new suite runs under it.
Adding a Python lane is arguably out of scope for this PR, but since the PR's value is largely the regression coverage, it seems worth either adding the lane or filing a follow-up.
Smaller notes
session_idon the structured message. TS setssession_id: input.threadId ?? "default", but the SDK's own string-prompt path normalizes tosession_id: "". Python's string path already passed the thread id through, so Python stays self-consistent — TS is the side that changed behaviour. Probably harmless, but worth one real CLI run to confirm the daemon ignores it, or just use""to match the SDK.- Diff noise.
typescript/src/utils.tscarries roughly 150 lines of unrelated prettier reformatting (trailing commas, argument wrapping) mixed in with the real change, which makes the multimodal logic harder to review. Splitting the reformat into its own commit would help. - Untested failure path. Neither runtime has a test that drives
run()with unconvertible content and asserts on the emitted events. That gap is exactly what hid items 1 and 2 — the existing tests callprocess_messages/processMessagesdirectly and assert only that it throws.
Checked and fine
ag-ui-protocol>=0.1.15genuinely exports every symbol the new Python import block pulls in (verified against the published 0.1.15 wheel), so the floor pin is not a trap.requires-python = ">=3.11"covers the eagerly-evaluatedstr | AsyncIterable[...]annotations and the module-levelClaudePromptalias.- Raw dict content blocks are not a regression risk:
UserMessage.contentis a discriminated union, so pydantic materializes the typed models before_convert_content_blocksees them. - TS streaming-input mode still terminates.
isSingleUserTurnbecomesfalsewith an iterable prompt, so the SDK skips its post-resultendInput()— butstreamInputcallsendInput()itself once the iterable is exhausted (waiting for the first result when bidirectional needs exist, which they always do here because of the SDK MCP server). No hung process. split(";", 1)[0]behaves correctly for media-type normalization in both languages, despite the differinglimit/maxsplitsemantics.
mxmzb
left a comment
mxmzb
left a comment
There was a problem hiding this comment.
Marking this as requesting changes to make the status explicit — details are in my review above.
Blocking on the two verified failure-path issues:
- Python — a single unsupported attachment tears down the thread's
SessionWorker, destroying the Claude CLI session and its conversation history for a validation failure that never reached the SDK (adapter.py:348raising into theexcept Exceptionatadapter.py:440). - TypeScript —
processMessagesis called outsidetranslateStream's try/catch (adapter.ts:156), so the throw surfaces as a bare RxJS error with zero AG-UI events emitted — noRUN_STARTED, noRUN_ERROR. Python emitsRUN_ERRORon the same path, so the runtimes disagree on the failure contract.
Also worth resolving before merge, though I'd defer to you on the first one:
- Whether hard-failing the whole run on protocol-legal-but-unconvertible content (audio, video, opaque
binaryids) is the behaviour we want, versus converting what we can and marking the rest. - Empty text blocks now reach the API, which rejects them, and the "No user message found" warning no longer fires for that case.
The conversion logic itself looks correct on both sides and the stated validation reproduces cleanly here — this is entirely about the error paths.
|
Addressed the review feedback in
Validation rerun:
The PR description now also includes |
2 participants
Summary
Linear
Validation
pnpm nx run @ag-ui/claude-agent-sdk:testpnpm nx run @ag-ui/claude-agent-sdk:typecheckuv run python -m pytest -q(fromintegrations/claude-agent-sdk/python)git diff --check origin/main...HEADFixes FAC-144
Fixes FAC-171