Repository navigation
fix(weixin): exempt slash commands from content-fingerprint dedup - #36707
JerryLiu369 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds command-specific, time-windowed content de-duplication for Weixin slash commands to avoid dropping repeated commands outside a short window while still de-duping rapid repeats.
Changes:
- Introduces
CMD_DEDUP_WINDOW_SECONDSand uses a time-bucketed de-dup key for texts starting with/. - Keeps existing content de-dup behavior for non-command text messages.
- Adds tests validating command de-dup keying, within-window dropping, and independence from regular content de-dup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/gateway/test_weixin.py | Adds regression tests for new slash-command de-dup behavior and keying. |
| gateway/platforms/weixin.py | Implements time-bucketed de-dup keys for slash commands and adds new constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| content_calls = [ | ||
| str(call) for call in adapter._dedup.is_duplicate.call_args_list | ||
| if "cmd:" in str(call) | ||
| ] |
| async def _drive(): | ||
| await adapter._process_message({**base_msg, "message_id": "cmd-1"}) | ||
| await adapter._process_message({**base_msg, "message_id": "cmd-2"}) | ||
| await asyncio.sleep(0.2) | ||
|
|
||
| asyncio.run(_drive()) |
| text_hash = hashlib.md5(text.encode()).hexdigest() | ||
| if text.startswith("/"): | ||
| time_bucket = int(time.time()) // CMD_DEDUP_WINDOW_SECONDS | ||
| content_key = f"cmd:{sender_id}:{text_hash}:{time_bucket}" |
mxnstrexgl
left a comment
There was a problem hiding this comment.
🤖 Automated PR Review
Security Scan
- ✓ No hardcoded secrets, injection sinks, unsafe deserialization, or dependency red flags found by this automated scan.
Code Quality
- ✓ No blocking code-quality issues found by this automated scan.
Summary
Status: APPROVE — security findings: 0, quality suggestions: 0.
Automated review; raw diff content intentionally omitted.
|
Hi @teknium1! 👋 This is a small fix for a real pain point when using Hermes via WeChat — slash commands like The change is minimal (+7/-1 in
Tests cover: windowed dedup for commands, within-window dedup, and independence from regular content dedup. Could you take a quick look when you have a moment? Thanks! 🙏 |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the Weixin command-dedup path. The premise remains valid on current main: gateway/platforms/weixin.py:1415-1422 sends every non-empty text message through a content key, while MessageDeduplicator retains that key for the adapter's 300-second TTL (gateway/platforms/helpers.py:43-58; weixin.py:1160). This can block repeat /approve messages even though the approval handler resolves only the oldest pending approval (gateway/slash_commands.py:4348-4354).
Problems
gateway/platforms/weixin.py:1391uses an epoch-alignedint(time.time()) // 3key. A duplicate sent immediately across a three-second boundary gets a different key and can execute twice; this is not a sliding three-second dedup window.- The new tests do not control time or verify either a boundary crossing or acceptance after expiry.
Suggested changes
- Use a command-only three-second
MessageDeduplicator(or equivalent sliding expiry) for sender/content command keys, leaving the existing 300-second text dedup unchanged. - Add mocked-clock coverage for suppression below three seconds, including a bucket-boundary case, and acceptance at expiry.
Automated hermes-sweeper review.
| text = _extract_text(item_list) | ||
| if text: | ||
| content_key = f"content:{sender_id}:{hashlib.md5(text.encode()).hexdigest()}" | ||
| text_hash = hashlib.md5(text.encode()).hexdigest() |
There was a problem hiding this comment.
// partitions by fixed epoch boundaries rather than elapsed time since the first command. A duplicate at 2.99s and 3.01s uses different keys and can execute twice. Please use a command-only TTL deduplicator (or equivalent sliding expiry) and add mocked-clock boundary coverage.
8030fd1 to
b6f038f
Compare
|
Thanks @teknium1. I reworked this around the delivery invariant rather than changing approval lookup semantics.
This is in b6f038fc5, based on current main. I also traced the approval path and kept approval queue resolution exact-keyed; the prefix fallback from #36706 is not carried into this PR. |
|
@teknium1 Could you take a final look at this small Weixin delivery fix when you have a moment? I re-verified it against current main ( The current patch at
Validation:
This should be safe to merge independently of the obsolete approval-key fallback in #36706. Thank you. |
b6f038f to
14ed9e0
Compare
|
Hi @teknium1 👋 I have rebased this PR onto current main ( What changed:
Current state:
Could you take a final look when you get a chance? Thanks 🙏 |
14ed9e0 to
e3a3312
Compare
|
Updated to a simpler approach: just exempt slash commands from content-fingerprint dedup entirely ( The 3-second sliding window from the previous commit was over-engineered — message-ID dedup already handles iLink retries, and the only practical scenario it blocked was the exact one we want to permit (intentional retries of the same command). This is now a +1/-1 production change with targeted regression tests. The approach matches what #29787, #29855, #35434, and earlier iterations of this PR all targeted — those were closed for unrelated bundled changes, not for the fix strategy itself. cc @teknium1 |
e3a3312 to
0ee57f4
Compare
0ee57f4 to
87a7b9a
Compare
87a7b9a to
f03e54c
Compare
|
Rebased onto latest main ( Validation: |
f03e54c to
c1af60d
Compare
Simpler approach: if text starts with "/", skip the content-fingerprint dedup entirely. Message-ID dedup (300s TTL) still handles iLink network retries, so the 3-second sliding window for commands is unnecessary. This is the approach that NousResearch#29787, NousResearch#29855, NousResearch#35434, and NousResearch#36707 all targeted — the earlier PRs were closed for bundled unrelated changes, not for the fix strategy itself. Tests: - test_slash_commands_bypass_content_dedup: two /approve with different message IDs → both reach the handler - test_slash_command_msg_id_dedup_still_applies: same message_id → deduped by message_id cache (not affected by this change) - Existing test_duplicate_content_with_different_message_ids_is_dropped still passes (regular text dedup unchanged)
c1af60d to
bfa2165
Compare
Simpler approach: if text starts with "/", skip the content-fingerprint dedup entirely. Message-ID dedup (300s TTL) still handles iLink network retries, so the 3-second sliding window for commands is unnecessary. This is the approach that NousResearch#29787, NousResearch#29855, NousResearch#35434, and NousResearch#36707 all targeted — the earlier PRs were closed for bundled unrelated changes, not for the fix strategy itself. Tests: - test_slash_commands_bypass_content_dedup: two /approve with different message IDs → both reach the handler - test_slash_command_msg_id_dedup_still_applies: same message_id → deduped by message_id cache (not affected by this change) - Existing test_duplicate_content_with_different_message_ids_is_dropped still passes (regular text dedup unchanged)
bfa2165 to
1b28051
Compare
|
Closing for now. Thanks! |
Problem
When a user sends the same slash command twice (e.g.
/approve,/deny,/retry) via Weixin, the second message is silently dropped by the content-fingerprint deduplication logic atgateway/platforms/weixin.py:1449-1456. Slash commands are intentionally repeatable — the user may need to send/approveagain or/retryto re-run the last turn.Fix
Skip content-fingerprint dedup for messages starting with
/:Message-ID dedup (300s TTL) still handles iLink network retries, so no additional command-specific dedup mechanism is needed.
Tests
test_slash_commands_bypass_content_dedup— two/approvewith different message IDs → both reach the handlertest_slash_command_msg_id_dedup_still_applies— same message ID → deduped by message_id cachetest_duplicate_content_with_different_message_ids_is_droppedstill passes (regular text dedup unchanged)Verification