From 9b0bdf8d9be9c8497c09b2b0b8ed142782b71e2d Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 30 Apr 2026 19:44:46 +0800 Subject: [PATCH 01/16] feat(core): enforce prior read before Edit / WriteFile mutates a file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces a session-scoped invariant: the model cannot mutate an existing file without having actually Read it (or its post-write state) earlier in this conversation. Builds on the FileReadCache landed in #3717. Two new ToolErrorType codes: - EDIT_REQUIRES_PRIOR_READ — file has no entry in the session cache. The model is told to use read_file first. - FILE_CHANGED_SINCE_READ — file has an entry but its mtime or size drifted since the recorded fingerprint. The model is told to re-read before retrying. EditTool blocks the existing-file path on cache.check; new-file creation (old_string === '' on a non-existent target) is exempt. WriteFileTool blocks the overwrite path; new-file creation (fileExists === false) is exempt. Both tools route through the existing fileReadCacheDisabled escape hatch on Config — flipping it bypasses enforcement byte-for-byte, matching pre-cache behaviour. Operators can use this as a kill switch if a session falls into a state where the cache cannot be trusted. ReadFile fix on the auto-memory path: PR #3717 had auto-memory reads skip the cache entirely (both lookup and record), but with the new enforcement that means a model that just Read AGENTS.md cannot then Edit it. Decoupled the two: auto-memory reads still skip the file_unchanged fast-path (the per-read freshness must always reach the model) but DO record into the cache so the follow-up Edit sees `fresh`. New regression test asserts this. Test plan - vitest run (all of @qwen-code/qwen-code-core): 6308 passed, 2 skipped - 9 new enforcement tests across edit.test.ts and write-file.test.ts: unknown rejects, stale rejects, new-file exempt, edit chain stays authorised, escape hatch bypasses, plus the auto-memory record regression in read-file.test.ts. - tsc --noEmit clean. eslint clean. core build succeeds. --- packages/core/src/tools/edit.test.ts | 128 +++++++++++++++++++++ packages/core/src/tools/edit.ts | 66 +++++++++++ packages/core/src/tools/read-file.test.ts | 36 ++++++ packages/core/src/tools/read-file.ts | 11 +- packages/core/src/tools/tool-error.ts | 11 ++ packages/core/src/tools/write-file.test.ts | 89 ++++++++++++++ packages/core/src/tools/write-file.ts | 66 +++++++++++ 7 files changed, 405 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 2b78956d9ff..2125584a81d 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -82,6 +82,7 @@ describe('EditTool', () => { getToolRegistry: () => ({}) as any, // Minimal mock for ToolRegistry getDefaultFileEncoding: vi.fn().mockReturnValue('utf-8'), getFileReadCache: () => fileReadCache, + getFileReadCacheDisabled: vi.fn().mockReturnValue(false), } as unknown as Config; // Reset mocks before each test @@ -96,6 +97,21 @@ describe('EditTool', () => { fs.rmSync(tempDir, { recursive: true, force: true }); }); + /** + * Simulate the model having read `filePath` earlier in the session, + * so the EditTool's prior-read enforcement does not reject the + * subsequent edit. Tests that exercise pure Edit-business behaviour + * (diffing, encoding, replace_all, etc.) should call this after + * writing the fixture file and before invoking `tool.execute`. + */ + function seedPriorRead(filePath: string) { + const stats = fs.statSync(filePath); + fileReadCache.recordRead(filePath, stats, { + full: true, + cacheable: true, + }); + } + describe('applyReplacement', () => { it('should return newString if isNewFile is true', () => { expect(applyReplacement(null, 'old', 'new', true)).toBe('new'); @@ -407,6 +423,7 @@ describe('EditTool', () => { const initialContent = 'This is some old text.'; const newContent = 'This is some new text.'; // old -> new fs.writeFileSync(filePath, initialContent, 'utf8'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: 'old', @@ -519,6 +536,7 @@ describe('EditTool', () => { // Create file with BOM (BOM is \ufeff character in string) const originalContent = '\ufeff// Original line\nconst x = 1;'; fs.writeFileSync(bomFilePath, originalContent, 'utf8'); + seedPriorRead(bomFilePath); const params: EditToolParams = { file_path: bomFilePath, @@ -572,6 +590,7 @@ describe('EditTool', () => { it('should successfully replace multiple occurrences when replace_all is true', async () => { fs.writeFileSync(filePath, 'old text\nold text\nold text', 'utf8'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: 'old', @@ -782,6 +801,7 @@ describe('EditTool', () => { 'should return FILE_WRITE_FAILURE on write error', async () => { fs.writeFileSync(filePath, 'content', 'utf8'); + seedPriorRead(filePath); // Make file readonly to trigger a write error fs.chmodSync(filePath, '444'); @@ -905,6 +925,114 @@ describe('EditTool', () => { }); }); + describe('prior-read enforcement', () => { + const abortSignal = new AbortController().signal; + let filePath: string; + + beforeEach(() => { + filePath = path.join(rootDir, 'enforce_target.txt'); + }); + + it('rejects an edit when the file has not been read in this session', async () => { + fs.writeFileSync(filePath, 'untouched content', 'utf8'); + // No seedPriorRead call — simulate the model trying to Edit a + // file it has never received via ReadFile. + const params: EditToolParams = { + file_path: filePath, + old_string: 'untouched', + new_string: 'modified', + }; + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); + expect(result.error?.message).toMatch( + /has not been read in this session/, + ); + // File must remain untouched. + expect(fs.readFileSync(filePath, 'utf8')).toBe('untouched content'); + }); + + it('rejects an edit when the file has been modified since the last read', async () => { + fs.writeFileSync(filePath, 'one', 'utf8'); + seedPriorRead(filePath); + // Simulate an out-of-band modification: change content + bump + // mtime far enough into the future that even coarse-resolution + // filesystems detect the change. + fs.writeFileSync(filePath, 'two with more bytes', 'utf8'); + const future = new Date(Date.now() + 60_000); + fs.utimesSync(filePath, future, future); + + const params: EditToolParams = { + file_path: filePath, + old_string: 'two', + new_string: 'three', + }; + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.error?.type).toBe(ToolErrorType.FILE_CHANGED_SINCE_READ); + expect(result.error?.message).toMatch(/has been modified since/); + // File must remain at the externally-modified content. + expect(fs.readFileSync(filePath, 'utf8')).toBe('two with more bytes'); + }); + + it('exempts new-file creation from prior-read enforcement', async () => { + // old_string === '' on a non-existent path is the new-file + // creation idiom in EditTool. The model has nothing to read + // first, so enforcement must not trigger. + const newPath = path.join(rootDir, 'brand-new-edit.txt'); + const params: EditToolParams = { + file_path: newPath, + old_string: '', + new_string: 'fresh creation', + }; + (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( + ApprovalMode.AUTO_EDIT, + ); + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.error).toBeUndefined(); + expect(fs.readFileSync(newPath, 'utf8')).toBe('fresh creation'); + }); + + it('allows a chain of edits without re-reading between them', async () => { + // After the first Edit, recordWrite stamps `lastWriteAt`. The + // second Edit's stat will still match the cache entry (because + // recordWrite refreshed the fingerprint), so it is `fresh` and + // proceeds without requiring an intervening Read. + fs.writeFileSync(filePath, 'alpha', 'utf8'); + seedPriorRead(filePath); + + const first = await tool + .build({ file_path: filePath, old_string: 'alpha', new_string: 'beta' }) + .execute(abortSignal); + expect(first.error).toBeUndefined(); + + const second = await tool + .build({ file_path: filePath, old_string: 'beta', new_string: 'gamma' }) + .execute(abortSignal); + expect(second.error).toBeUndefined(); + expect(fs.readFileSync(filePath, 'utf8')).toBe('gamma'); + }); + + it('bypasses enforcement entirely when fileReadCacheDisabled is true', async () => { + fs.writeFileSync(filePath, 'untouched', 'utf8'); + // No seed: with the cache disabled, the model is on the + // pre-cache contract — Edit must succeed without a prior Read. + (mockConfig.getFileReadCacheDisabled as Mock).mockReturnValueOnce(true); + const params: EditToolParams = { + file_path: filePath, + old_string: 'untouched', + new_string: 'modified', + }; + const result = await tool.build(params).execute(abortSignal); + expect(result.error).toBeUndefined(); + expect(fs.readFileSync(filePath, 'utf8')).toBe('modified'); + }); + }); + describe('workspace boundary validation', () => { it('should validate paths are within workspace root', () => { const validPath = { diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index fef091bf6e4..b549345ecdb 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -387,6 +387,20 @@ class EditToolInvocation implements ToolInvocation { }; } + // Prior-read enforcement: refuse to edit a pre-existing file the + // model has not read in this session, or whose on-disk fingerprint + // has drifted since the last read. The intent is to stop the model + // from "imagining" an old_string for a file it never actually saw. + // New file creation is exempt (no prior content for the model to + // know about); the cache-disabled escape hatch also bypasses this + // check, see `Config.fileReadCacheDisabled`. + if (!editData.isNewFile && !this.config.getFileReadCacheDisabled()) { + const enforcement = await this.requirePriorRead(); + if (enforcement) { + return enforcement; + } + } + try { this.ensureParentDirectoriesExist(this.params.file_path); @@ -528,6 +542,58 @@ class EditToolInvocation implements ToolInvocation { fs.mkdirSync(dirName, { recursive: true }); } } + + /** + * Block the edit unless the file has been read in this session and + * its on-disk fingerprint still matches the recorded one. Returns + * a populated `ToolResult` when the edit must be rejected, or + * `undefined` when the edit is cleared to proceed. + * + * Stat failures here (file deleted between fileExists check and + * now, EACCES, etc.) are intentionally non-blocking: the existing + * write path will surface a richer error. Returning a synthetic + * "you must read first" message in that case would be misleading. + */ + private async requirePriorRead(): Promise { + let stats: fs.Stats; + try { + stats = await fs.promises.stat(this.params.file_path); + } catch { + return undefined; + } + const status = this.config.getFileReadCache().check(stats); + if (status.state === 'fresh') { + return undefined; + } + if (status.state === 'unknown') { + const msg = + `File ${this.params.file_path} has not been read in this session. ` + + `Use the ${ReadFileTool.Name} tool first to verify the current ` + + `content before editing.`; + return { + llmContent: msg, + returnDisplay: `Error: ${ReadFileTool.Name} required before editing this file.`, + error: { + message: msg, + type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + }, + }; + } + // status.state === 'stale' + const msg = + `File ${this.params.file_path} has been modified since you last ` + + `read it (mtime or size changed). Re-read it with the ` + + `${ReadFileTool.Name} tool before editing to ensure your changes ` + + `are based on current content.`; + return { + llmContent: msg, + returnDisplay: `Error: file changed since last read; re-run ${ReadFileTool.Name} first.`, + error: { + message: msg, + type: ToolErrorType.FILE_CHANGED_SINCE_READ, + }, + }; + } } /** diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index 19bacd3e26d..4dd1323755d 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -709,6 +709,42 @@ describe('ReadFileTool', () => { expect(second.llmContent).toMatch(/Cannot display content of binary/); }); + it('records an auto-memory read in the cache so a follow-up Edit can pass enforcement', async () => { + // Auto-memory files skip the file_unchanged fast-path (they + // own a per-read freshness `` that must be + // re-emitted) but they MUST still be recorded in the cache + // — otherwise the prior-read enforcement on Edit / WriteFile + // would refuse to mutate a file the model legitimately just + // read. Put a file under .qwen// via + // QWEN_CODE_MEMORY_LOCAL=1 and assert recordRead happened. + const previousLocal = process.env['QWEN_CODE_MEMORY_LOCAL']; + process.env['QWEN_CODE_MEMORY_LOCAL'] = '1'; + try { + const { getAutoMemoryRoot, clearAutoMemoryRootCache } = await import( + '../memory/paths.js' + ); + clearAutoMemoryRootCache(); + const memRoot = getAutoMemoryRoot(tempRootDir); + await fsp.mkdir(memRoot, { recursive: true }); + const memFile = path.join(memRoot, 'AGENTS.md'); + await fsp.writeFile(memFile, '# memory', 'utf-8'); + + const result = await read({ file_path: memFile }); + // Slow path returned the actual content (not a placeholder). + expect(typeof result.llmContent).toBe('string'); + expect(result.llmContent).not.toMatch(/unchanged since/); + // The cache must contain the auto-memory file's entry so a + // subsequent EditTool / WriteFileTool call sees `fresh`. + expect(fileReadCache.check(fs.statSync(memFile)).state).toBe('fresh'); + } finally { + if (previousLocal === undefined) { + delete process.env['QWEN_CODE_MEMORY_LOCAL']; + } else { + process.env['QWEN_CODE_MEMORY_LOCAL'] = previousLocal; + } + } + }); + it('does not return the placeholder for image files', async () => { const imagePath = path.join(tempRootDir, 'pic.png'); const pngHeader = Buffer.from([ diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index bfb4aeabce2..29e625fb790 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -147,7 +147,14 @@ class ReadFileToolInvocation extends BaseToolInvocation< // transcript transformation). When disabled we bypass both the // fast-path lookup and the post-read record so behaviour matches // the pre-cache implementation byte-for-byte. - const cacheEnabled = !this.config.getFileReadCacheDisabled() && !isAutoMem; + // + // Auto-memory files are *recorded* in the cache (so prior-read + // enforcement on Edit / WriteFile recognises them as read) but + // never serve the file_unchanged placeholder — those files own a + // per-read freshness `` that must be re-emitted + // on every read. + const cacheEnabled = !this.config.getFileReadCacheDisabled(); + const useFastPath = cacheEnabled && !isAutoMem; const cache = this.config.getFileReadCache(); // A "full" Read consumes the whole file: no offset, no limit, no PDF // page range. Only full Reads are eligible for the file_unchanged @@ -172,7 +179,7 @@ class ReadFileToolInvocation extends BaseToolInvocation< }); } - if (cacheEnabled && stats && isFullRead) { + if (useFastPath && stats && isFullRead) { const status = cache.check(stats); if ( status.state === 'fresh' && diff --git a/packages/core/src/tools/tool-error.ts b/packages/core/src/tools/tool-error.ts index 6a29472c669..9fdb10dcdd9 100644 --- a/packages/core/src/tools/tool-error.ts +++ b/packages/core/src/tools/tool-error.ts @@ -36,6 +36,17 @@ export enum ToolErrorType { EDIT_EXPECTED_OCCURRENCE_MISMATCH = 'edit_expected_occurrence_mismatch', EDIT_NO_CHANGE = 'edit_no_change', EDIT_NO_CHANGE_LLM_JUDGEMENT = 'edit_no_change_llm_judgement', + // Returned when Edit / WriteFile is asked to mutate a file the model + // has not (yet) read this session via ReadFile. The session-scoped + // FileReadCache is the source of truth — see read-file.ts and the + // related FileReadCache service. + EDIT_REQUIRES_PRIOR_READ = 'edit_requires_prior_read', + // Returned when Edit / WriteFile is asked to mutate a file the model + // *has* read this session, but the on-disk bytes have changed since + // (mtime or size differs from the recorded fingerprint). The model + // is expected to re-read with ReadFile to refresh its mental model + // before retrying the edit. + FILE_CHANGED_SINCE_READ = 'file_changed_since_read', // Glob-specific Errors GLOB_EXECUTION_ERROR = 'glob_execution_error', diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index e5f972b0098..6d22f57e4c6 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -70,6 +70,7 @@ const mockConfigInternal = { }) as unknown as ToolRegistry, getDefaultFileEncoding: () => 'utf-8', getFileReadCache: () => fileReadCache, + getFileReadCacheDisabled: () => false, }; const mockConfig = mockConfigInternal as unknown as Config; @@ -123,6 +124,19 @@ describe('WriteFileTool', () => { vi.clearAllMocks(); }); + /** + * Simulate the model having read `filePath` earlier in the session, + * so the WriteFileTool's prior-read enforcement does not reject the + * subsequent overwrite. New-file creation paths do not need this. + */ + function seedPriorRead(filePath: string) { + const stats = fs.statSync(filePath); + fileReadCache.recordRead(filePath, stats, { + full: true, + cacheable: true, + }); + } + describe('build', () => { it('should return an invocation for a valid absolute path within root', () => { const params = { @@ -328,6 +342,7 @@ describe('WriteFileTool', () => { const initialContent = 'Initial content for execute.'; const proposedContent = 'Proposed overwrite for execute.'; fs.writeFileSync(filePath, initialContent, 'utf8'); + seedPriorRead(filePath); const params = { file_path: filePath, content: proposedContent }; const invocation = tool.build(params); @@ -609,6 +624,7 @@ describe('WriteFileTool', () => { Buffer.from(originalContent, 'utf-8'), ]), ); + seedPriorRead(filePath); // Spy on writeTextFile to verify BOM option const writeSpy = vi.spyOn(fsService, 'writeTextFile'); @@ -637,6 +653,7 @@ describe('WriteFileTool', () => { // Create file without BOM fs.writeFileSync(filePath, originalContent, 'utf-8'); + seedPriorRead(filePath); // Spy on writeTextFile to verify BOM option const writeSpy = vi.spyOn(fsService, 'writeTextFile'); @@ -750,4 +767,76 @@ describe('WriteFileTool', () => { } }); }); + + describe('prior-read enforcement', () => { + const abortSignal = new AbortController().signal; + + it('rejects a write that would overwrite an unread existing file', async () => { + const filePath = path.join(rootDir, 'enforce-overwrite.txt'); + fs.writeFileSync(filePath, 'untouched bytes', 'utf-8'); + // No seedPriorRead — model has not Read this file in the session. + + const params = { file_path: filePath, content: 'clobber attempt' }; + const result = await tool.build(params).execute(abortSignal); + + expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); + expect(result.error?.message).toMatch( + /has not been read in this session/, + ); + // File must remain at its pre-call content. + expect(fs.readFileSync(filePath, 'utf-8')).toBe('untouched bytes'); + + fs.unlinkSync(filePath); + }); + + it('rejects a write when the file has been modified since the last read', async () => { + const filePath = path.join(rootDir, 'enforce-stale.txt'); + fs.writeFileSync(filePath, 'one', 'utf-8'); + seedPriorRead(filePath); + fs.writeFileSync(filePath, 'two with more bytes', 'utf-8'); + const future = new Date(Date.now() + 60_000); + fs.utimesSync(filePath, future, future); + + const params = { + file_path: filePath, + content: 'clobber the stale file', + }; + const result = await tool.build(params).execute(abortSignal); + + expect(result.error?.type).toBe(ToolErrorType.FILE_CHANGED_SINCE_READ); + expect(result.error?.message).toMatch(/has been modified since/); + expect(fs.readFileSync(filePath, 'utf-8')).toBe('two with more bytes'); + + fs.unlinkSync(filePath); + }); + + it('exempts new-file creation from prior-read enforcement', async () => { + const filePath = path.join(rootDir, 'enforce-new.txt'); + // File does not exist; model has nothing to read first. + const params = { file_path: filePath, content: 'fresh content' }; + const result = await tool.build(params).execute(abortSignal); + + expect(result.error).toBeUndefined(); + expect(fs.readFileSync(filePath, 'utf-8')).toBe('fresh content'); + + fs.unlinkSync(filePath); + }); + + it('bypasses enforcement entirely when fileReadCacheDisabled is true', async () => { + const filePath = path.join(rootDir, 'enforce-bypass.txt'); + fs.writeFileSync(filePath, 'untouched', 'utf-8'); + const original = mockConfigInternal.getFileReadCacheDisabled; + mockConfigInternal.getFileReadCacheDisabled = () => true; + + try { + const params = { file_path: filePath, content: 'clobbered' }; + const result = await tool.build(params).execute(abortSignal); + expect(result.error).toBeUndefined(); + expect(fs.readFileSync(filePath, 'utf-8')).toBe('clobbered'); + } finally { + mockConfigInternal.getFileReadCacheDisabled = original; + fs.unlinkSync(filePath); + } + }); + }); }); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 0e7c979f7f5..d1caf2d6584 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -214,6 +214,19 @@ class WriteFileToolInvocation extends BaseToolInvocation< } } + // Prior-read enforcement: refuse to overwrite a pre-existing file + // the model has not read in this session, or whose on-disk + // fingerprint has drifted since the last read. The intent is to + // stop the model from blindly clobbering files it has not seen. + // New file creation is exempt; the cache-disabled escape hatch + // also bypasses this check. + if (fileExists && !this.config.getFileReadCacheDisabled()) { + const enforcement = await this.requirePriorRead(file_path); + if (enforcement) { + return enforcement; + } + } + if (!fileExists) { fs.mkdirSync(dirName, { recursive: true }); const userEncoding = this.config.getDefaultFileEncoding(); @@ -364,6 +377,59 @@ class WriteFileToolInvocation extends BaseToolInvocation< }; } } + + /** + * Block the write unless the file has been read in this session and + * its on-disk fingerprint still matches the recorded one. Returns a + * populated `ToolResult` when the write must be rejected, or + * `undefined` when the write is cleared to proceed. + * + * Stat failures here are intentionally non-blocking — the existing + * write path will surface a richer error. + */ + private async requirePriorRead( + filePath: string, + ): Promise { + let stats: fs.Stats; + try { + stats = await fs.promises.stat(filePath); + } catch { + return undefined; + } + const status = this.config.getFileReadCache().check(stats); + if (status.state === 'fresh') { + return undefined; + } + if (status.state === 'unknown') { + const msg = + `File ${filePath} has not been read in this session. ` + + `Use the read_file tool first to verify the current content ` + + `before overwriting it.`; + return { + llmContent: msg, + returnDisplay: + 'Error: read_file required before overwriting this file.', + error: { + message: msg, + type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + }, + }; + } + // status.state === 'stale' + const msg = + `File ${filePath} has been modified since you last read it ` + + `(mtime or size changed). Re-read it with the read_file tool ` + + `before overwriting it.`; + return { + llmContent: msg, + returnDisplay: + 'Error: file changed since last read; re-run read_file first.', + error: { + message: msg, + type: ToolErrorType.FILE_CHANGED_SINCE_READ, + }, + }; + } } /** From 3c78a1f96893fae6ee85db0c4570c7dafa36618d Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 30 Apr 2026 21:13:41 +0800 Subject: [PATCH 02/16] test(core): clear shared fileReadCache between write-file.test.ts cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI surfaced one Linux-only failure: the prior-read enforcement test 'rejects a write that would overwrite an unread existing file' returned FILE_CHANGED_SINCE_READ instead of EDIT_REQUIRES_PRIOR_READ. Root cause: the FileReadCache instance is declared at module scope (line 41) and shared across every test in write-file.test.ts. State from earlier tests — most recently the 'records a write' integration test that records the same path — leaks forward. On Linux the test ordering puts a record-bearing test before the enforcement test, so the cache reports `stale` (mtime drifted) instead of `unknown`. macOS / Windows happen to order them differently and never hit it. Adding a fileReadCache.clear() to beforeEach gives every test a known-empty cache, matching how edit.test.ts already isolates its per-test cache by re-instantiating it. --- packages/core/src/tools/write-file.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 6d22f57e4c6..00a9fb70eab 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -86,6 +86,12 @@ describe('WriteFileTool', () => { beforeEach(() => { vi.clearAllMocks(); + // The fileReadCache is module-scope (declared at L41) and shared + // across every test in this file, so state from one test leaks + // into the next. Clear it before each test so every test starts + // from a known-empty cache. CI surfaced this on Linux only because + // file-creation order across tests differs by platform. + fileReadCache.clear(); // Create a unique temporary directory for files created outside the root tempDir = fs.mkdtempSync( path.join(os.tmpdir(), 'write-file-test-external-'), From 3b79cfc81f12b025d103e97ba2f694bd0485987d Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 1 May 2026 06:28:10 +0800 Subject: [PATCH 03/16] fix(core): close prior-read enforcement gaps flagged in 3rd review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three concrete loopholes / regressions that the original PR-B introduction left open. All three are addressed in the same commit because the underlying refactor (move enforcement earlier and tighten the fresh predicate) is shared across them. 1. fresh != "model has seen the bytes". Pre-fix, requirePriorRead() accepted any cache.check === 'fresh'. ReadFileTool records every successful read into the cache, including ranged reads (offset/limit), truncated full reads, and non-cacheable binary/image/audio/video/PDF/notebook reads (lastReadCacheable = false). This let the model peek at a slice or a structured payload of a file and then mutate the whole thing. Tightened the accept predicate to fresh && lastReadAt && lastReadWasFull && lastReadCacheable. 2. Read-less content oracle through calculateEdit error codes. Pre-fix, execute() ran calculateEdit (which reads file bytes and counts matches) before the enforcement check. A model could probe an unread file by attempting Edits with candidate old_strings and observing NO_OCCURRENCE_FOUND vs EXPECTED_OCCURRENCE_MISMATCH vs EDIT_NO_CHANGE — reverse-engineering content without ever calling read_file. Moved enforcement to the top of calculateEdit, before any content read; only a stat is performed up to the rejection point. 3. Confirmation flow regression. Pre-fix, getConfirmationDetails() read the existing file to render a diff for the user, then approval flowed to execute() which would freshly check the cache and reject. The user could approve a diff computed from current bytes the model never saw, and the call would still fail. Moved enforcement before the confirmation read in both EditTool (via the shared calculateEdit path) and WriteFileTool (explicit check at the top of getConfirmationDetails). The user now never sees a confirmation diff for an unread file — the call rejects up front. Public API surface change: requirePriorRead() -> checkPriorRead() that returns a structured decision, so the same predicate can route into a CalculatedEdit.error (calculateEdit), a thrown error (getConfirmationDetails), or a ToolResult (execute) without duplicating the boolean / message / type plumbing in three shapes. Reported by pomelo-nwu (3 inline comments on PR #3774). --- packages/core/src/tools/edit.test.ts | 95 ++++++++++++++- packages/core/src/tools/edit.ts | 128 +++++++++++++-------- packages/core/src/tools/write-file.test.ts | 54 ++++++++- packages/core/src/tools/write-file.ts | 113 +++++++++++------- 4 files changed, 295 insertions(+), 95 deletions(-) diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 2125584a81d..38263d222cf 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -270,6 +270,7 @@ describe('EditTool', () => { it('should request confirmation for valid edit', async () => { fs.writeFileSync(filePath, 'some old content here'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: 'old', @@ -290,6 +291,7 @@ describe('EditTool', () => { it('should throw if old_string is not found', async () => { fs.writeFileSync(filePath, 'some content here'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: 'not_found', @@ -303,6 +305,7 @@ describe('EditTool', () => { it('should throw if multiple occurrences of old_string are found', async () => { fs.writeFileSync(filePath, 'old old content here'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: 'old', @@ -558,6 +561,7 @@ describe('EditTool', () => { it('should return error if old_string is not found in file', async () => { fs.writeFileSync(filePath, 'Some content.', 'utf8'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: 'nonexistent', @@ -575,6 +579,7 @@ describe('EditTool', () => { it('should return error if multiple occurrences of old_string are found and replace_all is false', async () => { fs.writeFileSync(filePath, 'multiple old old strings', 'utf8'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: 'old', @@ -626,6 +631,7 @@ describe('EditTool', () => { it('should return error if trying to create a file that already exists (empty old_string)', async () => { fs.writeFileSync(filePath, 'Existing content', 'utf8'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: '', @@ -683,6 +689,7 @@ describe('EditTool', () => { it('should return error if old_string and new_string are identical', async () => { const initialContent = 'This is some identical text.'; fs.writeFileSync(filePath, initialContent, 'utf8'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: 'identical', @@ -698,6 +705,7 @@ describe('EditTool', () => { // This can happen if the literal string replacement with `replaceAll` results in no change. const initialContent = 'line 1\nline 2\nline 3'; // Note the double space fs.writeFileSync(filePath, initialContent, 'utf8'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, // old_string has a single space, so it won't be found by replaceAll @@ -738,6 +746,7 @@ describe('EditTool', () => { it('should return ATTEMPT_TO_CREATE_EXISTING_FILE error', async () => { fs.writeFileSync(filePath, 'existing content', 'utf8'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: '', @@ -752,6 +761,7 @@ describe('EditTool', () => { it('should return NO_OCCURRENCE_FOUND error', async () => { fs.writeFileSync(filePath, 'content', 'utf8'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: 'not-found', @@ -764,6 +774,7 @@ describe('EditTool', () => { it('should return EXPECTED_OCCURRENCE_MISMATCH error when replace_all is false and text is not unique', async () => { fs.writeFileSync(filePath, 'one one two', 'utf8'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: 'one', @@ -778,6 +789,7 @@ describe('EditTool', () => { it('should return NO_CHANGE error', async () => { fs.writeFileSync(filePath, 'content', 'utf8'); + seedPriorRead(filePath); const params: EditToolParams = { file_path: filePath, old_string: 'content', @@ -947,12 +959,93 @@ describe('EditTool', () => { expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); expect(result.error?.message).toMatch( - /has not been read in this session/, + /has not been fully read in this session/, ); // File must remain untouched. expect(fs.readFileSync(filePath, 'utf8')).toBe('untouched content'); }); + it('rejects an edit when the previous read was ranged (offset/limit)', async () => { + // A model that only Read part of a file has not seen the bytes + // a free-form old_string would touch. Treat this the same as + // "no prior read". + fs.writeFileSync(filePath, 'line a\nline b\nline c\n', 'utf8'); + const stats = fs.statSync(filePath); + // Record as a ranged read: lastReadWasFull = false. + fileReadCache.recordRead(filePath, stats, { + full: false, + cacheable: true, + }); + + const result = await tool + .build({ file_path: filePath, old_string: 'line a', new_string: 'X' }) + .execute(abortSignal); + expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); + expect(fs.readFileSync(filePath, 'utf8')).toBe( + 'line a\nline b\nline c\n', + ); + }); + + it('rejects an edit when the previous read was non-cacheable (binary / pdf / image)', async () => { + // ReadFile records every successful read into the cache, + // including binary / PDF / image reads that produce a + // structured payload rather than text. lastReadCacheable=false + // marks those — Edit must not accept them. + fs.writeFileSync(filePath, 'pretend this is binary', 'utf8'); + const stats = fs.statSync(filePath); + fileReadCache.recordRead(filePath, stats, { + full: true, + cacheable: false, + }); + + const result = await tool + .build({ + file_path: filePath, + old_string: 'pretend', + new_string: 'X', + }) + .execute(abortSignal); + expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); + }); + + it('does not let an unread file be probed via NO_OCCURRENCE_FOUND', async () => { + // Regression for the read-less content oracle: pre-fix, a model + // could call Edit with candidate old_strings on an unread file + // and observe NO_OCCURRENCE_FOUND vs OCCURRENCE_MATCH to + // reverse-engineer the contents. With enforcement before + // calculateEdit, the call must be rejected with the prior-read + // error code regardless of whether the candidate string would + // have matched. + fs.writeFileSync(filePath, 'sensitive token: hunter2', 'utf8'); + + const result = await tool + .build({ + file_path: filePath, + old_string: 'hunter2', + new_string: 'redacted', + }) + .execute(abortSignal); + expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); + expect(result.error?.type).not.toBe( + ToolErrorType.EDIT_NO_OCCURRENCE_FOUND, + ); + }); + + it('rejects confirmation requests on an unread file before showing a diff', async () => { + // The user must not see a diff computed from current bytes the + // model never received — they would approve under a false + // assumption that the model worked from those bytes. + fs.writeFileSync(filePath, 'unread content', 'utf8'); + const invocation = tool.build({ + file_path: filePath, + old_string: 'unread', + new_string: 'modified', + }); + await expect( + invocation.getConfirmationDetails(abortSignal), + ).rejects.toThrow(/read_file required before editing this file/); + }); + it('rejects an edit when the file has been modified since the last read', async () => { fs.writeFileSync(filePath, 'one', 'utf8'); seedPriorRead(filePath); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index b549345ecdb..030d7a3df77 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -152,6 +152,31 @@ class EditToolInvocation implements ToolInvocation { let useBOM = false; let detectedEncoding = 'utf-8'; let detectedLineEnding: LineEnding = 'lf'; + // Prior-read enforcement runs before any content is read so that + // the read pipeline below (and the content-derived error codes + // it can produce — NO_OCCURRENCE_FOUND, EXPECTED_OCCURRENCE_MISMATCH, + // NO_CHANGE) cannot be used as a read-less content oracle on a + // file the model has never legitimately Read. New-file creation + // is exempt because there are no current bytes to read first. + if (fileExists && !this.config.getFileReadCacheDisabled()) { + const decision = await this.checkPriorRead(params.file_path); + if (!decision.ok) { + return { + currentContent: null, + newContent: '', + occurrences: 0, + error: { + display: decision.displayMessage, + raw: decision.rawMessage, + type: decision.type, + }, + isNewFile: false, + encoding: 'utf-8', + bom: false, + lineEnding: 'lf', + }; + } + } if (fileExists) { try { const fileInfo = await this.config @@ -387,20 +412,6 @@ class EditToolInvocation implements ToolInvocation { }; } - // Prior-read enforcement: refuse to edit a pre-existing file the - // model has not read in this session, or whose on-disk fingerprint - // has drifted since the last read. The intent is to stop the model - // from "imagining" an old_string for a file it never actually saw. - // New file creation is exempt (no prior content for the model to - // know about); the cache-disabled escape hatch also bypasses this - // check, see `Config.fileReadCacheDisabled`. - if (!editData.isNewFile && !this.config.getFileReadCacheDisabled()) { - const enforcement = await this.requirePriorRead(); - if (enforcement) { - return enforcement; - } - } - try { this.ensureParentDirectoriesExist(this.params.file_path); @@ -544,54 +555,71 @@ class EditToolInvocation implements ToolInvocation { } /** - * Block the edit unless the file has been read in this session and - * its on-disk fingerprint still matches the recorded one. Returns - * a populated `ToolResult` when the edit must be rejected, or - * `undefined` when the edit is cleared to proceed. + * Test whether the edit is cleared to proceed against an existing + * file based on the session FileReadCache. Returns a structured + * decision so the caller can route it into the right shape — a + * `CalculatedEdit.error` from `calculateEdit`, or a thrown error + * from `getConfirmationDetails`. * - * Stat failures here (file deleted between fileExists check and - * now, EACCES, etc.) are intentionally non-blocking: the existing - * write path will surface a richer error. Returning a synthetic - * "you must read first" message in that case would be misleading. + * Approval requires more than `state === 'fresh'`: the recorded + * read must also have been (a) stamped with `lastReadAt`, + * (b) `lastReadWasFull` (no offset / limit / pages), and + * (c) `lastReadCacheable` (i.e. plain text, not binary / image / + * audio / video / PDF / notebook). Otherwise the model has only + * seen a slice or a structured proxy of the file, not the bytes + * a prospective edit would mutate. + * + * Stat failures here are intentionally non-blocking: the existing + * write path surfaces a richer error. */ - private async requirePriorRead(): Promise { + private async checkPriorRead(filePath: string): Promise< + | { ok: true } + | { + ok: false; + type: ToolErrorType; + rawMessage: string; + displayMessage: string; + } + > { let stats: fs.Stats; try { - stats = await fs.promises.stat(this.params.file_path); + stats = await fs.promises.stat(filePath); } catch { - return undefined; + return { ok: true }; } const status = this.config.getFileReadCache().check(stats); - if (status.state === 'fresh') { - return undefined; + if ( + status.state === 'fresh' && + status.entry.lastReadAt !== undefined && + status.entry.lastReadWasFull && + status.entry.lastReadCacheable + ) { + return { ok: true }; } - if (status.state === 'unknown') { - const msg = - `File ${this.params.file_path} has not been read in this session. ` + - `Use the ${ReadFileTool.Name} tool first to verify the current ` + - `content before editing.`; + if (status.state === 'stale') { + const raw = + `File ${filePath} has been modified since you last read it ` + + `(mtime or size changed). Re-read it with the ${ReadFileTool.Name} ` + + `tool before editing to ensure your changes are based on current ` + + `content.`; return { - llmContent: msg, - returnDisplay: `Error: ${ReadFileTool.Name} required before editing this file.`, - error: { - message: msg, - type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, - }, + ok: false, + type: ToolErrorType.FILE_CHANGED_SINCE_READ, + rawMessage: raw, + displayMessage: `file changed since last read; re-run ${ReadFileTool.Name} first.`, }; } - // status.state === 'stale' - const msg = - `File ${this.params.file_path} has been modified since you last ` + - `read it (mtime or size changed). Re-read it with the ` + - `${ReadFileTool.Name} tool before editing to ensure your changes ` + - `are based on current content.`; + // unknown OR fresh-but-partial / non-cacheable: require a fresh + // full text read. + const raw = + `File ${filePath} has not been fully read in this session. ` + + `Use the ${ReadFileTool.Name} tool first (without offset / limit ` + + `/ pages) to load the entire current text content before editing.`; return { - llmContent: msg, - returnDisplay: `Error: file changed since last read; re-run ${ReadFileTool.Name} first.`, - error: { - message: msg, - type: ToolErrorType.FILE_CHANGED_SINCE_READ, - }, + ok: false, + type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + rawMessage: raw, + displayMessage: `${ReadFileTool.Name} required before editing this file.`, }; } } diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 00a9fb70eab..43f22514d8a 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -215,6 +215,7 @@ describe('WriteFileTool', () => { const filePath = path.join(rootDir, 'confirm_error_file.txt'); const params = { file_path: filePath, content: 'test content' }; fs.writeFileSync(filePath, 'original', { mode: 0o000 }); + seedPriorRead(filePath); const readError = new Error('Simulated read error for confirmation'); vi.spyOn(fsService, 'readTextFile').mockImplementationOnce(() => @@ -259,6 +260,7 @@ describe('WriteFileTool', () => { const originalContent = 'Original content for confirmation.'; const proposedContent = 'Proposed replacement for confirmation.'; fs.writeFileSync(filePath, originalContent, 'utf8'); + seedPriorRead(filePath); const params = { file_path: filePath, content: proposedContent }; const invocation = tool.build(params); @@ -286,6 +288,7 @@ describe('WriteFileTool', () => { const filePath = path.join(rootDir, 'execute_error_file.txt'); const params = { file_path: filePath, content: 'test content' }; fs.writeFileSync(filePath, 'original', { mode: 0o000 }); + seedPriorRead(filePath); vi.spyOn(fsService, 'readTextFile').mockImplementationOnce(() => { const readError = new Error('Simulated read error for execute'); @@ -787,7 +790,7 @@ describe('WriteFileTool', () => { expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); expect(result.error?.message).toMatch( - /has not been read in this session/, + /has not been fully read in this session/, ); // File must remain at its pre-call content. expect(fs.readFileSync(filePath, 'utf-8')).toBe('untouched bytes'); @@ -795,6 +798,55 @@ describe('WriteFileTool', () => { fs.unlinkSync(filePath); }); + it('rejects a write when the previous read was ranged (offset/limit)', async () => { + const filePath = path.join(rootDir, 'enforce-ranged.txt'); + fs.writeFileSync(filePath, 'unchanged', 'utf-8'); + const stats = fs.statSync(filePath); + fileReadCache.recordRead(filePath, stats, { + full: false, + cacheable: true, + }); + + const result = await tool + .build({ file_path: filePath, content: 'clobber' }) + .execute(abortSignal); + expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); + expect(fs.readFileSync(filePath, 'utf-8')).toBe('unchanged'); + + fs.unlinkSync(filePath); + }); + + it('rejects a write when the previous read was non-cacheable', async () => { + const filePath = path.join(rootDir, 'enforce-noncacheable.txt'); + fs.writeFileSync(filePath, 'pretend binary', 'utf-8'); + const stats = fs.statSync(filePath); + fileReadCache.recordRead(filePath, stats, { + full: true, + cacheable: false, + }); + + const result = await tool + .build({ file_path: filePath, content: 'clobber' }) + .execute(abortSignal); + expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); + + fs.unlinkSync(filePath); + }); + + it('rejects confirmation requests on an unread existing file before showing a diff', async () => { + const filePath = path.join(rootDir, 'enforce-confirm.txt'); + fs.writeFileSync(filePath, 'unread current bytes', 'utf-8'); + const invocation = tool.build({ + file_path: filePath, + content: 'replacement content', + }); + await expect( + invocation.getConfirmationDetails(abortSignal), + ).rejects.toThrow(/has not been fully read in this session/); + + fs.unlinkSync(filePath); + }); + it('rejects a write when the file has been modified since the last read', async () => { const filePath = path.join(rootDir, 'enforce-stale.txt'); fs.writeFileSync(filePath, 'one', 'utf-8'); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index d1caf2d6584..eb0e352c540 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -120,6 +120,17 @@ class WriteFileToolInvocation extends BaseToolInvocation< ): Promise { let originalContent = ''; const fileExists = await isFilefileExists(this.params.file_path); + // Run prior-read enforcement *before* we read the file to render + // a confirmation diff. Otherwise the user could approve a diff + // computed from current bytes that the model has never received, + // and the subsequent execute() would still reject the call — + // confusing UX for any approve flow. + if (fileExists && !this.config.getFileReadCacheDisabled()) { + const decision = await this.checkPriorRead(this.params.file_path); + if (!decision.ok) { + throw new Error(decision.rawMessage); + } + } if (fileExists) { try { const { content } = await this.config @@ -215,15 +226,23 @@ class WriteFileToolInvocation extends BaseToolInvocation< } // Prior-read enforcement: refuse to overwrite a pre-existing file - // the model has not read in this session, or whose on-disk - // fingerprint has drifted since the last read. The intent is to - // stop the model from blindly clobbering files it has not seen. - // New file creation is exempt; the cache-disabled escape hatch - // also bypasses this check. + // the model has not legitimately read in this session, or whose + // on-disk fingerprint has drifted since the last read. The intent + // is to stop the model from blindly clobbering files it has not + // seen — and to refuse mutations based on partial reads (offset / + // limit / pages) or non-text reads where the model never saw the + // bytes a write would replace. if (fileExists && !this.config.getFileReadCacheDisabled()) { - const enforcement = await this.requirePriorRead(file_path); - if (enforcement) { - return enforcement; + const decision = await this.checkPriorRead(file_path); + if (!decision.ok) { + return { + llmContent: decision.rawMessage, + returnDisplay: `Error: ${decision.displayMessage}`, + error: { + message: decision.rawMessage, + type: decision.type, + }, + }; } } @@ -379,55 +398,63 @@ class WriteFileToolInvocation extends BaseToolInvocation< } /** - * Block the write unless the file has been read in this session and - * its on-disk fingerprint still matches the recorded one. Returns a - * populated `ToolResult` when the write must be rejected, or - * `undefined` when the write is cleared to proceed. + * Test whether the write is cleared to proceed against an existing + * file based on the session FileReadCache. Returns a structured + * decision so the caller can route it into the right shape — a + * thrown error from `getConfirmationDetails` (so the diff is never + * shown) or a `ToolResult` from `execute`. * - * Stat failures here are intentionally non-blocking — the existing - * write path will surface a richer error. + * Approval requires more than `state === 'fresh'`: the recorded + * read must also be a full text read with `lastReadAt`, + * `lastReadWasFull`, and `lastReadCacheable`. A partial / ranged + * / binary read is not enough — the model has not seen the bytes + * an overwrite would clobber. */ - private async requirePriorRead( - filePath: string, - ): Promise { + private async checkPriorRead(filePath: string): Promise< + | { ok: true } + | { + ok: false; + type: ToolErrorType; + rawMessage: string; + displayMessage: string; + } + > { let stats: fs.Stats; try { stats = await fs.promises.stat(filePath); } catch { - return undefined; + return { ok: true }; } const status = this.config.getFileReadCache().check(stats); - if (status.state === 'fresh') { - return undefined; + if ( + status.state === 'fresh' && + status.entry.lastReadAt !== undefined && + status.entry.lastReadWasFull && + status.entry.lastReadCacheable + ) { + return { ok: true }; } - if (status.state === 'unknown') { - const msg = - `File ${filePath} has not been read in this session. ` + - `Use the read_file tool first to verify the current content ` + + if (status.state === 'stale') { + const raw = + `File ${filePath} has been modified since you last read it ` + + `(mtime or size changed). Re-read it with the read_file tool ` + `before overwriting it.`; return { - llmContent: msg, - returnDisplay: - 'Error: read_file required before overwriting this file.', - error: { - message: msg, - type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, - }, + ok: false, + type: ToolErrorType.FILE_CHANGED_SINCE_READ, + rawMessage: raw, + displayMessage: 'file changed since last read; re-run read_file first.', }; } - // status.state === 'stale' - const msg = - `File ${filePath} has been modified since you last read it ` + - `(mtime or size changed). Re-read it with the read_file tool ` + - `before overwriting it.`; + const raw = + `File ${filePath} has not been fully read in this session. ` + + `Use the read_file tool first (without offset / limit / pages) ` + + `to load the entire current text content before overwriting it.`; return { - llmContent: msg, - returnDisplay: - 'Error: file changed since last read; re-run read_file first.', - error: { - message: msg, - type: ToolErrorType.FILE_CHANGED_SINCE_READ, - }, + ok: false, + type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + rawMessage: raw, + displayMessage: 'read_file required before overwriting this file.', }; } } From 303b6b7d4ac7706d78fc3f4e40c6bef222fa7c9d Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 1 May 2026 16:39:01 +0800 Subject: [PATCH 04/16] refactor(core): close 4 prior-read enforcement gaps from 4th review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. recordWrite now seeds read metadata on brand-new entries (lastReadAt / lastReadWasFull / lastReadCacheable). The strict accept predicate added in the previous round (#3 review) requires all three, but recordWrite only set lastWriteAt — so a model creating a file with Edit (old_string="") or WriteFile and then editing it again was rejected on the second edit. The model authored the bytes it just wrote; for the purposes of prior-read enforcement that counts as having seen them. New regression test in edit.test.ts: "allows a create-then-edit-then-edit chain without an intervening read". 2. Extracted checkPriorRead into src/tools/priorReadEnforcement.ts. The two copies in edit.ts and write-file.ts had already drifted (one used ${ReadFileTool.Name}, the other hardcoded 'read_file'); the boolean guard is security-sensitive and a one-sided fix would silently weaken the boundary. The shared utility takes a verb ('editing' | 'overwriting') so the user-facing prose can differ between callers without duplicating the decision logic. 3. WriteFileTool.execute now runs prior-read enforcement BEFORE readTextFile. Pre-fix, an unread overwrite still slurped the entire file into memory (encoding / BOM / line-ending detection) and only then rejected it: wasted I/O, and momentary in-memory custody of bytes the model never legitimately read. Now matches the order in getConfirmationDetails(). 4. The "rejects a write that would overwrite an unread existing file" test now spies on FileSystemService.readTextFile and asserts not.toHaveBeenCalled() — without that, the test gave false confidence: it passed both pre-fix (read happened, then reject) and post-fix (reject before read), so the ordering regression in (3) was invisible to the assertion. Reported by glm-5.1 via /review on PR #3774. --- .../core/src/services/fileReadCache.test.ts | 14 ++- packages/core/src/services/fileReadCache.ts | 17 ++- packages/core/src/tools/edit.test.ts | 31 +++++ packages/core/src/tools/edit.ts | 76 +----------- .../core/src/tools/priorReadEnforcement.ts | 116 ++++++++++++++++++ packages/core/src/tools/write-file.test.ts | 10 +- packages/core/src/tools/write-file.ts | 114 +++++------------ 7 files changed, 220 insertions(+), 158 deletions(-) create mode 100644 packages/core/src/tools/priorReadEnforcement.ts diff --git a/packages/core/src/services/fileReadCache.test.ts b/packages/core/src/services/fileReadCache.test.ts index 3f9f92f9272..d8dd8ef5990 100644 --- a/packages/core/src/services/fileReadCache.test.ts +++ b/packages/core/src/services/fileReadCache.test.ts @@ -213,11 +213,19 @@ describe('FileReadCache', () => { expect(entry.lastWriteAt).toBe(Date.now()); }); - it('does not set lastReadAt or flip lastReadWasFull', () => { + it('seeds read metadata when recording a write on a brand-new entry', () => { + // The model authored the bytes it just wrote — for the purposes + // of prior-read enforcement on the *next* Edit, that counts as + // having seen the full text content. Without this seeding a + // create→edit→edit chain would reject the second edit because + // lastReadWasFull / lastReadCacheable would still be unset on + // the entry recordWrite created. const cache = new FileReadCache(); const entry = cache.recordWrite('/x/foo.ts', makeStats()); - expect(entry.lastReadAt).toBeUndefined(); - expect(entry.lastReadWasFull).toBe(false); + expect(entry.lastReadAt).toBeDefined(); + expect(entry.lastReadAt).toBe(entry.lastWriteAt); + expect(entry.lastReadWasFull).toBe(true); + expect(entry.lastReadCacheable).toBe(true); }); it('refreshes mtime+size so a follow-up Edit sees fresh', () => { diff --git a/packages/core/src/services/fileReadCache.ts b/packages/core/src/services/fileReadCache.ts index f3ddda490d0..f4537497c2b 100644 --- a/packages/core/src/services/fileReadCache.ts +++ b/packages/core/src/services/fileReadCache.ts @@ -118,10 +118,25 @@ export class FileReadCache { * differ from any prior Read snapshot, so we refresh the cached * fingerprint to the post-write Stats; otherwise the next Edit would * see its own write as a "stale" external change. + * + * If the entry has never been Read in this session (i.e. this is a + * fresh file just created by Edit / WriteFile), populate the read + * metadata as well: the model authored the bytes it just wrote, so + * for the purposes of prior-read enforcement it has effectively + * "seen" the full text content. Without this, a create→edit→edit + * chain would be rejected on the second edit because + * `lastReadAt`/`lastReadWasFull`/`lastReadCacheable` would still be + * undefined. */ recordWrite(absPath: string, stats: Stats): FileReadEntry { const entry = this.upsert(absPath, stats); - entry.lastWriteAt = Date.now(); + const now = Date.now(); + entry.lastWriteAt = now; + if (entry.lastReadAt === undefined) { + entry.lastReadAt = now; + entry.lastReadWasFull = true; + entry.lastReadCacheable = true; + } return entry; } diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 38263d222cf..94e74a2a4b7 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -1090,6 +1090,37 @@ describe('EditTool', () => { expect(fs.readFileSync(newPath, 'utf8')).toBe('fresh creation'); }); + it('allows a create-then-edit-then-edit chain without an intervening read', async () => { + // The author of a brand-new file has, by definition, "seen" + // the bytes it just wrote. Without recordWrite seeding read + // metadata, the second edit would be rejected because + // lastReadWasFull / lastReadCacheable would still be unset on + // the entry recordWrite created during the create step. + const newPath = path.join(rootDir, 'create-then-edit.txt'); + (mockConfig.getApprovalMode as Mock).mockReturnValue( + ApprovalMode.AUTO_EDIT, + ); + + const created = await tool + .build({ + file_path: newPath, + old_string: '', + new_string: 'first content\n', + }) + .execute(abortSignal); + expect(created.error).toBeUndefined(); + + const edited = await tool + .build({ + file_path: newPath, + old_string: 'first', + new_string: 'second', + }) + .execute(abortSignal); + expect(edited.error).toBeUndefined(); + expect(fs.readFileSync(newPath, 'utf8')).toBe('second content\n'); + }); + it('allows a chain of edits without re-reading between them', async () => { // After the first Edit, recordWrite stamps `lastWriteAt`. The // second Edit's stat will still match the cache entry (because diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 030d7a3df77..2f1c6550c21 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -29,6 +29,7 @@ import { } from '../services/fileSystemService.js'; import type { LineEnding } from '../services/fileSystemService.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; +import { checkPriorRead } from './priorReadEnforcement.js'; import { ReadFileTool } from './read-file.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; import { logFileOperation } from '../telemetry/loggers.js'; @@ -159,7 +160,11 @@ class EditToolInvocation implements ToolInvocation { // file the model has never legitimately Read. New-file creation // is exempt because there are no current bytes to read first. if (fileExists && !this.config.getFileReadCacheDisabled()) { - const decision = await this.checkPriorRead(params.file_path); + const decision = await checkPriorRead( + this.config.getFileReadCache(), + params.file_path, + 'editing', + ); if (!decision.ok) { return { currentContent: null, @@ -553,75 +558,6 @@ class EditToolInvocation implements ToolInvocation { fs.mkdirSync(dirName, { recursive: true }); } } - - /** - * Test whether the edit is cleared to proceed against an existing - * file based on the session FileReadCache. Returns a structured - * decision so the caller can route it into the right shape — a - * `CalculatedEdit.error` from `calculateEdit`, or a thrown error - * from `getConfirmationDetails`. - * - * Approval requires more than `state === 'fresh'`: the recorded - * read must also have been (a) stamped with `lastReadAt`, - * (b) `lastReadWasFull` (no offset / limit / pages), and - * (c) `lastReadCacheable` (i.e. plain text, not binary / image / - * audio / video / PDF / notebook). Otherwise the model has only - * seen a slice or a structured proxy of the file, not the bytes - * a prospective edit would mutate. - * - * Stat failures here are intentionally non-blocking: the existing - * write path surfaces a richer error. - */ - private async checkPriorRead(filePath: string): Promise< - | { ok: true } - | { - ok: false; - type: ToolErrorType; - rawMessage: string; - displayMessage: string; - } - > { - let stats: fs.Stats; - try { - stats = await fs.promises.stat(filePath); - } catch { - return { ok: true }; - } - const status = this.config.getFileReadCache().check(stats); - if ( - status.state === 'fresh' && - status.entry.lastReadAt !== undefined && - status.entry.lastReadWasFull && - status.entry.lastReadCacheable - ) { - return { ok: true }; - } - if (status.state === 'stale') { - const raw = - `File ${filePath} has been modified since you last read it ` + - `(mtime or size changed). Re-read it with the ${ReadFileTool.Name} ` + - `tool before editing to ensure your changes are based on current ` + - `content.`; - return { - ok: false, - type: ToolErrorType.FILE_CHANGED_SINCE_READ, - rawMessage: raw, - displayMessage: `file changed since last read; re-run ${ReadFileTool.Name} first.`, - }; - } - // unknown OR fresh-but-partial / non-cacheable: require a fresh - // full text read. - const raw = - `File ${filePath} has not been fully read in this session. ` + - `Use the ${ReadFileTool.Name} tool first (without offset / limit ` + - `/ pages) to load the entire current text content before editing.`; - return { - ok: false, - type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, - rawMessage: raw, - displayMessage: `${ReadFileTool.Name} required before editing this file.`, - }; - } } /** diff --git a/packages/core/src/tools/priorReadEnforcement.ts b/packages/core/src/tools/priorReadEnforcement.ts new file mode 100644 index 00000000000..3d61331f2a2 --- /dev/null +++ b/packages/core/src/tools/priorReadEnforcement.ts @@ -0,0 +1,116 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as fs from 'node:fs'; +import type { FileReadCache } from '../services/fileReadCache.js'; +import { ToolErrorType } from './tool-error.js'; +import { ToolNames } from './tool-names.js'; + +/** + * Result of checking whether a tool that mutates an existing file is + * cleared to proceed based on the session FileReadCache. + * + * - `ok: true` — the model has legitimately read the file in this + * session and the on-disk fingerprint still matches. + * - `ok: false` — the call must be rejected. `type` selects the + * error code; `rawMessage` is the model-facing prose; `displayMessage` + * is the short user-facing form. + * + * The decision is structured (rather than a `ToolResult` or thrown + * error) so each caller can route it into the shape its surrounding + * code expects — a `CalculatedEdit.error` from EditTool's + * `calculateEdit`, a thrown error from `getConfirmationDetails`, or a + * `ToolResult` from `execute`. + */ +export type PriorReadDecision = + | { ok: true } + | { + ok: false; + type: ToolErrorType; + rawMessage: string; + displayMessage: string; + }; + +/** + * Verb used in the user-facing prose ("editing" / "overwriting"). + * Kept as a parameter rather than baked into the tool because EditTool + * and WriteFileTool word their messages slightly differently and we + * do not want a future divergence to silently weaken the boundary. + */ +export type PriorReadVerb = 'editing' | 'overwriting'; + +/** + * Test whether a mutating tool is cleared to proceed against + * `filePath` based on the session FileReadCache. + * + * Approval requires more than `cache.check === 'fresh'`: the recorded + * read must also have been (a) stamped with `lastReadAt`, + * (b) `lastReadWasFull` (no offset / limit / pages), and + * (c) `lastReadCacheable` (i.e. plain text, not binary / image / + * audio / video / PDF / notebook). Otherwise the model has only seen + * a slice or a structured proxy of the file, not the bytes a + * prospective edit would mutate. + * + * Stat failures are intentionally non-blocking — the existing write + * path will surface a richer error than a synthetic "you must read + * first" message. + * + * Note on `recordWrite` interaction: when a tool *creates* a file via + * Edit (`old_string === ''`) or WriteFile (new path), the FileReadCache + * `recordWrite` call seeds `lastReadAt` / `lastReadWasFull` / + * `lastReadCacheable` on the brand-new entry, so a subsequent edit on + * that same file passes here without an intervening explicit Read. + * The model authored those bytes; for the purposes of prior-read + * enforcement that counts as having seen them. + */ +export async function checkPriorRead( + cache: FileReadCache, + filePath: string, + verb: PriorReadVerb, +): Promise { + let stats: fs.Stats; + try { + stats = await fs.promises.stat(filePath); + } catch { + return { ok: true }; + } + const status = cache.check(stats); + if ( + status.state === 'fresh' && + status.entry.lastReadAt !== undefined && + status.entry.lastReadWasFull && + status.entry.lastReadCacheable + ) { + return { ok: true }; + } + if (status.state === 'stale') { + const raw = + `File ${filePath} has been modified since you last read it ` + + `(mtime or size changed). Re-read it with the ${ToolNames.READ_FILE} ` + + `tool before ${verb} it to ensure your changes are based on current ` + + `content.`; + return { + ok: false, + type: ToolErrorType.FILE_CHANGED_SINCE_READ, + rawMessage: raw, + displayMessage: `file changed since last read; re-run ${ToolNames.READ_FILE} first.`, + }; + } + // unknown OR fresh-but-partial / non-cacheable: require a fresh + // full text read. + const raw = + `File ${filePath} has not been fully read in this session. ` + + `Use the ${ToolNames.READ_FILE} tool first (without offset / limit ` + + `/ pages) to load the entire current text content before ${verb} it.`; + const verbDisplay = + verb === 'editing' ? 'editing this file' : 'overwriting this file'; + return { + ok: false, + type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + rawMessage: raw, + displayMessage: `${ToolNames.READ_FILE} required before ${verbDisplay}.`, + }; +} diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 43f22514d8a..5d87c8a1f9b 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -785,6 +785,10 @@ describe('WriteFileTool', () => { fs.writeFileSync(filePath, 'untouched bytes', 'utf-8'); // No seedPriorRead — model has not Read this file in the session. + // Spy on readTextFile to assert enforcement runs *before* any + // I/O against the file's contents — see the L4 review comment. + const readSpy = vi.spyOn(fsService, 'readTextFile'); + const params = { file_path: filePath, content: 'clobber attempt' }; const result = await tool.build(params).execute(abortSignal); @@ -792,9 +796,13 @@ describe('WriteFileTool', () => { expect(result.error?.message).toMatch( /has not been fully read in this session/, ); - // File must remain at its pre-call content. + // File must remain at its pre-call content, and the tool must + // not have slurped the existing bytes into memory before + // rejecting. expect(fs.readFileSync(filePath, 'utf-8')).toBe('untouched bytes'); + expect(readSpy).not.toHaveBeenCalled(); + readSpy.mockRestore(); fs.unlinkSync(filePath); }); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index eb0e352c540..7c1e6a9642c 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -35,6 +35,7 @@ import type { LineEnding } from '../services/fileSystemService.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; +import { checkPriorRead } from './priorReadEnforcement.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; import type { ModifiableDeclarativeTool, @@ -126,7 +127,11 @@ class WriteFileToolInvocation extends BaseToolInvocation< // and the subsequent execute() would still reject the call — // confusing UX for any approve flow. if (fileExists && !this.config.getFileReadCacheDisabled()) { - const decision = await this.checkPriorRead(this.params.file_path); + const decision = await checkPriorRead( + this.config.getFileReadCache(), + this.params.file_path, + 'overwriting', + ); if (!decision.ok) { throw new Error(decision.rawMessage); } @@ -186,6 +191,31 @@ class WriteFileToolInvocation extends BaseToolInvocation< let detectedEncoding: string | undefined; let detectedLineEnding: LineEnding | undefined; const dirName = path.dirname(file_path); + + // Prior-read enforcement runs BEFORE we read the existing file: + // - rejecting a write should not first slurp the entire file + // into memory (wasted I/O on every reject), and + // - we should not be holding bytes of a file the model never + // legitimately saw, even transiently. + // Mirrors the order in getConfirmationDetails() above. + if (fileExists && !this.config.getFileReadCacheDisabled()) { + const decision = await checkPriorRead( + this.config.getFileReadCache(), + file_path, + 'overwriting', + ); + if (!decision.ok) { + return { + llmContent: decision.rawMessage, + returnDisplay: `Error: ${decision.displayMessage}`, + error: { + message: decision.rawMessage, + type: decision.type, + }, + }; + } + } + if (fileExists) { try { const fileInfo = await this.config @@ -225,27 +255,6 @@ class WriteFileToolInvocation extends BaseToolInvocation< } } - // Prior-read enforcement: refuse to overwrite a pre-existing file - // the model has not legitimately read in this session, or whose - // on-disk fingerprint has drifted since the last read. The intent - // is to stop the model from blindly clobbering files it has not - // seen — and to refuse mutations based on partial reads (offset / - // limit / pages) or non-text reads where the model never saw the - // bytes a write would replace. - if (fileExists && !this.config.getFileReadCacheDisabled()) { - const decision = await this.checkPriorRead(file_path); - if (!decision.ok) { - return { - llmContent: decision.rawMessage, - returnDisplay: `Error: ${decision.displayMessage}`, - error: { - message: decision.rawMessage, - type: decision.type, - }, - }; - } - } - if (!fileExists) { fs.mkdirSync(dirName, { recursive: true }); const userEncoding = this.config.getDefaultFileEncoding(); @@ -396,67 +405,6 @@ class WriteFileToolInvocation extends BaseToolInvocation< }; } } - - /** - * Test whether the write is cleared to proceed against an existing - * file based on the session FileReadCache. Returns a structured - * decision so the caller can route it into the right shape — a - * thrown error from `getConfirmationDetails` (so the diff is never - * shown) or a `ToolResult` from `execute`. - * - * Approval requires more than `state === 'fresh'`: the recorded - * read must also be a full text read with `lastReadAt`, - * `lastReadWasFull`, and `lastReadCacheable`. A partial / ranged - * / binary read is not enough — the model has not seen the bytes - * an overwrite would clobber. - */ - private async checkPriorRead(filePath: string): Promise< - | { ok: true } - | { - ok: false; - type: ToolErrorType; - rawMessage: string; - displayMessage: string; - } - > { - let stats: fs.Stats; - try { - stats = await fs.promises.stat(filePath); - } catch { - return { ok: true }; - } - const status = this.config.getFileReadCache().check(stats); - if ( - status.state === 'fresh' && - status.entry.lastReadAt !== undefined && - status.entry.lastReadWasFull && - status.entry.lastReadCacheable - ) { - return { ok: true }; - } - if (status.state === 'stale') { - const raw = - `File ${filePath} has been modified since you last read it ` + - `(mtime or size changed). Re-read it with the read_file tool ` + - `before overwriting it.`; - return { - ok: false, - type: ToolErrorType.FILE_CHANGED_SINCE_READ, - rawMessage: raw, - displayMessage: 'file changed since last read; re-run read_file first.', - }; - } - const raw = - `File ${filePath} has not been fully read in this session. ` + - `Use the read_file tool first (without offset / limit / pages) ` + - `to load the entire current text content before overwriting it.`; - return { - ok: false, - type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, - rawMessage: raw, - displayMessage: 'read_file required before overwriting this file.', - }; - } } /** From d6ffe807e72fdf01b4f472a5674a27acefd1ea0c Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 3 May 2026 00:47:11 +0800 Subject: [PATCH 05/16] refactor(core): close 4 prior-read enforcement gaps from 4th review (Copilot) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five concrete gaps that the previous round of enforcement work left open. Reported by Copilot via /review on PR #3774. 1. Confirmation-time rejections lost their ToolErrorType code. getConfirmationDetails() in both EditTool and WriteFileTool threw a plain Error on prior-read failure, which coreToolScheduler collapsed into UNHANDLED_EXCEPTION — silently breaking the EDIT_REQUIRES_PRIOR_READ / FILE_CHANGED_SINCE_READ contract for any approval-required flow. Fix: introduce PriorReadEnforcementError that carries `errorType: ToolErrorType`. Both confirmation paths now throw it, and coreToolScheduler reads `error.errorType` (falling back to UNHANDLED_EXCEPTION when absent). New regression tests assert the thrown error's `errorType` field for both tools. 2. checkPriorRead's "re-read with read_file" advice was wrong for binary / image / audio / video / PDF / notebook files. Their ReadFile result always sets lastReadCacheable=false, so the message would loop the agent forever on the same rejection. Fix: detect the fresh-but-non-cacheable case explicitly and return a dedicated message that explains the dead end ("Edit / WriteFile cannot mutate that payload safely") instead of asking for another read. Updated the existing non-cacheable regression test to assert the new message and the absence of "use the read_file tool first". 3. checkPriorRead swallowed every stat() failure and returned ok:true. EACCES, EBUSY, NFS hiccups, etc. would silently re-open the blind-write path the helper exists to block. Fix: only ENOENT continues to return ok:true (disappearance race). Any other code is fail-closed: returns EDIT_REQUIRES_PRIOR_READ with a message that names the errno. New regression test in write-file.test.ts spies on fs.promises .stat to inject EACCES and asserts the rejection. 4. The auto-memory record regression test only asserted `state === 'fresh'`. A future change that recorded auto-memory reads as partial / non-cacheable would still satisfy that assertion but would actually fail enforcement on every follow-up Edit. Fix: also assert lastReadAt is defined, lastReadWasFull is true, and lastReadCacheable is true. The full "what enforcement requires" predicate is now explicit in the test. (The 5th item, the WriteFile mirror of (1), is covered by the same PriorReadEnforcementError change.) --- packages/core/src/core/coreToolScheduler.ts | 11 ++- packages/core/src/tools/edit.test.ts | 30 +++++++ packages/core/src/tools/edit.ts | 15 +++- .../core/src/tools/priorReadEnforcement.ts | 83 +++++++++++++++++-- packages/core/src/tools/read-file.test.ts | 18 +++- packages/core/src/tools/write-file.test.ts | 47 +++++++++++ packages/core/src/tools/write-file.ts | 12 ++- 7 files changed, 201 insertions(+), 15 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 09a6c4ccf31..b207eb6e8bc 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -1257,13 +1257,22 @@ export class CoreToolScheduler { continue; } + // Errors thrown from getConfirmationDetails() may carry a + // structured ToolErrorType via an `errorType` instance + // field (see PriorReadEnforcementError in + // tools/priorReadEnforcement.ts). When present, surface + // that code instead of collapsing every confirmation-time + // failure into UNHANDLED_EXCEPTION. + const explicitErrorType = ( + error as { errorType?: ToolErrorType } | undefined + )?.errorType; this.setStatusInternal( reqInfo.callId, 'error', createErrorResponse( reqInfo, error instanceof Error ? error : new Error(String(error)), - ToolErrorType.UNHANDLED_EXCEPTION, + explicitErrorType ?? ToolErrorType.UNHANDLED_EXCEPTION, ), ); } diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 94e74a2a4b7..4250cc00777 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -1006,6 +1006,14 @@ describe('EditTool', () => { }) .execute(abortSignal); expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); + // Telling the model to re-read with read_file would loop the + // agent forever: a binary/image/PDF read also leaves + // lastReadCacheable=false. The message must explain the dead + // end instead of asking for another read. + expect(result.error?.message).toMatch( + /binary \/ image \/ audio \/ video \/ PDF \/ notebook payload/, + ); + expect(result.error?.message).not.toMatch(/Use the read_file tool first/); }); it('does not let an unread file be probed via NO_OCCURRENCE_FOUND', async () => { @@ -1155,6 +1163,28 @@ describe('EditTool', () => { expect(result.error).toBeUndefined(); expect(fs.readFileSync(filePath, 'utf8')).toBe('modified'); }); + + it('attaches a structured ToolErrorType when getConfirmationDetails rejects', async () => { + // Without an `errorType` field on the thrown Error, the tool + // scheduler reports every confirmation-time rejection as + // UNHANDLED_EXCEPTION — losing the EDIT_REQUIRES_PRIOR_READ / + // FILE_CHANGED_SINCE_READ contract this PR introduces. + fs.writeFileSync(filePath, 'unread content', 'utf8'); + const invocation = tool.build({ + file_path: filePath, + old_string: 'unread', + new_string: 'modified', + }); + let caught: unknown; + try { + await invocation.getConfirmationDetails(abortSignal); + } catch (err) { + caught = err; + } + expect((caught as { errorType?: string })?.errorType).toBe( + ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + ); + }); }); describe('workspace boundary validation', () => { diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 2f1c6550c21..6eb7f209574 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -29,7 +29,10 @@ import { } from '../services/fileSystemService.js'; import type { LineEnding } from '../services/fileSystemService.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; -import { checkPriorRead } from './priorReadEnforcement.js'; +import { + checkPriorRead, + PriorReadEnforcementError, +} from './priorReadEnforcement.js'; import { ReadFileTool } from './read-file.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; import { logFileOperation } from '../telemetry/loggers.js'; @@ -331,7 +334,15 @@ class EditToolInvocation implements ToolInvocation { } if (editData.error) { - throw new Error(`Edit error: ${editData.error.display}`); + // The error type from `calculateEdit` includes the new + // prior-read codes (EDIT_REQUIRES_PRIOR_READ / + // FILE_CHANGED_SINCE_READ). Wrap in PriorReadEnforcementError + // so coreToolScheduler surfaces the correct ToolErrorType + // instead of UNHANDLED_EXCEPTION. + throw new PriorReadEnforcementError( + `Edit error: ${editData.error.display}`, + editData.error.type, + ); } const fileName = path.basename(this.params.file_path); diff --git a/packages/core/src/tools/priorReadEnforcement.ts b/packages/core/src/tools/priorReadEnforcement.ts index 3d61331f2a2..5a8061b529c 100644 --- a/packages/core/src/tools/priorReadEnforcement.ts +++ b/packages/core/src/tools/priorReadEnforcement.ts @@ -9,6 +9,26 @@ import type { FileReadCache } from '../services/fileReadCache.js'; import { ToolErrorType } from './tool-error.js'; import { ToolNames } from './tool-names.js'; +/** + * Error thrown by `getConfirmationDetails()` when prior-read enforcement + * rejects a request before any diff is rendered. Carries the structured + * `ToolErrorType` so the tool scheduler can surface it as the right + * tool error code (`EDIT_REQUIRES_PRIOR_READ` / + * `FILE_CHANGED_SINCE_READ`) instead of the generic + * `UNHANDLED_EXCEPTION` that any plain `Error` would otherwise become. + * + * Caught by `coreToolScheduler` via the `errorType` instance field. + */ +export class PriorReadEnforcementError extends Error { + override readonly name = 'PriorReadEnforcementError'; + constructor( + message: string, + readonly errorType: ToolErrorType, + ) { + super(message); + } +} + /** * Result of checking whether a tool that mutates an existing file is * cleared to proceed based on the session FileReadCache. @@ -54,9 +74,13 @@ export type PriorReadVerb = 'editing' | 'overwriting'; * a slice or a structured proxy of the file, not the bytes a * prospective edit would mutate. * - * Stat failures are intentionally non-blocking — the existing write - * path will surface a richer error than a synthetic "you must read - * first" message. + * Stat policy: `ENOENT` means the path disappeared between the + * caller's `fileExists` check and now — a disappearance race that is + * harmless for our purposes (the downstream write will resurface the + * absence as its own error). Any other stat error (`EACCES`, `EBUSY`, + * NFS hiccup, …) is fail-closed: returning `ok: true` would re-open + * the blind-write path the helper exists to block, since a transient + * stat failure does not imply the subsequent read/write will fail. * * Note on `recordWrite` interaction: when a tool *creates* a file via * Edit (`old_string === ''`) or WriteFile (new path), the FileReadCache @@ -74,8 +98,29 @@ export async function checkPriorRead( let stats: fs.Stats; try { stats = await fs.promises.stat(filePath); - } catch { - return { ok: true }; + } catch (err) { + const code = (err as NodeJS.ErrnoException | undefined)?.code; + if (code === 'ENOENT') { + // Disappearance race vs the caller's fileExists check. Let the + // downstream write path surface the absence — synthesising a + // "you must read first" message here would be misleading. + return { ok: true }; + } + // Any other stat failure: fail closed. We cannot prove the file + // has been read; treating that as approval would silently bypass + // enforcement on transient metadata errors that don't prevent + // the subsequent write from succeeding. + const raw = + `Could not stat ${filePath} to verify prior read (${code ?? 'unknown error'}). ` + + `Re-read with the ${ToolNames.READ_FILE} tool, then retry ${verb} it.`; + const verbDisplay = + verb === 'editing' ? 'editing this file' : 'overwriting this file'; + return { + ok: false, + type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + rawMessage: raw, + displayMessage: `cannot verify prior read of ${filePath}; re-run ${ToolNames.READ_FILE} before ${verbDisplay}.`, + }; } const status = cache.check(stats); if ( @@ -99,8 +144,32 @@ export async function checkPriorRead( displayMessage: `file changed since last read; re-run ${ToolNames.READ_FILE} first.`, }; } - // unknown OR fresh-but-partial / non-cacheable: require a fresh - // full text read. + // Differentiate "fresh but the recorded read was non-cacheable" + // (binary / image / audio / video / PDF / notebook) from "no / + // partial read at all". Telling the model to "re-read with read_file" + // for a binary file would loop forever because that read would + // also leave `lastReadCacheable === false`. + if ( + status.state === 'fresh' && + status.entry.lastReadAt !== undefined && + status.entry.lastReadWasFull && + !status.entry.lastReadCacheable + ) { + const raw = + `File ${filePath} is a binary / image / audio / video / PDF / ` + + `notebook payload that the ${ToolNames.READ_FILE} tool returns ` + + `as a structured value rather than as plain text. The Edit / ` + + `WriteFile tools cannot mutate that payload safely — re-reading ` + + `it would not change this. Use a different mechanism (e.g. shell ` + + `tool with a binary-aware writer) if you need to overwrite it.`; + return { + ok: false, + type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + rawMessage: raw, + displayMessage: `non-text payload; cannot ${verb} via this tool.`, + }; + } + // unknown OR fresh-but-partial: require a fresh full text read. const raw = `File ${filePath} has not been fully read in this session. ` + `Use the ${ToolNames.READ_FILE} tool first (without offset / limit ` + diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index 4dd1323755d..cac82c53e66 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -733,9 +733,21 @@ describe('ReadFileTool', () => { // Slow path returned the actual content (not a placeholder). expect(typeof result.llmContent).toBe('string'); expect(result.llmContent).not.toMatch(/unchanged since/); - // The cache must contain the auto-memory file's entry so a - // subsequent EditTool / WriteFileTool call sees `fresh`. - expect(fileReadCache.check(fs.statSync(memFile)).state).toBe('fresh'); + // The cache must contain the auto-memory file's entry, AND + // the entry must be in a shape that satisfies prior-read + // enforcement on Edit / WriteFile (fresh + lastReadAt set + + // full + cacheable). Asserting only `fresh` would let a + // future regression that records auto-memory reads as + // partial/non-cacheable slip through silently — those reads + // would still report fresh but enforcement would reject + // every follow-up Edit. + const status = fileReadCache.check(fs.statSync(memFile)); + expect(status.state).toBe('fresh'); + if (status.state === 'fresh') { + expect(status.entry.lastReadAt).toBeDefined(); + expect(status.entry.lastReadWasFull).toBe(true); + expect(status.entry.lastReadCacheable).toBe(true); + } } finally { if (previousLocal === undefined) { delete process.env['QWEN_CODE_MEMORY_LOCAL']; diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 5d87c8a1f9b..b785acd4ee8 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -855,6 +855,53 @@ describe('WriteFileTool', () => { fs.unlinkSync(filePath); }); + it('attaches a structured ToolErrorType when getConfirmationDetails rejects', async () => { + // The thrown error must carry `errorType` so the scheduler + // surfaces EDIT_REQUIRES_PRIOR_READ instead of + // UNHANDLED_EXCEPTION on approval-required flows. + const filePath = path.join(rootDir, 'enforce-confirm-type.txt'); + fs.writeFileSync(filePath, 'unread current bytes', 'utf-8'); + const invocation = tool.build({ + file_path: filePath, + content: 'replacement content', + }); + let caught: unknown; + try { + await invocation.getConfirmationDetails(abortSignal); + } catch (err) { + caught = err; + } + expect((caught as { errorType?: string })?.errorType).toBe( + ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + ); + fs.unlinkSync(filePath); + }); + + it('rejects a write with a stat failure other than ENOENT (fail-closed)', async () => { + // checkPriorRead must NOT default to ok:true when stat fails + // for reasons other than disappearance race (EACCES, EBUSY, + // NFS hiccup, ...). Doing so reopens the blind-write path on + // transient metadata errors. + const filePath = path.join(rootDir, 'enforce-stat-fail.txt'); + fs.writeFileSync(filePath, 'untouched', 'utf-8'); + const statSpy = vi + .spyOn(fs.promises, 'stat') + .mockRejectedValueOnce( + Object.assign(new Error('EACCES'), { code: 'EACCES' }), + ); + + const result = await tool + .build({ file_path: filePath, content: 'clobber' }) + .execute(abortSignal); + expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); + expect(result.error?.message).toMatch(/Could not stat .*\(EACCES\)/); + // File untouched. + expect(fs.readFileSync(filePath, 'utf-8')).toBe('untouched'); + + statSpy.mockRestore(); + fs.unlinkSync(filePath); + }); + it('rejects a write when the file has been modified since the last read', async () => { const filePath = path.join(rootDir, 'enforce-stale.txt'); fs.writeFileSync(filePath, 'one', 'utf-8'); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 7c1e6a9642c..fa335dd9b4a 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -35,7 +35,10 @@ import type { LineEnding } from '../services/fileSystemService.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; -import { checkPriorRead } from './priorReadEnforcement.js'; +import { + checkPriorRead, + PriorReadEnforcementError, +} from './priorReadEnforcement.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; import type { ModifiableDeclarativeTool, @@ -133,7 +136,12 @@ class WriteFileToolInvocation extends BaseToolInvocation< 'overwriting', ); if (!decision.ok) { - throw new Error(decision.rawMessage); + // Surface the structured ToolErrorType through scheduler. + // A plain `throw new Error` would hit the scheduler's catch + // block and be reported as UNHANDLED_EXCEPTION — losing the + // EDIT_REQUIRES_PRIOR_READ / FILE_CHANGED_SINCE_READ contract + // for any flow that requires confirmation. + throw new PriorReadEnforcementError(decision.rawMessage, decision.type); } } if (fileExists) { From 248d1791a0619f861b78457b6df14fde0816a849 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 3 May 2026 10:10:43 +0800 Subject: [PATCH 06/16] refactor(core): tighten StructuredToolError naming + add scheduler test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four follow-ups raised by deepseek-v4-pro on PR #3774. None of them change the enforcement boundary; they are all about making the contract clearer and harder to break in future changes. 1. PriorReadEnforcementError -> StructuredToolError. The class now wraps any content-derived ToolErrorType from calculateEdit (EDIT_NO_OCCURRENCE_FOUND, EDIT_EXPECTED_OCCURRENCE_MISMATCH, EDIT_NO_CHANGE, ATTEMPT_TO_CREATE_EXISTING_FILE) on top of the prior-read codes. The old name suggested the class was prior- read-specific, which would mislead any oncall engineer seeing it paired with one of the calculateEdit error codes. 2. EDIT_REQUIRES_PRIOR_READ kept its name (the prefix mentions "edit" but the enum is shared with WriteFileTool) — chose documentation over rename to avoid the churn of a value rename across logs/dashboards already keyed on it. JSDoc now spells out the cross-tool usage explicitly. 3. Stat failures other than ENOENT now map to a new PRIOR_READ_VERIFICATION_FAILED code instead of being conflated with EDIT_REQUIRES_PRIOR_READ. The failure mode is "we cannot verify" rather than "definitely not read" — operators routing on error codes can distinguish the two populations. 4. Added a coreToolScheduler test (`surfaces error.errorType from a confirmation throw instead of UNHANDLED_EXCEPTION`) that constructs a stub tool whose getConfirmationDetails throws StructuredToolError and asserts the surfaced ToolCall response carries the correct ToolErrorType. Without this test the scheduler's explicitErrorType branch would have no coverage at all. Tool tests updated for the new StructuredToolError class name and the PRIOR_READ_VERIFICATION_FAILED code on the EACCES path. --- .../core/src/core/coreToolScheduler.test.ts | 155 ++++++++++++++++++ packages/core/src/core/coreToolScheduler.ts | 2 +- packages/core/src/tools/edit.ts | 7 +- .../core/src/tools/priorReadEnforcement.ts | 34 ++-- packages/core/src/tools/tool-error.ts | 17 ++ packages/core/src/tools/write-file.test.ts | 7 +- packages/core/src/tools/write-file.ts | 7 +- 7 files changed, 207 insertions(+), 22 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 1c917df124a..244b5e83ff7 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -27,6 +27,8 @@ import { DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD, } from '../index.js'; import { SkillTool } from '../tools/skill.js'; +import { StructuredToolError } from '../tools/priorReadEnforcement.js'; +import { ToolErrorType } from '../tools/tool-error.js'; import type { ToolCall, WaitingToolCall } from './coreToolScheduler.js'; import { CoreToolScheduler, @@ -196,6 +198,67 @@ class AbortDuringConfirmationTool extends BaseDeclarativeTool< } } +/** + * Test fixture: a tool whose getConfirmationDetails always throws a + * StructuredToolError carrying a configurable ToolErrorType. Used to + * pin the scheduler's behaviour of propagating error.errorType + * instead of collapsing every confirmation-time throw into + * UNHANDLED_EXCEPTION. + */ +class StructuredErrorOnConfirmationInvocation extends BaseToolInvocation< + Record, + ToolResult +> { + constructor( + private readonly errorType: ToolErrorType, + params: Record, + ) { + super(params); + } + + override async getDefaultPermission(): Promise { + return 'ask'; + } + + override async getConfirmationDetails( + _signal: AbortSignal, + ): Promise { + throw new StructuredToolError( + 'enforcement-rejected-during-confirmation', + this.errorType, + ); + } + + async execute(_abortSignal: AbortSignal): Promise { + throw new Error('execute should not run when confirmation rejects'); + } + + getDescription(): string { + return 'Structured error on confirmation'; + } +} + +class StructuredErrorOnConfirmationTool extends BaseDeclarativeTool< + Record, + ToolResult +> { + constructor(private readonly errorType: ToolErrorType) { + super( + 'structuredErrorOnConfirmationTool', + 'Structured Error On Confirmation Tool', + 'A tool that throws StructuredToolError from getConfirmationDetails.', + Kind.Other, + { type: 'object', properties: {} }, + ); + } + + protected createInvocation( + params: Record, + ): ToolInvocation, ToolResult> { + return new StructuredErrorOnConfirmationInvocation(this.errorType, params); + } +} + async function waitForStatus( onToolCallsUpdate: Mock, status: 'awaiting_approval' | 'executing' | 'success' | 'error' | 'cancelled', @@ -393,6 +456,98 @@ describe('CoreToolScheduler', () => { expect(statuses).not.toContain('error'); }); + it('surfaces error.errorType from a confirmation throw instead of UNHANDLED_EXCEPTION', async () => { + // Without the explicitErrorType extraction in the scheduler's + // catch block, every getConfirmationDetails throw (including + // structured prior-read enforcement rejections) would collapse + // into UNHANDLED_EXCEPTION — losing the new + // EDIT_REQUIRES_PRIOR_READ / FILE_CHANGED_SINCE_READ / + // PRIOR_READ_VERIFICATION_FAILED / EDIT_NO_OCCURRENCE_FOUND / + // ... contracts that StructuredToolError exists to carry. Pin + // the propagation here. + const declarativeTool = new StructuredErrorOnConfirmationTool( + ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + ); + + const mockToolRegistry = { + getTool: () => declarativeTool, + ensureTool: async () => declarativeTool, + getFunctionDeclarations: () => [], + tools: new Map(), + discovery: {}, + registerTool: () => {}, + getToolByName: () => declarativeTool, + getToolByDisplayName: () => declarativeTool, + getTools: () => [], + discoverTools: async () => {}, + getAllTools: () => [], + getToolsByServer: () => [], + } as unknown as ToolRegistry; + + const onAllToolCallsComplete = vi.fn(); + const onToolCallsUpdate = vi.fn(); + + const mockConfig = { + getSessionId: () => 'test-session-id', + getUsageStatisticsEnabled: () => true, + getDebugMode: () => false, + getApprovalMode: () => ApprovalMode.DEFAULT, + getPermissionsAllow: () => [], + getContentGeneratorConfig: () => ({ + model: 'test-model', + authType: 'gemini', + }), + getShellExecutionConfig: () => ({ + terminalWidth: 90, + terminalHeight: 30, + }), + storage: { + getProjectTempDir: () => '/tmp', + }, + getTruncateToolOutputThreshold: () => + DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD, + getTruncateToolOutputLines: () => DEFAULT_TRUNCATE_TOOL_OUTPUT_LINES, + getToolRegistry: () => mockToolRegistry, + getUseModelRouter: () => false, + getGeminiClient: () => null, + getChatRecordingService: () => undefined, + getMessageBus: vi.fn().mockReturnValue(undefined), + getDisableAllHooks: vi.fn().mockReturnValue(true), + } as unknown as Config; + + const scheduler = new CoreToolScheduler({ + config: mockConfig, + onAllToolCallsComplete, + onToolCallsUpdate, + getPreferredEditor: () => 'vscode', + onEditorClose: vi.fn(), + }); + + const request = { + callId: 'structured-1', + name: 'structuredErrorOnConfirmationTool', + args: {}, + isClientInitiated: false, + prompt_id: 'prompt-id-structured', + }; + + await scheduler.schedule([request], new AbortController().signal); + + expect(onAllToolCallsComplete).toHaveBeenCalled(); + const completedCalls = onAllToolCallsComplete.mock + .calls[0][0] as ToolCall[]; + expect(completedCalls[0].status).toBe('error'); + const errored = completedCalls[0] as ToolCall & { + response: { errorType?: ToolErrorType }; + }; + expect(errored.response.errorType).toBe( + ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + ); + expect(errored.response.errorType).not.toBe( + ToolErrorType.UNHANDLED_EXCEPTION, + ); + }); + describe('getToolSuggestion', () => { it('should suggest the top N closest tool names for a typo', () => { // Create mocked tool registry diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index b207eb6e8bc..252b744dc7c 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -1259,7 +1259,7 @@ export class CoreToolScheduler { // Errors thrown from getConfirmationDetails() may carry a // structured ToolErrorType via an `errorType` instance - // field (see PriorReadEnforcementError in + // field (see StructuredToolError in // tools/priorReadEnforcement.ts). When present, surface // that code instead of collapsing every confirmation-time // failure into UNHANDLED_EXCEPTION. diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 6eb7f209574..49a7fca5d5b 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -29,10 +29,7 @@ import { } from '../services/fileSystemService.js'; import type { LineEnding } from '../services/fileSystemService.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; -import { - checkPriorRead, - PriorReadEnforcementError, -} from './priorReadEnforcement.js'; +import { checkPriorRead, StructuredToolError } from './priorReadEnforcement.js'; import { ReadFileTool } from './read-file.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; import { logFileOperation } from '../telemetry/loggers.js'; @@ -339,7 +336,7 @@ class EditToolInvocation implements ToolInvocation { // FILE_CHANGED_SINCE_READ). Wrap in PriorReadEnforcementError // so coreToolScheduler surfaces the correct ToolErrorType // instead of UNHANDLED_EXCEPTION. - throw new PriorReadEnforcementError( + throw new StructuredToolError( `Edit error: ${editData.error.display}`, editData.error.type, ); diff --git a/packages/core/src/tools/priorReadEnforcement.ts b/packages/core/src/tools/priorReadEnforcement.ts index 5a8061b529c..79914733a0a 100644 --- a/packages/core/src/tools/priorReadEnforcement.ts +++ b/packages/core/src/tools/priorReadEnforcement.ts @@ -10,17 +10,26 @@ import { ToolErrorType } from './tool-error.js'; import { ToolNames } from './tool-names.js'; /** - * Error thrown by `getConfirmationDetails()` when prior-read enforcement - * rejects a request before any diff is rendered. Carries the structured - * `ToolErrorType` so the tool scheduler can surface it as the right - * tool error code (`EDIT_REQUIRES_PRIOR_READ` / - * `FILE_CHANGED_SINCE_READ`) instead of the generic - * `UNHANDLED_EXCEPTION` that any plain `Error` would otherwise become. + * Error thrown by `getConfirmationDetails()` when it needs to surface + * a structured `ToolErrorType` to the scheduler instead of letting + * the throw collapse into a generic `UNHANDLED_EXCEPTION`. Originally + * introduced for prior-read enforcement (hence the file location) + * but now also carries other content-derived `calculateEdit` errors + * — `EDIT_NO_OCCURRENCE_FOUND`, `EDIT_EXPECTED_OCCURRENCE_MISMATCH`, + * `EDIT_NO_CHANGE`, `ATTEMPT_TO_CREATE_EXISTING_FILE` — through the + * confirmation path so they keep their proper error code instead of + * being reported as "unhandled exception". * * Caught by `coreToolScheduler` via the `errorType` instance field. + * + * Naming note: kept generic (`StructuredToolError`) rather than + * `PriorReadEnforcementError` so the name matches the broader set of + * `ToolErrorType` values it actually carries — an oncall engineer + * seeing this in a log paired with `edit_no_occurrence_found` should + * not have to wonder what prior-read has to do with it. */ -export class PriorReadEnforcementError extends Error { - override readonly name = 'PriorReadEnforcementError'; +export class StructuredToolError extends Error { + override readonly name = 'StructuredToolError'; constructor( message: string, readonly errorType: ToolErrorType, @@ -109,7 +118,12 @@ export async function checkPriorRead( // Any other stat failure: fail closed. We cannot prove the file // has been read; treating that as approval would silently bypass // enforcement on transient metadata errors that don't prevent - // the subsequent write from succeeding. + // the subsequent write from succeeding. Use a distinct + // PRIOR_READ_VERIFICATION_FAILED code (rather than + // EDIT_REQUIRES_PRIOR_READ) because the model may have + // legitimately read this file — we just cannot verify it. + // Operators monitoring on error codes can route the two + // populations separately. const raw = `Could not stat ${filePath} to verify prior read (${code ?? 'unknown error'}). ` + `Re-read with the ${ToolNames.READ_FILE} tool, then retry ${verb} it.`; @@ -117,7 +131,7 @@ export async function checkPriorRead( verb === 'editing' ? 'editing this file' : 'overwriting this file'; return { ok: false, - type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + type: ToolErrorType.PRIOR_READ_VERIFICATION_FAILED, rawMessage: raw, displayMessage: `cannot verify prior read of ${filePath}; re-run ${ToolNames.READ_FILE} before ${verbDisplay}.`, }; diff --git a/packages/core/src/tools/tool-error.ts b/packages/core/src/tools/tool-error.ts index 9fdb10dcdd9..6467e4ca948 100644 --- a/packages/core/src/tools/tool-error.ts +++ b/packages/core/src/tools/tool-error.ts @@ -40,6 +40,15 @@ export enum ToolErrorType { // has not (yet) read this session via ReadFile. The session-scoped // FileReadCache is the source of truth — see read-file.ts and the // related FileReadCache service. + // + // Despite the `EDIT_` prefix this code is shared between EditTool + // and WriteFileTool: the boundary it guards is "the model is about + // to mutate bytes it has not legitimately seen", which both tools + // can hit. The prefix is kept for backwards-compatibility with + // logs/dashboards already keyed on it; consumers that need to + // distinguish edit-vs-write should look at the originating tool + // name in the surrounding ToolCallEvent rather than the error + // code itself. EDIT_REQUIRES_PRIOR_READ = 'edit_requires_prior_read', // Returned when Edit / WriteFile is asked to mutate a file the model // *has* read this session, but the on-disk bytes have changed since @@ -47,6 +56,14 @@ export enum ToolErrorType { // is expected to re-read with ReadFile to refresh its mental model // before retrying the edit. FILE_CHANGED_SINCE_READ = 'file_changed_since_read', + // Returned when Edit / WriteFile cannot determine whether the model + // has read a file because `fs.stat` itself failed for a reason + // other than ENOENT (typically EACCES, EBUSY, or an NFS hiccup). + // Distinct from EDIT_REQUIRES_PRIOR_READ ("definitely not read") + // because the model may have legitimately read the file — we just + // cannot verify. Operators monitoring on error codes can route this + // separately. + PRIOR_READ_VERIFICATION_FAILED = 'prior_read_verification_failed', // Glob-specific Errors GLOB_EXECUTION_ERROR = 'glob_execution_error', diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index b785acd4ee8..1c300dae71e 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -893,7 +893,12 @@ describe('WriteFileTool', () => { const result = await tool .build({ file_path: filePath, content: 'clobber' }) .execute(abortSignal); - expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); + // Distinct error code: the model may have legitimately read the + // file — we just cannot verify because stat itself failed. + // EDIT_REQUIRES_PRIOR_READ would imply "definitely not read". + expect(result.error?.type).toBe( + ToolErrorType.PRIOR_READ_VERIFICATION_FAILED, + ); expect(result.error?.message).toMatch(/Could not stat .*\(EACCES\)/); // File untouched. expect(fs.readFileSync(filePath, 'utf-8')).toBe('untouched'); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index fa335dd9b4a..0e54b77702c 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -35,10 +35,7 @@ import type { LineEnding } from '../services/fileSystemService.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; -import { - checkPriorRead, - PriorReadEnforcementError, -} from './priorReadEnforcement.js'; +import { checkPriorRead, StructuredToolError } from './priorReadEnforcement.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; import type { ModifiableDeclarativeTool, @@ -141,7 +138,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< // block and be reported as UNHANDLED_EXCEPTION — losing the // EDIT_REQUIRES_PRIOR_READ / FILE_CHANGED_SINCE_READ contract // for any flow that requires confirmation. - throw new PriorReadEnforcementError(decision.rawMessage, decision.type); + throw new StructuredToolError(decision.rawMessage, decision.type); } } if (fileExists) { From 323e0800c07786966716ab5b67414126973481e5 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 3 May 2026 11:15:20 +0800 Subject: [PATCH 07/16] fix(core): close TOCTOU + grammar + directory regressions in PR-B MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six concrete issues that the previous round of enforcement work left open. Reported by Copilot via /review on PR #3774. 1. TOCTOU window between pre-read checkPriorRead and readTextFile. The pre-read stat could pass enforcement, then an external writer could land between that stat and the actual read, leaving currentContent reflecting bytes the model never saw — exactly the stale-write path the PR is supposed to block. Closed by re-running checkPriorRead immediately after every readTextFile that fed currentContent / originalContent: EditTool.calculateEdit and the two WriteFileTool paths (execute + getConfirmationDetails). A `stale` outcome now fails the operation with FILE_CHANGED_SINCE_READ at the correct moment. 2. Directory targets sent the model into an enforcement loop. `fileExists` is a plain access check, so directories also entered the enforcement branch — the model would be told to call `read_file`, but `read_file` rejects directories with TARGET_IS_DIRECTORY, so the loop never terminated. Fixed in checkPriorRead: if `fs.stat` reports the path is not a regular file, return `ok: true` so the downstream readTextFile / write path can surface its own EISDIR / similar error. 3. Confirmation-time error messages used the short `display` form instead of the full `raw` form. Approval-required Edit calls therefore lost the remediation detail (file path, stale-vs-unread distinction, "without offset / limit / pages" hint) that the execute path already surfaced and that the WriteFile confirmation path already preserved. EditTool.getConfirmationDetails now throws StructuredToolError with `editData.error.raw`. 4. Non-text payload displayMessage was grammatically broken: built from the gerund `editing` / `overwriting`, it rendered as "cannot editing via this tool" / "cannot overwriting via this tool". Fixed by deriving a bare-verb form (`edit` / `overwrite`) alongside the gerund and using it in displayMessage. (Items 1, 5 and 6 from Copilot's batch are the same TOCTOU class — EditTool calculateEdit + WriteFile execute + WriteFile confirmation — addressed together in (1) above.) The "bypasses enforcement entirely" test now uses mockReturnValue instead of mockReturnValueOnce because calculateEdit calls getFileReadCacheDisabled twice — once for the pre-read check and once for the post-read TOCTOU re-check. Both must see disabled=true to actually bypass. --- packages/core/src/tools/edit.test.ts | 8 +++- packages/core/src/tools/edit.ts | 48 +++++++++++++++---- .../core/src/tools/priorReadEnforcement.ts | 18 ++++++- packages/core/src/tools/write-file.ts | 43 +++++++++++++++++ 4 files changed, 105 insertions(+), 12 deletions(-) diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 4250cc00777..734a749709f 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -1051,7 +1051,7 @@ describe('EditTool', () => { }); await expect( invocation.getConfirmationDetails(abortSignal), - ).rejects.toThrow(/read_file required before editing this file/); + ).rejects.toThrow(/has not been fully read in this session/); }); it('rejects an edit when the file has been modified since the last read', async () => { @@ -1153,7 +1153,11 @@ describe('EditTool', () => { fs.writeFileSync(filePath, 'untouched', 'utf8'); // No seed: with the cache disabled, the model is on the // pre-cache contract — Edit must succeed without a prior Read. - (mockConfig.getFileReadCacheDisabled as Mock).mockReturnValueOnce(true); + // Use mockReturnValue (not mockReturnValueOnce): calculateEdit + // now calls getFileReadCacheDisabled twice — once before + // readTextFile and once after, for the post-read TOCTOU + // re-check — and both must see disabled=true to actually bypass. + (mockConfig.getFileReadCacheDisabled as Mock).mockReturnValue(true); const params: EditToolParams = { file_path: filePath, old_string: 'untouched', diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 49a7fca5d5b..f7c09f53a89 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -210,6 +210,38 @@ class EditToolInvocation implements ToolInvocation { } } + // Post-read freshness re-check. The pre-read checkPriorRead above + // and readTextFile are two separate syscalls; if the file is + // modified between them, currentContent reflects post-write bytes + // the model never saw and any edit applied to it would still be + // a stale-write. Re-running checkPriorRead here closes the TOCTOU + // window: a stale state now (mtime/size drifted) means we read + // bytes the cache no longer trusts, and we reject before + // returning a CalculatedEdit that the call sites would honour. + if (fileExists && !this.config.getFileReadCacheDisabled()) { + const postDecision = await checkPriorRead( + this.config.getFileReadCache(), + params.file_path, + 'editing', + ); + if (!postDecision.ok) { + return { + currentContent: null, + newContent: '', + occurrences: 0, + error: { + display: postDecision.displayMessage, + raw: postDecision.rawMessage, + type: postDecision.type, + }, + isNewFile: false, + encoding: 'utf-8', + bom: false, + lineEnding: 'lf', + }; + } + } + const normalizedStrings = normalizeEditStrings( currentContent, finalOldString, @@ -331,15 +363,13 @@ class EditToolInvocation implements ToolInvocation { } if (editData.error) { - // The error type from `calculateEdit` includes the new - // prior-read codes (EDIT_REQUIRES_PRIOR_READ / - // FILE_CHANGED_SINCE_READ). Wrap in PriorReadEnforcementError - // so coreToolScheduler surfaces the correct ToolErrorType - // instead of UNHANDLED_EXCEPTION. - throw new StructuredToolError( - `Edit error: ${editData.error.display}`, - editData.error.type, - ); + // Use the full `raw` message, not the short `display` form: + // the scheduler propagates `error.message` straight into the + // model-facing tool response. `raw` carries the remediation + // detail (file path, stale-vs-unread distinction, "without + // offset / limit / pages" hint) that `execute()` already + // surfaces — confirmation-required flows should not lose it. + throw new StructuredToolError(editData.error.raw, editData.error.type); } const fileName = path.basename(this.params.file_path); diff --git a/packages/core/src/tools/priorReadEnforcement.ts b/packages/core/src/tools/priorReadEnforcement.ts index 79914733a0a..8c5b9aca731 100644 --- a/packages/core/src/tools/priorReadEnforcement.ts +++ b/packages/core/src/tools/priorReadEnforcement.ts @@ -136,6 +136,17 @@ export async function checkPriorRead( displayMessage: `cannot verify prior read of ${filePath}; re-run ${ToolNames.READ_FILE} before ${verbDisplay}.`, }; } + // Defensive: if stat succeeded but the path is not a regular file + // (directory, FIFO, socket, device), let the downstream + // readTextFile / write path surface its own error code (e.g. + // EISDIR -> TARGET_IS_DIRECTORY) instead of telling the model to + // re-read with read_file. read_file rejects directories with a + // dedicated error, so an enforcement loop here would tell the + // model to call read_file forever for directory targets — a + // regression vs the pre-PR behaviour. + if (!stats.isFile()) { + return { ok: true }; + } const status = cache.check(stats); if ( status.state === 'fresh' && @@ -176,11 +187,16 @@ export async function checkPriorRead( `WriteFile tools cannot mutate that payload safely — re-reading ` + `it would not change this. Use a different mechanism (e.g. shell ` + `tool with a binary-aware writer) if you need to overwrite it.`; + // displayMessage uses the bare verb (`edit` / `overwrite`) + // rather than the gerund — the noun phrase "cannot editing + // via this tool" would be ungrammatical, and `returnDisplay` + // surfaces this string directly to the user. + const verbBare = verb === 'editing' ? 'edit' : 'overwrite'; return { ok: false, type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, rawMessage: raw, - displayMessage: `non-text payload; cannot ${verb} via this tool.`, + displayMessage: `non-text payload; cannot ${verbBare} via this tool.`, }; } // unknown OR fresh-but-partial: require a fresh full text read. diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 0e54b77702c..c77e13b83ad 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -154,6 +154,25 @@ class WriteFileToolInvocation extends BaseToolInvocation< } } + // Post-read freshness re-check. Closes the TOCTOU window between + // the pre-read checkPriorRead above and the readTextFile that + // produced `originalContent`: showing the user a diff computed + // from bytes the model never saw is the very confusing-approval + // UX this enforcement block exists to prevent. + if (fileExists && !this.config.getFileReadCacheDisabled()) { + const postDecision = await checkPriorRead( + this.config.getFileReadCache(), + this.params.file_path, + 'overwriting', + ); + if (!postDecision.ok) { + throw new StructuredToolError( + postDecision.rawMessage, + postDecision.type, + ); + } + } + const relativePath = makeRelative( this.params.file_path, this.config.getTargetDir(), @@ -260,6 +279,30 @@ class WriteFileToolInvocation extends BaseToolInvocation< } } + // Post-read freshness re-check. Closes the TOCTOU window between + // the pre-read checkPriorRead above and the readTextFile that + // produced `originalContent`: an external write that lands + // between those two syscalls would otherwise overwrite bytes the + // model never saw, even though enforcement was supposed to block + // exactly that. + if (fileExists && !this.config.getFileReadCacheDisabled()) { + const postDecision = await checkPriorRead( + this.config.getFileReadCache(), + file_path, + 'overwriting', + ); + if (!postDecision.ok) { + return { + llmContent: postDecision.rawMessage, + returnDisplay: `Error: ${postDecision.displayMessage}`, + error: { + message: postDecision.rawMessage, + type: postDecision.type, + }, + }; + } + } + if (!fileExists) { fs.mkdirSync(dirName, { recursive: true }); const userEncoding = this.config.getDefaultFileEncoding(); From f4ef75667e3f9fe92fb0d102c11e1dca20a4b3af Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 3 May 2026 12:23:24 +0800 Subject: [PATCH 08/16] fix(core): close fileExists TOCTOU on WriteFile prior-read enforcement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WriteFile gated prior-read enforcement on `fileExists` from `isFilefileExists()`, but a file that sprang into existence between that check and the write would still be overwritten without enforcement — `fileExists === false` skipped the check entirely. Made the gate unconditional on `fileExists`. checkPriorRead's own `fs.stat` decides what to do: - ENOENT → ok:true, fall through to the new-file path as before - file exists right now (whether or not isFilefileExists saw it) → unknown / stale check runs, the race-created file is rejected. Applied to both getConfirmationDetails and execute. The path that actually creates new files is unchanged because checkPriorRead's ENOENT branch is the disappearance-race exit, which is the correct exit for "the file truly does not exist yet". Reported by glm-5.1 via /review on PR #3774. --- packages/core/src/tools/write-file.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index c77e13b83ad..f62333d1bdf 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -126,7 +126,15 @@ class WriteFileToolInvocation extends BaseToolInvocation< // computed from current bytes that the model has never received, // and the subsequent execute() would still reject the call — // confusing UX for any approve flow. - if (fileExists && !this.config.getFileReadCacheDisabled()) { + // + // Run unconditionally (not gated on `fileExists`): checkPriorRead's + // own stat decides whether the file actually exists right now. + // ENOENT means the path is genuinely absent → ok:true → fall + // through to the new-file diff; any other "stat says yes" outcome + // (including the file appearing between isFilefileExists() and + // here, a race window the pre-fix gating left wide open) means + // the model is about to clobber bytes it never read → reject. + if (!this.config.getFileReadCacheDisabled()) { const decision = await checkPriorRead( this.config.getFileReadCache(), this.params.file_path, @@ -222,7 +230,13 @@ class WriteFileToolInvocation extends BaseToolInvocation< // - we should not be holding bytes of a file the model never // legitimately saw, even transiently. // Mirrors the order in getConfirmationDetails() above. - if (fileExists && !this.config.getFileReadCacheDisabled()) { + // + // Run unconditionally (not gated on `fileExists`): checkPriorRead + // re-stats so a file that sprang into existence between + // isFilefileExists() and here — exactly the TOCTOU window pointed + // out in review — is now caught and rejected instead of being + // silently overwritten. + if (!this.config.getFileReadCacheDisabled()) { const decision = await checkPriorRead( this.config.getFileReadCache(), file_path, From a9ec1a8ad0b2fa7b30bfa43f388ed54e64ef0c1a Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 3 May 2026 15:31:41 +0800 Subject: [PATCH 09/16] fix(core): close 4 enforcement gaps + 1 critical bug from 5th Copilot review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six issues raised by deepseek-v4-pro / glm-5.1 / qwen3.6-plus on PR #3774. Listed by reviewer-assigned severity. [Critical] (qwen3.6-plus) recordWrite previously only seeded the read metadata for brand-new entries. The reproduction was real: ReadFile(limit=10) → WriteFile(full content) → Edit. The partial read's lastReadWasFull=false would persist through the write, and the Edit would be rejected with EDIT_REQUIRES_PRIOR_READ even though the model just authored every byte. recordWrite now unconditionally refreshes lastReadAt, lastReadWasFull=true, and lastReadCacheable=true. The fileReadCache.test.ts case that previously asserted "preserves lastReadAt" is rewritten to assert the new "refreshes lastReadAt to match the write" contract, and a new "upgrades lastReadWasFull / lastReadCacheable after a full write" regression test pins the reproduction reviewer described. [Suggestion] (deepseek-v4-pro) Narrowed the non-regular-file bypass in priorReadEnforcement from `!stats.isFile()` to `stats.isDirectory()`. The earlier broad form covered FIFOs, sockets, and devices that the model has no legitimate "read first" recourse for and that can block readTextFile (FIFO) or over-allocate (/dev/urandom). Those now flow through to cache.check() and reject with the unread-file path before any I/O. [Suggestion] (glm-5.1) Removed the `fileExists && ...` gate from EditTool.calculateEdit, mirroring the f4ef75667 fix on WriteFile. A file that springs into existence between isFilefileExists() and the enforcement check is now caught here as well; ENOENT inside checkPriorRead remains the disappearance-race exit and new-file creation flow is unchanged. [Suggestion] (deepseek-v4-pro) Added debugLogger.warn() at every post-read TOCTOU rejection site (Edit calculateEdit, WriteFile getConfirmationDetails, WriteFile execute). These rejections are rare and self-healing — without a debug record, an operator investigating "why did this Edit fail once?" had nothing to grep. debugLogger uses dedicated 'EDIT_PRIOR_READ' / 'WRITE_FILE' tags. [Suggestion] (qwen3.6-plus) Added a final pre-write checkPriorRead in EditTool.execute() and WriteFileTool.execute(). The earlier post-read check ran inside calculateEdit (Edit) or before mkdirSync (WriteFile), but the actual writeTextFile call could be arbitrarily later — user approval, modify-and-confirm, etc. The window from "post-read check → writeTextFile" is now bounded to "pre-write stat → writeTextFile" (two adjacent syscalls). --- .../core/src/services/fileReadCache.test.ts | 45 +++++++++++++--- packages/core/src/services/fileReadCache.ts | 25 +++++---- packages/core/src/tools/edit.ts | 54 +++++++++++++++++-- .../core/src/tools/priorReadEnforcement.ts | 25 +++++---- packages/core/src/tools/write-file.ts | 34 ++++++++++++ 5 files changed, 151 insertions(+), 32 deletions(-) diff --git a/packages/core/src/services/fileReadCache.test.ts b/packages/core/src/services/fileReadCache.test.ts index d8dd8ef5990..6b3af18fdd2 100644 --- a/packages/core/src/services/fileReadCache.test.ts +++ b/packages/core/src/services/fileReadCache.test.ts @@ -243,7 +243,12 @@ describe('FileReadCache', () => { ); }); - it('preserves lastReadAt set by an earlier recordRead', () => { + it('refreshes lastReadAt to match the write — the author saw all bytes', () => { + // recordWrite always re-stamps the read metadata: the model + // authored the bytes it just wrote, so for prior-read + // enforcement purposes it now counts as having seen the full + // current content, regardless of whatever partial / ranged / + // non-cacheable read happened earlier in the session. const cache = new FileReadCache(); const stats = makeStats(); cache.recordRead('/x/foo.ts', stats, { full: true, cacheable: true }); @@ -253,8 +258,30 @@ describe('FileReadCache', () => { '/x/foo.ts', makeStats({ mtimeMs: 9999 }), ); - expect(entry.lastReadAt).toBe(readTime); expect(entry.lastWriteAt).toBeGreaterThan(readTime); + expect(entry.lastReadAt).toBe(entry.lastWriteAt); + }); + + it('upgrades lastReadWasFull / lastReadCacheable after a full write', () => { + // Reproduction for the gap reviewer flagged: ReadFile(limit=10) + // → WriteFile(full) → Edit. Pre-fix, the partial read's + // lastReadWasFull=false persisted through the write and the + // Edit would be rejected with EDIT_REQUIRES_PRIOR_READ. + const cache = new FileReadCache(); + const stats = makeStats(); + cache.recordRead('/x/foo.ts', stats, { full: false, cacheable: true }); + const partialEntry = cache.check(stats); + expect(partialEntry.state).toBe('fresh'); + if (partialEntry.state === 'fresh') { + expect(partialEntry.entry.lastReadWasFull).toBe(false); + } + cache.recordWrite('/x/foo.ts', makeStats({ mtimeMs: 2000 })); + const afterWrite = cache.check(makeStats({ mtimeMs: 2000 })); + expect(afterWrite.state).toBe('fresh'); + if (afterWrite.state === 'fresh') { + expect(afterWrite.entry.lastReadWasFull).toBe(true); + expect(afterWrite.entry.lastReadCacheable).toBe(true); + } }); }); @@ -267,10 +294,14 @@ describe('FileReadCache', () => { vi.useRealTimers(); }); - it('records lastWriteAt > lastReadAt after Read → Write', () => { - // PR2 will use this ordering to decide whether a Read can return a - // file_unchanged placeholder. Verifying the timestamps move - // monotonically here gives that downstream logic a stable contract. + it('records lastWriteAt === lastReadAt after Read → Write', () => { + // recordWrite refreshes the read metadata to the write time + // because the model authored the bytes it just wrote and now + // counts as having seen them. ReadFile's file_unchanged + // fast-path therefore checks `lastReadAt > lastWriteAt` + // (strictly greater): equal timestamps mean "the last + // operation was a write" and a follow-up Read should re-emit + // the bytes rather than serve a placeholder. const cache = new FileReadCache(); cache.recordRead('/x/foo.ts', makeStats({ mtimeMs: 1000 }), { full: true, @@ -284,7 +315,7 @@ describe('FileReadCache', () => { const { lastReadAt, lastWriteAt } = result.entry; expect(lastReadAt).toBeDefined(); expect(lastWriteAt).toBeDefined(); - expect(lastWriteAt!).toBeGreaterThan(lastReadAt!); + expect(lastReadAt).toBe(lastWriteAt); } }); diff --git a/packages/core/src/services/fileReadCache.ts b/packages/core/src/services/fileReadCache.ts index f4537497c2b..3a39696dc06 100644 --- a/packages/core/src/services/fileReadCache.ts +++ b/packages/core/src/services/fileReadCache.ts @@ -119,24 +119,23 @@ export class FileReadCache { * fingerprint to the post-write Stats; otherwise the next Edit would * see its own write as a "stale" external change. * - * If the entry has never been Read in this session (i.e. this is a - * fresh file just created by Edit / WriteFile), populate the read - * metadata as well: the model authored the bytes it just wrote, so - * for the purposes of prior-read enforcement it has effectively - * "seen" the full text content. Without this, a create→edit→edit - * chain would be rejected on the second edit because - * `lastReadAt`/`lastReadWasFull`/`lastReadCacheable` would still be - * undefined. + * Read metadata is **always** refreshed alongside the write, not + * just for brand-new entries: the model authored the entire current + * content, so for prior-read enforcement purposes it has now "seen" + * all bytes — regardless of whether the prior recordRead happened + * to be partial (`lastReadWasFull=false`) or non-cacheable + * (`lastReadCacheable=false`). Without this, a sequence such as + * `ReadFile(limit=10)` → `WriteFile` (full content) → `Edit` would + * be rejected on the Edit because `lastReadWasFull=false` from the + * earlier partial read would persist through the write. */ recordWrite(absPath: string, stats: Stats): FileReadEntry { const entry = this.upsert(absPath, stats); const now = Date.now(); entry.lastWriteAt = now; - if (entry.lastReadAt === undefined) { - entry.lastReadAt = now; - entry.lastReadWasFull = true; - entry.lastReadCacheable = true; - } + entry.lastReadAt = now; + entry.lastReadWasFull = true; + entry.lastReadCacheable = true; return entry; } diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index f7c09f53a89..16d5311079d 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -31,6 +31,7 @@ import type { LineEnding } from '../services/fileSystemService.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; import { checkPriorRead, StructuredToolError } from './priorReadEnforcement.js'; import { ReadFileTool } from './read-file.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; import { logFileOperation } from '../telemetry/loggers.js'; import { FileOperationEvent } from '../telemetry/types.js'; @@ -52,6 +53,8 @@ import { normalizeEditStrings, } from '../utils/editHelper.js'; +const debugLogger = createDebugLogger('EDIT_PRIOR_READ'); + export function applyReplacement( currentContent: string | null, oldString: string, @@ -157,9 +160,15 @@ class EditToolInvocation implements ToolInvocation { // the read pipeline below (and the content-derived error codes // it can produce — NO_OCCURRENCE_FOUND, EXPECTED_OCCURRENCE_MISMATCH, // NO_CHANGE) cannot be used as a read-less content oracle on a - // file the model has never legitimately Read. New-file creation - // is exempt because there are no current bytes to read first. - if (fileExists && !this.config.getFileReadCacheDisabled()) { + // file the model has never legitimately Read. + // + // Run unconditionally (not gated on `fileExists`): checkPriorRead + // re-stats so a file that sprang into existence between + // isFilefileExists() and here — the same TOCTOU window WriteFile + // had — is now caught. ENOENT (genuinely absent) returns ok:true + // and falls through to the new-file path; an existing file that + // appeared in the race window is rejected as unread. + if (!this.config.getFileReadCacheDisabled()) { const decision = await checkPriorRead( this.config.getFileReadCache(), params.file_path, @@ -225,6 +234,15 @@ class EditToolInvocation implements ToolInvocation { 'editing', ); if (!postDecision.ok) { + // Forensic trail for post-read TOCTOU rejections. These are + // rare ("file changed between stat and read") and the model + // self-heals by re-reading, so without a debug record an + // operator investigating "why did this Edit fail once?" has + // nothing to grep. + debugLogger.warn('post-read TOCTOU rejection', { + path: params.file_path, + reason: postDecision.type, + }); return { currentContent: null, newContent: '', @@ -458,6 +476,36 @@ class EditToolInvocation implements ToolInvocation { try { this.ensureParentDirectoriesExist(this.params.file_path); + // Final pre-write freshness check. calculateEdit() ran a + // post-read check, but execute() can be called arbitrarily + // long after that (user approval, modify-and-confirm, etc.). + // Between the post-read check and the writeTextFile below, + // an external mutation could land and be silently overwritten. + // This last guard tightens the window from "post-read → + // writeTextFile (unbounded)" to "stat → writeTextFile (two + // syscalls)". + if (!editData.isNewFile && !this.config.getFileReadCacheDisabled()) { + const writeDecision = await checkPriorRead( + this.config.getFileReadCache(), + this.params.file_path, + 'editing', + ); + if (!writeDecision.ok) { + debugLogger.warn('pre-write TOCTOU rejection', { + path: this.params.file_path, + reason: writeDecision.type, + }); + return { + llmContent: writeDecision.rawMessage, + returnDisplay: `Error: ${writeDecision.displayMessage}`, + error: { + message: writeDecision.rawMessage, + type: writeDecision.type, + }, + }; + } + } + // For new files, apply default file encoding setting // For existing files, preserve the original encoding (BOM and charset) if (editData.isNewFile) { diff --git a/packages/core/src/tools/priorReadEnforcement.ts b/packages/core/src/tools/priorReadEnforcement.ts index 8c5b9aca731..08a46af2fd9 100644 --- a/packages/core/src/tools/priorReadEnforcement.ts +++ b/packages/core/src/tools/priorReadEnforcement.ts @@ -136,15 +136,22 @@ export async function checkPriorRead( displayMessage: `cannot verify prior read of ${filePath}; re-run ${ToolNames.READ_FILE} before ${verbDisplay}.`, }; } - // Defensive: if stat succeeded but the path is not a regular file - // (directory, FIFO, socket, device), let the downstream - // readTextFile / write path surface its own error code (e.g. - // EISDIR -> TARGET_IS_DIRECTORY) instead of telling the model to - // re-read with read_file. read_file rejects directories with a - // dedicated error, so an enforcement loop here would tell the - // model to call read_file forever for directory targets — a - // regression vs the pre-PR behaviour. - if (!stats.isFile()) { + // Narrow the bypass to **directories only**. The earlier + // !stats.isFile() form was too broad: it also covered FIFOs, + // sockets, and character / block devices, none of which the + // model has a legitimate way to "read first" anyway, and + // letting those flow through to readTextFile risks blocking + // I/O (FIFO) or over-allocation (/dev/urandom). + // + // Directory targets get the bypass because read_file already has + // a dedicated TARGET_IS_DIRECTORY error and an enforcement loop + // ("call read_file first" → "read_file says directory") would be + // a real regression vs the pre-PR behaviour. + // + // Other non-regular files fall through to cache.check(): they + // will report `unknown` (no entry can exist because read_file + // rejects them) and enforcement will reject before any I/O. + if (stats.isDirectory()) { return { ok: true }; } const status = cache.check(stats); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index f62333d1bdf..79d50d0c7d6 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -174,6 +174,10 @@ class WriteFileToolInvocation extends BaseToolInvocation< 'overwriting', ); if (!postDecision.ok) { + debugLogger.warn('post-read TOCTOU rejection (confirmation)', { + path: this.params.file_path, + reason: postDecision.type, + }); throw new StructuredToolError( postDecision.rawMessage, postDecision.type, @@ -306,6 +310,10 @@ class WriteFileToolInvocation extends BaseToolInvocation< 'overwriting', ); if (!postDecision.ok) { + debugLogger.warn('post-read TOCTOU rejection (execute)', { + path: file_path, + reason: postDecision.type, + }); return { llmContent: postDecision.rawMessage, returnDisplay: `Error: ${postDecision.displayMessage}`, @@ -333,6 +341,32 @@ class WriteFileToolInvocation extends BaseToolInvocation< detectedEncoding = undefined; } + // Final pre-write freshness check. The earlier post-read check + // ran before mkdirSync / encoding detection; we re-stat here so + // an external mutation that lands in the gap between those + // operations and the writeTextFile below is caught. + if (fileExists && !this.config.getFileReadCacheDisabled()) { + const writeDecision = await checkPriorRead( + this.config.getFileReadCache(), + file_path, + 'overwriting', + ); + if (!writeDecision.ok) { + debugLogger.warn('pre-write TOCTOU rejection', { + path: file_path, + reason: writeDecision.type, + }); + return { + llmContent: writeDecision.rawMessage, + returnDisplay: `Error: ${writeDecision.displayMessage}`, + error: { + message: writeDecision.rawMessage, + type: writeDecision.type, + }, + }; + } + } + try { await this.config.getFileSystemService().writeTextFile({ path: file_path, From fa9344c90d92f2f05e0cb13863e877afa9878e85 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 3 May 2026 18:06:34 +0800 Subject: [PATCH 10/16] fix(core): close new-file race + special-file enforcement loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues from the latest Copilot review on PR #3774. 1. New-file race in pre-write enforcement (write-file.ts:348, edit.ts:487). The earlier pre-write checkPriorRead was gated on `fileExists` (WriteFile) and `!editData.isNewFile` (Edit). If the path was absent at planning time and another process created it while approval was pending, the gated form would skip enforcement and silently overwrite a pre-existing file the model never read. Run unconditionally in both tools — checkPriorRead's own ENOENT branch is the disappearance-race exit, so genuine new-file creation is unaffected, but a race-created file now hits the `unknown` branch and is rejected as unread. 2. FIFO / socket / device sent the model into an enforcement loop (priorReadEnforcement.ts:220). After narrowing the non-regular-file bypass to directories only, FIFOs etc. fell through to cache.check, returned `unknown`, and produced a "use read_file first" message — but read_file rejects those same targets as "not a regular file", so the model would loop on read_file forever. Added a dedicated `!stats.isFile()` branch (after the directory exemption) that returns a "special file; cannot edit/overwrite via this tool — use shell instead" message, matching the shape of the existing non-text-payload guidance. (Tool-error.ts and the non-cacheable policy notes are addressed in the PR description update — not in code.) --- packages/core/src/tools/edit.ts | 9 ++++++- .../core/src/tools/priorReadEnforcement.ts | 24 ++++++++++++++++--- packages/core/src/tools/write-file.ts | 10 +++++++- 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 16d5311079d..79409b9d5f2 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -484,7 +484,14 @@ class EditToolInvocation implements ToolInvocation { // This last guard tightens the window from "post-read → // writeTextFile (unbounded)" to "stat → writeTextFile (two // syscalls)". - if (!editData.isNewFile && !this.config.getFileReadCacheDisabled()) { + // + // Run unconditionally (not gated on `editData.isNewFile`): + // `isNewFile` was decided back in calculateEdit, but a file + // could be created in the gap between then and now and a + // confirmation-pending Edit would otherwise clobber it + // without enforcement. ENOENT inside checkPriorRead returns + // ok:true so genuine new-file creation is unaffected. + if (!this.config.getFileReadCacheDisabled()) { const writeDecision = await checkPriorRead( this.config.getFileReadCache(), this.params.file_path, diff --git a/packages/core/src/tools/priorReadEnforcement.ts b/packages/core/src/tools/priorReadEnforcement.ts index 08a46af2fd9..df9108bb2cd 100644 --- a/packages/core/src/tools/priorReadEnforcement.ts +++ b/packages/core/src/tools/priorReadEnforcement.ts @@ -148,12 +148,30 @@ export async function checkPriorRead( // ("call read_file first" → "read_file says directory") would be // a real regression vs the pre-PR behaviour. // - // Other non-regular files fall through to cache.check(): they - // will report `unknown` (no entry can exist because read_file - // rejects them) and enforcement will reject before any I/O. + // Non-regular non-directory paths (FIFO / socket / character / + // block device) get a dedicated rejection: read_file rejects + // them as "not a regular file" with no recovery, so telling the + // model to "call read_file first" would loop forever. The + // dedicated message tells the model the path cannot be edited + // via Edit / WriteFile at all and to use a different tool. if (stats.isDirectory()) { return { ok: true }; } + if (!stats.isFile()) { + const verbBare = verb === 'editing' ? 'edit' : 'overwrite'; + const raw = + `${filePath} is a FIFO / socket / character or block device. ` + + `The Edit / WriteFile tools only operate on regular files; ` + + `the ${ToolNames.READ_FILE} tool also rejects these targets. ` + + `Use a different mechanism (e.g. shell tool with the appropriate ` + + `command) if you need to ${verbBare} this path.`; + return { + ok: false, + type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, + rawMessage: raw, + displayMessage: `special file; cannot ${verbBare} via this tool.`, + }; + } const status = cache.check(stats); if ( status.state === 'fresh' && diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 79d50d0c7d6..4e100327820 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -345,7 +345,15 @@ class WriteFileToolInvocation extends BaseToolInvocation< // ran before mkdirSync / encoding detection; we re-stat here so // an external mutation that lands in the gap between those // operations and the writeTextFile below is caught. - if (fileExists && !this.config.getFileReadCacheDisabled()) { + // + // Run unconditionally (not gated on `fileExists`): if the path + // was absent during the earlier checkPriorRead but a different + // process creates it before this writeTextFile, the gated form + // would skip enforcement and silently overwrite a pre-existing + // file the model never read. ENOENT inside checkPriorRead + // returns ok:true so the genuine new-file creation path is + // unchanged. + if (!this.config.getFileReadCacheDisabled()) { const writeDecision = await checkPriorRead( this.config.getFileReadCache(), file_path, From 9df900abb9c80e8f3a7e1f712c3291b00d3010b8 Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 4 May 2026 06:34:56 +0800 Subject: [PATCH 11/16] fix(core): close 4 enforcement gaps from 6th Copilot review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (Plus a doc-only update for the 5th — the mtime+size limitation warning in the Risk section now mentions the silent-overwrite escalation that this PR's mutation paths bring along.) 1. ENOENT after the model has already read the file is no longer silently treated as `ok: true`. Added an `expectExisting` option to `checkPriorRead`; post-read and pre-write callers pass `true`. ENOENT under that flag now rejects with `FILE_CHANGED_SINCE_READ` ("file disappeared after the model read it") rather than falling through to the new-file path with stale bytes. Pre-read callers keep the old default (ENOENT → ok:true → fall through to genuine new-file creation). EditTool's pre-write check derives the flag from `editData.isNewFile`; WriteFile's pre-write check derives it from the post-read `fileExists` value. 2. Directory targets now reject with `TARGET_IS_DIRECTORY` and a structured message instead of returning `ok: true`. The previous form fell through to readTextFile(), which on the WriteFile confirmation path threw a plain Error and was surfaced by the scheduler as `UNHANDLED_EXCEPTION`. Both Edit and WriteFile now emit a structured rejection at enforcement time. (WriteFile's build-time validateToolParamValues already rejects directories, so the change matters most for EditTool.) 3. Non-cacheable rejection's `rawMessage` no longer hard-codes "overwrite" — it now uses the same `verbBare` derivation as the `displayMessage`, so EditTool's path correctly says "if you need to edit it" and WriteFile's path stays "if you need to overwrite it". The previous form was confusing for in-place edits. 4. WriteFile.getConfirmationDetails now mirrors execute()'s ENOENT-to-new-file fallback: a file that disappears between isFilefileExists() and the readTextFile-for-diff call no longer throws a plain Error (which would surface as UNHANDLED_EXCEPTION) — it falls back to the brand-new-file diff so the user sees a clean confirmation rather than an unstructured crash. Tests: - New: `rejects an edit on a directory with TARGET_IS_DIRECTORY` - New: `confirmation falls back to a new-file diff when the file disappears mid-flight` (WriteFile) - Updated: non-cacheable rejection asserts `verbBare` is "edit" on the EditTool path and "overwrite" on the WriteFile path. Reported by Copilot via /review on PR #3774. --- packages/core/src/tools/edit.test.ts | 26 +++++ packages/core/src/tools/edit.ts | 9 ++ .../core/src/tools/priorReadEnforcement.ts | 95 +++++++++++++------ packages/core/src/tools/write-file.test.ts | 39 ++++++++ packages/core/src/tools/write-file.ts | 29 +++++- 5 files changed, 166 insertions(+), 32 deletions(-) diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 734a749709f..2e10454eadb 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -1014,6 +1014,32 @@ describe('EditTool', () => { /binary \/ image \/ audio \/ video \/ PDF \/ notebook payload/, ); expect(result.error?.message).not.toMatch(/Use the read_file tool first/); + // EditTool's verb is "edit", not "overwrite" — using the + // wrong one here would be confusing for in-place edits. + expect(result.error?.message).toMatch(/if you need to edit it\./); + expect(result.error?.message).not.toMatch( + /if you need to overwrite it\./, + ); + }); + + it('rejects an edit on a directory with TARGET_IS_DIRECTORY', async () => { + // Pre-fix, the directory exemption returned ok:true and + // readTextFile would either throw EISDIR (caught by execute as + // EDIT_PREPARATION_FAILURE) or — in WriteFile.getConfirmationDetails — + // collapse into UNHANDLED_EXCEPTION. The structured rejection + // here gives a stable error code regardless of where the call + // hits in the pipeline. + const dirPath = path.join(rootDir, 'enforce-dir'); + fs.mkdirSync(dirPath); + const result = await tool + .build({ + file_path: dirPath, + old_string: 'foo', + new_string: 'bar', + }) + .execute(abortSignal); + expect(result.error?.type).toBe(ToolErrorType.TARGET_IS_DIRECTORY); + expect(result.error?.message).toMatch(/is a directory/); }); it('does not let an unread file be probed via NO_OCCURRENCE_FOUND', async () => { diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 79409b9d5f2..4e60a87110d 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -232,6 +232,7 @@ class EditToolInvocation implements ToolInvocation { this.config.getFileReadCache(), params.file_path, 'editing', + { expectExisting: true }, ); if (!postDecision.ok) { // Forensic trail for post-read TOCTOU rejections. These are @@ -496,6 +497,14 @@ class EditToolInvocation implements ToolInvocation { this.config.getFileReadCache(), this.params.file_path, 'editing', + // For an in-place edit (`!isNewFile`), the file existed at + // read time and must still exist now — an ENOENT here + // means the original target disappeared and we should + // reject rather than fall through to a new-file write + // that would silently re-create a file from stale bytes. + // For genuine new-file creation, ENOENT is the expected + // pre-write state (ok:true → writeTextFile creates). + { expectExisting: !editData.isNewFile }, ); if (!writeDecision.ok) { debugLogger.warn('pre-write TOCTOU rejection', { diff --git a/packages/core/src/tools/priorReadEnforcement.ts b/packages/core/src/tools/priorReadEnforcement.ts index df9108bb2cd..d6acaf66177 100644 --- a/packages/core/src/tools/priorReadEnforcement.ts +++ b/packages/core/src/tools/priorReadEnforcement.ts @@ -71,6 +71,22 @@ export type PriorReadDecision = */ export type PriorReadVerb = 'editing' | 'overwriting'; +/** + * Options for {@link checkPriorRead}. + * + * - `expectExisting`: when true, an `ENOENT` from the stat call + * rejects with `FILE_CHANGED_SINCE_READ` instead of returning + * `ok: true`. Use this for the post-read and pre-write recheck + * calls — at those points the model has already committed to + * mutating an existing path, so a disappeared file is a stale-read + * drift, not a "the file genuinely never existed" disappearance + * race. The default (`expectExisting: false`) is the pre-read + * behaviour: ENOENT means "go ahead and create". + */ +export interface CheckPriorReadOptions { + expectExisting?: boolean; +} + /** * Test whether a mutating tool is cleared to proceed against * `filePath` based on the session FileReadCache. @@ -103,6 +119,7 @@ export async function checkPriorRead( cache: FileReadCache, filePath: string, verb: PriorReadVerb, + options: CheckPriorReadOptions = {}, ): Promise { let stats: fs.Stats; try { @@ -110,9 +127,28 @@ export async function checkPriorRead( } catch (err) { const code = (err as NodeJS.ErrnoException | undefined)?.code; if (code === 'ENOENT') { - // Disappearance race vs the caller's fileExists check. Let the - // downstream write path surface the absence — synthesising a - // "you must read first" message here would be misleading. + if (options.expectExisting) { + // Post-read or pre-write: the file existed at planning time + // but disappeared before this recheck. That is not a benign + // disappearance race — it is the original target going away + // from under the model. Reject so the caller does not + // silently fall through to the new-file path with stale + // bytes. + const raw = + `File ${filePath} disappeared after the model read it ` + + `(stat now returns ENOENT). Re-read with the ${ToolNames.READ_FILE} ` + + `tool — the path may have been deleted or moved — before ` + + `retrying ${verb} it.`; + return { + ok: false, + type: ToolErrorType.FILE_CHANGED_SINCE_READ, + rawMessage: raw, + displayMessage: `file disappeared after last read; re-run ${ToolNames.READ_FILE} first.`, + }; + } + // Pre-read disappearance race vs the caller's fileExists check. + // Let the downstream write path surface the absence — synthesising + // a "you must read first" message here would be misleading. return { ok: true }; } // Any other stat failure: fail closed. We cannot prove the file @@ -136,26 +172,26 @@ export async function checkPriorRead( displayMessage: `cannot verify prior read of ${filePath}; re-run ${ToolNames.READ_FILE} before ${verbDisplay}.`, }; } - // Narrow the bypass to **directories only**. The earlier - // !stats.isFile() form was too broad: it also covered FIFOs, - // sockets, and character / block devices, none of which the - // model has a legitimate way to "read first" anyway, and - // letting those flow through to readTextFile risks blocking - // I/O (FIFO) or over-allocation (/dev/urandom). - // - // Directory targets get the bypass because read_file already has - // a dedicated TARGET_IS_DIRECTORY error and an enforcement loop - // ("call read_file first" → "read_file says directory") would be - // a real regression vs the pre-PR behaviour. - // - // Non-regular non-directory paths (FIFO / socket / character / - // block device) get a dedicated rejection: read_file rejects - // them as "not a regular file" with no recovery, so telling the - // model to "call read_file first" would loop forever. The - // dedicated message tells the model the path cannot be edited - // via Edit / WriteFile at all and to use a different tool. + // Directory and other non-regular paths get dedicated rejections + // with structured ToolErrorType codes — never `ok: true`. Falling + // through to readTextFile would either block (FIFO), + // over-allocate (/dev/urandom), or throw a plain Error that the + // confirmation path collapses into UNHANDLED_EXCEPTION (e.g. + // EISDIR on WriteFile.getConfirmationDetails, which never reaches + // execute()'s explicit EISDIR mapping). if (stats.isDirectory()) { - return { ok: true }; + const verbBare = verb === 'editing' ? 'edit' : 'overwrite'; + const raw = + `${filePath} is a directory. The Edit / WriteFile tools only ` + + `operate on regular files. Use a different mechanism (e.g. ` + + `the shell tool) if you need to ${verbBare} the contents of ` + + `this directory.`; + return { + ok: false, + type: ToolErrorType.TARGET_IS_DIRECTORY, + rawMessage: raw, + displayMessage: `path is a directory; cannot ${verbBare} via this tool.`, + }; } if (!stats.isFile()) { const verbBare = verb === 'editing' ? 'edit' : 'overwrite'; @@ -205,18 +241,21 @@ export async function checkPriorRead( status.entry.lastReadWasFull && !status.entry.lastReadCacheable ) { + // Both raw and displayMessage use the bare verb (`edit` / + // `overwrite`) rather than the gerund — the noun phrase + // "cannot editing via this tool" would be ungrammatical, and + // both strings need to read correctly on the EditTool path + // (where "overwrite" would be the wrong verb for an in-place + // edit) and the WriteFileTool path (where "overwrite" is + // correct). + const verbBare = verb === 'editing' ? 'edit' : 'overwrite'; const raw = `File ${filePath} is a binary / image / audio / video / PDF / ` + `notebook payload that the ${ToolNames.READ_FILE} tool returns ` + `as a structured value rather than as plain text. The Edit / ` + `WriteFile tools cannot mutate that payload safely — re-reading ` + `it would not change this. Use a different mechanism (e.g. shell ` + - `tool with a binary-aware writer) if you need to overwrite it.`; - // displayMessage uses the bare verb (`edit` / `overwrite`) - // rather than the gerund — the noun phrase "cannot editing - // via this tool" would be ungrammatical, and `returnDisplay` - // surfaces this string directly to the user. - const verbBare = verb === 'editing' ? 'edit' : 'overwrite'; + `tool with a binary-aware writer) if you need to ${verbBare} it.`; return { ok: false, type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ, diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 1c300dae71e..fd2ce669933 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -837,10 +837,49 @@ describe('WriteFileTool', () => { .build({ file_path: filePath, content: 'clobber' }) .execute(abortSignal); expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); + // Verb in the dead-end guidance must read correctly for + // overwrite (the WriteFile path), not "edit". + expect(result.error?.message).toMatch(/if you need to overwrite it\./); fs.unlinkSync(filePath); }); + it('confirmation falls back to a new-file diff when the file disappears mid-flight', async () => { + // isFilefileExists() saw the file. Between that and the + // readTextFile inside getConfirmationDetails, an external + // process deletes it. Pre-fix, readTextFile threw ENOENT and + // the confirmation collapsed into UNHANDLED_EXCEPTION; the new + // catch falls back to fileExists=false so the user sees the + // brand-new-file diff instead. + const filePath = path.join(rootDir, 'enforce-disappear.txt'); + fs.writeFileSync(filePath, 'will disappear', 'utf-8'); + seedPriorRead(filePath); + const readSpy = vi + .spyOn(fsService, 'readTextFile') + .mockRejectedValueOnce( + Object.assign(new Error('ENOENT'), { code: 'ENOENT' }), + ); + + const invocation = tool.build({ + file_path: filePath, + content: 'new content', + }); + const confirmation = await invocation.getConfirmationDetails(abortSignal); + // Should produce a confirmation diff (not throw), with the + // new content as the proposed value. + expect(confirmation).toEqual( + expect.objectContaining({ + type: 'edit', + newContent: 'new content', + }), + ); + + readSpy.mockRestore(); + if (fs.existsSync(filePath)) { + fs.unlinkSync(filePath); + } + }); + it('rejects confirmation requests on an unread existing file before showing a diff', async () => { const filePath = path.join(rootDir, 'enforce-confirm.txt'); fs.writeFileSync(filePath, 'unread current bytes', 'utf-8'); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 4e100327820..0bacec49353 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -120,7 +120,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< _abortSignal: AbortSignal, ): Promise { let originalContent = ''; - const fileExists = await isFilefileExists(this.params.file_path); + let fileExists = await isFilefileExists(this.params.file_path); // Run prior-read enforcement *before* we read the file to render // a confirmation diff. Otherwise the user could approve a diff // computed from current bytes that the model has never received, @@ -156,9 +156,20 @@ class WriteFileToolInvocation extends BaseToolInvocation< .readTextFile({ path: this.params.file_path }); originalContent = content; } catch (err) { - throw new Error( - `Error reading existing file for confirmation: ${getErrorMessage(err)}`, - ); + // ENOENT here means the file disappeared between + // isFilefileExists() and readTextFile (a disappearance + // race). The pre-read checkPriorRead above already returned + // ok:true for ENOENT and let us fall through; mirror that + // in this read by falling back to the new-file diff (empty + // originalContent) instead of throwing a plain Error that + // the scheduler would surface as UNHANDLED_EXCEPTION. + if (isNodeError(err) && err.code === 'ENOENT') { + fileExists = false; + } else { + throw new Error( + `Error reading existing file for confirmation: ${getErrorMessage(err)}`, + ); + } } } @@ -172,6 +183,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< this.config.getFileReadCache(), this.params.file_path, 'overwriting', + { expectExisting: true }, ); if (!postDecision.ok) { debugLogger.warn('post-read TOCTOU rejection (confirmation)', { @@ -308,6 +320,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< this.config.getFileReadCache(), file_path, 'overwriting', + { expectExisting: true }, ); if (!postDecision.ok) { debugLogger.warn('post-read TOCTOU rejection (execute)', { @@ -358,6 +371,14 @@ class WriteFileToolInvocation extends BaseToolInvocation< this.config.getFileReadCache(), file_path, 'overwriting', + // If the file existed when we read it (`fileExists` is still + // true after readTextFile), ENOENT here means the original + // target disappeared between the post-read check and now — + // reject rather than fall through and silently re-create the + // file from stale bytes. For new-file creation + // (`fileExists === false`), ENOENT is the expected pre-write + // state (ok:true → writeTextFile creates). + { expectExisting: fileExists }, ); if (!writeDecision.ok) { debugLogger.warn('pre-write TOCTOU rejection', { From b053ae3ada5c796e68a0970141b364b2690918e7 Mon Sep 17 00:00:00 2001 From: wenshao Date: Tue, 5 May 2026 01:33:34 +0800 Subject: [PATCH 12/16] =?UTF-8?q?docs(core):=20clarify=20stat=E2=86=92writ?= =?UTF-8?q?e=20race=20+=20EDIT=5FREQUIRES=5FPRIOR=5FREAD=20scope?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three doc-only follow-ups from Copilot's latest review pass on PR #3774. None change behaviour; the pre-fix code state was already the actual contract — the docs just lagged it. 1. EDIT_REQUIRES_PRIOR_READ enum comment now lists the three cases the code actually returns it for (never-read, partial / ranged / non-cacheable read, structural dead end — non-text payload or special file). The previous one-liner described only the first case and would mislead future maintainers. 2. The Final pre-write freshness check blocks in EditTool.execute and WriteFileTool.execute now spell out that they DO NOT eliminate the stat → writeTextFile race. The window narrows from the previously-unbounded post-read-to-write gap down to two adjacent syscalls, but a concurrent writer landing in that pair can still be clobbered. Closing the residual would require an atomic write (write-to-temp + rename) or a content-hash post-write recheck — both deferred. Operators who need strict protection set `fileReadCacheDisabled: true` and rely on application-level locking. 3. PR description Risk section gains a "Known unmitigated: stat → write race window" subsection (English + Chinese mirror) matching the code comments. --- packages/core/src/tools/edit.ts | 13 ++++++++++++- packages/core/src/tools/tool-error.ts | 25 +++++++++++++++++++++---- packages/core/src/tools/write-file.ts | 11 +++++++++++ 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 4e60a87110d..e793cce34b5 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -484,7 +484,18 @@ class EditToolInvocation implements ToolInvocation { // an external mutation could land and be silently overwritten. // This last guard tightens the window from "post-read → // writeTextFile (unbounded)" to "stat → writeTextFile (two - // syscalls)". + // adjacent syscalls)". + // + // It does NOT eliminate the race. A concurrent writer that + // lands between this stat and the writeTextFile call below + // can still be clobbered — that residual is an OS-level + // limitation of the stat-then-write pattern, and the only + // way to close it is an atomic write (write to a temp file, + // then rename) or a content-hash post-check that re-reads + // the bytes after the write. Both are deferred to a follow-up + // PR; operators who care about strict overwrite-protection + // should set `fileReadCacheDisabled: true` and rely on + // application-level locking. // // Run unconditionally (not gated on `editData.isNewFile`): // `isNewFile` was decided back in calculateEdit, but a file diff --git a/packages/core/src/tools/tool-error.ts b/packages/core/src/tools/tool-error.ts index 6467e4ca948..b3d0c9f17ca 100644 --- a/packages/core/src/tools/tool-error.ts +++ b/packages/core/src/tools/tool-error.ts @@ -36,10 +36,27 @@ export enum ToolErrorType { EDIT_EXPECTED_OCCURRENCE_MISMATCH = 'edit_expected_occurrence_mismatch', EDIT_NO_CHANGE = 'edit_no_change', EDIT_NO_CHANGE_LLM_JUDGEMENT = 'edit_no_change_llm_judgement', - // Returned when Edit / WriteFile is asked to mutate a file the model - // has not (yet) read this session via ReadFile. The session-scoped - // FileReadCache is the source of truth — see read-file.ts and the - // related FileReadCache service. + // Returned when Edit / WriteFile is asked to mutate a file in a + // state the session-scoped FileReadCache cannot vouch for. Three + // cases share this code: + // + // 1. The file has not been read this session via ReadFile (the + // original "model never saw the bytes" case). + // 2. The file was read only as a partial / ranged / truncated + // view, so the model has not seen the full text content the + // mutation could touch. + // 3. The file is a structural dead end that no amount of + // re-reading can change: + // - non-text payloads (binary / image / audio / video / + // PDF / notebook) — read_file returns these as + // structured values that Edit / WriteFile cannot mutate + // safely; the rejection message tells the model to use + // a different tool (shell with a binary-aware writer). + // - special files (FIFO / socket / character or block + // device) — read_file rejects these as "not a regular + // file", so an enforcement loop on read_file would + // never terminate; the rejection points the model at + // shell instead. // // Despite the `EDIT_` prefix this code is shared between EditTool // and WriteFileTool: the boundary it guards is "the model is about diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 0bacec49353..2139554ea0e 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -359,6 +359,17 @@ class WriteFileToolInvocation extends BaseToolInvocation< // an external mutation that lands in the gap between those // operations and the writeTextFile below is caught. // + // It does NOT eliminate the race. A concurrent writer that + // lands between this stat and the writeTextFile call below + // can still be clobbered — that residual is an OS-level + // limitation of the stat-then-write pattern, and the only way + // to close it is an atomic write (write-to-temp + rename) or + // a content-hash post-check that re-reads the bytes after the + // write. Both are deferred to a follow-up; operators who care + // about strict overwrite-protection should set + // `fileReadCacheDisabled: true` and rely on application-level + // locking. + // // Run unconditionally (not gated on `fileExists`): if the path // was absent during the earlier checkPriorRead but a different // process creates it before this writeTextFile, the gated form From 9726c87e0766131e9a3a0d950d568fb10f22bba9 Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 6 May 2026 01:02:53 +0800 Subject: [PATCH 13/16] chore(core): minor follow-ups from review #4229917446 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the five MINOR items raised in the independent code review on 2026-05-05 — the cheap, isolated ones. The other two (race- simulating integration test, moving StructuredToolError out of priorReadEnforcement.ts) are deferred as the reviewer suggested. 1. EditTool now has a symmetric `PRIOR_READ_VERIFICATION_FAILED` regression test (mocks fs.promises.stat to reject with EACCES, asserts the EditTool path produces the same fail-closed result that the existing WriteFile EACCES test pins). Five-line fix to close the asymmetry that, while harmless today (the helper is shared), would let a future Edit-side change to checkPriorRead slip through without test coverage. 2. ensureParentDirectoriesExist / mkdirSync now run AFTER the pre-write checkPriorRead in both EditTool.execute() and WriteFileTool.execute(). Doing it before would leak intermediate directories on the rejection path — a real (if minor) FS litter the previous order created on every rejected new-file write. 3. EDIT_REQUIRES_PRIOR_READ enum docstring gains a one-line note for operators routing alerts on this code: a single `edit_requires_prior_read` signal can mean any of the three cases (no read / partial read / structural dead-end), and if per-cause monitoring becomes important the enum can be split in a follow-up. The originating tool name and the message text already disambiguate at runtime. --- packages/core/src/tools/edit.test.ts | 30 +++++++++++++++++++++++++++ packages/core/src/tools/edit.ts | 9 ++++++-- packages/core/src/tools/tool-error.ts | 7 +++++++ packages/core/src/tools/write-file.ts | 16 ++++++++++---- 4 files changed, 56 insertions(+), 6 deletions(-) diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index a9e3a37d877..ffea84a6237 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -1096,6 +1096,36 @@ describe('EditTool', () => { expect(result.error?.message).toMatch(/is a directory/); }); + it('rejects an edit with a stat failure other than ENOENT (fail-closed)', async () => { + // Symmetric with WriteFile's EACCES test. checkPriorRead is + // shared today, but if a future change adds an Edit-side + // fallback that downgrades a real verify failure to + // EDIT_REQUIRES_PRIOR_READ, only the write path would catch + // it without this test. + fs.writeFileSync(filePath, 'untouched', 'utf8'); + const statSpy = vi + .spyOn(fs.promises, 'stat') + .mockRejectedValueOnce( + Object.assign(new Error('EACCES'), { code: 'EACCES' }), + ); + + const result = await tool + .build({ + file_path: filePath, + old_string: 'untouched', + new_string: 'modified', + }) + .execute(abortSignal); + + expect(result.error?.type).toBe( + ToolErrorType.PRIOR_READ_VERIFICATION_FAILED, + ); + expect(result.error?.message).toMatch(/Could not stat .*\(EACCES\)/); + expect(fs.readFileSync(filePath, 'utf8')).toBe('untouched'); + + statSpy.mockRestore(); + }); + it('does not let an unread file be probed via NO_OCCURRENCE_FOUND', async () => { // Regression for the read-less content oracle: pre-fix, a model // could call Edit with candidate old_strings on an unread file diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 8dbea194647..a19f1974159 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -475,8 +475,6 @@ class EditToolInvocation implements ToolInvocation { } try { - this.ensureParentDirectoriesExist(this.params.file_path); - // Final pre-write freshness check. calculateEdit() ran a // post-read check, but execute() can be called arbitrarily // long after that (user approval, modify-and-confirm, etc.). @@ -533,6 +531,13 @@ class EditToolInvocation implements ToolInvocation { } } + // Create parent directories AFTER the pre-write enforcement + // check passes. Doing it before would leak intermediate + // directories on the failure path — a real (if minor) FS + // litter that the previous order created on every rejected + // edit. + this.ensureParentDirectoriesExist(this.params.file_path); + // For new files, apply default file encoding setting // For existing files, preserve the original encoding (BOM and charset) if (editData.isNewFile) { diff --git a/packages/core/src/tools/tool-error.ts b/packages/core/src/tools/tool-error.ts index b3d0c9f17ca..f031f8a754f 100644 --- a/packages/core/src/tools/tool-error.ts +++ b/packages/core/src/tools/tool-error.ts @@ -66,6 +66,13 @@ export enum ToolErrorType { // distinguish edit-vs-write should look at the originating tool // name in the surrounding ToolCallEvent rather than the error // code itself. + // + // Note for operators routing alerts: a single `edit_requires_prior_read` + // signal can mean any of the three cases above. If per-cause monitoring + // becomes important, splitting this into separate codes (e.g. + // `EDIT_NO_PRIOR_READ`, `EDIT_PARTIAL_PRIOR_READ`, + // `EDIT_TARGET_NOT_TEXT_EDITABLE`) is a follow-up; for now the + // originating tool name and the message text already disambiguate. EDIT_REQUIRES_PRIOR_READ = 'edit_requires_prior_read', // Returned when Edit / WriteFile is asked to mutate a file the model // *has* read this session, but the on-disk bytes have changed since diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 5b77e4d3fb8..6736d5270a6 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -339,7 +339,6 @@ class WriteFileToolInvocation extends BaseToolInvocation< } if (!fileExists) { - fs.mkdirSync(dirName, { recursive: true }); const userEncoding = this.config.getDefaultFileEncoding(); if (userEncoding === FileEncoding.UTF8_BOM) { // User explicitly configured UTF-8 BOM for all new files @@ -355,9 +354,9 @@ class WriteFileToolInvocation extends BaseToolInvocation< } // Final pre-write freshness check. The earlier post-read check - // ran before mkdirSync / encoding detection; we re-stat here so - // an external mutation that lands in the gap between those - // operations and the writeTextFile below is caught. + // ran before encoding detection; we re-stat here so an external + // mutation that lands in the gap between those operations and + // the writeTextFile below is caught. // // It does NOT eliminate the race. A concurrent writer that // lands between this stat and the writeTextFile call below @@ -407,6 +406,15 @@ class WriteFileToolInvocation extends BaseToolInvocation< } } + // Create parent directories AFTER the pre-write enforcement + // check passes. Doing it before would leak intermediate + // directories on the failure path (rejected new-file writes + // would otherwise litter the filesystem with empty mkdir'd + // ancestors). + if (!fileExists) { + fs.mkdirSync(dirName, { recursive: true }); + } + try { await this.config.getFileSystemService().writeTextFile({ path: file_path, From de8ddf5303a39c15ec7fca46e8903a486d1d95a4 Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 6 May 2026 11:16:06 +0800 Subject: [PATCH 14/16] fix(core): close 2 correctness gaps from maintainer review #4232751470 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both tracked back to the cache's "track most recent read shape" model diverging from prior-read enforcement's "model has seen these bytes" model. 1. SVG (and similar string-content fallbacks) recorded as non-cacheable, blocking subsequent Edit / WriteFile. `read-file.ts` derives `cacheable` from `originalLineCount !== undefined && !isTruncated`. The SVG branch in `fileUtils.ts` returned content without `originalLineCount`, so `cacheable` collapsed to false and a follow-up Edit hit the dead-end "non-text payload — use shell" rejection — telling the model to use shell to mutate a file it had just successfully read as text. This was a real regression vs pre-PR behaviour where SVG-as-text editing worked. Fix: SVG-as-text branch now sets `originalLineCount` (split on '\n') and `isTruncated: false`, so ReadFile records it as a full cacheable read. The binary-fallback string and over-1MB SVG branches are deliberately left non-cacheable — they return placeholder strings ("Cannot display content of ...") rather than file content, so blocking edits there is correct. New regression test in `read-file.test.ts`: `records SVG-as-text reads with cacheable=true so a follow-up Edit passes enforcement`. 2. recordRead unconditionally overwriting lastReadWasFull / lastReadCacheable, revoking prior write-author or full-read rights. The `WriteFile(create) → ReadFile(offset/limit) → Edit` sequence rejected the Edit because the partial read clobbered the `lastReadWasFull = true` that `recordWrite` had stamped at create time. Same shape applies to a full text read followed by a partial one of the same inode. Fix: `recordRead` is now sticky-on-true for the read flags — `if (opts.full) entry.lastReadWasFull = true;` and the matching guard for `cacheable`. Prior `true` survives a later partial / non-cacheable read. The fast-path `file_unchanged` check still gates on the incoming request's own `isFullRead` in `read-file.ts`, so a partial read still does not get a placeholder it shouldn't. Updated the existing "overwrites earlier lastReadWasFull" test to assert the new sticky semantics, and added a `lastReadCacheable` symmetric test plus a `Write → partial-Read → Edit` end-to-end test in `edit.test.ts`. Reported by tanzhenxin via independent maintainer review on 2026-05-06. --- .../core/src/services/fileReadCache.test.ts | 27 +++++++++++- packages/core/src/services/fileReadCache.ts | 22 +++++++++- packages/core/src/tools/edit.test.ts | 41 +++++++++++++++++++ packages/core/src/tools/read-file.test.ts | 30 ++++++++++++++ packages/core/src/utils/fileUtils.ts | 11 +++++ 5 files changed, 127 insertions(+), 4 deletions(-) diff --git a/packages/core/src/services/fileReadCache.test.ts b/packages/core/src/services/fileReadCache.test.ts index 6b3af18fdd2..02d6153a514 100644 --- a/packages/core/src/services/fileReadCache.test.ts +++ b/packages/core/src/services/fileReadCache.test.ts @@ -136,7 +136,13 @@ describe('FileReadCache', () => { } }); - it('overwrites earlier lastReadWasFull on subsequent reads', () => { + it('preserves earlier lastReadWasFull on a subsequent partial read (sticky-on-true)', () => { + // Prior-read enforcement asks "has the model seen these + // bytes (or fully authored them)?" — once a full read has + // happened, a follow-up offset/limit read should not revoke + // the model's right to mutate the file. Pre-fix this was + // the cause of the `WriteFile(create) → ReadFile(offset/limit) + // → Edit` regression flagged by the maintainer review. const cache = new FileReadCache(); const stats = makeStats(); cache.recordRead('/x/foo.ts', stats, { full: true, cacheable: true }); @@ -144,7 +150,24 @@ describe('FileReadCache', () => { const result = cache.check(stats); expect(result.state).toBe('fresh'); if (result.state === 'fresh') { - expect(result.entry.lastReadWasFull).toBe(false); + expect(result.entry.lastReadWasFull).toBe(true); + } + }); + + it('preserves earlier lastReadCacheable on a subsequent non-cacheable read', () => { + // Symmetric to lastReadWasFull. A model that once read a + // file as text and later read it as a structured payload + // (image, PDF, etc. — though uncommon in practice for a + // single inode) keeps the right to edit the bytes it saw + // as text. + const cache = new FileReadCache(); + const stats = makeStats(); + cache.recordRead('/x/foo.ts', stats, { full: true, cacheable: true }); + cache.recordRead('/x/foo.ts', stats, { full: true, cacheable: false }); + const result = cache.check(stats); + expect(result.state).toBe('fresh'); + if (result.state === 'fresh') { + expect(result.entry.lastReadCacheable).toBe(true); } }); diff --git a/packages/core/src/services/fileReadCache.ts b/packages/core/src/services/fileReadCache.ts index 3a39696dc06..8c4bdf2bf95 100644 --- a/packages/core/src/services/fileReadCache.ts +++ b/packages/core/src/services/fileReadCache.ts @@ -99,6 +99,20 @@ export class FileReadCache { * - `cacheable` — the produced content is suitable for substitution * with a `file_unchanged` placeholder. Set true for plain text, * false for binary / image / audio / video / PDF / notebook. + * + * The `lastReadWasFull` and `lastReadCacheable` flags are + * **sticky-on-true**: a partial / non-cacheable read records the + * timestamp but does not flip a previously-`true` flag back to + * `false`. The reason is that prior-read enforcement on Edit / + * WriteFile asks "has the model seen these bytes (or fully + * authored them)?", not "what was the *most recent* read shape?" + * — pre-fix, a `WriteFile(create) → ReadFile(offset/limit) → Edit` + * sequence would reject the Edit because the partial read clobbered + * the `lastReadWasFull = true` that `recordWrite` had stamped, and + * the same shape applies to a full text read followed by a partial + * one. The fast-path file_unchanged check still gates on the + * incoming request's own `isFullRead` (in `read-file.ts`), so a + * partial read does not get a placeholder it shouldn't. */ recordRead( absPath: string, @@ -107,8 +121,12 @@ export class FileReadCache { ): FileReadEntry { const entry = this.upsert(absPath, stats); entry.lastReadAt = Date.now(); - entry.lastReadWasFull = opts.full; - entry.lastReadCacheable = opts.cacheable; + if (opts.full) { + entry.lastReadWasFull = true; + } + if (opts.cacheable) { + entry.lastReadCacheable = true; + } return entry; } diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index ffea84a6237..e1cb47335e9 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -1239,6 +1239,47 @@ describe('EditTool', () => { expect(fs.readFileSync(newPath, 'utf8')).toBe('second content\n'); }); + it('allows Edit after Write→partial-Read (sticky-on-true preserves write-author rights)', async () => { + // Reproduction for the maintainer-review regression: pre-fix, + // a partial read recorded `lastReadWasFull = false` and + // clobbered the `true` that recordWrite had stamped at + // create time, so this Edit would then be rejected with + // EDIT_REQUIRES_PRIOR_READ even though the model had + // authored the file's full content. + const newPath = path.join(rootDir, 'write-then-partial-read.txt'); + (mockConfig.getApprovalMode as Mock).mockReturnValue( + ApprovalMode.AUTO_EDIT, + ); + + const created = await tool + .build({ + file_path: newPath, + old_string: '', + new_string: 'line one\nline two\nline three\n', + }) + .execute(abortSignal); + expect(created.error).toBeUndefined(); + + // Simulate a partial follow-up Read (offset/limit). Pre-fix + // this overwrote lastReadWasFull/lastReadCacheable to false. + fileReadCache.recordRead(newPath, fs.statSync(newPath), { + full: false, + cacheable: true, + }); + + const edited = await tool + .build({ + file_path: newPath, + old_string: 'line two', + new_string: 'second line', + }) + .execute(abortSignal); + expect(edited.error).toBeUndefined(); + expect(fs.readFileSync(newPath, 'utf8')).toBe( + 'line one\nsecond line\nline three\n', + ); + }); + it('allows a chain of edits without re-reading between them', async () => { // After the first Edit, recordWrite stamps `lastWriteAt`. The // second Edit's stat will still match the cache entry (because diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index ca5be9885ad..2b2916f508f 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -795,6 +795,36 @@ describe('ReadFileTool', () => { } }); + it('records SVG-as-text reads with cacheable=true so a follow-up Edit passes enforcement', async () => { + // Pre-fix the SVG branch in fileUtils.ts returned content + // without `originalLineCount`, which collapsed + // ReadFileToolInvocation's `cacheable` derivation to + // false. EditTool's prior-read enforcement then mistook + // the just-read SVG for a "non-text payload" and rejected + // a subsequent in-place edit. + const svgPath = path.join(tempRootDir, 'icon.svg'); + await fsp.writeFile( + svgPath, + '\n', + 'utf-8', + ); + + const result = await read({ file_path: svgPath }); + expect(typeof result.llmContent).toBe('string'); + expect(result.returnDisplay).toMatch(/^Read SVG as text:/); + + // The cache must record this as a full, cacheable read so + // that prior-read enforcement on Edit / WriteFile would + // recognise it as the model having seen the bytes. + const status = fileReadCache.check(fs.statSync(svgPath)); + expect(status.state).toBe('fresh'); + if (status.state === 'fresh') { + expect(status.entry.lastReadAt).toBeDefined(); + expect(status.entry.lastReadWasFull).toBe(true); + expect(status.entry.lastReadCacheable).toBe(true); + } + }); + it('does not return the placeholder for image files', async () => { const imagePath = path.join(tempRootDir, 'pic.png'); const pngHeader = Buffer.from([ diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index 79467fb0e5e..3606dbea802 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -698,9 +698,20 @@ export async function processSingleFileContent( }; } const content = await readFileWithEncoding(filePath); + // Populate `originalLineCount` and `isTruncated` so the + // ReadFile cache treats this exactly like a successful text + // read: ReadFileToolInvocation derives `cacheable` from + // those two fields, and an SVG-as-text read needs to be + // cacheable to keep working as an editable text file. Pre-fix, + // the absent `originalLineCount` collapsed cacheable to false + // and a follow-up Edit on the just-read SVG would be rejected + // as a "non-text payload" — a regression flagged by the + // independent maintainer review. return { llmContent: content, returnDisplay: `Read SVG as text: ${relativePathForDisplay}`, + originalLineCount: content.split('\n').length, + isTruncated: false, }; } case 'text': { From 72a1d67c48801b7a783cd7942e0701606b125349 Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 6 May 2026 15:07:09 +0800 Subject: [PATCH 15/16] fix(core): close 3 correctness gaps from re-review #4233904930 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three are tightenings of the prior `de8ddf530` round. 1. **Sticky-on-true narrowed to "no fingerprint drift"**. `fileReadCache.recordRead` previously kept `lastReadWasFull` / `lastReadCacheable` true across drifted recordings, which re-opened a `Read full @X → external write @Y → Read partial @Y → Edit` hole: the partial recordRead silently advanced the entry's mtime+size to Y while preserving the sticky `full=true` from X, so a follow-up Edit ran against bytes the model only saw the first 10 lines of. Now the sticky branch only fires when `(mtimeMs, sizeBytes)` matches the existing entry; on drift, both flags reset to exactly what this read produced. New regression test in `fileReadCache.test.ts` reproduces the reviewer's reported sequence. 2. **Subagent FileReadCache isolation now covers the inherits-model + same-approval-mode common case**. The own-property machinery from #3717 only triggers when an `Object.create(parent)` actually fires; both `agent.ts:990-993` (same-approval-mode) and `subagent-manager.ts:699-701` (inherits-model) had paths that returned the parent Config directly, so the subagent's `getFileReadCache()` resolved to the parent's instance — a parent Read could satisfy the subagent's Edit on a path the subagent's transcript never contained. Both sites now build a thin `Object.create(base)` override unconditionally; no method changes for the inherits / same-mode cases, but a distinct instance triggers the lazy-init in `Config.getFileReadCache()` so the subagent gets an isolated cache. 3. **Cache records the read pipeline's internal stat instead of a post-read re-stat**. `processSingleFileContent` now surfaces its internal stat via `result.stats`, and read-file uses that for `recordRead` instead of running its own stat after the read returns. Pre-fix, an external write between the pipeline call and the post-read stat let the cache record fingerprint Y for content the model received at X — a subsequent Edit would pass enforcement against bytes the model never legitimately saw. The internal-stat-to-read window is still a few microseconds wide; that residue is the same content-hash territory acknowledged in the Risk section. Reported by tanzhenxin via re-review on PR #3774. --- .../core/src/services/fileReadCache.test.ts | 26 ++++++++++ packages/core/src/services/fileReadCache.ts | 50 +++++++++++++------ .../core/src/subagents/subagent-manager.ts | 15 +++++- packages/core/src/tools/agent/agent.ts | 16 ++++-- packages/core/src/tools/read-file.ts | 34 ++++++------- packages/core/src/utils/fileUtils.ts | 12 +++++ 6 files changed, 115 insertions(+), 38 deletions(-) diff --git a/packages/core/src/services/fileReadCache.test.ts b/packages/core/src/services/fileReadCache.test.ts index 02d6153a514..491e4d8923a 100644 --- a/packages/core/src/services/fileReadCache.test.ts +++ b/packages/core/src/services/fileReadCache.test.ts @@ -154,6 +154,32 @@ describe('FileReadCache', () => { } }); + it('does NOT preserve lastReadWasFull when a drifted (mutated) fingerprint arrives', () => { + // Maintainer-review regression: T0 full read at fingerprint + // X → T1 external write advances on-disk to Y → T2 partial + // read records at Y. Sticky-on-true used to silently keep + // lastReadWasFull=true from T0 even though it described + // different bytes; a follow-up Edit then ran against bytes + // the model only saw the first 10 lines of. + const cache = new FileReadCache(); + cache.recordRead('/x/foo.ts', makeStats({ mtimeMs: 1000, size: 100 }), { + full: true, + cacheable: true, + }); + // Drift: external write advances mtime+size. + cache.recordRead('/x/foo.ts', makeStats({ mtimeMs: 2000, size: 200 }), { + full: false, + cacheable: true, + }); + const result = cache.check(makeStats({ mtimeMs: 2000, size: 200 })); + expect(result.state).toBe('fresh'); + if (result.state === 'fresh') { + // Reset to this read's actual shape, not sticky-on-true + // from the prior full read of different bytes. + expect(result.entry.lastReadWasFull).toBe(false); + } + }); + it('preserves earlier lastReadCacheable on a subsequent non-cacheable read', () => { // Symmetric to lastReadWasFull. A model that once read a // file as text and later read it as a structured payload diff --git a/packages/core/src/services/fileReadCache.ts b/packages/core/src/services/fileReadCache.ts index 8c4bdf2bf95..172b1fd4592 100644 --- a/packages/core/src/services/fileReadCache.ts +++ b/packages/core/src/services/fileReadCache.ts @@ -101,16 +101,21 @@ export class FileReadCache { * false for binary / image / audio / video / PDF / notebook. * * The `lastReadWasFull` and `lastReadCacheable` flags are - * **sticky-on-true**: a partial / non-cacheable read records the - * timestamp but does not flip a previously-`true` flag back to - * `false`. The reason is that prior-read enforcement on Edit / - * WriteFile asks "has the model seen these bytes (or fully - * authored them)?", not "what was the *most recent* read shape?" - * — pre-fix, a `WriteFile(create) → ReadFile(offset/limit) → Edit` - * sequence would reject the Edit because the partial read clobbered - * the `lastReadWasFull = true` that `recordWrite` had stamped, and - * the same shape applies to a full text read followed by a partial - * one. The fast-path file_unchanged check still gates on the + * **sticky-on-true** when the recorded fingerprint matches the + * existing entry's `(mtimeMs, sizeBytes)`. That preserves the + * model's read-rights across `Read full → Read partial` and + * `WriteFile(create) → Read partial → Edit` sequences against + * the same bytes. + * + * When the fingerprint drifts — i.e. the file was mutated between + * the prior record and this one — the flags are **reset** to + * exactly what this read produced. Sticky-on-true across drift + * would let a `Read full @X → external write → Read partial @Y → + * Edit` sequence pass enforcement against bytes the model only + * saw the first 10 lines of, exactly the regression flagged in + * the maintainer review. + * + * The fast-path `file_unchanged` check still gates on the * incoming request's own `isFullRead` (in `read-file.ts`), so a * partial read does not get a placeholder it shouldn't. */ @@ -119,13 +124,28 @@ export class FileReadCache { stats: Stats, opts: { full: boolean; cacheable: boolean }, ): FileReadEntry { + const key = FileReadCache.inodeKey(stats); + const existing = this.byInode.get(key); + const sameFingerprint = + existing !== undefined && + existing.mtimeMs === stats.mtimeMs && + existing.sizeBytes === stats.size; const entry = this.upsert(absPath, stats); entry.lastReadAt = Date.now(); - if (opts.full) { - entry.lastReadWasFull = true; - } - if (opts.cacheable) { - entry.lastReadCacheable = true; + if (sameFingerprint) { + // Same bytes the entry already described — sticky-on-true + // preserves prior `true` flags from full reads or writes. + if (opts.full) { + entry.lastReadWasFull = true; + } + if (opts.cacheable) { + entry.lastReadCacheable = true; + } + } else { + // Drift detected (or fresh entry): the prior flags described + // different bytes. Reset to what this read actually produced. + entry.lastReadWasFull = opts.full; + entry.lastReadCacheable = opts.cacheable; } return entry; } diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index 1d603985e91..cf2db5ab008 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -689,7 +689,12 @@ export class SubagentManager { * When a subagent's model selector specifies a model (bare ID or * authType-prefixed), build a Config override with a dedicated * ContentGenerator so the model actually reaches the API. - * Returns the original context unchanged for inherit selectors. + * For inherit selectors we still build a thin Object.create + * override so the subagent gets an isolated FileReadCache via + * the per-Config own-property machinery — returning `base` + * directly would let the subagent share the parent's read entries + * and silently weaken prior-read enforcement on its mutation + * paths. */ private async maybeOverrideContentGenerator( config: SubagentConfig, @@ -697,7 +702,13 @@ export class SubagentManager { ): Promise { const selection = parseSubagentModelSelection(config.model); if (selection.inherits) { - return base; + // Thin prototype-delegation override: no method changes, but a + // distinct instance triggers the lazy-init in + // `Config.getFileReadCache()` so the subagent gets its own + // cache rather than inheriting the parent's. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const isolated = Object.create(base) as any; + return isolated as Config; } const authType = diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index 9d8a31345d4..15ec71ca24c 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -987,10 +987,18 @@ class AgentToolInvocation extends BaseToolInvocation { this.config.isTrustedFolder(), ); const resolvedApprovalMode = permissionModeToApprovalMode(resolvedMode); - const agentConfig = - resolvedApprovalMode !== this.config.getApprovalMode() - ? createApprovalModeOverride(this.config, resolvedApprovalMode) - : this.config; + // ALWAYS produce a child Config via Object.create, even when the + // approval mode is identical to the parent. Subagents must run + // against an isolated FileReadCache so a parent's prior_read + // entries cannot satisfy enforcement on a path the subagent's + // transcript never contained — see the per-Config own-property + // machinery in `Config.getFileReadCache()`. Reusing + // `this.config` directly here would short-circuit that + // isolation for the same-mode path, which is the common case. + const agentConfig = createApprovalModeOverride( + this.config, + resolvedApprovalMode, + ); // Create the subagent. Fork bypasses SubagentManager because its // runtime configs are synthesized from the parent's cache-safe params. diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index c2418982ff8..b11b579ae15 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -224,27 +224,27 @@ class ReadFileToolInvocation extends BaseToolInvocation< // everything", so we force the next call back through the full // pipeline. // - // The stat we record is the one taken **after** the read pipeline, - // not `stats` from L158. processSingleFileContent does its own stat - // internally; if the file mutated between L158 and that internal - // stat, the bytes that landed in `result.llmContent` correspond to - // the post-read fingerprint, not the pre-read one. Recording - // `stats` would store a fingerprint that does not match the bytes - // we just emitted, so a later `check()` could report `fresh` and - // serve a placeholder pointing at content the model never saw. - if (cacheEnabled && stats) { + // The stat we record is the one taken inside `processSingleFileContent` + // and surfaced via `result.stats`. The internal stat happens + // immediately before the actual content read, so the fingerprint + // it captures is the one closest to the bytes the model received. + // Falling back to a post-read re-stat would describe a possibly- + // mutated file rather than the file the read returned: a write + // landing between the read and the post-stat would let the cache + // record fingerprint Y for content the model only saw at X, and + // a follow-up Edit would pass enforcement (`fresh + full + + // cacheable @ Y`) against bytes the model never legitimately saw. + // + // Race residue: the internal-stat-to-actual-read window is still + // a few microseconds wide. Closing it completely needs a content + // hash on the read pipeline (deferred follow-up — see Risk + // section in the PR description). + if (cacheEnabled && (result.stats ?? stats)) { const cacheable = typeof result.llmContent === 'string' && result.originalLineCount !== undefined && !result.isTruncated; - let recordStats: Stats = stats; - try { - recordStats = await fs.stat(absPath); - } catch { - // Stat after read failed — fall back to the pre-read stat. - // The fingerprint may not exactly match the bytes emitted, - // but it is the best we have without a second strategy. - } + const recordStats: Stats = result.stats ?? stats!; cache.recordRead(absPath, recordStats, { full: isFullRead, cacheable, diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index 3606dbea802..14b1a8b9dbc 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -530,6 +530,14 @@ export interface ProcessedFileReadResult { originalLineCount?: number; // For text files, the total number of lines in the original file isTruncated?: boolean; // For text files, indicates if content was truncated linesShown?: [number, number]; // For text files [startLine, endLine] (1-based for display) + /** + * The Stats taken at the start of the read pipeline, before the + * actual content read. Surfaced so the FileReadCache can record + * a fingerprint that matches the bytes the model actually + * received — a post-read re-stat would describe a possibly- + * mutated file rather than the file the read returned. + */ + stats?: import('node:fs').Stats; } /** @@ -687,6 +695,7 @@ export async function processSingleFileContent( return { llmContent: `Cannot display content of binary file: ${relativePathForDisplay}`, returnDisplay: `Skipped binary file: ${relativePathForDisplay}`, + stats, }; } case 'svg': { @@ -695,6 +704,7 @@ export async function processSingleFileContent( return { llmContent: `Cannot display content of SVG file larger than 1MB: ${relativePathForDisplay}`, returnDisplay: `Skipped large SVG file (>1MB): ${relativePathForDisplay}`, + stats, }; } const content = await readFileWithEncoding(filePath); @@ -712,6 +722,7 @@ export async function processSingleFileContent( returnDisplay: `Read SVG as text: ${relativePathForDisplay}`, originalLineCount: content.split('\n').length, isTruncated: false, + stats, }; } case 'text': { @@ -789,6 +800,7 @@ export async function processSingleFileContent( isTruncated, originalLineCount, linesShown: [startLine + 1, actualEndLine], + stats, }; } case 'image': From 70f7223e0d9c3199d54481b1f7ef9295139b5b70 Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 6 May 2026 15:42:40 +0800 Subject: [PATCH 16/16] docs(core): clarify partial subagent isolation per review #4234090906 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tanzhenxin's third review correctly observed that the `Object.create(parent)` wrappers in `agent.ts:createApprovalModeOverride` and `subagent-manager.ts:maybeOverrideContentGenerator` only isolate the FileReadCache for code that consults `Config.getFileReadCache()` directly. Bound `EditTool` / `WriteFileTool` instances were registered against the parent's tool registry at initialise time, so tool invocations still resolve `this.config` to the parent and reach the parent's cache. `InProcessBackend.createPerAgentConfig` already does the right thing (`override.createToolRegistry()` + `copyDiscoveredToolsFrom(base.getToolRegistry())`); bringing that to these two spawn sites is the real fix. Reviewer's verdict was COMMENT, not REQUEST_CHANGES — the gap pre-dates this PR (it's a property of #3717's per-Config own-property machinery) and pre-PR there was no enforcement on subagent mutations at all, so the PR is strictly an improvement on every spawn path. Documented the partial guarantee explicitly: - Inline comments on both spawn sites note the bound-tool caveat and point at `InProcessBackend.createPerAgentConfig` as the model for the follow-up. - PR description's subagent paragraph (English + Chinese mirror) now splits into "fully isolated" (`InProcessBackend.createPerAgentConfig`) and "partial isolation" (the two sites in this PR) so readers don't walk away with the wrong contract. Filing the registry-rebuild work as a follow-up; not in this PR. --- packages/core/src/subagents/subagent-manager.ts | 8 ++++++++ packages/core/src/tools/agent/agent.ts | 15 +++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index cf2db5ab008..011f191e059 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -706,6 +706,14 @@ export class SubagentManager { // distinct instance triggers the lazy-init in // `Config.getFileReadCache()` so the subagent gets its own // cache rather than inheriting the parent's. + // + // Same caveat as in `agent.ts:createApprovalModeOverride`: the + // tool registry was bound on the parent at initialise time, so + // tool invocations still resolve `this.config` to the parent + // and reach the parent's cache. `InProcessBackend.createPerAgentConfig` + // already rebuilds the registry via `override.createToolRegistry()` + // + `copyDiscoveredToolsFrom(base.getToolRegistry())`; doing + // that here is the follow-up that closes the bound-tool path. // eslint-disable-next-line @typescript-eslint/no-explicit-any const isolated = Object.create(base) as any; return isolated as Config; diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index 15ec71ca24c..ab6dbe49db4 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -995,6 +995,21 @@ class AgentToolInvocation extends BaseToolInvocation { // machinery in `Config.getFileReadCache()`. Reusing // `this.config` directly here would short-circuit that // isolation for the same-mode path, which is the common case. + // + // Known partial fix: `Config.createToolRegistry` (called once at + // initialise time on the parent) bound `EditTool` / `WriteFileTool` + // instances against the parent. The subagent's + // `runtimeContext.getToolRegistry()` walks the prototype chain + // back to that parent registry, so tool invocations resolve + // `this.config` to the parent and reach the parent's + // FileReadCache rather than the wrapper's lazy-init one. + // `InProcessBackend.createPerAgentConfig` already does the right + // thing (`override.createToolRegistry()` + `copyDiscoveredToolsFrom`); + // bringing that here is a follow-up. Pre-PR there was no + // enforcement on subagent mutations at all, so the wrapper here + // is still strictly an improvement (the cache lazy-init does + // shield code that *consumes the Config directly* rather than + // through a parent-bound tool). const agentConfig = createApprovalModeOverride( this.config, resolvedApprovalMode,