From 2efd66e14c5fffce0161711c5ba03359fde755cb Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Sun, 17 May 2026 00:31:04 +0800 Subject: [PATCH 1/2] fix(rewind): restore upstream TOCTOU ordering + heal sticky failed marker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related bugs that landed in #4064: 1. **trackEdit widened the pre-write TOCTOU window**. PR #4064 inserted `await trackEdit(filePath)` between `checkPriorRead` and `writeTextFile` in both `edit.ts` and `write-file.ts`. trackEdit does `stat` + `copyFile` and on large files can take hundreds of milliseconds. The pre-existing comment above `checkPriorRead` acknowledged a stat-then-write race window of "two adjacent syscalls"; the new ordering widened that to "freshness check → potentially-multi-second backup → write", so an external mutation landing during the backup would no longer be detected before the write clobbered it. The fix is the ordering upstream `claude-code/src/tools/FileEditTool` uses, with its own explicit comment on the equivalent block: > These awaits must stay OUTSIDE the critical section below — a > yield between the staleness check and writeTextContent lets > concurrent edits interleave. Moved `trackEdit` to BEFORE `checkPriorRead` in both tools. Backups are idempotent (deterministic `{hash}@v{version}` filename), so running the backup on a path that ends up rejected just leaves an unused-but-correct backup of the pre-edit state, not corrupt state. 2. **The `failed` marker added in d59838338 stayed sticky in `trackEdit`**. When `makeSnapshot` recorded `failed: true` for a file (transient I/O error during per-file backup), the next `trackEdit` for that file skipped because the early guard only checked existence, not the failed bit. The pre-edit state never got re-captured even after the underlying I/O recovered, leaving the snapshot marked filesFailed forever for that path. The guard now skips only on confirmed (non-failed) entries: `if (existing && !existing.failed) return;`. The re-check guard at the end is updated symmetrically so the fresh backup actually replaces the failed entry (instead of being silently dropped by the "already present" branch). Tests: - `edit.test.ts`: pin the TOCTOU ordering via spy on `fileReadCache.check` and `mockFileHistoryService.trackEdit`. Verifies `trackEdit` is called before the last `cache.check` (the pre-write freshness guard). Same pattern in `write-file.test.ts`. Both tests fail on the old ordering (verified by stashing the production change). - `fileHistoryService.test.ts`: `heals a failed entry on the next trackEdit attempt` — sabotage storage so makeSnapshot records failed, restore storage, run trackEdit, assert the entry is now non-failed with a real backupFileName. --- .../src/services/fileHistoryService.test.ts | 40 +++++++++++++ .../core/src/services/fileHistoryService.ts | 16 ++++- packages/core/src/tools/edit.test.ts | 54 +++++++++++++++++ packages/core/src/tools/edit.ts | 37 +++++++++--- packages/core/src/tools/write-file.test.ts | 60 +++++++++++++++++++ packages/core/src/tools/write-file.ts | 32 ++++++++-- 6 files changed, 223 insertions(+), 16 deletions(-) diff --git a/packages/core/src/services/fileHistoryService.test.ts b/packages/core/src/services/fileHistoryService.test.ts index c7f2f7c0907..c140ffcfa66 100644 --- a/packages/core/src/services/fileHistoryService.test.ts +++ b/packages/core/src/services/fileHistoryService.test.ts @@ -119,6 +119,46 @@ describe('FileHistoryService', () => { await expect(service.trackEdit(file)).resolves.toBeUndefined(); expect(service.getSnapshots()[0].trackedFileBackups).toEqual({}); }); + + // The sticky-failed guard symmetry test for trackEdit. After + // makeSnapshot recorded a `failed: true` marker for a file (e.g. + // transient disk full), the next trackEdit invocation — typically + // triggered by a tool about to modify the same file — must NOT + // skip just because the entry exists. It must attempt a fresh + // backup; on success the failed marker is replaced. Without this + // the failed flag stays sticky until the file content changes, + // permanently poisoning rewind for that file. + it('heals a failed entry on the next trackEdit attempt', async () => { + const file = join(projectDir, 'a.txt'); + await writeFile(file, 'p1-content'); + + await service.makeSnapshot('p1'); + await service.trackEdit(file); + + // Force makeSnapshot's per-file backup to throw. The file content + // is unchanged so checkOriginFileChanged short-circuits to "no + // change" — but we want the failure path here, so modify the file + // first to ensure createBackup is reached. + await writeFile(file, 'p2-content'); + await rm(storageDir, { recursive: true, force: true }); + await writeFile(storageDir, ''); + await service.makeSnapshot('p2'); + expect( + service.getSnapshots()[1].trackedFileBackups['a.txt']!.failed, + ).toBe(true); + + // Restore the backup target and have a tool about to edit the file + // call trackEdit. The guard must let createBackup run again; on + // success the failed marker is replaced with a real entry. + await rm(storageDir, { recursive: true, force: true }); + await mkdir(storageDir, { recursive: true }); + await service.trackEdit(file); + + const p2Backup = service.getSnapshots()[1].trackedFileBackups['a.txt']; + expect(p2Backup).toBeDefined(); + expect(p2Backup.failed).toBeFalsy(); + expect(p2Backup.backupFileName).not.toBeNull(); + }); }); describe('makeSnapshot', () => { diff --git a/packages/core/src/services/fileHistoryService.ts b/packages/core/src/services/fileHistoryService.ts index 047a1efa580..ecd57d27ac5 100644 --- a/packages/core/src/services/fileHistoryService.ts +++ b/packages/core/src/services/fileHistoryService.ts @@ -336,7 +336,15 @@ export class FileHistoryService { return; } - if (mostRecent.trackedFileBackups[trackingPath]) { + const existing = mostRecent.trackedFileBackups[trackingPath]; + // Skip only when we already have a confirmed (non-failed) backup. If + // the existing entry is marked `failed` (because makeSnapshot's + // per-file backup attempt threw earlier), let trackEdit retry: this + // is the next chance to capture the file's pre-edit state under + // hopefully-recovered I/O conditions. Without this allowance the + // failed marker would stay sticky until the file content changes + // again, permanently poisoning rewind for that file. + if (existing && !existing.failed) { return; } @@ -352,7 +360,11 @@ export class FileHistoryService { // Re-check after async backup — concurrent calls write the same // deterministic path, so the second overwrites the first harmlessly. - if (!mostRecent.trackedFileBackups[trackingPath]) { + // Allow overwriting a `failed` entry so the heal path actually + // records the fresh backup (otherwise we'd leave the failed marker + // in place even though we successfully captured the file). + const current = mostRecent.trackedFileBackups[trackingPath]; + if (!current || current.failed) { mostRecent.trackedFileBackups[trackingPath] = backup; this.state.trackedFiles.add(trackingPath); } diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 4975a5a98da..fef5b983957 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -534,6 +534,60 @@ describe('EditTool', () => { ); }); + // Pin the upstream-aligned ordering: trackEdit MUST run before the + // pre-write checkPriorRead. The upstream `claude-code/src/tools/ + // FileEditTool` comment on the equivalent block says: + // + // "These awaits must stay OUTSIDE the critical section below — a + // yield between the staleness check and writeTextContent lets + // concurrent edits interleave." + // + // Without this ordering the multi-hundred-ms `trackEdit` sat + // between checkPriorRead and writeTextFile, widening the + // already-acknowledged stat-then-write race window from microseconds + // to seconds. Test verifies trackEdit was invoked BEFORE the + // pre-write `cache.check(...)` call. + it('backs up before the pre-write freshness check (TOCTOU ordering)', async () => { + const initialContent = 'This is some old text.'; + fs.writeFileSync(filePath, initialContent, 'utf8'); + seedPriorRead(filePath); + + const callOrder: string[] = []; + mockFileHistoryService.trackEdit.mockImplementation(async () => { + callOrder.push('trackEdit'); + }); + // Pass-through spy: records each call but defers to the real impl. + const originalCheck = fileReadCache.check.bind(fileReadCache); + const cacheCheckSpy = vi + .spyOn(fileReadCache, 'check') + .mockImplementation((stats) => { + callOrder.push('cache.check'); + return originalCheck(stats); + }); + + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + + try { + await tool.build(params).execute(new AbortController().signal); + + // trackEdit must have run, and must have run BEFORE the pre-write + // cache.check (the last cache.check call covers the pre-write + // freshness guard — earlier ones come from calculateEdit's + // pre-read and post-read phases). + const trackEditIdx = callOrder.indexOf('trackEdit'); + const lastCheckIdx = callOrder.lastIndexOf('cache.check'); + expect(trackEditIdx).toBeGreaterThanOrEqual(0); + expect(lastCheckIdx).toBeGreaterThanOrEqual(0); + expect(trackEditIdx).toBeLessThan(lastCheckIdx); + } finally { + cacheCheckSpy.mockRestore(); + } + }); + // The Edit tool feeds the commit-attribution singleton on success so // commit notes can later report per-file AI/human ratios. Service- // level tests for `recordEdit` already exist; these guard against diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 553dc4cdba8..0b64c5fd94c 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -476,6 +476,35 @@ class EditToolInvocation implements ToolInvocation { } try { + // Backup the pre-edit content BEFORE the final freshness check. + // Mirrors the upstream `claude-code/src/tools/FileEditTool` ordering, + // which has an explicit comment on the equivalent block: + // + // "These awaits must stay OUTSIDE the critical section below — a + // yield between the staleness check and writeTextContent lets + // concurrent edits interleave." + // + // `trackEdit` does `stat` + `copyFile` and on large files can take + // hundreds of milliseconds. The previous ordering ran it AFTER + // `checkPriorRead` and before `writeTextFile`, which widened the + // already-acknowledged stat-then-write window from "two adjacent + // syscalls" to "freshness check → potentially-multi-second backup → + // write". An external mutation landing inside the backup window was + // therefore no longer detected before the write clobbered it. + // + // Backing up first is safe: backups are idempotent (deterministic + // `{hash}@v{version}` filename) and per-snapshot. If the freshness + // check below then rejects the edit, we keep an unused-but-correct + // backup of the pre-edit state — not corrupt state. The next + // makeSnapshot will reuse it if the file is unchanged. + try { + await this.config + .getFileHistoryService() + .trackEdit(this.params.file_path); + } catch { + // File history is best-effort; never block core tool operations. + } + // 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.). @@ -539,14 +568,6 @@ class EditToolInvocation implements ToolInvocation { // edit. this.ensureParentDirectoriesExist(this.params.file_path); - try { - await this.config - .getFileHistoryService() - .trackEdit(this.params.file_path); - } catch { - // File history is best-effort; never block core tool operations. - } - // 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/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 5c374c15eb2..d301ec3bdf6 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -403,6 +403,66 @@ describe('WriteFileTool', () => { expect(writtenContent).toBe(proposedContent); }); + // Pin the upstream-aligned ordering: trackEdit MUST run before the + // pre-write checkPriorRead. The upstream `claude-code/src/tools/ + // FileEditTool` comment on the equivalent block says: + // + // "These awaits must stay OUTSIDE the critical section below — a + // yield between the staleness check and writeTextContent lets + // concurrent edits interleave." + // + // Without this ordering the multi-hundred-ms `trackEdit` sat + // between checkPriorRead and writeTextFile, widening the + // already-acknowledged stat-then-write race window. Test verifies + // trackEdit was invoked BEFORE the pre-write `cache.check(...)`. + it('backs up before the pre-write freshness check (TOCTOU ordering)', async () => { + const filePath = path.join(rootDir, 'toctou_ordering.txt'); + const initialContent = 'pre-existing content'; + fs.writeFileSync(filePath, initialContent, 'utf8'); + const stats = fs.statSync(filePath); + fileReadCache.recordRead(filePath, stats, { + full: true, + cacheable: true, + }); + + const callOrder: string[] = []; + mockFileHistoryService.trackEdit.mockImplementation(async () => { + callOrder.push('trackEdit'); + }); + // Pass-through spy: records each call but defers to the real impl. + const originalCheck = fileReadCache.check.bind(fileReadCache); + const cacheCheckSpy = vi + .spyOn(fileReadCache, 'check') + .mockImplementation((stats) => { + callOrder.push('cache.check'); + return originalCheck(stats); + }); + + const params = { file_path: filePath, content: 'new content' }; + const invocation = tool.build(params); + + try { + const confirmDetails = + await invocation.getConfirmationDetails(abortSignal); + if ( + typeof confirmDetails === 'object' && + 'onConfirm' in confirmDetails && + confirmDetails.onConfirm + ) { + await confirmDetails.onConfirm(ToolConfirmationOutcome.ProceedOnce); + } + await invocation.execute(abortSignal); + + const trackEditIdx = callOrder.indexOf('trackEdit'); + const lastCheckIdx = callOrder.lastIndexOf('cache.check'); + expect(trackEditIdx).toBeGreaterThanOrEqual(0); + expect(lastCheckIdx).toBeGreaterThanOrEqual(0); + expect(trackEditIdx).toBeLessThan(lastCheckIdx); + } finally { + cacheCheckSpy.mockRestore(); + } + }); + it('should overwrite an existing file and return diff', async () => { const filePath = path.join(rootDir, 'execute_existing_file.txt'); const initialContent = 'Initial content for execute.'; diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index ec9cfef75d0..a4cc4404e3f 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -368,6 +368,32 @@ class WriteFileToolInvocation extends BaseToolInvocation< detectedEncoding = undefined; } + // Backup the pre-edit content BEFORE the final freshness check. + // Mirrors the upstream `claude-code/src/tools/FileEditTool` ordering, + // which has an explicit comment on the equivalent block: + // + // "These awaits must stay OUTSIDE the critical section below — a + // yield between the staleness check and writeTextContent lets + // concurrent edits interleave." + // + // `trackEdit` does `stat` + `copyFile` and on large files can take + // hundreds of milliseconds. The previous ordering ran it AFTER + // `checkPriorRead` and before `writeTextFile`, which widened the + // already-acknowledged stat-then-write window from "two adjacent + // syscalls" to "freshness check → potentially-multi-second backup → + // write". An external mutation landing inside the backup window was + // therefore no longer detected before the write clobbered it. + // + // Backing up first is safe: backups are idempotent (deterministic + // `{hash}@v{version}` filename) and per-snapshot. If the freshness + // check below then rejects the write, we keep an unused-but-correct + // backup of the pre-overwrite state — not corrupt state. + try { + await this.config.getFileHistoryService().trackEdit(file_path); + } catch { + // File history is best-effort; never block core tool operations. + } + // Final pre-write freshness check. The earlier post-read check // ran before encoding detection; we re-stat here so an external // mutation that lands in the gap between those operations and @@ -430,12 +456,6 @@ class WriteFileToolInvocation extends BaseToolInvocation< fs.mkdirSync(dirName, { recursive: true }); } - try { - await this.config.getFileHistoryService().trackEdit(file_path); - } catch { - // File history is best-effort; never block core tool operations. - } - try { await this.config.getFileSystemService().writeTextFile({ path: file_path, From 28ee0464aaa883ffb4ecfe3b3902762a55a08fc8 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Sun, 17 May 2026 00:56:51 +0800 Subject: [PATCH 2/2] test(rewind): switch TOCTOU tests to behavioral assertions + harden heal test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback from code-reviewer agent: - The TOCTOU ordering tests previously asserted ordering via `callOrder.lastIndexOf('cache.check')`, which would silently degrade to a tautology if a future refactor added any `cache.check` call AFTER the pre-write check (e.g. the deferred atomic-write + content- hash post-check explicitly mentioned in the comment above the freshness guard). Switch to a behavioral assertion that directly tests the invariant: install a `trackEdit` mock that mutates the file on disk (bumps mtime) before returning. The pre-write `checkPriorRead` must catch the in-trackEdit mutation — that only happens if `trackEdit` runs BEFORE the pre-write check. The broken ordering would run pre-write check first (passing on pre-mutation stats), then trackEdit (which mutates), then write (which clobbers the external mutation). Asserting on `result.error?.type === FILE_CHANGED_SINCE_READ` exercises the actual race protection the fix exists to preserve, and survives any future refactor that keeps the invariant intact regardless of how many `cache.check` calls happen on the way. Verified by reverting just the production change to HEAD~1: both behavioral tests fail under the broken ordering (file is silently overwritten, no FILE_CHANGED_SINCE_READ error), and pass under the fix. - The heal test (`heals a failed entry on the next trackEdit attempt`) previously asserted `backupFileName !== null` but not that the file on disk at that name actually contained the current content. Add a `readFile + expect(...).toBe('p2-content')` assertion so a regression that wrongly reuses `previous.backupFileName` (pointing to `p1-content`) fails loudly instead of silently passing. --- .../src/services/fileHistoryService.test.ts | 12 ++++ packages/core/src/tools/edit.test.ts | 58 ++++++++-------- packages/core/src/tools/write-file.test.ts | 68 ++++++++++--------- 3 files changed, 80 insertions(+), 58 deletions(-) diff --git a/packages/core/src/services/fileHistoryService.test.ts b/packages/core/src/services/fileHistoryService.test.ts index c140ffcfa66..ff154dbad2d 100644 --- a/packages/core/src/services/fileHistoryService.test.ts +++ b/packages/core/src/services/fileHistoryService.test.ts @@ -158,6 +158,18 @@ describe('FileHistoryService', () => { expect(p2Backup).toBeDefined(); expect(p2Backup.failed).toBeFalsy(); expect(p2Backup.backupFileName).not.toBeNull(); + + // Verify the on-disk backup at the new name actually contains the + // current file content. Catches a regression where the heal path + // accidentally reuses `previous.backupFileName` (pointing at the + // older `p1-content`) instead of writing a fresh backup. + const backupPath = join( + storageDir, + 'file-history', + 'test-session', + p2Backup.backupFileName!, + ); + expect(await readFile(backupPath, 'utf-8')).toBe('p2-content'); }); }); diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index fef5b983957..c6a09218ed0 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -545,25 +545,33 @@ describe('EditTool', () => { // Without this ordering the multi-hundred-ms `trackEdit` sat // between checkPriorRead and writeTextFile, widening the // already-acknowledged stat-then-write race window from microseconds - // to seconds. Test verifies trackEdit was invoked BEFORE the - // pre-write `cache.check(...)` call. + // to seconds. + // + // Test strategy: install a `trackEdit` mock that mutates the file + // on disk (bumps mtime) before returning. The mutation has to be + // detected by the pre-write `checkPriorRead`. That only happens if + // `trackEdit` runs BEFORE the pre-write check — the broken + // ordering would run the pre-write check first (passing on the + // pre-mutation stats), then trackEdit (which mutates), then write + // (which clobbers the external mutation silently). + // + // Asserting on `result.error` directly tests the behavioral + // invariant rather than the call-ordering proxy, so it survives + // future refactors that preserve the invariant even if they shift + // the number of `cache.check` calls. it('backs up before the pre-write freshness check (TOCTOU ordering)', async () => { const initialContent = 'This is some old text.'; fs.writeFileSync(filePath, initialContent, 'utf8'); seedPriorRead(filePath); - const callOrder: string[] = []; mockFileHistoryService.trackEdit.mockImplementation(async () => { - callOrder.push('trackEdit'); + // Simulate an external write that lands while trackEdit is + // copying the file to the backup directory. Bumping mtime by + // 5 s makes the change reliably "newer" under the cache's + // ~1 s comparison granularity on macOS. + const newTime = new Date(Date.now() + 5000); + fs.utimesSync(filePath, newTime, newTime); }); - // Pass-through spy: records each call but defers to the real impl. - const originalCheck = fileReadCache.check.bind(fileReadCache); - const cacheCheckSpy = vi - .spyOn(fileReadCache, 'check') - .mockImplementation((stats) => { - callOrder.push('cache.check'); - return originalCheck(stats); - }); const params: EditToolParams = { file_path: filePath, @@ -571,21 +579,17 @@ describe('EditTool', () => { new_string: 'new', }; - try { - await tool.build(params).execute(new AbortController().signal); - - // trackEdit must have run, and must have run BEFORE the pre-write - // cache.check (the last cache.check call covers the pre-write - // freshness guard — earlier ones come from calculateEdit's - // pre-read and post-read phases). - const trackEditIdx = callOrder.indexOf('trackEdit'); - const lastCheckIdx = callOrder.lastIndexOf('cache.check'); - expect(trackEditIdx).toBeGreaterThanOrEqual(0); - expect(lastCheckIdx).toBeGreaterThanOrEqual(0); - expect(trackEditIdx).toBeLessThan(lastCheckIdx); - } finally { - cacheCheckSpy.mockRestore(); - } + const result = await tool + .build(params) + .execute(new AbortController().signal); + + // trackEdit must have actually fired. + expect(mockFileHistoryService.trackEdit).toHaveBeenCalledWith(filePath); + // The pre-write check must have caught the in-trackEdit mutation + // and rejected, proving trackEdit ran BEFORE the pre-write check. + expect(result.error?.type).toBe(ToolErrorType.FILE_CHANGED_SINCE_READ); + // The file on disk is unchanged (rejected, not overwritten). + expect(fs.readFileSync(filePath, 'utf8')).toBe(initialContent); }); // The Edit tool feeds the commit-attribution singleton on success so diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index d301ec3bdf6..d9e4b8fd8cc 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -413,8 +413,20 @@ describe('WriteFileTool', () => { // // Without this ordering the multi-hundred-ms `trackEdit` sat // between checkPriorRead and writeTextFile, widening the - // already-acknowledged stat-then-write race window. Test verifies - // trackEdit was invoked BEFORE the pre-write `cache.check(...)`. + // already-acknowledged stat-then-write race window. + // + // Test strategy: install a `trackEdit` mock that mutates the file + // on disk before returning. That mutation must be detected by the + // pre-write `checkPriorRead`. That only happens if `trackEdit` + // runs BEFORE the pre-write check — the broken ordering would run + // the pre-write check first (passing on pre-mutation stats), then + // trackEdit (which mutates), then write (which clobbers the + // external mutation silently). + // + // Asserting on `result.error` directly tests the behavioral + // invariant rather than the call-ordering proxy, so it survives + // future refactors that preserve the invariant even if they shift + // the number of `cache.check` calls. it('backs up before the pre-write freshness check (TOCTOU ordering)', async () => { const filePath = path.join(rootDir, 'toctou_ordering.txt'); const initialContent = 'pre-existing content'; @@ -425,42 +437,36 @@ describe('WriteFileTool', () => { cacheable: true, }); - const callOrder: string[] = []; mockFileHistoryService.trackEdit.mockImplementation(async () => { - callOrder.push('trackEdit'); + // Simulate an external write that lands while trackEdit is + // copying the file. Bumping mtime by 5 s makes the change + // reliably "newer" under the cache's ~1 s comparison + // granularity on macOS. + const newTime = new Date(Date.now() + 5000); + fs.utimesSync(filePath, newTime, newTime); }); - // Pass-through spy: records each call but defers to the real impl. - const originalCheck = fileReadCache.check.bind(fileReadCache); - const cacheCheckSpy = vi - .spyOn(fileReadCache, 'check') - .mockImplementation((stats) => { - callOrder.push('cache.check'); - return originalCheck(stats); - }); const params = { file_path: filePath, content: 'new content' }; const invocation = tool.build(params); - try { - const confirmDetails = - await invocation.getConfirmationDetails(abortSignal); - if ( - typeof confirmDetails === 'object' && - 'onConfirm' in confirmDetails && - confirmDetails.onConfirm - ) { - await confirmDetails.onConfirm(ToolConfirmationOutcome.ProceedOnce); - } - await invocation.execute(abortSignal); - - const trackEditIdx = callOrder.indexOf('trackEdit'); - const lastCheckIdx = callOrder.lastIndexOf('cache.check'); - expect(trackEditIdx).toBeGreaterThanOrEqual(0); - expect(lastCheckIdx).toBeGreaterThanOrEqual(0); - expect(trackEditIdx).toBeLessThan(lastCheckIdx); - } finally { - cacheCheckSpy.mockRestore(); + const confirmDetails = + await invocation.getConfirmationDetails(abortSignal); + if ( + typeof confirmDetails === 'object' && + 'onConfirm' in confirmDetails && + confirmDetails.onConfirm + ) { + await confirmDetails.onConfirm(ToolConfirmationOutcome.ProceedOnce); } + const result = await invocation.execute(abortSignal); + + // trackEdit must have actually fired. + expect(mockFileHistoryService.trackEdit).toHaveBeenCalledWith(filePath); + // The pre-write check must have caught the in-trackEdit mutation + // and rejected, proving trackEdit ran BEFORE the pre-write check. + expect(result.error?.type).toBe(ToolErrorType.FILE_CHANGED_SINCE_READ); + // The file on disk is unchanged (rejected, not overwritten). + expect(fs.readFileSync(filePath, 'utf8')).toBe(initialContent); }); it('should overwrite an existing file and return diff', async () => {