Skip to content

fix(qqbot): accept is_reconnect kwarg in QQAdapter.connect - #58128

Closed
Timeral wants to merge 1 commit into
NousResearch:mainfrom
Timeral:fix/qqbot-accept-is-reconnect
Closed

fix(qqbot): accept is_reconnect kwarg in QQAdapter.connect#58128
Timeral wants to merge 1 commit into
NousResearch:mainfrom
Timeral:fix/qqbot-accept-is-reconnect

Conversation

@Timeral

@Timeral Timeral commented Jul 4, 2026

Copy link
Copy Markdown

Summary

QQAdapter.connect() violated the BasePlatformAdapter.connect(*, is_reconnect: bool = False) contract introduced in #52844 — it was bare async def connect(self). The gateway's reconnect watcher (gateway/run.py) recovers a platform after a fatal adapter error by building a fresh adapter and calling connect(is_reconnect=True). For QQBot, that call raised:

TypeError: connect() got an unexpected keyword argument 'is_reconnect'

so the watcher could never re-establish QQBot through the fatal-recovery path.

Live symptom

After a fatal QQBot adapter error (e.g. WebSocket close that exhausts the in-process retry ladder), the watcher's Reconnecting qqbot (attempt N)... step crashes with the TypeError. QQBot never comes back online, and the bot stops responding to messages until a full gateway restart.

Why QQBot doesn't need special reconnect logic for the flag (contract conformance only)

is_reconnect exists so adapters with a server-side update queue (e.g. Telegram's Bot API) preserve that queue across an outage instead of dropping it (#52844). QQBot has no such queue — the QQ gateway delivers messages over the live WebSocket connection, and the adapter re-authenticates with a fresh /gateway request on every connect(). There is no server-side state to preserve, and the watcher already disconnects the old adapter (cancelling the WebSocket supervisor) before building a fresh one.

So QQBot must accept the kwarg for contract conformance, and correctly has nothing special to do with it at the adapter layer — the existing connect() body already handles both cold boot and reconnect correctly once the TypeError is removed.

Changes

  • gateway/platforms/qqbot/adapter.py: Added *, is_reconnect: bool = False to QQAdapter.connect() to match the BasePlatformAdapter contract. Updated docstring to document the parameter.

Tests

Existing test_connect_uses_redirect_guard_hook calls adapter.connect() without is_reconnect, which continues to work (the parameter defaults to False). No regression risk.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/qqbot QQ Bot adapter P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jul 4, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #52966 -- identical one-line contract-conformance fix adding the keyword-only is_reconnect: bool = False to QQAdapter.connect() (gateway/platforms/qqbot/adapter.py) so the gateway reconnect watcher's connect(is_reconnect=True) no longer raises TypeError. This is a heavily saturated cluster fixing #52914; the original anchor #52922 is now CLOSED, so #52966 is the earliest still-OPEN canonical fix (siblings #53948/#54029/#54037/#53540/#53546 already deduped into it). related_to the bug #52914.

@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 (LGTM)

Contract conformance fix for QQAdapter.connect(). Adds is_reconnect kwarg to match the BasePlatformAdapter interface, with clear explanation of why no special reconnect logic is needed for QQBot. Clean and minimal.

What Looks Good

  • Minimal, targeted change
  • Clear explanation of design rationale
  • No security or debug artifacts

Reviewed by Hermes Agent

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

Tiny fix (+7/-2) accepting is_reconnect kwarg in QQAdapter.connect.

Looks Good

  • Minimal change, correct pattern
  • No security or performance concerns

Reviewed by Hermes Agent

@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.

Supplementary review: confirms LGTM. QQAdapter is_reconnect kwarg fix is a small, clean addition. Prior COMMENT found no issues. No additional concerns.


Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused QQBot contract fix. The requested behavior is already on current main, so this duplicate can be closed.

  • Automated hermes-sweeper review verified QQAdapter.connect(self, *, is_reconnect: bool = False) at gateway/platforms/qqbot/adapter.py:281.
  • Current main includes the regression test at tests/gateway/test_qqbot.py:193, covering both the default call and connect(is_reconnect=True).
  • The canonical work was cherry-picked through merged PR fix(gateway): enforce reconnect contract across adapters #61767; commit 276542c729c10ff9d093760897f4c2d1256a79ce contains the QQBot implementation and test.
  • The duplicate relationship noted by @alt-glitch is therefore now superseded by the merged implementation.

This is an automated hermes-sweeper review.

@teknium1 teknium1 closed this Jul 15, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 15, 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 duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists platform/qqbot QQ Bot adapter sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants