Skip to content

fix(cron): manual runs no longer silently drop media attachments - #88631

Merged
teknium1 merged 1 commit into
NousResearch:mainfrom
victor-kyriazakos:fix/cron-media-delivery-parity
Aug 18, 2026
Merged

fix(cron): manual runs no longer silently drop media attachments#88631
teknium1 merged 1 commit into
NousResearch:mainfrom
victor-kyriazakos:fix/cron-media-delivery-parity

Conversation

@victor-kyriazakos

@victor-kyriazakos victor-kyriazakos commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

hermes cron run <job-id> could deliver a cron job's text output while dropping its PDF/image attachments, with the run still reported as successful. Scheduled runs of the same job delivered both. Reported by an enterprise customer on v0.20.0 whose managed crons deliver reports to Slack DMs; the underlying defects are present on current main.

This PR makes attachment failures visible in the run status on both delivery lanes, and applies the same media path policy in every process, so one-off test runs behave like scheduled runs.

Root cause

Scheduled and manual runs share the same delivery body (run_one_job_deliver_result), and a clean-environment reproduction delivers attachments correctly on both paths. The field failure comes from three related gaps:

  1. The standalone lane discarded upload failures. Platform standalone senders (Slack files_upload_v2, Discord, ...) report per-file upload failures in result['warnings'] while returning success: True for the delivered text leg. _deliver_result read only result['error'], so the run was marked ok, the text arrived, and the attachment disappeared without a trace in the run status.
  2. The live-adapter lane did the same. _send_media_via_adapter logged failed sends at WARNING and returned None, so the caller had nothing to record either.
  3. Media path policy differed by process. gateway.strict, media_delivery_allow_dirs, and trust_recent_files were translated from config.yaml into the environment variables validate_media_delivery_path reads only during gateway startup. A manual run executes in the CLI process, which never runs that translation — in strict or allowlisted deployments it filtered attachment paths under different settings than the gateway process and dropped files a scheduled run would deliver.

On v0.20.0 the failure had an additional layer: the isinstance(resp, dict) gates fixed by 9cf2cbd (shipped in 2026.8.13) meant Slack upload failures were not even detectable in the sender. That commit fixed detection; this PR fixes visibility and policy parity.

Changes

  • _deliver_result surfaces standalone-sender warnings into delivery_errors, which reach the job's last_error and the cron run output, with target context.
  • _send_media_via_adapter returns per-file error strings (failed sends, unavailable loop, policy-dropped paths) and the live-adapter call site records them. A run that delivered text but not its attachment now reports a partial failure on either lane.
  • New gateway/media_policy.apply_media_policy_env() holds the config-to-env translation previously inlined in gateway startup. Gateway boot delegates to it unchanged; _deliver_result applies it before filtering, so all processes filter under the same configured policy. Attachments dropped by the policy filter are reported in the run status with a pointer to the relevant config keys.

No new environment variables, config keys, or tools. The helper moves existing translation; already-set environment values keep precedence, so operator overrides are unaffected.

Validation

Check Result
New suite tests/cron/test_media_delivery_parity.py (8 tests) Written first: 6 fail on unpatched main, 8 pass after
Field-shape reproduction (strict via .env, allowlist via config.yaml) Unpatched drops the attachment as reported; patched delivers it
Full gates: tests/cron, shutdown drain, send_message, media suites 923 passed, 0 failed, 1 skipped
Mutation check (warnings loop disabled, bridge disabled) Exactly the 2 guarding tests fail; restore returns green

Controls: clean delivery still returns no error; media still reaches the sender; both edited files syntax-verified.

Field report (enterprise, v0.20.0): cron jobs delivering text + PDF/image
attachments to Slack DMs deliver both on scheduled ticks but text-only on
manual `hermes cron run <job-id>`. Same box, same token, same scopes —
the divergence is process context and error visibility, not credentials.

Three defects, one bug class (attachment failures invisible + policy
divergence between the gateway process and standalone processes):

1. Standalone lane swallowed warnings: platform standalone senders
   (Slack files_upload_v2, Discord, ...) report per-file upload failures
   in result['warnings'] while returning success=True for the delivered
   text leg. _deliver_result only read result['error'], so the run was
   marked ok and the attachment vanished without a trace. Warnings now
   surface into delivery_errors (and the job's last_error).

2. Live-adapter lane swallowed media failures: _send_media_via_adapter
   logged failures at WARNING and returned None. It now returns per-file
   error strings and _deliver_result records them — text-delivered-but-
   attachment-failed is a visible partial failure on BOTH lanes.

3. Media-policy env bridge was gateway-only: gateway.strict /
   media_delivery_allow_dirs / trust_recent_files were translated from
   config.yaml to the env vars validate_media_delivery_path reads ONLY in
   gateway startup. A CLI-process manual run filtered attachment paths
   under a different policy — in strict/allowlisted deployments the exact
   reported symptom (scheduled delivers, manual drops, silently). The
   translation now lives in gateway/media_policy.apply_media_policy_env
   (idempotent, env-wins, never raises); gateway startup delegates to it
   and _deliver_result applies it before filtering. Attachments dropped
   by the policy filter are also reported in the run status instead of
   only a stderr WARNING.

On v0.20.0 specifically the failure was double-blind: the pre-9cf2cbd382
isinstance(resp, dict) gates meant upload failures were undetectable in
the sender AND unsurfaced by the scheduler. 9cf2cbd (in 2026.8.13)
fixed detection; this fixes visibility and policy parity.

8 new tests (tests/cron/test_media_delivery_parity.py): warnings→errors,
clean-delivery control, media-reaches-sender control, live-adapter
failure/dropped-path reporting, bridge helper semantics, strict+allowlist
end-to-end in a non-gateway process, and the .env-strict/config-allowlist
split that reproduces the field symptom. Mutation check: disabling the
warnings loop and the bridge fails exactly the 2 guarding tests.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 17, 2026
@teknium1
teknium1 merged commit 22f0f22 into NousResearch:main Aug 18, 2026
49 checks passed
teknium1 added a commit that referenced this pull request Aug 18, 2026
Follow-up on the salvaged commits from PRs #87965 and #87967
(@AiwendilInTheWoods):

- Promote the media-send timeout to the standard resolution pattern:
  HERMES_CRON_MEDIA_SEND_TIMEOUT env var, then
  cron.media_send_timeout_seconds in config.yaml, then 300s default
  (mirrors script_timeout_seconds; .env stays secrets-only).
- Register the config key in DEFAULT_CONFIG and document both surfaces
  (environment-variables reference + cron user guide).
- Fold the empty-str() exception fallback into the error string recorded
  in delivery_errors (post-#88631 the reason reaches the run status, not
  just the log line).
- Tests: timeout resolution precedence + TimeoutError reason fallback.
lisajlau pushed a commit to lisajlau/hermes-agent that referenced this pull request Aug 20, 2026
Follow-up on the salvaged commits from PRs NousResearch#87965 and NousResearch#87967
(@AiwendilInTheWoods):

- Promote the media-send timeout to the standard resolution pattern:
  HERMES_CRON_MEDIA_SEND_TIMEOUT env var, then
  cron.media_send_timeout_seconds in config.yaml, then 300s default
  (mirrors script_timeout_seconds; .env stays secrets-only).
- Register the config key in DEFAULT_CONFIG and document both surfaces
  (environment-variables reference + cron user guide).
- Fold the empty-str() exception fallback into the error string recorded
  in delivery_errors (post-NousResearch#88631 the reason reaches the run status, not
  just the log line).
- Tests: timeout resolution precedence + TimeoutError reason fallback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants