fix(core): report teammate result settled before event bridge attach - #10245
Conversation
TeamManager.spawnTeammate awaits backend.spawnAgent() before attaching the event bridge, and AgentEventEmitter does not buffer events for late subscribers. If the in-process initial round settles (final round text plus IDLE) while spawnAgent() is still resolving, both events are lost: the leader never receives the initial result, and the existing idle reconciliation only flushes pending messages (#10211). At bridge-attach time, recover the last model-visible answer from the agent's message history (AgentCore appends one assistant message per ROUND_TEXT) and seed pendingFinalReports with it. When the agent already settled IDLE before attach, replay the missed IDLE STATUS_CHANGE through the existing handler so the result is reported and pending messages flush exactly once. Without pre-attach round text there is no completed round to report, so the previous flush-only behavior is kept. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Re-run at the new head Template looks good ✓ Problem: this one is honest about itself — the linked issue (#10211) says up front that the lost-result window was never observed in production, only confirmed in source. Normally that is where a Direction: aligned. Silently losing a teammate's initial result is exactly the kind of correctness gap the multi-agent work can't ship with. CHANGELOG has no direct reference, but the area is core roadmap territory. Size: core paths touched — 112 production lines (TeamManager.ts 68, agent-types.ts 19, backends/types.ts 8, ArenaManager.ts 17) vs 320 test lines (new suite 289, fake-agent harness 31). Well below the escalation thresholds; author is a maintainer regardless. Approach: the scope feels right. It reuses the existing Risk: no elevated risk signals — none of the revert-correlated high-risk paths are touched. Moving on to code review. 🔍 中文说明感谢贡献！ 已在新的 head 模板完整 ✓ 问题：这个 PR 很诚实——关联 issue（#10211）明确说明该丢失窗口从未在生产中观测到，只在源码中确认。通常 方向：对齐。静默丢失 teammate 初始结果正是多智能体功能不能带着上线的正确性缺口。CHANGELOG 无直接引用，但该领域是核心路线图范围。 规模：触及核心路径——生产代码 112 行（TeamManager.ts 68 行、agent-types.ts 19 行、backends/types.ts 8 行、ArenaManager.ts 17 行），测试 320 行（新套件 289 行、fake-agent 测试架 31 行）。远低于升级阈值；且作者是维护者。 方案：范围合理。复用既有 风险：无升级风险信号——未触及任何与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review (re-run at
|
| Check | Conclusion |
|---|---|
Classify PR |
✅ success |
Dependency CVE audit |
✅ success |
Desktop Shell (ubuntu-22.04) |
✅ success |
Desktop Shell (windows-2022) |
✅ success |
Post Coverage Comment (ubuntu-latest, 22.x) |
✅ success |
Secret scan (TruffleHog) |
✅ success |
Test (ubuntu-latest, Node 22.x) |
✅ success |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
✅ success |
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。
The big change since the last pass: the main build break is fixed (#10243 merged) and the fix is on this branch — I verified client.telemetrySwap.test.ts at this head carries exactly one getToolRegistry property, where the duplicate is what killed every prior commit's tsc --build during install. The ubuntu unit job started 09:58 UTC and was still running at fetch time (10:05 UTC) — well past the install-time death point — so the suite is genuinely executing this commit for the first time. Per triage rules I don't poll; the table above updates in place once CI settles. The PR body's red-before / green-after numbers remain the author's local claims until that lands.
Sandboxed verification would settle the rest: @qwen-code /verify — that the new suite is genuinely red on base and green with the fix still rests on the author's local run; now that #10243 has landed, /verify's A/B base build no longer hits the old tsc failure and can prove the suite actually pins the change.
中文说明
代码审查（在 fbd6e769 上重新运行，基线 main）
新提交是作者对上一轮 /review 的回应。我逐项对照代码核实，而不是采信回复本身：
- R1-1（Critical）修复正确：种子现在会
explicitLeaderReports.delete(agentId)，且仅在恢复文本非空时执行——与实时onRoundText处理器完全镜像。第四个新测试钉住了它：worker 在 attach 前显式send_message(to: leader)后带可见文本进入 IDLE——若不清除标志，回放的结算会跳过恢复的答案，leader 将收不到任何自动上报。 - R1-4 的幸存变异现在会被抓到：测试 5 发两个 pre-attach ROUND_TEXT、只期望第二个（钉住反向扫描）；测试 6 追加一个纯 thought 事件、期望更早的可见答案（钉住 thought 跳过）——正是上一轮点名的两个变异。
- R1-3 的去重方向正确：
lastVisibleAnswer提取到agent-types.ts；我核实了ArenaTranscriptEntry与AgentMessage结构完全一致，ArenaManager 无需映射即可委托，竞技场的选择语义逐位不变。 - R1-5 的静默已消除：种子时与回放时各一条
debug.info（含 agent 名、id、恢复长度），与相邻异常守卫使用同一个debug日志器。 - R1-2 的延后合理：回放的报告仍在
writeTeamFile持久化之前发出，需要就"文件写入期间实时回合的过期守卫 + 故障注入测试"做设计决定——属于下一轮的正当内容；该窗口是 spawn 期间的本地文件系统失败，不是用户会撞到的正确性路径。
本轮另外复核的点：恰好一次语义仍然成立（种子先于回放写入；回放走与实时路径相同的 onStatusChange IDLE 分支，读取并删除种子后按正常路径上报）；终态对账分支在重构后保留（pre-attach 进入 FAILED/COMPLETED 时，终态回放会清除种子而不发送）；测试架复现的是真实竞态（FakeBackend.spawnAgent() 等待 agent.start()，onStart 脚本先行执行；FakeAgent 的消息历史镜像与 AgentCore.setupStateListeners 完全一致）；无扩散影响（getMessages?() 为可选；消费方均已点名：InProcessBackend→AgentInteractive 本就实现、FakeBackend→FakeAgent 本 PR 新增、PTY backend 不受影响）。
遗留的不阻塞小问题（与上轮相同）：合成回放不带 roundCancelledByUser（pre-attach 被用户取消的回合理论上会被上报而非抑制——需要用户在 spawn 微秒窗口内交互，实际不可达）；以及 attach 时 RUNNING 且带种子的极端情形（在 run loop 的结算不变式下不可达）。
本 head 无 Critical 发现。
CI 证据（经 API 读取——本审查未运行测试）
与上轮最大的不同：main 的构建损坏已修复（#10243 已合入）且修复已在本分支上——我核实了本 head 的 client.telemetrySwap.test.ts 只有一个 getToolRegistry 属性（重复属性正是此前每个提交在安装阶段压垮 tsc --build 的原因）。ubuntu 单元测试 job 于 09:58 UTC 开始，读取时（10:05 UTC）仍在运行——已远远越过此前的安装期死亡点——套件这次是真正在执行本提交。按 triage 规则不轮询；CI 落定后上表会原地更新。PR 正文中"修复前红 / 修复后绿"的数字在落定前仍是作者本地声明。
沙箱验证可以定论其余部分：@qwen-code /verify —— 新套件是否在基线上红、在修复后绿仍只有作者本地运行背书；现在 #10243 已合入，/verify 的 A/B 基线构建不会再撞上旧的 tsc 失败，可以证明套件确实钉住了修复。
— Qwen Code · qwen3.8-max
Reviewed at fbd6e7696485ad8f2c05eb200b8616301ee53136 · re-run with @qwen-code /triage
|
Confidence: 4/5 — the review round landed exactly as asked and I verified every claimed fix line-by-line at the new head; the one point off is that CI on Stepping back: this is what a review round done right looks like. The previous pass found one real Critical — the seed restoring My independent proposal before reading the original diff had the same shape as what shipped, and I still don't see a simpler path: the message history is the one record that survives event loss, and replaying the settlement through the live handler is the only way to get exactly-once without buffering events. The two residual nits (cancelled-round flag, RUNNING-at-attach seed) both need physically unreachable shapes and are correctly left alone. The deferred R1-2 item — replayed report firing before the team file persists — is a real but narrow fault-injection question that deserves its own design discussion, not a block here. Six months from now this reads naturally: three small comments mark the reconciliation block, the shared helper documents its own selection rule, and the test file names the issue it exists for. I'd thank the author, not curse them. Verdict: approve. CI on the reviewed commit was still running at review time (unit job ~8 minutes in, suite genuinely executing for the first time on this branch now that the main build break is fixed), so approval deferred until CI lands green on 中文说明置信度：4/5 —— 这一轮 review 的整改完全按要求落地，我在新的 head 上逐行核实了每一项声明的修复；扣掉的一分是审查时 退一步看：这是一次正确完成的 review 整改。上一轮发现了一个真实的 Critical——种子恢复了 我在读 diff 之前的独立方案与最终落地的形状相同，我仍然没有看到更简的路径：消息历史是事件丢失后唯一幸存的记录，通过实时处理器回放结算是不缓存事件而实现恰好一次的唯一方式。两个遗留小问题（取消回合标志、attach 时 RUNNING 的种子）都需要物理上不可达的形态，正确地不予处理。延后的 R1-2 项——回放的报告在 team 文件持久化之前发出——是一个真实但狭窄的故障注入问题，值得单独的设计讨论，不构成阻塞。 六个月后回看也顺理成章：三条简短注释标出对账块，共享辅助函数自述其选择规则，测试文件点名了它为之存在的 issue。值得感谢作者。 结论：批准。审查时该提交的 CI 仍在运行（单元测试 job 已运行约 8 分钟——在 main 构建损坏修复后，套件第一次真正在本分支上执行），因此批准推迟到 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
tmux E2E test report (head
|
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Approving. Independent review at this head (c387127) found no blocking (Critical) issues, and I verified it by execution in a prior pass: the pre-attach round-text recovery mirrors the live ROUND_TEXT→pendingFinalReports semantics, replays the RUNNING→IDLE settlement through the live handler exactly once only when pre-attach text exists, and falls back unchanged for PTY handles / no-text spawns. Local tmux run at this head: TeamManager.initial-result.test.ts 3/3 and the surrounding team suites 79/79. The bot has approved at this head and the test lanes are green; concur.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent 5": executing the new TeamManager.initial-result.test.ts suite to confirm it passes (worktree has no node_modules; a full npm ci + build exceeded the remaining ….
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): src/agents/team/TeamManager.initial-result.test.ts — no such file or directory; 440 tests green — this review observed 21978, 1743, 25106, 1667, 605, 4329, 638 passed; 1682 passed — this review observed 21978, 1743, 25106, 1667, 605, 4329, 638 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："agent 5"：executing the new TeamManager.initial-result.test.ts suite to confirm it passes (worktree has no node_modules; a full npm ci + build exceeded the remaining …。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
Test Plan（非阻断）：src/agents/team/TeamManager.initial-result.test.ts — no such file or directory; 440 tests green — this review observed 21978, 1743, 25106, 1667, 605, 4329, 638 passed; 1682 passed — this review observed 21978, 1743, 25106, 1667, 605, 4329, 638 passed。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| onStatusChange({ | ||
| agentId, | ||
| previousStatus: AgentStatus.RUNNING, | ||
| newStatus: AgentStatus.IDLE, | ||
| timestamp: Date.now(), | ||
| } as AgentStatusChangeEvent); |
There was a problem hiding this comment.
[Suggestion] The replayed IDLE settlement fires the leader report (fire-and-forget reportFinalAnswer → sendMessage → writeMessage) synchronously inside setupEventBridge, i.e. before spawnTeammate persists the team file via writeTeamFile. If writeTeamFile then rejects, rollback() splices the member out and stops the agent, but cannot retract the already-started inbox write — the leader inbox still receives a "{name} completed a turn" report from a teammate the persisted team never contained, and any follow-up send_message to that name throws Teammate not found. The replaced flush-only reconciliation was rollback-safe, so this exposure is introduced by this diff. Verified by fault injection: mocking writeTeamFile to reject with EACCES, spawnTeammate threw and rollback ran (in-memory members []), yet the leader inbox still received the phantom teammate's report. Consider deferring the replay's report emission until after writeTeamFile succeeds — e.g. have setupEventBridge return the pending reconciliation event and fire it in spawnTeammate after the write (live subscriptions are already in place, so no event is lost) — or have the report re-check membership before writing. A harness case that completes the initial round pre-attach, forces writeTeamFile to throw, and asserts getLeaderMessages() stays empty must go red if the deferral is removed.
中文说明
[Suggestion] 回放的 IDLE 结算在 setupEventBridge 内部同步触发 leader 上报（fire-and-forget 的 reportFinalAnswer → sendMessage → writeMessage），也就是在 spawnTeammate 通过 writeTeamFile 持久化团队文件之前。如果 writeTeamFile 随后失败，rollback() 会把成员移除并停止 agent，但无法撤回已经开始的收件箱写入 —— leader 收件箱仍会收到一条来自"持久化团队中根本不存在的 teammate"的"{name} completed a turn"报告，之后任何对该名字的 send_message 都会抛出 Teammate not found。被替换的仅刷新对账是回滚安全的，因此这一暴露面是本 diff 引入的。已通过故障注入验证：把 writeTeamFile mock 成以 EACCES 拒绝，spawnTeammate 抛错、回滚执行（内存成员为 []），但 leader 收件箱仍然收到了幻影 teammate 的报告。建议把回放的上报推迟到 writeTeamFile 成功之后 —— 例如让 setupEventBridge 返回待执行的对账事件，由 spawnTeammate 在写文件成功后触发（此时实时订阅已就位，不会丢事件）—— 或者让上报在写入前复查成员资格。验收测试：初始回合在 attach 前完成、强制 writeTeamFile 抛错、断言 getLeaderMessages() 为空；移除延迟逻辑必须让该测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
Valid — deferred to the next round (this round's budget was 4 findings: the Critical fix, the missing tests, the diagnostics, and the dedup). Deferring the replay's report emission until after writeTeamFile needs a decision on how the deferred replay interacts with live events in the spawn window (e.g. a staleness guard if a live round starts while the file write is in flight), plus the fault-injection harness case forcing writeTeamFile to reject — leaving this thread open to track it.
…result Squashed replay of two checkpoint commits (git transport down; pushed via git-data API). Commit 1 — fix(core): clear explicit leader-report flag when seeding pre-attach result The pre-attach seed restores pendingFinalReports but left explicitLeaderReports untouched, while the live onRoundText handler it mirrors clears that flag on every non-empty round text. A teammate following the default initialTask prompt calls send_message(to: "leader") mid-round — sendMessage sets the flag synchronously, no event bridge needed — so when the initial round completed entirely before attach, the replayed IDLE settlement saw explicitlyReported === true and skipped the recovered answer, leaving the leader with zero automatic reports of the initial result. Clear the flag when seeding (mirroring onRoundText's semantics) and add diagnostics for the history-recovery and IDLE-replay paths so recovered pre-attach reports are distinguishable from live ones in debug logs. Adds regression coverage for the explicit pre-attach send, for last-visible-answer selection across multiple pre-attach round texts, and for thought-only trailing events (internal reasoning must never be reported as the final answer). Commit 2 — refactor(core): share last-visible-answer scan between team and arena TeamManager.lastVisibleAnswer and ArenaManager.getFinalTextFromTranscript were two implementations of the same rule — walk a message history backwards, return content.trim() of the most recent non-empty, non-thought assistant entry. Any future semantic change would have to land in both places, and missing one would make team pre-attach recovery and the arena final-text fallback silently disagree on identical histories. Extract the scan as lastVisibleAnswer() next to AgentMessage in agent-types.ts (ArenaTranscriptEntry is a field-for-field map of AgentMessage, so the helper applies to both) and delegate from both call sites. Behavior is unchanged; the new initial-result suite and the existing ArenaManager tests stay green. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 44 passed · 0 failed · 44 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：44 通过 · 0 失败 · 44 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10245 — fix(core): report teammate result settled before event bridge attachVerdict: 中文摘要
Central claim + A/BThe PR exists to fix one behavior: when an in-process teammate's initial round A/B was run in a scratch
Both halves are load-bearing; neither alone restores the report. The single Mutation matrix (each new guard killed by its own test)
Each single-point mutant is caught by exactly the test written to pin it and Boundary probes (compiled dist, mock-free)
Arena refactor equivalence
Gates
CorrectionsThe PR body's CI note claims a pre-existing duplicate- Not covered
MethodologyEnvironment: CI merge-ref checkout ( Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Re-approving at the current head (fbd6e76). My earlier execution-based review covered c387127 (initial-result recovery 3/3, surrounding team suites 79/79); the single commit since is a safe consolidation plus a small, documented, tested addition: lastVisibleAnswer is extracted to agent-types.ts and shared with ArenaManager's final-text fallback (identical selection rule, behavior preserved), and the pre-attach recovery now clears explicitLeaderReports to mirror the live onRoundText supersede semantics — covered by three new tests (explicit-leader-message pre-attach, multi-turn last-visible-text, thought-only fallback). No blocking (Critical) issues in the delta. The bot has approved at this head and the test lanes are green; concur.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): src/agents/team/TeamManager.initial-result.test.ts — no such file or directory; 440 tests green — this review observed 21981, 1743, 25103, 1667, 605, 4329, 638 passed; 1682 passed — this review observed 21981, 1743, 25103, 1667, 605, 4329, 638 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/agents/team/TeamManager.ts:1752 — [review] Seed block hand-copies onRoundText's supersession pair; extract one shared closurepackages/core/src/agents/team/TeamManager.ts:1752 — [probe] RUNNING-at-attach seed path is protected by zero tests (mutation survives all six)
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：src/agents/team/TeamManager.initial-result.test.ts — no such file or directory; 440 tests green — this review observed 21981, 1743, 25103, 1667, 605, 4329, 638 passed; 1682 passed — this review observed 21981, 1743, 25103, 1667, 605, 4329, 638 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| onStatusChange({ | ||
| agentId, | ||
| previousStatus: AgentStatus.RUNNING, | ||
| newStatus: AgentStatus.IDLE, | ||
| timestamp: Date.now(), | ||
| } as AgentStatusChangeEvent); |
There was a problem hiding this comment.
[Suggestion] R1-2: Still standing from round 1 (an independent round-2 rediscovery is merged into this re-report): the replayed IDLE settlement fires the leader report (fire-and-forget reportFinalAnswer → sendMessage → writeMessage) synchronously inside setupEventBridge, i.e. before spawnTeammate persists the team file via writeTeamFile. If writeTeamFile then rejects, rollback() splices the member out and stops the agent but cannot retract the already-started inbox write — the leader inbox receives a "{name} completed a turn" report from a teammate the persisted team never contained, and any follow-up send_message to that name throws "Teammate not found". The replaced flush-only reconciliation was rollback-safe, so this exposure was introduced by the replay. It takes two independent rare events (the pre-attach IDLE race this PR fixes, plus a team-file write failure), but given both the phantom is deterministic — reproduced again this round by fault injection:
Probe: writeTeamFile mocked to throw EACCES after a pre-attach IDLE settlement
spawnTeammate rejected; rollback removed the member (roster assertion passed)
leader inbox still received: [{"from":"phantom","text":"initial result",...}]
Consider deferring the replay's report emission until after writeTeamFile succeeds — e.g. have setupEventBridge return the pending reconciliation event and fire it in spawnTeammate after the write (live subscriptions are already in place, so no event is lost) — or gate the automatic report in sendMessage's leader branch on from matching a current team member. Fix acceptance: a harness case that completes the initial round pre-attach, forces writeTeamFile to throw, and asserts getLeaderMessages() stays empty must go red if the deferral/membership guard is removed — please add it and verify by removing the guard.
中文说明
[Suggestion] R1-2：第 1 轮遗留（第 2 轮的独立重新发现已并入本条重发）：回放的 IDLE 结算在 setupEventBridge 内部同步触发 leader 上报（fire-and-forget 的 reportFinalAnswer → sendMessage → writeMessage），也就是在 spawnTeammate 通过 writeTeamFile 持久化团队文件之前。如果 writeTeamFile 随后失败，rollback() 会把成员移除并停止 agent，但无法撤回已经开始的收件箱写入 —— leader 收件箱仍会收到一条来自"持久化团队中根本不存在的 teammate"的"{name} completed a turn"报告，之后任何对该名字的 send_message 都会抛出 "Teammate not found"。被替换的仅刷新对账是回滚安全的，因此这一暴露面是回放引入的。触发需要两个独立的罕见事件（本 PR 修复的 pre-attach IDLE 竞态 + 团队文件写入失败），但两者同时发生时幻影上报是确定性的 —— 本轮故障注入再次复现：把 writeTeamFile mock 成抛出 EACCES 后，spawnTeammate 拒绝、回滚移除了成员，但 leader 收件箱仍收到了幻影报告。
建议把回放的上报推迟到 writeTeamFile 成功之后 —— 例如让 setupEventBridge 返回待执行的对账事件，由 spawnTeammate 在写文件成功后触发（此时实时订阅已就位，不会丢事件）—— 或者让 sendMessage 的 leader 分支在 from 不是当前团队成员时跳过自动上报。修复验收：初始回合在 attach 前完成、强制 writeTeamFile 抛错、断言 getLeaderMessages() 为空的测试用例，在移除延迟/成员资格守卫时必须变红 —— 请补充该用例并通过移除守卫验证。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| for (let i = messages.length - 1; i >= 0; i--) { | ||
| const message = messages[i]!; | ||
| if (message.role !== 'assistant' || message.thought) continue; |
There was a problem hiding this comment.
[Suggestion] lastVisibleAnswer's role !== '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.setupStateListeners pushes user, tool_call and tool_result entries into the same history, and at attach time a RUNNING teammate mid-round can legitimately have a tool_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:
Mutation `if (message.thought) continue;` (role check dropped):
all 150 existing tests still passed (6 + 88 + 28 + 28)
mixed-role probe flipped 4/4 green -> 1/4:
expected 'Tool read_file succeeded' to be 'draft answer'
expected 'Tool shell succeeded' to be undefined
expected 'INITIAL TASK PROMPT — report via send…' to be undefined
Add a small direct unit test for the exported lastVisibleAnswer with 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 trailing tool_result text) — 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)
… telemetrySwap test) PR CI was red solely due to the pre-existing TS1117 duplicate getToolRegistry in client.telemetrySwap.test.ts (introduced by #10220, fixed by #10243 which is now merged). Syncing main to turn CI green without touching the fix commits. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- RUNNING-at-attach seed path unprotected by tests — already reported in the round-2 deferral list (review 5040396832)
Test Plan (not a blocker): src/agents/team/TeamManager.initial-result.test.ts — no such file or directory; 440 tests green — this review observed 22069, 1776, 25342, 1667, 605, 4331, 638 passed; 1682 passed — this review observed 22069, 1776, 25342, 1667, 605, 4331, 638 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/agents/team/test-utils/fake-agent.ts:104 — [review] ROUND_TEXT→history append rule now hand-copied in three places (FakeAgent mirror can silently diverge from AgentCore)
中文说明
已审查——无阻断问题。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：src/agents/team/TeamManager.initial-result.test.ts — no such file or directory; 440 tests green — this review observed 22069, 1776, 25342, 1667, 605, 4331, 638 passed; 1682 passed — this review observed 22069, 1776, 25342, 1667, 605, 4331, 638 passed。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| onStatusChange({ | ||
| agentId, | ||
| previousStatus: AgentStatus.RUNNING, | ||
| newStatus: AgentStatus.IDLE, | ||
| timestamp: Date.now(), | ||
| } as AgentStatusChangeEvent); |
There was a problem hiding this comment.
[Suggestion] R1-2: Still standing from round 1 (re-checked against this head — the ordering is unchanged since): the replayed IDLE settlement fires the leader report (fire-and-forget reportFinalAnswer → sendMessage → writeMessage) synchronously inside setupEventBridge, i.e. before spawnTeammate persists the team file via writeTeamFile. If writeTeamFile then rejects, rollback() splices the member out and stops the agent but cannot retract the already-started inbox write — the leader inbox receives a "{name} completed a turn" report from a teammate the persisted team never contained, and any follow-up send_message to that name throws "Teammate not found". The replaced flush-only reconciliation was rollback-safe, so this exposure was introduced by the replay. It takes two independent rare events (the pre-attach IDLE race this PR fixes, plus a team-file write failure), but given both the phantom is deterministic — reproduced in round 2 by fault injection:
Probe: writeTeamFile mocked to throw EACCES after a pre-attach IDLE settlement
spawnTeammate rejected; rollback removed the member (roster assertion passed)
leader inbox still received: [{"from":"phantom","text":"initial result",...}]
Consider deferring the replay's report emission until after writeTeamFile succeeds — e.g. have setupEventBridge return the pending reconciliation event and fire it in spawnTeammate after the write (live subscriptions are already in place, so no event is lost) — or gate the automatic report in sendMessage's leader branch on from matching a current team member. Fix acceptance: a harness case that completes the initial round pre-attach, forces writeTeamFile to throw, and asserts getLeaderMessages() stays empty must go red if the deferral/membership guard is removed — please add it and verify by removing the guard.
中文说明
[Suggestion] R1-2：第 1 轮遗留（已对照本 head 复核——顺序自那以后未变）：回放的 IDLE 结算在 setupEventBridge 内部同步触发 leader 上报（fire-and-forget 的 reportFinalAnswer → sendMessage → writeMessage），也就是在 spawnTeammate 通过 writeTeamFile 持久化团队文件之前。如果 writeTeamFile 随后失败，rollback() 会把成员移除并停止 agent，但无法撤回已经开始的收件箱写入——leader 收件箱仍会收到一条来自"持久化团队中根本不存在的 teammate"的"{name} completed a turn"报告，之后任何对该名字的 send_message 都会抛出 "Teammate not found"。被替换的仅刷新对账是回滚安全的，因此这一暴露面是回放引入的。触发需要两个独立的罕见事件（本 PR 修复的 pre-attach IDLE 竞态 + 团队文件写入失败），但两者同时发生时幻影上报是确定性的——第 2 轮故障注入已复现：把 writeTeamFile mock 成抛出 EACCES 后，spawnTeammate 拒绝、回滚移除了成员，但 leader 收件箱仍收到了幻影报告。
建议把回放的上报推迟到 writeTeamFile 成功之后——例如让 setupEventBridge 返回待执行的对账事件，由 spawnTeammate 在写文件成功后触发（此时实时订阅已就位，不会丢事件）——或者让 sendMessage 的 leader 分支在 from 不是当前团队成员时跳过自动上报。修复验收：初始回合在 attach 前完成、强制 writeTeamFile 抛错、断言 getLeaderMessages() 为空的测试用例，在移除延迟/成员资格守卫时必须变红——请补充该用例并通过移除守卫验证。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const message = messages[i]!; | ||
| if (message.role !== 'assistant' || message.thought) continue; |
There was a problem hiding this comment.
[Suggestion] R2-1: Still standing from round 2 (re-checked at this head — code and tests unchanged since): lastVisibleAnswer's role !== '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.setupStateListeners pushes user, tool_call and tool_result entries into the same history, and at attach time a RUNNING teammate mid-round can legitimately have a tool_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 in round 2:
Mutation: role check dropped (`if (message.thought) continue;`)
all 150 existing tests still passed (6 + 88 + 28 + 28)
mixed-role probe flipped 4/4 green -> 1/4:
expected 'Tool read_file succeeded' to be 'draft answer'
expected 'Tool shell succeeded' to be undefined
expected 'INITIAL TASK PROMPT — report via send…' to be undefined
Add a small direct unit test for the exported lastVisibleAnswer with 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 trailing tool_result text) — please add it and verify by removing the check.
中文说明
[Suggestion] R2-1：第 2 轮遗留（已对照本 head 复核——代码与测试自那以后未变）：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。第 2 轮已用变异验证：删除 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)
|
Released in v0.22.3. |
4 participants
What this PR does
Fixes an Agent Team spawn-ordering gap: when an in-process teammate's initial round completes before
TeamManagerattaches its event bridge, the round's final answer and IDLE settlement were lost. The bridge now recovers round text emitted before attachment from the agent's message history and replays the missed IDLE settlement through the existing handler, so the initial round is reported to the leader exactly once.Why it's needed
spawnTeammate()awaitsbackend.spawnAgent()before callingsetupEventBridge(), andAgentEventEmitterdoes not buffer events for late subscribers. The in-process backend'sspawnAgent()resolves while the initial run loop is still executing (startInner()kicks off the loop without awaiting it), so a fast initial round can emit its finalROUND_TEXTand reach IDLE before the bridge subscribes. The existing late-idle reconciliation only flushes pending messages — it neither replays the completed round nor reports its result — so the leader silently loses the teammate's initial answer. Follow-up of the #10074 lifecycle audit, split out as #10211.Reviewer Test Plan
How to verify
New focused suite
packages/core/src/agents/team/TeamManager.initial-result.test.ts, built on the existing FakeBackend/FakeAgent harness:spawnAgent()(before the bridge attaches) → the leader inbox stays empty; the first two tests fail onmainwithexpected [] to deeply equal [{ text: 'initial result' }].Commands (from
packages/core):npx vitest run src/agents/team/TeamManager.initial-result.test.tsfor the repro/regression suite,npx vitest run src/agents/team src/agents/backendsfor the surrounding regression surface (440 tests green locally),npx vitest run src/agentsfor the full agents area (1682 passed, 6 skipped).tsc --noEmiton packages/core is clean for all files touched here; the only reported error is the pre-existing duplicategetToolRegistryproperty inclient.telemetrySwap.test.tsthat #10220 introduced on main (present without this PR).Evidence (Before & After)
N/A — internal coordination logic, covered by the deterministic unit tests above (pre-fix failure output:
AssertionError: expected [] to deeply equal [ ObjectContaining{ text: 'initial result' } ]).Tested on
Environment (optional)
Unit tests only (vitest), Node v24 on Linux.
Risk & Scope
getMessages?()is an optionalTeamAgentHandleaddition satisfied by both existing in-process handles (AgentInteractive already implements it).Linked Issues
Fixes #10211
Related: #10074
中文说明
本 PR 做了什么
修复 Agent Team spawn 顺序窗口：当 in-process teammate 的初始回合在
TeamManager安装 event bridge 之前就完成时，该回合的最终回答和 IDLE 事件会丢失。现在 bridge 在安装时会从 agent 的消息历史中恢复 attach 之前发出的回合文本，并通过既有处理器回放错过的 IDLE 事件，使初始回合恰好向 leader 报告一次。为什么需要
spawnTeammate()先等待backend.spawnAgent()再调用setupEventBridge()，而AgentEventEmitter不为迟到的订阅者缓冲事件。in-process backend 的spawnAgent()在初始 run loop 还在运行时就返回（startInner()启动 loop 但不等待它完成），因此一个足够快的初始回合可能在 bridge 订阅之前就发出最终ROUND_TEXT并进入 IDLE。现有的迟到 idle 对账只刷新 pending 消息，既不回放已完成回合、也不上报其结果，leader 会静默丢失 teammate 的初始答案。这是 #10074 生命周期审计的后续项，按 triage 要求拆分为 #10211。审阅者测试方案
如何验证
新增聚焦测试
packages/core/src/agents/team/TeamManager.initial-result.test.ts，基于现有 FakeBackend/FakeAgent 测试架：spawnAgent()内部（bridge attach 之前）发出最终回合文本并进入 IDLE → leader 收件箱为空；前两个测试在main上以expected [] to deeply equal [{ text: 'initial result' }]失败。命令（在
packages/core下）：npx vitest run src/agents/team/TeamManager.initial-result.test.ts跑复现/回归套件；npx vitest run src/agents/team src/agents/backends跑周边回归面（本地 440 个测试全绿）；npx vitest run src/agents跑整个 agents 目录（1682 通过、6 跳过）。packages/core 的tsc --noEmit对本 PR 改动的文件全部干净；唯一报错是 #10220 在 main 上引入的client.telemetrySwap.test.ts重复getToolRegistry属性（不带本 PR 也同样存在）。证据（修复前后对比）
N/A —— 内部协调逻辑，由上述确定性单元测试覆盖（修复前失败输出：
AssertionError: expected [] to deeply equal [ ObjectContaining{ text: 'initial result' } ]）。测试环境
运行环境（可选）
仅单元测试（vitest），Linux 上 Node v24。
风险与范围
getMessages?()是TeamAgentHandle的可选新增，两个现有 in-process handle 均满足（AgentInteractive 本就实现了它）。关联 Issue
Fixes #10211
相关：#10074