From 0e6bf30abb0e93aab3c84db75c3e3032ce2d3086 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 20 Jul 2026 19:41:30 -0700 Subject: [PATCH] fix(editor): don't consume move-echo provenance in the safety-net verify (round 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-3 proactive post-commit verify ran resolveLiveMoveVerification, which consumed pendingSelfMoveEcho. On FSEvents/SSH the real destination watcher event reliably lands AFTER the fast local read, so it then found no provenance, took the immediate changed-on-disk mark (no baseline check when there is no recent self-write), and raised a false conflict banner on the just-moved dirty tab — the exact data-loss this change removes. The proactive verify is a safety net: it now releases the autosave gate but leaves the provenance, so a later destination event is still recognized as the move's own echo and content-verified. Only a real watcher event consumes the provenance. Keeping it is safe — every watcher consumer verifies by content, which is strictly safer than the immediate mark. Co-authored-by: Orca --- .../useEditorExternalWatch-self-move.test.ts | 3 +- .../src/hooks/useEditorExternalWatch.ts | 40 +++++++++++---- .../lib/execute-open-editor-path-move.test.ts | 50 ++++++++++++++++++- 3 files changed, 82 insertions(+), 11 deletions(-) diff --git a/src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts b/src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts index c7a4edca097..e1c3c68744a 100644 --- a/src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts +++ b/src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts @@ -285,7 +285,8 @@ describe('verifyLatchedMoveDestinations (proactive, cross-worktree path)', () => expect(readFile).toHaveBeenCalledWith(expect.objectContaining({ filePath: FLOATING_FILE_PATH })) // Disk == baseline -> echo -> suppress + release the gate, no false banner. expect(setExternalMutation).not.toHaveBeenCalledWith('file-x', 'changed') - expect(clearSelfMoveEcho).toHaveBeenCalledWith('file-x') expect(setPendingLiveDiskVerification).toHaveBeenCalledWith('file-x', false) + // Safety net does NOT consume provenance (a later watcher event still needs it). + expect(clearSelfMoveEcho).not.toHaveBeenCalled() }) }) diff --git a/src/renderer/src/hooks/useEditorExternalWatch.ts b/src/renderer/src/hooks/useEditorExternalWatch.ts index 7d4082a7f0b..53df48fc0c3 100644 --- a/src/renderer/src/hooks/useEditorExternalWatch.ts +++ b/src/renderer/src/hooks/useEditorExternalWatch.ts @@ -669,7 +669,9 @@ export function createExternalWatchEventHandler( normalizedAbsolutePath ) if (isSelfMoveEcho) { - scheduleSelfMoveEchoVerification(target, dirtyIds) + // A real destination fs event confirms the echo — consume the + // provenance so a later genuine write takes the normal path. + scheduleSelfMoveEchoVerification(target, dirtyIds, true) } else { // An external write on a dirty tab must not vanish silently (issue // #7265); mark it so the editor shows the reload banner. @@ -810,7 +812,8 @@ type LiveMoveVerifyCandidate = { function resolveLiveMoveVerification( candidate: LiveMoveVerifyCandidate, diskSignature: string | null, - connectionId: string | undefined + connectionId: string | undefined, + consumeProvenance: boolean ): void { const { fileId, baseline, generation, operationId } = candidate if (liveMoveVerifyGeneration.get(fileId) !== generation) { @@ -832,9 +835,14 @@ function resolveLiveMoveVerification( ) { return } - // Consume the provenance: this echo is settled, so a later write at this path - // takes the normal changed-on-disk path. - state.clearSelfMoveEcho(fileId) + // Consume the provenance only when a real watcher event drove this check: the + // proactive post-commit verify is a safety net and must LEAVE the provenance, + // or the destination fs event (which on FSEvents/SSH lands after the faster + // local read) would fall through to the immediate changed-on-disk mark and + // raise a false conflict banner. Keeping it is safe — every consumer verifies. + if (consumeProvenance) { + state.clearSelfMoveEcho(fileId) + } const isMoveEcho = baseline !== undefined && diskSignature === baseline if (!isMoveEcho) { markFileChangedOnDisk(state, file, { connectionId, origin: 'live' }) @@ -860,9 +868,12 @@ export function verifyLatchedMoveDestinations( } // Owner/worktree are resolved per-tab inside the read; only connectionId is // shared. worktreePath is unused here (each tab is read at its own filePath). + // consumeProvenance:false — this is the safety net; leave the provenance so a + // destination watcher event arriving after this read is still seen as an echo. scheduleSelfMoveEchoVerification( { worktreeId: '', worktreePath, connectionId, runtimeEnvironmentId: null }, - gated + gated, + false ) } @@ -870,7 +881,11 @@ export function verifyLatchedMoveDestinations( // external write does not. Read the destination once and settle each dirty tab // by content identity. Autosave is suspended synchronously first so a write // landing mid-read can't be overwritten before we decide. -function scheduleSelfMoveEchoVerification(target: WatchedTarget, fileIds: string[]): void { +function scheduleSelfMoveEchoVerification( + target: WatchedTarget, + fileIds: string[], + consumeProvenance: boolean +): void { if (fileIds.length === 0) { return } @@ -902,9 +917,16 @@ function scheduleSelfMoveEchoVerification(target: WatchedTarget, fileIds: string }) .then((result) => { const diskSignature = result.isBinary ? null : getDiskBaselineSignature(result.content) - resolveLiveMoveVerification(candidate, diskSignature, target.connectionId) + resolveLiveMoveVerification( + candidate, + diskSignature, + target.connectionId, + consumeProvenance + ) }) - .catch(() => resolveLiveMoveVerification(candidate, null, target.connectionId)) + .catch(() => + resolveLiveMoveVerification(candidate, null, target.connectionId, consumeProvenance) + ) } } diff --git a/src/renderer/src/lib/execute-open-editor-path-move.test.ts b/src/renderer/src/lib/execute-open-editor-path-move.test.ts index 91770f80aeb..63e729dc65e 100644 --- a/src/renderer/src/lib/execute-open-editor-path-move.test.ts +++ b/src/renderer/src/lib/execute-open-editor-path-move.test.ts @@ -16,6 +16,7 @@ import { useAppStore } from '@/store' import { executeOpenEditorPathMove } from './execute-open-editor-path-move' import { __activeEditorPathMoveCountForTests } from '@/components/editor/editor-path-move-inflight' import { getDiskBaselineSignature } from '@/components/editor/diff-content-signature' +import { createExternalWatchEventHandler } from '@/hooks/useEditorExternalWatch' const CONTEXT = { settings: null, @@ -100,9 +101,56 @@ describe('executeOpenEditorPathMove', () => { const moved = useAppStore.getState().openFiles[0]! // Disk matched the carried baseline: gate released, no false conflict banner. expect(moved.pendingLiveDiskVerification).toBeFalsy() - expect(moved.pendingSelfMoveEcho).toBeUndefined() expect(moved.externalMutation).toBeUndefined() expect(moved.isDirty).toBe(true) + // Safety-net verify LEAVES the provenance so a destination watcher event + // arriving after this read is still recognized as the move's own echo. + expect(moved.pendingSelfMoveEcho?.targetPath).toBe('/repo/sub/a.md') + }) + + it('does not raise a false banner when the destination watcher event lands after the move', async () => { + // Regression: the proactive verify must NOT consume the echo provenance, or + // the real destination fs event (which on FSEvents/SSH lands after the fast + // local read) would fall through to the immediate changed-on-disk mark. + const DISK_CONTENT = 'the file as it exists on disk\n' + const id = openDirtyTab() + useAppStore.getState().setLastKnownDiskSignature(id, getDiskBaselineSignature(DISK_CONTENT)) + vi.stubGlobal('window', { + api: { + fs: { readFile: vi.fn().mockResolvedValue({ isBinary: false, content: DISK_CONTENT }) } + } + }) + + await executeOpenEditorPathMove({ + context: CONTEXT, + fromPath: '/repo/a.md', + toPath: '/repo/sub/a.md', + worktreeId: 'wt-1', + worktreePath: '/repo' + }) + await new Promise((resolve) => setTimeout(resolve, 20)) + + // The delayed destination create/update event finally arrives. + const { handleFsChanged, dispose } = createExternalWatchEventHandler((worktreePath) => + worktreePath === '/repo' + ? { + worktreeId: 'wt-1', + worktreePath: '/repo', + connectionId: undefined, + runtimeEnvironmentId: null + } + : undefined + ) + handleFsChanged({ + worktreePath: '/repo', + events: [{ kind: 'update', absolutePath: '/repo/sub/a.md' }] + }) + await new Promise((resolve) => setTimeout(resolve, 120)) + + const moved = useAppStore.getState().openFiles[0]! + expect(moved.externalMutation).toBeUndefined() + expect(moved.isDirty).toBe(true) + dispose() }) it('undoes the on-disk rename when the editor rekey collides', async () => {