fix(send_message): add WeCom callback platform support - #23653
mehmetkr-31 wants to merge 1 commit into
Conversation
2b97548 to
9ee46de
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the standalone-delivery gap. The issue still exists on current main, but this implementation needs rework for the plugin migration.
Problems
tools/send_message_tool.py:1795importsgateway.platforms.wecom_callback, but commitf2a7adba5410cc1799e56fa77b47f84fcd9a2936moved that adapter toplugins/platforms/wecom/callback_adapter.py; the helper would return an error on current main.tools/send_message_tool.py:1799callsWecomCallbackAdapter.connect(). Currentconnect()rejects an occupied callback port and starts the callback HTTP server (plugins/platforms/wecom/callback_adapter.py:126-150), so this cannot coexist with a running gateway callback listener.- The current registry fallback is the intended integration point:
tools/send_message_tool.py:743-767invokesstandalone_sender_fn, but thewecom_callbackregistration atplugins/platforms/wecom/adapter.py:1875-1888does not supply one.
Suggested changes
- Add a callback-specific standalone sender through the platform registry that performs outbound delivery without binding the callback listener port.
- Add a no-live-runner regression test for
wecom_callbackdelivery.
Automated hermes-sweeper review.
| async def _send_wecom_callback(extra, chat_id, message): | ||
| """Send via WeCom Callback (webhook) using the adapter's send pipeline.""" | ||
| try: | ||
| from gateway.platforms.wecom_callback import WecomCallbackAdapter |
There was a problem hiding this comment.
Blocking: this module was moved in current main by f2a7adba5410cc1799e56fa77b47f84fcd9a2936 to plugins.platforms/wecom/callback_adapter.py. This import will fail and the broad exception below turns every callback send into an error.
| from gateway.config import PlatformConfig | ||
| pconfig = PlatformConfig(extra=extra) | ||
| adapter = WecomCallbackAdapter(pconfig) | ||
| connected = await adapter.connect() |
There was a problem hiding this comment.
Blocking: WecomCallbackAdapter.connect() is listener startup, not an outbound-only setup. It rejects an occupied callback port before creating its HTTP client (plugins/platforms/wecom/callback_adapter.py:126-149), so cron/CLI delivery cannot coexist with the gateway's callback listener. Please implement this as a registry standalone sender that does not bind the listener port.
9ee46de to
1b4d2b5
Compare
|
Reworked per the sweeper review — the direct-adapter approach is gone; the branch was rebuilt on current main using the intended registry integration point:
|
…ousResearch#23653) `send_message(platform="wecom_callback")` and `deliver=wecom_callback` cron jobs route through the registry's standalone_sender_fn when they run outside the gateway. `wecom` registers one; `wecom_callback` registered none, so those sends had nothing to call and failed. The sender deliberately does NOT go through connect(). Callback delivery is outbound-only — the aiohttp app, the bound port and the poll loop exist purely to *receive* callbacks — and connect() refuses outright when the port is already held, which is exactly the situation an out-of-process send runs in. The ephemeral connect/disconnect pattern the WebSocket-based `wecom` sender uses is therefore wrong here. To reuse the real send path without any of that, the client setup inside connect() is extracted to _ensure_http_client() (plus aclose_http_client()), so both connect() and the standalone sender open the same outbound client. send() itself is untouched, keeping its token-refresh retry and app resolution. Rebuilt on current main rather than rebased — the branch was ~4200 commits behind and its direct-adapter approach imported gateway.platforms.wecom_callback, a module that no longer exists after the plugin migration. The scripts/release.py addition to the frozen LEGACY_AUTHOR_MAP is dropped; contributors/emails/mehmet.kar@std.yildiz.edu.tr already exists on main. No cron_deliver_env_var is claimed: no WECOM_CALLBACK_* home-channel variable exists anywhere in the tree, and inventing one is how the sibling status PR got its env names wrong. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1b4d2b5 to
d873b9e
Compare
|
Rebuilt on current Premise re-verified. In Why the sender can't reuse the The seam. Tests —
Verified both guarantees rather than assuming: dropping the registration fails the first test, and swapping One thing I deliberately did not add: a Attribution hunk dropped — Verification note: |
|
Correcting the verification numbers in my previous comment — they were measured before I noticed my local environment was missing two test dependencies, so they understated things.
Same two pre-existing failures ( The two behavioural checks I described still hold: dropping the registration fails |
|
CI on this branch is red, but not from this diff — worth flagging because it is now hitting more than one PR. The only failure is It is non-deterministic. The failing assertion is the test's first one — I spent a little time trying to pin the mechanism so I could offer a fix rather than just a complaint, and I want to be straight that I could not:
So the obvious explanation is wrong and I don't have a confirmed one. I'd rather say that than push a speculative fix to someone else's test. Happy to rebase this branch onto a main that carries a fix once one lands. The three-file diff itself is unaffected, and the WeCom tests it adds pass ( |
|
CI note: the single red job is not this PR. The only failing test on the whole run is |
|
Closing this myself to clear stale work, not because anything here was reviewed and found wanting. Measuring this repo's merge behaviour: across four separate weekly samples, 157 externally-authored PRs were merged and not one took longer than 24 hours — median ~20 minutes. A PR that has been open for a month is not queued behind anything; the decision window closed long ago, and leaving it open just adds noise to a list maintainers scan. The branch and its commits are untouched, so if any of this is still wanted, say so and I will rebase it onto current main and reopen rather than have you dig it out of a stale diff. |
Adds a missing elif branch and _send_wecom_callback() helper so cron jobs and CLI can deliver messages to WeCom callback (webhook) platforms. Previously, Platform.WECOM_CALLBACK fell through to the generic plugin adapter path and always failed with a missing-platform error.