Skip to content

refactor(gateway): add adapter reply delivery policy - #40931

Open
jethac wants to merge 6 commits into
NousResearch:mainfrom
jethac:refactor/gateway-reply-delivery-policy
Open

refactor(gateway): add adapter reply delivery policy#40931
jethac wants to merge 6 commits into
NousResearch:mainfrom
jethac:refactor/gateway-reply-delivery-policy

Conversation

@jethac

@jethac jethac commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a generic ReplyDeliveryPolicy hook for platform adapters.
  • Lets adapters observe inbound messages before gateway dispatch.
  • Routes the gateway's auto voice-reply decision through the adapter policy while preserving default behavior.

Why this matters

  • Keeps platform-specific delivery decisions inside each adapter instead of growing more platform branches in the shared gateway runner.
  • Gives adapters a narrow, testable ownership point for reply behavior that depends on inbound platform context.
  • Reduces future merge-conflict risk by letting LINE, Telegram, Discord, and other adapters evolve delivery policy without repeatedly editing the same central voice/text decision path.
  • Preserves the current gateway default for adapters that do not opt in, so this is an extension seam rather than a behavior change.

Stack

Test Plan

  • python -m py_compile gateway/run.py gateway/platforms/base.py plugins/platforms/line/adapter.py
  • python -m pytest tests/gateway/test_reply_delivery_policy.py tests/gateway/test_line_plugin.py tests/gateway/test_telegram_audio_vs_voice.py -q -o 'addopts='
  • git diff --check line-media-adapter-pr35785-rebased...refactor/gateway-reply-delivery-policy

@liuhao1024

Copy link
Copy Markdown
Contributor

cache_image_from_url regression: image validation lost

The refactoring from cache_image_from_urlcache_media_from_url drops the _looks_like_image() validation that previously guarded the download-and-cache path.

Before (current main):

async def cache_image_from_url(url, ext=".jpg", retries=2):
    # ...
    return cache_image_from_bytes(response.content, ext)
    #   ^ validates via _looks_like_image(), raises ValueError for non-image data

After (this PR):

async def cache_image_from_url(url, ext=".jpg", retries=2):
    """Deprecated alias for cache_media_from_url"""
    return await cache_media_from_url(url, ext=ext, retries=retries)

async def cache_media_from_url(url, ext=".bin", retries=2):
    # ...
    return cache_media_from_bytes(response.content, ext)
    #   ^ is_image defaults to False — no image validation, cache dir becomes cache/media/

The Accept header also changes from image/*,*/*;q=0.8 to */*, so non-image responses are now silently cached instead of rejected.

Callers affected (at least):

  • plugins/platforms/discord/adapter.py:4648await cache_image_from_url(att.url, ext=ext)
  • plugins/platforms/teams/adapter.py:790await cache_image_from_url(content_url)

Suggested fix — add is_image parameter to cache_media_from_url so cache_image_from_url can preserve validation:

async def cache_media_from_url(url, ext=".bin", retries=2, *, is_image=False):
    # ...
    return cache_media_from_bytes(response.content, ext, is_image=is_image)

async def cache_image_from_url(url, ext=".jpg", retries=2):
    return await cache_media_from_url(url, ext=ext, retries=retries, is_image=True)

@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have labels Jun 7, 2026
@jethac
jethac force-pushed the refactor/gateway-reply-delivery-policy branch from e149f06 to 9dedf3c Compare June 7, 2026 02:55
@jethac
jethac force-pushed the refactor/gateway-reply-delivery-policy branch from 9dedf3c to 9931c84 Compare June 23, 2026 10:22
@jethac

jethac commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Rebased this PR onto the refreshed LINE media branch (#35785) and fixed the voice-reply regressions from the previous revision.

Additional cleanup in this update:

  • Preserves legacy empty/error response gates before adapter reply policy can request TTS.
  • Ignores invalid/non-ReplyDeliveryPolicy adapter returns and falls back to legacy behavior.
  • Keeps existing streaming/fresh-final hooks while adding the adapter policy seam.

Local verification:

uv run --with pytest --with pytest-asyncio --with aiohttp python -m pytest \
  tests/gateway/test_voice_command.py \
  tests/gateway/test_reply_delivery_policy.py \
  tests/gateway/test_media_download_retry.py \
  tests/gateway/test_line_plugin.py \
  -q -o 'addopts='
# 283 passed, 21 skipped

@jethac
jethac force-pushed the refactor/gateway-reply-delivery-policy branch 2 times, most recently from b0121e3 to 498fa50 Compare June 23, 2026 10:39
@jethac
jethac force-pushed the refactor/gateway-reply-delivery-policy branch 7 times, most recently from 1b83acb to 14b775e Compare June 28, 2026 13:46
@jethac
jethac force-pushed the refactor/gateway-reply-delivery-policy branch from 14b775e to 913cb33 Compare July 2, 2026 06:16
@jethac

jethac commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@liuhao1024 thanks again for catching the cache_image_from_url regression. This is actioned in the current branch head.\n\nThe refactor now preserves the image-specific path by threading an is_image flag through cache_media_from_url(...) / cache_media_from_bytes(...), so cache_image_from_url(...) calls cache_media_from_url(..., is_image=True). That keeps:\n\n- _looks_like_image() validation via cache_image_from_bytes(...)\n- image cache routing\n- the stricter Accept: image/*,*/*;q=0.8 header\n\nI also added regression coverage for rejecting non-image response bodies and preserving the image Accept header. Verified locally with the targeted tests passing.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for extracting a platform delivery seam; the stated LINE follow-up gives it a concrete consumer. Two changes are needed when salvaging onto current main.

Problems

  • gateway/run.py:12331 uses self.adapters.get(event.source.platform). Current main commit 8a9bc38c documents that this selects the default profile's adapter in multiplex mode; use _adapter_for_source(event.source) so secondary-profile delivery policy is evaluated by the correct adapter.
  • gateway/run.py:12339 calls the new adapter policy without a fallback on exception. The adjacent new observe_inbound_message path deliberately catches callback failures (gateway/run.py:12314-12321); policy failures should likewise preserve the legacy reply path rather than disrupt final delivery.

Suggested changes

  • Resolve via _adapter_for_source(event.source) and add a multiplex-source regression.
  • Catch policy callback exceptions, fall back to the legacy policy, and test that behavior.

Automated hermes-sweeper review.

Comment thread gateway/run.py Outdated

):
"""Return the adapter's reply delivery policy for this turn."""
adapter = self.adapters.get(event.source.platform)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Current main's multiplex fix 8a9bc38c establishes that self.adapters is the default-profile map. Resolve this through _adapter_for_source(event.source) so a secondary profile evaluates its own adapter's policy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1646488 — both lookups this PR introduced (_observe_inbound_message and _reply_delivery_policy) now resolve via self._adapter_for_source(event.source). Two multiplex regressions in tests/gateway/test_reply_delivery_policy.py prove a source stamped for a secondary profile consults that profile's adapter's policy, and the default-profile adapter is never called.

Comment thread gateway/run.py Outdated
return ReplyDeliveryPolicy()

if adapter and hasattr(adapter, "reply_delivery_policy"):
policy = adapter.reply_delivery_policy(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please catch policy-callback exceptions and fall back to the legacy policy. The adjacent inbound-observer callback is already isolated; an adapter policy failure should not interrupt final-reply delivery.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e1a9252 — the reply_delivery_policy callback is now wrapped in try/except mirroring the inbound-observer isolation pattern: on exception it logs a warning (exc_info=True) and falls back to the legacy voice-mode gate, so a policy failure can never disrupt final-reply delivery. Covered by test_policy_callback_failure_falls_back_to_legacy_delivery (raising policy → legacy gate still honored in both the off and /voice all states).

@teknium1 teknium1 added 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 sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 14, 2026
@jethac
jethac force-pushed the refactor/gateway-reply-delivery-policy branch from 913cb33 to e1a9252 Compare July 19, 2026 01:19
@jethac

jethac commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review — both points fixed; specifics in the inline replies (adapter resolution via _adapter_for_source with multiplex regressions in 1646488; exception-isolated policy callback with legacy fallback in e1a9252). The branch is also rebased onto current main and restacked on #35785's re-scoped media fix, so this diff no longer carries the old generic cache API. CI green, mergeable.

@jethac
jethac force-pushed the refactor/gateway-reply-delivery-policy branch 4 times, most recently from 55bceff to f72e34a Compare August 1, 2026 01:28
jethac and others added 6 commits August 11, 2026 22:06
Rebase note: upstream 73e193c ("fix(line): normalize inbound media
types and cache routing") independently shipped a superset of this
branch's adapter fix — typed cache helper dispatch, fileName threading,
and a (path, media_type) return from _download_media. The adapter
changes are therefore dropped in favor of upstream's version.

What remains from the original commit: the mocked regression tests
covering _download_media routing for all four LINE content types,
cache/fetch failure fallbacks, and fileName threading from the message
event — adapted to upstream's keyword-only ``filename=`` parameter and
tuple return, and to media_types now carrying MIME types.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Map the contributor email used on the LINE media PR stack to the jethac
GitHub handle, replacing the stale legacy-map entry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rebase note (onto f75b577): upstream's voice.auto_tts sync
(_should_auto_tts_for_chat, "voice accompanies text replies unless the
chat explicitly set off", with unset voice_mode None distinct from
"off") and the NousResearch#60671 streaming-TTS skip landed in these same paths
since this branch's base. Adapted the graft to preserve both: the base
adapter's default reply_delivery_policy and the runner's legacy
fallback now include the auto_tts term, _reply_delivery_policy passes
voice_mode through raw (None when unset) instead of defaulting to
"off", and the runner call site keeps upstream's streaming-TTS guard
while threading _voice_reply_sent/suppress-text through it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

[rebase note 2026-08-01: re-resolved against upstream e444d16 — adopted upstream's tightened voice.auto_tts precedence (fallback only when chat has no explicit voice mode, `voice_mode is None`, upstream changed from `!= "off"`) in both the base adapter's default reply_delivery_policy and the runner's legacy fallback]
self.adapters is the default profile's adapter map. Resolve the
reply-delivery-policy and inbound-observe adapter through
_adapter_for_source(event.source) so multiplex secondary profiles
consult their own adapter's policy (aligns with 8a9bc38).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Catch exceptions from the adapter reply_delivery_policy callback and
fall back to the legacy voice-mode gate so a buggy adapter policy can
never disrupt final reply delivery. Mirrors the isolation pattern used
by _observe_inbound_message.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jethac
jethac force-pushed the refactor/gateway-reply-delivery-policy branch from f72e34a to 795998e Compare August 12, 2026 05:22
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 P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit 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/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants