-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): report teammate result settled before event bridge attach #10245
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.
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
Changes from all commits
a284e5b
c387127
fbd6e76
6d1af9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -230,6 +230,25 @@ export interface AgentMessage { | |
| metadata?: Record<string, unknown>; | ||
| } | ||
|
|
||
| /** | ||
| * The last model-visible answer in a message history, or undefined | ||
| * when there is none. Scans most-recent-first; the first non-empty, | ||
| * non-thought assistant message wins. Shared by the team pre-attach | ||
| * recovery (TeamManager) and the arena final-text fallback | ||
| * (ArenaManager) so both apply the same selection rule. | ||
| */ | ||
| export function lastVisibleAnswer( | ||
| messages: readonly AgentMessage[], | ||
| ): string | undefined { | ||
| for (let i = messages.length - 1; i >= 0; i--) { | ||
| const message = messages[i]!; | ||
| if (message.role !== 'assistant' || message.thought) continue; | ||
|
Comment on lines
+244
to
+245
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R2-1: Still standing from round 2 (re-checked at this head — code and tests unchanged since): Add a small direct unit test for the exported 中文说明[Suggestion] R2-1:第 2 轮遗留(已对照本 head 复核——代码与测试自那以后未变): 建议为导出的 — qwen3.8-max via Qwen Code /review (v0.22.2) |
||
| const text = message.content.trim(); | ||
| if (text) return text; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Snapshot of in-progress streaming state for UI mid-switch handoff. | ||
| * Returned by AgentInteractive.getInProgressStream(). | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,289 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| /** | ||
| * Regression tests for #10211: the initial teammate round can complete | ||
| * (final round text + IDLE) before TeamManager.setupEventBridge attaches, | ||
| * because spawnTeammate awaits backend.spawnAgent() first and the emitter | ||
| * does not buffer events for late subscribers. The leader must still | ||
| * receive the initial round's result exactly once. | ||
| */ | ||
|
|
||
| import { describe, it, expect, vi, afterEach } from 'vitest'; | ||
| import { TeamCoordinationHarness } from './test-utils/coordination-harness.js'; | ||
| import { Storage } from '../../config/storage.js'; | ||
| import { AgentStatus } from '../runtime/agent-types.js'; | ||
| import { AgentEventType } from '../runtime/agent-events.js'; | ||
|
|
||
| vi.mock('../../config/storage.js', async (importOriginal) => { | ||
| const original = | ||
| await importOriginal<typeof import('../../config/storage.js')>(); | ||
| let mockGlobalDir = ''; | ||
| return { | ||
| ...original, | ||
| Storage: { | ||
| ...original.Storage, | ||
| getGlobalQwenDir: () => mockGlobalDir, | ||
| __setMockGlobalDir: (dir: string) => { | ||
| mockGlobalDir = dir; | ||
| }, | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| function setMockDir(dir: string): void { | ||
| ( | ||
| Storage as unknown as { | ||
| __setMockGlobalDir: (d: string) => void; | ||
| } | ||
| ).__setMockGlobalDir(dir); | ||
| } | ||
|
|
||
| /** Let queued fire-and-forget coordination work settle. */ | ||
| async function settleAsyncWork(): Promise<void> { | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| } | ||
|
|
||
| describe('initial teammate result before event bridge attachment (#10211)', () => { | ||
| let harness: TeamCoordinationHarness | undefined; | ||
|
|
||
| afterEach(async () => { | ||
| await harness?.cleanup(); | ||
| harness = undefined; | ||
| }); | ||
|
|
||
| async function createHarness(): Promise<TeamCoordinationHarness> { | ||
| const h = await TeamCoordinationHarness.create(); | ||
| setMockDir(h.tmpDir); | ||
| harness = h; | ||
| return h; | ||
| } | ||
|
|
||
| it('reports final text to the leader when the initial round completes before spawnAgent resolves', async () => { | ||
| const h = await createHarness(); | ||
|
|
||
| // onStart runs inside FakeBackend.spawnAgent(), i.e. before | ||
| // TeamManager.setupEventBridge() subscribes. Emits the final round | ||
| // text and settles IDLE while spawnAgent() is still resolving — | ||
| // the in-process race from the issue. | ||
| await h.spawnTeammate('worker', { | ||
| onStart: (agent) => { | ||
| agent.setStatus(AgentStatus.RUNNING); | ||
| agent.getEventEmitter().emit(AgentEventType.ROUND_TEXT, { | ||
| subagentId: agent.agentId, | ||
| round: 1, | ||
| text: 'initial result', | ||
| thoughtText: '', | ||
| timestamp: Date.now(), | ||
| }); | ||
| agent.setStatus(AgentStatus.IDLE); | ||
| }, | ||
| }); | ||
|
|
||
| await vi.waitFor(async () => { | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([ | ||
| expect.objectContaining({ | ||
| from: 'worker', | ||
| text: 'initial result', | ||
| }), | ||
| ]); | ||
| }); | ||
|
|
||
| // Exactly once: after coordination settles, no duplicate report | ||
| // for the same round may arrive. | ||
| await settleAsyncWork(); | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([]); | ||
| }); | ||
|
|
||
| it('does not re-report the initial result when a later round completes live', async () => { | ||
| const h = await createHarness(); | ||
|
|
||
| await h.spawnTeammate('worker', { | ||
| onStart: (agent) => { | ||
| agent.setStatus(AgentStatus.RUNNING); | ||
| agent.getEventEmitter().emit(AgentEventType.ROUND_TEXT, { | ||
| subagentId: agent.agentId, | ||
| round: 1, | ||
| text: 'initial result', | ||
| thoughtText: '', | ||
| timestamp: Date.now(), | ||
| }); | ||
| agent.setStatus(AgentStatus.IDLE); | ||
| }, | ||
| onMessage: (_message, agent) => { | ||
| agent.getEventEmitter().emit(AgentEventType.ROUND_TEXT, { | ||
| subagentId: agent.agentId, | ||
| round: 2, | ||
| text: 'follow-up result', | ||
| thoughtText: '', | ||
| timestamp: Date.now(), | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| await vi.waitFor(async () => { | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([ | ||
| expect.objectContaining({ text: 'initial result' }), | ||
| ]); | ||
| }); | ||
|
|
||
| await h.teamManager.sendMessage('worker', 'next task', 'leader'); | ||
| await h.waitForStatus('worker', AgentStatus.IDLE); | ||
|
|
||
| // The live second round reports its own text exactly once; the | ||
| // pre-attach initial result must not be reported again. | ||
| await vi.waitFor(async () => { | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([ | ||
| expect.objectContaining({ text: 'follow-up result' }), | ||
| ]); | ||
| }); | ||
|
|
||
| await settleAsyncWork(); | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([]); | ||
| }); | ||
|
|
||
| it('does not report anything at spawn when no round ran before the bridge attached', async () => { | ||
| const h = await createHarness(); | ||
|
|
||
| // Plain spawn: the harness agent is IDLE at attach time but never | ||
| // emitted round text (no pre-attach round). The attach-time | ||
| // reconciliation must not invent a report. | ||
| await h.spawnTeammate('worker'); | ||
| await settleAsyncWork(); | ||
|
|
||
| expect(await h.teamManager.getLeaderMessages()).toEqual([]); | ||
|
|
||
| // The live path still works afterwards. | ||
| await h.teamManager.sendMessage('worker', 'task', 'leader'); | ||
| await h.waitForStatus('worker', AgentStatus.IDLE); | ||
| await vi.waitFor(async () => { | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([ | ||
| expect.objectContaining({ | ||
| text: expect.stringContaining( | ||
| 'completed a turn without a model-visible final answer', | ||
| ), | ||
| }), | ||
| ]); | ||
| }); | ||
| }); | ||
|
|
||
| it('still reports the recovered result when the teammate sent an explicit leader message pre-attach', async () => { | ||
| const h = await createHarness(); | ||
|
|
||
| // The default initialTask prompt instructs teammates to report via | ||
| // send_message(to: "leader"). Such a send goes through | ||
| // TeamManager.sendMessage synchronously — no event bridge needed — | ||
| // and marks the sender as having reported explicitly. The seed must | ||
| // clear that flag exactly like the live onRoundText handler does, | ||
| // or the replayed IDLE settlement skips the recovered answer and | ||
| // the leader receives zero automatic reports of the initial result. | ||
| await h.spawnTeammate('worker', { | ||
| onStart: async (agent) => { | ||
| agent.setStatus(AgentStatus.RUNNING); | ||
| await h.teamManager.sendMessage('leader', 'progress note', 'worker'); | ||
| agent.getEventEmitter().emit(AgentEventType.ROUND_TEXT, { | ||
| subagentId: agent.agentId, | ||
| round: 1, | ||
| text: 'initial result', | ||
| thoughtText: '', | ||
| timestamp: Date.now(), | ||
| }); | ||
| agent.setStatus(AgentStatus.IDLE); | ||
| }, | ||
| }); | ||
|
|
||
| await vi.waitFor(async () => { | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([ | ||
| expect.objectContaining({ from: 'worker', text: 'progress note' }), | ||
| expect.objectContaining({ from: 'worker', text: 'initial result' }), | ||
| ]); | ||
| }); | ||
|
|
||
| // Exactly once: the explicit note plus one automatic forwarding. | ||
| await settleAsyncWork(); | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([]); | ||
| }); | ||
|
|
||
| it('reports the last pre-attach round text when the round had multiple turns', async () => { | ||
| const h = await createHarness(); | ||
|
|
||
| // A multi-turn pre-attach round emits several ROUND_TEXT events; | ||
| // the recovery scan must walk the history backwards so the most | ||
| // recent non-empty visible answer wins, not the earliest one. | ||
| await h.spawnTeammate('worker', { | ||
| onStart: (agent) => { | ||
| agent.setStatus(AgentStatus.RUNNING); | ||
| agent.getEventEmitter().emit(AgentEventType.ROUND_TEXT, { | ||
| subagentId: agent.agentId, | ||
| round: 1, | ||
| text: 'early turn answer', | ||
| thoughtText: '', | ||
| timestamp: Date.now(), | ||
| }); | ||
| agent.getEventEmitter().emit(AgentEventType.ROUND_TEXT, { | ||
| subagentId: agent.agentId, | ||
| round: 1, | ||
| text: 'final turn answer', | ||
| thoughtText: '', | ||
| timestamp: Date.now(), | ||
| }); | ||
| agent.setStatus(AgentStatus.IDLE); | ||
| }, | ||
| }); | ||
|
|
||
| await vi.waitFor(async () => { | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([ | ||
| expect.objectContaining({ | ||
| from: 'worker', | ||
| text: 'final turn answer', | ||
| }), | ||
| ]); | ||
| }); | ||
|
|
||
| // The earlier turn text must never be reported on its own. | ||
| await settleAsyncWork(); | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([]); | ||
| }); | ||
|
|
||
| it('never reports thought-only round text, falling back to the earlier visible answer', async () => { | ||
| const h = await createHarness(); | ||
|
|
||
| // AgentCore emits ROUND_TEXT whenever roundThoughtText is | ||
| // non-empty, so a trailing thought-only event is a reachable | ||
| // pre-attach shape. The recovery must skip thought messages and | ||
| // never surface internal reasoning as the round's final answer. | ||
| await h.spawnTeammate('worker', { | ||
| onStart: (agent) => { | ||
| agent.setStatus(AgentStatus.RUNNING); | ||
| agent.getEventEmitter().emit(AgentEventType.ROUND_TEXT, { | ||
| subagentId: agent.agentId, | ||
| round: 1, | ||
| text: 'visible answer', | ||
| thoughtText: '', | ||
| timestamp: Date.now(), | ||
| }); | ||
| agent.getEventEmitter().emit(AgentEventType.ROUND_TEXT, { | ||
| subagentId: agent.agentId, | ||
| round: 1, | ||
| text: '', | ||
| thoughtText: 'internal reasoning about the task', | ||
| timestamp: Date.now(), | ||
| }); | ||
| agent.setStatus(AgentStatus.IDLE); | ||
| }, | ||
| }); | ||
|
|
||
| await vi.waitFor(async () => { | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([ | ||
| expect.objectContaining({ from: 'worker', text: 'visible answer' }), | ||
| ]); | ||
| }); | ||
|
|
||
| // Nothing else — in particular no thought content — may arrive. | ||
| await settleAsyncWork(); | ||
| expect(await h.teamManager.getLeaderMessages()).toEqual([]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]
lastVisibleAnswer'srole !== 'assistant'filter has no discriminating test: every oracle in the suite builds assistant-only histories (FakeAgent records only ROUND_TEXT-derived assistant messages; the arena mock returns a single assistant entry), so deleting the role check leaves all 150 surrounding tests green while the filter is load-bearing in production.AgentCore.setupStateListenerspushesuser,tool_callandtool_resultentries into the same history, and at attach time a RUNNING teammate mid-round can legitimately have atool_result(or the initial-task user prompt) as its newest entry — with the role check removed, that entry would be seeded and reported to the leader as the teammate's initial result. Verified by mutation this round:Add a small direct unit test for the exported
lastVisibleAnswerwith mixed roles:[user, assistant('draft answer'), tool_call, tool_result]→'draft answer'; a history with no assistant message →undefined; whitespace-only assistant content →undefined. Fix acceptance:expect(lastVisibleAnswer(mixed)).toBe('draft answer')must go red when the role check is removed (it would return the trailingtool_resulttext) — please add it and verify by removing the check.中文说明
[Suggestion]
lastVisibleAnswer的role !== 'assistant'过滤没有判别性测试:套件中所有 oracle 都只构造纯 assistant 历史(FakeAgent 只记录 ROUND_TEXT 派生的 assistant 消息,arena mock 只返回单条 assistant 条目),因此删除 role 检查后全部 150 个周边测试仍然通过,而该过滤在生产路径上是承重的。AgentCore.setupStateListeners会向同一历史写入user、tool_call、tool_result条目,attach 时一个处于 RUNNING、回合进行中的 teammate 完全可能以tool_result(或初始任务 user 提示词)作为最新条目 —— 删除 role 检查后,该条目会被作为初始结果上报给 leader。本轮已用变异验证:删除 role 检查的变异下 150 个现有测试全部通过,而混合角色探针从 4/4 通过变为 1/4(工具结果文本和任务提示词被当作答案返回)。建议为导出的
lastVisibleAnswer补一个混合角色的直接单测:[user, assistant('draft answer'), tool_call, tool_result]→'draft answer';无 assistant 消息的历史 →undefined;仅空白的 assistant 内容 →undefined。修复验收:移除 role 检查时expect(lastVisibleAnswer(mixed)).toBe('draft answer')必须变红 —— 请补充该用例并通过移除检查验证。— qwen3.8-max via Qwen Code /review (v0.22.2)