fix(core): disambiguate send_message destinations - #10083
Conversation
E2E / reproduction reportEnvironment: Windows PowerShell, Node.js v24.11.0, isolated worktree. Baseline reproduction
Post-fix verification
The all-workspace preflight test phase was not green on this Windows host because unrelated suites hit missing WSL |
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! This re-run reviews the rebased head Template looks good ✓ Problem: observed bug — #10073 reports the exact Direction: aligned. The issue triage endorsed exactly this shape — runtime rejection of ambiguous Size: core paths touched ( Approach: the scope is right. Two focused edits implementing exactly the triaged direction — no unrelated changes, no drive-by refactors. It reuses the existing Risk: no elevated risk signals (none of the changed files match the revert-correlated path list). One thing worth knowing for the review below: this rebase lands on top of #10158, so the Stage 2 comment pays specific attention to that interaction. Moving on to code review. 🔍 中文说明感谢贡献！本次 re-run 审查的是 rebase 后的 head 模板完整 ✓ 问题：已观测到的 bug——#10073 报告了真实 Agent Team 会话中出现的 方向：对齐。issue 分诊明确认可了这个形态——运行时拒绝同时携带 规模：触及核心路径（ 方案：范围合理。两处聚焦的修改恰好实现分诊认可的方向——无夹带改动、无顺手重构。复用既有 风险：无升级风险信号（改动文件均未命中与 revert 相关的高风险路径）。一个审查时需要知道的点：本次 rebase 落在 #10158 之上，因此 Stage 2 评论专门审查了二者的交互。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal for this issue, written before reading the diff: reject both-field calls through the standard What I verified on this rebased head:
No blockers. Known non-blocking gaps, all in the same class of confusing diagnostic that #10073 is about, and all observable now that the hint is live: TestingThis comment carries: the maintainer's two verification rounds cited in this thread (round 1 macOS, round 2 Linux — wire-level A/B evidence on a patch proven identical to this head), a static review of the #10158 interaction, and the CI snapshot below. This is an unattended CI re-run, so no tmux was driven here; nothing below is the author's self-report. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 CI is running on the rebased head right now — the first execution of this commit. The earlier red on The central behavioural claim is already substantiated beyond what CI alone could give — round 2 ran this exact patch through a real Agent Team and checked delivery on the wire. What remains unsettled is only the same patch on the #10158 base, which the running suite exercises at the test level. Sandboxed verification would settle it end-to-end: 中文说明代码审查我在看 diff 之前的独立提案：用标准 在这个 rebase 后的 head 上核实的内容：
无阻塞项。已知的非阻塞缺口，均属 #10073 所针对的同类误导性诊断，且提示语可达后均可观测： 测试本评论携带的证据：本线程中维护者的两轮验证（第 1 轮 macOS、第 2 轮 Linux——在与本 head 证明逐行一致的补丁上做的链路级 A/B）、针对 #10158 交互的静态审查，以及下方 CI 快照。这是无人值守的 CI re-run，未在此驱动 tmux；以下不含任何作者自述结果。 CI 正在 rebase 后的 head 上运行——这是该提交的首次执行。此前 核心行为声明已得到超出 CI 本身的证实——第 2 轮用这个完全相同的补丁驱动了真实 Agent Team 并在链路上核对了投递。尚未落定的只剩同一补丁在 #10158 基线上的表现，正在运行的套件会在测试层面覆盖它。沙盒验证可以端到端落定： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — this is the exact patch @wenshao verified end-to-end in round 2, rebased onto a moving Stepping back: my last pass deferred at 3/5 for one reason — the behavioural claim had no evidence behind it. That gap has since been closed thoroughly: round 1 found the one real defect (the hint inert because the scheduler forwards only Reservations, none of them blocking: the three hint gaps named in Stage 2 (leader, one-directional, and the new peer-session case) should land in a follow-up issue now that the hint is live. And #10090 remains open as a competing fix for the same issue — whichever lands should close the other; on the evidence in this thread, this one is further along. Verdict: approve. CI is still running on 中文说明置信度：4/5 —— 这正是 @wenshao 第 2 轮端到端验证过的补丁，rebase 到移动中的 退后一步看：上一轮我以 3/5 暂缓，唯一原因是行为声明没有证据支撑。这个缺口此后已被彻底补上：第 1 轮找到了唯一的真实缺陷（提示语失效——调度器对带 保留意见，均不阻断：Stage 2 点名的三个提示语缺口（leader、单向、以及新增的 peer 会话场景）应落入一个后续 issue——提示语可达之后它们都是可观测的。另外 #10090 作为同一 issue 的竞争修复仍然 open——无论哪个合入都应关闭另一个；就本线程的证据而言，本 PR 走得更远。 结论：批准。CI 仍在 — 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.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 5": running send-message.test.ts to confirm the suite is green — the worktree has no node_modules or built dist/ , and npm ci + npm run build exceeded the ….
Test Plan (not a blocker): 23 tests passed — this review observed 21609, 1702, 24740, 1659, 601, 4235, 630 passed.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 5"：running send-message.test.ts to confirm the suite is green — the worktree has no node_modules or built dist/ , and npm ci + npm run build exceeded the …。
Test Plan（非阻断）：23 tests passed — this review observed 21609, 1702, 24740, 1659, 601, 4235, 630 passed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| getTeamManager: () => | ||
| ({ | ||
| getTeamFile: () => ({ members: [{ name: 'qa-reviewer' }] }), | ||
| }) as ReturnType<Config['getTeamManager']>, |
There was a problem hiding this comment.
[Suggestion] Swapping this fixture from getTeamManager: () => null to a non-null stub removed the only test coverage of the new hint code's no-team branch (teamManager ? findMemberByName(...) : undefined in send-message.ts). After the swap, no test exercises a task_id miss while no team is active — the production-common solo-session path — so a follow-up edit that drops the null guard would ship green.
A mutation probe confirmed the gap: removing the teamManager ? guard leaves all 23 existing tests green, while a null-manager {task_id: 'nope', message} call then crashes into EXECUTION_FAILED instead of returning SEND_MESSAGE_NOT_FOUND:
AssertionError: expected 'execution_failed' to be 'send_message_not_found'
Keep the stub for the hint test, but add one not-found case with a null team manager: build a config with getTeamManager: () => null, call validateBuildAndExecute({ task_id: 'nope', message: 'hello' }, ...), and expect ToolErrorType.SEND_MESSAGE_NOT_FOUND with 'No background task found' and no hint.
中文说明
将此处 fixture 从 getTeamManager: () => null 换成非空 stub，移除了新增提示代码"无团队"分支（send-message.ts 中的 teamManager ? findMemberByName(...) : undefined）的唯一测试覆盖。替换之后，没有任何测试覆盖"无活动团队时 task_id 未命中"这一场景——而这正是生产上常见的单人会话路径——因此后续若有编辑移除空值保护，测试仍会全绿通过。
变异探针确认了该缺口：移除 teamManager ? 保护后，现有 23 个测试仍全部通过，而空团队管理器下的 {task_id: 'nope', message} 调用会崩溃为 EXECUTION_FAILED，而不是返回 SEND_MESSAGE_NOT_FOUND：
AssertionError: expected 'execution_failed' to be 'send_message_not_found'
建议保留该 stub 用于提示测试，但补充一个空团队管理器的未命中用例：构造 getTeamManager: () => null 的 config，调用 validateBuildAndExecute({ task_id: 'nope', message: 'hello' }, ...)，断言 ToolErrorType.SEND_MESSAGE_NOT_FOUND、包含 'No background task found' 且不含提示文本。
— qwen3.8-max via Qwen Code /review (v0.22.0)
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): 23 tests passed — this review observed 21610, 1702, 24741, 1659, 601, 4235, 630 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/send-message.ts:341 — [review] to / task_id mutual exclusivity not documented in the schema descriptionspackages/core/src/tools/send-message.ts:101 — [review] Teammate hint can fire for terminated teammates, steering into a guaranteed-failing retry
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：23 tests passed — this review observed 21610, 1702, 24741, 1659, 601, 4235, 630 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| returnDisplay: teammate | ||
| ? `Task not found; use "to" for teammate "${teammate.name}".` | ||
| : 'Task not found.', |
There was a problem hiding this comment.
[Suggestion] This new teammate-hint branch of returnDisplay has no test assertion — the one-line mutation replacing this ternary with 'Task not found.' survives the whole suite. I confirmed this with a mutation probe in a scratch tree: baseline send-message.test.ts is 24/24 green, the mutant is 24/24 green as well, and adding the assertion below against the mutant turns it red (× suggests the teammate destination for a matching task ID → expected 'Task not found.' to contain 'use "to" for teammate "qa-reviewer"'). So if a later change or revert drops the user-facing half of the hint, every test stays green while the TUI silently loses the disambiguation guidance this PR exists to surface — the user sees a bare "Task not found." with no pointer to the to: field. With the assertion added and the PR code restored the suite is 24/24 green again.
The assertion belongs in the suggests the teammate destination for a matching task ID test in send-message.test.ts:
expect(result.returnDisplay).toContain('use "to" for teammate "qa-reviewer"');中文说明
returnDisplay 中新增的 teammate 提示分支没有任何测试断言——把这个三元表达式直接替换为 'Task not found.' 的单行突变可以在整套测试中存活。我在独立的 scratch 工作树中做了突变探测验证：基线 send-message.test.ts 为 24/24 全绿；应用该突变后同样 24/24 全绿；对突变体加入下面的断言后变红（× suggests the teammate destination for a matching task ID → expected 'Task not found.' to contain 'use "to" for teammate "qa-reviewer"'）。因此，如果后续改动（或回退）删掉了提示中面向用户的这一半，所有测试仍会是绿的，而 TUI 会悄悄丢失本 PR 要呈现的歧义消除指引——用户只会看到一个没有任何 to: 字段指引的 "Task not found."。加入下面的断言并恢复 PR 代码后，重新回到 24/24 全绿。
在 send-message.test.ts 的 suggests the teammate destination for a matching task ID 测试中加入该断言：
expect(result.returnDisplay).toContain('use "to" for teammate "qa-reviewer"');— qwen3.8-max via Qwen Code /review (v0.22.0)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 5": none — no check was cut short by the tool budget; the only unfinished check (running the suite) was blocked by the environment, as disclosed above..
Test Plan (not a blocker): 23 tests passed — this review observed 21610, 1702, 24740, 1659, 601, 4235, 630 passed.
Convergence: round 3 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 5"：none — no check was cut short by the tool budget; the only unfinished check (running the suite) was blocked by the environment, as disclosed above.。
Test Plan（非阻断）：23 tests passed — this review observed 21610, 1702, 24740, 1659, 601, 4235, 630 passed。
收敛情况：第 3 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| expect(result.returnDisplay).toContain( | ||
| 'use "to" for teammate "qa-reviewer"', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This round adds returnDisplay coverage to the positive hint test, but its two negative siblings — returns error for non-existent task and returns error for non-existent task without an active team — still assert only on llmContent/error type, leaving the no-hint returnDisplay ('Task not found.') unpinned. Production builds both surfaces from the same teammate ternary in send-message.ts; if a future refactor unconditionally formats the hint into returnDisplay (the ternary condition dropped, or the hint hoisted into a shared formatter), the UI shows a teammate hint precisely when no teammate matches or no team exists — the misleading display this PR exists to prevent — and the whole suite stays green because only the positive case inspects returnDisplay. I confirmed this with a mutation probe in a scratch tree at this commit: dropping the ternary so returnDisplay always emits the hint leaves all 24 tests green (the mutation survives); adding the assertions suggested below makes exactly the two negative tests flip red; with the mutation reverted and the assertions kept the suite is green again, so they pin real behaviour without false-failing.
Consider adding display assertions to both no-hint tests, e.g.:
expect(result.returnDisplay).toContain('Task not found.');
expect(result.returnDisplay).not.toContain('use "to"');Fix witness: mutate the not-found branch in send-message.ts to always emit the hint-formatted returnDisplay — with the added assertions the two negative tests must go red (without them that mutation survives today).
中文说明
本轮为正向提示测试新增了 returnDisplay 断言，但它的两个负向兄弟测试（returns error for non-existent task 和 returns error for non-existent task without an active team）仍然只断言 llmContent/错误类型，未对无提示时的 returnDisplay（'Task not found.'）形成约束。生产代码中两个界面来自 send-message.ts 里同一个 teammate 三元表达式；如果未来重构把提示无条件地写入 returnDisplay（去掉三元条件，或把提示移入共享的格式化逻辑），那么在没有匹配 teammate 或没有团队时界面也会显示 teammate 提示——这正是本 PR 要防止的误导性显示——而整个测试套件仍会全绿，因为只有正向用例检查了 returnDisplay。我在本提交的临时工作树中用变异探针确认了这一点：去掉三元表达式使 returnDisplay 始终输出提示时，24 个测试仍全部通过（变异存活）；加上下方建议的断言后，恰好那两个负向测试变红；还原变异并保留断言后套件再次全绿，说明断言约束的是真实行为、不会误报。
建议在两个无提示测试中各加一条显示断言，例如：
expect(result.returnDisplay).toContain('Task not found.');
expect(result.returnDisplay).not.toContain('use "to"');修复见证：将 send-message.ts 的 not-found 分支变异为始终输出带提示的 returnDisplay——加上断言后那两个负向测试必须变红（目前不加断言时该变异可以存活）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Test Plan (not a blocker): 23 tests passed — this review observed 21610, 1702, 24736, 1659, 601, 4235, 630 passed.
中文说明
未发现问题。LGTM！✅
Test Plan（非阻断）：23 tests passed — this review observed 21610, 1702, 24736, 1659, 601, 4235, 630 passed。
— qwen3.8-max via Qwen Code /review (v0.22.2)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- Teammate hint can fire for terminated teammates (send-message.ts:101) — already reported (round 2 deferral, review 5029062877)
Test Plan (not a blocker): 23 tests passed — this review observed 21610, 1702, 24740, 1659, 601, 4235, 630 passed.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/send-message.ts:102 — [probe] disambiguation hint can never fire for leader (TeamFile.members excludes the leader)packages/core/src/tools/send-message.ts:108 — [probe] corrective hint is one-directional; mirror confusion (background-task ID passed as to ) gets no task_id hint
中文说明
已审查——无阻断问题。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：23 tests passed — this review observed 21610, 1702, 24740, 1659, 601, 4235, 630 passed。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Maintainer verification — local real-stack run on macOSI built and ran this PR end-to-end locally to supply the evidence the triage bot flagged as missing (CI never executed on this commit, and the PR self-reports macOS as untested). Tree under test: Three arms, identical trees except one file:
Harness, raw ledgers and full-size images: https://github.com/wenshao/qwen-code/tree/assets-pr10083 Verdict
Finding 1 — the disambiguation half works, and #10073 reproduces on current
|
| check | result |
|---|---|
packages/core full suite |
617 files / 22271 tests passed, 28 skipped, exit 0 (71s) |
send-message.test.ts focused |
24/24 (23 in the PR + 1 arriving from main) |
npm run typecheck (all workspaces) |
pass |
eslint + prettier --check on both changed files |
pass |
merge into current main |
clean, no semantic conflict with #10081, which touched the same file |
Mutation matrix (6 mutants over the fix): 6/6 killed — the tests do discriminate at the unit level. Worth reading alongside Finding 2 though: the three mutants covering the hint (M3 hint dropped, M4 returnDisplay wording, M5 un-normalized name) are killed only by assertions on that caller-less method, so a green matrix here does not imply the behaviour ships.
Merge reference
The disambiguation is real, correctly placed, and the smallest version of the fix — that half I'd take as-is. The hint half is dead code as written and should either move into error.message (one line, above) or come out.
One more input for the decision: #10090 is a competing fix for the same issue #10073, opened 36 minutes after this one. I verified it the same way; its disambiguation half also works, and its hint half fails for the identical reason. Whichever lands, the other should close, and the surviving one should carry the error.message change.
中文说明
维护者验证 —— macOS 上的本地真实栈实跑
我在本地构建并端到端跑通了这个 PR，用来补上分诊机器人指出的证据缺口（CI 从未在该提交上执行过，且 PR 自述 macOS 未测试）。
被测树： origin/main bfda082 + PR head 99e47d5 本地合并 —— ort 干净合并，有效 diff 仍是 97+/4-。
主机： macOS 26.6.2（arm64）、Node v24.18.1、npm 11.16.0。
方法： 以下全部经由打包后的 CLI（npm run build && npm run bundle → dist/cli.js）、隔离的 QWEN_HOME、QWEN_CODE_ENABLE_AGENT_TEAM=1，以及一个脚本化的假 OpenAI provider，驱动一个真实 Agent Team：team_create → 用 agent({name}) 拉起具名队友 qa-reviewer → 真实后台任务 → 10 个 send_message 探针。判据取自链路上抓到的、模型实际收到的 role: "tool" 帧。
三个臂，除一个文件外树完全相同：
| 臂 | bundle |
|---|---|
base |
合并树，仅把 send-message.ts 的生产 hunk 回退到 origin/main |
head |
合并树原样 |
witness |
head + 一行改动（见发现 2） |
harness、原始流水账与大图：https://github.com/wenshao/qwen-code/tree/assets-pr10083
结论
| 声明 | 结果 | |
|---|---|---|
| 1 | 同时给 to + task_id 会在任一分发路径前被拒绝 |
✅ 生产链路确认有效 |
| 2 | task_id 未命中且撞上队友名时提示改用 to |
❌ 生产上完全失效 —— 模型和用户都看不到 |
| 3 | 单目的地调用行为不变 | ✅ 10 个探针无回归 |
| 4 | macOS 上 packages/core 套件 / typecheck / lint / prettier |
✅ 全绿 |
发现 1 —— 消歧这一半有效，且 #10073 在当前 main 上可复现
base 精确复现了报告的 bug：调用同时带 to: "qa-reviewer" 和 task_id，工具报成功，消息却进了后台任务，队友会话什么也没收到。截图见英文部分：base 是静默误投（绿勾），head 是在 build() 内拒绝——早于注册表查找、早于任一分发路径。
投递与否是在链路上核的，不是只看文案：歧义 sentinel 在两个臂里都出现在 0 个非 leader 的模型请求中；而对照探针 send_message { to: "qa-reviewer" }（不带 task_id）出现在 1 个中——说明队友通道确实是活的，base 确实丢了 to。10 个探针里 base 与 head 只差这一行。
发现 2 —— 队友提示语在已发布的代码路径上不可达
这一条是 CI 无论如何也抓不到的，因为 PR 的测试打的是一个没有生产调用方的方法。
对 send_message { task_id: "QA Reviewer" }（名册里是 qa-reviewer），base 与 head 的结果逐字节相同——Task not found: QA Reviewer——链路上如此，TUI 上也如此。提示语从未出现。
机制在 packages/core/src/core/coreToolScheduler.ts：
ToolResult带error时，面向模型的载荷由createErrorResponse（L928）构造为response: { error: error.message }（L943）——llmContent被整体丢弃；- 面向用户的载荷是
resultDisplay: resultDisplay ?? error.message（L947），而错误分支在returnDisplay是字符串时传入undefined（L5744）——returnDisplay同样被丢弃。
本 PR 把提示语追加到 llmContent 与 returnDisplay，而未找到的结果是带 error 的。两半都被丢掉了。
测试之所以全绿，是因为 BaseDeclarativeTool.validateBuildAndExecute 返回的是工具自己的 ToolResult，llmContent 原样保留——而该方法没有任何生产调用点（grep -rn 'validateBuildAndExecute' packages --include='*.ts' | grep -v '\.test\.ts' 只命中它自己的定义）。于是 expect(result.llmContent).toContain(...) 能绿，而那段文案在真实链路上根本不存在。
建设性证明与修法。 一行见证补丁——把同一个提示语改追加到 error.message——提示语立刻出现在链路上（diff 见英文部分）。main 上已有这个约定：broadcast 失败那次改动（#10081）就把同一串同时写进 llmContent 和 error.message。
顺带说明：这也让两条被延后的评审发现（leader 不在 members、提示单向）变得无意义——我对二者都做了实测探针（task_id: "leader"，以及把后台任务 id 当 to 传），都不出提示；但按现状本来就没有任何提示可达。
发现 3 —— 单目的地路径无回归
在 head 上实测：只给 to 能投递（Message sent to "qa-reviewer".，且 sentinel 出现在队友自己的模型请求里）；只给 task_id 仍能排队；通用的任务未找到错误不变；无活动团队时的 task_id 未命中返回同样的错误，未在 getTeamManager() 空值守卫上崩溃。
发现 4 —— macOS 上的套件
| 检查 | 结果 |
|---|---|
packages/core 全量套件 |
617 文件 / 22271 测试通过，28 跳过，exit 0（71 秒） |
send-message.test.ts 聚焦 |
24/24（PR 的 23 条 + 从 main 合入的 1 条） |
npm run typecheck（全工作区） |
通过 |
两个改动文件的 eslint + prettier --check |
通过 |
合入当前 main |
干净，与同文件的 #10081 无语义冲突 |
变异矩阵（针对修复的 6 个变异体）：6/6 被杀——单测层面确有辨别力。但要和发现 2 一起读：覆盖提示语的三个变异体（M3 去掉提示、M4 returnDisplay 措辞、M5 不做名称规范化）只被那个无调用方方法上的断言杀死，所以矩阵全绿并不意味着该行为真的会随包发布。
合并参考
消歧是真实有效的，位置也放对了，而且是这个修复的最小形态——这一半我倾向原样接受。提示语那一半按现状是死代码，要么移进 error.message（就上面那一行），要么去掉。
还有一条决策输入：#10090 是同一 issue #10073 的竞争修复，比本 PR 晚 36 分钟提出。我用同样的方法验过它：它的消歧那一半同样有效，提示语那一半也因完全相同的原因失效。无论哪个合入，另一个都该关闭，并由存活的那个带上 error.message 的改动。
Verified locally by @wenshao at 99e47d5307b442504582a607ce9a198b46b4dc3a. Harness and raw evidence: https://github.com/wenshao/qwen-code/tree/assets-pr10083
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R6-1 to/task_id mutual exclusivity not stated in the schema descriptions (send-message.ts:341) — already reported (round 2 deferral, review 5029062877)
- R6-2 teammate hint can never fire for leader (TeamFile.members excludes the leader) (send-message.ts:102) — already reported (round 5 deferral, review 5042676420)
Not explored to full depth (tool budget reached): "agent 6b": running packages/core vitest for send-message.test.ts (worktree has no node_modules; installing would mutate the shared review tree and exceed budget) — ass….
中文说明
已审查。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 6b"：running packages/core vitest for send-message.test.ts (worktree has no node_modules; installing would mutate the shared review tree and exceed budget) — ass…。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Maintainer verification — round 2, on Linux, at
|
| claim | result | |
|---|---|---|
| 1 | round-1 Finding 2 (hint unreachable) is fixed by d5f1106c23 |
✅ resolved — confirmed on the wire and in the TUI |
| 2 | ambiguous to + task_id rejected before either dispatch path |
✅ confirmed, including when task_id is a live task |
| 3 | single-destination calls unchanged | ✅ no regression |
| 4 | red Test (ubuntu-latest) check |
main |
| 5 | packages/core suite / typecheck / lint / prettier on Linux |
✅ green (2 pre-existing env failures, proven at merge-base) |
Recommendation: merge, after a rebase onto main. Two residual hint gaps below are real but out of the PR's stated scope — they were listed as deferred in round 1 and were moot then because no hint was reachable; now that hints do reach the model, they are live again. Neither blocks.
Finding 1 — the hint now reaches both the model and the user
Round 1's central objection is closed. Same probe, both arms, at the new head:
base (send-message.ts at merge-base) — the exact diagnostic #10073 reported:
head (this PR) — hint present, ambiguous call rejected, and the hint is actionable:
Scenario C is the part worth calling out: the run follows the hint it was just given (to: "qa-reviewer"), the message is delivered, and the teammate reports back — so the suggestion is not just wording, it points at a destination that actually works.
The user-visible half is the screenshot; the model-visible half was checked off the wire, in the role: "tool" frame the provider actually received:
head arm — packages/core/.../send-message.ts as shipped
role: tool
content: Task not found: QA Reviewer. Did you mean to message teammate "qa-reviewer"?
If so, use `to: "qa-reviewer"` instead of `task_id`.
base arm — same probe, hint string appears in 0 of the model requests
validateBuildAndExecute still has no production call site (grep -rn 'validateBuildAndExecute' packages --include='*.ts' | grep -v '\.test\.ts' → only its own definition at tools.ts:364), so the llmContent / returnDisplay assertions remain assertions on a caller-less method. What changed is that d5f1106c23 also added an assertion on result.error?.message — the one field production forwards:
expect(result.error?.message).toContain('use `to: "qa-reviewer"` instead of `task_id`');That is the assertion that now carries the behaviour, and it is the one my M5 mutant is killed by.
Finding 2 — disambiguation holds, including the silent-misroute case
The strongest form of the bug is a both-field call where task_id names a live background task: on base the tool reports success and the teammate is never told.
base — green check, message queued to the background task, to silently dropped:
head — rejected in build(), before the registry lookup and before either dispatch path:
Non-delivery was checked on disk as well as in the message text: after the base run, ~/.qwen/teams/verify-team/inboxes/ contains only leader.json — no qa-reviewer.json was ever created. The guard is also destination-agnostic and pre-team: it fires with to: "*", and with no active team at all. An empty-string to does not trip it, so a to: "" + task_id call still routes to the task rather than erroring.
Finding 3 — two hint gaps, now live again (non-blocking)
Round 1 deferred these as moot. With hints reachable they are observable, and both are the same class of confusing diagnostic that #10073 is about.
leader is a valid to: destination but is excluded from the hint. The leader is never in teamFile.members (TeamManager.ts says so explicitly), and leader is a reserved teammate name, so findMemberByName can never match it:
LEADER_NAME is already imported in send-message.ts, so this is small. I built and ran the witness rather than proposing it untested — tsc -p packages/core exit 0, focused suite still 24/24, and the hint fires in the real CLI:
-import { findMemberByName } from '../agents/team/teamHelpers.js';
+import {
+ findMemberByName,
+ sanitizeName,
+} from '../agents/team/teamHelpers.js';
@@
- const teammate = teamManager
- ? findMemberByName(
- teamManager.getTeamFile().members,
- this.params.task_id,
- )
- : undefined;
+ const teammate = !teamManager
+ ? undefined
+ : sanitizeName(this.params.task_id) === LEADER_NAME
+ ? { name: LEADER_NAME }
+ : findMemberByName(
+ teamManager.getTeamFile().members,
+ this.params.task_id,
+ );
One wording nit if you take it: the leader is not a teammate, so that branch reads better with its own sentence than by reusing the "teammate" phrasing.
The hint is one-directional. A live background-task id passed as to gets Teammate "..." not found. with no suggestion to use task_id — the mirror image of the case this PR fixes:
One more sharp edge, informational: findMemberByName normalizes through sanitizeName, which maps every non-[a-z0-9-] character to a hyphen. qa reviewer, QA_REVIEWER, qa.reviewer, qa/reviewer and --qa--reviewer-- all produce the qa-reviewer hint. That is the right call for a suggestion (generous beats silent), and a live task always wins the lookup before the hint path is reached — I verified that precedence with a task and a teammate sharing the name qa-reviewer.
Finding 4 — the red CI check is stale base, not this diff
Test (ubuntu-latest, Node 22.x) fails at step Check TUI dependency direction:
$ npm run check:tui-dep-direction
npm error Missing script: "check:tui-dep-direction"
The script exists in origin/main's package.json and not in the PR branch's — it was added after this PR's merge-base, while ci.yml:489 already invokes it:
PR head d5f1106c23 -> 0 occurrences
merge-base 0756be0ce -> 0 occurrences
origin/main -> 1 occurrence
Merging main into the branch locally resolves it: clean merge, and the failing step then passes —
[rule] packages/core/src — framework-neutral business core
scanned 1310 files, 9639 import specifiers
OK: no violations
PASS — dependency direction holds.
A rebase should turn the check green with no code change.
Finding 5 — suites, and non-vacuity
| check | result |
|---|---|
send-message.test.ts focused |
24/24 at head; 24/24 on the tree merged with current main |
packages/core full suite |
21680 passed / 2 failed / 13 skipped (603 files) |
| the 2 failures | skill-curator + session-writer-lease — fail identically at merge-base 0756be0ce7 with no PR code in the tree; both inject failure via filesystem permissions and this host runs as uid 0 |
packages/core/src/tools + src/agents/team scoped |
100 files / 3422 tests passed |
npm run typecheck (all workspaces) |
pass |
eslint + prettier --check on both changed files |
pass |
Non-vacuity A/B — production hunk reverted to merge-base, tests kept from head: 2 failed | 22 passed. The third new test (returns error for non-existent task without an active team) survives, correctly — it is a negative control, not a fix-proving test.
Mutation matrix — 5/5 killed:
| mutant | result |
|---|---|
M1 drop the teamManager null-guard |
killed — and only by the surviving A/B test above, which is what earns it its place |
M2 to && task_id → to || task_id |
killed (19 tests) |
| M3 drop name normalization | killed |
M4 drop the returnDisplay teammate branch |
killed |
M5 drop the error.message hint append (the d5f1106c23 change) |
killed |
Merge reference
The objection from round 1 is fixed, and fixed in the way that was suggested. The disambiguation half is real, correctly placed at the validateToolParamValues hook, and reaches production; the hint half now reaches both the model and the user. I would take this as-is after a rebase, and file the two hint gaps in Finding 3 as a follow-up rather than holding the PR for them.
#10090 is still open as a competing fix for the same issue; whichever lands, the other should close.
中文说明
维护者验证 —— 第 2 轮，Linux 平台，基于 d5f1106c23
第 1 轮（macOS，head 99e47d5）的结论是：消歧那一半有效，但队友提示语在生产上完全失效——ToolResult 带 error 时 coreToolScheduler 只转发 error.message，而提示语写在 llmContent / returnDisplay 里。随后作者推了 d5f1106，正是把提示语移进 error.message 的那一行改动。
本轮在这个新 head 上重新验证整个 PR，平台选的是 PR 自述仍未测试的 Linux。
被测树： PR head d5f1106。同时本地合入当前 origin/main（比 merge-base 领先 156 个提交）：干净合并，与改动同一文件的 #10081 无冲突，合并后聚焦套件仍是 24/24。
主机： Debian 13（内核 6.12.63，x86-64）、Node v22.22.2、npm 10.9.7。
方法： 以下全部经由打包后的 CLI（npm run build && npm run bundle → dist/cli.js）、隔离的 HOME、QWEN_CODE_ENABLE_AGENT_TEAM=1、--approval-mode yolo，以及一个脚本化的假 OpenAI provider，在 tmux 中驱动真实 Agent Team：team_create → 用 agent({name}) 拉起具名队友 qa-reviewer → 真实后台任务 → 六个目的地场景。两个臂，除 packages/core/src/tools/send-message.ts 外树完全相同（main 臂把生产 hunk 回退到 merge-base 0756be0ce7）。
harness、原始链路流水账、pane 捕获与大图：https://github.com/wenshao/qwen-code/tree/assets-pr10083/round2
结论
| 声明 | 结果 | |
|---|---|---|
| 1 | 第 1 轮发现 2（提示语不可达）已被 d5f1106c23 修复 |
✅ 已解决 —— 链路与 TUI 双向确认 |
| 2 | 同时给 to + task_id 会在任一分发路径前被拒绝 |
✅ 确认，包括 task_id 指向存活任务的情况 |
| 3 | 单目的地调用行为不变 | ✅ 无回归 |
| 4 | 变红的 Test (ubuntu-latest) 检查 |
main 后通过 |
| 5 | Linux 上 packages/core 套件 / typecheck / lint / prettier |
✅ 全绿（2 个既有环境性失败，已在 merge-base 上证明） |
建议：rebase 后合入。 下面第 3 条的两个提示语残留缺口是真实存在的，但不在本 PR 声明的范围内——它们在第 1 轮被列为「延后」，当时因为没有任何提示可达而无意义；现在提示语确实能到达模型，它们又变成可观测的了。两者都不构成阻断。
发现 1 —— 提示语现在同时到达模型和用户
第 1 轮的核心反对意见已关闭。同一探针、两个臂、新 head：base 臂复现 #10073 报告的那条诊断；head 臂提示语出现，歧义调用被拒绝，且提示可执行——截图见英文部分。
场景 C 值得单独点出：那一轮直接照着刚拿到的提示改用 to: "qa-reviewer"，消息投递成功，队友回报——说明这条建议不只是文案，它指向的目的地是真能用的。
用户可见的那一半看截图；模型可见的那一半是在链路上核的，取的是 provider 实际收到的 role: "tool" 帧：
head 臂
role: tool
content: Task not found: QA Reviewer. Did you mean to message teammate "qa-reviewer"?
If so, use `to: "qa-reviewer"` instead of `task_id`.
base 臂 —— 同一探针，提示语字符串在 0 个模型请求中出现
validateBuildAndExecute 仍然没有生产调用点（grep 只命中 tools.ts:364 它自己的定义），所以 llmContent / returnDisplay 上的断言依旧打在一个无调用方的方法上。变化在于 d5f1106c23 同时加了 expect(result.error?.message).toContain(...) —— 打在生产真正转发的那个字段上。现在是这条断言在承载行为。
发现 2 —— 消歧成立，包括静默误投这一档
这个 bug 最强的形态是：两个字段都给，且 task_id 指向一个存活的后台任务。base 上工具报成功，队友完全不知情。head 上在 build() 内被拒绝——早于注册表查找、早于任一分发路径。
未投递不仅看文案，也在磁盘上核过：base 跑完后 ~/.qwen/teams/verify-team/inboxes/ 里只有 leader.json，qa-reviewer.json 从未被创建。该守卫也与目的地无关、且早于团队存在：to: "*" 会触发，完全没有活动团队时也会触发。空字符串 to 不会误触发，所以 to: "" + task_id 仍然正常投给任务而不是报错。
发现 3 —— 两个提示语缺口，现在重新变成可观测（不阻断）
第 1 轮把它们判为无意义。提示语可达之后它们就能被观察到了，而且属于 #10073 所针对的同一类误导性诊断。
leader 是有效的 to: 目的地，却被排除在提示之外。 leader 从不在 teamFile.members 里（TeamManager.ts 有明确注释），且 leader 是保留的队友名，所以 findMemberByName 永远匹配不到它。LEADER_NAME 在 send-message.ts 里已经 import 了，改动很小。我没有直接抛出未验证的建议，而是把 witness 构建跑通了：tsc -p packages/core exit 0、聚焦套件仍是 24/24、真实 CLI 中提示语正常触发——diff 与截图见英文部分。如果采纳，有个措辞小建议：leader 并不是 teammate，那个分支单独写一句话会比复用「teammate」的句式更贴切。
提示是单向的。 把一个存活的后台任务 id 当作 to 传进去，得到的是 Teammate "..." not found.，没有任何「改用 task_id」的建议——正是本 PR 所修那一档的镜像。
还有一个信息性的边角：findMemberByName 经由 sanitizeName 归一化，会把每一个非 [a-z0-9-] 字符都替换成连字符。qa reviewer、QA_REVIEWER、qa.reviewer、qa/reviewer、--qa--reviewer-- 全都会给出 qa-reviewer 的提示。作为「建议」这样处理是对的（宁可宽松也别沉默），而且存活任务总是在提示路径之前赢得查找——我用一个同名为 qa-reviewer 的任务和队友验过这个优先级。
发现 4 —— 变红的 CI 是基线过旧，不是本次 diff
Test (ubuntu-latest, Node 22.x) 挂在 Check TUI dependency direction 这一步：npm error Missing script: "check:tui-dep-direction"。该脚本存在于 origin/main 的 package.json，不存在于 PR 分支——它是在本 PR 的 merge-base 之后才加进 main 的，而 ci.yml:489 已经在调用它（PR head 0 处、merge-base 0 处、origin/main 1 处）。本地把 main 合进分支即可解决：干净合并，且那一步随后通过（PASS — dependency direction holds.）。rebase 一下应该就能让这个检查转绿，不需要改任何代码。
发现 5 —— 套件与非平凡性
| 检查 | 结果 |
|---|---|
send-message.test.ts 聚焦 |
head 上 24/24；与当前 main 合并后的树上同样 24/24 |
packages/core 全量套件 |
21680 通过 / 2 失败 / 13 跳过（603 文件） |
| 那 2 个失败 | skill-curator + session-writer-lease —— 在没有任何 PR 代码的 merge-base 0756be0ce7 上同样失败；两者都靠文件系统权限注入失败，而本机以 uid 0 运行 |
packages/core/src/tools + src/agents/team 范围 |
100 文件 / 3422 测试通过 |
npm run typecheck（全工作区） |
通过 |
两个改动文件的 eslint + prettier --check |
通过 |
非平凡性 A/B —— 把生产 hunk 回退到 merge-base、保留 head 的测试：2 失败 | 22 通过。第三条新测试（returns error for non-existent task without an active team）存活，这是对的——它是负向对照，不是用来证明修复的。
变异矩阵 —— 5/5 被杀： M1 去掉 teamManager 空值守卫（只被上面那条存活的 A/B 测试杀死，这正是它存在的理由）、M2 && → ||（杀死 19 条）、M3 去掉名称归一化、M4 去掉 returnDisplay 队友分支、M5 去掉 error.message 提示追加（即 d5f1106c23 那行改动）。
合并参考
第 1 轮的反对意见已被修复，而且是按当时建议的方式修的。消歧那一半真实有效、位置放在 validateToolParamValues 钩子上是对的、且能到达生产；提示语那一半现在模型和用户都能收到。我倾向 rebase 之后原样接受，把发现 3 的两个提示语缺口作为后续 issue 处理，而不是为它们卡住本 PR。
#10090 作为同一 issue 的竞争修复仍处于 open；无论哪个合入，另一个都该关闭。
Verified locally by @wenshao at d5f1106c238f88d3844231a6dded2975b7ccc6a5 on Linux. Harness and raw evidence: https://github.com/wenshao/qwen-code/tree/assets-pr10083/round2
d5f1106 to
3267e34
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
@qwen-code /triage |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
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: 52 passed · 0 failed · 52 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：52 通过 · 0 失败 · 52 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10083 Deep Verification —
|
| # | Scenario | Oracle | base (37e7491) |
head (3267e34) |
|---|---|---|---|---|
| S1 | both to:'alice' + task_id:'agent-1', both valid |
error type; queued messages; teammate-send calls | queued to task, teammate silently skipped, no error | INVALID_TOOL_PARAMS, queue empty, 0 sends |
| S2 | task_id:'QA Reviewer', teammate qa-reviewer exists, no such task |
error type/message; hint on 3 surfaces | SEND_MESSAGE_NOT_FOUND, bare Task not found: QA Reviewer, no hint |
same type + hint on llmContent, returnDisplay, error.message |
| S3 | task_id:'nope', unrelated |
byte-identity of all 4 result fields | generic error | byte-identical to base |
| S4 | task miss with getTeamManager() → null |
no crash, generic error | generic error | generic error, no crash |
| S5 | both fields, task missing, name matches teammate | error type; sends | plain miss error, to silently discarded |
INVALID_TOOL_PARAMS, 0 sends |
Score: head arm 17/17 assertions, base arm 13/13 (each base cell asserts the predicted pre-fix behavior — the flip is 3 broken/ambiguous behaviors → 3 fixed, with S3/S4 held byte-identical as regression controls).
Production seam verified by reading, not only by the tool-level harness: CoreToolScheduler.buildInvocation (both call sites, coreToolScheduler.ts L1763 and L2591) wraps tool.build() and converts a validation throw into createErrorResponse(..., INVALID_TOOL_PARAMS, 'not_started') — the invocation is never constructed, so execute() cannot run. BaseDeclarativeTool.build runs schema validation first, then the new validateToolParamValues. The only instantiation site is the tool factory in config.ts; nothing constructs SendMessageInvocation directly. The secondary gate suite coreToolScheduler.test.ts (which exercises this seam) passed 382/382.
Corrections
None — no inaccurate claims found in prior review rounds (first round).
Findings
None. Two non-blocking observations from the shape probes (03-shape-probes.png, 14/14):
- Guard boundary is truthiness, not trim:
to: ''+task_idroutes as task-only (empty = absent, consistent with theto-only path), whileto: ' '+task_idhits the mutual-exclusion error. Both outcomes are non-delivery with an explicit error, so no miscast delivery exists; noted only for completeness. - Hint matcher inherits
sanitizeName: ASCII-adjacent spellings all reach the hint ('QA Reviewer','qa_reviewer','QA-Reviewer', padded forms); fullwidth Unicode (ＱＡ Ｒｅｖｉｅｗｅｒ) sanitizes to empty and falls through to the generic error — consistent with howTeamManageritself resolves names, and the hint is advisory. Related exactness checks:'*','leader', near-miss suffixes and substrings correctly get no hint; a registered task literally named'QA Reviewer'is still queued, never hinted.
Also noted: the description says the change "preserves the machine-readable message" — strictly, error.message gains the hint suffix in the matching case. The type is preserved, and no code anywhere parses the message text (grepped all consumers of SEND_MESSAGE_NOT_FOUND and Task not found; the type has no branching consumer), so this is wording, not a defect.
Vacuity / mutation matrix
All runs against src/tools/send-message.test.ts at head (45 tests), each mutation a single-hunk edit reverted after the run (tree restored, git status clean). Witness captures: 04-matrix-m0-control.png, 05-matrix-m1-no-validation.png, 06-matrix-m2-no-hint.png, 07-matrix-ctrl-reword.png.
| row | mutation | result | killed by (verbatim assertion) |
|---|---|---|---|
| M0 | none (control) | 45/45 green | — |
| M1 | validateToolParamValues override removed |
44/45, exactly 1 red | rejects ambiguous teammate and background-task destinations → expected undefined to be 'invalid_tool_params' (behavioral: the call now succeeds and queues) |
| M2 | teammate-hint hunk reverted to base block | 44/45, exactly 1 red | suggests the teammate destination for a matching task ID → expected 'Task not found: QA Reviewer' to contain 'use to: "qa-reviewer"...' |
| C | validation message reworded (Exactly one of...), same file |
44/45, exactly 1 red | same test as M1 — positive control proving the vitest invocation collects and executes this file |
No survivors among the PR's two guards; both hunks defend different hazards (no layered-guard combination row applies). Additional pinned axes: expect(result.error?.message).toBe('Task not found: nope') (exact match) pins the no-hint behavior for unrelated misses, and the no-team variant test pins the null-manager guard. The ambiguous test's sendMessage not called assertion does not by itself distinguish M1 (base also skips the teammate send when a task matches) — the pinning comes from the pendingMessages and error-type assertions, which is precisely the silent-ignore behavior the PR fixes. Note: the PR body cites "23/23 focused tests"; the file at head contains 45 tests, all green here (Linux/CI) — the author's count appears to reflect a focused subset on their Windows run.
Reviewer Test Plan walkthrough
All three steps executable and verified: step 1 → S1 head cell (+ scheduler seam above); step 2 → S2 + probes ('QA Reviewer' → to: "qa-reviewer"); step 3 → S3 (byte-identical generic error, no hint). No unreachable step.
Not covered
- Repo-wide suite and the full
packages/coresuite — only the two targeted files ran (send-message.test.ts45/45,coreToolScheduler.test.ts382/382). No other suite exercises the changed logic (others reference the tool only viaMockTool/registration checks). - TUI/pixel-level rendering of the new
returnDisplaystrings — verified at the tool-result level only. - Real daemon / real
TeamManagerend-to-end: the harness uses the Config seam with a stub manager exposinggetTeamFile(); the real one returns its in-memoryteamFilewith identical shape (read atTeamManager.ts:1442), but no live team was booted. - Per-commit attribution: checkout is depth 2 (
git rev-list HEAD^1..HEAD^2sees 1 of the 5 commits in the metadata), so only the aggregateHEAD^1..HEADdiff was verified. integration-tests/and the Windows-specific failures the author reported (unrelated suites on their host).verify-capture.mjsworked for every capture; no evidence gaps.
Methodology
Environment: CI verify container (node:22-bookworm), working tree at refs/pull/10083/merge; npm ci + npm run build pre-done at head. Control build: git worktree add tmp/base-tree HEAD^1, then npm run build -w packages/core inside it after symlinking the root node_modules and packages/core/node_modules (per-package otel deps are not hoisted; first build attempt failed on that, second succeeded after the links — the link targets are third-party-only and identical for both cells since the lockfile is untouched). A/B and probe harnesses (ab-harness.mjs, probe-harness.mjs) import the compiled dists directly (mock-free for the unit under test; Config is the seam) and ran under scripts/verify-capture.mjs for the PNG witnesses. Mutation matrix via apply-mutation.mjs (exact one-occurrence string replacement, fails loud otherwise) + matrix-verify.mjs (asserts collected count and exact failed-test set from vitest JSON); all raw vitest JSON/stderr logs in logs/. Source restored with git checkout -- after every mutation; git status clean at exit.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/core/src/tools/send-message.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/send-message.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/tools/send-message.test.ts: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/tools/send-message.test.ts: P (exit 0)
round 2 · packages/core/src/tools/send-message.test.ts: P (exit 0)
round 3 · packages/core/src/tools/send-message.test.ts: P (exit 0)
round 4 · packages/core/src/tools/send-message.test.ts: P (exit 0)
round 5 · packages/core/src/tools/send-message.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
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.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R7-1 corrective hint is one-directional (to→task_id mirror confusion gets no hint) — already reported (round 5 deferral, review 5042676420)
- R7-2 to/task_id mutual exclusivity not stated in the schema descriptions — already reported (round 2 deferral, review 5029062877; re-confirmed as R6-1 in round 6, review 5057551601)
- R7-3 hint can never fire for leader (TeamFile.members excludes the leader) — already reported (round 5 deferral, review 5042676420; re-confirmed as R6-2 in round 6, review 5057551601)
- R7-4 teammate hint can fire for terminated teammates — already reported (round 2 deferral, review 5029062877)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 6b": run packages/core unit tests ( send-message.test.ts ) to confirm green — the review worktree has no node_modules , and npm ci + npm run build exceeds the…; "agent 6a": run packages/core/src/tools/send-message.test.ts (worktree has no node_modules ; vitest failed at config load) — judgment rests on the static trace above, wh….
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): 23 tests passed — this review observed 1795 passed.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 6b"：run packages/core unit tests ( send-message.test.ts ) to confirm green — the review worktree has no node_modules , and npm ci + npm run build exceeds the…；"agent 6a"：run packages/core/src/tools/send-message.test.ts (worktree has no node_modules ; vitest failed at config load) — judgment rests on the static trace above, wh…。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
Test Plan（非阻断）：23 tests passed — this review observed 1795 passed。
— qwen3.8-max via Qwen Code /review (v0.22.3)
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. ✅
Local verification — Linux laneI built a real verification environment for this PR on Linux and drove the change three ways: the PR's own suite with a negative control, an independent probe that shares no scaffolding with that suite, and a real TUI run with a real Agent Team. Result: merge-ready, no blocking findings. The PR's "Tested on" table lists Windows only, so this adds the 🐧 Linux lane. Arms: base = merge-base 1. Real TUI, real Agent Team — the two shapes from #10073Same scripted leader driven against a scripted OpenAI-compatible endpoint, in a real session with
Before (base). After (PR). The first call is refused before dispatch, and the second carries the hint: Worth noting for anyone reviewing the shape of the fix: the TUI surfaces 2. Independent probe — real Config, real TeamManager, real registryTo avoid grading the PR against its own scaffolding, this probe uses a real
S1 is the one that matters: on base the ambiguous call is queued onto the background task while 3. Tests, controls, and static gates
The negative control is the part I'd point a reviewer at: 3 test cases are added and 2 of them go red when only the production file is reverted, so the suite genuinely discriminates the fix rather than just being green. I also checked that nothing can reach Non-blocking notesN1 — the model is never told the fields are exclusive. The tool description and both property descriptions still read as two independent optional fields, so a model's first ambiguous call is rejected rather than prevented; the rejection also doesn't say which field to keep. One clause in the N2 — the hint covers roster members only. N3 — the exclusivity test and the routing normalization disagree on whitespace. The guard is raw truthiness ( N4 — two of the three added cases pin behaviour rather than prove it. "returns error for non-existent task without an active team" and the added None of these change my read: the fix is correct, minimal, and the runtime-validation route is the right call given #7984. 中文版本本地验证 —— Linux lane我在 Linux 上为这个 PR 搭了一套真实验证环境，从三个方向驱动这次改动：PR 自带的测试套件加反向对照、一套与该套件不共享任何脚手架的独立探针，以及一次带真实 Agent Team 的真实 TUI 运行。结论：可以合入，无阻塞问题。 PR 的 "Tested on" 表格只列了 Windows，这里补上 🐧 Linux。 两臂：base = merge-base 1. 真实 TUI、真实 Agent Team —— #10073 里的两种调用形态同一份脚本化 leader，对着一个脚本化的 OpenAI 兼容端点运行，会话真实开启
修复前（base）。 修复后（PR）。 第一个调用在投递前就被拒绝，第二个则带上了提示： 有一点值得评审者注意：TUI 展示的是 2. 独立探针 —— 真实 Config、真实 TeamManager、真实 registry为了避免用 PR 自己的脚手架给 PR 打分，这套探针使用真实的
关键是 S1：base 上歧义调用被排进了后台任务队列、 3. 测试、对照与静态门禁
反向对照是我最想让评审者看的一项：新增 3 个用例，其中 2 个在只回退生产文件后变红，说明这套测试真的能判别这次修复，而不只是"绿着"。 我还确认了没有任何路径能绕过新增的守卫直达 非阻塞事项N1 —— 模型从来没被告知这两个字段互斥。 工具描述和两个属性描述读起来仍然是两个彼此独立的可选字段，所以模型的第一次歧义调用是被拒绝、而不是被预防；拒绝信息里也没说该保留哪一个。在 N2 —— 提示只覆盖 roster 成员。 N3 —— 互斥判断与路由归一化在空白字符上不一致。 守卫用的是裸真值（ N4 —— 新增的三个用例里有两个是在钉行为、而不是在证明修复。 "returns error for non-existent task without an active team" 以及加到既有用例上的那几条 这些都不改变我的判断：修复是正确的、最小的，考虑到 #7984，走运行时校验这条路是对的。 🤖 Generated with Claude Code — Claude Opus 5 (1M context) |
3 participants
What this PR does
This PR makes
send_messagedestination selection unambiguous. Calls that supply both a teammate destination and a background-task destination now fail validation before either message path is invoked. When a background-task lookup misses but the supplied identifier matches a teammate in the active team roster, the error also suggests retrying with the teammate destination field.The change preserves the existing missing-task error type and machine-readable message, and adds focused regression coverage for non-dispatch on ambiguous input, teammate-name normalization, and the unchanged generic error for unrelated task IDs.
Why it's needed
Both destination fields are optional in the tool schema, while execution previously prioritized the background-task field. If both were supplied, the teammate destination was silently ignored; if the task lookup then missed, the caller received only a background-task error even when the intended recipient was a teammate. Runtime validation closes that ambiguity without introducing the provider-incompatible top-level
oneOfschema shape discussed in #7984.Reviewer Test Plan
How to verify
send_messagewith bothtoandtask_idtargeting valid destinations. Confirm that it returns an invalid-parameters error and that neither the teammate delivery path nor the background-task queue receives the message.task_idwhose normalized value matches a teammate name, such asQA Reviewerforqa-reviewer. Confirm that the result remainsSEND_MESSAGE_NOT_FOUNDand suggeststo: "qa-reviewer".task_id. Confirm that it retains the generic missing-background-task error and does not add a teammate hint.Evidence (Before & After)
Before: a call carrying both destinations queued the message to the background task and never called teammate delivery. A missing task identifier matching a teammate returned only
Error: No background task found with ID "qa-reviewer".After: the ambiguous call returns
Only one of "to" or "task_id" may be provided.before dispatch. The matching missing-task case retainsSEND_MESSAGE_NOT_FOUNDand addsDid you mean to message teammate "qa-reviewer"? If so, use to: "qa-reviewer" instead of task_id.Focused verification: 23/23 tests passed. Targeted ESLint and Prettier checks passed. The repository build and typecheck passed. The full preflight completed clean/install/format/lint/build/typecheck successfully, but its all-workspace test phase was not green on this Windows host: unrelated suites reported WSL
/bin/bashabsence, Windows symlink/path behavior differences, filesystem lock/permission errors, performance/time-limit failures, and one DWS timing/state expectation. No full-suite failure referenced the changed behavior; the focused test file also passed 23/23 inside the core full-suite run.Tested on
Environment (optional)
Windows PowerShell, Node.js v24.11.0, npm 11.6.1, isolated Git worktree, no sandbox runtime.
Risk & Scope
task_idnow receive an explicit validation error. No data migration or schema change is required.Linked Issues
Fixes #10073
中文说明
本 PR 做了什么
本 PR 消除了
send_message目的地选择的歧义。同时提供 teammate 目的地和后台任务目的地的调用，现在会在进入任一消息通道前校验失败。当后台任务查找未命中，但提供的标识符与当前团队 roster 中的 teammate 名称匹配时，错误还会提示改用 teammate 目的地字段重试。本次修改保留了原有的任务未找到错误类型和机器可读错误消息，并增加了聚焦回归覆盖：歧义输入不得投递、teammate 名称规范化匹配，以及无关任务 ID 仍保持通用错误。
为什么需要它
工具 schema 中两个目的地字段均为可选，而原执行逻辑优先处理后台任务字段。如果两者同时提供，teammate 目的地会被静默忽略；随后如果任务查找未命中，即使调用者本意是联系 teammate，也只会收到后台任务错误。运行时校验关闭了这一歧义，同时没有引入 #7984 中提到的、与部分 provider 不兼容的顶层
oneOfschema 结构。Reviewer 测试计划
如何验证
to和task_id、且两者都指向有效目的地的参数调用send_message。确认它返回参数无效错误，并且 teammate 投递路径和后台任务队列均未收到消息。task_id，但其规范化值与 teammate 名称匹配，例如用QA Reviewer匹配qa-reviewer。确认结果仍为SEND_MESSAGE_NOT_FOUND，并提示使用to: "qa-reviewer"。task_id。确认它保留通用的后台任务未找到错误，并且不增加 teammate 提示。前后证据
修复前：同时携带两个目的地的调用会把消息排入后台任务队列，且永远不会调用 teammate 投递。与 teammate 同名的缺失任务标识符只会返回
Error: No background task found with ID "qa-reviewer".。修复后：歧义调用会在投递前返回
Only one of "to" or "task_id" may be provided.。同名的任务未命中场景仍保留SEND_MESSAGE_NOT_FOUND，并增加Did you mean to message teammate "qa-reviewer"? If so, use to: "qa-reviewer" instead of task_id.提示。聚焦验证：23/23 项测试通过。目标 ESLint 和 Prettier 检查通过。仓库 build 和 typecheck 通过。完整 preflight 的 clean/install/format/lint/build/typecheck 阶段均成功，但其全工作区测试阶段在这台 Windows 主机上未全绿：无关套件报告了 WSL
/bin/bash缺失、Windows 符号链接/路径行为差异、文件锁/权限错误、性能/超时失败，以及一个 DWS 时序/状态断言失败。所有全量失败均未引用本次修改行为；目标测试文件在 core 全量测试中也为 23/23 通过。测试平台
环境（可选）
Windows PowerShell、Node.js v24.11.0、npm 11.6.1、隔离 Git worktree、未使用 sandbox runtime。
风险与范围
task_id的调用，现在会收到明确的校验错误。不需要数据迁移或 schema 变更。关联 Issue
Fixes #10073