fix(gateway): add canonical cache paths to MEDIA_DELIVERY_SAFE_ROOTS#31764
fix(gateway): add canonical cache paths to MEDIA_DELIVERY_SAFE_ROOTS#31764tvy14 wants to merge 1 commit into
Conversation
image_generate saves generated images under ~/.hermes/cache/images/, but MEDIA delivery on Telegram only validated paths under the legacy ~/.hermes/image_cache/ and the IMAGE_CACHE_DIR variable (which resolves to the legacy path when it exists on disk). Add the canonical new paths explicitly so generated images are deliverable regardless of which cache layout the user has. Fixes NousResearch#31733
|
I hit this same bug today on a Linux Telegram DM flow, and the fix direction here looks right. Additional repro context from #31733:
So explicitly adding the canonical One thing I would tighten before merge: the tests added here mostly monkeypatch A stronger regression test would assert the default/media-root construction includes the canonical cache dirs, or simulate the actual failure shape:
That distinction matters because the original failure is caused by Also worth rebasing this against current Net: this is the right fix direction, but I would strengthen the regression tests and resolve the conflicts before merge. |
|
Quick follow-up: I have opened #33251 as a complementary draft PR covering the same What #33251 adds on top of the canonical-roots change:
Happy to fold whatever subset of #33251 is useful into this PR (or close #33251 in favour of this one) — whichever path you and the maintainers prefer. If #31764 lands first, the safe-roots block in #33251 is a clean merge against it. |
…sResearch#31733) Builds on the direction in NousResearch#31764 with a broader cut at the same bug surface that surfaces in operator reports as "MEDIA tag did not attach": * MEDIA_DELIVERY_SAFE_ROOTS now lists the canonical ~/.hermes/cache/{images,audio,videos,documents,screenshots} explicitly alongside the legacy ~/.hermes/{type}_cache/ entries. get_hermes_dir returns the legacy path when it exists, so IMAGE_CACHE_DIR alone drops the canonical layout on co-existing installs (root cause of NousResearch#31733). * validate_media_delivery_path now delegates to a private _validate_media_delivery_path_with_reason helper that returns (resolved_or_None, kebab_reason). filter_media_delivery_paths and filter_local_delivery_paths log the reason plus a redacted path (_redact_path_for_log), so operators chasing rejected MEDIA tags can tell allowlist-miss from stale-mtime from denied-prefix from does-not-resolve without rebuilding the bot with debug logging. * _redact_path_for_log neutralises control characters (newline, NUL, carriage return) so a model-emitted path containing an embedded newline cannot forge a second log line. * Hoisted _FINAL_RESPONSE_IMAGE_EXTS / _FINAL_RESPONSE_VIDEO_EXTS to module scope so the regression test imports the same partition the final-response dispatch actually uses, instead of snapshotting a local copy that could drift. * Regression coverage added in tests/gateway/test_platform_base.py: - default safe-roots tuple contains both legacy and canonical layouts - legacy and canonical image caches co-exist (the strengthened failure-shape test NousResearch#31764 review asked for) - PDF deliverable from canonical ~/.hermes/cache/documents and legacy ~/.hermes/document_cache - filter_*_delivery_paths logs the redacted path and the kebab reason tag (using the imported _MEDIA_REASON_* constants, not raw strings) for stale-mtime, no-recency, does-not-resolve, denied-prefix, and newline-injection cases - validate_media_delivery_path delegation invariant: public API matches the resolved path returned by the reason-bearing helper - send_message tool and final-response paths share extract_media + filter_media_delivery_paths (asserted via source introspection so a future split surfaces here) - PDF MEDIA tag routes to send_document via the production extension partition (no image / video misroute) Scope: gateway media delivery only. Does not duplicate NousResearch#32472's extension-derivation refactor or NousResearch#31561 / NousResearch#33206's markdown attachment recognition work; if any of those land first, the diagnosable-logging contract here still applies and the canonical-root entries become a clean merge against NousResearch#31764. Deferred follow-up (out of scope for this PR): - Surfacing the rejection reason to the calling agent (send_message tool currently returns {success: true} regardless of dropped attachments) - Unifying the three dispatch-site extension sets across _process_message_background, _deliver_media_from_response, and the send_message tool path - Pre-existing residual: symlink-swap of a cache root before image_generate writes; TOCTOU between resolve() and upload; /var/tmp / /var/cache not in denylist
Summary
image_generatesaves generated images under~/.hermes/cache/images/, but the gateway'sMEDIA_DELIVERY_SAFE_ROOTSonly validated paths under the legacy~/.hermes/image_cache/and theIMAGE_CACHE_DIRvariable — which resolves to the legacy path when it exists on disk, creating a mismatch.This adds the canonical new paths (
cache/images,cache/audio,cache/videos,cache/documents,cache/screenshots) alongside the existing legacy paths so generated media is deliverable regardless of cache layout.Changes
gateway/platforms/base.py: Added 5 canonical paths toMEDIA_DELIVERY_SAFE_ROOTStests/gateway/test_platform_base.py: Added 3 regression testsTesting
Fixes #31733