wenshao · May 16, 2026 · May 16, 2026 · May 16, 2026 · wenshao · May 16, 2026
@@ -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', () => {

@@ -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);
     }

@@ -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

@@ -476,6 +476,35 @@ class EditToolInvocation implements ToolInvocation<EditToolParams, ToolResult> {
     }
 
     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<EditToolParams, ToolResult> {
       // 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) {

@@ -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.';

@@ -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,