fix(sync): unify push pull and move decisions

Author: ClaudiaFang

feature_list.json

@@ -8,7 +8,7 @@
       "description": "Preserve sync behavior while extracting SyncStatusView presentation state/controller boundaries and SyncManager scanner/planner/executor/workspace boundaries with regression and integration coverage.",
       "dependencies": [],
       "status": "in-progress",
-      "evidence": "In progress on refactor/sync-domain-pipeline: View 11.5 KB and Manager facade 13.7 KB; UI/domain workspace, planner/scanner/coordinators/executors/FileDiff boundaries covered by 598 passing tests; automated gates green, desktop/mobile manual smoke pending."
+      "evidence": "In progress on refactor/sync-domain-pipeline: push, pull, single pull, and move decisions now route through SyncPlanner; View 11.5 KB and Manager facade 13.7 KB; 610 tests and automated gates green, desktop/mobile manual smoke pending."
     },
     {
       "id": "feat-004",

progress.md

@@ -4,8 +4,8 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont
 
 ## Current State
 
-**Last Updated:** 2026-08-19
-**Active Feature:** feat-026 / issue #105 — sync architecture refactor on `refactor/sync-domain-pipeline`. Automated implementation is complete: the actual View is 11.5 KB, the Manager facade is 13.7 KB, UI commands cross `SyncWorkspace`, and scanner/planner/coordinator/executor/diff boundaries have focused and integration coverage. Only real Obsidian desktop/mobile manual verification remains before declaring the feature complete.
+**Last Updated:** 2026-08-20
+**Active Feature:** feat-026 / issue #105 — sync architecture refactor on `refactor/sync-domain-pipeline`. `SyncPlanner` is now the decision source for normal push, batch pull/preview, single pull, and moves. Edited tracked renames with a free destination plan one move instead of being auto-skipped; remote-only changes pull without false conflicts; real two-sided divergence and occupied move destinations remain conflicts. Automated implementation is green; real Obsidian desktop/mobile manual verification remains before declaring the feature complete.
 **Parallel Work:** PR #87 (4x Dependabot security alerts via npm overrides) and Issue #57 (live-credential smoke test).
 
 ## Outstanding Items
@@ -17,6 +17,8 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont
 
 ## Latest Evidence
 
+- [x] Issue #105 unified sync decisions and move regression (2026-08-20): added operation-aware `SyncPlanner.planFor(push|pull)`, `MoveFacts`, and the `move` domain action. Normal push, batch pull and preview, single pull, and tracked moves now consume planner decisions instead of reimplementing SHA conflict checks. Removed `PushCoordinator.queueMove`'s stale-metadata gate, so an edited tracked rename with a free destination appears under Moves and commits once; occupied destinations remain conflicts. Fixed the complementary pull false positive: a remote-only change now pulls, while real two-sided divergence still resolves as conflict. Content-fetched text/binary paths normalize equal bytes to the provider blob SHA before planning, preserving binary and GitLab legacy-baseline behavior. Added planner operation matrix, coordinator move regression, batch pull, and single pull coverage. Verification: `npx eslint .` — 0 errors; `npm run build` — clean including Obsidian 1.11 compatibility; `npx vitest run` — 54 files / 610 tests; `git diff --check` — clean. Manual Obsidian verification remains.
+
 - [x] Issue #105 architecture implementation (2026-08-19): extracted `SyncStatusRenderer` and `SyncStatusComposition`; `SyncStatusView.ts` is 11.5 KB / 251 lines. Extracted `PullCoordinator` and `PushCoordinator`; `SyncManager.ts` is 13.7 KB / 298 lines and retains its public compatibility API. `SyncManagerWorkspace` now owns refresh/tree-snapshot reuse, push/pull, diff, local/remote deletion, move, metadata mutations, provider URLs and UI-safe workspace info; sync-status UI code no longer reaches provider/tree/settings/vault mutation helpers, and `src/logic/**` has no UI imports. Legacy refresh characterization cases now target the extracted service instead of private View delegates; legacy modal tests explicitly inject the Obsidian interaction adapter. Added real refresh integration plus focused push-coordinator/workspace regression tests. Independent verification: `npx eslint .` — 0 errors; `npm run build` — clean including Obsidian 1.11 compatibility; `npx vitest run` — 54 files / 598 tests; `npm run test:e2e -- --provider gitea` — 2 files / 14 tests with container cleanup; `git diff --check` — clean. Desktop/mobile Obsidian smoke remains manual.
 
 - [x] Issue #105 architecture slice 3 (2026-08-19): added tested `SyncDiffService` and `SyncStatusNavigator`, so lazy blob loading/cache/content-kind projection is a domain `FileDiff` boundary. Extracted single-file, batch push/pull, local/remote delete, move revert, remote-tree reuse, progress/confirmation, and optimistic-status orchestration into `SyncStatusOperations`; all View row/group events now enter through `SyncStatusController`. The actual View is about 40 KB (down from 58 KB this slice and 80 KB initially). Independent verification: `npx eslint .` — 0 errors; `npm run build` — clean including Obsidian 1.11 compatibility; `npx vitest run` — 52 files / 594 tests; `npm run test:e2e -- --provider gitea` — 2 files / 14 tests with container cleanup; `git diff --check` — clean. Feature remains in progress because renderer composition and the ~50 KB manager facade are still oversized.

session-handoff.md

@@ -1,57 +1,43 @@
 # Session Handoff
 
-**Date:** 2026-08-19
-**Branch:** `refactor/sync-domain-pipeline` based on `origin/main@6fc3d6b`
+**Date:** 2026-08-20
+**Branch:** `refactor/sync-domain-pipeline` (PR #127)
 **Active Feature:** feat-026 / issue #105 — sync architecture refactor
 
-## Current Stopping Point
+## Completed This Session
 
-The requested architecture refactor and automated regression safety net are implemented. Do not start the VS Code-like source-control redesign on this branch. The only Definition-of-Done item that cannot be executed in this environment is real Obsidian desktop/mobile manual verification.
+Fixed the reported false conflict for a tracked rename whose content was also edited, then audited
+and removed the remaining sync-decision bypasses. Move classification previously returned before
+reaching `SyncPlanner` and used a duplicate `oldEntry.sha === lastSyncedSha` safety check;
+`PushCoordinator`, `PullCoordinator`, and single pull also retained separate SHA predicates after
+the planner extraction.
 
-## Implemented Architecture
+Added operation-aware `SyncPlanner.planFor(push|pull)`, `MoveFacts`, and the `move` domain action.
+Normal push, batch pull/preview, single pull, and tracked move now consume planner decisions. A
+free destination produces one move containing current local content; an occupied destination
+remains a conflict. A remote-only change now pulls rather than being mistaken for two-sided
+divergence. Content-fetched text/binary paths normalize equal bytes to the provider blob SHA, so
+binary behavior and GitLab legacy baselines remain compatible.
 
-- `src/ui/SyncStatusView.ts` and `src/logic/sync-manager.ts` are thin compatibility entrypoints.
-- Actual `src/ui/sync-status/SyncStatusView.ts` is 11.5 KB / 251 lines and composes state, renderer, controller, navigator, operations and workspace.
-- `SyncStatusViewState` owns presentation state; `SyncStatusSelectors` are pure and table-tested; `SyncStatusRenderer` owns list/tree/group/header rendering.
-- Every row/group/action command enters `SyncStatusController`; `SyncStatusOperations` performs UI notifications/confirmation and calls `SyncWorkspace` for mutations.
-- `SyncManagerWorkspace` owns refresh, remote-tree snapshot validation/reuse, push/pull, `FileDiff`, local/remote deletion, move, metadata mutations, provider URLs and UI-safe workspace info. Sync-status UI code no longer imports provider/tree/settings or vault mutation helpers.
-- `SyncStatusRefreshService` owns discovery, hidden files, path mapping, status classification, out-of-band move reconciliation and live modify/rename transitions.
-- Actual `src/logic/sync/SyncManager.ts` is 13.7 KB / 298 lines, preserves the historical public API, and delegates batch use cases to `PushCoordinator` and `PullCoordinator`.
-- Historical `src/logic/sync-manager.ts` is now a pure domain re-export. `src/logic/**` has no UI imports; production and modal characterization tests inject `ObsidianSyncInteraction` at the composition boundary.
-- Domain components include `SyncScanner`, pure `SyncPlanner`, `SyncMetadataStore`, `PushExecutor`, `PullExecutor`, `RemoteDeleteExecutor`, `ConflictResolver`, `SyncExecutor`, `SyncDiffService`, and the injected `SyncInteractionPort`.
-- `DiffView` consumes only `FileDiff`.
-- New tests cover state/selectors/controller, planner matrix, scanner/metadata, every executor, diff, push coordinator, workspace snapshot behavior, and real Scanner/Manager/Workspace integration paths.
+The clean-code skill drove the TDD sequence: the coordinator regression failed first with the
+file under `skippedConflicts`, then passed after the planner integration. No commit or push was
+performed. The pre-existing untracked `.codex-gitlab.env` remains untouched.
 
 ## Verification Evidence
 
 ```text
-npx eslint .                 -> PASS, 0 errors
-npm run build                -> PASS, incl. Obsidian 1.11 compatibility
-npx vitest run               -> PASS, 54 files / 598 tests
-git diff --check             -> PASS
-npm run test:e2e -- --provider gitea -> PASS, 2 files / 14 tests; container removed
+npx eslint .      -> PASS, 0 errors
+npm run build     -> PASS, incl. Obsidian 1.11 compatibility
+npx vitest run    -> PASS, 54 files / 610 tests
+git diff --check  -> PASS
 ```
 
-The independent verifier used the closest available low-tier model because the AGENTS-required Haiku model is unavailable.
+The AGENTS-required Haiku verifier was unavailable in this environment, so verification ran
+locally in this session.
 
-## Required Manual Smoke Before Marking Complete
+## Exact Next Step
 
-Desktop:
-
-1. Open sync view → refresh → local-only file → Push → refresh → synced.
-2. Remote-only → Pull → local file created → synced.
-3. Modified → Diff → Push/Pull.
-4. Conflict → resolve → refresh.
-
-Mobile:
-
-1. Open sync view → refresh → select one file → Diff → Push.
-2. Remote-only → select file → Pull.
-
-After manual confirmation, mark feat-026 complete and archive its active progress entry. Then branch `feat/vscode-source-control-ui` from the updated `main`.
-
-## Workspace Safety
-
-- `.codex-gitlab.env` is untracked and must remain untouched/uncommitted.
-- Prior detached tracked changes remain preserved in `stash@{0}` with message `pre-refactor preserved tracked changes from detached 1.5.6 checkout`.
-- No commit or push has been made for this refactor workspace.
+Manually verify in Obsidian that moving and editing a tracked file shows it under Moves and that
+Apply creates the new path with edited content while removing the old path. Also verify an
+existing remote destination still appears as a skipped conflict. After the remaining desktop and
+mobile smoke paths pass, feat-026 can be marked complete.

src/logic/sync/PullCoordinator.ts

@@ -7,7 +7,7 @@ import { contentsEqual, isBinaryPath } from '../../utils/path';
 import type { PullExecutor } from './PullExecutor';
 import type { SyncScanner } from './SyncScanner';
 import { SyncPlanner } from './SyncPlanner';
-import type { SyncPlan, SyncPlanEntry, SyncResult } from './types';
+import type { PlannedFileAction, SyncPlan, SyncPlanEntry, SyncResult } from './types';
 import { isSyncPlanEmpty } from './types';
 
 type BatchOutcome = 'done' | 'unchanged' | 'conflict';
@@ -106,13 +106,9 @@ export class PullCoordinator {
         const repoPath = this.dependencies.scanner.toRepoPath(path);
         const entry = tree?.get(this.dependencies.scanner.toTreePath(repoPath));
         if (tree && !entry) return 'skip';
-        if (!await this.fileExists(file)) return 'addition';
         if (!entry?.sha || entry.symlink) return 'modification';
-        const localSha = await gitBlobSha(await this.dependencies.scanner.readContent(file));
-        if (localSha === entry.sha) return 'unchanged';
-        await this.dependencies.migrateBaseline(path, repoPath, entry);
-        const baseline = this.dependencies.settings.syncMetadata[path];
-        return baseline && entry.sha !== baseline.lastSyncedSha ? 'conflict' : 'modification';
+        const decision = await this.planFromTree(file, path, isString, entry);
+        return this.planKindFor(decision);
     }
 
     private async processFile(
@@ -131,15 +127,12 @@ export class PullCoordinator {
         }
         const remote = await this.dependencies.gitService().getFile(repoPath, this.dependencies.settings.branch);
         if (!remote.sha) throw new Error('File not found in remote');
-        if (await this.fileExists(file)) {
-            const localContent = await this.dependencies.scanner.readContent(file);
-            if (contentsEqual(localContent, remote.content)) {
-                await this.dependencies.updateMetadata(path, remote.sha);
-                return 'unchanged';
-            }
-            const baseline = this.dependencies.settings.syncMetadata[path];
-            if (baseline && !this.sameBaseline(baseline.lastSyncedSha, remote)) return 'conflict';
+        const decision = await this.planFromRemote(file, path, isString, remote);
+        if (decision.action === 'none') {
+            await this.dependencies.updateMetadata(path, remote.sha);
+            return 'unchanged';
         }
+        if (decision.action === 'resolve-conflict') return 'conflict';
         const target = typeof file === 'string' ? { path, name } : file;
         await this.dependencies.executor.pull(target, remote.content, remote.sha, true, this.symlinkTarget(remote));
         return 'done';
@@ -151,27 +144,61 @@ export class PullCoordinator {
         isString: boolean,
         entry: GitTreeEntry,
     ): Promise<BatchOutcome | null> {
-        if (entry.symlink || !entry.sha || !await this.fileExists(file)) return null;
-        const localSha = await gitBlobSha(await this.dependencies.scanner.readContent(file));
-        const baseline = this.dependencies.settings.syncMetadata[path];
-        const classification = this.planner.classify({
-            local: { path, exists: true, blobSha: localSha, kind: isBinaryPath(path) ? 'binary' : 'text' },
-            remote: {
-                path,
-                repoPath: this.dependencies.scanner.toRepoPath(path),
-                exists: true,
-                blobSha: entry.sha,
-                kind: 'text',
-            },
-            base: { blobSha: baseline?.lastSyncedSha },
-        });
-        if (classification === 'synced') {
+        if (entry.symlink || !entry.sha) return null;
+        const decision = await this.planFromTree(file, path, isString, entry);
+        if (decision.action === 'none') {
             await this.dependencies.updateMetadata(path, entry.sha);
             return 'unchanged';
         }
-        await this.dependencies.migrateBaseline(path, this.dependencies.scanner.toRepoPath(path), entry);
-        const migratedBaseline = this.dependencies.settings.syncMetadata[path];
-        return migratedBaseline && entry.sha !== migratedBaseline.lastSyncedSha ? 'conflict' : null;
+        return decision.action === 'resolve-conflict' ? 'conflict' : null;
+    }
+
+    private async planFromTree(
+        file: TFile | string,
+        path: string,
+        isString: boolean,
+        entry: GitTreeEntry,
+    ): Promise<PlannedFileAction> {
+        const repoPath = this.dependencies.scanner.toRepoPath(path);
+        await this.dependencies.migrateBaseline(path, repoPath, entry);
+        const exists = await this.fileExists(file);
+        const kind = isBinaryPath(path) ? 'binary' : 'text';
+        const localSha = exists ? await gitBlobSha(await this.dependencies.scanner.readContent(file)) : undefined;
+        return this.planner.planFor('pull', {
+            local: { path, exists, blobSha: localSha, kind },
+            remote: { path, repoPath, exists: true, blobSha: entry.sha, kind },
+            base: { blobSha: this.dependencies.settings.syncMetadata[path]?.lastSyncedSha },
+        });
+    }
+
+    private async planFromRemote(
+        file: TFile | string,
+        path: string,
+        isString: boolean,
+        remote: GitFile,
+    ): Promise<PlannedFileAction> {
+        const exists = await this.fileExists(file);
+        const kind = isBinaryPath(path) ? 'binary' : 'text';
+        const localContent = exists ? await this.dependencies.scanner.readContent(file) : undefined;
+        let localSha: string | undefined;
+        if (localContent !== undefined) {
+            localSha = contentsEqual(localContent, remote.content) ? remote.sha : await gitBlobSha(localContent);
+        }
+        const baseline = this.dependencies.settings.syncMetadata[path]?.lastSyncedSha;
+        const blobBaseline = baseline === remote.revision ? remote.sha : baseline;
+        return this.planner.planFor('pull', {
+            local: { path, exists, blobSha: localSha, kind },
+            remote: { path, repoPath: this.dependencies.scanner.toRepoPath(path), exists: true, blobSha: remote.sha, kind },
+            base: { blobSha: blobBaseline },
+        });
+    }
+
+    private planKindFor(decision: PlannedFileAction): PlanKind {
+        if (decision.action === 'pull-create') return 'addition';
+        if (decision.action === 'pull-overwrite') return 'modification';
+        if (decision.action === 'resolve-conflict') return 'conflict';
+        if (decision.action === 'none') return 'unchanged';
+        return 'skip';
     }
 
     private fileExists(file: TFile | string): Promise<boolean> | boolean {
@@ -190,10 +217,6 @@ export class PullCoordinator {
         return remote.isSymlink ? remote.symlinkTarget ?? '' : undefined;
     }
 
-    private sameBaseline(sha: string, remote: GitFile): boolean {
-        return sha === remote.sha || sha === remote.revision;
-    }
-
     private notifyResult(result: SyncResult): void {
         if (result.success > 0) this.dependencies.notify(`Pulled ${result.success} file(s) to ${this.dependencies.serviceName()}`);
         if (result.conflicts > 0) {