refactor

Author: ErcinDedeoglu

docs/TODO.md

@@ -19,6 +19,33 @@
 - [x] Tombstones for deletion tracking
 - [x] Per-file granularity (not repo-level overwrite)
 - [x] Pull-then-push flow (when remote is newer, pull first then push local changes)
+- [x] Consolidate logger files - unified logging system with ports & adapters pattern
+
+## In Progress: Code Quality & Refactoring
+
+### High Priority
+- [x] **Consolidate encoding utilities** - `packer.ts` & `item-packer.ts` have identical `calculateChecksum()`, `uint8ArrayToBase64()`, `base64ToUint8Array()`
+  - Created `src/shared/encoding-utils.ts` with shared encoding functions
+- [x] **Extract error handling pattern** - pattern `error instanceof Error ? error.message : String(error)` repeated in 5+ files
+  - Created `src/shared/error-utils.ts` with `getErrorMessage()`, `getErrorStack()`, `toError()`, `isError()`
+- [x] **Consolidate sync result processing** - `background-sync.ts` & `sync-handler.ts` have duplicate write/persist logic
+  - Created `src/shared/sync-result-handler.ts` with `writePulledData()`, `persistLocalState()`, `processSyncResult()`
+
+### Medium Priority
+- [x] **Unify error classes** - 6 error classes (`RepoApiError`, `SyncError`, `EncryptionError`, `PackerError`, `ItemPackerError`, `MergeError`) unified
+  - Created `src/shared/errors.ts` with `AppError` base class and all domain error classes
+- [x] **Centralize retry configuration** - retry constants scattered across `fetch.ts`, `constants.ts`, `retry.ts`
+  - Created `src/shared/retry-config.ts` with `CONFLICT_RETRY` and `API_RETRY` config objects, plus `calculateBackoff()` and `sleep()` utilities
+- [x] **Extract plugin state validation** - `if (!state.config || !state.engine)` pattern in 3+ files
+  - Created `src/plugin/validation.ts` with type guards: `hasConfig()`, `isPluginReady()`, `isContinuousSyncReady()`, `isFullyInitialized()`
+- [x] **Move CryptoOptions type** - currently in `operations/types.ts`, should be in `src/types/crypto.ts`
+  - Created `src/types/crypto.ts` with `CryptoOptions` and `PassphraseOption` types, re-exported from `operations/types.ts` for backward compatibility
+
+### Low Priority
+- [x] **Review helper duplication** - `engine/helpers.ts` vs `operations/helpers.ts` may have overlapping functionality
+  - Reviewed: No duplication found. `engine/helpers.ts` handles orchestration (checksums, pull/push options), `operations/helpers.ts` handles low-level operations (tombstones, manifest, context). Complementary, not duplicative.
+- [x] **Consider API client base class** - `http-client.ts` and `graphql-client.ts` have similar initialization patterns
+  - Reviewed: Not worth abstracting. Only ~15 lines similar (config fields). Different APIs (REST vs GraphQL), different methods, different URL patterns. Base class would add complexity without benefit.
 
 ## In Progress: Safety & Observability
 
@@ -40,21 +67,3 @@
 - [ ] Compression improvements for large messages
 - [ ] Parallel category processing during sync
 
-## Design Notes
-
-### Timestamp-Based Sync Trade-offs (Research-Backed)
-
-- **Clock skew risk**: NTP drift 1-50ms (cloud), 100-500ms (geo-distributed)
-  - Source: "Clock Skew Conflict in Distributed Systems" (2026)
-  - Mitigation: Modern OS auto-sync via NTP (atomic clock: 1s in 3M years)
-  - Impact: Very low - machines rarely drift years apart
-  - Real-world: CockroachDB uses 500ms max offset tolerance successfully
-- **Concurrent edit data loss**: Last-write-wins on same file from 2 machines
-  - Source: "Beyond Timestamps: Conflict Resolution" (2025)
-  - Likelihood: Very low for CLI tool (one session at a time)
-  - Impact: One edit lost, logged for manual recovery
-  - Mitigation: Sessions/messages are mostly append-only (different files)
-- **Config overwrites**: Settings changes on both machines = last wins
-  - Mitigation: Show warning, allow manual merge via flags
-  - Frequency: Low - settings changed infrequently
-  - Precedent: VS Code Settings Sync (4M+ users) uses simple last-write-wins

src/crypto/encrypt.ts

@@ -7,6 +7,7 @@
 
 import { createCipheriv, createDecipheriv, pbkdf2Sync, randomBytes, createHash } from 'node:crypto';
 import type { EncryptedData } from '../types/index.js';
+import { EncryptionError } from '../shared/index.js';
 
 /** PBKDF2 iterations for key derivation */
 const PBKDF2_ITERATIONS = 100000;
@@ -124,12 +125,5 @@ export function isEncryptedData(data: unknown): data is EncryptedData {
   );
 }
 
-/**
- * Error thrown when encryption/decryption fails.
- */
-export class EncryptionError extends Error {
-  constructor(message: string) {
-    super(message);
-    this.name = 'EncryptionError';
-  }
-}
+// Re-export for backward compatibility
+export { EncryptionError } from '../shared/index.js';

src/plugin/background-sync.ts

@@ -5,56 +5,24 @@
 import type { PathConfig } from '../types/paths.js';
 import type { SyncCategory } from '../types/index.js';
 import type { SyncResult } from '../types/sync.js';
-import type { CategoryData } from '../sync/operations/types.js';
-import {
-  loadLocalData,
-  saveLocalState,
-  writeLocalData,
-  deleteTombstonedItems,
-} from '../data/index.js';
+import { loadLocalData } from '../data/index.js';
 import { FileWatcher } from '../sync/watcher/index.js';
-import { syncLog, log } from '../logging/index.js';
+import { log } from '../logging/index.js';
 import { getPluginState } from './state-manager.js';
+import { isContinuousSyncReady } from './validation.js';
+import { getErrorMessage, writePulledData, persistLocalState } from '../shared/index.js';
 
 /** Active file watcher instance */
 let activeWatcher: FileWatcher | null = null;
 
 /** Active interval timer for periodic sync */
 let syncInterval: NodeJS.Timeout | null = null;
 
-/** Write pulled data to disk after a successful pull/merge */
-export async function writePulledData(pathConfig: PathConfig, result: SyncResult): Promise<void> {
-  if (result.action !== 'pulled' && result.action !== 'merged') {
-    return;
-  }
-
-  if (result.pulledData) {
-    const data = result.pulledData as CategoryData[];
-    syncLog(`[WRITE] Writing ${String(data.length)} categories to disk`);
-    await writeLocalData(pathConfig, data);
-    syncLog(`[WRITE] Finished writing to disk`);
-  }
-
-  if (result.tombstonedItems) {
-    await deleteTombstonedItems(pathConfig, result.tombstonedItems);
-  }
-}
-
-/** Persist engine's local state to disk after successful sync */
-export async function persistLocalState(pathConfig: PathConfig): Promise<void> {
-  const state = getPluginState();
-  const newState = state.engine?.getLocalState();
-  if (newState) {
-    await saveLocalState(pathConfig, newState);
-    state.localState = newState;
-  }
-}
-
 /** Start file watcher for continuous sync */
 export function startFileWatcher(pathConfig: PathConfig): void {
   const state = getPluginState();
 
-  if (!state.config?.continuousSync || !state.engine) {
+  if (!isContinuousSyncReady(state)) {
     return;
   }
 
@@ -81,24 +49,22 @@ export function startFileWatcher(pathConfig: PathConfig): void {
             await persistLocalState(pathConfig);
           }
         } catch (error) {
-          const errMsg = error instanceof Error ? error.message : String(error);
-          log(`WARNING: File watcher sync failed: ${errMsg}`);
+          log(`WARNING: File watcher sync failed: ${getErrorMessage(error)}`);
         }
       },
     });
     void activeWatcher.start();
     log('File watcher started');
   } catch (error) {
-    const errMsg = error instanceof Error ? error.message : String(error);
-    log(`WARNING: Failed to start file watcher: ${errMsg}`);
+    log(`WARNING: Failed to start file watcher: ${getErrorMessage(error)}`);
   }
 }
 
 /** Start interval-based sync */
 export function startIntervalSync(pathConfig: PathConfig): void {
   const state = getPluginState();
 
-  if (!state.config?.continuousSync || !state.engine) {
+  if (!isContinuousSyncReady(state)) {
     return;
   }
 
@@ -117,8 +83,7 @@ export function startIntervalSync(pathConfig: PathConfig): void {
         }
         logIntervalResult(result);
       } catch (error) {
-        const errMsg = error instanceof Error ? error.message : String(error);
-        log(`WARNING: Interval sync failed: ${errMsg}`);
+        log(`WARNING: Interval sync failed: ${getErrorMessage(error)}`);
       }
     })();
   }, intervalMs);
@@ -151,3 +116,6 @@ export function stopBackgroundSync(): void {
     log('Interval sync stopped');
   }
 }
+
+// Re-export for backward compatibility
+export { writePulledData, persistLocalState } from '../shared/index.js';

src/plugin/plugin.ts

@@ -14,6 +14,7 @@ import {
   updateConfig,
   initializeEngine,
 } from './state-manager.js';
+import { hasConfig, isFullyInitialized } from './validation.js';
 import { getTokenSource, loadLocalData } from '../data/index.js';
 import { RepoStorageBackend } from '../storage/index.js';
 import type { PluginState } from './types.js';
@@ -26,13 +27,14 @@ import {
   startIntervalSync,
   stopBackgroundSync,
 } from './background-sync.js';
+import { getErrorMessage, getErrorStack } from '../shared/index.js';
 
 /** Default repo name for sync storage */
 const DEFAULT_REPO_NAME = '.opencode-sync';
 
 /** Validate configuration and log status. Returns true if valid. */
 function validateAndLogConfig(state: PluginState): boolean {
-  if (!state.config) {
+  if (!hasConfig(state)) {
     log('WARNING: No configuration found');
     logSetupInstructions();
     log('Plugin loaded but sync is disabled');
@@ -102,8 +104,7 @@ async function ensureStorageExists(pathConfig: PathConfig): Promise<void> {
     await updateConfig(pathConfig, { repoOwner: owner, repoName });
     initializeEngine();
   } catch (error) {
-    const errMsg = error instanceof Error ? error.message : String(error);
-    log(`ERROR: Failed to setup storage: ${errMsg}`);
+    log(`ERROR: Failed to setup storage: ${getErrorMessage(error)}`);
     throw error;
   }
 }
@@ -112,12 +113,12 @@ async function ensureStorageExists(pathConfig: PathConfig): Promise<void> {
 function performInitialSync(pathConfig: PathConfig): void {
   const state = getPluginState();
 
-  if (!state.isInitialized || !state.engine) {
+  if (!isFullyInitialized(state)) {
     log('Skipping initial sync - engine not initialized');
     return;
   }
 
-  if (!state.config?.autoSyncOnStartup) {
+  if (!state.config.autoSyncOnStartup) {
     log('Auto-sync on startup disabled');
     return;
   }
@@ -144,9 +145,9 @@ function performInitialSync(pathConfig: PathConfig): void {
         log(`Sync completed in ${String(dur)}ms: ${result.message}`);
       }
     } catch (error) {
-      const errMsg = error instanceof Error ? error.message : String(error);
-      log(`WARNING: Initial sync failed: ${errMsg}`);
-      if (error instanceof Error && error.stack) log(`Stack: ${error.stack}`);
+      log(`WARNING: Initial sync failed: ${getErrorMessage(error)}`);
+      const stack = getErrorStack(error);
+      if (stack) log(`Stack: ${stack}`);
     }
   })();
 }
@@ -169,9 +170,8 @@ export const OpencodeSyncPlugin = async (_ctx: unknown): Promise<Record<string,
 
     return { cleanup: stopBackgroundSync };
   } catch (error) {
-    const errMsg = error instanceof Error ? error.message : String(error);
-    const stack = error instanceof Error ? error.stack : undefined;
-    log(`ERROR: Plugin initialization failed: ${errMsg}`);
+    log(`ERROR: Plugin initialization failed: ${getErrorMessage(error)}`);
+    const stack = getErrorStack(error);
     if (stack) {
       console.error(stack);
     }

src/plugin/state-manager.ts

@@ -15,6 +15,7 @@ import { createFileWatcher } from '../sync/watcher/index.js';
 import { loadConfig, saveConfig, loadLocalState, generateMachineId } from '../data/index.js';
 import type { PluginState } from './types.js';
 import { createInitialState } from './types.js';
+import { hasConfig } from './validation.js';
 
 const state: PluginState = createInitialState();
 
@@ -38,7 +39,7 @@ export function getPluginState(): PluginState {
  * Initialize the sync engine with current configuration.
  */
 export function initializeEngine(): void {
-  if (!state.config) return;
+  if (!hasConfig(state)) return;
   if (!state.config.repoOwner || !state.config.repoName) return;
 
   const backendConfig: RepoClientConfig = {
@@ -86,7 +87,7 @@ export async function updateConfig(
     return;
   }
 
-  if (!state.config) {
+  if (!hasConfig(state)) {
     state.config = {
       ...DEFAULT_CONFIG,
       token: updates.token ?? '',
@@ -106,7 +107,7 @@ export async function updateConfig(
  */
 export function setPassphrase(passphrase: string): void {
   state.passphrase = passphrase;
-  if (state.config) {
+  if (hasConfig(state)) {
     initializeEngine();
   }
 }
@@ -118,7 +119,7 @@ export async function startWatcher(
   pathConfig: PathConfig,
   onSync: () => Promise<void>
 ): Promise<void> {
-  if (!state.config?.continuousSync) return;
+  if (!hasConfig(state) || !state.config.continuousSync) return;
 
   state.watcher = createFileWatcher(
     pathConfig,

src/plugin/sync-handler.ts

@@ -6,15 +6,10 @@
 
 import type { PathConfig } from '../types/paths.js';
 import type { SyncResult } from '../types/index.js';
-import type { CategoryData } from '../sync/operations/types.js';
-import {
-  loadLocalData,
-  saveLocalState,
-  writeLocalData,
-  deleteTombstonedItems,
-} from '../data/index.js';
+import { loadLocalData } from '../data/index.js';
 import { getPluginState } from './state-manager.js';
-import { syncLog } from '../logging/index.js';
+import { isPluginReady } from './validation.js';
+import { getErrorMessage, writePulledData, persistLocalState } from '../shared/index.js';
 
 type LogLevel = 'error' | 'info' | 'debug' | 'warn';
 
@@ -32,7 +27,7 @@ interface LogClient {
 export async function performSync(pathConfig: PathConfig, client: LogClient): Promise<void> {
   const state = getPluginState();
 
-  if (!state.config || !state.engine) {
+  if (!isPluginReady(state)) {
     return;
   }
 
@@ -46,7 +41,7 @@ export async function performSync(pathConfig: PathConfig, client: LogClient): Pr
       await logSyncError(client, result.message);
     }
   } catch (error) {
-    await logSyncError(client, error instanceof Error ? error.message : 'Unknown');
+    await logSyncError(client, getErrorMessage(error));
   }
 }
 
@@ -58,32 +53,9 @@ async function handleSyncSuccess(
   client: LogClient,
   result: SyncResult
 ): Promise<void> {
-  const state = getPluginState();
-  const newState = state.engine?.getLocalState();
-
-  // Write pulled data to local filesystem
-  // This includes data from pull, merge, AND remote items fetched during push
-  if (result.action === 'pulled' || result.action === 'merged' || result.action === 'pushed') {
-    syncLog(
-      `[WRITE] Action: ${result.action}, pulledData: ${result.pulledData ? 'present' : 'missing'}`
-    );
-    if (result.pulledData) {
-      const data = result.pulledData as CategoryData[];
-      syncLog(`[WRITE] Writing ${String(data.length)} categories to disk`);
-      await writeLocalData(pathConfig, data);
-      syncLog(`[WRITE] Finished writing to disk`);
-    }
-    if (result.tombstonedItems) {
-      await deleteTombstonedItems(pathConfig, result.tombstonedItems);
-    }
-  } else {
-    syncLog(`[WRITE] Skipping write - action: ${result.action}`);
-  }
-
-  if (newState) {
-    await saveLocalState(pathConfig, newState);
-    state.localState = newState;
-  }
+  // Write pulled data and persist state using shared utilities
+  await writePulledData(pathConfig, result);
+  await persistLocalState(pathConfig);
 
   await client.app.log({
     body: {