Repository navigation
fix(mindie): degrade unserializable tool args to str() in XML replay - #6839
Conversation
The XML parser's literal_eval fallback puts values json.dumps rejects (e.g. Python sets from set-literal arguments) into AIMessage tool_calls args. Replaying that message from history hit the unguarded json.dumps in _fix_messages' args join, raising TypeError and failing the whole request — and every later turn, since the poisoned message rides the history. Mirror the tool-result json block handling already in the same function: degrade the value to str() so one unserializable argument cannot fail the request.
github-actions
Bot
added
area:backend
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at 4cee6f1: clean, minimal fix. _render_tool_arg is byte-identical to the old inline expression for every value that serialized before, and its TypeError/ValueError fallback mirrors the existing ToolMessage json-block branch (lines 85-88) exactly. I checked the only other json.dumps site in the file (line 86) is already guarded, so no history-replay path is left unguarded, and the set -> {1, 2, 3} -> literal_eval round trip is stable across turns rather than growing. Both new tests pin real behavior, and the parser-to-serializer chain test is the right shape for the regression. One pre-existing, adjacent suggestion about _safe_literal not recursing into sets is posted inline; fine to defer to a follow-up since this PR does not introduce it.
| def _render_tool_arg(value: object) -> str: | ||
| """Serialize a tool-call argument for the XML wire format. | ||
|
|
||
| The XML parser's literal_eval fallback can put values json.dumps rejects |
There was a problem hiding this comment.
Suggestion (pre-existing, adjacent — not introduced here): the premise this helper and TestParserToSerializerChain codify, literal_eval-produced sets landing in tool args, also exposes a small gap in _safe_literal (lines 30-38 in this file). It recurses into lists/dicts to reject non-finite floats but treats a set as an opaque value that hits return True, so ast.literal_eval('{1e400}') -> {inf} passes the gate and inf enters args as a float, while the same value nested in a list or dict is rejected and the raw string preserved (the intent documented at the except ValueError branch of _parse_xml_tool_call_to_dict, which exists to keep overflow as text instead of turning it into a float). Verified against head: literal_eval('{1e400}') == {inf} and _safe_literal of that is True. Nothing crashes either way — json.dumps renders Infinity, and this PR's str() fallback covers set replay — but if you want the overflow-rejection invariant to hold for the set value class this PR cements as supported, a set branch (all(_safe_literal(item) for item in value)) in _safe_literal would close it. Happy to see it in a follow-up.
github-actions
Bot
added
area:docs
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Follow-up at 869d231: the _safe_literal set-recursion gap flagged in the previous review is fixed — line 34 now recurses into (list, tuple, set), the README and models/AGENTS.md contracts were updated to match, and the new parametrized tests pin both directions (non-finite floats in nested sets/tuples/dicts keep the whole argument as text; finite set/tuple containers still parse and replay). Everything else since 4cee6f1 is additive test coverage; no new issues found. One residual hole in the same class is posted inline (dict keys are not recursed, so a non-finite float can ride in as a key); it does not crash thanks to _render_tool_arg, so fine to defer like the set gap was.
| if isinstance(value, (list, tuple, set)): | ||
| return all(_safe_literal(item) for item in value) | ||
| if isinstance(value, dict): | ||
| return all(_safe_literal(item) for item in value.values()) |
There was a problem hiding this comment.
Suggestion (residual of the set gap just fixed, pre-existing): _safe_literal recurses into dict values on this line but not dict keys, so the same class this PR closed for sets still has one hole — ast.literal_eval('{1e400: 2}') -> {inf: 2} passes the gate because the non-finite float rides in as a key, while {'k': 1e400} is rejected and kept as text. Verified against head: literal_eval('{1e400: 2}') == {inf: 2} and _safe_literal of it is True. Nothing crashes either way: json.dumps renders the float key as "Infinity", and the nested-tuple-key case ({(1e400,): 2}) fails json.dumps on the tuple key itself, which the new _render_tool_arg degrades to str(). One line closes it: all(_safe_literal(k) and _safe_literal(v) for k, v in value.items()). Happy to see it in a follow-up.
Why
The XML tool-call parser in the MindIE provider falls back to
ast.literal_evalfor{-prefixed parameter values thatjson.loadsrejects (backend/packages/harness/deerflow/models/mindie_provider.py:157 onmain), and by design the parsed values flow into tool-call args "to satisfy downstream Pydantic validation" (mindie_provider.py:150). So a model emitting a set literal such as{1, 2, 3}yields an AIMessage whose tool-call args contain a Pythonset.That AIMessage rides the conversation history. On the next turn
_fix_messagesre-serializes it into the XML wire format, and the args join rendered every non-str value through an unguardedjson.dumps(v, ensure_ascii=False)(mindie_provider.py:81 onmain). A set raisesTypeError: Object of type set is not JSON serializable, and since the message stays in history, every subsequent request of the thread fails — not just the one tool call.The ToolMessage json-block branch in the same function already tolerates exactly this class of value: its comment reads "Circular payloads raise ValueError, other non-serializable values TypeError; degrade both to str()" (mindie_provider.py:67-68), implemented as a try/except around
json.dumpsfalling back tostr()(mindie_provider.py:69-72), covered bytest_tool_message_json_block_unserializable_degrades_to_str. The AIMessage args join was the one serializer without that fallback.What changed
_render_tool_arghelper and routed the AIMessage args join in_fix_messagesthrough it: strings render unchanged, other values go throughjson.dumps(..., ensure_ascii=False)and degrade tostr(v)onTypeError/ValueError— the same fallback the tool-result json block branch in the same function already uses. No behavior change for any argument that serialized successfully before.Surface area
backend/packages/harness/deerflow/models/mindie_provider.py); no graph wiring, prompt, or API-shape change.Bug fix verification
backend/tests/test_mindie_provider.py::TestFixMessages::test_ai_message_tool_args_unserializable_degrades_to_strbackend/tests/test_mindie_provider.py::TestParserToSerializerChain::test_set_literal_arg_replayed_through_fix_messages(parser → AIMessage → next-turn_fix_messagesround trip)mainand green on this branch? yes — onmainboth fail withTypeError: Object of type set is not JSON serializable; on this branch both pass.Validation
Run from
backend/:PYTHONPATH=. PYTHONIOENCODING=utf-8 PYTHONUTF8=1 uv run pytest tests/test_mindie_provider.py -q→ 126 passedPYTHONPATH=. PYTHONIOENCODING=utf-8 PYTHONUTF8=1 uv run pytest tests/test_mindie_provider.py tests/test_pii_redaction_middleware.py tests/test_setup_wizard.py tests/test_model_factory.py tests/test_tool_output_budget_middleware.py -q→ 649 passedmake lint→ruff check .: "All checks passed!";ruff format --check .: "2048 files already formatted"AI assistance
Tool(s) used: ZCode agent (GLM model)
How you used it: The agent located the crash path by reading the provider and its existing tests, wrote the regression tests and the one-spot fix, and ran the validation commands listed above.