From 7a912a3a24de5eb91a29bd028a075dfb2ca6197a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 21 Oct 2026 19:20:04 -0700 Subject: [PATCH] Autosave relies on the guarded save instead of pause flags; identical saves don't rewrite; non-UTF-8 files aren't autosaved (#31768) * Autosave relies on the guarded write instead of pausing restored or moved tabs Delete the pendingDiskBaselineVerification and pendingLiveDiskVerification pause flags. Every editor save is already a guarded write; a refusal raises the changed-on-disk banner, which is what stops autosave until an explicit save. A guarded write now succeeds when disk changed but already holds exactly the text being saved (local/host compare is byte-exact; client-side fallbacks compare decoded text and refuse when it holds U+FFFD), so a restored draft that matches disk goes clean instead of showing a banner. The restore scan only raises the banner early for tabs that arrive dirty; a watch resync no longer re-checks dirty tabs. Co-Authored-By: Claude * fix(editor): skip guarded saves when disk is already identical Return a compatible optional no-op result from hosts and client fallbacks, clean the tab, and clear its provisional self-write stamp without rewriting identical bytes. Co-Authored-By: Claude * fix(editor): refuse lossy automatic saves, keep identical-skip on older hosts, flag dirty tabs on resync - Autosave and pre-restart saves refuse when disk text can't be decoded losslessly, so a baseline that decoded to U+FFFD can't authorize overwriting different bytes. - New files.write.identical-noop.v1 capability; for hosts with only the v1 guarded write the client compares first, skips identical saves, then delegates to the host's guarded write. - Watch resync compares dirty tabs against disk and shows the changed-on-disk banner. - Local guarded writes check for identical content before the write-only regular-file probe. Co-Authored-By: Claude * fix(editor): tell non-UTF-8 files apart from disk changes, and quiet false resync banners - Local guarded writes judge losslessness by bytes, so a valid UTF-8 file containing U+FFFD autosaves again. A file that really isn't UTF-8 is refused with its own reason, 'not-utf8' (older clients read it as 'changed'), which pauses autosave for that file with one notice until an explicit save converts it. - A watch resync whose disk read fails leaves dirty tabs alone; the guarded save still catches a real conflict. - Resync and restored-tab checks treat disk holding the tab's own just-written save as an echo. - On hosts with only the v1 guarded write, only automatic saves read the file first; explicit saves go straight to the host's guard. Co-Authored-By: Claude * fix(editor): lift the non-UTF-8 autosave pause when its edits end, and report disk changes first - The pause is released once no dirty tab of the file remains (close, reload from disk, discard), so a reopened or reloaded file's next autosave asks the host again and notifies. - Client-side precondition checks compare the signature before looking for U+FFFD, so an external rewrite shows the changed-on-disk banner rather than the non-UTF-8 notice. Co-Authored-By: Claude --------- Co-authored-by: Claude --- .../filesystem/filesystem-write-handlers.ts | 2 - .../filesystem-write-identical.test.ts | 64 ++++++ .../local-disk-write-precondition.test.ts | 212 ++++++++++++++++-- .../local-disk-write-precondition.ts | 43 +++- .../markdown-tab-disk-documents.test.ts | 6 +- src/main/runtime/markdown-tab-disk-host.ts | 27 ++- ...runtime-mobile-markdown-tab-routes.test.ts | 40 +++- .../files-write-disk-precondition.test.ts | 6 +- .../editor-autosave-conflict-flow.test.ts | 70 +++--- ...editor-autosave-controller-test-fixture.ts | 5 +- .../src/components/editor/editor-autosave.ts | 16 +- .../editor/editor-non-utf8-autosave-pause.ts | 57 +++++ ...restored-tab-conflict-scan.recheck.test.ts | 97 +------- .../editor-restored-tab-conflict-scan.test.ts | 64 +++--- .../editor-restored-tab-conflict-scan.ts | 188 +++++----------- ...ditor-save-disk-precondition.races.test.ts | 2 +- .../editor-save-disk-precondition.test.ts | 42 +++- .../editor/editor-save-disk-precondition.ts | 15 +- .../editor-save-guarded-disk-fixture.ts | 6 +- .../editor/editor-save-large-paste.test.ts | 15 +- .../editor-save-lossless-baseline.test.ts | 100 +++++++++ .../components/editor/editor-save-queue.ts | 17 +- .../editor/editor-save-rebaseline.ts | 22 ++ .../editor-save-shared-model-dirty.test.ts | 1 - ...editor-external-watch-dirty-resync.test.ts | 84 +++++++ ...editor-external-watch-disk-verification.ts | 32 +-- .../editor-external-watch-overflow.test.ts | 21 +- .../hooks/editor-external-watch-overflow.ts | 35 ++- ...itorExternalWatch-background-trim.test.tsx | 2 +- .../useEditorExternalWatch-self-move.test.ts | 12 +- ...seEditorExternalWatch-shared-root.test.tsx | 3 +- ...seEditorExternalWatch-switch-back.test.tsx | 58 +---- src/renderer/src/i18n/locales/en.json | 3 + .../lib/execute-open-editor-path-move.test.ts | 6 +- ...ntime-file-write-disk-precondition.test.ts | 80 ++++++- .../runtime-file-write-disk-precondition.ts | 33 ++- .../editor-buffer-record-lifecycle.test.ts | 35 +-- .../editor-layout-owner-records.test.ts | 4 - .../slices/editor-read-only-tabs.test.ts | 1 - .../slices/editor-rekey-open-files.test.ts | 13 +- .../actions/hydrated-editor-session-state.ts | 7 - .../editor/actions/open-file-mutations.ts | 16 -- .../editor/actions/rekey-open-files-action.ts | 17 +- .../editor/buffers/editor-buffer-record.ts | 11 +- .../slices/editor/types/editor-files-slice.ts | 3 - .../store/slices/editor/types/open-file.ts | 4 - .../restored-editor-owner-reparent.test.ts | 6 +- .../src/web/web-file-mutation-methods.ts | 28 ++- .../web-file-write-disk-precondition.test.ts | 85 ++++++- .../file-write-disk-precondition.test.ts | 54 ++++- src/shared/file-write-disk-precondition.ts | 62 ++++- src/shared/files-runtime-capabilities.ts | 6 +- .../rpc-contract/files-mutation-params.ts | 3 +- 53 files changed, 1226 insertions(+), 615 deletions(-) create mode 100644 src/main/ipc/filesystem/filesystem-write-identical.test.ts create mode 100644 src/renderer/src/components/editor/editor-non-utf8-autosave-pause.ts create mode 100644 src/renderer/src/components/editor/editor-save-lossless-baseline.test.ts create mode 100644 src/renderer/src/hooks/editor-external-watch-dirty-resync.test.ts diff --git a/src/main/ipc/filesystem/filesystem-write-handlers.ts b/src/main/ipc/filesystem/filesystem-write-handlers.ts index f6ca923e686..edf0891936d 100644 --- a/src/main/ipc/filesystem/filesystem-write-handlers.ts +++ b/src/main/ipc/filesystem/filesystem-write-handlers.ts @@ -13,7 +13,6 @@ import { isENOENT } from '../filesystem-path-containment' import { registerFilesystemMutationHandlers } from '../filesystem-mutations' import { registerAgentSessionAttachmentUploadHandlers } from '../agent-session-attachment-upload-ipc' import type { FilesystemHandlerContext } from './filesystem-handler-context' -import { assertLocalWriteTargetIsRegularFile } from './local-regular-file-read' import type { FileWriteDiskPrecondition, FileWriteResult @@ -55,7 +54,6 @@ export function registerFilesystemWriteHandlers(context: FilesystemHandlerContex throw error } } - await assertLocalWriteTargetIsRegularFile(filePath) return writeLocalTextFileWithDiskPrecondition(filePath, args.content, args.diskPrecondition) } ) diff --git a/src/main/ipc/filesystem/filesystem-write-identical.test.ts b/src/main/ipc/filesystem/filesystem-write-identical.test.ts new file mode 100644 index 00000000000..91e73a07b4b --- /dev/null +++ b/src/main/ipc/filesystem/filesystem-write-identical.test.ts @@ -0,0 +1,64 @@ +import { chmod, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import { getDiskBaselineSignature } from '../../../shared/disk-baseline-signature' +import { NOT_A_REGULAR_FILE_MESSAGE } from './local-regular-file-read' +import { registerFilesystemWriteHandlers } from './filesystem-write-handlers' +import type { FilesystemHandlerContext } from './filesystem-handler-context' + +const handlers = vi.hoisted(() => new Map unknown>()) +vi.mock('electron', () => ({ + ipcMain: { + handle: (name: string, handler: (_event: unknown, args: unknown) => unknown) => + handlers.set(name, handler) + }, + shell: {} +})) +vi.mock('../local-file-access-resolution', () => ({ + resolveLocalWriteRequestPath: async (path: string) => path +})) +vi.mock('../filesystem-mutations', () => ({ registerFilesystemMutationHandlers: vi.fn() })) +vi.mock('../agent-session-attachment-upload-ipc', () => ({ + registerAgentSessionAttachmentUploadHandlers: vi.fn() +})) + +let root = '' +beforeEach(async () => { + root = await mkdtemp(join(tmpdir(), 'orca-identical-ipc-')) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: authorization is stubbed, and no other context member is used by this handler. + registerFilesystemWriteHandlers({ store: {} } as FilesystemHandlerContext) +}) +afterEach(async () => { + await rm(root, { recursive: true, force: true }) + handlers.clear() +}) + +it('accepts identical content on a readable, non-writable file through IPC', async () => { + const filePath = join(root, 'read-only.txt') + await writeFile(filePath, 'edited') + await chmod(filePath, 0o444) + try { + await expect( + handlers.get('fs:writeFile')!(null, { + filePath, + content: 'edited', + diskPrecondition: { expectedSignature: getDiskBaselineSignature('original') } + }) + ).resolves.toEqual({ ok: true, unchanged: true }) + expect(await readFile(filePath, 'utf8')).toBe('edited') + } finally { + await chmod(filePath, 0o644) + } +}) + +it + .skipIf(process.platform === 'win32') + .each([undefined, { expectedSignature: getDiskBaselineSignature('') }])( + 'still refuses non-regular files through IPC (%j)', + async (diskPrecondition) => { + await expect( + handlers.get('fs:writeFile')!(null, { filePath: '/dev/null', content: '', diskPrecondition }) + ).rejects.toThrow(NOT_A_REGULAR_FILE_MESSAGE) + } +) diff --git a/src/main/ipc/filesystem/local-disk-write-precondition.test.ts b/src/main/ipc/filesystem/local-disk-write-precondition.test.ts index 3e37ee6d8db..8f869ba3c65 100644 --- a/src/main/ipc/filesystem/local-disk-write-precondition.test.ts +++ b/src/main/ipc/filesystem/local-disk-write-precondition.test.ts @@ -1,4 +1,14 @@ -import { link, mkdir, mkdtemp, readFile, realpath, rm, symlink, writeFile } from 'node:fs/promises' +import { + chmod, + link, + mkdir, + mkdtemp, + readFile, + realpath, + rm, + symlink, + writeFile +} from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -7,7 +17,8 @@ import type { RuntimeStore } from '../../runtime/runtime-store-contract' import { createLocalFilesystemProvider } from '../../providers/local-filesystem-provider' import { checkLocalDiskWritePrecondition, - diskSignatureOfUtf8 + diskSignatureOfUtf8, + writeLocalTextFileWithDiskPrecondition } from './local-disk-write-precondition' import type * as LocalRegularFileRead from './local-regular-file-read' import type * as NodeFsPromises from 'node:fs/promises' @@ -51,14 +62,14 @@ vi.mock('node:fs/promises', async (importOriginal) => { return zeroInodes.enabled ? Object.assign(stats, { ino: 0n }) : stats }, // Why: holds one save between its passed check and its write, where a competing save must wait. - writeFile: async (...args: Parameters) => { + writeFile: vi.fn(async (...args: Parameters) => { const gate = typeof args[1] === 'string' ? writeGates.get(args[1]) : undefined if (gate) { gate.entered() await gate.release } return actual.writeFile(...args) - } + }) } }) vi.mock('./local-regular-file-read', async (importOriginal) => { @@ -104,20 +115,80 @@ describe('checkLocalDiskWritePrecondition', () => { await writeFile(filePath, content) await expect( - checkLocalDiskWritePrecondition(filePath, { - expectedSignature: getDiskBaselineSignature(content) - }) + checkLocalDiskWritePrecondition( + filePath, + { + expectedSignature: getDiskBaselineSignature(content) + }, + 'my save' + ) ).resolves.toBeNull() }) + it.each([2, 1024 * 1024 + 2])( + 'refuses lossy automatic baseline matches (%i bytes)', + async (size) => { + const filePath = join(base, 'invalid.ts') + const old = Buffer.alloc(size, 0x61) + old[size - 1] = 0xfe + const newer = Buffer.from(old) + newer[size - 1] = 0xff + await writeFile(filePath, newer) + const decoded = old.toString('utf8') + const expectedSignature = getDiskBaselineSignature(decoded) + await expect( + writeLocalTextFileWithDiskPrecondition(filePath, decoded, { + expectedSignature, + requireLosslessUtf8: true + }) + ).resolves.toEqual({ ok: false, diskConflict: 'not-utf8' }) + expect(await readFile(filePath)).toEqual(newer) + await expect( + writeLocalTextFileWithDiskPrecondition(filePath, decoded, { expectedSignature }) + ).resolves.toEqual({ ok: true }) + expect(await readFile(filePath)).toEqual(Buffer.from(decoded)) + } + ) + + it.each([0, 1024 * 1024 + 2])( + 'lets an automatic save edit valid UTF-8 that contains U+FFFD (%i padding)', + async (padding) => { + const filePath = join(base, 'replacement-char.ts') + const original = `${'a'.repeat(padding)}\uFFFD` + await writeFile(filePath, original) + await expect( + writeLocalTextFileWithDiskPrecondition(filePath, `${original}!`, { + expectedSignature: getDiskBaselineSignature(original), + requireLosslessUtf8: true + }) + ).resolves.toEqual({ ok: true }) + expect(await readFile(filePath, 'utf8')).toBe(`${original}!`) + } + ) + + it('reports an automatic save over a changed non-UTF-8 file as changed', async () => { + const filePath = join(base, 'latin1.ts') + await writeFile(filePath, Buffer.from([0x63, 0x61, 0x66, 0xe9])) + await expect( + writeLocalTextFileWithDiskPrecondition(filePath, 'x', { + expectedSignature: getDiskBaselineSignature('other'), + requireLosslessUtf8: true + }) + ).resolves.toEqual({ ok: false, diskConflict: 'changed' }) + }) + it('reports a same-size external rewrite as changed', async () => { const filePath = join(base, 'a.ts') await writeFile(filePath, 'agent') await expect( - checkLocalDiskWritePrecondition(filePath, { - expectedSignature: getDiskBaselineSignature('human') - }) + checkLocalDiskWritePrecondition( + filePath, + { + expectedSignature: getDiskBaselineSignature('human') + }, + 'my save' + ) ).resolves.toBe('changed') }) @@ -126,9 +197,13 @@ describe('checkLocalDiskWritePrecondition', () => { await writeFile(filePath, 'x'.repeat(100)) await expect( - checkLocalDiskWritePrecondition(filePath, { - expectedSignature: getDiskBaselineSignature('short') - }) + checkLocalDiskWritePrecondition( + filePath, + { + expectedSignature: getDiskBaselineSignature('short') + }, + 'my save' + ) ).resolves.toBe('changed') }) @@ -138,9 +213,13 @@ describe('checkLocalDiskWritePrecondition', () => { fileRead.understateSizeBy = 10 await expect( - checkLocalDiskWritePrecondition(filePath, { - expectedSignature: getDiskBaselineSignature('ab') - }) + checkLocalDiskWritePrecondition( + filePath, + { + expectedSignature: getDiskBaselineSignature('ab') + }, + 'my save' + ) ).resolves.toBe('changed') }) @@ -148,15 +227,57 @@ describe('checkLocalDiskWritePrecondition', () => { const filePath = join(base, 'gone.ts') const expectedSignature = getDiskBaselineSignature('was here') - await expect(checkLocalDiskWritePrecondition(filePath, { expectedSignature })).resolves.toBe( - 'missing' - ) await expect( - checkLocalDiskWritePrecondition(filePath, { expectedSignature, allowMissing: true }) + checkLocalDiskWritePrecondition(filePath, { expectedSignature }, 'my save') + ).resolves.toBe('missing') + await expect( + checkLocalDiskWritePrecondition( + filePath, + { expectedSignature, allowMissing: true }, + 'my save' + ) ).resolves.toBeNull() }) }) +describe('a changed disk that already holds the save', () => { + const baseline = { expectedSignature: getDiskBaselineSignature('original') } + + it.each([ + ['the same length as the baseline', 'origin2'], + ['a length the baseline cannot decode to', 'x'.repeat(100)], + ['multi-byte text', 'café ☕ 𝄞\r\n'] + ])('passes when disk holds exactly the bytes being saved (%s)', async (_label, content) => { + const filePath = join(base, 'a.ts') + await writeFile(filePath, content) + + await expect(checkLocalDiskWritePrecondition(filePath, baseline, content)).resolves.toBe( + 'identical' + ) + }) + + it('compares bytes, so a BOM or a line-ending difference still refuses', async () => { + const filePath = join(base, 'a.ts') + await writeFile(filePath, '\uFEFFmine\n') + await expect(checkLocalDiskWritePrecondition(filePath, baseline, 'mine\n')).resolves.toBe( + 'changed' + ) + await writeFile(filePath, 'mine\r\n') + await expect(checkLocalDiskWritePrecondition(filePath, baseline, 'mine\n')).resolves.toBe( + 'changed' + ) + }) + + it('treats invalid UTF-8 on disk as different from its decoded text', async () => { + const filePath = join(base, 'a.ts') + await writeFile(filePath, Buffer.from([0x61, 0xff])) + + await expect(checkLocalDiskWritePrecondition(filePath, baseline, 'a\uFFFD')).resolves.toBe( + 'changed' + ) + }) +}) + describe('sliced disk hashing', () => { it('matches the whole-file signature across slice boundaries and invalid bytes', async () => { const slice = 1024 * 1024 @@ -196,6 +317,45 @@ describe('local provider guarded write', () => { }) ).resolves.toEqual({ ok: false, diskConflict: 'changed' }) expect(await readFile(filePath, 'utf8')).toBe('edited') + + await expect( + provider().writeFileWithDiskPrecondition(filePath, 'edited', { + expectedSignature: getDiskBaselineSignature('original') + }) + ).resolves.toEqual({ ok: true, unchanged: true }) + }) + + it.each(['original', 'edited'])( + 'does not write identical bytes against baseline %s', + async (baseline) => { + const filePath = join(base, 'identical.ts') + await writeFile(filePath, 'edited') + vi.mocked(writeFile).mockClear() + + await expect( + provider().writeFileWithDiskPrecondition(filePath, 'edited', { + expectedSignature: getDiskBaselineSignature(baseline) + }) + ).resolves.toEqual({ ok: true, unchanged: true }) + expect(writeFile).not.toHaveBeenCalled() + } + ) + + it('accepts identical content on a read-only file without opening it for write', async () => { + const filePath = join(base, 'read-only.ts') + await writeFile(filePath, 'edited') + await chmod(filePath, 0o444) + vi.mocked(writeFile).mockClear() + try { + await expect( + provider().writeFileWithDiskPrecondition(filePath, 'edited', { + expectedSignature: getDiskBaselineSignature('original') + }) + ).resolves.toEqual({ ok: true, unchanged: true }) + expect(writeFile).not.toHaveBeenCalled() + } finally { + await chmod(filePath, 0o644) + } }) it('lets only one of two concurrent saves from the same baseline write', async () => { @@ -441,10 +601,14 @@ describe('local provider guarded write', () => { ) await expect( - checkLocalDiskWritePrecondition(filePath, { - expectedSignature: getDiskBaselineSignature('original'), - allowMissing: true - }) + checkLocalDiskWritePrecondition( + filePath, + { + expectedSignature: getDiskBaselineSignature('original'), + allowMissing: true + }, + 'my save' + ) ).rejects.toThrow('EACCES') }) diff --git a/src/main/ipc/filesystem/local-disk-write-precondition.ts b/src/main/ipc/filesystem/local-disk-write-precondition.ts index f46303fbb56..e9c2d9d99e5 100644 --- a/src/main/ipc/filesystem/local-disk-write-precondition.ts +++ b/src/main/ipc/filesystem/local-disk-write-precondition.ts @@ -1,3 +1,4 @@ +import { isUtf8 } from 'node:buffer' import { StringDecoder } from 'node:string_decoder' import { DiskBaselineSignatureBuilder, @@ -8,7 +9,8 @@ import { import { compareDiskWritePrecondition, FILE_WRITE_OK, - type FileWriteDiskConflict, + FILE_WRITE_UNCHANGED, + type FileWriteDiskComparison, type FileWriteDiskPrecondition, type FileWriteResult } from '../../../shared/file-write-disk-precondition' @@ -17,6 +19,7 @@ import { basename, dirname, join, normalize, resolve } from 'node:path' import { runKeyedSerializedOperation } from '../../cli/keyed-promise-queue' import { serializedParentDirectoryKey } from '../../destination-serialized-local-rename' import { + assertLocalWriteTargetIsRegularFile, FileTooLargeError, openLocalRegularFile, readLocalFileBounded @@ -41,36 +44,51 @@ export async function diskSignatureOfUtf8(buffer: Buffer): Promise { /** Reads the file the writer is about to replace and reports why it no longer matches the baseline. */ export async function checkLocalDiskWritePrecondition( filePath: string, - precondition: FileWriteDiskPrecondition -): Promise { + precondition: FileWriteDiskPrecondition, + content: string +): Promise { let opened: Awaited> try { opened = await openLocalRegularFile(filePath) } catch (error) { // Why the code alone: the message quotes the path, which can spell any error. if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { - return compareDiskWritePrecondition(precondition, null) + return compareDiskWritePrecondition(precondition, null, content) } throw error } const { handle, stats } = opened try { + const written = Buffer.from(content, 'utf8') const expectedLength = diskBaselineSignatureLength(precondition.expectedSignature) - // Why: the signature pins the decoded length, so a size it can't decode to skips reading a huge file. - if (expectedLength === null || !utf8ByteSizeCanDecodeToLength(stats.size, expectedLength)) { + // Why: the signature pins the decoded length, so a size neither it nor the save can match skips reading a huge file. + const baselineFits = + expectedLength !== null && utf8ByteSizeCanDecodeToLength(stats.size, expectedLength) + if (!baselineFits && stats.size !== written.length) { return 'changed' } let buffer: Buffer try { - buffer = await readLocalFileBounded(handle, expectedLength * 3, stats.size) + buffer = await readLocalFileBounded( + handle, + Math.max(baselineFits ? expectedLength * 3 : 0, written.length), + stats.size + ) } catch (error) { - // Why: grown past what the baseline could decode from since the size check, so it changed. + // Why: grown past what the baseline or the save could match since the size check, so it changed. if (error instanceof FileTooLargeError) { return 'changed' } throw error } - return (await diskSignatureOfUtf8(buffer)) === precondition.expectedSignature ? null : 'changed' + if (buffer.equals(written)) { + return 'identical' + } + if (!baselineFits || (await diskSignatureOfUtf8(buffer)) !== precondition.expectedSignature) { + return 'changed' + } + // Why bytes: a decoded U+FFFD may be a real character or replaced bytes; only the bytes tell. + return precondition.requireLosslessUtf8 && !isUtf8(buffer) ? 'not-utf8' : null } finally { await handle.close() } @@ -147,14 +165,19 @@ export async function writeLocalTextFileWithDiskPrecondition( ): Promise { // Why: an unguarded write has nothing to compare, so it skips the locks a stuck guarded save holds. if (!precondition) { + await assertLocalWriteTargetIsRegularFile(filePath) await writeFile(filePath, content, 'utf-8') return FILE_WRITE_OK } const compareAndWrite = async (): Promise => { - const diskConflict = await checkLocalDiskWritePrecondition(filePath, precondition) + const diskConflict = await checkLocalDiskWritePrecondition(filePath, precondition, content) + if (diskConflict === 'identical') { + return FILE_WRITE_UNCHANGED + } if (diskConflict) { return { ok: false, diskConflict } } + await assertLocalWriteTargetIsRegularFile(filePath) await writeFile(filePath, content, 'utf-8') return FILE_WRITE_OK } diff --git a/src/main/runtime/markdown-tab-disk-documents.test.ts b/src/main/runtime/markdown-tab-disk-documents.test.ts index 123f984cc63..eeb448eea3e 100644 --- a/src/main/runtime/markdown-tab-disk-documents.test.ts +++ b/src/main/runtime/markdown-tab-disk-documents.test.ts @@ -58,7 +58,11 @@ function makeHost(disk: string, draft?: string, saved: Partial { - if (compareDiskWritePrecondition(precondition, state.disk)) { + const comparison = compareDiskWritePrecondition(precondition, state.disk, content) + if (comparison === 'identical') { + return { ok: true as const, unchanged: true } + } + if (comparison) { return { ok: false as const, diskConflict: 'changed' as const } } state.disk = content diff --git a/src/main/runtime/markdown-tab-disk-host.ts b/src/main/runtime/markdown-tab-disk-host.ts index a0c844340d7..03e38248a88 100644 --- a/src/main/runtime/markdown-tab-disk-host.ts +++ b/src/main/runtime/markdown-tab-disk-host.ts @@ -11,8 +11,10 @@ import { diskConflictFromReadFailure, FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, FILE_WRITE_IF_UNCHANGED_METHOD, + hostEnforcesPrecondition, + FILE_WRITE_UNCHANGED, parseFileWriteResult, - type FileWriteDiskConflict, + type FileWriteDiskComparison, type FileWriteDiskPrecondition, type FileWriteResult } from '../../shared/file-write-disk-precondition' @@ -223,35 +225,42 @@ async function writeFileOnEnvironment( // Why `local`: the server writes only a workspace it runs itself, so the write never hops on. expectedExecutionHostId: 'local' } - if (capabilities.includes(FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY)) { + if (hostEnforcesPrecondition(capabilities, diskPrecondition)) { return parseFileWriteResult( await pinned(FILE_WRITE_IF_UNCHANGED_METHOD, { ...params, diskPrecondition }) ) } const diskConflict = await diskConflictOnServer(pinned, params, diskPrecondition) + if (diskConflict === 'identical') { + return FILE_WRITE_UNCHANGED + } if (diskConflict) { return { ok: false, diskConflict } } - return parseFileWriteResult(await pinned('files.write', params)) + return parseFileWriteResult( + capabilities.includes(FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY) + ? await pinned(FILE_WRITE_IF_UNCHANGED_METHOD, { ...params, diskPrecondition }) + : await pinned('files.write', params) + ) } /** For a server that would write unguarded: a read that cannot prove the disk fails the save. */ async function diskConflictOnServer( pinned: (method: string, params: unknown) => Promise, - address: { worktree: string; relativePath: string }, + write: { worktree: string; relativePath: string; content: string }, precondition: FileWriteDiskPrecondition -): Promise { +): Promise { let read: unknown try { read = await pinned('files.read', { - worktree: address.worktree, - relativePath: address.relativePath + worktree: write.worktree, + relativePath: write.relativePath }) } catch (error) { return diskConflictFromReadFailure(precondition, error) } if (!isRecord(read) || typeof read.content !== 'string' || read.truncated === true) { - return compareDiskWritePrecondition(precondition, undefined) + return compareDiskWritePrecondition(precondition, undefined, write.content) } - return compareDiskWritePrecondition(precondition, read.content) + return compareDiskWritePrecondition(precondition, read.content, write.content) } diff --git a/src/main/runtime/orca-runtime-mobile-markdown-tab-routes.test.ts b/src/main/runtime/orca-runtime-mobile-markdown-tab-routes.test.ts index bcb09b2bb39..bc17398e0c9 100644 --- a/src/main/runtime/orca-runtime-mobile-markdown-tab-routes.test.ts +++ b/src/main/runtime/orca-runtime-mobile-markdown-tab-routes.test.ts @@ -12,6 +12,7 @@ import { RemoteRuntimeClientError } from '../../shared/remote-runtime-client-err import { FILE_MUTATION_OWNERSHIP_RUNTIME_CAPABILITY } from '../../shared/protocol-version' import { FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, + FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY, FILE_WRITE_IF_UNCHANGED_METHOD } from '../../shared/file-write-disk-precondition' import { SshHostNotServedError } from '../providers/ssh-host-not-served-error' @@ -96,7 +97,8 @@ async function runtimeOnHost( const CURRENT_SERVER = [ FILE_MUTATION_OWNERSHIP_RUNTIME_CAPABILITY, - FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY + FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, + FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY ] /** A server from before #30605: it advertises no disk check and serves no `files.writeIfUnchanged`. */ const PRE_DISK_CHECK_SERVER = [FILE_MUTATION_OWNERSHIP_RUNTIME_CAPABILITY] @@ -271,6 +273,42 @@ describe("a phone's Markdown tab on a paired server's workspace", () => { expect(await readFile(readme, 'utf8')).toBe('# Changed on the server\n') }) + it('saves on a server too old to check the disk when its changed disk already holds the text', async () => { + const { desktop, call, serverCalls } = await desktopOnServer('local', PRE_DISK_CHECK_SERVER) + const forward = call.getMockImplementation()! + call.mockImplementation(async (environmentId, method, params, ...rest) => { + if (method === 'status.get') { + await writeFile(readme, '# Phone\n') + } + return forward(environmentId, method, params, ...rest) + }) + + await expect( + desktop.saveMobileMarkdownTab( + `id:${TEST_WORKTREE_ID}`, + readme, + hashMarkdownContent('# Notes\n'), + '# Phone\n' + ) + ).resolves.toMatchObject({ content: '# Phone\n', isDirty: false }) + expect(serverCalls).toEqual(['files.read', 'status.get', 'files.read']) + }) + + it('sends a phone save straight to a v1 server guard, without reading first', async () => { + const { desktop, serverCalls } = await desktopOnServer('local', [ + FILE_MUTATION_OWNERSHIP_RUNTIME_CAPABILITY, + FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY + ]) + await desktop.saveMobileMarkdownTab( + `id:${TEST_WORKTREE_ID}`, + readme, + hashMarkdownContent('# Notes\n'), + '# Phone\n' + ) + expect(serverCalls).toEqual(['files.read', 'status.get', 'files.writeIfUnchanged']) + expect(await readFile(readme, 'utf8')).toBe('# Phone\n') + }) + it('refuses a save whose server was re-paired after its first read, writing nothing', async () => { const { desktop, call, serverCalls, pairing } = await desktopOnServer() const forward = call.getMockImplementation()! diff --git a/src/main/runtime/rpc/methods/files-write-disk-precondition.test.ts b/src/main/runtime/rpc/methods/files-write-disk-precondition.test.ts index 60f6e1cca81..d4f96a82f8c 100644 --- a/src/main/runtime/rpc/methods/files-write-disk-precondition.test.ts +++ b/src/main/runtime/rpc/methods/files-write-disk-precondition.test.ts @@ -17,7 +17,11 @@ describe('files.writeIfUnchanged', () => { writeFileExplorerFile: vi.fn().mockResolvedValue({ ok: false, diskConflict: 'changed' }) } as unknown as OrcaRuntimeService const dispatcher = new RpcDispatcher({ runtime, methods: FILE_METHODS }) - const diskPrecondition = { expectedSignature: 'a-b-1', allowMissing: true } + const diskPrecondition = { + expectedSignature: 'a-b-1', + allowMissing: true, + requireLosslessUtf8: true + } const response = await dispatcher.dispatch( makeRequest('files.writeIfUnchanged', { diff --git a/src/renderer/src/components/editor/editor-autosave-conflict-flow.test.ts b/src/renderer/src/components/editor/editor-autosave-conflict-flow.test.ts index c8b7b576eca..609b6649a5b 100644 --- a/src/renderer/src/components/editor/editor-autosave-conflict-flow.test.ts +++ b/src/renderer/src/components/editor/editor-autosave-conflict-flow.test.ts @@ -8,9 +8,12 @@ import { attachEditorAutosaveController } from './editor-autosave-controller' import { __clearSelfWriteRegistryForTests, recordSelfWrite } from './editor-self-write-registry' import { createEditorStore, + createFakeEditorDisk, stubEditorWindow, + stubEditorWindowWithDisk, withLoadedBaselines } from './editor-autosave-controller-test-fixture' +import { getDiskBaselineSignature } from '../../../../shared/disk-baseline-signature' const mocks = vi.hoisted(() => ({ getConnectionIdForFile: vi.fn() @@ -144,33 +147,48 @@ describe('editor autosave changed-on-disk conflict flow', () => { } }) - it('suspends autosave while a restored tab awaits disk baseline verification', async () => { - const writeFile = stubEditorWindow() - const store = createEditorStore() - openDirtyFile(store, 'restored draft') - // Why: mimic hydration — the scan has not yet compared disk against the - // persisted baseline, so autosave must hold off (a slow remote read must - // not lose a race to this timer). - store.getState().setPendingDiskBaselineVerification('/repo/file.ts', true) - - const cleanup = attachEditorAutosaveController(withLoadedBaselines(store)) - try { - await vi.advanceTimersByTimeAsync(1500) - expect(writeFile).not.toHaveBeenCalled() - - store.getState().clearPendingDiskBaselineVerification('/repo/file.ts') - await vi.advanceTimersByTimeAsync(1500) - expect(writeFile).toHaveBeenCalledWith( - expect.objectContaining({ - filePath: '/repo/file.ts', - content: 'restored draft', - connectionId: undefined, - expectedExecutionHostId: 'local' - }) - ) - } finally { - cleanup() + describe('a restored draft autosaves through the guarded write alone', () => { + // Why: hydration restores the draft with the persisted baseline; nothing pauses its autosave. + function restore(disk: string) { + const fakeDisk = stubEditorWindowWithDisk(createFakeEditorDisk({ '/repo/file.ts': disk })) + const store = createEditorStore() + openDirtyFile(store, 'restored draft') + store.getState().setLastKnownDiskSignature('/repo/file.ts', getDiskBaselineSignature('base')) + return { fakeDisk, store, cleanup: attachEditorAutosaveController(store) } } + + it.each([ + ['disk still holds the baseline', 'base'], + ['disk already holds the draft', 'restored draft'] + ])('lands and goes clean without a banner when %s', async (_label, disk) => { + const { fakeDisk, store, cleanup } = restore(disk) + try { + await vi.advanceTimersByTimeAsync(1500) + expect(fakeDisk.files.get('/repo/file.ts')).toBe('restored draft') + expect(store.getState().openFiles[0]).toMatchObject({ isDirty: false }) + expect(store.getState().openFiles[0]?.externalMutation).toBeUndefined() + } finally { + cleanup() + } + }) + + it('is refused with the banner, keeps the draft and stops autosaving when disk changed', async () => { + const { fakeDisk, store, cleanup } = restore('agent write') + try { + await vi.advanceTimersByTimeAsync(1500) + expect(fakeDisk.files.get('/repo/file.ts')).toBe('agent write') + expect(store.getState().openFiles[0]).toMatchObject({ + isDirty: true, + externalMutation: 'changed' + }) + expect(store.getState().editorDrafts['/repo/file.ts']).toBe('restored draft') + + await vi.advanceTimersByTimeAsync(5000) + expect(fakeDisk.fs.writeFile).toHaveBeenCalledTimes(1) + } finally { + cleanup() + } + }) }) it("clears the changed-on-disk mark after the banner's Overwrite", async () => { diff --git a/src/renderer/src/components/editor/editor-autosave-controller-test-fixture.ts b/src/renderer/src/components/editor/editor-autosave-controller-test-fixture.ts index dd9d9b5fd61..82231c25c39 100644 --- a/src/renderer/src/components/editor/editor-autosave-controller-test-fixture.ts +++ b/src/renderer/src/components/editor/editor-autosave-controller-test-fixture.ts @@ -39,8 +39,11 @@ export function createFakeEditorDisk(initialFiles: Record = {}): diskPrecondition?: FileWriteDiskPrecondition }) => { const diskConflict = diskPrecondition - ? compareDiskWritePrecondition(diskPrecondition, files.get(filePath) ?? null) + ? compareDiskWritePrecondition(diskPrecondition, files.get(filePath) ?? null, content) : null + if (diskConflict === 'identical') { + return { ok: true, unchanged: true } + } if (diskConflict) { return { ok: false, diskConflict } } diff --git a/src/renderer/src/components/editor/editor-autosave.ts b/src/renderer/src/components/editor/editor-autosave.ts index 1a143059c29..7b13dea95a4 100644 --- a/src/renderer/src/components/editor/editor-autosave.ts +++ b/src/renderer/src/components/editor/editor-autosave.ts @@ -8,6 +8,7 @@ import { MIN_EDITOR_AUTO_SAVE_DELAY_MS } from '../../../../shared/constants' import { clampNumber } from '@/lib/terminal-theme' +import { isAutosavePausedForNonUtf8File } from './editor-non-utf8-autosave-pause' export const ORCA_EDITOR_QUIESCE_FILE_SAVES_EVENT = 'orca:editor-quiesce-file-saves' export const ORCA_EDITOR_EXTERNAL_FILE_CHANGE_EVENT = 'orca:editor-external-file-change' @@ -106,24 +107,19 @@ export function canAutoSaveOpenFile(file: OpenFile): boolean { return file.mode === 'edit' || (file.mode === 'diff' && file.diffSource === 'unstaged') } -// Why: autosave must not resolve a changed-on-disk conflict by overwriting -// the newer external content, nor write over a restored tab whose disk -// baseline is still unverified (the conflict may simply not be marked YET). +// Why: autosave must not resolve a changed-on-disk conflict by overwriting the newer external +// content. An unmarked conflict needs no pause: every save is guarded, and a refusal sets the mark. // One predicate so the save-queue gate and the timer scheduler cannot drift. export function isAutosaveSuspendedForFile( file: Pick< OpenFile, - | 'externalMutation' - | 'pendingDiskBaselineVerification' - | 'pendingLiveDiskVerification' - | 'pendingOwnerMigration' + 'externalMutation' | 'pendingOwnerMigration' | 'filePath' | 'runtimeEnvironmentId' > ): boolean { return ( file.externalMutation === 'changed' || - file.pendingDiskBaselineVerification === true || - file.pendingLiveDiskVerification === true || - file.pendingOwnerMigration === true + file.pendingOwnerMigration === true || + isAutosavePausedForNonUtf8File(file) ) } diff --git a/src/renderer/src/components/editor/editor-non-utf8-autosave-pause.ts b/src/renderer/src/components/editor/editor-non-utf8-autosave-pause.ts new file mode 100644 index 00000000000..25997e5a114 --- /dev/null +++ b/src/renderer/src/components/editor/editor-non-utf8-autosave-pause.ts @@ -0,0 +1,57 @@ +import { toast } from 'sonner' +import { translate } from '@/i18n/i18n' +import { basename } from '@/lib/path' +import { normalizeAbsolutePathForComparison } from '@/components/right-sidebar/file-explorer-paths' +import type { OpenFile } from '@/store/slices/editor' + +type PausedFile = Pick + +// Why: autosave must not convert a non-UTF-8 file unasked; an explicit save consents and lifts the pause. +const pausedFiles = new Set() + +function pauseKey(file: PausedFile): string { + return `${file.runtimeEnvironmentId?.trim() || 'client'}::${normalizeAbsolutePathForComparison(file.filePath)}` +} + +export function isAutosavePausedForNonUtf8File(file: PausedFile): boolean { + return pausedFiles.size > 0 && pausedFiles.has(pauseKey(file)) +} + +/** Tells the user once per pause, so a refused autosave never repeats as a loop of notices. */ +export function pauseAutosaveForNonUtf8File(file: PausedFile): void { + const key = pauseKey(file) + if (pausedFiles.has(key)) { + return + } + pausedFiles.add(key) + toast.warning( + translate( + 'auto.components.editor.editor.save.notUtf8.notice', + "Autosave is paused for {{name}} because it isn't UTF-8 text. Save it yourself to convert it to UTF-8.", + { name: basename(file.filePath) } + ) + ) +} + +export function resumeAutosaveForNonUtf8File(file: PausedFile): void { + pausedFiles.delete(pauseKey(file)) +} + +/** Why: a close, reload or discard ends the edits the pause held back, so the next edit asks the host again. */ +export function releaseNonUtf8PausesWithoutDirtyTab( + openFiles: readonly (PausedFile & Pick)[] +): void { + if (pausedFiles.size === 0) { + return + } + const dirty = new Set(openFiles.filter((file) => file.isDirty).map(pauseKey)) + for (const key of pausedFiles) { + if (!dirty.has(key)) { + pausedFiles.delete(key) + } + } +} + +export function __clearNonUtf8AutosavePausesForTests(): void { + pausedFiles.clear() +} diff --git a/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.recheck.test.ts b/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.recheck.test.ts index d4e3abb235a..e152f3bf704 100644 --- a/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.recheck.test.ts +++ b/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.recheck.test.ts @@ -23,9 +23,7 @@ vi.mock('@/lib/file-owner-runtime-target', () => { import { createTestStore } from '@/store/slices/store-test-helpers' import { attachRestoredTabConflictScan, - getRestoredTabReverificationRequestCountForTest, - MAX_VERIFY_ATTEMPTS, - requestRestoredTabReverification + MAX_VERIFY_ATTEMPTS } from './editor-restored-tab-conflict-scan' import { getDiskBaselineSignature } from '../../../../shared/disk-baseline-signature' @@ -59,64 +57,9 @@ afterEach(() => { vi.useRealTimers() }) -it('reads once more when a new check is requested while a read is in flight', async () => { - const replies: ((content: string) => void)[] = [] - mocks.readRuntimeFileContent.mockImplementation( - () => new Promise((resolve) => replies.push((content) => resolve({ content, isBinary: false }))) - ) - store.getState().setPendingDiskBaselineVerification(fileId, true) - detach = attachRestoredTabConflictScan(store) - await vi.advanceTimersByTimeAsync(0) - expect(replies).toHaveLength(1) - - // A second catch-up lands mid-read, after an external write. - requestRestoredTabReverification(fileId) - requestRestoredTabReverification(fileId) - replies[0]('v1') - await vi.advanceTimersByTimeAsync(0) - expect(tab().pendingDiskBaselineVerification).toBe(true) - // Coalesced: two requests mid-read still cost one more read. - expect(replies).toHaveLength(2) - - replies[1]('v2 external write') - await vi.advanceTimersByTimeAsync(0) - expect(tab().externalMutation).toBe('changed') - expect(tab().pendingDiskBaselineVerification).toBeFalsy() -}) - -it.each([ - { disk: 'v1', banner: undefined }, - { disk: 'v2 external write', banner: 'changed' } -])('waits out overlapping checks, then judges a fresh read ($disk)', async ({ disk, banner }) => { - const replies: ((content: string) => void)[] = [] - mocks.readRuntimeFileContent.mockImplementation( - () => new Promise((resolve) => replies.push((content) => resolve({ content, isBinary: false }))) - ) - store.getState().setPendingDiskBaselineVerification(fileId, true) - detach = attachRestoredTabConflictScan(store) - await vi.advanceTimersByTimeAsync(0) - expect(replies).toHaveLength(1) - - // Every read is slower than the re-verify interval, so a newer check always lands first. - for (let i = 0; i < MAX_VERIFY_ATTEMPTS + 5; i += 1) { - requestRestoredTabReverification(fileId) - replies[i]('v1') - await vi.advanceTimersByTimeAsync(60_000) - expect(replies).toHaveLength(i + 2) - } - expect(tab().pendingDiskBaselineVerification).toBe(true) - expect(tab().externalMutation).toBeUndefined() - - replies.at(-1)!(disk) - await vi.advanceTimersByTimeAsync(0) - expect(tab().pendingDiskBaselineVerification).toBeFalsy() - expect(tab().externalMutation).toBe(banner) -}) - it('marks a dirty runtime-owned tab deleted when its host says the file is gone', async () => { mocks.readRuntimeFileContent.mockRejectedValue(new Error('read failed')) mocks.runtimePathsExist.mockResolvedValue([{ exists: false }]) - store.getState().setPendingDiskBaselineVerification(fileId, true) detach = attachRestoredTabConflictScan(store) await vi.advanceTimersByTimeAsync(0) @@ -127,47 +70,15 @@ it('marks a dirty runtime-owned tab deleted when its host says the file is gone' { codedAbsenceOnly: true } ) expect(tab().externalMutation).toBe('deleted') - expect(tab().pendingDiskBaselineVerification).toBeFalsy() }) -it('stops retrying and shows the conflict banner when the disk can never be checked', async () => { +it('stops retrying without a banner when the disk can never be checked', async () => { mocks.readRuntimeFileContent.mockRejectedValue(new Error('host unreachable')) mocks.runtimePathsExist.mockResolvedValue([{ error: 'unverifiable' }]) - store.getState().setPendingDiskBaselineVerification(fileId, true) detach = attachRestoredTabConflictScan(store) await vi.advanceTimersByTimeAsync(10 * 60_000) expect(mocks.readRuntimeFileContent).toHaveBeenCalledTimes(MAX_VERIFY_ATTEMPTS) - expect(tab().pendingDiskBaselineVerification).toBeFalsy() - expect(tab().externalMutation).toBe('changed') -}) - -it('does not let an absence answer from before a newer check resolve that check', async () => { - let answerProbe: (rows: unknown) => void = () => {} - mocks.readRuntimeFileContent.mockRejectedValueOnce(new Error('read failed')) - mocks.runtimePathsExist.mockImplementationOnce( - () => new Promise((resolve) => (answerProbe = resolve)) - ) - store.getState().setPendingDiskBaselineVerification(fileId, true) - detach = attachRestoredTabConflictScan(store) - await vi.advanceTimersByTimeAsync(0) - - // The file comes back and a new catch-up asks again while the old probe is pending. - requestRestoredTabReverification(fileId) - mocks.readRuntimeFileContent.mockResolvedValueOnce({ content: 'v2 recreated', isBinary: false }) - answerProbe([{ exists: false }]) - await vi.advanceTimersByTimeAsync(0) - - expect(tab().externalMutation).toBe('changed') - expect(tab().pendingDiskBaselineVerification).toBeFalsy() -}) - -it("forgets a tab's check requests once the tab closes", async () => { - mocks.readRuntimeFileContent.mockResolvedValue({ content: 'v1', isBinary: false }) - detach = attachRestoredTabConflictScan(store) - requestRestoredTabReverification(fileId) - expect(getRestoredTabReverificationRequestCountForTest()).toBeGreaterThan(0) - - store.getState().closeFile(fileId) - expect(getRestoredTabReverificationRequestCountForTest()).toBe(0) + // Why no banner: the guarded save refuses newer disk on its own. + expect(tab().externalMutation).toBeUndefined() }) diff --git a/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.test.ts b/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.test.ts index 10f6d31d683..dd6f52fce9a 100644 --- a/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.test.ts +++ b/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.test.ts @@ -5,6 +5,7 @@ import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../shared/constants' import type { AppState } from '@/store' import { attachRestoredTabConflictScan } from './editor-restored-tab-conflict-scan' import { getDiskBaselineSignature } from '../../../../shared/disk-baseline-signature' +import { __clearSelfWriteRegistryForTests, recordSelfWrite } from './editor-self-write-registry' const mocks = vi.hoisted(() => ({ readRuntimeFileContent: vi.fn(), @@ -39,8 +40,6 @@ function openRestoredDirtyTab( store.getState().setEditorDraft(fileId, 'restored draft') store.getState().markFileDirty(fileId, true) store.getState().setLastKnownDiskSignature(fileId, getDiskBaselineSignature(baselineContent)) - // Why: hydration flags restored dirty tabs for verification; tests mimic it. - store.getState().setPendingDiskBaselineVerification(fileId, true) } describe('attachRestoredTabConflictScan', () => { @@ -94,6 +93,22 @@ describe('attachRestoredTabConflictScan', () => { } }) + it('leaves a tab unmarked when disk holds its own just-written save', async () => { + mocks.readRuntimeFileContent.mockResolvedValue({ content: 'saved earlier', isBinary: false }) + const store = createTestStore() + openRestoredDirtyTab(store, '/repo/file.ts', 'original baseline') + recordSelfWrite('/repo/file.ts', 'saved earlier') + + const detach = attachRestoredTabConflictScan(store) + try { + await vi.advanceTimersByTimeAsync(10) + expect(store.getState().openFiles[0]?.externalMutation).toBeUndefined() + } finally { + detach() + __clearSelfWriteRegistryForTests() + } + }) + it('does not read files for clean tabs or tabs without a baseline', async () => { const store = createTestStore() store.getState().openFile({ @@ -141,37 +156,24 @@ describe('attachRestoredTabConflictScan', () => { } }) - it('clears the pending-verification flag on both verification outcomes', async () => { - mocks.readRuntimeFileContent.mockResolvedValue({ - content: 'original baseline', - isBinary: false - }) + it('leaves a restored dirty tab unmarked when disk already holds its draft', async () => { + // Why: its next guarded save lands (disk already equals it) and the tab goes clean. + mocks.readRuntimeFileContent.mockResolvedValue({ content: 'restored draft', isBinary: false }) const store = createTestStore() - openRestoredDirtyTab(store, '/repo/match.ts', 'original baseline') + openRestoredDirtyTab(store, '/repo/file.ts', 'original baseline') const detach = attachRestoredTabConflictScan(store) try { await vi.advanceTimersByTimeAsync(10) - // Why: the flag suspends autosave — leaving it set after a clean - // verification would strand the tab's autosave forever. - expect(store.getState().openFiles[0]?.pendingDiskBaselineVerification).toBeUndefined() - - mocks.readRuntimeFileContent.mockResolvedValue({ - content: 'agent rewrote this offline', - isBinary: false - }) - openRestoredDirtyTab(store, '/repo/mismatch.ts', 'original baseline') - await vi.advanceTimersByTimeAsync(10) - const mismatchTab = store.getState().openFiles.find((f) => f.id === '/repo/mismatch.ts') - expect(mismatchTab?.externalMutation).toBe('changed') - expect(mismatchTab?.pendingDiskBaselineVerification).toBeUndefined() + expect(store.getState().openFiles[0]?.externalMutation).toBeUndefined() } finally { detach() } }) - it('does not re-verify live dirty tabs that were never flagged at hydration', async () => { + it('does not verify a tab dirtied after the scan attached', async () => { const store = createTestStore() + const detach = attachRestoredTabConflictScan(store) store.getState().openFile({ filePath: '/repo/live.ts', relativePath: 'live.ts', @@ -183,7 +185,6 @@ describe('attachRestoredTabConflictScan', () => { store.getState().markFileDirty('/repo/live.ts', true) store.getState().setLastKnownDiskSignature('/repo/live.ts', getDiskBaselineSignature('base')) - const detach = attachRestoredTabConflictScan(store) try { await vi.advanceTimersByTimeAsync(10) // Why: in-session drift is the live watcher's job; re-reading here would @@ -195,9 +196,6 @@ describe('attachRestoredTabConflictScan', () => { }) it('resolves verification with a deleted mark when the file is definitively gone', async () => { - // Why: a file deleted while the app was closed can never verify — without - // a terminal state the retry loop keeps the tab's autosave silently - // suspended for the whole session. mocks.readRuntimeFileContent.mockRejectedValue(new Error('ENOENT')) mocks.pathExists.mockResolvedValue(false) const store = createTestStore() @@ -207,7 +205,6 @@ describe('attachRestoredTabConflictScan', () => { try { await vi.advanceTimersByTimeAsync(10) const tab = store.getState().openFiles[0] - expect(tab?.pendingDiskBaselineVerification).toBeUndefined() expect(tab?.externalMutation).toBe('deleted') // Why: the verification is resolved — no further retries may fire. await vi.advanceTimersByTimeAsync(60_000) @@ -217,10 +214,8 @@ describe('attachRestoredTabConflictScan', () => { } }) - it('keeps retrying with the suspension intact when the existence probe fails', async () => { - // Why: a transport that is down cannot disprove existence — lifting the - // suspension unverified would reopen the clobber window the moment the - // transport recovers. + it('keeps retrying when the existence probe fails', async () => { + // Why: a transport that is down cannot disprove existence. mocks.readRuntimeFileContent.mockRejectedValue(new Error('connection not ready')) mocks.pathExists.mockRejectedValue(new Error('connection not ready')) const store = createTestStore() @@ -231,7 +226,6 @@ describe('attachRestoredTabConflictScan', () => { await vi.advanceTimersByTimeAsync(10) await vi.advanceTimersByTimeAsync(2_100) const tab = store.getState().openFiles[0] - expect(tab?.pendingDiskBaselineVerification).toBe(true) expect(tab?.externalMutation).toBeUndefined() expect(mocks.readRuntimeFileContent.mock.calls.length).toBeGreaterThan(1) } finally { @@ -263,7 +257,6 @@ describe('attachRestoredTabConflictScan', () => { expect(mocks.readRuntimeFileContent).toHaveBeenLastCalledWith( expect.objectContaining({ filePath: '/Users/me/notes.txt', access: { kind: 'user-file' } }) ) - expect(store.getState().openFiles[0]?.pendingDiskBaselineVerification).toBeUndefined() } finally { detach() } @@ -304,7 +297,6 @@ describe('attachRestoredTabConflictScan', () => { await vi.advanceTimersByTimeAsync(10) expect(mocks.readRuntimeFileContent).not.toHaveBeenCalled() expect(mocks.pathExists).not.toHaveBeenCalled() - expect(store.getState().openFiles[0]?.pendingDiskBaselineVerification).toBe(true) } finally { detach() } @@ -339,9 +331,6 @@ describe('attachRestoredTabConflictScan', () => { await vi.advanceTimersByTimeAsync(10) } expect(mocks.readRuntimeFileContent).toHaveBeenCalledTimes(6) - for (const file of store.getState().openFiles) { - expect(file.pendingDiskBaselineVerification).toBeUndefined() - } } finally { detach() } @@ -413,7 +402,6 @@ describe('attachRestoredTabConflictScan', () => { const tab = store.getState().openFiles.find((f) => f.id === '/repo/file-3.ts') expect(tab?.externalMutation).toBeUndefined() - expect(tab?.pendingDiskBaselineVerification).toBeUndefined() } finally { detach() } diff --git a/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.ts b/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.ts index 8b5943f03b6..beb58bd7883 100644 --- a/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.ts +++ b/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.ts @@ -1,4 +1,4 @@ -// Why: a restored dirty tab's changed-on-disk mark doesn't survive restarts (issue #7265), so re-derive the conflict from disk — else a resumed autosave silently overwrites newer content; autosave stays suspended (pendingDiskBaselineVerification) until verification resolves. +// Why: a restored dirty tab's changed-on-disk mark doesn't survive restarts (issue #7265), so re-derive it from disk to show the banner early; the guarded save is what keeps autosave off newer content. import type { StoreApi } from 'zustand' import type { AppState } from '@/store' import type { OpenFile } from '@/store/slices/editor' @@ -13,54 +13,32 @@ import { canAutoSaveOpenFile } from './editor-autosave' import { getDiskBaselineSignature } from '../../../../shared/disk-baseline-signature' import { markFileChangedOnDisk } from './editor-changed-on-disk-mark' import { runtimePathsExist } from '@/runtime/runtime-path-existence-batch' +import { getRecentSelfWrite } from './editor-self-write-registry' type AppStoreApi = Pick, 'getState' | 'subscribe'> -// Retry fast then slow: reads fail while SSH/runtime transport is still coming up; giving up would strand autosave suspension. +// Retry fast then slow: reads fail while SSH/runtime transport is still coming up. const VERIFY_RETRY_MS = 2_000 const VERIFY_SLOW_RETRY_MS = 15_000 const VERIFY_FAST_ATTEMPTS = 30 // Cap concurrent reads: many restored dirty tabs would otherwise fire N concurrent 15s RPCs competing with startup connection recovery. const MAX_CONCURRENT_VERIFY_READS = 3 -// Why: an unreachable host must not suspend autosave forever; give up with the conflict banner instead. export const MAX_VERIFY_ATTEMPTS = VERIFY_FAST_ATTEMPTS + 10 -// Why: a disk check requested while one is in flight must not be answered by the older read. -// A global sequence, so dropping a closed tab's entry can't let a reopened tab's request reuse an old read's number. -const verifyRequestGenerationByFileId = new Map() -let verifyRequestGenerationSeq = 0 - -/** Asks for a fresh disk check of a tab; a read already in flight is followed by one more. */ -export function requestRestoredTabReverification(fileId: string): void { - verifyRequestGenerationSeq += 1 - verifyRequestGenerationByFileId.set(fileId, verifyRequestGenerationSeq) -} - -export function getRestoredTabReverificationRequestCountForTest(): number { - return verifyRequestGenerationByFileId.size -} - -function forgetClosedTabReverificationRequests(openFiles: readonly OpenFile[]): void { - if (verifyRequestGenerationByFileId.size === 0) { - return - } - const openIds = new Set(openFiles.map((file) => file.id)) - for (const fileId of verifyRequestGenerationByFileId.keys()) { - if (!openIds.has(fileId)) { - verifyRequestGenerationByFileId.delete(fileId) - } - } -} - -function verifyRequestGeneration(fileId: string): number { - return verifyRequestGenerationByFileId.get(fileId) ?? 0 +function needsDiskCheck(file: OpenFile): boolean { + return ( + file.isDirty && + file.lastKnownDiskSignature !== undefined && + file.externalMutation !== 'changed' && + canAutoSaveOpenFile(file) + ) } export function attachRestoredTabConflictScan(store: AppStoreApi): () => void { - // Dedupes queued + in-flight verifications; the store's pending flag is the durable "needs verification" signal. + // Why: only a tab that arrives dirty (a restore, or a dirty tab's rekey) is checked; edits made here start in sync. + const seenFileIds = new Set() const inFlightFileIds = new Set() const attemptsByFileId = new Map() - const supersededReadsByFileId = new Map() const retryTimers = new Set>() // Queue ids, not OpenFile snapshots: a snapshot can go stale (close/reopen or same-path save) while it waits for a slot. const verifyQueue: string[] = [] @@ -125,30 +103,19 @@ export function attachRestoredTabConflictScan(store: AppStoreApi): () => void { } } - // Why: a newer check was requested mid-read; this answer may predate it, so keep the flag and read once more. - const rereadIfSuperseded = (fileId: string, requestGeneration: number): boolean => { - if (verifyRequestGeneration(fileId) === requestGeneration) { - return false - } - // Why back off, not cap: a superseded answer may predate the newer check, so it never resolves; churn only delays the fresh read. - const superseded = (supersededReadsByFileId.get(fileId) ?? 0) + 1 - supersededReadsByFileId.set(fileId, superseded) - const timer = setTimeout( - () => { - retryTimers.delete(timer) - inFlightFileIds.delete(fileId) - scan() - }, - Math.min((superseded - 1) * VERIFY_RETRY_MS, VERIFY_SLOW_RETRY_MS) - ) - retryTimers.add(timer) - return true + // Why: a save (or reload) since the read re-baselined the tab, so the read proves nothing about it. + const unchangedSinceRead = (file: OpenFile): OpenFile | null => { + const liveFile = store.getState().openFiles.find((f) => f.id === file.id) + return liveFile && + needsDiskCheck(liveFile) && + liveFile.lastKnownDiskSignature === file.lastKnownDiskSignature + ? liveFile + : null } const verify = async (file: OpenFile): Promise => { // ids are file paths: a stray leftover marker would skip a reopened same-path tab, so only a scheduled retry keeps it set. let retryScheduled = false - const requestGeneration = verifyRequestGeneration(file.id) try { const state = store.getState() const result = await readRuntimeFileContent({ @@ -160,56 +127,31 @@ export function attachRestoredTabConflictScan(store: AppStoreApi): () => void { expectedExternalSshTargetId: file.externalSshTargetId, access: editorTabFileAccess(state, file) }) - if (disposed) { - return - } - const liveFile = store.getState().openFiles.find((f) => f.id === file.id) - if (!liveFile) { - return - } - if (rereadIfSuperseded(file.id, requestGeneration)) { - retryScheduled = true - return - } attemptsByFileId.delete(file.id) - supersededReadsByFileId.delete(file.id) - // Verification resolved: lift autosave suspension regardless of outcome; wasPending flags a save that already re-baselined. - const wasPending = liveFile.pendingDiskBaselineVerification === true - store.getState().clearPendingDiskBaselineVerification(file.id) + const liveFile = disposed ? null : unchangedSinceRead(file) if ( - !wasPending || + !liveFile || result.isBinary || - !liveFile.isDirty || - liveFile.externalMutation === 'changed' + getDiskBaselineSignature(result.content) === file.lastKnownDiskSignature || + // Why: disk already holds the draft, so the next save goes through clean. + result.content === store.getState().editorDrafts[file.id] || + // Why: the tab's own save can land before it rebaselines. + getRecentSelfWrite(file.filePath, file.runtimeEnvironmentId)?.content === result.content ) { return } - if (getDiskBaselineSignature(result.content) !== file.lastKnownDiskSignature) { - markFileChangedOnDisk(store.getState(), liveFile, { - connectionId: getConnectionIdForFile(file.worktreeId, file.filePath) ?? undefined, - origin: 'restore' - }) - } + markFileChangedOnDisk(store.getState(), liveFile, { + connectionId: getConnectionIdForFile(file.worktreeId, file.filePath) ?? undefined, + origin: 'restore' + }) } catch { if (disposed) { return } if (await probeFileMissing(file)) { - if (disposed) { - return - } - if (rereadIfSuperseded(file.id, requestGeneration)) { - retryScheduled = true - return - } - // Definitive not-found = resolved: no newer disk content for a save to clobber, so mark deleted instead of retrying forever. - const liveFile = store.getState().openFiles.find((f) => f.id === file.id) - if (!liveFile) { - return - } - const wasPending = liveFile.pendingDiskBaselineVerification === true - store.getState().clearPendingDiskBaselineVerification(file.id) - if (wasPending && liveFile.isDirty && liveFile.externalMutation !== 'changed') { + attemptsByFileId.delete(file.id) + const liveFile = disposed ? null : unchangedSinceRead(file) + if (liveFile && liveFile.externalMutation !== 'deleted') { store.getState().setExternalMutation(file.id, 'deleted') } return @@ -219,16 +161,17 @@ export function attachRestoredTabConflictScan(store: AppStoreApi): () => void { } const attempts = (attemptsByFileId.get(file.id) ?? 0) + 1 attemptsByFileId.set(file.id, attempts) + // Why stop silently: the banner is only early warning; the guarded save still refuses newer disk. if (attempts >= MAX_VERIFY_ATTEMPTS) { - giveUp(file) + attemptsByFileId.delete(file.id) return } retryScheduled = true const timer = setTimeout( () => { retryTimers.delete(timer) - inFlightFileIds.delete(file.id) - scan() + verifyQueue.push(file.id) + pumpVerifyQueue() }, attempts < VERIFY_FAST_ATTEMPTS ? VERIFY_RETRY_MS : VERIFY_SLOW_RETRY_MS ) @@ -240,37 +183,14 @@ export function attachRestoredTabConflictScan(store: AppStoreApi): () => void { } } - // Why: still unverifiable after every retry; resume nothing silently — the banner makes the user choose. - const giveUp = (file: OpenFile): void => { - attemptsByFileId.delete(file.id) - supersededReadsByFileId.delete(file.id) - const liveFile = store.getState().openFiles.find((f) => f.id === file.id) - if (!liveFile?.pendingDiskBaselineVerification) { - return - } - store.getState().clearPendingDiskBaselineVerification(file.id) - if (liveFile.isDirty && liveFile.externalMutation !== 'changed') { - markFileChangedOnDisk(store.getState(), liveFile, { - connectionId: getConnectionIdForFile(file.worktreeId, file.filePath) ?? undefined, - origin: 'restore' - }) - } - } - const pumpVerifyQueue = (): void => { while (!disposed && activeVerifyReads < MAX_CONCURRENT_VERIFY_READS && verifyQueue.length > 0) { const fileId = verifyQueue.shift()! - // Re-read live file: a queued id may now be a reopened/saved same-path tab; re-validate before a disk read, and skipping frees the dedupe marker so a later scan can re-queue it. + // Re-read live file: a queued id may now be a reopened/saved same-path tab; skipping frees the dedupe marker. const file = store.getState().openFiles.find((f) => f.id === fileId) - if ( - !file || - !file.pendingDiskBaselineVerification || - !file.isDirty || - !file.lastKnownDiskSignature || - file.externalMutation === 'changed' || - !canAutoSaveOpenFile(file) - ) { + if (!file || !needsDiskCheck(file)) { inFlightFileIds.delete(fileId) + attemptsByFileId.delete(fileId) continue } activeVerifyReads += 1 @@ -287,19 +207,22 @@ export function attachRestoredTabConflictScan(store: AppStoreApi): () => void { if (disposed) { return } - for (const file of store.getState().openFiles) { - if ( - !file.pendingDiskBaselineVerification || - !file.isDirty || - !file.lastKnownDiskSignature || - file.externalMutation === 'changed' || - !canAutoSaveOpenFile(file) || - inFlightFileIds.has(file.id) - ) { + const openFiles = store.getState().openFiles + const openIds = new Set(openFiles.map((file) => file.id)) + for (const fileId of seenFileIds) { + if (!openIds.has(fileId)) { + seenFileIds.delete(fileId) + } + } + for (const file of openFiles) { + if (seenFileIds.has(file.id)) { continue } - inFlightFileIds.add(file.id) - verifyQueue.push(file.id) + seenFileIds.add(file.id) + if (needsDiskCheck(file) && !inFlightFileIds.has(file.id)) { + inFlightFileIds.add(file.id) + verifyQueue.push(file.id) + } } pumpVerifyQueue() } @@ -311,7 +234,6 @@ export function attachRestoredTabConflictScan(store: AppStoreApi): () => void { return } previousOpenFiles = nextOpenFiles - forgetClosedTabReverificationRequests(nextOpenFiles) scan() }) scan() diff --git a/src/renderer/src/components/editor/editor-save-disk-precondition.races.test.ts b/src/renderer/src/components/editor/editor-save-disk-precondition.races.test.ts index 8882b6a5b1a..7612a7cbd4b 100644 --- a/src/renderer/src/components/editor/editor-save-disk-precondition.races.test.ts +++ b/src/renderer/src/components/editor/editor-save-disk-precondition.races.test.ts @@ -251,7 +251,7 @@ describe('guarded editor saves under races', () => { gate.resolve() await saves - // Each save is guarded against the advanced baseline and rewrites identical bytes. + // The second guarded save completes without rewriting the identical bytes. expect(disk.writeFile).toHaveBeenCalledTimes(2) expect(disk.files.get(GUARDED_PATH)).toBe('X') expect([...shown.values()]).toEqual(['X', 'X']) diff --git a/src/renderer/src/components/editor/editor-save-disk-precondition.test.ts b/src/renderer/src/components/editor/editor-save-disk-precondition.test.ts index 2a465d05d65..a15c1cab608 100644 --- a/src/renderer/src/components/editor/editor-save-disk-precondition.test.ts +++ b/src/renderer/src/components/editor/editor-save-disk-precondition.test.ts @@ -1,7 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { requestEditorFileSave } from './editor-autosave' import { attachEditorAutosaveController } from './editor-autosave-controller' -import { __clearSelfWriteRegistryForTests } from './editor-self-write-registry' +import { __clearSelfWriteRegistryForTests, hasRecentSelfWrite } from './editor-self-write-registry' import { createEditorStore } from './editor-autosave-controller-test-fixture' import { keepTabEditsOverExternalChange, @@ -11,7 +11,10 @@ import { getDiskBaselineSignature, getWrittenContentDiskSignature } from '../../../../shared/disk-baseline-signature' -import { FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY } from '../../../../shared/file-write-disk-precondition' +import { + FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, + FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY +} from '../../../../shared/file-write-disk-precondition' import { GUARDED_PATH, openGuardedFile, stubGuardedDisk } from './editor-save-guarded-disk-fixture' import { createCompatibleRuntimeStatusResponse, @@ -68,6 +71,29 @@ describe('editor saves guarded by the disk precondition', () => { __clearSelfWriteRegistryForTests() }) + it('cleans a tab whose disk already holds the edit, without retaining a self-write stamp', async () => { + const disk = stubGuardedDisk({ [PATH]: 'user edit' }) + const store = createEditorStore() + openFile(store, { baseline: 'original', draft: 'user edit' }) + const cleanup = attachEditorAutosaveController(store) + try { + await requestEditorFileSave({ fileId: PATH }) + + await expect(disk.writeFile.mock.results[0]?.value).resolves.toEqual({ + ok: true, + unchanged: true + }) + expect(store.getState().openFiles[0]).toMatchObject({ + isDirty: false, + lastKnownDiskSignature: getWrittenContentDiskSignature('user edit') + }) + expect(store.getState().editorDrafts[PATH]).toBeUndefined() + expect(hasRecentSelfWrite(PATH)).toBe(false) + } finally { + cleanup() + } + }) + it('blocks an autosave over an unnoticed agent write and raises the banner', async () => { const disk = stubGuardedDisk({ [PATH]: 'original' }) const store = createEditorStore() @@ -84,7 +110,8 @@ describe('editor saves guarded by the disk precondition', () => { content: 'user edit', diskPrecondition: { expectedSignature: getDiskBaselineSignature('original'), - allowMissing: false + allowMissing: false, + requireLosslessUtf8: true } }) ) @@ -152,9 +179,12 @@ describe('editor saves guarded by the disk precondition', () => { store.getState().markFileDirty(PATH, true) await vi.advanceTimersByTimeAsync(1500) + // Why: this host only sees decoded text, so a U+FFFD on disk pauses autosave conservatively. expect(disk.writeFile).toHaveBeenCalledTimes(2) - expect(disk.files.get(PATH)).toBe('second edit') + expect(disk.files.get(PATH)).toBe('a�b') expect(store.getState().openFiles[0]?.externalMutation).toBeUndefined() + await requestEditorFileSave({ fileId: PATH }) + expect(disk.files.get(PATH)).toBe('second edit') } finally { cleanup() } @@ -489,7 +519,9 @@ describe('editor saves through a remote host without the guarded write (client c throw new Error('fixture status must succeed') } const capabilities = (status.result.capabilities ?? []).filter( - (capability) => capability !== FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY + (capability) => + capability !== FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY && + capability !== FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY ) const olderStatus = { ...status, result: { ...status.result, capabilities } } const meta = { _meta: { runtimeId: 'runtime-env-1' } } diff --git a/src/renderer/src/components/editor/editor-save-disk-precondition.ts b/src/renderer/src/components/editor/editor-save-disk-precondition.ts index 2738f6aa1ce..ba586b3fbd3 100644 --- a/src/renderer/src/components/editor/editor-save-disk-precondition.ts +++ b/src/renderer/src/components/editor/editor-save-disk-precondition.ts @@ -71,7 +71,7 @@ export function editorModelSiblingTabs(state: AppState, file: OpenFile): OpenFil * write gets the writer's compatibility fallback: the client reads and compares, then writes. */ export type EditorSavePrecondition = - | { kind: 'guarded'; signature: string; allowMissing: boolean } + | { kind: 'guarded'; signature: string; allowMissing: boolean; requireLosslessUtf8?: boolean } | { kind: 'explicit-overwrite' } | { kind: 'blocked'; reason: 'no-baseline' } @@ -128,7 +128,12 @@ export function editorSavePrecondition( return { kind: 'blocked', reason: 'no-baseline' } } // Why: autosave must not resurrect a file deleted behind the editor's back; explicit saves may. - return { kind: 'guarded', signature: baseline, allowMissing: trigger !== 'autosave' } + return { + kind: 'guarded', + signature: baseline, + allowMissing: trigger !== 'autosave', + requireLosslessUtf8: trigger === 'autosave' || trigger === 'restart' + } } /** The only file write editor save code makes. */ @@ -146,7 +151,11 @@ export function writeEditorSave( content, access, write.kind === 'guarded' - ? { expectedSignature: write.signature, allowMissing: write.allowMissing } + ? { + expectedSignature: write.signature, + allowMissing: write.allowMissing, + ...(write.requireLosslessUtf8 ? { requireLosslessUtf8: true } : {}) + } : undefined, beforeSend ) diff --git a/src/renderer/src/components/editor/editor-save-guarded-disk-fixture.ts b/src/renderer/src/components/editor/editor-save-guarded-disk-fixture.ts index 4987a2fd474..4a2864fafd2 100644 --- a/src/renderer/src/components/editor/editor-save-guarded-disk-fixture.ts +++ b/src/renderer/src/components/editor/editor-save-guarded-disk-fixture.ts @@ -28,8 +28,12 @@ export function stubGuardedDisk( if (diskPrecondition) { const diskConflict = compareDiskWritePrecondition( diskPrecondition, - files.get(filePath) ?? null + files.get(filePath) ?? null, + content ) + if (diskConflict === 'identical') { + return { ok: true, unchanged: true } + } if (diskConflict) { return { ok: false, diskConflict } } diff --git a/src/renderer/src/components/editor/editor-save-large-paste.test.ts b/src/renderer/src/components/editor/editor-save-large-paste.test.ts index e08dcad89bc..49a17d7988c 100644 --- a/src/renderer/src/components/editor/editor-save-large-paste.test.ts +++ b/src/renderer/src/components/editor/editor-save-large-paste.test.ts @@ -58,8 +58,15 @@ function pasteIntoGuardedTab(pasted: string) { const files = new Map([[PATH, 'base']]) const writeFile = vi.fn(async (args: GuardedWriteArgs) => { const conflict = args.diskPrecondition - ? compareDiskWritePrecondition(args.diskPrecondition, files.get(args.filePath) ?? null) + ? compareDiskWritePrecondition( + args.diskPrecondition, + files.get(args.filePath) ?? null, + args.content + ) : null + if (conflict === 'identical') { + return { ok: true, unchanged: true } + } if (conflict) { return { ok: false, diskConflict: conflict } } @@ -118,7 +125,11 @@ it('holds autosave while a chunked paste is in flight, then saves the pasted tex expect(files.get(PATH)).toBe(`${pasted}base`) expect(writeFile).toHaveBeenCalledWith( expect.objectContaining({ - diskPrecondition: { expectedSignature: getDiskBaselineSignature('base'), allowMissing: false } + diskPrecondition: { + expectedSignature: getDiskBaselineSignature('base'), + allowMissing: false, + requireLosslessUtf8: true + } }) ) queue.dispose() diff --git a/src/renderer/src/components/editor/editor-save-lossless-baseline.test.ts b/src/renderer/src/components/editor/editor-save-lossless-baseline.test.ts new file mode 100644 index 00000000000..8a66f202d0b --- /dev/null +++ b/src/renderer/src/components/editor/editor-save-lossless-baseline.test.ts @@ -0,0 +1,100 @@ +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import { toast } from 'sonner' +import { createEditorStore } from './editor-autosave-controller-test-fixture' +import { createEditorSaveQueue } from './editor-save-queue' +import { GUARDED_PATH, openGuardedFile, stubGuardedDisk } from './editor-save-guarded-disk-fixture' +import { isAutosaveSuspendedForFile } from './editor-autosave' +import { __clearNonUtf8AutosavePausesForTests } from './editor-non-utf8-autosave-pause' + +vi.mock('sonner', () => ({ toast: { warning: vi.fn() } })) + +beforeEach(() => vi.useFakeTimers()) +afterEach(() => { + vi.useRealTimers() + vi.unstubAllGlobals() + vi.mocked(toast.warning).mockClear() + __clearNonUtf8AutosavePausesForTests() +}) + +it.each(['autosave', 'restart', 'user', 'overwrite'] as const)( + 'requires a lossless baseline only for automatic writes (%s)', + async (trigger) => { + const decoded = Buffer.from([0x61, 0xfe]).toString('utf8') + const newer = Buffer.from([0x61, 0xff]).toString('utf8') + const disk = stubGuardedDisk({ [GUARDED_PATH]: newer }) + const store = createEditorStore() + openGuardedFile(store, { baseline: decoded, draft: decoded }) + const queue = createEditorSaveQueue(store) + try { + const automatic = trigger === 'autosave' || trigger === 'restart' + expect(await queue.queueSave(store.getState().openFiles[0], decoded, trigger)).toBe( + automatic ? 'failed' : 'written' + ) + expect(store.getState().openFiles[0]?.isDirty).toBe(automatic) + // Why: a non-UTF-8 refusal is not a changed-on-disk conflict. + expect(store.getState().openFiles[0]?.externalMutation).toBeUndefined() + expect(isAutosaveSuspendedForFile(store.getState().openFiles[0]!)).toBe(automatic) + expect(toast.warning).toHaveBeenCalledTimes(automatic ? 1 : 0) + expect(disk.files.get(GUARDED_PATH)).toBe(decoded) + } finally { + queue.dispose() + } + } +) + +it('pauses autosave once for a non-UTF-8 file until an explicit save converts it', async () => { + const latin1 = Buffer.from([0x63, 0x61, 0x66, 0xe9]).toString('utf8') + const disk = stubGuardedDisk({ [GUARDED_PATH]: latin1 }) + const store = createEditorStore() + openGuardedFile(store, { baseline: latin1, draft: `${latin1}!` }) + const queue = createEditorSaveQueue(store) + try { + const file = (): NonNullable['openFiles'][0]> => + store.getState().openFiles[0]! + expect(await queue.queueSave(file(), `${latin1}!`, 'autosave')).toBe('failed') + // A second automatic attempt is gated before it reaches the host, with no second notice. + expect(await queue.queueSave(file(), `${latin1}!`, 'autosave')).toBe('failed') + expect(disk.writeFile).toHaveBeenCalledTimes(1) + expect(toast.warning).toHaveBeenCalledTimes(1) + expect(vi.mocked(toast.warning).mock.calls[0]?.[0]).toContain("isn't UTF-8 text") + + expect(await queue.queueSave(file(), `${latin1}!`, 'user')).toBe('written') + expect(disk.files.get(GUARDED_PATH)).toBe(`${latin1}!`) + expect(isAutosaveSuspendedForFile(file())).toBe(false) + } finally { + queue.dispose() + } +}) + +it.each(['close', 'reload'] as const)( + 'lifts the pause on %s, so the next edit is asked about again', + async (ending) => { + const latin1 = Buffer.from([0x63, 0x61, 0x66, 0xe9]).toString('utf8') + const disk = stubGuardedDisk({ [GUARDED_PATH]: latin1 }) + const store = createEditorStore() + openGuardedFile(store, { baseline: latin1, draft: `${latin1}!` }) + const queue = createEditorSaveQueue(store) + try { + expect(await queue.queueSave(store.getState().openFiles[0]!, `${latin1}!`, 'autosave')).toBe( + 'failed' + ) + if (ending === 'close') { + // Why: the minimal fixture store lacks the layout state closeFile walks. + store.setState({ openFiles: [], editorBuffers: {}, editorDrafts: {} }) + } else { + store.getState().markFileDirty(GUARDED_PATH, false) + } + queue.syncAutoSave() + openGuardedFile(store, { baseline: latin1, draft: `${latin1}?` }) + queue.syncAutoSave() + expect(isAutosaveSuspendedForFile(store.getState().openFiles[0]!)).toBe(false) + expect(await queue.queueSave(store.getState().openFiles[0]!, `${latin1}?`, 'autosave')).toBe( + 'failed' + ) + expect(disk.writeFile).toHaveBeenCalledTimes(2) + expect(toast.warning).toHaveBeenCalledTimes(2) + } finally { + queue.dispose() + } + } +) diff --git a/src/renderer/src/components/editor/editor-save-queue.ts b/src/renderer/src/components/editor/editor-save-queue.ts index d7f76d2a2fa..22e901adf75 100644 --- a/src/renderer/src/components/editor/editor-save-queue.ts +++ b/src/renderer/src/components/editor/editor-save-queue.ts @@ -23,6 +23,7 @@ import { trackExternalChangeConflictAction } from './editor-external-change-tele import { editorTabFileAccess } from '@/lib/local-file-access' import { isEditorLargePasteInFlight } from './editor-large-paste-in-flight' import { + markEditorSaveRefusal, rebaselineAfterEditorWrite, setModelExternalMutation, waitForModelBaselineRestamps @@ -40,6 +41,7 @@ import { editorSaveWriterView } from './editor-save-writer-view' import { createEditorSaveSlots, type EditorSaveRequest } from './editor-save-slots' import { EditorSaveDiscardedError, editorDiscardEpoch } from './editor-discard-epoch' import { selectWorktreesByRepo } from '@/store/workspace-record-selectors' +import { releaseNonUtf8PausesWithoutDirtyTab } from './editor-non-utf8-autosave-pause' export type AppStoreApi = Pick, 'getState' | 'subscribe'> @@ -175,15 +177,13 @@ export function createEditorSaveQueue(store: AppStoreApi): EditorSaveQueue { } if (!writeResult.ok) { clearSelfWrite(liveFile.filePath, liveFile.runtimeEnvironmentId) - // Why: the banner (or deleted tombstone) explains the refusal and pauses autosave. - setModelExternalMutation( - store.getState(), - fileId, - writeResult.diskConflict === 'missing' ? 'deleted' : 'changed' - ) - return 'blocked-stale' + markEditorSaveRefusal(store.getState(), liveFile, writeResult.diskConflict) + return writeResult.diskConflict === 'not-utf8' ? 'failed' : 'blocked-stale' } + if (writeResult.unchanged) { + clearSelfWrite(liveFile.filePath, liveFile.runtimeEnvironmentId) + } rebaselineAfterEditorWrite(store.getState(), fileId, contentToSave) const nextState = store.getState() const currentDraft = nextState.editorDrafts[fileId] @@ -194,8 +194,6 @@ export function createEditorSaveQueue(store: AppStoreApi): EditorSaveQueue { if (!stillDirty) { nextState.clearEditorDraft(fileId) } - // Why: disk now holds contentToSave, so a pending restore verification is moot. - nextState.clearPendingDiskBaselineVerification(fileId) // Why: the write made disk match the buffer, so clear any now-stale changed-on-disk conflict. const savedFile = nextState.openFiles.find((openFile) => openFile.id === fileId) if (savedFile?.externalMutation === 'changed') { @@ -259,6 +257,7 @@ export function createEditorSaveQueue(store: AppStoreApi): EditorSaveQueue { const syncAutoSave = (): void => { const state = store.getState() holds.flagOverdue(state) + releaseNonUtf8PausesWithoutDirtyTab(state.openFiles) const openFilesByKey = new Map( state.openFiles.map((file) => [editorBufferIdOfView(state, file.id), file]) ) diff --git a/src/renderer/src/components/editor/editor-save-rebaseline.ts b/src/renderer/src/components/editor/editor-save-rebaseline.ts index 9f00939bf46..971bba97550 100644 --- a/src/renderer/src/components/editor/editor-save-rebaseline.ts +++ b/src/renderer/src/components/editor/editor-save-rebaseline.ts @@ -1,7 +1,12 @@ import type { AppState } from '@/store' import type { OpenFile } from '@/store/slices/editor' import { getWrittenContentDiskSignature } from '../../../../shared/disk-baseline-signature' +import type { FileWriteDiskConflict } from '../../../../shared/file-write-disk-precondition' import { notifyEditorFileCacheStale } from './editor-autosave' +import { + pauseAutosaveForNonUtf8File, + resumeAutosaveForNonUtf8File +} from './editor-non-utf8-autosave-pause' import { editorModelSiblingTabs, noteEditorBaselineWrite, @@ -21,6 +26,19 @@ export function setModelExternalMutation( } } +/** The banner, deleted tombstone or non-UTF-8 notice explains a refused save and pauses autosave. */ +export function markEditorSaveRefusal( + state: AppState, + file: OpenFile, + diskConflict: FileWriteDiskConflict +): void { + if (diskConflict === 'not-utf8') { + pauseAutosaveForNonUtf8File(file) + return + } + setModelExternalMutation(state, file.id, diskConflict === 'missing' ? 'deleted' : 'changed') +} + /** A Keep in any tab of this text restamps the baseline the save uses, so wait for all of them. */ export function waitForModelBaselineRestamps(state: AppState, file: OpenFile): Promise { const tab = state.openFiles.find((openFile) => openFile.id === file.id) @@ -37,6 +55,10 @@ export function rebaselineAfterEditorWrite(state: AppState, fileId: string, cont const moved = setEditorDiskBaseline(state, fileId, getWrittenContentDiskSignature(content)) noteEditorBaselineWrite(fileId) const file = state.openFiles.find((openFile) => openFile.id === fileId) + if (file) { + // Why: disk now holds this UTF-8 write, so a non-UTF-8 pause no longer applies. + resumeAutosaveForNonUtf8File(file) + } // Why: while any tab still has edits beyond this write, the model holds them; a sibling must // show that, not disk. The saving tab's own written draft is about to be settled. const draft = state.editorDrafts[fileId] diff --git a/src/renderer/src/components/editor/editor-save-shared-model-dirty.test.ts b/src/renderer/src/components/editor/editor-save-shared-model-dirty.test.ts index d5272cd6ed5..9c199f78470 100644 --- a/src/renderer/src/components/editor/editor-save-shared-model-dirty.test.ts +++ b/src/renderer/src/components/editor/editor-save-shared-model-dirty.test.ts @@ -115,7 +115,6 @@ describe('a clean tab of a dirty shared model', () => { it('after a restart, a file changed while closed banners every tab of the model', async () => { const { store, a, b, tab } = openSharedTabsWithDirtySibling() - store.getState().setPendingDiskBaselineVerification(b.id, true) mocks.readRuntimeFileContent.mockResolvedValue({ content: 'E', isBinary: false }) const detach = attachRestoredTabConflictScan(store) diff --git a/src/renderer/src/hooks/editor-external-watch-dirty-resync.test.ts b/src/renderer/src/hooks/editor-external-watch-dirty-resync.test.ts new file mode 100644 index 00000000000..adf1518b163 --- /dev/null +++ b/src/renderer/src/hooks/editor-external-watch-dirty-resync.test.ts @@ -0,0 +1,84 @@ +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import { createEditorStore } from '@/components/editor/editor-autosave-controller-test-fixture' +import { openGuardedFile, GUARDED_PATH } from '@/components/editor/editor-save-guarded-disk-fixture' +import { createGlobalSettingsFixture } from '../../../shared/global-settings-test-fixture' +import { + __clearSelfWriteRegistryForTests, + recordSelfWrite +} from '@/components/editor/editor-self-write-registry' + +const storeRef = vi.hoisted((): { current: ReturnType | null } => ({ + current: null +})) +vi.mock('@/store', () => ({ useAppStore: { getState: () => storeRef.current?.getState() } })) + +import { reconcileEditorTabsAfterOverflow } from './editor-external-watch-overflow' + +beforeEach(() => vi.useFakeTimers()) +afterEach(() => { + __clearSelfWriteRegistryForTests() + vi.useRealTimers() + vi.unstubAllGlobals() +}) + +it.each(['original', 'external rewrite'])( + 'compares a dirty tab after an unwatched period with autosave off (%s)', + async (content) => { + const store = createEditorStore() + storeRef.current = store + store.setState({ settings: createGlobalSettingsFixture({ editorAutoSave: false }) }) + openGuardedFile(store, { baseline: 'original', draft: 'my edits' }) + let finishRead: () => void = () => undefined + const readFile = vi.fn( + () => + new Promise((resolve) => { + finishRead = () => resolve({ content, isBinary: false }) + }) + ) + const writeFile = vi.fn() + vi.stubGlobal('window', { api: { fs: { readFile, writeFile } } }) + + reconcileEditorTabsAfterOverflow({ worktreeId: 'wt-1', worktreePath: '/repo' }, true) + expect(readFile).toHaveBeenCalled() + expect(store.getState().openFiles[0]?.externalMutation).toBeUndefined() + store.getState().setEditorDraft(GUARDED_PATH, 'still typing') + finishRead() + await vi.advanceTimersByTimeAsync(0) + + expect(store.getState().openFiles[0]?.externalMutation).toBe( + content === 'original' ? undefined : 'changed' + ) + expect(store.getState().openFiles[0]?.isDirty).toBe(true) + expect(store.getState().editorDrafts[GUARDED_PATH]).toBe('still typing') + expect(writeFile).not.toHaveBeenCalled() + } +) + +it('leaves a dirty tab unmarked when its resync read fails', async () => { + const store = createEditorStore() + storeRef.current = store + openGuardedFile(store, { baseline: 'original', draft: 'my edits' }) + const readFile = vi.fn(() => Promise.reject(new Error('connection lost'))) + vi.stubGlobal('window', { api: { fs: { readFile } } }) + + reconcileEditorTabsAfterOverflow({ worktreeId: 'wt-1', worktreePath: '/repo' }, true) + await vi.advanceTimersByTimeAsync(0) + + expect(readFile).toHaveBeenCalled() + expect(store.getState().openFiles[0]?.externalMutation).toBeUndefined() +}) + +it("treats the tab's own just-written save as an echo, not an external change", async () => { + const store = createEditorStore() + storeRef.current = store + openGuardedFile(store, { baseline: 'original', draft: 'my edits' }) + recordSelfWrite(GUARDED_PATH, 'saved edits') + vi.stubGlobal('window', { + api: { fs: { readFile: vi.fn(async () => ({ content: 'saved edits', isBinary: false })) } } + }) + + reconcileEditorTabsAfterOverflow({ worktreeId: 'wt-1', worktreePath: '/repo' }, true) + await vi.advanceTimersByTimeAsync(0) + + expect(store.getState().openFiles[0]?.externalMutation).toBeUndefined() +}) diff --git a/src/renderer/src/hooks/editor-external-watch-disk-verification.ts b/src/renderer/src/hooks/editor-external-watch-disk-verification.ts index ca402b5b88e..4904736fc6d 100644 --- a/src/renderer/src/hooks/editor-external-watch-disk-verification.ts +++ b/src/renderer/src/hooks/editor-external-watch-disk-verification.ts @@ -162,7 +162,7 @@ export function scheduleEditorChangedOnDiskMark( }) } -// Per-file generation so a newer echo-verify read supersedes an older one — overlapping reads can't clear each other's autosave gate or apply a stale verdict. +// Per-file generation so a newer echo-verify read supersedes an older one — an overlapping read can't apply a stale verdict. const liveMoveVerifyGeneration = new Map() let liveMoveVerifyCounter = 0 @@ -174,10 +174,10 @@ type LiveMoveVerifyCandidate = { operationId?: string } -// Fails closed: anything but a proven baseline match surfaces the conflict, so a real external write is never swallowed. +// Fails closed: `false` (anything but a proven match) surfaces the conflict; `null` (unread, on a resync) proves nothing. function resolveLiveMoveVerification( candidate: LiveMoveVerifyCandidate, - diskSignature: string | null, + matchesDisk: boolean | null, connectionId: string | undefined, consumeProvenance: boolean ): void { @@ -187,7 +187,6 @@ function resolveLiveMoveVerification( } liveMoveVerifyGeneration.delete(fileId) const state = useAppStore.getState() - state.setPendingLiveDiskVerification(fileId, false) const file = state.openFiles.find((candidateFile) => candidateFile.id === fileId) if ( !file || @@ -202,8 +201,7 @@ function resolveLiveMoveVerification( if (consumeProvenance) { state.clearSelfMoveEcho(fileId) } - const isMoveEcho = baseline !== undefined && diskSignature === baseline - if (!isMoveEcho) { + if (matchesDisk === false) { markFileChangedOnDisk(state, file, { connectionId, origin: 'live' }) } } @@ -228,11 +226,12 @@ export function verifyLatchedEditorMoveDestinations( ) } -// Autosave is suspended synchronously first so a write landing mid-read can't be overwritten before verification settles. +/** `resync`: a watch reconnect check, where a failed read leaves the tab to its guarded save. */ export function scheduleEditorSelfMoveEchoVerification( target: EditorExternalWatchTarget, fileIds: string[], - consumeProvenance: boolean + consumeProvenance: boolean, + resync = false ): void { if (fileIds.length === 0) { return @@ -245,7 +244,6 @@ export function scheduleEditorSelfMoveEchoVerification( } const generation = ++liveMoveVerifyCounter liveMoveVerifyGeneration.set(fileId, generation) - state.setPendingLiveDiskVerification(fileId, true) const candidate: LiveMoveVerifyCandidate = { fileId, baseline: file.lastKnownDiskSignature, @@ -262,16 +260,22 @@ export function scheduleEditorSelfMoveEchoVerification( expectedExternalSshTargetId: file.externalSshTargetId }) .then((result) => { - const diskSignature = result.isBinary ? null : getDiskBaselineSignature(result.content) + const matchesDisk = + !result.isBinary && + ((candidate.baseline !== undefined && + getDiskBaselineSignature(result.content) === candidate.baseline) || + // Why: the tab's own save can land before it rebaselines; that is not an external change. + getRecentSelfWrite(file.filePath, file.runtimeEnvironmentId)?.content === + result.content) + resolveLiveMoveVerification(candidate, matchesDisk, target.connectionId, consumeProvenance) + }) + .catch(() => resolveLiveMoveVerification( candidate, - diskSignature, + resync ? null : false, target.connectionId, consumeProvenance ) - }) - .catch(() => - resolveLiveMoveVerification(candidate, null, target.connectionId, consumeProvenance) ) } } diff --git a/src/renderer/src/hooks/editor-external-watch-overflow.test.ts b/src/renderer/src/hooks/editor-external-watch-overflow.test.ts index d1614fd5bff..12e58286510 100644 --- a/src/renderer/src/hooks/editor-external-watch-overflow.test.ts +++ b/src/renderer/src/hooks/editor-external-watch-overflow.test.ts @@ -6,6 +6,7 @@ const harness = vi.hoisted(() => { state, runtimePathsExist: vi.fn(), scheduleReload: vi.fn(), + compareDirty: vi.fn(), forgetCombinedBodies: vi.fn() } }) @@ -18,7 +19,8 @@ vi.mock('@/lib/file-owner-runtime-target', () => ({ getRuntimeTargetForFileOwner: () => ({ kind: 'local' }) })) vi.mock('./editor-external-watch-disk-verification', () => ({ - scheduleDebouncedEditorExternalReload: harness.scheduleReload + scheduleDebouncedEditorExternalReload: harness.scheduleReload, + scheduleEditorSelfMoveEchoVerification: harness.compareDirty })) vi.mock('@/components/editor/combined-diff/remember-view/combined-diff-view-cache', () => ({ forgetCombinedDiffViewBodiesForWorktree: harness.forgetCombinedBodies @@ -32,7 +34,6 @@ import { } from './editor-external-watch-overflow' const setExternalMutation = vi.fn() -const setPendingDiskBaselineVerification = vi.fn() function tab(id: string, extra: Record = {}): Record { return { @@ -47,7 +48,7 @@ function tab(id: string, extra: Record = {}): Record[]): void { - harness.state = { openFiles, setExternalMutation, setPendingDiskBaselineVerification } + harness.state = { openFiles, setExternalMutation } } const target = { worktreeId: 'wt-1', worktreePath: '/repo' } @@ -110,16 +111,22 @@ describe('reconcileEditorTabsAfterOverflow', () => { expect(setExternalMutation).not.toHaveBeenCalledWith('a.ts', 'deleted') }) - it('hands dirty tabs to the disk check instead of reloading them', () => { + it('compares dirty tabs asynchronously without reloading them', () => { setTabs(tab('dirty.ts', { isDirty: true, lastKnownDiskSignature: 'sig' })) reconcileEditorTabsAfterOverflow(target, true) - expect(setPendingDiskBaselineVerification).toHaveBeenCalledWith('dirty.ts', true) + expect(harness.compareDirty).toHaveBeenCalledWith( + { ...target, connectionId: undefined, runtimeEnvironmentId: null }, + ['dirty.ts'], + false, + true + ) + expect(setExternalMutation).not.toHaveBeenCalled() expect(harness.runtimePathsExist).not.toHaveBeenCalled() expect(harness.scheduleReload).not.toHaveBeenCalled() }) - it('a storm overflow reloads clean tabs without probing, pausing autosave or dropping combined bodies', () => { + it('a storm overflow reloads clean tabs without probing or dropping combined bodies', () => { setTabs( tab('a.md', { externalMutation: 'deleted' }), tab('dirty.ts', { isDirty: true, lastKnownDiskSignature: 'sig' }) @@ -129,8 +136,8 @@ describe('reconcileEditorTabsAfterOverflow', () => { expect(harness.scheduleReload.mock.calls).toEqual([[notification('a.md')]]) expect(setExternalMutation).toHaveBeenCalledWith('a.md', null) expect(harness.runtimePathsExist).not.toHaveBeenCalled() - expect(setPendingDiskBaselineVerification).not.toHaveBeenCalled() expect(harness.forgetCombinedBodies).not.toHaveBeenCalled() + expect(harness.compareDirty).not.toHaveBeenCalled() }) it('a storm overflow supersedes a pending resync probe', async () => { diff --git a/src/renderer/src/hooks/editor-external-watch-overflow.ts b/src/renderer/src/hooks/editor-external-watch-overflow.ts index 254a8f9ed92..7d544805af7 100644 --- a/src/renderer/src/hooks/editor-external-watch-overflow.ts +++ b/src/renderer/src/hooks/editor-external-watch-overflow.ts @@ -1,10 +1,7 @@ import { useAppStore } from '@/store' import type { OpenFile } from '@/store/slices/editor' import type { EditorExternalWatchBatchPathIndex } from '@/components/editor/editor-external-watch-path-index' -import { - canAutoSaveOpenFile, - isExternalReloadableEditorTab -} from '@/components/editor/editor-autosave' +import { isExternalReloadableEditorTab } from '@/components/editor/editor-autosave' import { forgetCombinedDiffViewBodiesForWorktree } from '@/components/editor/combined-diff/remember-view/combined-diff-view-cache' import { getRuntimeTargetForFileOwner } from '@/lib/file-owner-runtime-target' import { runtimePathsExist } from '@/runtime/runtime-path-existence-batch' @@ -12,7 +9,6 @@ import { PATH_EXISTENCE_BATCH_MAX, type PathExistenceResult } from '../../../shared/path-existence-batch' -import { requestRestoredTabReverification } from '@/components/editor/editor-restored-tab-conflict-scan' import { getLocalWindowsWslAliasOption, getOpenFileRuntimeOwner, @@ -20,6 +16,7 @@ import { } from './editor-external-watch-targets' import { scheduleDebouncedEditorExternalReload, + scheduleEditorSelfMoveEchoVerification, type EditorExternalWatchNotification } from './editor-external-watch-disk-verification' @@ -80,8 +77,8 @@ type EditTabProbe = { /** * An overflow names no paths, so every clean tab of the workspace reloads. - * A resync (the watch was down) also probes clean edit tabs for deletion and gives dirty tabs the restored-tab - * disk check; an event-storm overflow skips both so it never pauses autosave or fans out per-tab probes. + * A resync (the watch was down) also probes clean edit tabs for deletion; an event-storm overflow + * skips that so it never fans out per-tab probes. */ export function reconcileEditorTabsAfterOverflow(target: OverflowTarget, resync: boolean): void { if (resync) { @@ -98,19 +95,19 @@ export function reconcileEditorTabsAfterOverflow(target: OverflowTarget, resync: ) { continue } + // Why: compare dirty tabs asynchronously; autosave stays free to run its own guarded check. if (file.isDirty) { - if (!resync) { - continue - } - // Why: a dirty tab is never reloaded; the restored-tab scan shows the changed-on-disk banner if disk moved. - // Mirrors the scan's eligibility so it always resolves the flag. - if ( - file.lastKnownDiskSignature && - file.externalMutation !== 'changed' && - canAutoSaveOpenFile(file) - ) { - requestRestoredTabReverification(file.id) - state.setPendingDiskBaselineVerification(file.id, true) + if (resync) { + scheduleEditorSelfMoveEchoVerification( + { + ...target, + connectionId: target.connectionId, + runtimeEnvironmentId: target.runtimeEnvironmentId ?? null + }, + [file.id], + false, + true + ) } continue } diff --git a/src/renderer/src/hooks/useEditorExternalWatch-background-trim.test.tsx b/src/renderer/src/hooks/useEditorExternalWatch-background-trim.test.tsx index f04ed38ffb8..9d7ad5292bf 100644 --- a/src/renderer/src/hooks/useEditorExternalWatch-background-trim.test.tsx +++ b/src/renderer/src/hooks/useEditorExternalWatch-background-trim.test.tsx @@ -29,7 +29,7 @@ const harness = vi.hoisted(() => { vi.mock('@/store', () => ({ useAppStore: Object.assign((selector: (state: unknown) => unknown) => selector({}), { - getState: () => ({ openFiles: [], setPendingDiskBaselineVerification: vi.fn() }) + getState: () => ({ openFiles: [] }) }) })) vi.mock('./editor-external-watch-targets', () => ({ diff --git a/src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts b/src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts index 4de08af22b8..54125564e16 100644 --- a/src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts +++ b/src/renderer/src/hooks/useEditorExternalWatch-self-move.test.ts @@ -147,7 +147,6 @@ describe('self-move source suppression (in-flight)', () => { describe('self-move echo verification (content identity)', () => { const setExternalMutation = vi.fn() - const setPendingLiveDiskVerification = vi.fn() const clearSelfMoveEcho = vi.fn() const BASELINE_CONTENT = 'the file on disk\n' @@ -175,7 +174,6 @@ describe('self-move echo verification (content identity)', () => { ...createTestStore().getState(), openFiles: [file], setExternalMutation, - setPendingLiveDiskVerification, clearSelfMoveEcho }) vi.mocked(getOpenFilesForExternalFileChange).mockReturnValue([file]) @@ -205,17 +203,15 @@ describe('self-move echo verification (content identity)', () => { vi.unstubAllGlobals() }) - it('suspends autosave synchronously and suppresses when disk matches the baseline', async () => { + it('suppresses when disk matches the baseline', async () => { mockState(movedDirtyTab()) stubDiskRead({ isBinary: false, content: BASELINE_CONTENT }) const { handleFsChanged, dispose } = createExternalWatchEventHandler(findTarget) handleFsChanged(payloadFor()) - expect(setPendingLiveDiskVerification).toHaveBeenCalledWith('file-notes', true) await vi.advanceTimersByTimeAsync(100) expect(setExternalMutation).not.toHaveBeenCalledWith('file-notes', 'changed') - expect(setPendingLiveDiskVerification).toHaveBeenCalledWith('file-notes', false) expect(clearSelfMoveEcho).toHaveBeenCalledWith('file-notes') dispose() }) @@ -253,7 +249,6 @@ describe('self-move echo verification (content identity)', () => { await vi.advanceTimersByTimeAsync(100) expect(setExternalMutation).toHaveBeenCalledWith('file-notes', 'changed') - expect(setPendingLiveDiskVerification).toHaveBeenCalledWith('file-notes', false) dispose() }) @@ -284,7 +279,6 @@ describe('self-move echo verification (content identity)', () => { }) describe('verifyLatchedMoveDestinations (proactive, cross-worktree path)', () => { - const setPendingLiveDiskVerification = vi.fn() const clearSelfMoveEcho = vi.fn() const setExternalMutation = vi.fn() @@ -316,7 +310,6 @@ describe('verifyLatchedMoveDestinations (proactive, cross-worktree path)', () => vi.mocked(useAppStore.getState).mockReturnValue({ ...createTestStore().getState(), openFiles: [floatingTab], - setPendingLiveDiskVerification, clearSelfMoveEcho, setExternalMutation }) @@ -332,9 +325,8 @@ describe('verifyLatchedMoveDestinations (proactive, cross-worktree path)', () => await vi.advanceTimersByTimeAsync(50) expect(readFile).toHaveBeenCalledWith(expect.objectContaining({ filePath: FLOATING_FILE_PATH })) - // Disk == baseline -> echo -> suppress + release the gate, no false banner. + // Disk == baseline -> echo -> suppress, no false banner. expect(setExternalMutation).not.toHaveBeenCalledWith('file-x', 'changed') - 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-shared-root.test.tsx b/src/renderer/src/hooks/useEditorExternalWatch-shared-root.test.tsx index b7b6f163729..6accfc79e06 100644 --- a/src/renderer/src/hooks/useEditorExternalWatch-shared-root.test.tsx +++ b/src/renderer/src/hooks/useEditorExternalWatch-shared-root.test.tsx @@ -30,8 +30,7 @@ vi.mock('@/store', () => ({ useAppStore: Object.assign((selector: (state: unknown) => unknown) => selector({}), { getState: () => ({ openFiles: harness.openFiles, - setExternalMutation: harness.setExternalMutation, - setPendingDiskBaselineVerification: vi.fn() + setExternalMutation: harness.setExternalMutation }) }) })) diff --git a/src/renderer/src/hooks/useEditorExternalWatch-switch-back.test.tsx b/src/renderer/src/hooks/useEditorExternalWatch-switch-back.test.tsx index 95045b22b1d..4b286b53c4b 100644 --- a/src/renderer/src/hooks/useEditorExternalWatch-switch-back.test.tsx +++ b/src/renderer/src/hooks/useEditorExternalWatch-switch-back.test.tsx @@ -89,7 +89,6 @@ describe('switching back to a workspace that was not watched', () => { let previousApi: unknown let detachScan: () => void let settleWatch: () => void - let emitIpc: (payload: unknown) => void let pathExists: Mock<(args: { filePath: string }) => Promise> let reloads: string[] let fileChangeEvents: number @@ -126,6 +125,8 @@ describe('switching back to a workspace that was not watched', () => { vi.useFakeTimers({ shouldAdvanceTime: true }) store = createTestStore() harness.holder.store = store + // Why before the edits: a tab dirtied in this session is not a restored draft. + detachScan = attachRestoredTabConflictScan(store) openTab(CLEAN) openTab(DIRTY) store.getState().setEditorDraft(DIRTY, 'unsaved edit') @@ -141,10 +142,7 @@ describe('switching back to a workspace that was not watched', () => { }) ), unwatchWorktree: vi.fn().mockResolvedValue(undefined), - onFsChanged: vi.fn((listener: (payload: unknown) => void) => { - emitIpc = listener - return vi.fn() - }), + onFsChanged: vi.fn(() => vi.fn()), pathExists, pathsExist: vi.fn(async ({ filePaths }: { filePaths: string[] }) => Promise.all( @@ -157,7 +155,6 @@ describe('switching back to a workspace that was not watched', () => { fileChangeEvents = 0 window.addEventListener(ORCA_EDITOR_EXTERNAL_FILE_CHANGE_EVENT, onReload) window.addEventListener(ORCA_WORKTREE_FILE_CHANGE_EVENT, onFileChange) - detachScan = attachRestoredTabConflictScan(store) container = document.body.appendChild(document.createElement('div')) root = createRoot(container) }) @@ -180,55 +177,17 @@ describe('switching back to a workspace that was not watched', () => { await act(async () => vi.advanceTimersByTimeAsync(200)) } - it('reloads clean tabs, banners a dirty tab changed on disk, and refreshes git status/Files once', async () => { + it('reloads clean tabs, flags a changed dirty tab without pausing it, and refreshes git status/Files once', async () => { harness.readRuntimeFileContent.mockResolvedValue({ content: 'v2 agent write', isBinary: false }) await switchBack() expect(reloads).toEqual(['clean.ts']) - expect(file(DIRTY).externalMutation).toBe('changed') - expect(file(DIRTY).isDirty).toBe(true) + expect(harness.readRuntimeFileContent).toHaveBeenCalledTimes(1) + expect(file(DIRTY)).toMatchObject({ isDirty: true, externalMutation: 'changed' }) expect(file(CLEAN).externalMutation).toBeFalsy() expect(fileChangeEvents).toBe(1) }) - it('checks dirty tabs only once the watch is back, so a write in between still gets the banner', async () => { - harness.readRuntimeFileContent.mockResolvedValue({ content: 'v1', isBinary: false }) - await render([active], [background]) - await render([active, background], []) - await act(async () => vi.advanceTimersByTimeAsync(200)) - expect(harness.readRuntimeFileContent).not.toHaveBeenCalled() - - // An agent writes after the switch but before the watch is installed. - harness.readRuntimeFileContent.mockResolvedValue({ content: 'v2 late write', isBinary: false }) - await act(async () => settleWatch()) - await act(async () => vi.advanceTimersByTimeAsync(200)) - - expect(file(DIRTY).externalMutation).toBe('changed') - }) - - it('checks dirty tabs again when a late install (capacity, SSH retry) lands', async () => { - harness.readRuntimeFileContent.mockResolvedValue({ content: 'v1', isBinary: false }) - // The request returns while the watcher waits for capacity; the catch-up finds nothing yet. - await switchBack() - expect(file(DIRTY).externalMutation).toBeFalsy() - - harness.readRuntimeFileContent.mockResolvedValue({ - content: 'v2 before install', - isBinary: false - }) - // Main announces the late install with the same overflow it sends after a reconnect. - await act(async () => - emitIpc({ - worktreePath: '/background', - events: [{ kind: 'overflow', absolutePath: '/background' }], - resync: true - }) - ) - await act(async () => vi.advanceTimersByTimeAsync(200)) - - expect(file(DIRTY).externalMutation).toBe('changed') - }) - it('marks a clean tab deleted and keeps it when its file was deleted while unwatched', async () => { harness.readRuntimeFileContent.mockResolvedValue({ content: 'v1', isBinary: false }) pathExists.mockImplementation(async ({ filePath }: { filePath: string }) => filePath !== CLEAN) @@ -283,8 +242,9 @@ describe('switching back to a workspace that was not watched', () => { harness.readRuntimeFileContent.mockResolvedValue({ content: 'v1', isBinary: false }) await switchBack() + expect(harness.readRuntimeFileContent).toHaveBeenCalledTimes(1) expect(file(DIRTY).externalMutation).toBeFalsy() - expect(file(DIRTY).pendingDiskBaselineVerification).toBeFalsy() + expect(isAutosaveSuspendedForFile(file(DIRTY))).toBe(false) }) it('does not re-read tabs of a workspace that was never dropped', async () => { @@ -295,7 +255,6 @@ describe('switching back to a workspace that was not watched', () => { expect(reloads).toEqual([]) expect(harness.readRuntimeFileContent).not.toHaveBeenCalled() - expect(file(DIRTY).pendingDiskBaselineVerification).toBeFalsy() }) it('never holds autosave on a tab edited before the watch is back', async () => { @@ -306,7 +265,6 @@ describe('switching back to a workspace that was not watched', () => { store.getState().markFileDirty(CLEAN, true) await act(async () => {}) - expect(file(CLEAN).pendingDiskBaselineVerification).toBeFalsy() expect(isAutosaveSuspendedForFile(file(CLEAN))).toBe(false) }) }) diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 1e189263902..37cd0338523 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -16680,6 +16680,9 @@ }, "sectionStale": { "notice": "This file changed on disk since this section loaded." + }, + "notUtf8": { + "notice": "Autosave is paused for {{name}} because it isn't UTF-8 text. Save it yourself to convert it to UTF-8." } } }, 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 af9841f026a..09c3f9c8400 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 @@ -98,8 +98,7 @@ describe('executeOpenEditorPathMove', () => { expect(moved.isDirty).toBe(true) expect(moved.lastKnownDiskSignature).toBe('sig-a') expect(useAppStore.getState().editorDrafts['/repo/sub/a.md']).toBe('unsaved work') - // Dirty destination gets the content-verify gate + provenance. - expect(moved.pendingLiveDiskVerification).toBe(true) + // Dirty destination gets the echo provenance. expect(moved.pendingSelfMoveEcho?.targetPath).toBe('/repo/sub/a.md') // The in-flight transaction is settled (no leak). expect(__activeEditorPathMoveCountForTests()).toBe(0) @@ -200,8 +199,7 @@ describe('executeOpenEditorPathMove', () => { await new Promise((resolve) => setTimeout(resolve, 20)) const moved = useAppStore.getState().openFiles[0]! - // Disk matched the carried baseline: gate released, no false conflict banner. - expect(moved.pendingLiveDiskVerification).toBeFalsy() + // Disk matched the carried baseline: no false conflict banner. expect(moved.externalMutation).toBeUndefined() expect(moved.isDirty).toBe(true) // Safety-net verify LEAVES the provenance so a destination watcher event diff --git a/src/renderer/src/runtime/runtime-file-write-disk-precondition.test.ts b/src/renderer/src/runtime/runtime-file-write-disk-precondition.test.ts index 447575dd52d..738fc8a817a 100644 --- a/src/renderer/src/runtime/runtime-file-write-disk-precondition.test.ts +++ b/src/renderer/src/runtime/runtime-file-write-disk-precondition.test.ts @@ -5,7 +5,10 @@ import { MIN_COMPATIBLE_RUNTIME_CLIENT_VERSION, RUNTIME_PROTOCOL_VERSION } from '../../../shared/protocol-version' -import { FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY } from '../../../shared/file-write-disk-precondition' +import { + FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, + FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY +} from '../../../shared/file-write-disk-precondition' import { getDiskBaselineSignature } from '../../../shared/disk-baseline-signature' import { advanceRuntimeEnvironmentConnectionGeneration } from '@/store/slices/runtime-status-connection-generation' import { replaceRuntimeEnvironmentRevisions } from './runtime-environment-revision' @@ -33,6 +36,7 @@ function rpcOk(result: unknown) { function serveHost(options: { enforcesPrecondition: boolean + identicalNoop?: boolean diskContent?: string readError?: string readErrorCode?: string @@ -50,7 +54,12 @@ function serveHost(options: { capabilities: [ FILE_MUTATION_OWNERSHIP_RUNTIME_CAPABILITY, ...(options.enforcesPrecondition - ? [FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY] + ? [ + FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, + ...(options.identicalNoop === false + ? [] + : [FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY]) + ] : []) ] }) @@ -196,6 +205,73 @@ describe('writeRuntimeFile disk precondition', () => { expect(writeCalls()).toHaveLength(0) }) + it('skips writing through an older host whose changed disk already holds the text', async () => { + serveHost({ enforcesPrecondition: false, diskContent: 'edited' }) + runtimeEnvironmentCall.mockResolvedValue(rpcOk({ ok: true })) + + const result = await writeRuntimeFile( + remoteContext, + '/remote/repo/file.ts', + 'edited', + undefined, + precondition + ) + + expect(result).toEqual({ ok: true, unchanged: true }) + expect(writeCalls()).toHaveLength(0) + }) + + it.each(['original', 'edited'])( + 'skips identical content on a v1 guarded host for an automatic save (baseline %s)', + async (baseline) => { + serveHost({ enforcesPrecondition: true, identicalNoop: false, diskContent: 'edited' }) + expect( + await writeRuntimeFile(remoteContext, '/remote/repo/file.ts', 'edited', undefined, { + expectedSignature: getDiskBaselineSignature(baseline), + requireLosslessUtf8: true + }) + ).toEqual({ ok: true, unchanged: true }) + expect(writeCalls()).toHaveLength(0) + } + ) + + it('keeps the v1 host guard for non-identical automatic saves after comparing locally', async () => { + serveHost({ enforcesPrecondition: true, identicalNoop: false, diskContent: 'original' }) + runtimeEnvironmentCall.mockResolvedValue(rpcOk({ ok: false, diskConflict: 'changed' })) + expect( + await writeRuntimeFile(remoteContext, '/remote/repo/file.ts', 'edited', undefined, { + ...precondition, + requireLosslessUtf8: true + }) + ).toEqual({ ok: false, diskConflict: 'changed' }) + expect(writeCalls()[0]?.method).toBe('files.writeIfUnchanged') + }) + + it('sends an explicit save straight to a v1 guarded host without reading first', async () => { + let reads = 0 + serveHost({ + enforcesPrecondition: true, + identicalNoop: false, + diskContent: 'edited', + onRead: () => (reads += 1) + }) + runtimeEnvironmentCall.mockResolvedValue(rpcOk({ ok: true })) + await writeRuntimeFile(remoteContext, '/remote/repo/file.ts', 'edited', undefined, precondition) + expect(reads).toBe(0) + expect(writeCalls()[0]?.method).toBe('files.writeIfUnchanged') + }) + + it('refuses an ambiguous automatic baseline before sending to a v1 host', async () => { + serveHost({ enforcesPrecondition: true, identicalNoop: false, diskContent: 'a�' }) + expect( + await writeRuntimeFile(remoteContext, '/remote/repo/file.ts', 'a�', undefined, { + expectedSignature: getDiskBaselineSignature('a�'), + requireLosslessUtf8: true + }) + ).toEqual({ ok: false, diskConflict: 'not-utf8' }) + expect(writeCalls()).toHaveLength(0) + }) + it('reports a confirmed-missing file to an autosave through an older host', async () => { serveHost({ enforcesPrecondition: false }) diff --git a/src/renderer/src/runtime/runtime-file-write-disk-precondition.ts b/src/renderer/src/runtime/runtime-file-write-disk-precondition.ts index affe2525f19..172cc82978a 100644 --- a/src/renderer/src/runtime/runtime-file-write-disk-precondition.ts +++ b/src/renderer/src/runtime/runtime-file-write-disk-precondition.ts @@ -3,9 +3,11 @@ import { diskConflictFromReadFailure, FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, FILE_WRITE_IF_UNCHANGED_METHOD, + hostEnforcesPrecondition, FILE_WRITE_OK, + FILE_WRITE_UNCHANGED, parseFileWriteResult, - type FileWriteDiskConflict, + type FileWriteDiskComparison, type FileWriteDiskPrecondition, type FileWriteResult } from '../../../shared/file-write-disk-precondition' @@ -102,7 +104,7 @@ export async function writeRemoteFileIfUnchanged( context.target, 'files.write', async (capabilities) => { - if (capabilities.includes(FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY)) { + if (hostEnforcesPrecondition(capabilities, precondition)) { return { method: FILE_WRITE_IF_UNCHANGED_METHOD, params: { ...params, diskPrecondition: precondition } @@ -111,9 +113,21 @@ export async function writeRemoteFileIfUnchanged( const diskConflict = await checkDiskPreconditionFromClient( context, filePath, - precondition + precondition, + content ) - return diskConflict ? { result: { ok: false, diskConflict } } : { params } + if (diskConflict === 'identical') { + return { result: FILE_WRITE_UNCHANGED } + } + if (diskConflict) { + return { result: { ok: false, diskConflict } } + } + return capabilities.includes(FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY) + ? { + method: FILE_WRITE_IF_UNCHANGED_METHOD, + params: { ...params, diskPrecondition: precondition } + } + : { params } }, 15_000, pairingRevision, @@ -148,13 +162,18 @@ export async function writeRemoteFileIfUnchanged( async function checkDiskPreconditionFromClient( context: RemoteFileContext, filePath: string, - precondition: FileWriteDiskPrecondition -): Promise { + precondition: FileWriteDiskPrecondition, + content: string +): Promise { let current: { content: string; isBinary: boolean } try { current = await readRemoteFile(context, filePath) } catch (error) { return diskConflictFromReadFailure(precondition, error) } - return compareDiskWritePrecondition(precondition, current.isBinary ? undefined : current.content) + return compareDiskWritePrecondition( + precondition, + current.isBinary ? undefined : current.content, + content + ) } diff --git a/src/renderer/src/store/editor-buffer-record-lifecycle.test.ts b/src/renderer/src/store/editor-buffer-record-lifecycle.test.ts index 99735f04752..792af3d5490 100644 --- a/src/renderer/src/store/editor-buffer-record-lifecycle.test.ts +++ b/src/renderer/src/store/editor-buffer-record-lifecycle.test.ts @@ -68,8 +68,6 @@ describe('editor buffer records', () => { isDirty: false, lastKnownDiskSignature: undefined, externalMutation: undefined, - pendingDiskBaselineVerification: undefined, - pendingLiveDiskVerification: undefined, pendingOwnerMigration: undefined }) @@ -97,18 +95,6 @@ describe('editor buffer records', () => { 'lastKnownDiskSignature', 'sig-1' ], - [ - 'setPendingDiskBaselineVerification', - (id: string) => useAppStore.getState().setPendingDiskBaselineVerification(id, true), - 'pendingDiskBaselineVerification', - true - ], - [ - 'setPendingLiveDiskVerification', - (id: string) => useAppStore.getState().setPendingLiveDiskVerification(id, true), - 'pendingLiveDiskVerification', - true - ], [ 'setRestoredEditorOwnerMigrationPending', (id: string) => useAppStore.getState().setRestoredEditorOwnerMigrationPending(id, true), @@ -127,19 +113,7 @@ describe('editor buffer records', () => { expect(fileOf(id)?.[field]).toBe(value) }) - it('clears the restored-baseline gate on the record and its mirror', () => { - const id = openEditTab() - useAppStore.getState().setPendingDiskBaselineVerification(id, true) - - useAppStore.getState().clearPendingDiskBaselineVerification(id) - - expect(useAppStore.getState().editorBuffers[id]?.pendingDiskBaselineVerification).toBe( - undefined - ) - expect(fileOf(id)?.pendingDiskBaselineVerification).toBe(undefined) - }) - - it('re-keys the record with a self-move and gates the moved dirty tab on both copies', async () => { + it('re-keys the record with a self-move', async () => { const main = await drawTabsFromMain(useAppStore) const id = openEditTab() await main.settle() @@ -168,10 +142,8 @@ describe('editor buffer records', () => { expect(Object.keys(records)).toEqual(['/repo/b.md']) expect(records['/repo/b.md']).toMatchObject({ isDirty: true, - lastKnownDiskSignature: 'sig-a', - pendingLiveDiskVerification: true + lastKnownDiskSignature: 'sig-a' }) - expect(fileOf('/repo/b.md')?.pendingLiveDiskVerification).toBe(true) }) it('creates hydrated records from the session, not from a replaced same-id tab', () => { @@ -203,8 +175,7 @@ describe('editor buffer records', () => { expect(useAppStore.getState().editorBuffers[id]).toMatchObject({ isDirty: true, - lastKnownDiskSignature: 'sig-saved', - pendingDiskBaselineVerification: true + lastKnownDiskSignature: 'sig-saved' }) }) diff --git a/src/renderer/src/store/slices/editor-layout-owner-records.test.ts b/src/renderer/src/store/slices/editor-layout-owner-records.test.ts index 968e8d340c1..590bdb3dd9a 100644 --- a/src/renderer/src/store/slices/editor-layout-owner-records.test.ts +++ b/src/renderer/src/store/slices/editor-layout-owner-records.test.ts @@ -452,14 +452,10 @@ describe.runIf(ORCA_LAYOUT_OWNER_IS_MAIN)('open files renamed on disk, layout-ow expect.objectContaining({ id: '/repo-a/lib/a.ts', isDirty: true, - pendingLiveDiskVerification: true, pendingSelfMoveEcho: { operationId: 'move-1', targetPath: '/repo-a/lib/a.ts' } }) ]) expect(state.editorDrafts).toEqual({ '/repo-a/lib/a.ts': 'unsaved' }) - expect(state.editorBuffers['/repo-a/lib/a.ts']).toMatchObject({ - pendingLiveDiskVerification: true - }) }) it('changes nothing when main refuses the rename', async () => { diff --git a/src/renderer/src/store/slices/editor-read-only-tabs.test.ts b/src/renderer/src/store/slices/editor-read-only-tabs.test.ts index e76a4b8df8a..17d05d65603 100644 --- a/src/renderer/src/store/slices/editor-read-only-tabs.test.ts +++ b/src/renderer/src/store/slices/editor-read-only-tabs.test.ts @@ -139,7 +139,6 @@ describe('read-only editor tabs (AI Vault View Log)', () => { expect(restored).toEqual( expect.objectContaining({ readOnly: true, liveTail: true, isDirty: false, mode: 'edit' }) ) - expect(restored?.pendingDiskBaselineVerification).toBeUndefined() expect(store.getState().editorDrafts[LOG_PATH]).toBeUndefined() }) diff --git a/src/renderer/src/store/slices/editor-rekey-open-files.test.ts b/src/renderer/src/store/slices/editor-rekey-open-files.test.ts index 1e92b87f463..187a214087d 100644 --- a/src/renderer/src/store/slices/editor-rekey-open-files.test.ts +++ b/src/renderer/src/store/slices/editor-rekey-open-files.test.ts @@ -222,7 +222,7 @@ describe('rekeyOpenFilesForPathChange', () => { expect(moved.filePath).toBe('/repo/named.md') }) - it('installs the move-echo gate + provenance on a dirty destination when moveOperationId is set', async () => { + it('installs the move-echo provenance on a dirty destination when moveOperationId is set', async () => { seedEditTab() const oldId = useAppStore.getState().openFiles[0]!.id const newPath = '/repo/sub/a.md' @@ -233,16 +233,13 @@ describe('rekeyOpenFilesForPathChange', () => { }) const moved = useAppStore.getState().openFiles[0]! - // Autosave is gated synchronously by the same commit that re-homes the tab. - expect(moved.pendingLiveDiskVerification).toBe(true) expect(moved.pendingSelfMoveEcho).toEqual({ operationId: 'op-42', targetPath: newPath }) }) - it('does not gate a tab already showing the changed-on-disk banner', async () => { + it('does not verify a tab already showing the changed-on-disk banner', async () => { seedEditTab() const oldId = useAppStore.getState().openFiles[0]!.id - // A dirty tab that already conflicts with disk: autosave is suspended by the - // banner; gating it would strand the gate (verification skips a 'changed' tab). + // A dirty tab that already conflicts with disk keeps its banner. useAppStore.getState().setExternalMutation(oldId, 'changed') await rekey({ @@ -252,7 +249,6 @@ describe('rekeyOpenFilesForPathChange', () => { const moved = useAppStore.getState().openFiles[0]! expect(moved.externalMutation).toBe('changed') - expect(moved.pendingLiveDiskVerification).toBeUndefined() expect(moved.pendingSelfMoveEcho).toBeUndefined() }) @@ -272,7 +268,7 @@ describe('rekeyOpenFilesForPathChange', () => { expect(reveal.filePath).toBe('/repo/sub/a.md') }) - it('does not gate a clean destination', async () => { + it('does not verify a clean destination', async () => { seedEditTab() const oldId = useAppStore.getState().openFiles[0]!.id useAppStore.getState().markFileDirty(oldId, false) @@ -283,7 +279,6 @@ describe('rekeyOpenFilesForPathChange', () => { }) const moved = useAppStore.getState().openFiles[0]! - expect(moved.pendingLiveDiskVerification).toBeUndefined() expect(moved.pendingSelfMoveEcho).toBeUndefined() }) diff --git a/src/renderer/src/store/slices/editor/actions/hydrated-editor-session-state.ts b/src/renderer/src/store/slices/editor/actions/hydrated-editor-session-state.ts index d73048f2206..adaabf157ea 100644 --- a/src/renderer/src/store/slices/editor/actions/hydrated-editor-session-state.ts +++ b/src/renderer/src/store/slices/editor/actions/hydrated-editor-session-state.ts @@ -113,13 +113,6 @@ export function buildHydratedEditorSessionState( ...(isReadOnly ? { readOnly: true } : {}), ...(isReadOnly && pf.liveTail === true ? { liveTail: true } : {}), lastKnownDiskSignature: isReadOnly ? undefined : pf.lastKnownDiskSignature, - // Why: suspend autosave until the conflict scan verifies disk vs baseline, else a slow remote read clobbers an offline write. - pendingDiskBaselineVerification: - !isReadOnly && - pf.dirtyDraftContent !== undefined && - pf.lastKnownDiskSignature !== undefined - ? true - : undefined, mode: 'edit' }) } diff --git a/src/renderer/src/store/slices/editor/actions/open-file-mutations.ts b/src/renderer/src/store/slices/editor/actions/open-file-mutations.ts index cfd2ccf9651..d6b5acb5386 100644 --- a/src/renderer/src/store/slices/editor/actions/open-file-mutations.ts +++ b/src/renderer/src/store/slices/editor/actions/open-file-mutations.ts @@ -31,9 +31,6 @@ export function createOpenFileMutations( | 'markFileDirty' | 'setExternalMutation' | 'setLastKnownDiskSignature' - | 'clearPendingDiskBaselineVerification' - | 'setPendingDiskBaselineVerification' - | 'setPendingLiveDiskVerification' | 'clearSelfMoveEcho' | 'clearUntitled' > { @@ -146,19 +143,6 @@ export function createOpenFileMutations( setLastKnownDiskSignature: (fileId, signature) => set((s) => updateEditorBuffer(s, fileId, { lastKnownDiskSignature: signature })), - clearPendingDiskBaselineVerification: (fileId) => - get().setPendingDiskBaselineVerification(fileId, false), - - setPendingDiskBaselineVerification: (fileId, value) => - set((s) => - updateEditorBuffer(s, fileId, { pendingDiskBaselineVerification: value || undefined }) - ), - - setPendingLiveDiskVerification: (fileId, value) => - set((s) => - updateEditorBuffer(s, fileId, { pendingLiveDiskVerification: value || undefined }) - ), - clearSelfMoveEcho: (fileId) => set((s) => { const file = s.openFiles.find((f) => f.id === fileId) diff --git a/src/renderer/src/store/slices/editor/actions/rekey-open-files-action.ts b/src/renderer/src/store/slices/editor/actions/rekey-open-files-action.ts index 42965343e22..3c140f445f9 100644 --- a/src/renderer/src/store/slices/editor/actions/rekey-open-files-action.ts +++ b/src/renderer/src/store/slices/editor/actions/rekey-open-files-action.ts @@ -44,23 +44,19 @@ export function rekeyOpenFilesPatch( } } - const echoGatedIds = new Set() const nextOpenFiles = s.openFiles.map((f) => { const rekey = rekeyByOldId.get(f.id) if (!rekey) { return f } // Spread the whole OpenFile so fields this action doesn't know about survive; change only the path-derived ones. - // Gate atomically here so autosave is suspended before any echo can be verified (only a dirty autosave-capable tab can be clobbered). - const gatesEcho = + // Only a dirty autosave-capable tab needs its destination echo verified. + const verifiesEcho = moveOperationId !== undefined && f.isDirty && - // A 'changed' tab is already autosave-suspended via externalMutation; gating it would strand the gate (verification skips a 'changed' tab), so leave the banner as terminal. + // A 'changed' tab already shows the banner, which verification would only repeat. f.externalMutation !== 'changed' && (f.mode === 'edit' || (f.mode === 'diff' && f.diffSource === 'unstaged')) - if (gatesEcho) { - echoGatedIds.add(f.id) - } return { ...f, id: rekey.newFileId, @@ -75,9 +71,8 @@ export function rekeyOpenFilesPatch( ...(rekey.consumeUntitled ? { isUntitled: undefined, deleteUntouchedOnClose: undefined } : {}), - ...(gatesEcho + ...(verifiesEcho ? { - pendingLiveDiskVerification: true, pendingSelfMoveEcho: { operationId: moveOperationId, targetPath: rekey.newFilePath @@ -119,9 +114,7 @@ export function rekeyOpenFilesPatch( ok: true, patch: { openFiles: nextOpenFiles, - editorBuffers: rekeyEditorBuffers(s.editorBuffers, s.openFiles, migrations, (f) => - echoGatedIds.has(f.id) ? { pendingLiveDiskVerification: true } : undefined - ), + editorBuffers: rekeyEditorBuffers(s.editorBuffers, s.openFiles, migrations), editorDrafts: rekeyFileIdRecord(s.editorDrafts, migrations), editorCursorLine: rekeyFileIdRecord(s.editorCursorLine, migrations), markdownViewMode: rekeyFileIdRecord(s.markdownViewMode, migrations), diff --git a/src/renderer/src/store/slices/editor/buffers/editor-buffer-record.ts b/src/renderer/src/store/slices/editor/buffers/editor-buffer-record.ts index a24a793bd22..f6d407b59ef 100644 --- a/src/renderer/src/store/slices/editor/buffers/editor-buffer-record.ts +++ b/src/renderer/src/store/slices/editor/buffers/editor-buffer-record.ts @@ -11,12 +11,7 @@ import { */ export type EditorBufferRecord = Pick< OpenFile, - | 'isDirty' - | 'lastKnownDiskSignature' - | 'externalMutation' - | 'pendingDiskBaselineVerification' - | 'pendingLiveDiskVerification' - | 'pendingOwnerMigration' + 'isDirty' | 'lastKnownDiskSignature' | 'externalMutation' | 'pendingOwnerMigration' > export type EditorBufferRecords = Record @@ -31,8 +26,6 @@ export const EDITOR_BUFFER_FIELDS = [ 'isDirty', 'lastKnownDiskSignature', 'externalMutation', - 'pendingDiskBaselineVerification', - 'pendingLiveDiskVerification', 'pendingOwnerMigration' ] as const satisfies readonly (keyof EditorBufferRecord)[] @@ -41,8 +34,6 @@ export function editorBufferRecordFromFile(file: OpenFile): EditorBufferRecord { isDirty: file.isDirty, lastKnownDiskSignature: file.lastKnownDiskSignature, externalMutation: file.externalMutation, - pendingDiskBaselineVerification: file.pendingDiskBaselineVerification, - pendingLiveDiskVerification: file.pendingLiveDiskVerification, pendingOwnerMigration: file.pendingOwnerMigration } } diff --git a/src/renderer/src/store/slices/editor/types/editor-files-slice.ts b/src/renderer/src/store/slices/editor/types/editor-files-slice.ts index 6ae79cccd5e..2bbd96e6070 100644 --- a/src/renderer/src/store/slices/editor/types/editor-files-slice.ts +++ b/src/renderer/src/store/slices/editor/types/editor-files-slice.ts @@ -108,9 +108,6 @@ export type EditorFilesSlice = { setExternalMutation: (fileId: string, mutation: 'deleted' | 'renamed' | 'changed' | null) => void /** `undefined` un-records a baseline a failed operation stamped provisionally. */ setLastKnownDiskSignature: (fileId: string, signature: string | undefined) => void - clearPendingDiskBaselineVerification: (fileId: string) => void - setPendingDiskBaselineVerification: (fileId: string, value: boolean) => void - setPendingLiveDiskVerification: (fileId: string, value: boolean) => void clearSelfMoveEcho: (fileId: string) => void /** Atomically retargets open editor sessions across an Orca-owned move — one commit-only update migrating every path-derived id + all id-keyed state, no close/reopen. Returns collision/stale without mutating. */ rekeyOpenFilesForPathChange: (args: { diff --git a/src/renderer/src/store/slices/editor/types/open-file.ts b/src/renderer/src/store/slices/editor/types/open-file.ts index d0788a292f1..631659a8da1 100644 --- a/src/renderer/src/store/slices/editor/types/open-file.ts +++ b/src/renderer/src/store/slices/editor/types/open-file.ts @@ -73,10 +73,6 @@ export type OpenFile = { externalMutation?: 'deleted' | 'renamed' | 'changed' /** Signature of the disk content this tab's edits are based on; persisted so a restore detects a changed-on-disk conflict before autosave clobbers an agent write. */ lastKnownDiskSignature?: string - /** Why: gates autosave for restored dirty tabs until the conflict scan compares disk vs baseline, else a slow SSH read loses the race. Not persisted. */ - pendingDiskBaselineVerification?: boolean - /** Why: gates autosave during a live self-move echo's disk verification; separate flag from the restored scan's so the two can't clear each other's gate. Not persisted. */ - pendingLiveDiskVerification?: boolean /** Blocks saves while a restored tab's filesystem authority is being replaced. */ pendingOwnerMigration?: boolean /** Why: routes an Orca-owned move's destination-watcher echo into content verification. On the tab so it survives the atomic rekey; operationId supersedes a stale verification on re-move. Not persisted. */ diff --git a/src/renderer/src/store/slices/restored-editor-owner-reparent.test.ts b/src/renderer/src/store/slices/restored-editor-owner-reparent.test.ts index 636cb92842f..a426f7ea0c2 100644 --- a/src/renderer/src/store/slices/restored-editor-owner-reparent.test.ts +++ b/src/renderer/src/store/slices/restored-editor-owner-reparent.test.ts @@ -77,8 +77,7 @@ function openRestoredSource(): string { worktreeId: SOURCE, runtimeEnvironmentId: null, language: 'markdown', - mode: 'edit', - pendingDiskBaselineVerification: true + mode: 'edit' }, { suppressActiveRuntimeFallback: true } ) @@ -172,8 +171,7 @@ describe('restored editor owner reparent', () => { relativePath: 'docs/readme.md', worktreeId: TARGET, runtimeEnvironmentId: null, - isDirty: true, - pendingDiskBaselineVerification: true + isDirty: true }) expect(moved.pendingOwnerMigration).toBeUndefined() expect(moved.operationProvenance?.generation.route).toEqual({ diff --git a/src/renderer/src/web/web-file-mutation-methods.ts b/src/renderer/src/web/web-file-mutation-methods.ts index fd569d16ce5..ea7e463c294 100644 --- a/src/renderer/src/web/web-file-mutation-methods.ts +++ b/src/renderer/src/web/web-file-mutation-methods.ts @@ -11,8 +11,10 @@ import { diskConflictFromReadFailure, FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, FILE_WRITE_IF_UNCHANGED_METHOD, + hostEnforcesPrecondition, + FILE_WRITE_UNCHANGED, parseFileWriteResult, - type FileWriteDiskConflict, + type FileWriteDiskComparison, type FileWriteDiskPrecondition, type FileWriteResult } from '../../../shared/file-write-disk-precondition' @@ -135,8 +137,9 @@ async function readWholeTextFile( async function checkWebDiskPreconditionFromClient( session: WebFileMutationSession, file: ResolvedWebRuntimeFile, - precondition: FileWriteDiskPrecondition -): Promise { + precondition: FileWriteDiskPrecondition, + content: string +): Promise { let preview: unknown try { preview = await session.callRuntimeResult('files.readPreview', { @@ -148,7 +151,7 @@ async function checkWebDiskPreconditionFromClient( if (error instanceof Error && error.message === 'file_too_large') { // Why undefined on failure: a host that can't page it keeps the earlier answer, 'changed'. const text = await readWholeTextFile(session, file).catch(() => undefined) - return compareDiskWritePrecondition(precondition, text) + return compareDiskWritePrecondition(precondition, text, content) } return diskConflictFromReadFailure(precondition, error) } @@ -160,7 +163,7 @@ async function checkWebDiskPreconditionFromClient( typeof preview.content === 'string' ? preview.content : undefined - return compareDiskWritePrecondition(precondition, text) + return compareDiskWritePrecondition(precondition, text, content) } export function createWebFileMutationMethods( @@ -205,7 +208,7 @@ export function createWebFileMutationMethods( if (!diskPrecondition) { return { method: 'files.write', params } } - if (capabilities.includes(FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY)) { + if (hostEnforcesPrecondition(capabilities, diskPrecondition)) { return { method: FILE_WRITE_IF_UNCHANGED_METHOD, params: { ...params, diskPrecondition } @@ -214,9 +217,18 @@ export function createWebFileMutationMethods( const diskConflict = await checkWebDiskPreconditionFromClient( session, file, - diskPrecondition + diskPrecondition, + content ) - return diskConflict ? { ok: false, diskConflict } : { method: 'files.write', params } + if (diskConflict === 'identical') { + return FILE_WRITE_UNCHANGED + } + if (diskConflict) { + return { ok: false, diskConflict } + } + return capabilities.includes(FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY) + ? { method: FILE_WRITE_IF_UNCHANGED_METHOD, params: { ...params, diskPrecondition } } + : { method: 'files.write', params } }, diskPrecondition !== undefined ) diff --git a/src/renderer/src/web/web-file-write-disk-precondition.test.ts b/src/renderer/src/web/web-file-write-disk-precondition.test.ts index 7ab5634385f..7605f47cfb8 100644 --- a/src/renderer/src/web/web-file-write-disk-precondition.test.ts +++ b/src/renderer/src/web/web-file-write-disk-precondition.test.ts @@ -1,6 +1,9 @@ import { describe, expect, it, vi } from 'vitest' import { getDiskBaselineSignature } from '../../../shared/disk-baseline-signature' -import { FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY } from '../../../shared/file-write-disk-precondition' +import { + FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, + FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY +} from '../../../shared/file-write-disk-precondition' import { createWebFileMutationMethods } from './web-file-mutation-methods' const precondition = { expectedSignature: getDiskBaselineSignature('original') } @@ -37,7 +40,7 @@ function methodsFor( describe('paired web client save precondition', () => { it('lets a server that enforces the precondition compare at write time', async () => { const { methods, callRuntimeResult, calledMethods } = methodsFor( - [FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY], + [FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY], 'original' ) @@ -64,6 +67,19 @@ describe('paired web client save precondition', () => { expect(calledMethods()).toEqual(['files.readPreview']) }) + it('skips writing through an older server whose changed disk already holds the text', async () => { + const { methods, calledMethods } = methodsFor([], 'edited') + + const result = await methods.writeFile({ + filePath: '/hub/repo/readme.md', + content: 'edited', + diskPrecondition: precondition + }) + + expect(result).toEqual({ ok: true, unchanged: true }) + expect(calledMethods()).toEqual(['files.readPreview']) + }) + it('writes through an older server without the field once the disk matches', async () => { const { methods, callRuntimeResult, calledMethods } = methodsFor([], 'original') @@ -78,6 +94,71 @@ describe('paired web client save precondition', () => { expect(callRuntimeResult.mock.calls[1]?.[1]).not.toHaveProperty('diskPrecondition') }) + it.each(['original', 'edited'])( + 'skips identical content on a v1 guarded server for an automatic save (baseline %s)', + async (baseline) => { + const { methods, calledMethods } = methodsFor( + [FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY], + 'edited' + ) + expect( + await methods.writeFile({ + filePath: '/hub/repo/readme.md', + content: 'edited', + diskPrecondition: { + expectedSignature: getDiskBaselineSignature(baseline), + requireLosslessUtf8: true + } + }) + ).toEqual({ ok: true, unchanged: true }) + expect(calledMethods()).toEqual(['files.readPreview']) + } + ) + + it('keeps the v1 server guard for non-identical automatic saves', async () => { + const { methods, calledMethods } = methodsFor( + [FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY], + 'original' + ) + await methods.writeFile({ + filePath: '/hub/repo/readme.md', + content: 'edited', + diskPrecondition: { ...precondition, requireLosslessUtf8: true } + }) + expect(calledMethods()).toEqual(['files.readPreview', 'files.writeIfUnchanged']) + }) + + it('sends an explicit save straight to a v1 guarded server without reading first', async () => { + const { methods, calledMethods } = methodsFor( + [FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY], + 'original' + ) + await methods.writeFile({ + filePath: '/hub/repo/readme.md', + content: 'edited', + diskPrecondition: precondition + }) + expect(calledMethods()).toEqual(['files.writeIfUnchanged']) + }) + + it('refuses an ambiguous automatic baseline before sending to a v1 server', async () => { + const { methods, calledMethods } = methodsFor( + [FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY], + 'a�' + ) + expect( + await methods.writeFile({ + filePath: '/hub/repo/readme.md', + content: 'a�', + diskPrecondition: { + expectedSignature: getDiskBaselineSignature('a�'), + requireLosslessUtf8: true + } + }) + ).toEqual({ ok: false, diskConflict: 'not-utf8' }) + expect(calledMethods()).toEqual(['files.readPreview']) + }) + it('reports a confirmed-missing file to an autosave through an older server', async () => { const { methods, calledMethods } = methodsFor([], null) diff --git a/src/shared/file-write-disk-precondition.test.ts b/src/shared/file-write-disk-precondition.test.ts index 973f8a34f93..317a4c8e90d 100644 --- a/src/shared/file-write-disk-precondition.test.ts +++ b/src/shared/file-write-disk-precondition.test.ts @@ -13,16 +13,60 @@ import { describe('disk write precondition', () => { it('compares decoded disk text against the baseline signature', () => { const precondition = { expectedSignature: getDiskBaselineSignature('base') } - expect(compareDiskWritePrecondition(precondition, 'base')).toBeNull() - expect(compareDiskWritePrecondition(precondition, 'newer')).toBe('changed') - expect(compareDiskWritePrecondition(precondition, undefined)).toBe('changed') - expect(compareDiskWritePrecondition(precondition, null)).toBe('missing') - expect(compareDiskWritePrecondition({ ...precondition, allowMissing: true }, null)).toBeNull() + expect(compareDiskWritePrecondition(precondition, 'base', 'mine')).toBeNull() + expect(compareDiskWritePrecondition(precondition, 'newer', 'mine')).toBe('changed') + expect(compareDiskWritePrecondition(precondition, undefined, 'mine')).toBe('changed') + expect(compareDiskWritePrecondition(precondition, null, 'mine')).toBe('missing') + expect( + compareDiskWritePrecondition({ ...precondition, allowMissing: true }, null, 'mine') + ).toBeNull() + }) + + it('accepts a changed disk that already holds exactly the text being saved', () => { + const precondition = { expectedSignature: getDiskBaselineSignature('base') } + expect(compareDiskWritePrecondition(precondition, 'mine', 'mine')).toBe('identical') + expect(compareDiskWritePrecondition(precondition, 'base', 'base')).toBe('identical') + expect(compareDiskWritePrecondition(precondition, 'mine\r\n', 'mine\n')).toBe('changed') + expect(compareDiskWritePrecondition(precondition, undefined, 'mine')).toBe('changed') + // A replacement character may stand for bytes the decode dropped, so it proves nothing. + expect(compareDiskWritePrecondition(precondition, 'a�', 'a�')).toBe('changed') + }) + + it('refuses ambiguous automatic baseline matches but preserves explicit saves', () => { + const decoded = Buffer.from([0x61, 0xfe]).toString('utf8') + const current = Buffer.from([0x61, 0xff]).toString('utf8') + const precondition = { expectedSignature: getDiskBaselineSignature(decoded) } + expect( + compareDiskWritePrecondition({ ...precondition, requireLosslessUtf8: true }, current, decoded) + ).toBe('not-utf8') + expect(compareDiskWritePrecondition(precondition, current, decoded)).toBeNull() + }) + + it('reports an external rewrite as changed before checking for U+FFFD', () => { + expect( + compareDiskWritePrecondition( + { expectedSignature: getDiskBaselineSignature('original'), requireLosslessUtf8: true }, + 'rewritten �', + 'mine' + ) + ).toBe('changed') }) it('treats anything but an explicit refusal from a host as a completed write', () => { + expect(parseFileWriteResult({ ok: false, diskConflict: 'not-utf8' })).toEqual({ + ok: false, + diskConflict: 'not-utf8' + }) + expect(parseFileWriteResult({ ok: false, diskConflict: 'later' })).toEqual({ + ok: false, + diskConflict: 'changed' + }) expect(parseFileWriteResult(undefined)).toEqual({ ok: true }) expect(parseFileWriteResult({ ok: true })).toEqual({ ok: true }) + expect(parseFileWriteResult({ ok: true, unchanged: true })).toEqual({ + ok: true, + unchanged: true + }) expect(parseFileWriteResult({ ok: false, diskConflict: 'missing' })).toEqual({ ok: false, diskConflict: 'missing' diff --git a/src/shared/file-write-disk-precondition.ts b/src/shared/file-write-disk-precondition.ts index caa7f97f0ef..98eaa3d1f80 100644 --- a/src/shared/file-write-disk-precondition.ts +++ b/src/shared/file-write-disk-precondition.ts @@ -4,42 +4,86 @@ import { getDiskBaselineSignature, getWrittenContentDiskSignature } from './disk export const FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY = 'files.write.disk-precondition.v1' as const +// Why separate: v1 hosts can rewrite identical text and cannot enforce lossless automatic saves. +export const FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY = 'files.write.identical-noop.v1' as const + export const FILE_WRITE_IF_UNCHANGED_METHOD = 'files.writeIfUnchanged' +/** + * Whether the host's guarded write can be sent as is. A v1 host can't check bytes or skip identical + * text, so only automatic saves (which must refuse non-UTF-8 files) pay for a client-side read first. + */ +export function hostEnforcesPrecondition( + capabilities: readonly unknown[], + precondition: FileWriteDiskPrecondition +): boolean { + return ( + capabilities.includes(FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY) || + (capabilities.includes(FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY) && + precondition.requireLosslessUtf8 !== true) + ) +} + /** Refuse the write unless disk still holds the content this signature was taken over. */ export type FileWriteDiskPrecondition = { expectedSignature: string /** A missing file is written (recreated) instead of refused. */ allowMissing?: boolean + /** Automatic saves refuse a file that isn't valid UTF-8 rather than convert it unasked. */ + requireLosslessUtf8?: boolean } -export type FileWriteDiskConflict = 'changed' | 'missing' +/** `not-utf8`: an automatic save met bytes it can't round-trip; older clients read it as `changed`. */ +export type FileWriteDiskConflict = 'changed' | 'missing' | 'not-utf8' -export type FileWriteResult = { ok: true } | { ok: false; diskConflict: FileWriteDiskConflict } +export type FileWriteDiskComparison = FileWriteDiskConflict | 'identical' | null + +export type FileWriteResult = + | { ok: true; unchanged?: boolean } + | { ok: false; diskConflict: FileWriteDiskConflict } export const FILE_WRITE_OK: FileWriteResult = { ok: true } +export const FILE_WRITE_UNCHANGED: FileWriteResult = { ok: true, unchanged: true } -/** `current` is the decoded disk text, `null` when missing, `undefined` when not comparable text. */ +/** `current` is the decoded disk text, `null` when missing, `undefined` when not comparable text; `content` is the text being saved. */ export function compareDiskWritePrecondition( precondition: FileWriteDiskPrecondition, - current: string | null | undefined -): FileWriteDiskConflict | null { + current: string | null | undefined, + content: string +): FileWriteDiskComparison { if (current === null) { return precondition.allowMissing === true ? null : 'missing' } if (current === undefined) { return 'changed' } - return getDiskBaselineSignature(current) === precondition.expectedSignature ? null : 'changed' + // Why: U+FFFD may stand for bytes a decode dropped, so decoded equality proves nothing. + if (current === content && !current.includes('\uFFFD')) { + return 'identical' + } + if (getDiskBaselineSignature(current) !== precondition.expectedSignature) { + return 'changed' + } + // Why: decoded text can't tell a real U+FFFD from replaced bytes, so a reader of text stays conservative. + return precondition.requireLosslessUtf8 && current.includes('\uFFFD') ? 'not-utf8' : null } /** Why: old hosts answer `{ ok: true }` or nothing; only an explicit `ok: false` is a refusal. */ export function parseFileWriteResult(value: unknown): FileWriteResult { - if (typeof value !== 'object' || value === null || !('ok' in value) || value.ok !== false) { + if (typeof value !== 'object' || value === null || !('ok' in value)) { return FILE_WRITE_OK } + if (value.ok !== false) { + return value.ok === true && 'unchanged' in value && value.unchanged === true + ? FILE_WRITE_UNCHANGED + : FILE_WRITE_OK + } const diskConflict = 'diskConflict' in value ? value.diskConflict : undefined - return { ok: false, diskConflict: diskConflict === 'missing' ? 'missing' : 'changed' } + return { + ok: false, + diskConflict: + diskConflict === 'missing' || diskConflict === 'not-utf8' ? diskConflict : 'changed' + } } /** Only a not-found answer proves the file is gone; a lost transport proves nothing. */ @@ -79,7 +123,7 @@ export function diskConflictFromReadFailure( error: unknown ): FileWriteDiskConflict | null { if (isConfirmedFileNotFoundError(error)) { - return compareDiskWritePrecondition(precondition, null) + return precondition.allowMissing === true ? null : 'missing' } // Why: the baseline was read whole, so content too big to read now cannot match it. if (isFileTooLargeToReadError(error)) { diff --git a/src/shared/files-runtime-capabilities.ts b/src/shared/files-runtime-capabilities.ts index 9484b3d42b4..db168c2177b 100644 --- a/src/shared/files-runtime-capabilities.ts +++ b/src/shared/files-runtime-capabilities.ts @@ -1,4 +1,7 @@ -import { FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY } from './file-write-disk-precondition' +import { + FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, + FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY +} from './file-write-disk-precondition' // Why: older runtimes strip mutation owner fields, so clients must fence writes before RPC. export const FILE_MUTATION_OWNERSHIP_RUNTIME_CAPABILITY = 'files.mutation-ownership.v1' as const @@ -12,5 +15,6 @@ export const FILES_RUNTIME_CAPABILITIES = [ 'files.resolve-terminal-path.cross-workspace.v1', FILE_DOC_PREVIEW_RUNTIME_CAPABILITY, FILE_WRITE_DISK_PRECONDITION_RUNTIME_CAPABILITY, + FILE_WRITE_IDENTICAL_RUNTIME_CAPABILITY, FILE_MUTATION_OWNERSHIP_RUNTIME_CAPABILITY ] as const diff --git a/src/shared/rpc-contract/files-mutation-params.ts b/src/shared/rpc-contract/files-mutation-params.ts index f122b5f1f8b..fada0a6fc96 100644 --- a/src/shared/rpc-contract/files-mutation-params.ts +++ b/src/shared/rpc-contract/files-mutation-params.ts @@ -32,7 +32,8 @@ export const FileWrite = FileMutationOpen.extend({ export const FileWriteIfUnchanged = FileWrite.extend({ diskPrecondition: z.object({ expectedSignature: z.string().min(1).max(64), - allowMissing: z.boolean().optional() + allowMissing: z.boolean().optional(), + requireLosslessUtf8: z.boolean().optional() }) })