Skip to content

fix(gateway): deliver MEDIA files for queued follow-up responses - #18546

Closed
vnhwd wants to merge 4 commits into
NousResearch:mainfrom
vnhwd:fix/queue-media-delivery
Closed

vnhwd wants to merge 4 commits into
NousResearch:mainfrom
vnhwd:fix/queue-media-delivery

Conversation

@vnhwd

@vnhwd vnhwd commented May 1, 2026

Copy link
Copy Markdown

Summary

When multiple /queue items are chained in a single session, only the last item's MEDIA files (PDFs, images, etc.) were delivered. Preceding items showed raw MEDIA:/path tags because the queue drain path called adapter.send() without first extracting and delivering media attachments.

Fix

Call adapter.extract_media() before adapter.send() in the queue follow-up path (gateway/run.py), mirroring the normal delivery path in gateway/platforms/base.py L2724. Each extracted file is then delivered via adapter.send_document().

+ media_files, first_response = adapter.extract_media(first_response)
  await adapter.send(source.chat_id, first_response, ...)
+ for media_path, _is_voice in media_files:
+     await adapter.send_document(chat_id=source.chat_id, file_path=media_path, ...)

Testing

  • Existing test suites pass: 3385 passed, 10 skipped
    • tests/gateway/test_pending_drain_no_recursion.py — 5/5 ✓
    • tests/gateway/test_tts_media_routing.py — 29/29 ✓
    • tests/gateway/test_send_image_file.py — all ✓

Closes #18539

When multiple /queue items are chained, only the last item's MEDIA
files (PDFs, images, etc.) were delivered.  Preceding items showed raw
MEDIA:/path tags because the queue drain path called adapter.send()
without first extracting and delivering media attachments.

Fix: call adapter.extract_media() before adapter.send() in the queue
follow-up path, mirroring the normal delivery path in
gateway/platforms/base.py L2724, and deliver each extracted file via
adapter.send_document().

Closes NousResearch#18539
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery labels May 1, 2026
TS added 2 commits May 2, 2026 04:53
Add the three missing processing steps that the normal
delivery path (platforms/base.py L2724-2736) applies but
the queue follow-up path was missing:

- extract_images() — native delivery of markdown images
- re.sub(r'MEDIA:\s*\S+', ...) — secondary strip safety net
- extract_local_files() — bare-path detection for small models

Also clean [[audio_as_voice]] directive from text content.

Refs: NousResearch#18539
Cover the three newly-aligned processing steps in the queue
follow-up response path (b6fbcb6):

- extract_images() — markdown image extraction and delivery
- Secondary re.sub(r'MEDIA:\s*\S+', ...) safety strip
- extract_local_files() — bare-path detection
- [[audio_as_voice]] directive cleanup

9 new tests covering: single/multiple MEDIA tags, HTML fallback,
voice directive, markdown images, stray MEDIA cleanup, plain text
no-crash, and mixed media+image combos.

Refs: NousResearch#18539

@liuhao1024 liuhao1024 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The media extraction pipeline for queued follow-ups is correct and well-tested. One production-reliability issue though:

Inconsistent error handling in delivery loops. The image and local-file delivery loops use bare except Exception: pass, silently swallowing all failures:

# Deliver extracted images
for image_url, alt_text in (images or []):
    try:
        await adapter.send_image(...)
    except Exception:
        pass  # ← silent
# Deliver extracted local files
for local_path in (local_files or []):
    try:
        await adapter.send_document(...)
    except Exception:
        pass  # ← silent

But the media-file loop correctly logs:

except Exception as media_e:
    logger.warning("Failed to deliver queue media %s: %s", media_path, media_e)

This means image and file delivery failures are completely invisible in production logs. If an adapter's send_image raises (bad URL, expired token, platform rate-limit), the user silently loses the image with no trace.

Suggestion: Match the media-file pattern for the other two loops:

except Exception as img_e:
    logger.warning("Failed to deliver queue image %s: %s", image_url, img_e)
except Exception as doc_e:
    logger.warning("Failed to deliver queue file %s: %s", local_path, doc_e)

…ia delivery

Match the media-file loop pattern for image and local-file delivery loops
so failures are visible in production logs instead of silently swallowed.

The media-file loop already logged with logger.warning; the image and
local-file loops used bare 'except Exception: pass'. Reviewer feedback:
make all three loops consistent for production observability.

Refs: NousResearch#18546
@vnhwd

vnhwd commented May 3, 2026

Copy link
Copy Markdown
Author

Thanks for catching the inconsistency — pushed a fix in e1bb21d. All three delivery loops (images, local files, media files) now use logger.warning() with consistent formatting. Full gateway test suite passes (2992 passed, the 1 failure is a pre-existing unrelated Discord test).

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for identifying a real queued-delivery gap. Current main still sends the raw queued response at gateway/run.py:19724-19728, so this needs a fix; however, this revision needs rework before salvage.

Problems

  • The added queue pipeline bypasses the media-path allowlist now required by the normal and post-stream paths (gateway/platforms/base.py:4903-4924, gateway/run.py:13121-13132). Commit 41d2c758c39a20c1213b65253a3459c1f96759a3 established that model-emitted attachment paths must be filtered before upload.
  • The proposed loop sends all extracted files as documents and discards _is_voice, unlike the current type-aware routing for images, voice, video, and documents (gateway/platforms/base.py:5050-5114).
  • The new tests replay copied extraction logic rather than executing the queued raw-send branch at gateway/run.py:19724, so they would not prove the integration point is fixed.

Suggested changes

  • Rework the current queued branch to preserve cleaned-text delivery while sharing the existing validation and native-routing behavior in gateway/run.py:13097-13216.
  • Add a regression test through the actual queued-follow-up path, including safe-path filtering and attachment routing.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 12, 2026
@teknium1

teknium1 commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Closing as superseded: #82162 (salvage of #71031) landed on main and fixes queued follow-up MEDIA delivery for both branches — the non-streamed fallback that leaked literal MEDIA: text (this PR's target) and the streamed branch that silently dropped attachments — while routing through the existing extract_media + _deliver_media_from_response machinery so image batching, voice routing, path-security filtering, and the explicit-only policy are preserved. Thanks for attacking this bug; your report and approach helped establish the fix's requirements.

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 sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: /queue FIFO chain drops MEDIA files — only last item gets media delivery

4 participants