Skip to content

fix(gateway): thread a real task_id through post-stream media delivery and cron tasks - #74066

Open
elphamale wants to merge 1 commit into
NousResearch:mainfrom
elphamale:fix/thread-task-id-media-delivery
Open

elphamale wants to merge 1 commit into
NousResearch:mainfrom
elphamale:fix/thread-task-id-media-delivery

Conversation

@elphamale

Copy link
Copy Markdown

What does this PR do?

Stacked on #47716 — this branch is built on top of that PR's, so the diff
below includes its 5 commits until #47716 merges. Only the last commit
(fix(gateway): thread a real task_id through post-stream media delivery and cron tasks) is this PR's own; the file-level review should focus there.

Follow-up to #47716. That PR added Docker container→host path translation for
MEDIA: file delivery, but several call sites have no task_id available, so
translate_docker_media_paths() degrades to mount-table-only translation and
only proceeds when exactly one Docker terminal environment is active
gateway-wide — with zero or multiple concurrently active environments (as on
any busy multi-session deployment) it no-ops and the file is dropped, even
though the run that actually produced it is still alive and its environment
still resolvable by its own task_id.

Reproduced in production: a Telegram guest-mode reply's buffered "OPC"
flush (guest mode buffers the whole reply and delivers it as a single edit at
turn end, not via live streaming) routes through
gateway/run.py::_deliver_media_from_response — one of the task-id-less
paths. Across three separate turns in the same guest chat, the agent
correctly emitted MEDIA:/home/.../cache/videos/....mp4, and each time the
gateway log showed Skipping unsafe MEDIA directive path — the video was
silently dropped, with only the text reply delivered.

This PR threads a real task_id through the two paths where the producing
task genuinely is identifiable: the post-stream media rescan, and cron jobs.

Related Issue

Addresses #64889 for the two paths listed below.
Not filing as Fixes #64889 — the issue's third path
(gateway/platforms/base.py's generic handle_message() / weixin.py's
send()) is intentionally left open; see "Deliberately out of scope."

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • run_agent.py: AIAgent.run_conversation() already computes its own
    effective_task_id before calling into
    agent.conversation_loop.run_conversation(). It now surfaces that value as
    result["task_id"] on every dict return (the exception path re-raises and
    never returns a dict, so no other branch needs touching) — any caller
    holding the turn's result dict can recover the exact task_id that
    produced it, the same way already_sent/failed already work.
  • gateway/run.py: _deliver_media_from_response() takes an optional
    task_id parameter and forwards it into translate_docker_media_paths().
    Its one caller now passes agent_result.get("task_id").
  • cron/scheduler.py: run_job() now passes task_id=str(job_id) into
    agent.run_conversation(), so a cron job's own terminal environment is
    deterministically identifiable by its own job id instead of an unrelated
    random UUID generated fresh per run. _send_media_via_adapter() already
    receives the full job dict — it now reads job["id"] back as that same
    task_id (previously the function's own comment noted this was not the
    real task_id; it now is).

Deliberately out of scope

gateway/platforms/base.py's generic handle_message() and weixin.py's
send() still can't identify the producing task without a larger contract
change: both receive their text from self._message_handler(event), and
MessageHandler is typed to return a plain Optional[str] — the full
agent_result dict (and its new task_id key) never reaches this layer.
Threading it through would mean changing that protocol's return type across
every platform adapter that registers a handler, which is exactly the kind of
"bigger than a bug fix" shared-contract change #47716 itself declined to
bundle (see that PR's "Why this isn't in #47716" note). Left degraded,
unchanged from before this PR — still tracked by #64889.

How to Test

  1. scripts/run_tests.sh tests/run_agent/test_run_agent.py tests/gateway/test_post_stream_media_delivery.py tests/cron/test_scheduler.py tests/agent/test_subagent_lifecycle.py
    all pass, including four new tests covering explicit/auto-generated
    task_id surfacing, forwarding into translate_docker_media_paths, and
    the cron job-id threading.
  2. Manual repro of the original bug: enable a Docker terminal backend, start
    a Telegram guest-mode chat, have the agent create a video file and reply
    with a MEDIA: tag while at least one other, unrelated terminal
    environment is concurrently active elsewhere on the gateway. Before this
    fix: video silently dropped (Skipping unsafe MEDIA directive path in the
    log) whenever zero or 2+ environments are active gateway-wide at flush
    time. After: delivered, because translation now resolves this turn's own
    environment directly by its own task_id regardless of what else is
    running.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes
  • I've tested on my platform: Linux (Ubuntu, x86_64)

Documentation & Housekeeping

  • N/A — no config keys, docs, or tool schemas changed; no architecture/workflow change to CONTRIBUTING.md/AGENTS.md
  • Cross-platform impact considered — this is Python dict/string plumbing only, no OS-touching code

Screenshots / Logs

Live log excerpt from the reproduced failure, before this fix:

WARNING gateway.platforms.base: Skipping unsafe MEDIA directive path: /home/hermes/.hermes/cache/videos/insta_....mp4
WARNING hermes_plugins.telegram_platform.adapter: [Telegram] guest OPC flush (chat=... buffered_len=26 turn_media=False imi=...)

turn_media=False on every flush confirms the guest-mode media staging
function was never reached — the file never survived path translation to get
there.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/gateway Gateway runner, session dispatch, delivery comp/cron Cron scheduler and job management comp/tools Tool registry, model_tools, toolsets platform/wecom WeCom / WeChat Work adapter backend/docker Docker container execution sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P2 Medium — degraded but workaround exists labels Jul 29, 2026
@elphamale
elphamale force-pushed the fix/thread-task-id-media-delivery branch 4 times, most recently from 7c823df to 2760d7d Compare July 30, 2026 11:25

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for tracing the post-stream task identity through the gateway; that path matches a real current-main limitation (gateway/run.py:18393-18394).

Problems

  • Blocking: the cron path still loses container-local media before the new task-bound lookup runs. At cron/scheduler.py:1525-1527, _deliver_result() extracts media, uses unscoped translation, and filters it. Under the multi-environment case described by this PR, that filter drops the unchanged container path. The later task-bound translation at cron/scheduler.py:1342-1344 therefore receives no media.

Suggested changes

  • Thread str(job["id"]) into the translation at cron/scheduler.py:1526 before the first filter, or move filtering to the task-bound helper. Add a two-active-Docker-environment regression proving a container-only cron attachment reaches the adapter.

Automated hermes-sweeper review.

Comment thread cron/scheduler.py Outdated
# instead of degrading to mount-table-only / single-Docker-environment-only
# translation.
media_files = BasePlatformAdapter.translate_docker_media_paths(
media_files, task_id=str(job.get("id") or "") or None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This task-bound translation is too late for cron delivery: _deliver_result() already calls task-id-less translation and filter_media_delivery_paths() at lines 1525-1527. In the multiple-environment case, that earlier filter drops the container path, so this helper receives an empty list. Pass the job task_id into the earlier translation (or defer filtering) and cover that ordering with a regression test.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/streaming Streaming responses: gateway delivery, provider wire labels Jul 30, 2026
@elphamale
elphamale force-pushed the fix/thread-task-id-media-delivery branch 4 times, most recently from 48e81ec to 20ac63f Compare August 9, 2026 08:11
@elphamale
elphamale force-pushed the fix/thread-task-id-media-delivery branch from 20ac63f to d1578c1 Compare August 18, 2026 06:32
@elphamale
elphamale force-pushed the fix/thread-task-id-media-delivery branch from d1578c1 to c258c24 Compare August 26, 2026 07:48
@elphamale

Copy link
Copy Markdown
Author

Re-scoped this PR following @teknium1's guidance on #94441 (thanks for flagging it there rather than letting it re-derive independently).

What changed and why:

The original approach here (thread task_id through _default_docker_workspace_host_root/_docker_persistent_home_host_root to resolve <sandbox>/docker/<task_id>/...) is now fully superseded by #94560, which fixed the actual root cause behind #93950/#64889: persistent Docker was never supposed to be per-session at all — it's one container per profile. That PR rewrote the resolution into _docker_sandbox_dir_candidates(session_key), keyed by profile name. Re-threading task_id into that mechanism today would resolve a directory that mostly doesn't exist under the corrected architecture, so that whole approach is dropped from this PR.

What's kept — a still-open gap, independently re-confirmed against current main:

_docker_sandbox_dir_candidates()'s own docstring already explains why it takes session_key explicitly rather than reading it ambiently: delivery code runs after the turn's _profile_runtime_scope has often already exited (#93950), so an ambient lookup can silently resolve the wrong profile. But the function's primary candidate still calls get_active_profile_name() — an ambient lookup — while session_key only ever feeds the legacy fallback candidate. The same class of bug the docstring warns about, just not fully closed out.

Separately, I traced the actual call sites and found two in gateway/run.py that thread no identity at all today: _deliver_media_from_response (the post-stream MEDIA rescan) and the delegate-task background-result delivery path both call filter_media_delivery_paths(media_files) bare. Same for cron's _send_media_via_adapter/_deliver_result. None of these were touched by #94437/#94509/#94560.

This PR now:

  • Adds an explicit profile: Optional[str] = None parameter alongside session_key on _docker_sandbox_dir_candidates and everything in its call chain, used in place of the ambient lookup when the caller has it.
  • Threads SessionSource.profile/session_key (via _session_key_for_source) explicitly through both gateway/run.py call sites, derived from data carried on the event/source object itself — not from a contextvar, so it's correct regardless of whether _profile_runtime_scope is still active at that point.
  • Threads the job's own id as session_key through cron's two delivery functions, matching this PR's original identifier construction — cron's own per-profile-process design (see _get_hermes_home()'s docstring) made ambient profile lookup a plausibly-safer bet there, so I left profile on ambient fallback for cron rather than inventing a value with no verified source on the job dict.

All changes are additive (new optional params, defaults preserve existing behavior for every other caller) — squashed to one fresh commit against current main since the old approach shared essentially no code with what's here now.

Open question I didn't want to resolve unilaterally: is ambient get_active_profile_name() actually reliable at cron's delivery call sites in a multiplexed gateway, or does cron need the same explicit-profile treatment I gave the gateway path? I traced the gateway path concretely enough to be confident; cron's dispatch architecture (per-profile ticker process vs. a shared multiplexed one) I could not fully verify either way without more time in that code. Flagging rather than guessing.

elphamale pushed a commit to elphamale/hermes-agent that referenced this pull request Aug 30, 2026
…DIA delivery

Re-scopes NousResearch#74066 following maintainer guidance on NousResearch#94441: NousResearch#94560 (merged)
fixed the root cause behind the original bug report by making persistent
Docker profile-scoped instead of per-session, which supersedes this PR's
original task_id-based sandbox resolution entirely. That mechanism is
dropped.

What survives is a still-live, independently-confirmed gap: NousResearch#94560's own
_docker_sandbox_dir_candidates() takes session_key explicitly (its own
docstring cites NousResearch#93950 - ambient lookups run after the turn's profile scope
has often already exited) but still resolves its PRIMARY profile candidate
via ambient get_active_profile_name(), not the explicit session_key. Delivery
call sites inherit that same ambient-lookup risk. This adds an explicit
`profile` override (alongside the existing session_key) to
_docker_sandbox_dir_candidates and every function in its call chain, and
threads SessionSource.profile/session_key explicitly through the two
gateway/run.py call sites that had NO identity threading at all
(_deliver_media_from_response's post-stream rescan, and the delegate-task
background-result delivery path) plus cron's own _send_media_via_adapter/
_deliver_result (job id as session_key, matching the PR's original identifier
construction).

All changes are additive (new optional params, existing callers unaffected).
@elphamale
elphamale force-pushed the fix/thread-task-id-media-delivery branch from c258c24 to b6bff38 Compare August 30, 2026 09:33
…DIA delivery

Re-derived against the 2026-09-03/04 simplify(compat) refactor, which
split cron/scheduler.py's delivery helpers into cron/scheduler_delivery.py,
consolidated _default_docker_workspace_host_roots/_docker_persistent_home_host_roots
into gateway/platforms/base.py's shared _docker_persistent_sandbox_roots(),
and split gateway/run.py's post-stream/background media delivery into
gateway/run_notifications.py and gateway/run_turn.py.
@elphamale
elphamale force-pushed the fix/thread-task-id-media-delivery branch from b6bff38 to dd84684 Compare September 6, 2026 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/streaming Streaming responses: gateway delivery, provider wire backend/docker Docker container execution comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists platform/wecom WeCom / WeChat Work adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

3 participants