fix(provider): preserve sync correctness

Author: ClaudiaFang

src/logic/sync-manager.ts

@@ -22,7 +22,7 @@ type PlanClassification = { kind: 'addition' | 'modification' | 'move' | 'unchan
 type ToPushEntry = { path: string; name: string; repoPath: string; content: string | ArrayBuffer; existingSha?: string; existingRevision?: string };
 
 /** A renamed file classified as a safe move, queued for the grouped batch-commit call. */
-type ToMoveEntry = { path: string; name: string; repoPath: string; oldPath: string; oldRepoPath: string; content: string | ArrayBuffer };
+type ToMoveEntry = { path: string; name: string; repoPath: string; oldPath: string; oldRepoPath: string; content: string | ArrayBuffer; oldRevision?: string };
 
 /**
  * Result of a batch push. `syncedPaths` lists every path that's now confirmed
@@ -682,6 +682,7 @@ export class SyncManager {
         }
 
         const treeEntry = treeByFullPath.get(this.getFullPathForTree(repoPath));
+        await this.migrateGitLabLegacyBaseline(path, repoPath, treeEntry);
         const outcome = await this.classifyAgainstTreeEntry(path, content, treeEntry, true);
         if (outcome === 'queued') return { kind: treeEntry ? 'modification' : 'addition' };
         // classifyAgainstTreeEntry's dry-run path only ever resolves to
@@ -714,7 +715,7 @@ export class SyncManager {
         if (!renamedFrom) return undefined;
 
         const scratch: ToMoveEntry[] = [];
-        const outcome = this.queueMove(path, name, renamedFrom, content, treeByFullPath, scratch);
+        const outcome = await this.queueMove(path, name, renamedFrom, content, treeByFullPath, scratch);
         return outcome === 'queued' ? { kind: 'move', movedFrom: renamedFrom } : { kind: 'conflict' };
     }
 
@@ -742,6 +743,7 @@ export class SyncManager {
 
         const localSha = await gitBlobSha(await this.getFileContent(fileOrPath));
         if (localSha === entry.sha) return 'unchanged';
+        await this.migrateGitLabLegacyBaseline(path, repoPath, entry);
         const lastSynced = this.settings.syncMetadata[path];
         if (lastSynced && entry.sha !== lastSynced.lastSyncedSha) return 'conflict';
         return 'modification';
@@ -891,15 +893,18 @@ export class SyncManager {
             const trackedOldPath = this.settings.syncMetadata[path]?.renamedFrom;
             const renamedFrom = trackedOldPath ?? (hasOrphans ? await this.detectRename(fileOrPath, content, treeByFullPath) : null);
             if (renamedFrom) {
-                return this.queueMove(path, name, renamedFrom, content, treeByFullPath, toMove);
+                return await this.queueMove(path, name, renamedFrom, content, treeByFullPath, toMove);
             }
         }
 
-        const treeEntry = treeByFullPath.get(this.getFullPathForTree(repoPath));
+        let treeEntry = treeByFullPath.get(this.getFullPathForTree(repoPath));
+        await this.migrateGitLabLegacyBaseline(path, repoPath, treeEntry);
+        const revision = await this.refreshGitLabBatchRevision(repoPath, treeEntry);
+        if (revision) treeEntry = { ...treeEntry!, sha: revision.sha };
         const outcome = await this.classifyAgainstTreeEntry(path, content, treeEntry);
         if (outcome !== 'queued') return outcome;
 
-        toPush.push({ path, name, repoPath, content, existingSha: treeEntry?.sha });
+        toPush.push({ path, name, repoPath, content, existingSha: treeEntry?.sha, existingRevision: revision?.revision });
         return 'queued';
     }
 
@@ -912,28 +917,50 @@ export class SyncManager {
      * silently deleted. Both surface as 'conflict' so the batch can't quietly
      * clobber either side the way a plain content push already refuses to.
      */
-    private queueMove(
+    private async queueMove(
         path: string,
         name: string,
         oldPath: string,
         content: string | ArrayBuffer,
         treeByFullPath: Map<string, GitTreeEntry>,
         toMove: ToMoveEntry[]
-    ): BatchOutcome | 'queued' {
+    ): Promise<BatchOutcome | 'queued'> {
         const repoPath = this.getNormalizedPath(path);
         const oldRepoPath = this.getNormalizedPath(oldPath);
 
         if (treeByFullPath.get(this.getFullPathForTree(repoPath))) return 'conflict';
 
-        const oldEntry = treeByFullPath.get(this.getFullPathForTree(oldRepoPath));
+        let oldEntry = treeByFullPath.get(this.getFullPathForTree(oldRepoPath));
+        await this.migrateGitLabLegacyBaseline(oldPath, oldRepoPath, oldEntry);
+        const oldRevision = await this.refreshGitLabBatchRevision(oldRepoPath, oldEntry);
+        if (oldRevision) oldEntry = { ...oldEntry!, sha: oldRevision.sha };
         const metadata = this.settings.syncMetadata[path] ?? this.settings.syncMetadata[oldPath];
         const safeToDeleteOld = !oldEntry?.sha || !metadata?.lastSyncedSha || oldEntry.sha === metadata.lastSyncedSha;
         if (oldEntry?.sha && !safeToDeleteOld) return 'conflict';
 
-        toMove.push({ path, name, repoPath, oldPath, oldRepoPath, content });
+        toMove.push({ path, name, repoPath, oldPath, oldRepoPath, content, oldRevision: oldRevision?.revision });
         return 'queued';
     }
 
+    /** GitLab tree rows expose blob identity but not the commit revision needed
+     * for optimistic locking. Read it during planning and compare the fresh blob
+     * again before accepting the action; the stored revision then protects the
+     * interval between planning and the atomic commit. */
+    private async refreshGitLabBatchRevision(repoPath: string, entry: GitTreeEntry | undefined): Promise<{ sha: string; revision?: string } | undefined> {
+        if (this.settings.serviceType !== 'gitlab' || !entry?.sha) return undefined;
+        const remote = await this.gitService.getFile(repoPath, this.settings.branch);
+        return remote.sha ? { sha: remote.sha, revision: remote.revision } : undefined;
+    }
+
+    /** Migrates a legacy GitLab last_commit_id baseline only when the current
+     * file endpoint proves it still describes this tree blob. */
+    private async migrateGitLabLegacyBaseline(path: string, repoPath: string, entry: GitTreeEntry | undefined): Promise<void> {
+        const metadata = this.settings.syncMetadata[path];
+        if (this.settings.serviceType !== 'gitlab' || !metadata?.lastSyncedSha || !entry?.sha || entry.sha === metadata.lastSyncedSha) return;
+        const remote = await this.gitService.getFile(repoPath, this.settings.branch);
+        if (remote.sha === entry.sha && remote.revision === metadata.lastSyncedSha) await this.updateMetadata(path, remote.sha);
+    }
+
     /**
      * Decides a non-symlink, non-renamed file's outcome purely from a
      * pre-fetched tree entry and a locally-computed git blob sha — no network
@@ -1060,7 +1087,7 @@ export class SyncManager {
         try {
             const commitMessage = `Push ${chunk.length} file(s) from Obsidian`;
             const batchResults = await this.gitService.pushBatch!(
-                chunk.map(f => ({ path: f.repoPath, content: f.content, existedRemotely: !!f.existingSha })),
+                chunk.map(f => ({ path: f.repoPath, content: f.content, existedRemotely: !!f.existingSha, revision: f.existingRevision })),
                 this.settings.branch,
                 commitMessage
             );
@@ -1106,8 +1133,8 @@ export class SyncManager {
             const commitMessage = this.combinedChunkCommitMessage(pushEntries.length, moveEntries.length);
 
             const batchResults = await this.gitService.commitBatch!(
-                pushEntries.map(f => ({ path: f.repoPath, content: f.content, existedRemotely: !!f.existingSha })),
-                moveEntries.map(f => ({ oldPath: f.oldRepoPath, newPath: f.repoPath, content: f.content })),
+                pushEntries.map(f => ({ path: f.repoPath, content: f.content, existedRemotely: !!f.existingSha, revision: f.existingRevision })),
+                moveEntries.map(f => ({ oldPath: f.oldRepoPath, newPath: f.repoPath, content: f.content, oldRevision: f.oldRevision })),
                 this.settings.branch,
                 commitMessage
             );
@@ -1249,6 +1276,8 @@ export class SyncManager {
             return 'unchanged';
         }
 
+        await this.migrateGitLabLegacyBaseline(path, this.getNormalizedPath(path), entry);
+
         // Same conflict check as the content path below: local differs and the
         // remote has moved since we last synced, so pulling would discard one of
         // the two changes.

src/services/git-service-base.ts

@@ -1,6 +1,7 @@
 import { requestUrl, RequestUrlResponse } from 'obsidian';
 import { logger } from '../utils/logger';
 import { GitTreeEntry } from './git-service-interface';
+import { isBinaryPath } from '../utils/path';
 
 export interface GitFile {
     content: string | ArrayBuffer;
@@ -195,18 +196,6 @@ export abstract class BaseGitService {
         return cleanRoot + path;
     }
 
-    protected isBinary(path: string): boolean {
-        const ext = path.split('.').pop()?.toLowerCase();
-        if (!ext) return false;
-        const BINARY_EXTENSIONS = new Set([
-            'png', 'jpg', 'jpeg', 'gif', 'bmp', 'ico', 'pdf', 'zip', 'gz', '7z', 'rar',
-            'mp3', 'mp4', 'wav', 'ogg', 'webm', 'mov', 'avi', 'wmv', 'webp',
-            'doc', 'docx', 'xls', 'xlsx', 'ppt', 'pptx', 'epub', 'exe', 'dll', 'so',
-            'ttf', 'woff', 'woff2', 'eot', 'wasm', 'dmg', 'iso'
-        ]);
-        return BINARY_EXTENSIONS.has(ext);
-    }
-
     protected encodeContent(content: string | ArrayBuffer): string {
         if (typeof content === 'string') {
             const bytes = new TextEncoder().encode(content);
@@ -235,7 +224,7 @@ export abstract class BaseGitService {
             bytes[i] = cp !== undefined ? cp : 0;
         }
 
-        return this.isBinary(path) ? bytes.buffer : new TextDecoder().decode(bytes);
+        return isBinaryPath(path) ? bytes.buffer : new TextDecoder().decode(bytes);
     }
 
     /**

src/services/git-service-interface.ts

@@ -36,6 +36,8 @@ export interface BatchPushItem {
      * action 'create' vs 'update'); GitHub/Gitea's tree-based commit ignores it.
      */
     existedRemotely?: boolean;
+    /** Revision read during batch planning, used by GitLab's optimistic lock. */
+    revision?: string;
 }
 
 /** Result for one file after a batch push completes. */
@@ -52,6 +54,8 @@ export interface BatchMoveItem {
     /** Path relative to rootPath, where the file now lives. */
     newPath: string;
     content: string | ArrayBuffer;
+    /** Revision of oldPath read during batch planning, used by GitLab's optimistic lock. */
+    oldRevision?: string;
 }
 
 export interface GitServiceInterface {

src/services/gitea-service.ts

@@ -1,6 +1,5 @@
 import { GitServiceInterface, GitTreeEntry, BatchPushItem, BatchPushResult, BatchMoveItem } from './git-service-interface';
 import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE, BLOB_CREATE_CONCURRENCY } from './git-service-base';
-import { logger } from '../utils/logger';
 
 export class GiteaService extends BaseGitService implements GitServiceInterface {
     private baseUrl: string = '';
@@ -156,9 +155,7 @@ export class GiteaService extends BaseGitService implements GitServiceInterface
         const treeResponse = await this.safeRequest(treeUrl, 'GET');
         const treeData = this.parseJson<GitHubTreeResponse>(treeResponse);
 
-        if (treeData.truncated) {
-            logger.warn('Gitea tree result is truncated. Some files might not be shown.');
-        }
+        if (treeData.truncated) throw new Error(`Gitea tree for branch "${branch}" is truncated; sync stopped to avoid treating an incomplete remote tree as a snapshot.`);
 
         const entries = treeData.tree
             .filter(item => item.type === 'blob')

src/services/github-service.ts

@@ -1,6 +1,5 @@
 import { GitServiceInterface, GitTreeEntry, BatchPushItem, BatchPushResult, BatchMoveItem } from './git-service-interface';
 import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE, BLOB_CREATE_CONCURRENCY } from './git-service-base';
-import { logger } from '../utils/logger';
 import { PushTimingCollector, PushTimingHandler, PushTimingRecord } from './push-timing';
 
 /**
@@ -104,6 +103,10 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
     }
 
     async pushFile(path: string, content: string | ArrayBuffer, branch: string, message: string, _existingSha?: string, _revision?: string): Promise<{ path: string, sha?: string }> {
+        const entry = (await this.listFilesDetailed(branch, false)).find(item => item.path === this.getFullPath(path));
+        if (entry?.symlink) {
+            throw new Error(`Cannot overwrite symlink "${path}" with a regular file.`);
+        }
         const [result] = await this.pushBatch([{ path, content }], branch, message);
         return result ?? { path };
     }
@@ -309,9 +312,7 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
             throw this.branchNotFoundError(e, branch);
         }
 
-        if (data.truncated) {
-            logger.warn('GitHub tree result is truncated. Some files might not be shown.');
-        }
+        if (data.truncated) throw new Error(`GitHub tree for branch "${branch}" is truncated; sync stopped to avoid treating an incomplete remote tree as a snapshot.`);
 
         const entries = data.tree
             .filter(item => item.type === 'blob')

src/services/gitlab-service.ts

@@ -1,5 +1,6 @@
 import { GitServiceInterface, GitTreeEntry, BatchPushItem, BatchPushResult, BatchMoveItem } from './git-service-interface';
 import { BaseGitService, ConnectionTestResult, GitFile, GitLabFileResponse, GitLabTreeItem, GIT_SYMLINK_MODE } from './git-service-base';
+import { isBinaryPath } from '../utils/path';
 
 export class GitLabService extends BaseGitService implements GitServiceInterface {
     private baseUrl: string = 'https://gitlab.com';
@@ -61,12 +62,13 @@ export class GitLabService extends BaseGitService implements GitServiceInterface
         const encodedProjectId = encodeURIComponent(this.projectId);
         const url = `${this.baseUrl}/api/v4/projects/${encodedProjectId}/repository/commits`;
 
-        const actions = items.map(item => ({
+        const actions = await Promise.all(items.map(async item => ({
             action: item.existedRemotely ? 'update' : 'create',
             file_path: this.getFullPath(item.path),
             content: this.encodeContent(item.content),
             encoding: 'base64',
-        }));
+            ...(item.existedRemotely && item.revision ? { last_commit_id: item.revision } : {}),
+        })));
 
         await this.safeRequest(url, 'POST', { branch, commit_message: message, actions });
 
@@ -89,19 +91,21 @@ export class GitLabService extends BaseGitService implements GitServiceInterface
         const url = `${this.baseUrl}/api/v4/projects/${encodedProjectId}/repository/commits`;
 
         const actions = [
-            ...additions.map(item => ({
+            ...await Promise.all(additions.map(async item => ({
                 action: item.existedRemotely ? 'update' : 'create',
                 file_path: this.getFullPath(item.path),
                 content: this.encodeContent(item.content),
                 encoding: 'base64',
-            })),
-            ...moves.map(item => ({
+                ...(item.existedRemotely && item.revision ? { last_commit_id: item.revision } : {}),
+            }))),
+            ...await Promise.all(moves.map(async item => ({
                 action: 'move',
                 file_path: this.getFullPath(item.newPath),
                 previous_path: this.getFullPath(item.oldPath),
                 content: this.encodeContent(item.content),
                 encoding: 'base64',
-            })),
+                ...(item.oldRevision ? { last_commit_id: item.oldRevision } : {}),
+            }))),
         ];
 
         await this.safeRequest(url, 'POST', { branch, commit_message: message, actions });
@@ -160,7 +164,7 @@ export class GitLabService extends BaseGitService implements GitServiceInterface
         const encodedProjectId = encodeURIComponent(this.projectId);
         const url = `${this.baseUrl}/api/v4/projects/${encodedProjectId}/repository/blobs/${sha}/raw`;
         const response = await this.safeRequest(url, 'GET');
-        const content = this.isBinary(path) ? response.arrayBuffer : response.text;
+        const content = isBinaryPath(path) ? response.arrayBuffer : response.text;
         return { content, sha };
     }
 
@@ -179,7 +183,11 @@ export class GitLabService extends BaseGitService implements GitServiceInterface
         const encodedProjectId = encodeURIComponent(this.projectId);
         const url = `${this.baseUrl}/api/v4/projects/${encodedProjectId}/repository/commits`;
 
-        const actions = paths.map(path => ({ action: 'delete', file_path: this.getFullPath(path) }));
+        const actions = await Promise.all(paths.map(async path => ({
+            action: 'delete',
+            file_path: this.getFullPath(path),
+            last_commit_id: (await this.getFile(path, branch)).revision,
+        })));
 
         await this.safeRequest(url, 'POST', { branch, commit_message: message, actions });
     }

tests/logic/sync-manager-batch.test.ts

@@ -148,6 +148,27 @@ describe('SyncManager Batch Operations', () => {
             ]);
         });
 
+        it('reads and forwards GitLab revision for an existing batch update', async () => {
+            const path = 'locked.md';
+            const adapter = mockApp.vault.adapter as Mocked<DataAdapter>;
+            mockSettings.serviceType = 'gitlab';
+            mockSettings.syncMetadata = {
+                [path]: { lastSyncedSha: 'remote-blob', lastSyncedAt: 0, lastKnownPath: path }
+            };
+            vi.mocked(adapter.exists).mockResolvedValue(true);
+            vi.mocked(adapter.read).mockResolvedValue('local edit');
+            vi.mocked(mockGitService.listFilesDetailed).mockResolvedValue([{ path, symlink: false, sha: 'remote-blob' }]);
+            vi.mocked(mockGitService.getFile).mockResolvedValue({ content: 'remote original', sha: 'remote-blob', revision: 'remote-commit' });
+            mockGitService.pushBatch = vi.fn().mockResolvedValue([{ path, sha: 'new-blob' }]);
+
+            const results = await manager.pushAllFiles([path]);
+
+            expect(results.success).toBe(1);
+            expect(mockGitService.pushBatch).toHaveBeenCalledWith([
+                { path, content: 'local edit', existedRemotely: true, revision: 'remote-commit' },
+            ], 'main', expect.any(String));
+        });
+
         it('reports syncedPaths via the sequential fallback when the provider has no pushBatch', async () => {
             const files = ['a.md', 'b.md'];
             const adapter = mockApp.vault.adapter as Mocked<DataAdapter>;
@@ -319,6 +340,25 @@ describe('SyncManager Batch Operations', () => {
             expect(adapter.write).not.toHaveBeenCalled();
         });
 
+        it('migrates a legacy GitLab last_commit_id baseline instead of creating a false pull conflict', async () => {
+            const path = 'legacy.md';
+            const adapter = mockApp.vault.adapter as Mocked<DataAdapter>;
+            mockSettings.serviceType = 'gitlab';
+            mockSettings.syncMetadata = {
+                [path]: { lastSyncedSha: 'legacy-last-commit', lastSyncedAt: 0, lastKnownPath: path }
+            };
+            vi.mocked(adapter.exists).mockResolvedValue(true);
+            vi.mocked(adapter.read).mockResolvedValue('local old copy');
+            vi.mocked(mockGitService.listFilesDetailed).mockResolvedValue([{ path, symlink: false, sha: 'remote-blob' }]);
+            vi.mocked(mockGitService.getFile).mockResolvedValue({ content: 'remote current copy', sha: 'remote-blob', revision: 'legacy-last-commit' });
+
+            const results = await manager.pullAllFiles([path]);
+
+            expect(results.conflicts).toBe(0);
+            expect(results.success).toBe(1);
+            expect(mockSettings.syncMetadata[path]?.lastSyncedSha).toBe('remote-blob');
+        });
+
         it('still downloads when the local file differs and the remote has not moved', async () => {
             const path = 'stale.md';
             mockSettings.syncMetadata = {