Skip to content

fix(qqbot): authorize approval buttons for dm chat type - #43957

Open
LeDaVinci wants to merge 2 commits into
NousResearch:mainfrom
LeDaVinci:fix/qqbot-dm-approval-auth
Open

fix(qqbot): authorize approval buttons for dm chat type#43957
LeDaVinci wants to merge 2 commits into
NousResearch:mainfrom
LeDaVinci:fix/qqbot-dm-approval-auth

Conversation

@LeDaVinci

Copy link
Copy Markdown

Problem

Approval button clicks in QQ DM channels are always rejected with:

[QQBot] Rejected unauthorized approval click for session agent:main:qqbot:dm:<user_id> (operator=<user_id>)

even when the operator and session owner are the same person.

Root Cause

The QQBot adapter uses dm as the chat_type for direct messages (both guild-based DMs and direct chats — see lines 1288 and 1498 in adapter.py). However, _is_authorized_interaction_for_session only checks for c2c:

if chat_type == "c2c":
    return bool(chat_id) and operator == chat_id

Since dm doesn't match c2c or group/guild, it falls through to return False, blocking all approval interactions.

Fix

Add dm to the same authorization branch as c2c — both are 1-on-1 contexts where operator == chat_id is the correct check:

if chat_type in {"c2c", "dm"}:
    return bool(chat_id) and operator == chat_id

Testing

Verified locally: approval buttons in QQ DM now correctly authorize and unblock the agent's pending command.

…#43926)

The QQBot adapter uses "dm" as the chat_type for direct messages
(guild-based DMs and direct chats), but _is_authorized_interaction_for_session
only checked for "c2c", causing all approval button clicks in DM contexts
to be rejected as unauthorized even when the operator matched the session owner.

Add "dm" to the same authorization branch as "c2c" since both are
1-on-1 contexts where operator == chat_id is the correct check.
@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/qqbot QQ Bot adapter duplicate This issue or pull request already exists labels Jun 11, 2026
@alt-glitch

alt-glitch commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #32528 and open #31593: the DM authorization hunk overlaps #31593, but this branch also includes an independently useful QQBot reconnect/retrying-state change. It is a mixed bundle, not a duplicate; please split or rebase it for review.

@LeDaVinci

Copy link
Copy Markdown
Author

Added a follow-up commit for QQBot transport recovery: transient DNS / websocket / token failures now leave QQBot in retrying state and keep the reconnect loop alive instead of hard-stopping at the reconnect limit. Also added a regression test in tests/gateway/test_qqbot.py.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the direct-C2C session-key mismatch: it remains present on current main. gateway/platforms/qqbot/adapter.py:1291-1296 creates a dm session source, while :1103-1113 only authorizes c2c, so the authorization hunk addresses a real bug.

Problems

  • The follow-up reconnect changes remove both MAX_RECONNECT_ATTEMPTS exits. Commit ec7e92082 added those bounds specifically to avoid permanent failures retrying forever; the new test only asserts platform_state="retrying" and does not cover the loop.
  • The PR adds no regression test for the reported dm approval flow. Existing approval-dispatch tests use c2c keys (tests/gateway/test_qqbot.py:1601-1659); the added test is transport-status-only.
  • The proposed predicate does not cover the stated guild-DM case: guild-DM sources use guild_id as chat_id and author id as user_id (adapter.py:1504-1506), while the new condition compares the operator to chat_id.

Suggested changes

  • Keep the verified direct-C2C authorization fix, add matching/mismatching dm approval-dispatch tests, and split or substantiate the reconnect-policy change.
  • Scope the claim to C2C or implement and test guild-DM authorization separately.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@alt-glitch alt-glitch added needs-decision Awaiting maintainer decision before any implementation and removed duplicate This issue or pull request already exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #31593 and #32528. This includes the DM authorization fix but separately adds QQBot reconnect recovery, so it is a broader competing change rather than a duplicate.

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 needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists platform/qqbot QQ Bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

3 participants