From d119b60bf0560763adcfe4496bb6d4efae2c83f0 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 7 May 2026 19:35:51 +0800 Subject: [PATCH] test(sdk): align tool-control E2E with prior-read enforcement (#3898) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- .../sdk-typescript/tool-control.test.ts | 37 ++++++++++++++----- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/integration-tests/sdk-typescript/tool-control.test.ts b/integration-tests/sdk-typescript/tool-control.test.ts index c4b48fc82..973659295 100644 --- a/integration-tests/sdk-typescript/tool-control.test.ts +++ b/integration-tests/sdk-typescript/tool-control.test.ts @@ -1121,8 +1121,9 @@ describe('Tool Control Parameters (E2E)', () => { it( 'should apply updatedInput from canUseTool callback', async () => { - await helper.createFile('test.txt', 'original'); - + // Don't pre-create test.txt: prior-read enforcement requires + // existing files to have been read via read_file first, but + // this test restricts coreTools to write_file only. let capturedInput: Record = {}; const q = query({ @@ -1171,8 +1172,9 @@ describe('Tool Control Parameters (E2E)', () => { it( 'canUseTool should not be called for allowedTools even if it would modify input', async () => { - await helper.createFile('test.txt', 'original'); - + // Don't pre-create test.txt: prior-read enforcement requires + // existing files to have been read via read_file first, but + // this test restricts coreTools to write_file only. let canUseToolCalled = false; const q = query({ @@ -1433,7 +1435,10 @@ describe('Tool Control Parameters (E2E)', () => { session_id: crypto.randomUUID(), message: { role: 'user', - content: 'Write "modified" to test.txt.', + // Read-first instruction satisfies prior-read enforcement + // so the deny path is exercised by canUseTool, not by the + // write tool's pre-write guard. + content: 'Read test.txt and then write "modified" to it.', }, parent_tool_use_id: null, }; @@ -1447,14 +1452,16 @@ describe('Tool Control Parameters (E2E)', () => { cwd: testDir, permissionMode: 'default', coreTools: ['read_file', 'write_file'], - canUseTool: async (toolName) => { + canUseTool: async (toolName, input) => { if (toolName === 'write_file') { return { behavior: 'deny', message: 'Write operations are not allowed', }; } - return { behavior: 'allow', updatedInput: {} }; + // Pass-through: empty `updatedInput` would erase + // file_path and break the read_file call. + return { behavior: 'allow', updatedInput: input }; }, debug: false, }, @@ -1470,6 +1477,15 @@ describe('Tool Control Parameters (E2E)', () => { } } + // Make the read-first dependency explicit: if the model + // skipped read_file, prior-read enforcement would surface + // EDIT_REQUIRES_PRIOR_READ instead of the canUseTool deny + // message we are asserting on below — fail fast with a + // clear signal instead of a confusing toContain mismatch. + const toolCalls = findToolCalls(messages); + const toolNames = toolCalls.map((tc) => tc.toolUse.name); + expect(toolNames).toContain('read_file'); + // write_file should have been attempted but stream was closed const writeFileResults = findToolResults(messages, 'write_file'); expect(writeFileResults.length).toBeGreaterThan(0); @@ -1533,9 +1549,12 @@ describe('Tool Control Parameters (E2E)', () => { cwd: testDir, permissionMode: 'default', coreTools: ['read_file', 'write_file'], - canUseTool: async (toolName) => { + canUseTool: async (toolName, input) => { canUseToolCalls.push(toolName); - return { behavior: 'allow', updatedInput: {} }; + // Pass-through: empty `updatedInput` would erase + // file_path on the SDK→CLI boundary + // (permissionController.ts:444 truthy-replaces args). + return { behavior: 'allow', updatedInput: input }; }, debug: false, },