Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
45 changes: 37 additions & 8 deletions packages/channels/qqbot/src/QQChannel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,21 @@ export class QQChannel extends ChannelBase {
private _reconnectId: number = 0;
private blockStreaming: boolean = false;
private flushedSessions: Set<string> = new Set();
/**
* Sessions with a prompt turn currently in flight, tracked via
* onPromptStart/onPromptEnd.
*
* This is the discriminator the cron textChunk handler uses to tell
* "prompt-response chunk" from "cron/non-prompt chunk". streamState
* cannot serve that role (#6094): it is never populated when
* blockStreaming is 'on' (onResponseChunk early-returns), so prompt
* chunks leak into cronBuffer; and a residual entry from a finished
* turn's unsettled flush silently blocks cron delivery. This set is
* reliable because ChannelBase always brackets a prompt turn with
* onPromptStart and onPromptEnd (onPromptEnd runs in the prompt path's
* finally, even on error/cancel), independent of streaming config.
Comment on lines +227 to +229

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 "always brackets" claim here has a real hole: a turn whose bridge.prompt() never settles never runs the finally that calls onPromptEnd — inbound prompts have no timeout (only loop prompts do), so the wedge can be permanent, keeping the session marked until /clear, onSessionDied, or disconnect(). ChannelBase documents this state itself ("its finally may settle long after — or never", ChannelBase.ts:300). Failure shape: an inbound prompt for session S wedges permanently and nobody /clears it; an external cron flow later emits textChunks for S → this guard returns on every chunk → scheduled messages for S are silently dropped, with no log line, until disconnect/sessionDied. Under the old guard with blockStreaming:'on' the same chunks were delivered, so this is a narrowed corner the rationale doesn't argue (it defends error/cancel settlement, but not never-settlement).

Suggested change
* reliable because ChannelBase always brackets a prompt turn with
* onPromptStart and onPromptEnd (onPromptEnd runs in the prompt path's
* finally, even on error/cancel), independent of streaming config.
* reliable because ChannelBase brackets prompt turns with
* onPromptStart and onPromptEnd (onPromptEnd runs in the prompt path's
* finally, even on error/cancel), independent of streaming config
* except a turn whose bridge.prompt() never settles: it keeps the
* session marked until `/clear`, `onSessionDied`, or disconnect.
中文说明

这里的 "总是成对调用(always brackets)" 声明有一个真实的漏洞:如果某个回合的 bridge.prompt() 永远不结束,那么调用 onPromptEndfinally 就永远不会执行 —— 入站 prompt 没有超时(只有 loop prompt 有),因此这种卡死可能是永久的,session 会一直被标记,直到 /clearonSessionDieddisconnect()。ChannelBase 自己也记录了这种状态("其 finally 可能很久之后才落定 —— 或者永远不落定",ChannelBase.ts:300)。失败场景:session S 的一个入站 prompt 永久卡死且没有人 /clear 它;之后外部 cron 流对 S 发出 textChunk → 该守卫对每个块都直接返回 → S 的定时消息被静默丢弃且没有任何日志,直到 disconnect/sessionDied。在旧守卫 + blockStreaming:'on' 下,同样的块是会被投递的,因此这是收窄了一个注释中的理由并未论证的角落(理由论证了出错/取消时的落定,但没有论证永不落定的情况)。

— qwen3.8-max via Qwen Code /review (v0.22.0)

*/
private activePromptSessions: Set<string> = new Set();
private readonly qqStatePath: string;
/**
* Path to the global sessions.json managed by start.ts.
Expand Down Expand Up @@ -302,7 +317,13 @@ export class QQChannel extends ChannelBase {
return;
}
if (!wasInCronFlow) return;
if (this.streamState.has(sessionId)) return;
// Sessions with an active prompt turn belong to the prompt path
// (which delivers the response itself) — never capture their chunks
// into the cron buffer. Keyed on activePromptSessions rather than
// streamState (#6094): streamState is empty under blockStreaming:'on'
// (prompt chunks would be duplicated) and can linger after a turn
// ends (cron chunks would be silently dropped).
if (this.activePromptSessions.has(sessionId)) return;

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] No test pins the per-session keying of this guard: the mutant if (this.activePromptSessions.size > 0) return; — which blocks ALL cron capture while any prompt turn is active, not just the marked session's — passes the entire suite (verified by running it: 298 tests pass; the only failure was a purpose-built probe). Every test expecting cron capture has an empty marker set, and every test with a marker triggers chunks only for the marked session. The discriminator's core isolation property — session B's scheduled output still delivers while session A has an active prompt turn — is therefore unguarded, and a one-token mutant ships green. Suggested test (helper names per this file's conventions):

it('does not block cron capture for other sessions while a prompt is active', () => {
  const ch = makeChannel();
  ch.onPromptStart('test-chat', 'sess-A');
  (ch as unknown as { _inCronFlow: number })._inCronFlow = 1;
  triggerTextChunk(ch, 'sess-B', 'cron for b');
  flushSetImmediate();
  const cronBuffer = (ch as unknown as {
    cronBuffer: Map<string, { buffer: string }>;
  }).cronBuffer;
  expect(cronBuffer.get('sess-B')?.buffer).toBe('cron for b');
  expect(cronBuffer.has('sess-A')).toBe(false);
});
中文说明

没有任何测试钉住该守卫的"按 session 区分"这一关键性质:变异体 if (this.activePromptSessions.size > 0) return;(只要存在任何活跃 prompt 回合就阻止所有 cron 捕获,而不仅仅是被标记 session 的块)能通过整个测试套件(已实际运行验证:298 个测试通过,唯一失败的是一个专门构造的探针测试)。所有期望 cron 被捕获的测试里标记集合都是空的,而所有设置了标记的测试都只对被标记的 session 触发块。因此该判别器的核心隔离性质 —— session A 有活跃 prompt 回合时,session B 的定时输出仍能正常投递 —— 目前没有任何测试保护,一个单 token 的变异体就能全绿通过。建议补充测试(辅助函数名遵循本文件现有约定,见上方代码块)。

— qwen3.8-max via Qwen Code /review (v0.22.0)

let entry = this.cronBuffer.get(sessionId);
if (!entry) {
entry = { buffer: '', timer: null };
Expand Down Expand Up @@ -968,24 +989,31 @@ export class QQChannel extends ChannelBase {
this.flushingSessions.clear();
this.pendingStreamDelete.clear();
this.flushedSessions.clear();
this.activePromptSessions.clear();
Comment on lines 991 to +992

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] This new disconnect-time activePromptSessions.clear() has no test gating it — a measured deletion mutant (removing just this line) leaves the entire suite green, per the test-efficacy probe (harness validated: the sibling deletion mutants in onPromptEnd and onSessionDied were both killed, so the kit discriminates). If a future edit drops this line it ships with a green suite: a session mid-prompt when disconnect() runs stays permanently marked active across the restart (start() resets disposed on the same instance), and handleCronTextChunk then silently drops every subsequent cron textChunk for that session until that session's next prompt turn completes — reintroducing exactly the #6094 item-2 silent cron drop, via the disconnect path. Suggested regression test (fits the existing disconnect cron cleanup block):

it('clears active-prompt markers on disconnect', () => {
  const ch = makeChannel();
  ch.onPromptStart('test-chat', 'sess-dc');
  ch.disconnect();
  expect(
    (ch as unknown as { activePromptSessions: Set<string> })
      .activePromptSessions.size,
  ).toBe(0);
});
中文说明

这行新增的 disconnect 时 activePromptSessions.clear() 没有任何测试保护 —— 按测试有效性探针的实测结果,仅删除这一行的变异体能让整个套件保持全绿(探针 harness 已验证有效:onPromptEndonSessionDied 中对应的删除变异体都被杀死,说明该工具具备区分能力)。如果未来某次编辑删掉了这行,它会在测试全绿的情况下合入:disconnect() 发生时正处于 prompt 中的 session,其标记会在重启后永久保留(start() 会在同一实例上重置 disposed),此后 handleCronTextChunk 会静默丢弃该 session 的所有后续 cron textChunk,直到该 session 的下一个 prompt 回合完成 —— 这正是 #6094 第 2 条的静默丢消息问题,只不过经由 disconnect 路径复现。建议的回归测试(可放入现有 disconnect cron cleanup 块,见上方代码块)。

— qwen3.8-max via Qwen Code /review (v0.22.0)

}

/**
* QQ Bot API V2 does not provide a typing indicator endpoint.
* ChannelBase calls these hooks to signal prompt start/end;
* they are intentionally no-ops for this channel.
* QQ Bot API V2 does not provide a typing indicator endpoint, but these
* hooks still maintain activePromptSessions — the cron textChunk
* discriminator (see activePromptSessions). ChannelBase always pairs the
* two calls per prompt turn (onPromptEnd runs in the prompt path's
* finally, even on error/cancel).
*/
protected override onPromptStart(
_chatId: string,
_sessionId: string,
sessionId: string,
_messageId?: string,
): void {}
): void {
this.activePromptSessions.add(sessionId);
}

protected override onPromptEnd(
_chatId: string,
_sessionId: string,
sessionId: string,
_messageId?: string,
): void {}
): void {
this.activePromptSessions.delete(sessionId);
}

// ── Streaming (idle-flush with per-session buffers) ────────────

Expand Down Expand Up @@ -1290,6 +1318,7 @@ export class QQChannel extends ChannelBase {
this.flushingSessions.delete(sessionId);
this.pendingStreamDelete.delete(sessionId);
this.flushedSessions.delete(sessionId);
this.activePromptSessions.delete(sessionId);
super.onSessionDied(sessionId);
}
// ── State Persistence (cross-server context continuation) ──────
Expand Down
196 changes: 185 additions & 11 deletions packages/channels/qqbot/src/cron.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ vi.mock('@qwen-code/channel-base', () => ({
protected handleInbound(_env: unknown): Promise<void> {
return Promise.resolve();
}
protected onSessionDied(_sessionId: string): void {}
},
SessionRouter: class {
restoreSessions(): Promise<void> {
Expand All @@ -77,7 +78,9 @@ function mockResponse(
return { ok, status, text: async () => '' };
}

function makeChannel(): QQChannelClass {
function makeChannel(
configOverrides?: Record<string, unknown>,
): QQChannelClass {
textChunkHandlers.length = 0;

const router = {
Expand Down Expand Up @@ -109,6 +112,7 @@ function makeChannel(): QQChannelClass {
appID: 'test-app-id',
appSecret: 'test-secret',
'cron-msg-experimental': true,
...configOverrides,
},
bridge as unknown as import('@qwen-code/channel-base').AcpBridge,
{ router } as unknown as Record<string, unknown>,
Expand Down Expand Up @@ -312,21 +316,22 @@ describe('cronTextHandler', () => {
stderrSpy.mockRestore();
});

// A6: streamState isolation — cron handler skips sessions owned by prompt path
it('streamState isolation: cron handler skips sessions with existing streamState entry', async () => {
// A6: prompt-path isolation — cron handler skips sessions with an active
// prompt turn. (Discriminator is activePromptSessions, keyed by
// onPromptStart/onPromptEnd — see #6094. A bare streamState entry no
// longer blocks cron; covered by the #6094 item 2 test below.)
it('prompt-path isolation: cron handler skips sessions with an active prompt turn', async () => {
const ch = makeChannel();
const pvt = ch as unknown as Record<string, unknown>;
pvt['_ready'] = true;
pvt['_inCronFlow'] = 1;

// Pre-populate streamState for this session — prompt path owns it
const ss = pvt['streamState'] as Map<string, unknown>;
ss.set('sess-stream', {
chatId: 'test-chat',
buffer: 'existing prompt text',
timer: null,
retryCount: 0,
});
// ChannelBase brackets the prompt turn with onPromptStart/onPromptEnd.
(
ch as unknown as {
onPromptStart: (chatId: string, sessionId: string) => void;
}
).onPromptStart('test-chat', 'sess-stream');

triggerTextChunk('sess-stream', 'should be ignored by cron');
await flushSetImmediate();
Expand Down Expand Up @@ -568,3 +573,172 @@ describe('disconnect cron cleanup', () => {
expect(pvt['_inCronFlow']).toBe(0);
});
});

// ---------------------------------------------------------------------------
// prompt/cron discriminator (issue #6094)
// ---------------------------------------------------------------------------
describe('prompt/cron textChunk discriminator (#6094)', () => {

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] This comment is about the PR description, not this code line (anchored here because this describe block is the diff's only reference to #6094): the PR body's "Partially fixes #6094" contains GitHub's closing keyword fixes #6094, so merging this PR will auto-close the umbrella issue #6094 even though items 5 and 6 remain open — as the PR itself declares ("items 5 and 6 stay open"). GitHub resolves closing keywords anywhere in the PR body, and "Partially fixes" still matches. Same-repo precedent: PR #9631 ("Partially fixes #9487") auto-closed #9487 on merge (closed 2 seconds after the merge, state_reason: completed, close actor = the merging user). Items 5 (botOpenId instruction timing) and 6 (token-refresh connect() retry) have no other tracking issue, so they would be silently orphaned at merge. Suggestion: reword the Linked Issues section to a non-closing reference (e.g. "Part of #6094 — addresses items 1 and 2; items 5 and 6 tracked separately"), or split items 5/6 into their own issues before merge.

中文说明

本条评论针对的是 PR 描述,而不是这行代码(锚点选在这里,是因为这个 describe 块是 diff 中唯一引用 #6094 的位置):PR 正文中的 "Partially fixes #6094" 包含 GitHub 的关闭关键字 fixes #6094,因此合入本 PR 会自动关闭伞 issue #6094 —— 尽管第 5、6 条仍然开放(PR 自己也声明了 "items 5 and 6 stay open")。GitHub 会解析 PR 正文任意位置的关闭关键字,"Partially fixes" 同样会命中。同仓库先例:PR #9631("Partially fixes #9487")在合入时自动关闭了 #9487(合入后 2 秒即关闭,state_reason: completed,关闭者为合入人)。第 5 条(botOpenId 指令时机)和第 6 条(token 刷新后 connect() 无重试)没有其他跟踪 issue,合入后会被静默遗弃。建议:把 Linked Issues 部分改写为非关闭式引用(例如 "Part of #6094 — addresses items 1 and 2; items 5 and 6 tracked separately"),或在合入前把第 5/6 条拆成独立 issue。

— qwen3.8-max via Qwen Code /review (v0.22.0)

beforeEach(() => {
vi.clearAllMocks();
textChunkHandlers.length = 0;
mockSendQQMessage.mockResolvedValue(mockResponse(true));
vi.useFakeTimers();
});

afterEach(() => {
vi.useRealTimers();
});

async function flushSetImmediate(): Promise<void> {
await vi.advanceTimersByTimeAsync(0);
}

function promptHooks(ch: QQChannelClass): {
onPromptStart: (chatId: string, sessionId: string) => void;
onPromptEnd: (chatId: string, sessionId: string) => void;
} {
return ch as unknown as {
onPromptStart: (chatId: string, sessionId: string) => void;
onPromptEnd: (chatId: string, sessionId: string) => void;
};
}

// #6094 item 1: with blockStreaming:'on', onResponseChunk early-returns
// without populating streamState, so the streamState.has(sessionId) guard
// cannot tell prompt chunks from cron chunks. While a cron flow is active,
// every prompt-response chunk leaks into cronBuffer and is re-sent by the
// 2s idle flush on top of the BlockStreamer delivery.
it('item 1: blockStreaming=on prompt chunks during a cron flow are not duplicated into cronBuffer', async () => {
const ch = makeChannel({ blockStreaming: 'on' });
const pvt = ch as unknown as Record<string, unknown>;
pvt['_ready'] = true;

// ChannelBase brackets every prompt turn with onPromptStart/onPromptEnd.
promptHooks(ch).onPromptStart('test-chat', 'sess-prompt');

// A cron flow is active concurrently (scheduled-message flow in flight).
pvt['_inCronFlow'] = 1;

// Prompt response chunk arrives. With blockStreaming:'on' the streaming
// path early-returns, so no streamState entry exists for this session.
triggerTextChunk('sess-prompt', 'prompt response text');
await flushSetImmediate();

const cronBuffer = pvt['cronBuffer'] as Map<string, { buffer: string }>;
// The prompt text belongs to an active prompt — it must not be captured
// by the cron buffer (BlockStreamer already delivers it).
expect(cronBuffer.has('sess-prompt')).toBe(false);

await vi.advanceTimersByTimeAsync(2000);
expect(mockSendQQMessage).not.toHaveBeenCalled();

promptHooks(ch).onPromptEnd('test-chat', 'sess-prompt');
});

// Regression guard for item 1 fix: genuine cron chunks (no active prompt
// for the session) must still be buffered and delivered with
// blockStreaming:'on'.
it('item 1 regression: cron chunks without an active prompt are still delivered (blockStreaming=on)', async () => {
const ch = makeChannel({ blockStreaming: 'on' });
const pvt = ch as unknown as Record<string, unknown>;
pvt['_ready'] = true;
pvt['_inCronFlow'] = 1;

triggerTextChunk('sess-cron', 'cron text');
await flushSetImmediate();

await vi.advanceTimersByTimeAsync(2000);

expect(mockSendQQMessage).toHaveBeenCalledTimes(1);
expect(mockSendQQMessage).toHaveBeenCalledWith(
'https://api.sgroup.qq.com',
'/v2/users/test-chat/messages',
'test-token',
{ msg_type: 2, markdown: { content: 'cron text' } },
);
});

// #6094 item 2: a lingering streamState entry from an earlier prompt (e.g.
// cancelled/errored turn whose cleanup has not settled) silently drops all
// subsequent cron textChunks for the same sessionId because the guard keys
// on streamState. The guard must key on whether a prompt turn is actually
// active, not on residual streaming state.
it('item 2: lingering streamState entry after prompt end does not block cron delivery', async () => {
const ch = makeChannel();
const pvt = ch as unknown as Record<string, unknown>;
pvt['_ready'] = true;

// A prompt turn streams a partial answer, then ends (ChannelBase always
// runs onPromptEnd in its finally, even on error/cancel).
promptHooks(ch).onPromptStart('test-chat', 'sess-leak');
(
ch as unknown as {
onResponseChunk: (
chatId: string,
chunk: string,
sessionId: string,
) => void;
}
).onResponseChunk('test-chat', 'partial answer', 'sess-leak');
promptHooks(ch).onPromptEnd('test-chat', 'sess-leak');

// The streamState entry lingers until its idle flush settles.
const ss = pvt['streamState'] as Map<string, unknown>;
expect(ss.has('sess-leak')).toBe(true);

// A cron flow now emits output for the same session.
pvt['_inCronFlow'] = 1;
triggerTextChunk('sess-leak', 'cron text');
await flushSetImmediate();

const cronBuffer = pvt['cronBuffer'] as Map<string, { buffer: string }>;
expect(cronBuffer.get('sess-leak')?.buffer).toBe('cron text');

await vi.advanceTimersByTimeAsync(2000);

// The cron text must be delivered despite the lingering streamState.
const sentBodies = mockSendQQMessage.mock.calls.map(
(call) => call[3] as { markdown?: { content?: string } },
);
expect(
sentBodies.some((body) => body.markdown?.content === 'cron text'),
).toBe(true);
});

// After the prompt turn ends, cron chunks for the session flow again.
it('resumes cron capture after the prompt turn ends', async () => {
const ch = makeChannel();
const pvt = ch as unknown as Record<string, unknown>;
pvt['_ready'] = true;
pvt['_inCronFlow'] = 1;

promptHooks(ch).onPromptStart('test-chat', 'sess-resume');
promptHooks(ch).onPromptEnd('test-chat', 'sess-resume');

triggerTextChunk('sess-resume', 'cron after prompt');
await flushSetImmediate();

const cronBuffer = pvt['cronBuffer'] as Map<string, { buffer: string }>;
expect(cronBuffer.get('sess-resume')?.buffer).toBe('cron after prompt');
});

// Session-death cleanup: a dead session must not stay marked as an active
// prompt (mirrors streamState cleanup in onSessionDied).
it('onSessionDied clears the active-prompt marker', async () => {
const ch = makeChannel();
const pvt = ch as unknown as Record<string, unknown>;
pvt['_ready'] = true;
pvt['_inCronFlow'] = 1;

promptHooks(ch).onPromptStart('test-chat', 'sess-died');
(
ch as unknown as { onSessionDied: (sessionId: string) => void }
).onSessionDied('sess-died');

triggerTextChunk('sess-died', 'cron after death');
await flushSetImmediate();

const cronBuffer = pvt['cronBuffer'] as Map<string, { buffer: string }>;
expect(cronBuffer.get('sess-died')?.buffer).toBe('cron after death');
});
});
Loading