test(sdk): align tool-control E2E with prior-read enforcement - #3898
Merged
Conversation
Three tests in tool-control.test.ts broke deterministically after the
prior-read enforcement landed. Each seeded test.txt then prompted a
direct write — the new guard now rejects the write before canUseTool
fires, with "File X has not been fully read in this session...".
- updatedInput-application + allowedTools-bypass tests: drop the seed
so write_file takes the new-file path (exempt from enforcement).
- asyncGenerator-deny test: keep seed (assertion requires unchanged
content), rewrite prompt to "Read X then write Y" — the pattern
used by 27 passing tests in the same file. Also fix a latent bug
where canUseTool returned `updatedInput: {}` for non-write tools,
which would erase file_path on read_file (SDK `?? toolInput` only
catches nullish, not empty objects).
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. |
wenshao
commented
May 7, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Findings
[Critical] 同一 describe 块中遗漏了 updatedInput: {} 反模式 (integration-tests/sdk-typescript/tool-control.test.ts:1545)
PR 在行 ~1457 已修复了这个 bug（注释说明空的 updatedInput 会擦除 file_path），但此处的 canUseTool 回调仍返回 updatedInput: {} 且不接收 input 参数：
canUseTool: async (toolName) => {
canUseToolCalls.push(toolName);
return { behavior: 'allow', updatedInput: {} };
},当工具调用经过 canUseTool 时，所有参数会被静默擦除——{} 是 truthy，绕过了 Query.ts:478 的 ?? 回退。建议采用与行 ~1457 相同的透传修复：
canUseTool: async (toolName, input) => {
canUseToolCalls.push(toolName);
return { behavior: 'allow', updatedInput: input };
},
[Suggestion] deny 测试 (tool-control.test.ts:1438-1468) 依赖模型遵循多步指令 "Read test.txt and then write..."。若模型跳过读取直接写，prior-read 强制会用 EDIT_REQUIRES_PRIOR_READ（而非预期的 "Write operations are not allowed"）拦截写入，导致混淆的测试失败。建议添加 expect(toolNames).toContain('read_file') 以显式化此依赖。
— deepseek-v4-pro via Qwen Code /review
- Fix the same `updatedInput: {}` reverse-pattern at line 1545 in
the multi-turn asyncGenerator test. The CLI side at
permissionController.ts:444 does `if (updatedInput && typeof
updatedInput === 'object')` — an empty object is truthy, so it
silently replaces args. Mirror the pass-through fix from line ~1457.
- In the deny test, add `expect(toolNames).toContain('read_file')`
before the canUseTool-deny assertion. If the model skips the
read-first instruction, prior-read enforcement would surface
EDIT_REQUIRES_PRIOR_READ rather than the canUseTool deny message,
causing a confusing toContain mismatch. Fail fast with a clear
signal instead.
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 8, 2026
…#3898) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DragonnZhang
pushed a commit
that referenced
this pull request
May 8, 2026
* test(sdk): align tool-control E2E with prior-read enforcement
Three tests in tool-control.test.ts broke deterministically after the
prior-read enforcement landed. Each seeded test.txt then prompted a
direct write — the new guard now rejects the write before canUseTool
fires, with "File X has not been fully read in this session...".
- updatedInput-application + allowedTools-bypass tests: drop the seed
so write_file takes the new-file path (exempt from enforcement).
- asyncGenerator-deny test: keep seed (assertion requires unchanged
content), rewrite prompt to "Read X then write Y" — the pattern
used by 27 passing tests in the same file. Also fix a latent bug
where canUseTool returned `updatedInput: {}` for non-write tools,
which would erase file_path on read_file (SDK `?? toolInput` only
catches nullish, not empty objects).
* test(sdk): address self-review on #3898
- Fix the same `updatedInput: {}` reverse-pattern at line 1545 in
the multi-turn asyncGenerator test. The CLI side at
permissionController.ts:444 does `if (updatedInput && typeof
updatedInput === 'object')` — an empty object is truthy, so it
silently replaces args. Mirror the pass-through fix from line ~1457.
- In the deny test, add `expect(toolNames).toContain('read_file')`
before the canUseTool-deny assertion. If the model skips the
read-first instruction, prior-read enforcement would surface
EDIT_REQUIRES_PRIOR_READ rather than the canUseTool deny message,
causing a confusing toContain mismatch. Fail fast with a clear
signal instead.
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…#3898) * test(sdk): align tool-control E2E with prior-read enforcement Three tests in tool-control.test.ts broke deterministically after the prior-read enforcement landed. Each seeded test.txt then prompted a direct write — the new guard now rejects the write before canUseTool fires, with "File X has not been fully read in this session...". - updatedInput-application + allowedTools-bypass tests: drop the seed so write_file takes the new-file path (exempt from enforcement). - asyncGenerator-deny test: keep seed (assertion requires unchanged content), rewrite prompt to "Read X then write Y" — the pattern used by 27 passing tests in the same file. Also fix a latent bug where canUseTool returned `updatedInput: {}` for non-write tools, which would erase file_path on read_file (SDK `?? toolInput` only catches nullish, not empty objects). * test(sdk): address self-review on QwenLM#3898 - Fix the same `updatedInput: {}` reverse-pattern at line 1545 in the multi-turn asyncGenerator test. The CLI side at permissionController.ts:444 does `if (updatedInput && typeof updatedInput === 'object')` — an empty object is truthy, so it silently replaces args. Mirror the pass-through fix from line ~1457. - In the deny test, add `expect(toolNames).toContain('read_file')` before the canUseTool-deny assertion. If the model skips the read-first instruction, prior-read enforcement would surface EDIT_REQUIRES_PRIOR_READ rather than the canUseTool deny message, causing a confusing toContain mismatch. Fail fast with a clear signal instead.
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
2 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.
Summary
Three tests in
integration-tests/sdk-typescript/tool-control.test.tsbroke deterministically after #3774 landed (prior-read enforcement before Edit / WriteFile mutates a file). Same pattern in each:helper.createFileseedstest.txt, then the prompt asks the model to write to it directly. The new guard rejects the write beforecanUseToolfires withFile X has not been fully read in this session....The smoking gun — test 3 expected
[Operation Cancelled] Reason: Write operations are not allowed(fromcanUseTooldeny) but received the prior-read error instead.Both Linux and macOS jobs hit the same 3 failures, retry x2, on every E2E run since #3774 merged on 2026-05-06.
Changes
should apply updatedInput from canUseTool callbackandcanUseTool should not be called for allowedTools even if it would modify input: drop the seed file.write_filethen takes the new-file path, which the enforcement exempts (fileExists === false). Both tests' assertions still hold (capturedInputpopulated /canUseToolCalled === false/ final content matches).should deny tool when canUseTool returns deny with asyncGenerator prompt: keep the seed (the assertion requires unchanged content), change prompt toRead test.txt and then write "modified" to it.. This matches the read-first pattern already used by 27 passing tests in the same file (line 57, 145, 1361). Also fix a latent bug —canUseToolreturnedupdatedInput: {}for non-write tools, which would erasefile_pathon the now-requiredread_filecall. The SDK'sresult.updatedInput ?? toolInputfallback (Query.ts:478) only catches nullish, not empty objects. Passinputthrough instead.No production code changes — test-side alignment only. The #3774 enforcement contract stays intact.
Test plan
中文
概要
tool-control.test.ts里 3 个用例在 #3774（prior-read enforcement）合入后确定性挂掉。三个用例同样套路：helper.createFile预存test.txt，然后提示模型直接Write X to test.txt。新加的强制现在在canUseTool之前就把 write 拦下，错误信息是File X has not been fully read in this session...。冒烟枪是用例 3——它期望
[Operation Cancelled] Reason: Write operations are not allowed（来自canUseTool的 deny），实际拿到 prior-read 错误。Linux + macOS 两个 job 同样 3 个用例，retry x2 仍然挂，2026-05-06 #3774 合并后每一次 E2E 都红。
修改
should apply updatedInput from canUseTool callback和canUseTool should not be called for allowedTools...：移除前置 seed 文件。write_file走 new-file 路径，fileExists === false时 enforcement 豁免。两个用例断言（capturedInput有值 /canUseToolCalled === false/ 最终文件内容）都成立。should deny tool when canUseTool returns deny with asyncGenerator prompt：保留 seed（断言要求文件内容不变），prompt 改成Read test.txt and then write "modified" to it.。同 file 27 个过的用例（line 57、145、1361）已在用这个 read-first 模式。顺手修了个 latent bug——canUseTool对非 write 工具返回updatedInput: {}，会把read_file的file_path抹空。SDK 的result.updatedInput ?? toolInput兜底（Query.ts:478）只接 nullish，不接空对象。改成透传input。不动生产代码，只在测试侧对齐 #3774 的契约。