Skip to content

fix(gateway): accept routing kwargs in Weixin media sends - #113645

Open
KoNit-K wants to merge 1 commit into
NousResearch:mainfrom
KoNit-K:fix/weixin-media-kwargs
Open

KoNit-K wants to merge 1 commit into
NousResearch:mainfrom
KoNit-K:fix/weixin-media-kwargs

Conversation

@KoNit-K

@KoNit-K KoNit-K commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Weixin voice and video attachments can now accept the routing kwargs supplied by the base media dispatcher. Previously, WeixinAdapter.send_voice() and send_video() rejected is_voice before their upload path ran, so a MEDIA: audio attachment could be silently omitted while the text reply was delivered. The adapter consumes compatibility-only kwargs and preserves its existing file-upload behavior.

Related Issue

Fixes #113640

Type of Change

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

Changes Made

  • gateway/platforms/weixin.py — added **kwargs compatibility to send_voice() and send_video() without forwarding routing-only values to _send_file().
  • tests/gateway/test_weixin.py — added voice and video regressions that pass metadata plus is_voice, then verify the existing upload arguments remain unchanged.

How to Test

  • HERMES_PYTHON=/Users/blockkonit./Dev/hermes/hermes-agent/.venv/bin/python scripts/run_tests.sh tests/gateway/test_weixin.py -q — 38 passed
  • /Users/blockkonit./Dev/hermes/hermes-agent/.venv/bin/python -m ruff check gateway/platforms/weixin.py tests/gateway/test_weixin.py — passed

Evidence

  • BEFORE RED: scripts/run_tests.sh tests/gateway/test_weixin.py -k media_senders_ignore_routing_kwargs_before_file_upload -q produced 2 failures: both send_voice() and send_video() raised TypeError for is_voice.
  • AFTER GREEN: HERMES_PYTHON=/Users/blockkonit./Dev/hermes/hermes-agent/.venv/bin/python scripts/run_tests.sh tests/gateway/test_weixin.py -q produced 38 passed.
  • CONTROL: the voice case still calls _send_file() with only force_file_attachment=True; the video case still calls it with no extra arguments.

Review follow-up

The Python, scan, Docker, and Nix CI lanes are selected by the changed production Python file. This macOS verification covers the Python test and lint checks; Docker publishing and Windows-only checks are not run locally.

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
  • I've run relevant tests locally (see How to Test)
  • I've added tests for my changes
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • Documentation update: N/A
  • cli-config.yaml.example: N/A
  • CONTRIBUTING.md or AGENTS.md: N/A
  • Cross-platform impact considered
  • Tool descriptions/schemas: N/A

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/wecom WeCom / WeChat Work adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Sep 17, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related: #101381 (open, earlier) fixes the same send_voice() **kwargs gap for #101380; this PR additionally covers send_video(). Reviewers may want to merge one and close the other.

@whyyagswhy

Copy link
Copy Markdown
Contributor

Independent verification on the PR head (d05a16d): tests/gateway/test_weixin.py passes locally, 38/38 (canonical runner). Diff check and Windows-footgun scan clean.

Premise proven on current main, not just trusted: gateway/run_notifications.py:326 calls adapter.send_voice with is_voice=is_voice for every adapter, so Weixin voice notifications TypeErrored before this change. Every sibling adapter (discord, google_chat, feishu, wecom, simplex, line) already accepts routing kwargs; Weixin was the odd one out. Dropping is_voice is behavior-preserving here since Weixin forces file attachment regardless.

No findings.

@kyssta-exe

Copy link
Copy Markdown
Contributor

Summary

Fixes #113640: the base media dispatcher passes routing kwargs (e.g. is_voice) that WeixinAdapter.send_voice/send_video rejected with TypeError, silently dropping MEDIA: audio while text delivered. Adds **kwargs swallow to both methods, preserving existing _send_file behavior.

What changed

  • gateway/platforms/weixin.py: send_video and send_voice signatures gain **kwargs; kwargs are accepted and intentionally not forwarded to _send_file.
  • tests/gateway/test_weixin.py: parametrized compatibility tests pass metadata + is_voice=True to both methods and assert _send_file args unchanged (force_file_attachment=True for voice, {} for video).

Strengths

  • Minimal, correct fix at the right layer — adapter boundary absorbs dispatcher kwargs instead of changing shared dispatch.
  • Tests pin the important invariant: routing kwargs must not leak into the upload call.
  • Concrete before/after evidence (2 TypeError failures → 38 passed).

Findings

  • gateway/platforms/weixin.py:send_voice / send_video: bare **kwargs silently swallows any typo'd kwarg (e.g. captoin=), not just routing keys. Prefer an explicit allowlist — accept is_voice (and known routing keys) by name, or pop documented keys and log unexpected ones.
  • tests/gateway/test_weixin.py: only exercises is_voice=True; add one case with a second routing kwarg if the dispatcher can send others, to lock the swallow set.

Verdict

Needs minor polish.
Reviewed using Hermes-Agent

@KoNit-K
KoNit-K force-pushed the fix/weixin-media-kwargs branch from d05a16d to b1652e3 Compare September 17, 2026 10:44
@KoNit-K

KoNit-K commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current upstream/main (cleanly) and re-ran the focused media-routing coverage. This PR overlaps with earlier open #101381: #101381 carries the live send_voice defect, while #113645 also preserves the base/sibling-adapter **kwargs signature-parity shape and covers the latent send_video gap. The current base adapter contract still uses extensible **kwargs, so this intentionally keeps that shape rather than narrowing it to an allowlist; routing-only kwargs remain unforwarded to the Weixin file-upload helpers. Maintainers can choose the preferred carrier; this PR is intentionally left open.

@KoNit-K

KoNit-K commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up on the review feedback: I checked the rebased current main contract. BasePlatformAdapter.send_voice() and send_video() still expose extensible **kwargs, and sibling adapters retain the same compatibility shape, so I have kept **kwargs here rather than introducing a Weixin-only allowlist. The regression asserts those routing-only values are consumed at the adapter boundary and never reach _send_file / _send_file_result. The current dispatcher supplies is_voice as the only additional routing kwarg for voice; video currently has no additional routing kwarg. That is why the focused regression covers is_voice for both signature-parity methods rather than inventing an unsupported second router value.

@KoNit-K

KoNit-K commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Reproduced the reported failure on current upstream/main @ ebe3fcb2f8: both WeixinAdapter.send_voice(..., is_voice=True) and send_video(..., is_voice=True) raised TypeError before their upload path.

Prepared the minimal current-main repair in isolated worktree temp/pr-113645-current, local commit fbf2a55934:

  • both media senders now consume compatibility/routing-only **kwargs: Any;
  • routing values are deliberately not forwarded to _send_file_result / Weixin upload;
  • a parameterized regression covers voice and video and asserts the pre-existing upload kwargs remain exact.

Evidence:

  • RED: 2 failures, both unexpected is_voice keyword errors.
  • GREEN: routing regression plus voice-upload control → 3 passed.
  • Ruff and git diff --check → clean.

The full tests/gateway/test_weixin.py reached 39 passed but one unrelated QR-login test could not initialize because this shared local environment lacks optional aiohttp; it is not caused by this two-method change. The remote PR has no unresolved review threads and is conflict-free but stale. I did not force-push the local current-main commit.

This branch has not been deployed

No deployments
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 P2 Medium — degraded but workaround exists platform/wecom WeCom / WeChat Work adapter 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.

[Bug] Weixin: send_voice()/send_video() drop **kwargs, audio attachments silently fail

4 participants