fix(integration-tests): stabilize async SDK MCP tool handler E2E (#8222) - #8223
Merged
Conversation
Assert on the deterministic tool result instead of the model's paraphrased final text, which did not reliably echo the value verbatim and caused intermittent failures on main.
Collaborator
Author
Autofix E2E Report — Issue #8222Failing test: Root causeThe test asserted that the model's free-text final answer contained the literal string expect(assistantText.toLowerCase()).toMatch(/test_async/i);The prompt asks the model to "Tell me the result", but the model paraphrases the tool output non-deterministically and does not always echo the value verbatim. When it paraphrased (e.g. "the tool returned a delayed result"), the assertion failed intermittently on FixAssert on the deterministic tool result via the existing Verification
中文说明Autofix E2E 报告 — Issue #8222失败测试： 根因该测试断言模型的自由文本最终回答中包含字面字符串 expect(assistantText.toLowerCase()).toMatch(/test_async/i);提示词要求模型“告诉我结果”，但模型会非确定性地对工具输出进行改写，并不总是逐字回显该值。当模型改写时（例如输出“the tool returned a delayed result”），该断言会在 修复改为通过现有的 验证
🧠 Handled by Qwen Code · model/模型 |
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
github-actions
Bot
added
the
review/self-reported
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. Linked issue #8222 is a real Direction: aligned. This is a straightforward flaky-test stabilization squarely within scope — no auth/sandbox/model-selection/telemetry/public-contract surface involved. CHANGELOG has no direct reference, but the area (SDK MCP integration tests) is clearly relevant. Size: not applicable — the only changed file is Approach: the scope feels right and genuinely minimal. The fix swaps a non-deterministic assertion (model prose) for a deterministic one (the tool result the async handler actually returns), reusing the existing Risk: no elevated risk signals. The changed file is a test file and matches none of the high-risk revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到，而非理论性的。关联的 issue #8222 是一次真实的 方向：对齐。这是一次范围内、直接的不稳定测试稳定化——不涉及 auth/sandbox/模型选择/telemetry/公共契约。CHANGELOG 无直接引用，但该领域（SDK MCP 集成测试）显然相关。 规模：不适用——唯一改动的文件是 方案：范围合理且真正最小化。该修复把非确定性断言（模型散文）换成确定性断言（异步处理器实际返回的工具结果），并复用现有的 风险：无升级风险信号。改动文件是测试文件，不匹配任何与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code reviewMy independent take before reading the diff: a test that asserts on the model's free-text answer is inherently flaky, because the model paraphrases tool output. The right fix is to assert on the deterministic value the tool handler actually returns, ideally via an existing helper rather than new code. That is exactly what this PR does, and it does it cleanly. The async handler returns No correctness, security, or regression concerns; no convention violations. Nothing to block on. Test evidence (PR's own CI)This is an unattended CI run, so I'm quoting the PR's own check results rather than running anything. As of this review the Linux suite is still running and nothing has failed; the macOS/Windows test jobs and the optional verify/tmux lanes were skipped by CI config for this run. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The table above is updated in place by the finalize job once CI settles. The flaky test itself lives in the integration suite, so the green-signal that matters here is the integration test passing on CI rather than a single local run. No sandboxed lane ( 中文说明代码审查读 diff 之前我的独立判断：断言模型自由文本答案的测试天生不稳定，因为模型会改写工具输出。正确的修复是断言工具处理器实际返回的确定性值，最好复用现有辅助函数而非新增代码。 这正是本 PR 所做的，而且做得干净。异步处理器返回 无正确性、安全性或回归问题；无规范违规。无可阻塞项。 测试证据（PR 自身 CI）这是无人值守的 CI 运行，因此我引用 PR 自身的检查结果，而非自行运行任何东西。截至本次审查，Linux 套件仍在运行，目前没有任何失败；macOS/Windows 测试任务以及可选的 verify/tmux 通道在本次运行中被 CI 配置跳过。 上表会在 CI 结束后由 finalize 任务就地更新。这个不稳定测试本身位于集成测试套件中，因此这里关键的绿色信号是集成测试在 CI 上通过，而非单次本地运行。 这里有意不指定沙箱通道（ — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 5/5 — a textbook flaky-test stabilization: minimal, correct, and it preserves coverage rather than deleting the check. Stepping back, this is exactly the kind of PR that should be easy to merge. The motivation is a real, observed The only reason I'm not approving this second is timing, not doubt: the Linux test suite is still running on the reviewed commit, and I won't attest to a result that doesn't exist yet. Approval is deferred until CI lands green on 中文说明置信度：5/5 —— 一次教科书式的不稳定测试稳定化：最小、正确，并且保留了覆盖面而非删除检查。 退一步看，这正是那种应该轻松合入的 PR。动机是一次真实观测到的 此刻没有立即批准的唯一原因是时机，而非疑虑：Linux 测试套件仍在所审查的提交上运行，我不会为一个尚不存在的结果背书。批准将推迟到 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
Contributor
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. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
Comment on lines
+458
to
+460
| const toolResults = findToolResults(messages, MCP_DELAYED_RESPONSE); | ||
| expect(toolResults.length).toBeGreaterThan(0); | ||
| expect(toolResults[0]?.isError).toBe(false); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This stabilized test is not exercised by any PR-gating CI check. integration-tests/ is outside every npm workspace, so the workspace npm test never collects it; the only workflow that runs it (e2e.yml) has no pull_request trigger and runs post-merge on main, nightly, and on-demand. (The skipped Integration Tests (CLI, No Sandbox) check runs the cli suite via test:integration:cli:sandbox:none, not the sdk-typescript suite this file lives in, so it would not collect this test even when it runs.)
Failure scenario: if the new assertion were defective — e.g. MCP_DELAYED_RESPONSE not matching the real tool name so toolResults is empty and toBeGreaterThan(0) should fail — no PR check would catch it; the PR merges green and the failure surfaces only post-merge on main or at the nightly run. (The assertion is in fact type-correct and the tool name matches — this is a verification gap, not a known defect.)
Concrete step before merge: run the SDK suite on-demand against a live model — npm run test:integration:sdk:sandbox:none (or target this file: QWEN_SANDBOX=false npx vitest run --root ./integration-tests sdk-typescript/sdk-mcp-server.test.ts).
中文说明
[建议] 本次稳定化后的测试不会被任何 PR 门禁 CI 检查执行。integration-tests/ 不在任何 npm workspace 内，因此 workspace 的 npm test 永远不会收集它；唯一运行它的 workflow（e2e.yml）没有 pull_request 触发器，仅在合并到 main 后、每夜定时以及手动触发时运行。（被跳过的 Integration Tests (CLI, No Sandbox) 检查通过 test:integration:cli:sandbox:none 运行的是 cli 套件，而非本文件所在的 sdk-typescript 套件，因此即便它运行也不会收集本测试。）
失败场景：如果新断言存在缺陷——例如 MCP_DELAYED_RESPONSE 与真实工具名不匹配，导致 toolResults 为空、toBeGreaterThan(0) 本应失败——没有任何 PR 检查会捕获它；PR 会以绿色合并，失败只会在合并到 main 后或每夜运行时才显现。（事实上该断言类型正确、工具名也匹配——这是一个验证缺口，而非已知缺陷。）
合并前的具体步骤：针对真实模型按需运行 SDK 套件——npm run test:integration:sdk:sandbox:none（或只运行本文件：QWEN_SANDBOX=false npx vitest run --root ./integration-tests sdk-typescript/sdk-mcp-server.test.ts）。
— qwen3.8-max-preview via Qwen Code /review
Collaborator
Author
Collaborator
Author
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no code changesReviewed the feedback on PR #8223 (issue #8222). No code changes this round. The single inline finding is a verification-gap Suggestion that the reviewer explicitly states is "not a known defect," and the one failed check is provably unrelated to this change. Details below. Inline suggestion — verification gap (rc:3690197876)Finding: the stabilized test in Disposition: decline the code-change interpretation; defer the manual live-model run to the maintainer's pre-merge step. Reasons:
The thread is left open so the maintainer can run the live-model suite before merge, as the reviewer recommended. Failed check —
|
Collaborator
ReviewOverviewTest-only change that de-flakes Correctness
Minor suggestions (non-blocking)
VerdictLGTM — small, well-scoped, follows the established helper pattern, and removes the flake without losing coverage. 🤖 Generated with Claude Code — Claude Fable 5 |
qwen-code-ci-bot
approved these changes
Jul 31, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
wenshao
approved these changes
Jul 31, 2026
Collaborator
|
Released in v0.21.3. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Stabilizes a flaky SDK MCP integration test. The "should handle async tool handlers with delays" test previously asserted that the model's free-text final answer contained the literal value
test_async. It now asserts on the deterministic tool result returned by the async tool handler, using the existingfindToolResultshelper. The test still confirms the tool was called, that the handler returned a non-error result containing the expected value, and that the query completed successfully. This is a test-only change; no production code is touched.Why it's needed
The model paraphrases tool output non-deterministically and does not always echo the requested value verbatim, so asserting on its prose made the test fail intermittently on
main(issue #8222). The behavior actually under test — that an async MCP tool handler runs and returns its delayed value — is captured deterministically in the tool result, so asserting there removes the flakiness without weakening coverage.Reviewer Test Plan
How to verify
Run the targeted integration test against a configured model (requires
npm run build && npm run bundlefirst sodist/cli.jsexists):QWEN_SANDBOX=false npx vitest run --root ./integration-tests sdk-typescript/sdk-mcp-server.test.ts -t "should handle async tool handlers with delays"Expected: the test passes consistently. A reviewer can also confirm the assertion now reads the tool result (
Delayed result: test_async) rather than the assistant's paraphrased text, and that the other tests in the file are unchanged.Evidence (Before & After)
N/A (test-only change, no user-visible/TUI behavior). Locally the targeted test passed against the live model in ~22s; before the fix the same assertion was the source of intermittent
mainfailures.Tested on
Environment (optional)
Linux runner,
QWEN_SANDBOX=false, bundled CLI atdist/cli.js, live model via the configured OpenAI-compatible endpoint.Risk & Scope
Linked Issues
Fixes #8222
中文说明
本 PR 做了什么
稳定一个易失败的 SDK MCP 集成测试。“should handle async tool handlers with delays” 测试此前断言模型的自由文本最终回答中包含字面值
test_async。现在改为使用现有的findToolResults辅助函数，断言异步工具处理器返回的确定性工具结果。测试仍然确认：工具被调用、处理器返回了包含预期值的非错误结果、以及查询成功完成。这是一次仅涉及测试的改动；未改动任何生产代码。为什么需要
模型会非确定性地改写工具输出，并不总是逐字回显所请求的值，因此对它的散文式回答做断言会导致该测试在
main上间歇性失败（issue #8222）。真正被测试的行为——异步 MCP 工具处理器执行并返回其延迟后的值——被确定性地记录在工具结果中，因此在那里做断言可以在不削弱覆盖面的前提下消除不稳定性。审阅者测试计划
如何验证
针对已配置好的模型运行目标集成测试（需先执行
npm run build && npm run bundle以生成dist/cli.js）：QWEN_SANDBOX=false npx vitest run --root ./integration-tests sdk-typescript/sdk-mcp-server.test.ts -t "should handle async tool handlers with delays"预期：测试稳定通过。审阅者还可以确认断言现在读取的是工具结果（
Delayed result: test_async）而非助手改写后的文本，并且该文件中的其他测试保持不变。证据（改动前后）
N/A（仅测试改动，无用户可见/TUI 行为）。在本地针对真实模型运行，目标测试约 22 秒通过；修复前同一断言是
main上间歇性失败的来源。测试环境
环境（可选）
Linux 运行环境，
QWEN_SANDBOX=false，打包后的 CLI 位于dist/cli.js，通过已配置的 OpenAI 兼容端点调用真实模型。风险与范围
关联 Issue
Fixes #8222