test: deflake write_file content assertion in tool-control E2E test - #7613
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: observed CI failure — #7605 shows the Direction: test deflake for CI stability — clearly within scope. No product-direction concerns. Size: not applicable (integration-tests only, no core paths). Approach: minimal and focused — one assertion swapped from a disk read-back to inspecting the tool-call arguments directly. The tradeoff is honestly documented (no longer catches a hypothetical write-that-never-persists). Scope is right; nothing to cut. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 CI 失败——#7605 显示 方向:测试去抖以稳定 CI——完全在范围内,无产品方向顾虑。 规模:不适用(仅 integration-tests,未触及核心路径)。 方案:最小且聚焦——将一个断言从磁盘回读改为直接检查工具调用参数。权衡已诚实说明(不再捕获假设性的"写入未持久化"场景)。范围合理,无需删减。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewClean, minimal change. The fix swaps a racy disk read-back for a deterministic assertion on the No correctness issues, no convention violations. One observation: the TestingRan the specific test with The original flake is 中文说明代码审查干净、最小的改动。将有竞争条件的磁盘回读替换为对 无正确性问题,无规范违反。一个观察:下方的 测试使用 测试通过(38.5 秒)。原始不稳定问题仅在 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean deflake, minimal and correct, would merge without hesitation. This is exactly the right fix. The test's intent is to verify the model requested a content change via Single file, single assertion swapped, honest documentation of the tradeoff. Nothing to cut, nothing missing. 中文说明置信度:5/5 — 干净的去抖修复,最小且正确,毫不犹豫可以合并。 这正是正确的修复方式。测试的意图是验证模型通过 单文件、单断言替换、诚实记录权衡。无需删减,无遗漏。 — Qwen Code · qwen3.8-max-preview Reviewed at |
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. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
What this PR does
Updates the
coreToolsE2E test intool-control.test.tsto verify thatwrite_filewas called with different content than the original by inspecting the tool call's arguments, instead of re-reading the file from disk after the run completes.Why it's needed
CI failed on main with
expect(content).not.toBe('original content')intool-control.test.ts:95, but only on thesandbox:dockerjob — the same commit passed on macOS andsandbox:nonein the same run. The triggering commit was unrelated to tool control or file I/O, confirming this is a flaky assertion rather than a regression: re-reading the file from disk right after the model invokeswrite_fileis subject to filesystem visibility timing in the Docker sandbox, so the read can race the write.This preserves the original intent of the test — confirming the model requested a real content change, not just an empty/no-op write — without depending on when the sandboxed filesystem write becomes observable to the test process.
Reviewer Test Plan
How to verify
Run
cd integration-tests && cross-env QWEN_SANDBOX=false npx vitest run sdk-typescript/tool-control.test.tsand confirm thecoreTools parameter > should only allow specified tools when coreTools is settest passes. Ideally re-run a few times undersandbox:dockerto confirm the previous flakiness is gone.Evidence (Before & After)
N/A — test-only change, no user-visible behavior affected.
Tested on
Environment (optional)
sandbox:nonelocally; the original failure was specific tosandbox:docker.Risk & Scope
sandbox:dockerlocally (Docker unavailable in this environment).Linked Issues
中文说明
此 PR 的作用
更新
tool-control.test.ts中的coreToolsE2E 测试,通过检查write_file工具调用的参数来验证其写入了与原始内容不同的内容,而不是在运行结束后重新读取磁盘上的文件。为什么需要这个改动
main 分支的 CI 在
tool-control.test.ts:95处因expect(content).not.toBe('original content')失败,但仅在sandbox:docker任务中失败——同一次运行中,同一个提交在 macOS 和sandbox:none上均通过。触发该失败的提交与工具控制或文件 I/O 无关,这证实了这是一个不稳定的断言,而非真正的回归:在模型调用write_file后立即重新读取磁盘文件,会受 Docker 沙箱中文件系统可见性时序的影响,导致读取操作可能与写入操作产生竞争。这保留了测试的原始意图——确认模型请求了真正的内容变更,而非空的无操作写入——同时不再依赖沙箱化文件系统写入何时对测试进程可见。
审阅者测试计划
如何验证
运行
cd integration-tests && cross-env QWEN_SANDBOX=false npx vitest run sdk-typescript/tool-control.test.ts,确认coreTools parameter > should only allow specified tools when coreTools is set测试通过。最好在sandbox:docker下多次运行,确认之前的不稳定性已消失。证据(前后对比)
不适用——仅测试文件改动,不影响用户可见行为。
测试环境
环境(可选)
本地使用
sandbox:none;原始失败仅出现在sandbox:docker中。风险与范围
sandbox:docker下的原始不稳定问题(本环境中 Docker 不可用)。