Conversation
c78442e to
17c089d
Compare
|
Rebased this PR onto current Local verification after rebase:
|
17c089d to
fb5df14
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the queued-delivery path; the current direct send at gateway/run.py:19845 does bypass the normal media pipeline, matching the report in #60845.
Problems
gateway/run.py:12764discards the images returned byadapter.extract_images(). The later_deliver_media_from_response()call also discards extracted images (gateway/run.py:13250), whereas the normal response path dispatches them withsend_multiple_images()(gateway/platforms/base.py:4930,gateway/platforms/base.py:5055). A queued first response containing a remote markdown/HTML image would therefore lose its attachment.
Suggested changes
- Preserve and dispatch extracted image URLs with the queued response metadata, and add a remote-image regression test.
- Exercise the queued fallback call site in addition to the helper directly, so synthetic-event routing and ordering are covered.
This is an automated hermes-sweeper review.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address the same queued-follow-up delivery defect: both replace the bare first-response send with clean-text delivery followed by native media handling, while #46223 additionally preserves remote extracted images and #51805 provides broader queued-call-site coverage.
Related pull requests
- #46223
related— (+173/-6) — keep open with a salvage path: The focused helper removes media directives from queued-response text, routes local files and remote images through the attachment pipeline, and constructs the routing event required by the resend branch. This aligns with the keep_open review on #46223 and already addresses its remote-image concern, but the diff still needs a regression test that exercises the queued fallback call site rather than only the helper. - #51805
duplicate— (+426/-32) — close as duplicate of #46223 after salvaging its integration test: It fixes the same bare-send cause and uniquely exercises the full queued-follow-up path, but it makes a broader helper-signature refactor and does not deliver extracted remote markdown/HTML images. Despite the keep_open, high-salvageability maintainer-bot verdict on #51805, the diff of #46223 contains the narrower current implementation and remote-image handling; the valuable end-to-end test from #51805 should be ported to #46223.
Duplicates
#51805 and #46223 implement substantially the same clean-text-plus-media resend path; treat #51805 as a duplicate of #46223, with its full queued-follow-up regression test salvaged into #46223.
Suggested consolidation
Keep #46223 open with a salvage path: add the queued fallback call-site and ordering coverage demonstrated by #51805, while retaining #46223's remote-image dispatch and synthetic-event routing. Then close #51805 as duplicate of #46223; this differs from the maintainer-bot keep_open verdict on #51805 because the competing diff in #46223 covers the same root cause more narrowly and handles remote extracted images, while #51805's distinct value is its portable end-to-end regression test.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup46223 ["PRs duplicating each other"]
P46223["PR #46223 (open)"]
P51805["PR #51805 (open)"]
end
class P46223 open
class P51805 open
class P46223 target
click P46223 "https://github.com/NousResearch/hermes-agent/pull/46223"
click P51805 "https://github.com/NousResearch/hermes-agent/pull/51805"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 30 kB of PR diffs, 6 kB of issue/PR text, 1 kB of discussion (2 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
|
+1, also getting this problem on Slack and I'm running a local patch |
|
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 |
What does this PR do?
Fixes a gateway delivery path where a completed agent response is sent before draining a queued follow-up message. That path previously called the first-response delivery from inside
_run_agent, where it could either bypass normal response media post-processing or, in the helper-based path, reference an out-of-scopeeventvariable.The change adds a small helper that mirrors normal response delivery for this queued path: send only displayable text, then deliver extracted
MEDIA:/ local files via the existing post-stream media delivery helper. The call site now builds a syntheticMessageEventfrom the currentsourcebefore invoking the helper, so queued first-response delivery has the chat/thread target it needs without relying on a non-existent localevent.This prevents raw
MEDIA:/path/to/filetext from leaking to users, preserves attachments when a user sends a follow-up while the first turn is completing, and avoids warnings like:Related Issue
No related upstream issue or PR found. Searched PRs and issues for
MEDIA queued follow-up,final stream delivery,already_sent,MEDIA directive, andsend_document MEDIA Telegram.Type of Change
Changes Made
gateway/run.py_send_queued_first_response()and use it in the queued follow-up first-response branch instead of rawadapter.send().MessageEventfrom the currentsourceat the queued call site so media delivery has chat/thread metadata and does not reference an undefined localevent.tests/gateway/test_queued_followup_media_delivery.pyMEDIA:and media-only queued first responses.How to Test
MEDIA:/path/to/file.htmlwhile a follow-up message is already queued.MEDIA:text and skip native attachment delivery, or fail the helper call withname 'event' is not defined.MEDIA:,send_document()receives the referenced file, and the queued first-response branch has a valid response event.Commands run:
/usr/local/lib/hermes-agent/venv/bin/python -m pytest -q -o addopts='' tests/gateway/test_queued_followup_media_delivery.py tests/gateway/test_media_extraction.py tests/gateway/test_duplicate_reply_suppression.py /usr/local/lib/hermes-agent/venv/bin/python -m ruff check gateway/run.py tests/gateway/test_queued_followup_media_delivery.py git diff --checkResults:
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
cli-config.yaml.exampleupdate N/ACONTRIBUTING.mdorAGENTS.mdupdate N/AScreenshots / Logs
Observed live on Telegram: a queued first response containing text plus
MEDIA:/root/pivin-scenario.htmlrendered the rawMEDIA:line instead of delivering the HTML attachment; a later media-only retry delivered the file.A later live text-batch split also hit the same queued first-response branch and logged:
The regression tests encode the media delivery failure mode without depending on Telegram network state; the call-site fix removes the undefined
eventreference from the queued branch.