Skip to content

fix(qqbot): restore AcpBridge session loading — return input sessionId, patch catch path #7722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 1 commit into from
Jul 25, 2026
Merged

fix(qqbot): restore AcpBridge session loading — return input sessionId, patch catch path #7722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/channels/base/src/AcpBridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,12 +203,12 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge {
async loadSession(sessionId: string, cwd: string): Promise<string> {
const conn = this.ensureConnection();
await this.registerChannelLoopMcpServer();
const response = await conn.loadSession({
await conn.loadSession({
sessionId,
cwd,
mcpServers: [],
});
return response.sessionId;
return sessionId;
Comment on lines +206 to +211

@qwen-code-ci-bot qwen-code-ci-bot Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] AcpBridge.loadSession has no unit test — AcpBridge.test.ts has zero matches for loadSession. The very return-value bug this PR fixes (returning response.sessionId which was always undefined) went undetected because the SessionRouter tests mock loadSession at the interface level and never exercise the real implementation. Without a test asserting the return value, a future regression re-introducing return response.sessionId would silently pass all existing tests.

Suggested: add a test that mocks conn.loadSession to return a LoadSessionResponse (no sessionId field) and asserts that bridge.loadSession() returns the input sessionId.

— qwen3.7-max via Qwen Code /review

}

async prompt(
Expand Down
1 change: 1 addition & 0 deletions packages/channels/qqbot/src/QQChannel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1973,6 +1973,7 @@ export class QQChannel extends ChannelBase {
onReady();
})
.catch(() => {
this.fixRestoredSessions();
Comment on lines 1975 to +1976

@qwen-code-ci-bot qwen-code-ci-bot Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The .then() and .catch() handlers now share four calls — fixRestoredSessions(), finalizeReady(), _checkGroupAllPolicyRequireMention(), and onReady() — duplicated across both paths. A future change to post-restore finalization must be mirrored in two places; if a developer adds a step in one handler but forgets the other, the error path silently diverges from the success path.

Consider extracting the shared calls into .finally():

.then(() => {
  // success-specific: session count logging
})
.catch(() => {
  // error-specific: warning log
})
.finally(() => {
  this.fixRestoredSessions();
  this.finalizeReady();
  this._checkGroupAllPolicyRequireMention();
  onReady();
});

— qwen3.7-max via Qwen Code /review

Comment on lines 1975 to +1976

@qwen-code-ci-bot qwen-code-ci-bot Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The READY cold-start .catch() handler has no test coverage. The success path (.then()) is tested in events.test.ts, but the failure path — where fixRestoredSessions() is newly added — is never exercised. If a future refactor accidentally removes or misplaces this call, no test would detect the regression.

Suggested: add a test that mocks router.restoreSessions() to reject, dispatches a READY with coldStart=true, and asserts that fixRestoredSessions() is called.

— qwen3.7-max via Qwen Code /review

process.stderr.write(
`[QQ:${this.name}] WARNING: router session restore failed — cron messages will be dropped until sessions re-establish\n`,
);
Expand Down
Loading