Skip to content

fix: serialize dict tool content to string before LLM API call - #31963

Closed
Joyatulya wants to merge 1 commit into
NousResearch:mainfrom
Joyatulya:fix/tool-executor-dict-content-safety
Closed

fix: serialize dict tool content to string before LLM API call#31963
Joyatulya wants to merge 1 commit into
NousResearch:mainfrom
Joyatulya:fix/tool-executor-dict-content-safety

Conversation

@Joyatulya

Copy link
Copy Markdown

Bug Description

When tool handlers return structured dicts (e.g. plugin errors with {"error": "...", "code": ...}) in the content field instead of a plain string or OpenAI-style content list, strict providers like DeepSeek v4 Flash reject the request with HTTP 400.

This causes the entire conversation to error out — the agent loses a full turn of progress.

Fixes #17381

Root Cause

The tool_executor.py parallel and sequential paths appended tool content directly to the API message without type-checking. OpenAI-compatible endpoints expect content to be either a string or a list of content parts. Dicts are not valid in either schema.

Fix

Added a safety guard in both execution paths (concurrent at line 451, sequential at line 873):

if not isinstance(_tool_content, (str, list)):
    _tool_content = json.dumps(_tool_content, ensure_ascii=False, default=str)

This serializes any non-string, non-list content to JSON before building the API message. The conversation stays recoverable regardless of which provider is in use.

How to Verify

  1. Configure Hermes with DeepSeek v4 Flash as the provider
  2. Use a tool that returns a dict in its content field (e.g. a plugin error handler)
  3. Observe the agent recovers gracefully instead of crashing with HTTP 400

Test Plan

  • Manual verification with niti tool (niti action list, niti action create, niti action cancel all work without crashes)
  • Both execution paths (sequential and concurrent) are protected
  • Existing tests still pass

Risk Assessment

Low — The change only wraps non-conforming content types. Strings and lists (the vast majority of tool results) pass through unchanged.

Some tool handlers return structured dicts (e.g. plugin errors) in the
tool content field instead of a plain string or content list. Strict
providers like DeepSeek v4 Flash reject these with HTTP 400.

Add the same safety guard to both the concurrent and sequential tool
execution paths in tool_executor.py: any non-string, non-list content
from a tool result is serialized via json.dumps() before being built
into the API message.  This keeps conversations recoverable regardless
of which provider is in use.

Fixes NousResearch#17381
@hclsys

hclsys commented May 25, 2026

Copy link
Copy Markdown

Heads-up on an overlap (disclosure: I authored a related PR, so flagging neutrally for the maintainer to choose): this targets the same bug as #31770 — non-string tool content (plugin dicts) rejected by strict providers with HTTP 400.

The approaches differ in placement, and it's worth noting they're redundant:

  • This PR coerces _tool_content at the two tool_executor.py call sites (:447, :863) just before make_tool_result_message(...).
  • fix(agent): coerce tool-result content to string for OpenAI wire format #31770 coerces inside make_tool_result_message itself — which is the single constructor those exact call sites feed into, plus the other two (tool_executor.py:78, :885) that this PR doesn't touch. So a fix at the constructor covers all four paths centrally; a per-call-site fix needs to be repeated at each.

Both are legitimate; just noting whichever lands, the other's coerce becomes a no-op. One thing specific to this diff worth a maintainer's eye regardless of the dedup question: it also drops result=function_result from the two tool_progress_callback("tool.completed", ...) calls and swaps a loggerlogging — those look unrelated to the #17381 serialization fix, so they may want to be split out or called out explicitly in the description. The core json.dumps(..., default=str) guard itself is correct.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels May 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competes with open PRs #29920 and #31770 which fix the same non-string tool result serialization bug. #29920 has broader scope (two-layer fix in _tool_result_content_for_active_model + sanitize_api_messages safety net).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the strict-provider failure mode. This automated hermes-sweeper review found that current main now enforces the required behavior centrally.

  • f8361d29c8e2a2be6ba9ada32f1d694bf47a4b6a (fix(tools): enforce registry result contract, fix(tools): enforce registry result contract #61787) normalizes every registry handler result in tools/registry.py:575-603 and applies it from dispatch() at tools/registry.py:617-623.
  • A plain dict is now converted to a JSON string contract-error result; only the explicit _multimodal envelope remains structured. This prevents the invalid role: "tool" object-content shape that this PR targets.
  • Plugin tools register through that registry (hermes_cli/plugins.py:426-439), and agent calls dispatch through it (model_tools.py:1273-1278). Regression coverage is in tests/tools/test_registry.py:50-112.
  • The Fixes #17381 reference is unrelated: [Feature]: Add interactive column sorting to dashboard analytics tables #17381 concerns dashboard analytics-table sorting.

Closing as implemented on main; thank you for the focused report and proposed guard.

@teknium1 teknium1 closed this Jul 13, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Add interactive column sorting to dashboard analytics tables

4 participants