fix(agent): harden structured message text projection - #67380
Conversation
Related to merged #66945: this is a broader residual structured-content projection repair, not a duplicate of the already-landed immediate-crash fix. |
|
Thanks for extending the residual structured-content repair. Static inspection confirms the premise still holds on current main: The allowlisted projector preserves the established supported shapes in Automated hermes-sweeper review. |
Structured assistant/tool content (typed parts, multimodal lists, legacy wrappers) was handled inconsistently across call sites, with three observable gaps after NousResearch#66267/NousResearch#66945: * split reasoning tags: flatten_message_text joined parts with '\n', so a provider splitting '<think>' across parts ('<thi' + 'nk>...') reassembled a tag the scrubber regex could no longer match, leaking reasoning into visible text; * unknown-shape leakage: untyped Mappings with extra provider/tool fields, arbitrary objects with .text/.content, and top-level str(content) fallbacks could put provider metadata, tool payloads or object reprs into the visible reply; * interim dedup ran the assistant text extractor on any dict message, including user/tool/system roles. Make agent/message_content.py the single canonical projector: * allow only typed text parts (text/input_text/output_text), explicit summary parts, plain strings and PURE legacy wrappers (keys limited to text/content); everything else — unknown typed parts, images, base64, audio, tool metadata, encrypted/redacted reasoning, unknown objects, hostile Mappings whose accessors raise — yields ''; * scrub pipeline (strip_think_blocks, build_assistant_message, interim visible text, refusal/length paths) joins parts with sep='' so provider-split tags reassemble exactly before scrubbing; * gate interim dedup on role == 'assistant' before text extraction; * preserve structured reasoning before projecting assistant content. Adds tests/run_agent/test_structured_content_projection.py covering the contract: typed parts, ordering, split <think>, unknown objects, hostile Mappings, pure wrappers, tool-role guard, refusal/length normalization.
0c4cb95 to
1f385c8
Compare
|
Rebased onto current main and semantically revalidated the structured-content projection boundary.
This PR is ready for direct merge. |
Follow-up hardening for NousResearch#66267 / NousResearch#66945 (independent companion to NousResearch#67380, with no code dependency on it). The traceback-module classifier cannot reliably tell 'provider request failed' from 'Hermes-local post-processing failed': once the provider has delivered a terminal response, any later local exception (aggregation, usage handling, normalization, even an exception whose type LOOKS like a network error) could still be routed into provider retry, reconnect, fallback, continuation, or a partial-stream 'length' stub — re-requesting an already-completed, billable response. A stale-killed but still-alive worker can also leak its terminal signal into a newer attempt when phase lives in shared agent state. Introduce an explicit, attempt-scoped lifecycle: * ProviderAttemptLifecycle token (in_flight -> terminal_received) per real provider attempt, created by the conversation retry loop and held as a loop-local reference. * Transports capture the token ONCE at the attempt's main entry (interruptible_api_call / direct_api_call / interruptible_streaming_api_call, on the calling thread before any worker starts) and pass it down via explicit parameters or bound closures — _dispatch_nonstreaming_api_request, _run_codex_stream, _anthropic_messages_create, on_terminal / on_response_received callbacks, and the loop's backstop. Nothing past worker start re-reads agent._provider_attempt for writer identity. * Auxiliary entry points invoked without an attempt (e.g. Codex iteration-limit summaries, Anthropic auxiliary calls) run on a fresh DETACHED token, so they never inherit the main loop's terminal state and keep their own internal reconnect. * Terminal boundary marks: non-streaming raw return on every dispatch branch, Chat finish_reason, Anthropic message_stop (incl. shim), Codex terminal response event. * After terminal: no retry / reconnect / fallback / continuation / length stub — the turn ends through the unified finalizer as local_post_response_error with failed=True and the real api_calls. * Before terminal: upstream retry/fallback behavior is unchanged; the traceback classifier remains only as pre-terminal defense-in-depth. * Interruption semantics unchanged. Scope: Chat Completions (streaming/non-streaming), Codex Responses, Anthropic Messages (streaming/non-streaming, incl. the Kimi Coding path), DeepSeek chat-completions, and the Bedrock non-streaming raw-return boundary. Bedrock streaming (converse_stream messageStop) is explicitly out of scope. Tests (new, deterministic wire-seam fakes through the real AIAgent.run_conversation entry): * test_post_response_retry_boundary.py — non-streaming boundary: post-return local error call_count=1, pre-terminal network error call_count=2, Bedrock raw-return boundary. * test_streaming_terminal_boundary.py — finish_reason / message_stop / terminal event then local failure: wire call_count=1, no stub, no continuation; pre-terminal reconnect preserved; interruption intact. * test_provider_lifecycle_paths.py — real provider identities and routing: Codex Responses, Kimi Coding, Kimi API, DeepSeek, plus detached-token auxiliary calls. * test_provider_attempt_lifecycle.py — deterministic stale-worker races (worker alive past stale-kill, and worker delayed before dispatch) proving a late terminal only ever lands on its own token.
What
Follow-up to #66267 / #66945. Those fixes stopped the immediate crashes from
multimodal list content, but three gaps remain reproducible on current main:
Split reasoning tags break the scrubber.
flatten_message_textjoinsparts with
"\n"by default. When a provider splits<think>across twoparts (
"<thi"+"nk>secret</think>visible"), the reassembled stringcontains a newline the original never had, the
<think>...</think>regexno longer matches, and reasoning leaks into the visible reply.
Unknown shapes leak into visible text. Untyped Mappings are read for
text/contentregardless of extra fields(
{"provider": "x", "content": "SECRET"}→"SECRET";[{"tool_call_id": "abc", "content": "SECRET"}]→"SECRET"); objects ofunknown type contribute attribute values; top-level
str(content)stringifies unknown objects; Mappings whose accessors raise propagate the
exception instead of yielding
"".Interim dedup has no role gate. The assistant-visible-text extractor
runs on any dict message — user, tool, system — before checking whether it
is an assistant turn at all.
There are also multiple slightly different list-flattening implementations
across call sites with divergent contracts.
How
agent/message_content.pybecomes the single canonical projector with astrict allowlist: plain
str; typed text parts(
text/input_text/output_text); explicitsummary_text; and purelegacy wrappers (untyped Mappings whose keys are a subset of
{text, content}). Everything else — unknown typed parts, images, base64,audio, tool metadata, encrypted/redacted reasoning, unknown objects,
hostile Mappings — yields
"".explicitly allowlisted shapes contribute text.
allowlisted textual type (
.type∈ text/input_text/output_text);non-textual or unknown types are never read.
str()/repr()fallback anywhere in the projection path; Mappingswhose accessors raise yield
""without propagating.strip_think_blocks,build_assistant_message,interim visible text, refusal/length normalization) joins parts with
sep=""so provider-split tags reassemble byte-exactly before scrubbing.role == "assistant"before text extraction.thinking blocks are not silently dropped.
Tests
tests/run_agent/test_structured_content_projection.py: typed parts,multi-part order, empty text, non-string text fields, unknown part types,
unknown objects (
.text/.content/custom__str__/nested), image/base64/reasoning/metadata non-leakage, split
<think>reassembly, tool-roleguard, refusal/
lengthnormalization, pure legacy wrappers, and hostileMappings (raising
get/keys/iteration, top-level and in-list, neverpropagating).
test_66267_multimodal_interim.py,test_message_content.py,think/streaming-context scrubber suites and related Codex/Kimi/DeepSeek
suites continue to pass.
No behavior change for plain-string content; no prompt, config, or transport
changes.
Compatibility contract
Preserved:
Intentionally rejected:
This is a visible-text projection contract, not a general message
serialization or history-schema migration.
Related defense: #67411
#67380 removes the concrete structured-content projection failure.
#67411 independently prevents a provider request from being reissued when
a Hermes-local processing failure occurs after a terminal provider response.
The PRs share no commits, are independently mergeable, and may land in
either order.
Merge-order matrix:
local_post_response_errorwithout a repeated provider request; #67380 later removes the root cause.Current-main validation
Rebased onto
e702a45b5(current main) and semantically revalidated:unknown-object/hostile-mapping leaks, split
<think>scrub miss,non-assistant interim extraction, structured reasoning loss);
media / hostile shapes stay fail-closed;
sep=""before scrubbing;