Skip to content

fix(qqbot): accept shared-context group session keys in approval auth gate - #55458

Open
sablea wants to merge 2 commits into
NousResearch:mainfrom
sablea:fix/qqbot-approval-shared-group-context
Open

fix(qqbot): accept shared-context group session keys in approval auth gate#55458
sablea wants to merge 2 commits into
NousResearch:mainfrom
sablea:fix/qqbot-approval-shared-group-context

Conversation

@sablea

@sablea sablea commented Jun 30, 2026

Copy link
Copy Markdown

What does this PR do?

Fix a follow-up edge case from #30737: when the gateway runs with
group_sessions_per_user: false (shared group context, all members
share one session), QQ bot's approval-button authorization gate
silently rejects every click — the agent thread then blocks forever
on a dangerous-command approval, with no error surfaced to the user.

The agent writes group session keys without a trailing user_id
suffix in this mode:

agent:main:qqbot:group:<chat_id>

The previous _is_authorized_interaction_for_session implementation
required a user_id suffix for all group/guild keys and dropped the
click. Result: every shared-context QQ group was unable to use
inline-keyboard approvals (3-minute freeze, then fallback to text
slash command, but the slash command itself works on /approve once).

This PR makes the authorization gate adapt to whichever key format
the agent produced: per-user isolation (user_id present) keeps the
existing strict-match contract; shared-context (no user_id) accepts
any operator in the matching chat — since real authorization still
happens at the platform allowlist layer (QQ_ALLOWED_USERS) above
this function.

Related Issue

Closes the follow-up from #30737. Reproduction is identical:
set group_sessions_per_user: false, send a heredoc command in a
group chat, click "Allow once" — nothing happens, log shows
Rejected unauthorized approval click.

Type of Change

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

Changes Made

  • gateway/platforms/qqbot/adapter.py_is_authorized_interaction_for_session:
    in the group/guild branch, accept a key without user_id (the
    shared-context shape). When user_id IS present, behavior is
    unchanged (per-user isolation still requires a strict match).
  • tests/gateway/test_qqbot.py — new TestGroupSharedContextAuthorization
    class with 7 tests covering: group-without-user_id, guild-without-user_id,
    group-with-user_id, group-chat-mismatch, c2c, non-qqbot, empty-operator.
    All 166 tests in test_qqbot.py pass.

How to Test

# 1. Unit tests
pytest tests/gateway/test_qqbot.py::TestGroupSharedContextAuthorization -v

# 2. Full QQ bot test suite (no regressions)
pytest tests/gateway/test_qqbot.py -v

# 3. End-to-end (requires running gateway)
#    a. Set group_sessions_per_user: false in config.yaml
#    b. Restart gateway
#    c. In a QQ group, send: python3 -c "print(1+1)"
#    d. Click "Allow once" on the approval button
#    e. Result: < 5s response with "2"
#    f. Pre-fix: agent blocks 3+ min, then log shows
#       "Rejected unauthorized approval click"

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run pytest tests/gateway/test_qqbot.py -v and all 166 tests pass
  • I've added 7 tests for my changes
  • I've tested on my platform: Ubuntu 24.04 / Python 3.11.15

Documentation & Housekeeping

  • I've updated relevant docstrings on the modified function
  • N/A — no config keys changed
  • N/A — no architecture/workflow changes
  • N/A — the same contract likely exists in other adapters
    (telegram/slack/feishu/matrix/weixin/wecom/whatsapp), but
    that's out of scope here; happy to file follow-up issues
    with the same diagnosis if maintainers want
  • N/A — no tool behavior changed

Screenshots / Logs

Pre-fix log (click silently rejected):

WARNING gateway.platforms.qqbot.adapter: [QQBot:1904122748]
  Rejected unauthorized approval click for session
  agent:main:qqbot:group:7A7AFFA3236085DD9252F7ED98418DC4
  (operator=C280211EF55D1642300850ADC0557A8C)

Post-fix log (click correctly resolved):

INFO gateway.platforms.qqbot.adapter: [QQBot:1904122748]
  Button resolved 1 approval(s) for session
  agent:main:qqbot:group:7A7AFFA3236085DD9252F7ED98418DC4
  (choice=once, operator=C280211EF55D1642300850ADC0557A8C)

End-to-end round-trip latency: < 1 second (tested with heredoc
triggering the approval, user clicks "Allow once", command output
arrives back in the chat within ~600ms).

sablea added 2 commits June 30, 2026 14:20
… gate

When the gateway runs with group_sessions_per_user=false (shared group
context, no per-user isolation), the agent writes group session keys
*without* a trailing user_id suffix:

  agent:main:qqbot:group:<chat_id>

The previous _is_authorized_interaction_for_session implementation
required a user_id suffix for all group/guild chat_type keys and
silently dropped the click — the agent thread then blocked on the
dangerous-command approval forever, with no error surfaced to the user.

This change makes the authorization gate adapt to whichever key format
the agent produced: per-user isolation (user_id present) keeps the
existing strict-match contract; shared-context (no user_id) accepts any
operator in the matching chat, since real authorization still happens
at the platform allowlist layer (QQ_ALLOWED_USERS) above this function.

Fixes the follow-up edge case from NousResearch#30737, which made the session_key
format itself unambiguous but didn't account for the shared-context
shape produced by group_sessions_per_user=false.

Tests: TestGroupSharedContextAuthorization covers all six paths
(group-without-user_id, guild-without-user_id, group-with-user_id,
group-chat-mismatch, c2c, non-qqbot, empty-operator). Full
test_qqbot.py suite still passes (166/166).
…click

End-to-end dispatch tests that exercise _default_interaction_dispatch
with the no-user_id group session_key shape produced by
group_sessions_per_user=false. Complements the unit tests added in the
parent commit with full parse → authorize → resolve flow coverage.

  * test_approval_click_accepts_group_shared_context: any member of
    a shared-context group can resolve approvals.
  * test_approval_click_rejects_group_mismatch_in_shared_context:
    cross-group click attempts are still blocked.

Full test_qqbot.py suite: 170/170 pass via scripts/run_tests.sh.
@sablea

sablea commented Jun 30, 2026

Copy link
Copy Markdown
Author

Updated with the following additions per CONTRIBUTING.md guidance:

  1. Added 2 end-to-end integration tests (test_approval_click_accepts_group_shared_context, test_approval_click_rejects_group_mismatch_in_shared_context) to TestDefaultInteractionDispatch. These exercise the full parse → authorize → resolve_gateway_approval flow with the no-user_id group session_key shape, complementing the unit tests in TestGroupSharedContextAuthorization.

  2. Ran scripts/run_tests.sh (canonical CI test runner): 170/170 pass in 3.8s with hermetic env (TZ=UTC, LANG=C.UTF-8, PYTHONHASHSEED=0, 4 workers).

  3. Ran scripts/check-windows-footguns.py on the diff: ✓ No Windows footguns found. The change is qqbot-only (no POSIX/Windows-relevant code paths), as expected.

  4. Split the work into 2 atomic commits (preserves clean history):

    • 8ca5ecb40 — the fix itself (fix(qqbot): accept shared-context group session keys in approval auth gate)
    • 582547a75 — integration tests (test(qqbot): add integration tests for shared-context group approval click)

No changes to adapter.py since the first push — only test additions + verification artifacts.

@alt-glitch alt-glitch added type/bug Something isn't working platform/qqbot QQ Bot adapter comp/gateway Gateway runner, session dispatch, delivery 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 P2 Medium — degraded but workaround exists labels Jun 30, 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\n\nVerdict: LGTM\n\nFix for QQ bot shared-context group session keys in approval auth gate. When group_sessions_per_user: false, the agent writes session keys without a user_id suffix. The previous implementation required a user_id suffix for all group/guild keys, silently rejecting clicks. The fix correctly adapts to whichever key format the agent produced.\n\n### Looks Good\n- Well-scoped 2-file change with clear root cause\n- Tests added for both per-user and shared-context modes\n- Security preserved: real authorization still happens at the platform allowlist layer\n---\nReviewed by Hermes Agent

@sablea

sablea commented Jun 30, 2026

Copy link
Copy Markdown
Author

👋 Friendly ping — this PR is currently in mergeable_state: "blocked" because the fork-PR CI workflows haven't been approved to run (0 check_runs, 0 statuses). Per repo settings, a maintainer needs to click "Approve and run workflows" on the PR (or enable fork workflow runs in repo settings) for the required CI / all-checks-pass gate to fire.

The fix is small, well-scoped, and ready to merge:

Happy to address any review feedback or push additional commits if needed. If fork CI runs are intentionally disabled here, just say the word and I'll paste the test output inline for a maintainer-side local run.

@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: LGTM

Fixes QQBot group shared-context authorization. When group_sessions_per_user=False, the session key has no user_id suffix -- the old code hard-required it and silently dropped the click. The fix accepts any operator from the matching chat in shared-context mode while preserving the strict per-user match when user_id is present. Comprehensive test coverage (8 tests in TestGroupSharedContextAuthorization).

Verdict: LGTM -- correct authorization fix with thorough tests.

@DavidMetcalfe DavidMetcalfe mentioned this pull request Jul 6, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused QQBot regression fix. The premise remains live on current main: gateway/session.py:952-957 deliberately emits shared group keys without a participant suffix, while gateway/platforms/qqbot/adapter.py:1106-1111 currently rejects every group/guild approval whose key lacks that suffix. The changed branch preserves strict owner matching when the suffix exists and requires a matching chat in both modes.

The added tests cover both the authorization predicate and the dispatch path to resolve_gateway_approval; no blocking issue found. Note that QQ_ALLOWED_USERS is DM-scoped in the current adapter (gateway/platforms/qqbot/adapter.py:3158-3179), while QQ group admission is group-scoped (gateway/platforms/qqbot/adapter.py:3181-3190), so the PR description's allowlist wording is imprecise but not a defect in the implementation.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery 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 sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants