Skip to content

fix(gateway): resolve MEDIA-tag producer-tool id through the canonical tool_call_id policy - #94331

Open
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/media-tag-divergent-tool-call-id
Open

fix(gateway): resolve MEDIA-tag producer-tool id through the canonical tool_call_id policy#94331
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/media-tag-divergent-tool-call-id

Conversation

@pierrenode

Copy link
Copy Markdown
Contributor

Summary

_collect_auto_append_media_tags() and _collect_history_media_paths() (gateway/run.py) each build a private tool-call-id → producer-name map when scanning an assistant message's tool_calls, using call.get("id") or call.get("call_id") to register the key, then look up the paired role="tool" result via msg.get("tool_call_id") or msg.get("call_id").

For a Codex/Responses-style tool_call whose id ("fc_...") and call_id ("call_...") diverge — the exact shape agent/message_sanitization.py's tool_call_id_variants() / coalesce_tool_call_id() were consolidated to handle (see #55626 / #58168 / #63000, and the same-week id-variant cluster in agent/agent_runtime_helpers.py) — the registration key (id-first) and the lookup key (tool_call_id, populated from call_id on Responses-shaped calls) never match.

Effect: the producer-tool name is never resolved, so a genuinely allowlisted text_to_speech / image_generate result is treated as an untrusted tool:

  • Its MEDIA: text tag is never auto-appended — the file is silently never delivered, no error surfaced.
  • Its image_generate JSON-payload path (host_image/image/agent_visible_image) is invisible to both the auto-append collector and the history-dedup collector, which would also let the same file be re-delivered on a later turn once the id happens to line up (e.g. after compression rewrites message shapes).

Fix

Route both functions through the single policy owner the rest of the codebase already uses for this exact divergent-id shape:

  • tool_call_id_variants(call) when registering — every wire spelling of a tool_call's pairing id (id, call_id, response_item_id, pipe-encoded bridge ids) maps to the producer name, not just one.
  • tool_result_id_variants(msg.get("tool_call_id") or msg.get("call_id")) when looking a result up — checked against every registered variant.

No behavior change for the common (single, matching id) case — only the divergent-id case is affected.

Verification

  • Empirically verified pre-fix: a tool_call with id="fc_abc123", call_id="call_xyz789" paired with a tool result whose tool_call_id is "call_xyz789" returns zero media tags for both the MEDIA: text-tag path and the image_generate JSON-payload path.
  • New regression tests (tests/gateway/test_media_tag_divergent_call_id.py, 6 tests): divergent-id text-tag survival, divergent-id JSON-payload survival (both functions), a control case (single matching id, unaffected), and a control case confirming the producer-tool allowlist still rejects an ineligible tool even with a divergent id.
  • Mutation-verified: stashing the fix reproduces the failure in exactly the 3 divergent-id tests; the 3 control tests are unaffected either way.
  • Full tests/gateway/test_73771_media_resend_dedup.py, test_media_extraction.py, test_media_spaced_paths_and_history_dedupe.py (28 tests) pass unchanged.
  • Broad tests/gateway/ sweep (5849 tests, excluding files that fail to collect in this environment due to a pre-existing aiohttp.test_utils import gap unrelated to this change): 12 pre-existing failures reproduce byte-for-byte with the fix stashed — confirmed independent of this change.
  • ruff check clean on both changed files.

Competitor / conflict notes (checked fresh before opening)

  • fix(gateway): handle silent audio and media producers #65745 (open, "handle silent audio and media producers") touches the same 3-line region in _collect_auto_append_media_tags for an unrelated concern — adding a fallback that reads msg.get("tool_name")/msg.get("name") directly when the call_id-based lookup misses entirely (e.g. a message that never went through a tool_calls pairing at all). Textual proximity only; no semantic overlap with this fix's id-resolution change. Whichever lands first will need a small rebase for the other.
  • refactor(gateway): extract auto-continue helpers from run.py (slice 8 of #54962) #77706 (open, "slice 8 of Extract Gateway Platform Routing from gateway/run.py #54962" — a large-scale extraction of helpers out of gateway/run.py) moves both target functions verbatim (bug intact) into a new gateway/auto_continue_helpers.py module. Pure relocation, no logic change on that branch — if it lands, this fix would need to be re-applied at the new location, but there is no semantic conflict.
  • No open or closed PR fixes the id-resolution logic itself in either function.

…l tool_call_id policy

_collect_auto_append_media_tags() and _collect_history_media_paths()
each built a private tool_call-id-to-name map using
call.get("id") or call.get("call_id") when registering an assistant
tool_call's producer name, then looked up the paired tool result via
msg.get("tool_call_id") or msg.get("call_id").

For a Codex/Responses-style tool_call whose id ("fc_...") and call_id
("call_...") diverge — the exact shape agent/message_sanitization.py's
tool_call_id_variants()/coalesce_tool_call_id() were consolidated to
handle (NousResearch#55626/NousResearch#58168/NousResearch#63000) — the registration key (id-first) and the
lookup key (tool_call_id, populated from call_id) never match. The
producer-tool name is never resolved, so a real text_to_speech/
image_generate result is treated as an untrusted tool: its MEDIA: tag
is never auto-appended (silent data loss, no error surfaced) and its
JSON-payload path is never collected for cross-turn dedup.

Route both functions through the single policy owner
(tool_call_id_variants / tool_result_id_variants) that the rest of the
codebase already uses for this exact divergent-id shape, instead of
re-implementing a narrower one-field lookup.

Empirically verified: a tool_call with id="fc_abc123",
call_id="call_xyz789" paired with a tool result whose tool_call_id is
"call_xyz789" returned zero media tags on pre-fix code (both the
MEDIA: text-tag path and the image_generate JSON-payload path); the
image_generate path was also invisible to history-dedup collection,
which would let the same file be re-delivered on a later turn.

Mutation-verified: reverting the fix reproduces exactly this failure —
the 3 divergent-id regression tests fail, the 3 control tests (single
matching id, untrusted-tool filtering) are unaffected.
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery tool/tts Text-to-speech and transcription tool/vision Vision analysis and image generation provider/openai OpenAI / Codex Responses API P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 25, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

Correct fix in the right direction: routing both MEDIA collectors through agent.message_sanitization's variant helpers removes a private id-before-call_id policy whose divergence from the result-pairing key silently dropped TTS audio and image paths (fail-closed data loss, no error anywhere), and reusing the consolidated policy owner prevents the next divergence. The test set is well-chosen — especially test_untrusted_tool_still_filtered_with_divergent_id, which proves the fix repairs id resolution without widening the producer allowlist. Two small notes:

  1. The variant-resolution expression is duplicatedgateway/run.py:1880-1884 and again at :1985-1989 build result_variants + the same next(...) generator to resolve the producer name. A three-line local helper ("tool name for this result message") shared by both functions would keep the two call sites from drifting apart again — the exact failure mode this PR is cleaning up.

  2. The bridge-id spelling isn't exercised by tests — the comments (and the policy owner's contract) mention pipe-encoded bridge ids as one of the wire spellings tool_result_id_variants normalizes, but every test uses plain id/call_id. One case with an encoded variant result paired against a decoded registration would pin the helper's actual normalization behavior rather than trusting it — cheap insurance if the encoding scheme ever changes.

Also fine: registering all variants unconditionally when a name exists (vs. the old if call_id and name gate) only enlarges the map with keys that can't collide across distinct calls, so no false-attribution risk introduced.

prmartinow pushed a commit to prmartinow/hermes-agent that referenced this pull request Aug 26, 2026
Fixes NousResearch#898 — Python 3.11 changed argparse to raise an exception on
duplicate subparser names (CPython NousResearch#94331). The 'skills' name was
registered twice: once for Skills Hub and once for skills config.

Changes:
- Remove duplicate 'skills' subparser registration
- Add 'config' as a sub-action under the existing 'hermes skills' command
- Route 'hermes skills config' to skills_config module
- Add regression test to catch future duplicates

Migration: 'hermes skills' (config) is now 'hermes skills config'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists provider/openai OpenAI / Codex Responses API sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages tool/tts Text-to-speech and transcription tool/vision Vision analysis and image generation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants