Skip to content

fix(qqbot): route guild DMs, classify inbound documents, and harden send/connect errors - #35646

Open
lambertian wants to merge 1 commit into
NousResearch:mainfrom
lambertian:fix/qqbot-routing-and-errors
Open

fix(qqbot): route guild DMs, classify inbound documents, and harden send/connect errors#35646
lambertian wants to merge 1 commit into
NousResearch:mainfrom
lambertian:fix/qqbot-routing-and-errors

Conversation

@lambertian

Copy link
Copy Markdown

fix(qqbot): route guild DMs, classify inbound documents, and harden send/connect error handling

Depth over breadth: this fixes 5 independently-verified bugs in the QQ Bot adapter, each with a focused regression test that is red on the current tree and green after the change. The collapsed findings (7 raw items → 5 distinct bugs; two pairs were duplicates) all reproduce against gateway/platforms/qqbot/adapter.py as it stands today.

1. Guild direct-message replies are silently dropped (no dm send branch)

Symptom: A user direct-messages the bot inside a guild; the agent processes the message and generates a reply, but the reply never reaches the user. Media replies hit the wrong endpoint and 404.

Root cause: _handle_dm_message stamps the routing map self._chat_type_map[guild_id] = "dm", but every outbound path resolves the kind via _guess_chat_type() and none understood "dm". _send_chunk only branched on c2c/group/guild, so a guild DM fell into the else returning SendResult(success=False, error="Unknown chat type …"). _send_media only short-circuited chat_type == "guild", so a DM fell through and POSTed the guild_id to /v2/groups/{id}/messages — the wrong endpoint.

Fix: Add a _send_dm_text helper that POSTs to the correct guild-DM endpoint POST /dms/{guild_id}/messages (mirroring the existing _send_guild_text pattern), wire a "dm" branch into _send_chunk, and extend _send_media's guard to short-circuit {"guild", "dm"} so a DM media send never reaches the group endpoint.

2. Inbound file attachments are never classified DOCUMENT

Symptom: A user sends a PDF/spreadsheet/generic file. The agent receives an inline text marker [file: name (<host cache path>)] and the raw host path embedded in the body. Under a sandboxed agent backend that path is in the wrong namespace and the file cannot be opened.

Root cause: Non-image, non-voice attachments were appended only to attachment_info text; they never entered media_urls/media_types, and _detect_message_type could only return VOICE/VIDEO/PHOTO/TEXT — never DOCUMENT. The inbound-document routing in gateway/run.py fires only when event.media_urls is non-empty AND event.message_type == MessageType.DOCUMENT; it then path-translates the host cache path to the agent-visible path and emits the standard "[The user sent a document …]" note. QQ files satisfied neither condition, so that handling never ran.

Fix: In _process_attachments, also append cached file paths to media_urls/media_types (real MIME, falling back to application/octet-stream) while keeping the human-readable marker. The no-attachment early-return dict carries the same two keys, so the text-only path (every handler reads att_result["media_urls"]/["media_types"] unconditionally) does not KeyError. Extend _detect_message_type to return MessageType.DOCUMENT for an application/* or text/* leading type. Videos stay text-only on purpose — the run.py document path only handles application/* and text/*. Images remain first in the media list, so image-led events keep their PHOTO classification.

3. Send retry/permanent-error classification keyed on English keywords the QQ API never returns

Symptom: A permanently-failing send (403 to a user the bot may not message, 404 unknown target) is retried 3× with back-off and then reported retryable=True, so the gateway re-queues a doomed send.

Root cause: _send_chunk decided retry-vs-permanent by substring-matching the lowercased exception against ('invalid','forbidden','not found','bad request'). The QQ Bot v2 REST API returns its error message field in Chinese, so none of those keywords match a genuine 4xx. There was also an internal inconsistency: the break-set included 'bad request' but the final retryable recompute omitted it, so an English bad request 400 could break the loop yet still be flagged retryable=True.

Fix: Classify on the HTTP status _api_request already embeds as [<status>]. New _is_permanent_send_error treats 4xx (except 429) as permanent and 429/5xx/timeouts/transport errors as transient, with a keyword fallback for local (non-HTTP) errors. Both the loop's break decision and the final retryable flag now call the same classifier, so they cannot disagree.

4. _api_request parsed resp.json() before checking status

Symptom: A 4xx/5xx with a non-JSON body (CDN/proxy HTML page, empty 429) raised a raw JSONDecodeError instead of the structured RuntimeError. That broke the daily-quota detection in chunked_upload (which matches the 40093002 biz code in the message) and left the send classifier with a meaningless decode-error string.

Root cause: data = resp.json() ran before the if resp.status_code >= 400 check; only httpx.TimeoutException was caught, so the decode error escaped unwrapped.

Fix: Check the status first. On >= 400, build the message from a guarded body read (resp.json() with a fallback to resp.text[:200]), then raise the structured QQ Bot API error [<status>] …. The success body is parsed only after the status check.

5. Permanent connect() failures marked retryable=True and retried forever

Symptom: A missing aiohttp/httpx install, or a wrong app_id/client_secret, leaves the operator status stuck on "retrying" while the reconnect watcher hammers the token endpoint at the back-off cap indefinitely.

Root cause: The missing-dependency branches set retryable=True (a missing pip package cannot self-heal by retrying), and the broad except Exception bucketed a 401/403 from _ensure_token()/_get_gateway_url() into the retryable qq_connect_error catch-all.

Fix: Set retryable=False on the missing-dependency branches. Add _is_auth_failure, which walks the exception cause chain for an httpx.HTTPStatusError of 401/403 (the token fetch wraps it in a generic RuntimeError via from exc, so the status survives on __cause__). On an auth failure, escalate to a non-retryable qq_auth_failed fatal; genuine network/DNS/5xx/timeout errors stay qq_connect_error/retryable=True. This matches the convention in weixin.py/whatsapp.py/telegram.py.

Overlap

  • fix(gateway): mark WeCom and QQBot missing-credential errors as non-retryable #19891 ("mark WeCom and QQBot missing-credential errors as non-retryable") makes the qq_missing_credentials retryable True→False edit plus an equivalent test. This PR does not touch the qq_missing_credentials line; its connect hardening (fix 5) is a superset that instead covers the missing-dependency branches and escalates 401/403 to qq_auth_failed via _is_auth_failure. The credential hunk overlaps in spirit (same connect() non-retryable direction), but the missing-dependency and auth-failure escalation here are distinct.
  • fix(qqbot): surface cached file attachments to agents #26405 ("surface cached file attachments to agents") partially overlaps fix 2: it also passes the cached file path through the media channel. Built on an older base, it routes every non-image attachment (including video) into the media list and does not touch _detect_message_type, so the event is never classified DOCUMENT and the gateway/run.py path-translation + document note still do not fire. Fix 2 here is the superset that actually triggers that routing: it adds the DOCUMENT classification and scopes videos out (the run.py document path handles only application/* and text/*).
  • fix(qqbot): authorize dm chat_type in interaction approval #32752 / fix(qqbot): authorize approval clicks for dm session keys #31593 touch only _is_authorized_interaction_for_session (the interaction-approval dm chat_type), not the outbound send path; they do not overlap fix 1.
  • fix(qqbot): refactor C2C media routing and fix event loop binding issues #35153 refactors C2C media routing and tools/send_message_tool.py; it does not add a guild-DM send branch, the document classification, or the status-based send/connect classifiers. No functional overlap (a textual merge against its adapter rewrite is possible).

…end/connect errors

Fixes five independently-verified bugs in the QQ Bot adapter, each with a
regression test:

1. Guild direct-message replies were silently dropped: _handle_dm_message
   stamped chat_type "dm", but no send path understood it. _send_chunk hit
   "Unknown chat type" and _send_media POSTed the guild_id to the group
   endpoint. Add _send_dm_text (POST /dms/{guild_id}/messages), a "dm" branch
   in _send_chunk, and extend _send_media's guard to {"guild", "dm"}.

2. Inbound file attachments were never classified DOCUMENT: they only entered
   attachment_info text with the raw host cache path, so the run.py
   inbound-document routing (path-translation + document note) never fired.
   Surface cached file paths via media_urls/media_types and return
   MessageType.DOCUMENT for application/* and text/* leading types. Videos
   stay text-only (run.py's document path handles only application/text). The
   no-attachment early return carries the two keys too, so the text-only path
   does not KeyError.

3. Send retry/permanent-error classification keyed on English keywords the QQ
   API never returns (it replies in Chinese), so permanent 4xx were retried 3x
   and reported retryable=True; the break-set and final flag also disagreed on
   "bad request". Classify on the HTTP status _api_request embeds as
   [<status>] via _is_permanent_send_error, shared by both decisions.

4. _api_request parsed resp.json() before the status check, raising a raw
   JSONDecodeError on non-JSON 4xx/5xx bodies that bypassed the structured
   RuntimeError (and the chunked_upload daily-limit detection). Check status
   first; read the error body defensively (json then text fallback).

5. connect() marked permanent failures retryable=True: a missing aiohttp/httpx
   install and a 401/403 from the token/gateway fetch were retried forever. Set
   the missing-dependency branches retryable=False and escalate 401/403 to a
   non-retryable qq_auth_failed via _is_auth_failure; genuine network errors
   stay qq_connect_error/retryable=True.
@lambertian
lambertian force-pushed the fix/qqbot-routing-and-errors branch from e45c938 to 0de6f51 Compare May 31, 2026 02:56
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have platform/qqbot QQ Bot adapter comp/plugins Plugin system and bundled plugins comp/gateway Gateway runner, session dispatch, delivery labels May 31, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved ✅ — QQBot platform correctness fixes.

Changes

Route guild DMs correctly, classify inbound documents, harden send/connect error handling.


Reviewed by Hermes Agent

@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 the focused QQBot regression coverage. The guild-DM, attachment, API-status, text-send, and connect premises are still present on current main.

Problems

  • The new _is_permanent_send_error helper is only used by text _send_chunk (gateway/platforms/qqbot/adapter.py PR lines 2532-2557). Media URL sends still go through _upload_media; current main lines 2393-2409 retry a structured Chinese-message [403] because the guard only recognizes 400, 401, and selected English words. This leaves a permanent outbound-media failure retried three times.

Suggested changes

  • Reuse the status classifier in _upload_media and add a [403] ... 无权限 media-upload regression test.
  • Preserve current main's connect(*, is_reconnect: bool = False) contract at gateway/platforms/qqbot/adapter.py:281 when salvaging; it was added in 276542c729c10ff9d093760897f4c2d1256a79ce.

Automated hermes-sweeper review.

return False

@staticmethod
def _is_permanent_send_error(exc: BaseException) -> bool:

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.

_is_permanent_send_error is only called from _send_chunk. Please apply it in _upload_media too: its existing retry loop still retries a structured Chinese-message [403] because it only detects 400, 401, and English keywords.

@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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
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 comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/qqbot QQ Bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants